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. > - 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. > - 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. > - 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. > - 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. Please add an example to the documentation and explain better how this interacts with the existing ENCRYPTED PASSWORD clause. > - 0007, the SCRAM implementation. No documentation about pg_hba.conf changes, so I don't know how to use this. ;-) 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. I would also like to see a test suite that covers the authentication specifically. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers