Re: WIP: SCRAM authentication

On 08/08/2015 03:21 PM, Michael Paquier wrote:
> On Sun, Aug 9, 2015 at 6:51 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> 1. we drop the parameter password_encryption
>> 2. we add the parameter password_storage, which takes a list:
>> - plain : plain text
>> - md5 : current md5 hashes
>> - scram : new scram hashed passwords
>> This defaults to 'md5, scram' if not specified.
>> This list might be extended in the future.
>
> Perhaps using a different GUC than password_encryption is safer... I
> am not that sure. Still that's how I switched password_encryption to
> actually handle a list. Default is 'md5' with the first patch, and
> 'md5,scram' with the scram patch added and it sets the list of
> password verifiers created when PASSWORD with ENCRYPTED/UNENCRYPTED is
> used.

Well, generally I feel like if we're going to change the *type* of a GUC
parameter, we ought to change the *name*. It's far easier for users to
figure out that the contents of a parameter need to change if the name
is also changed.

In other words, I think "invalid parameter 'password_encryption'" is an
easier to understand error message than "invalid password_encryption
type 'on'". Besides which, password_encryption was always a misnomer.

Unless you're going to still accept "on, off" in some kind of wierd
backwards-compatibitlity mode? If so, how does that work?

> Like password ENCRYPTED (md5,scram) or similar? If no method is
> passed, I think that we should default to password_storage instead.

Make sense.

> Also, I still think that something like PASSWORD VERIFIERS is needed,
> users may want to set the verifier user for each method after
> calculating it on client-side: we authorize that for md5 even now, and
> that's not something this spec authorizes.

I don't follow this. Mind you, I'm not sure that I need to.

>> 5. we add the superuser-only function pg_apply_password_policy(). This
>> applies the policy expressed by password_storage, generating or erasing
>> passwords for each user.
>
> pg_upgrade could make use of that to control password aging with an
> option to do the cleanup or not. Not sure what the default should be
> though. pg_apply_password_policy(roleid) would be useful as well to do
> it on a role base.

No objections to an optional roleid parameter, if you think people will
use it.

>> 6. We add a new connection error for "authentication __method__ not
>> supported for user"
>
> Hm? This would let any user trying to connect with a given method know
> that if a method is used or not. What's wrong with failing as we do
> now. In case of PASSWORD NULL for example, an attempt of connection
> fails all the time with "incorrect password" or similar.

So, the DBA sets password_storage = 'scram', but doesn't take the md5
lines out of pg_hba.conf.

The app dev tries to connect using a driver which only supports md5.
What error should they get? A user/DBA who is getting "invalid
password" is going to spend a long time debugging it. Also, it would be
very useful to have a distinctive error in the log, so that DBAs could
see who is *trying* to connect with the wrong verifier.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com