Re: Password identifiers, protocol aging and SCRAM protocol

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Fetter <david(at)fetter(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Magnus Hagander <magnus(at)hagander(dot)net>, Julian Markwort <julian(dot)markwort(at)uni-muenster(dot)de>, Stephen Frost <sfrost(at)snowman(dot)net>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Valery Popov <v(dot)popov(at)postgrespro(dot)ru>
Subject: Re: Password identifiers, protocol aging and SCRAM protocol
Date: 2016-12-08 08:18:16
Message-ID: CAB7nPqSqEwRgmVCS_OW3sJn6baymQ6MsO3kKMRvHLC0RZaKaYA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Dec 8, 2016 at 5:54 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-12-08 08:21:23 Re: Hang in pldebugger after git commit : 98a64d0
Previous Message Heikki Linnakangas 2016-12-08 07:55:44 Re: tuplesort_gettuple_common() and *should_free argument