Re: WIP: SCRAM authentication - Mailing list pgsql-hackers

On Sun, Aug 9, 2015 at 6:51 AM, Josh Berkus <josh@agliodbs.com> wrote:
> Obviously the backwards-compatibility issues are pretty major ... it'll
> be years before all drivers support SCRAM ... but we also want to
> provide a path forwards for secure installations in which no md5 hashes
> are stored.
>
> This says "backwards-compatible GUC" to me.  Here's one idea on how to
> handle this:
>
> 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.

> 3. All password types in the list are generated.  This means having
> multiple columns in pg_shadow, or an array.  An array would support the
> addition of future password storage methods.

Yeah, the patch switches pg_shadow to an array like that, with as
elements method:value, so you get actually md5:md5blah,scram:stuff in
all the patches applied.

> 4. CREATE ROLE / ALTER ROLE syntax is changed to accept a parameter to
> ENCRYPTED in order to support md5, scram, and future methods.  If no
> parameter is supplied, ENCRYPTED will default to 'md5, scram'.

Like password ENCRYPTED (md5,scram) or similar? If no method is
passed, I think that we should default to password_storage instead.
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.

> 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.

> 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.

> 7. Two versions from now, we change the defaults.

Or three. We cannot expect users to change immediately, and it is
wiser to let dust set on the new feature in case critical bugs show up
after the first GA.

Something that sounds more like a detail in this thread by reading
other comments: I think that it is important to store password
verifiers in a different catalog than pg_authid for two reasons:
- that's more user-friendly, a sysadmin could directly join the new
catalog with pg_authid to get all the verifiers for a single user
method
- at password lookup when authorizing connection, there is no need to
fetch all the password verifiers and parse the array with all
verifiers.
- more scalable if we have many verifier methods in the future, though
we are not going to have hundreds of them. Though I am wondering about
per-method validtime and per-method authorization options.
Regards,
-- 
Michael