Re: Password identifiers, protocol aging and SCRAM protocol

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