Re: Password identifiers, protocol aging and SCRAM protocol

Michael Paquier <michael.paquier@gmail.com>

From: Michael Paquier <michael.paquier@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, David Steele <david@pgmasters.net>, Tom Lane <tgl@sss.pgh.pa.us>, David Fetter <david@fetter.org>, Alvaro Herrera <alvherre@2ndquadrant.com>, Magnus Hagander <magnus@hagander.net>, Julian Markwort <julian.markwort@uni-muenster.de>, Stephen Frost <sfrost@snowman.net>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>, Valery Popov <v.popov@postgrespro.ru>
Date: 2016-12-08T08:18:16Z
Lists: pgsql-hackers
On Thu, Dec 8, 2016 at 5:54 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Attached those here, as add-on patches to your latest patch set.

Thanks for looking at it!

> 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.

Hmmm. How do we handle the case where the user name does not match
then? The spec gives an error message e= specifically for this case.
If this is taken into account we need to perform sanity checks at
initialization phase I am afraid as the number of iterations and the
salt are part of the verifier. So you mean that just sending out a
normal ERROR message is fine at an earlier step (with *logdetails
filled for the backend)? I just want to be sure I understand what you
mean here.

> 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.

OK, it would be better to add a paragraph in client-auth.sgml
regarding the mapping of the two settings. For the example of file in
postgresql.conf, I would have really thought that adding directly a
line with "scram" listed was enough though. Perhaps a comment to say
that if md5 and scram are specified the first one wins where a user
and database name map?
-- 
Michael


Commits

  1. Support SCRAM-SHA-256 authentication (RFC 5802 and 7677).

  2. Refactor SHA2 functions and move them to src/common/.

  3. Replace isMD5() with a more future-proof way to check if pw is encrypted.

  4. Remove bogus notice that older clients might not work with MD5 passwords.

  5. Refactor the code for verifying user's password.

  6. Replace PostmasterRandom() with a stronger source, second attempt.

  7. Remove support for (insecure) crypt authentication.