Re: scram and \password

On Wed, Mar 22, 2017 at 8:54 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)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