Re: Password identifiers, protocol aging and SCRAM protocol

On 12/08/2016 10:18 AM, Michael Paquier wrote:
> 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.

Hmm, interesting. I wonder how/when they imagine that error message to
be used. I suppose you could send a dummy server-first message, with a
made-up salt and iteration count, if the user is not found, so that you
can report that in the server-final message. But that seems
unnecessarily complicated, compared to just sending the error
immediately. I could imagine using a dummy server-first messaage to hide
whether the user exists, but that argument doesn't hold water if you're
going to report an "unknown-user" error, anyway.

Actually, we don't give away that information currently. If you try to
log in with password or MD5 authentication, and the user doesn't exist,
you get the same error as with an incorrect password. So, I think we do
need to give the client a made-up salt and iteration count in that case,
to hide the fact that the user doesn't exist. Furthermore, you can't
just generate random salt and iteration count, because then you could
simply try connecting twice, and see if you get the same salt and
iteration count. We need to deterministically derive the salt from the
username, so that you get the same salt/iteration count every time you
try connecting with that username. But it needs indistinguishable from a
random salt, to the client. Perhaps a SHA hash of the username and some
per-cluster secret value, created by initdb. There must be research
papers out there on how to do this..

To be really pedantic about that, we should also ward off timing
attacks, by making sure that the dummy authentication is no
faster/slower than a real one..

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

That's right, we can send a normal ERROR message. (But not for the
"user-not-found" case, as discussed above.)

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

So, I think this makes no sense:

> # Allow any user from host 192.168.12.10 to connect to database
> -# "postgres" if the user's password is correctly supplied.
> +# "postgres" if the user's password is correctly supplied and is
> +# using the correct password method.
> #
> # TYPE DATABASE USER ADDRESS METHOD
> host postgres all 192.168.12.10/32 md5
> +host postgres all 192.168.12.10/32 scram

But this is OK:

> +# Same as previous entry, except that the supplied password must be
> +# encrypted with SCRAM-SHA-256.
> +host all all .example.com scram
> +

Although, currently, the whole pg_hba.conf file in that example is a
valid file that someone might have on a real server. With the above
addition, it would not be. You would never have the two lines with the
same host/database/user combination in pg_hba.conf.

Overall, I think something like this would make sense in the example:

# Allow any user from hosts in the example.com domain to connect to
# any database, if the user's password is correctly supplied.
#
# Most users use SCRAM authentication, but some users use older clients
# that don't support SCRAM authentication, and need to be able to log
# in using MD5 authentication. Such users are put in the @md5users
# group, everyone else must use SCRAM.
#
# TYPE DATABASE USER ADDRESS METHOD
host all @md5users .example.com md5
host all all .example.com scram

- Heikki