Re: Password identifiers, protocol aging and SCRAM protocol

On Wed, Mar 30, 2016 at 1:44 AM, Julian Markwort
<julian.markwort@uni-muenster.de> wrote:
> ----[This is a rather informal user-review]----
>
> Here are some thoughts and experiences on using the new features, I focused
> on testing the basic funcionality of setting password_encryption to scram
> and then generating some users with passwords. After that, I took a look at
> the documentation, specifically all those parts that mentioned "md5", but
> not SCRAM, so i took some time to write those down and add my thoughts on
> them.
>
> We're quite keen on seeing these features in a future release, so I suggest
> that we add these patches to the next commitfest asap in order to keep the
> discussion on this topic flowing.
>
> For those of you who like to put the authentication method itself up for
> discussion, I'd like to add that it seems fairly simple to insert code for
> new authentication mechanisms.
> In conclusion I think these patches are very useful.

The reception of the concept of multiple password verifiers for a
single role was rather... cold. So except if a committer pushes hard
for it is never going to show up. There is clear consensus that SCRAM
is something needed though, so we may as well just focus on that.

> Things I noticed:
> 1.
>     when using either
>         CREATE ROLE
>         ALTER ROLE
>     with the parameter
>         ENCRYPTED
>     md5 encryption is always assumed (I've come to realize that UNENCRYPTED
> always equals plain and, in the past, ENCRYPTED equaled md5 since there were
> no other options)

Yes, that's to match the current behavior, and make something fully
backward-compatible. Switching to md5 + scram may have made sense as
well though.

>     I don't know if this is intended behaviour.

This is an intended behavior.

> Maybe this option should be
> omitted (or marked as deprecated in the documentation) from the CREATE/ALTER
> functions (since without this Option, the password_encryption from
> pg_conf.hba is used)
>     or maybe it should have it's own parameter like
>         CREATE ROLE testuser WITH LOGIN ENCRYPTED 'SCRAM' PASSWORD 'test';
>     so that the desired encryption is used.
>     From my point of view, this would be the sensible thing to do,
> especially if different verifiers should be allowed (as proposed by these
> patches).

The extension PASSWORD VERIFIERS is aimed at covering this need. The
grammar of those queries is not a fixed thing though.

>     In either case, a bit of text explaining the (UN)ENCRYPTED option should
> be added to the documentation of the CREATE/ALTER ROLE functions.

It is specified here;
http://www.postgresql.org/docs/devel/static/sql-createrole.html
And the patch does not ignore that.

> 2.
>     Documentation
>     III.
>         17. Server Setup and Operation
>             17.2. Creating a Database Cluster: maybe list SCRAM as a
> possible method for securing the db-admin

Indeed.

>         19. Client Authentication
>             19.1. The pg_hba.conf File: SCRAM is not listed in the list of
> available auth_methods to be specified in pg_conf.hba
>             19.3 Authentication Methods
>                 19.3.2 Password Authentication: SCRAM would belong to the
> same category as md5 and password, as they are all password-based.
>
>         20. Database Roles
>             20.2. Role Attributes: password : list SCRAM as authentication
> method as well

Indeed.

>     VI.
>         ALTER ROLE: is SCRAM also dependent on the role name for salting? if
> so, add warning.

No.

>                     (it doesn't seem that way, however I'm curious as to why
> the function FlattenPasswordIdentifiers in src/backend/commands/user.c
> called by AlterRole passes rolname to scram_build_verifier(), when that
> function does absolutely nothing with this argument?)

Yeah, this argument could be removed.

>         CREATE ROLE: can SCRAM also be used in the list of PASSWORD
> VERIFIERS?

Yes.

>     VII.
>         49. System Catalogs:
>             49.9 pg_auth_verifiers: Column names and types are mixed up
>                                     in description for column vervalue:

Yes, things are messed up a bit there. Thanks for noticing.

>                                     remark: naming inconsistency: md5
> vervalues are stored "md5*" why don't we take the same approach and use it
> on SCRAM hashes (i.e. "scram*" ).

Perhaps this makes sense if there is no pg_auth_verifiers.
-- 
Michael