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
- 0001-Add-facility-to-store-multiple-password-verifiers.patch
- 0002-Introduce-password_protocols.patch
- 0003-Add-pg_auth_verifiers_sanitize.patch
- 0004-Remove-password-verifiers-for-unsupported-protocols-.patch
- 0005-Move-sha1.c-to-src-common.patch
- 0006-Refactor-sendAuthRequest.patch
- 0007-Refactor-RandomSalt-to-handle-salts-of-different-len.patch
- 0008-Move-encoding-routines-to-src-common.patch
- 0009-SCRAM-authentication.patch
pgsql-hackers by date: