Re: Password identifiers, protocol aging and SCRAM protocol
От | Michael Paquier |
---|---|
Тема | Re: Password identifiers, protocol aging and SCRAM protocol |
Дата | |
Msg-id | CAB7nPqS99Z31f7jhoYYMoBDbuZSQRpn+HQzByA=EwfMDYwCk1Q@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Password identifiers, protocol aging and SCRAM protocol (Peter Eisentraut <peter.eisentraut@2ndquadrant.com>) |
Список | pgsql-hackers |
On Sat, Nov 5, 2016 at 12:58 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > The organization of these patches makes sense to me. > > On 10/20/16 1:14 AM, Michael Paquier wrote: >> - 0001, moving all the SHA2 functions to src/common/ and introducing a >> PG-like interface. No actual changes here. > > That's probably alright, although the patch contains a lot more changes > than I would imagine for a simple file move. I'll still have to review > that in detail. The main point is to know if people are happy of having an interface of the type pg_sha256_[init|update|finish] to tackle the fact that core code contains a set of routines that map with some of the OpenSSL APIs... >> - 0002, replacing PostmasterRandom by pg_strong_random(), with a fix >> for the cancel key problem. >> - 0003, adding for pg_strong_random() a fallback for any nix platform >> not having /dev/random. This should be grouped with 0002, but I split >> it for clarity. > > Also makes sense, but will need more detailed review. I did not follow > the previous PostmasterRandom issues closely. pademelon does not have /dev/random and /dev/urandom, so the issue is related to having a fallback method... But Heikki feels that having a method producing potentially weak keys should not be in pg_strong_random(). I'd suggest to control that with a ./configure switch and call it a day. Platforms without any of the four randomness methods pg_strong_random includes play a dangerous game but... >> - 0004, Add encoding routines for base64 without whitespace in >> src/common/. I improved the error handling here by making them return >> -1 in case of error and let the caller handle the error. > > I don't think we want to have two different copies of base64 routines. > Surely we can make the existing routines do what we want with a > parameter or two about whitespace and line length. We could. Though after hacking on that I find cleaner copying the code from encoding.c after removing the whitespace handling, as Heikki has suggested. >> - 0005, Refactor decision-making of password encryption into a single routine. > > It makes sense to factor this out. We probably don't need the pstrdup > if we just keep the string as is. (You could make an argument for it if > the input values were const char *.) We probably also don't need the > pfree. The Assert(0) can probably be done better. We usually use > elog() in such cases. Hm, OK. Agreed with that. >> - 0006, Add clause PASSWORD val USING protocol to CREATE/ALTER ROLE. > > "protocol" is a weird choice here. Maybe something like "method" is > better. The way the USING clause is placed can be confusing. It's not > clear that it belongs to PASSWORD. If someone wants to augment another > clause in CREATE ROLE with a secondary argument, then it could get > really confusing. I'd suggest something to group things together, like > PASSWORD (val USING method). The method could be an identifier instead > of a string. Why not. > Please add an example to the documentation and explain better how this > interacts with the existing ENCRYPTED PASSWORD clause. Sure. >> - 0007, the SCRAM implementation. > > No documentation about pg_hba.conf changes, so I don't know how to use > this. ;-) Oops. I have focused on the code a lot during last rewrite of the patch and forgot that. I'll think about something. > This implements SASL and SCRAM and SHA256. We need to be clear about > which term we advertise to users. An explanation in the missing > documentation would probably be a good start. pg_hba.conf uses "scram" as keyword, but scram refers to a family of authentication methods. There is as well SCRAM-SHA-1, SCRAM-SHA-256 (what this patch does). Hence wouldn't it make sense to use scram_sha256 in pg_hba.conf instead? If for example in the future there is a SHA-512 version of SCRAM we could switch easily to that and define scram_sha512. There is also the channel binding to think about... So we could have a list of keywords perhaps associated with SASL? Imagine for example: sasl $algo,$channel_binding Giving potentially: sasl scram_sha256 sasl scram_sha256,channel sasl scram_sha512 sasl scram_sha512,channel In the case of the patch of this thread just the first entry would make sense, once channel binding support is added a second keyword/option could be added. And there are of course other methods that could replace SCRAM.. > I would also like to see a test suite that covers the authentication > specifically. What you have in mind is a TAP test with a couple of roles and pg_hba.conf getting rewritten then reloaded? Adding it in src/test/recovery/ is the first place that comes in mind but that's not really something related to recovery... Any ideas? -- Michael
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Michael PaquierДата:
Сообщение: Re: Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200)