Re: [HACKERS] scram and \password - Mailing list pgsql-hackers

On Wed, Mar 22, 2017 at 8:54 PM, Heikki Linnakangas <hlinnaka@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
sendpassword */
 
   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