Re: Password identifiers, protocol aging and SCRAM protocol - Mailing list pgsql-hackers

Hi Michael,

On 3/14/16 7:07 PM, Michael Paquier wrote:
> On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Mon, Mar 14, 2016 at 4:32 PM, David Steele <david@pgmasters.net> wrote:
>>> Could you provide an updated set of patches for review?  Meanwhile I am
>>> marking this as "waiting for author".
>>
>> Sure. I'll provide them shortly with all the comments addressed. Up to
>> now I just had a couple of comments about docs and whitespaces, so I
>> didn't really bother sending a new set, but this meritates a rebase.
> 
> And here they are. I have addressed the documentation and the
> whitespaces reported up to now at the same time.

Here's my full review of this patch set.

First let me thank you for submitting this patch for the current CF.  I
feel a bit guilty that I requested it and am only now posting a full
review.  In my defense I can only say that being CFM has been rather
more work than I was expecting, but I'm sure you know the feeling.

* [PATCH 1/9] Add facility to store multiple password verifiers

This is a pretty big patch but I went through it carefully and found
nothing to complain about.  Your attention to detail is impressive as
always.

Be sure to update the column names for pg_auth_verifiers as we discussed
in [1].

* [PATCH 2/9] Introduce password_protocols

diff --git a/src/test/regress/expected/password.out
b/src/test/regress/expected/password.out
+SET password_protocols = 'plain';
+ALTER ROLE role_passwd5 PASSWORD VERIFIERS (plain = 'foo'); -- ok
+ALTER ROLE role_passwd5 PASSWORD VERIFIERS (md5 = 'foo'); -- error
+ERROR:  specified password protocol not allowed
+DETAIL:  List of authorized protocols is specified by password_protocols.

So that makes sense but you get the same result if you do:

postgres=# alter user role_passwd5 password 'foo';
ERROR:  specified password protocol not allowed
DETAIL:  List of authorized protocols is specified by password_protocols.

I don't think this makes sense - if I have explicitly set
password_protocols to 'plain' and I don't specify a verifier for alter
user then it seems like it should work.  If nothing else the error
message lacks information needed to identify the problem.

* [PATCH 3/9] Add pg_auth_verifiers_sanitize

This function is just a little scary but since password_protocols
defaults to 'plain,md5' I can live with it.

* [PATCH 4/9] Remove password verifiers for unsupported protocols in
pg_upgrade

Same as above - it will always be important for password_protocols to
default to *all* protocols to avoid data being dropped during the
pg_upgrade by accident.  You've done that here (and later in the SCRAM
patch) so I'm satisfied but it bears watching.

What I would do is add some extra comments in the GUC code to make it
clear to always update the default when adding new verifiers.

* [PATCH 5/9] Move sha1.c to src/common

This looks fine to me and is a good reuse of code.

* [PATCH 6/9] Refactor sendAuthRequest

I tested this across different client versions and it seems to work fine.

* [PATCH 7/9] Refactor RandomSalt to handle salts of different lengths

A simple enough refactor.

* [PATCH 8/9] Move encoding routines to src/common/

A bit surprising that these functions were never used by any front end code.

* Subject: [PATCH 9/9] SCRAM authentication

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
@@ -1616,18 +1619,34 @@ FlattenPasswordIdentifiers(List *verifiers, char
*rolname)         * instances of Postgres, an md5 hash passed as a plain verifier         * should still be treated as
anMD5 entry.         */
 
-        if (spec->veriftype == AUTH_VERIFIER_MD5 &&
-            !isMD5(spec->value))
+        switch (spec->veriftype)        {
-            char encrypted_passwd[MD5_PASSWD_LEN + 1];
-            if (!pg_md5_encrypt(spec->value, rolname, strlen(rolname),
-                                encrypted_passwd))
-                elog(ERROR, "password encryption failed");
-            spec->value = pstrdup(encrypted_passwd);
+            case AUTH_VERIFIER_MD5:

It seems like this case statement should have been introduced in patch
0001.  Were you just trying to avoid churn in the code unless SCRAM is
committed?

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
+
+static char *
+read_attr_value(char **input, char attr)
+{

Numerous functions like the above in auth-scram.c do not have comments.

diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
+    else if (strcmp(token->string, "scram") == 0)
+    {
+        if (Db_user_namespace)
+        {
+            ereport(LOG,
+                    (errcode(ERRCODE_CONFIG_FILE_ERROR),
+                     errmsg("SCRAM authentication is not supported when
\"db_user_namespace\" is enabled"),
+                     errcontext("line %d of configuration file \"%s\"",
+                                line_num, HbaFileName)));
+            return NULL;
+        }
+        parsedline->auth_method = uaSASL;
+    }

Why is that?  Is it because gss auth should be expected in this case or
some limitation of SCRAM?  Anyway, it wasn't clear to me why this would
be true so some comments here would be good.

diff --git a/src/common/scram-common.c b/src/common/scram-common.c
+void
+scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen)
+{
+    SHA1Update(&ctx->sha1ctx, (const uint8 *) str, slen);
+}

Same in scram-common.c WRT comments.

diff --git a/src/include/common/scram-common.h
b/src/include/common/scram-common.h
+extern void scram_ClientOrServerKey(const char *password, const char
*salt, int saltlen, int iterations, const char *keystr, uint8 *result);

My, that's a very long line!

* A few general things:

Most of the new scram modules are seriously in need of better comments -
I pointed out a few but all the new files suffer from this lack.

The strings "plain", "md5", and "scram" are used often enough that I
think it would be nice if they were constants.  I feel the same way
about verifier methods 'm', 'p', 's' -- perhaps more so because they
aren't very verbose.

It looks like this will need a bit of work if the GSSAPI patch goes in
(and vice versa).  Not a problem but you'll need to be prepared to do
that quickly in the event - time is flying.

-- 
-David
david@pgmasters.net

[1]
http://www.postgresql.org/message-id/CAB7nPqSGm-9c4yFULt4GS9TzoSuz8XbO-K7TGGGw08sztfG2Uw@mail.gmail.com