Re: Password identifiers, protocol aging and SCRAM protocol

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: 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-07 20:54:56
Message-ID: e139fcbc-cfb2-3a64-93ac-afd5ba342960@iki.fi
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 12/07/2016 08:39 AM, Michael Paquier wrote:
> On Tue, Nov 29, 2016 at 1:36 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)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

Attachment Content-Type Size
0008-Rewrite-nonce-generation.patch text/x-patch 8.3 KB
0009-Random-number-fixes.patch text/x-patch 7.4 KB
0010-No-need-to-disallow-SCRAM-with-db_user_namespace.patch text/x-patch 1.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2016-12-07 21:03:32 Re: Unlogged tables cleanup
Previous Message Pavel Stehule 2016-12-07 20:48:20 Re: patch: function xmltable