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

From Michael Paquier
Subject Re: Password identifiers, protocol aging and SCRAM protocol
Date
Msg-id CAB7nPqTkoC4NLyyivz2=usUOo47iZnS+2e14nkmVKM3P8hWqwA@mail.gmail.com
Whole thread Raw
In response to Re: Password identifiers, protocol aging and SCRAM protocol  (David Steele <david@pgmasters.net>)
Responses Re: Password identifiers, protocol aging and SCRAM protocol
List pgsql-hackers
On Fri, Mar 18, 2016 at 3:16 AM, David Steele <david@pgmasters.net> wrote:
> Here's my full review of this patch set.

Thanks!

> 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.

I get the idea. That's a very draining activity and I can see what you
are doing. That's impressive. Really.

> * [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].

Done. I have added as well the block of 0009 you pointed out into this
patch for clarity.

> * [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.

Hm. The problem here is the interaction between the new
password_protocols and the existing password_encryption.
password_protocols involves that password_encryption should not
contain elements not listed in it, in short password_protocols @>
password_encryption. So I think that the GUC callbacks checking the
validity of those parameter values should check that each other are
not set to incorrect values. One thing to simplify those validity
checks would be to make password_protocols a PGC_POSTMASTER, aka it
needs a restart to be updated. This sacrifices a large portion of the
regression tests though... Do others have thoughts to share? I have
not updated the patch yet, and I would personally let both parameters
as they are now, aka password_protocols as PGC_SUSET and
password_encryption as PGC_USERSET, and check their validity when they
are updated, but I am not alone here (hopefully).

> * [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.

Another thing that I thought about was to integrate as part of
pg_upgrade_support part. That's no big deal to do it this way as well,
though I thought that it could be useful for admins. So extra ideas
are welcome. That's superuser-only anyway... And a critical part to
manage old protocol deprecation.

> * [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.

We could have an extra keyword like "all" to all mapping to all the
existing protocols, but I find listing the protocols explicitly a more
verbose and simple concept, that's why I chose that.

> 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.

Good idea.

> * [PATCH 5/9] Move sha1.c to src/common
>
> This looks fine to me and is a good reuse of code.

Yes.

> * [PATCH 6/9] Refactor sendAuthRequest
>
> I tested this across different client versions and it seems to work fine.

OK, cool!

> * [PATCH 7/9] Refactor RandomSalt to handle salts of different lengths
>
> A simple enough refactor.

That's something we should do as an independent change I think.

> * [PATCH 8/9] Move encoding routines to src/common/
>
> A bit surprising that these functions were never used by any front end code.

Perhaps there are some client tools that copy-paste it. I cannot be
sure. At least it seems to me that this is useful enough as an
independent change.

> * 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 an MD5 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?

Yeah, right. I have now plugged this portion into 0001.

> 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.

Noted. I have done nothing on that yet though :) And I am lowering the
priority for 0009 in this CF to keep focus on the core machinery
instead, as well as other patches that need feedback.

> 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.

The username is part of the identifier used as part of the protocol,
so we cannot rely on mappings of db_user_namespace.

> 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.

OK, noted. I have not updated those comments yet though. At this stage
of the game considering 0009 for integration is a rather difficult
task, and I suspect enough work with the underlying patches. For 9.6,
I would be happy enough if we got the basic infra in core.

> 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!

Oops. Sorry.

> * 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.

Indeed. Honestly, as you say, time flies, and by the time of the
feature freeze I am thinking that the only sane target for the CF
would be to focus on 0001~0004. That's the basic infrastructure I
think we need anyway. 0005~0008 are things that I think are useful
taken independently and are simple refactoring, so they could be
considered with the time frame we have. 0009 is a bit too complex. I
expect enough comments on the first patches to keep my time busy until
the end of this CF without that, that's still useful for testing by
the way.

> The strings "plain", "md5", and "scram" are used often enough that I
> think it would be nice if they were constants.

This makes sense. So I switched the code this way. Note that for md5 I
think that it makes sense to use a #define variable when referring to
the verifier method, not when referring to the prefix of a md5
verifier. Those full names are added in pg_auth_verifiers.h.

> I feel the same way
> about verifier methods 'm', 'p', 's' -- perhaps more so because they
> aren't very verbose.

I am thinking of the verifier abbreviations in the system catalog in a
way similar to pg_class' relkind, explaining the one-character
identifier, so I wish letting them as-is.

> 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.

That's not an issue for me to rebase this set of patches. The only
conflicts that I anticipate are on 0009, but I don't have high hopes
to get this portion integrating into core for 9.6, the rest of the
patches is complicated enough, and everyone bandwidth is limited.
--
Michael

Attachment

pgsql-hackers by date:

Previous
From: Christoph Berg
Date:
Subject: Re: Relaxing SSL key permission checks
Next
From: Robert Haas
Date:
Subject: Re: pgbench - allow backslash-continuations in custom scripts