Re: Password identifiers, protocol aging and SCRAM protocol

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