Re: Password identifiers, protocol aging and SCRAM protocol

On 12/07/2016 08:39 AM, Michael Paquier wrote:
> On Tue, Nov 29, 2016 at 1:36 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Nothing more will likely happen in this CF, so I have moved it to
>> 2017-01 with the same status of "Needs Review".
>
> Attached is a new set of patches using the new routines
> pg_backend_random() and pg_strong_random() to handle the randomness in
> SCRAM:
> - 0001 refactors the SHA2 routines. pgcrypto uses raw files from
> src/common when compiling with this patch. That works on any platform,
> and this is the simplified version of upthread.
> - 0002 adds base64 routines to src/common.
> - 0003 does some refactoring regarding the password encryption in
> ALTER/CREATE USER queries.
> - 0004 adds the clause PASSWORD (val USING method) in CREATE/ALTER USER.
> - 0005 is the code patch for SCRAM. Note that this switches pgcrypto
> to link to libpgcommon as SHA2 routines are used by the backend.
> - 0006 adds some regression tests for passwords.
> - 0007 adds some TAP tests for authentication.
> This is added to the upcoming CF.

I spent a little time reading through this once again. Steady progress, 
did some small fixes:

* Rewrote the nonce generation. In the server-side, it first generated a 
string of ascii-printable characters, then base64-encoded them, which is 
superfluous. Also, avoid calling pg_strong_random() one byte at a time, 
for performance reasons.

* Added a more sophisticated fallback implementation in libpq, for the 
--disable-strong-random cases, similar to pg_backend_random().

* No need to disallow SCRAM with db_user_namespace. It doesn't include 
the username in the salt like MD5 does.

Attached those here, as add-on patches to your latest patch set. I'll 
continue reviewing, but a couple of things caught my eye that you may 
want to jump on, in the meanwhile:

On error messages, the spec says:

> o  e: This attribute specifies an error that occurred during
>       authentication exchange.  It is sent by the server in its final
>       message and can help diagnose the reason for the authentication
>       exchange failure.  On failed authentication, the entire server-
>       final-message is OPTIONAL; specifically, a server implementation
>       MAY conclude the SASL exchange with a failure without sending the
>       server-final-message.  This results in an application-level error
>       response without an extra round-trip.  If the server-final-message
>       is sent on authentication failure, then the "e" attribute MUST be
>       included.

Note that it says that the server can send the error message with the e= 
attribute, in the *final message*. It's not a valid response in the 
earlier state, before sending server-first-message. I think we need to 
change the INIT state handling in pg_be_scram_exchange() to not send e= 
messages to the client. On an error at that state, it needs to just bail 
out without a message. The spec allows that. We can always log the 
detailed reason in the server log, anyway.

As Peter E pointed out earlier, the documentation is lacking, on how to 
configure MD5 and/or SCRAM. If you put "scram" as the authentication 
method in pg_hba.conf, what does it mean? If you have a line for both 
"scram" and "md5" in pg_hba.conf, with the same database/user/hostname 
combo, what does that mean? Answer: The first one takes effect, the 
second one has no effect. Yet the example in the docs now has that, 
which is nonsense :-). Hopefully we'll have some kind of a "both" 
option, before the release, but in the meanwhile, we need describe how 
this works now in the docs.

- Heikki