Re: [REVIEW]: Password identifiers, protocol aging and SCRAM protocol

Valery Popov <v.popov@postgrespro.ru>

From: Valery Popov <v.popov@postgrespro.ru>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Date: 2016-03-03T10:32:35Z
Lists: pgsql-hackers
This is a review of "Password identifiers, protocol aging and SCRAM 
protocol" patches
http://www.postgresql.org/message-id/CAB7nPqSMXU35g=W9X74HVeQp0uvgJxvYOuA4A-A3M+0wfEBv-w@mail.gmail.com

Contents & Purpose
--------------------------
There was a discussion dedicated to SCRAM:
http://www.postgresql.org/message-id/55192AFE.6080106@iki.fi

This set of patches implements the following:
- Introduce in Postgres an extensible password aging facility, by having 
a new concept of 1 user/multiple password verifier, one password 
verifier per protocol.
- Give to system administrators tools to decide unsupported protocols, 
and have pg_upgrade use that
- Introduce new password protocols for Postgres, aimed at replacing 
existing, say limited ones.

This set of patches consists of 9 separate patches.
Description of each patch is well described in initial thread email and 
in comments.
The first set of patches 0001-0008 adds facility to store multiple 
password verifiers,
CREATE ROLE and ALTER ROLE are extended with PASSWORD VERIFIERS, new 
superuser GUC parameters which specifies a list of supported password
protocols in Postgres backend, added pg_auth_verifiers_sanitize 
function, removed password verifiers for unsupported protocols in 
pg_upgrade, and more features.
The second set of patch 0005~0008 introduces a new protocol, SCRAM, and 
0009 is SCRAM itself.

Initial Run
-------------
Included in the patches are:
- source code
- regression tests
- documentation
The source code is well commented.
The patches are in context diff format and were applied correctly to 
HEAD (there were 2 warnings, and it was fixed by author).
There were several markup warnings, should be fixed by author.
Regression tests pass successfully, without errors. It seems that the 
patches work as expected.
The patch 0009 depends on all previous patches 0001-0008: first we need 
to apply patches 0001-0008, then 0009.

Performance
-----------
I have not tested possible performance issues yet.

Conclusion
--------------
I think introduced features are useful and I vote for commit +1.

On 03/02/2016 02:55 PM, Michael Paquier wrote:
> On Wed, Mar 2, 2016 at 5:43 PM, Valery Popov <v.popov@postgrespro.ru> wrote:
> So the <literal> markup is missing. Thanks. I am taking note of it. 

-- 
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company



Commits

  1. Support SCRAM-SHA-256 authentication (RFC 5802 and 7677).

  2. Refactor SHA2 functions and move them to src/common/.

  3. Replace isMD5() with a more future-proof way to check if pw is encrypted.

  4. Remove bogus notice that older clients might not work with MD5 passwords.

  5. Refactor the code for verifying user's password.

  6. Replace PostmasterRandom() with a stronger source, second attempt.

  7. Remove support for (insecure) crypt authentication.