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>, David Steele <david(at)pgmasters(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, David Fetter <david(at)fetter(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Magnus Hagander <magnus(at)hagander(dot)net>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, 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-09-27 12:01:09
Message-ID: 4709913e-639f-6696-da82-0ea18a3444c4@iki.fi
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 09/26/2016 09:02 AM, Michael Paquier wrote:
> On Mon, Sep 26, 2016 at 2:15 AM, David Steele <david(at)pgmasters(dot)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

Attachment Content-Type Size
0001-Refactor-SHA-functions-and-move-them-to-src-common.patch text/x-diff 75.9 KB
0002-Move-encoding-routines-to-src-common.patch text/x-diff 21.9 KB
0003-Switch-password_encryption-to-a-enum.patch text/x-diff 9.1 KB
0004-Refactor-decision-making-of-password-encryption-into.patch text/x-diff 4.7 KB
0005-Create-generic-routine-to-fetch-password-and-valid-u.patch text/x-diff 4.4 KB
0006-Support-for-SCRAM-SHA-256-authentication-RFC-5802-an.patch text/x-diff 80.8 KB
0007-Add-clause-PASSWORD-val-USING-protocol-to-CREATE-ALT.patch text/x-diff 8.8 KB
0008-Add-regression-tests-for-passwords.patch text/x-diff 9.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2016-09-27 12:06:38 Re: Transactions involving multiple postgres foreign servers
Previous Message Jeevan Chalke 2016-09-27 11:55:32 Re: Add support for restrictive RLS policies