On 09/26/2016 09:02 AM, Michael Paquier wrote:
> On Mon, Sep 26, 2016 at 2:15 AM, David Steele <david@pgmasters.net> wrote:
>> On 9/3/16 8:36 AM, Michael Paquier wrote:
>>>
>>> Attached is a new series:
>
> Thanks for the review and the comments!
I read-through this again, and did a bunch of little fixes:
* Added error-handling for OOM and other errors in liybpq
* In libpq, added check that the server sent back the same client-nonce
* Turned ERRORs into COMMERRORs and removed DEBUG4 lines (they could
reveal useful information to an attacker)
* Improved comments
Some things that need to be resolved (I also added FIXME comments for
some of this):
* A source of random values. This currently uses PostmasterRandom()
similarly to how the MD5 salt is generated, in the server, but plain old
random() in the client. If built with OpenSSL, we should probably use
RAND_bytes(). But what if the client is built without OpenSSL? I believe
the protocol doesn't require cryptographically strong randomness for the
nonces, i.e. it's OK if they're predictable, but they should be
different for each session.
* Nonce and salt lengths. The patch currently uses 10 bytes for both,
but I think I just pulled number that out of thin air. The spec doesn't
say anything about nonce and salt lengths AFAICS. What do other
implementations use? Is 10 bytes enough?
* The spec defines a final "server-error" message that the server sends
on authentication failure, or e.g. if a required extension is not
supported. The patch just uses FATAL for those. Should we try to send a
server-error message instead, or before, the elog(FATAL) ?
I'll continue hacking this later, but need a little break for now.
>> I'm a bit concerned that a mixture of md5/scram could cause confusion
>> and think this may warrant discussion somewhere in the documentation
>> since the idea is for users to migrate from md5 to scram.
>
> We could finish with a red warning in the docs to say that users are
> recommended to use SCRAM instead of MD5. Just an idea, perhaps that's
> not mandatory for the first shot though.
Some sort of Migration Guide would certainly be in order. There isn't
any easy migration path with this patch series alone, so perhaps that
should be part of the follow-up patches that add the "MD5 or SCRAM"
authentication method to pg_hba.conf, or support for having both
verifiers for the same user in pg_authid.
- Heikki