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