On Wed, Mar 22, 2017 at 8:54 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote: > On 03/17/2017 05:38 AM, Michael Paquier wrote: >> >> Regression tests are proving to be useful here (it would be nice to >> get those committed first!). I am noticing that this patch breaks >> connection for users with cleartext or md5-hashed verifier when >> "password" is used in pg_hba.conf. > > Are you sure? It works for me.
Hm... All the tests of password are still broken for me on macos, but work on Linux: not ok 4 - authentication success for method password, role scram_role # Failed test 'authentication success for method password, role scram_role' # at t/001_password.pl line 39. # got: '2' # expected: '0' not ok 5 - authentication success for method password, role md5_role # Failed test 'authentication success for method password, role md5_role' # at t/001_password.pl line 39. # got: '2' # expected: '0' not ok 6 - authentication success for method password, role plain_role [... dig ... dig ...] And after a lookup the failure is here: - result = get_role_password(port->user_name, &shadow_pass, logdetail); + shadow_pass = get_role_password(port->user_name, logdetail); if (result == STATUS_OK) result is never setup in this code path, so that may crash. And you need to do something like that, which makes the tests pass here: @@ -721,6 +721,7 @@ CheckPasswordAuth(Port *port, char **logdetail) return STATUS_EOF; /* client wouldn't send password */ shadow_pass = get_role_password(port->user_name, logdetail); + result = shadow_pass != NULL ? STATUS_OK : STATUS_ERROR; if (result == STATUS_OK) result = plain_crypt_verify(port->user_name, shadow_pass, passwd, logdetail); > Here's a slightly updated patch that includes required changes to the test > case (now that those have been committed), and some re-wording in the docs, > per Joe's suggestion. All the tests pass here. + verifier = scram_build_verifier(username, shadow_pass, 0); + + (void) parse_scram_verifier(verifier, &state->salt, &state->iterations, + state->StoredKey, state->ServerKey); + pfree(verifier); Not directly a problem of this patch, but scram_build_verifier can return NULL. >> -# Most users use SCRAM authentication, but some users use older clients >> -# that don't support SCRAM authentication, and need to be able to log >> -# in using MD5 authentication. Such users are put in the @md5users >> -# group, everyone else must use SCRAM. >> +# Require SCRAM authentication for most users, but make an exception >> +# for user 'mike', who uses an older client that doesn't support SCRAM >> +# authentication. >> # >> # TYPE DATABASE USER ADDRESS METHOD >> -host all @md5users .example.com md5 >> +host all mike .example.com md5 >> Why not still using @md5users? > > The old example didn't make much sense, now that md5 means "md5 or scram". > Could've still used @md5users, but I think this is more clear. The old > explanation was wrong or at least misleading anyway, because @md5users > doesn't refer to a group, but a flat file that lists roles. This patch introduces for the first time a non-generic user name in pg_hba.conf, that's why, keeping in mind that users could just copy-paste what is in the docs to make their own file, the approach of using an @ marker looks more generic to me. But I won't insist on this point more. I like the move of removing the status error codes from get_role_password(). With this support grid, only users with MD5-hashed verifiers cannot login when the matching hba entry uses scram. -- Michael -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers