Re: Password identifiers, protocol aging and SCRAM protocol

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


My remarks follow below.

Kind regards,
Julian Markwort
julian.markwort@uni-muenster.de




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)

     I don't know if this is intended behaviour. 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).
     In either case, a bit of text explaining the (UN)ENCRYPTED option 
should be added to the documentation of the CREATE/ALTER ROLE functions.

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

         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

     VI.
         ALTER ROLE: is SCRAM also dependent on the role name for 
salting? if so, add warning.
                     (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?)
         CREATE ROLE: can SCRAM also be used in the list of PASSWORD 
VERIFIERS?

     VII.
         49. System Catalogs:
             49.9 pg_auth_verifiers: Column names and types are mixed up
                                     in description for column vervalue:
                                     explain some basic stuff about md5 
maybe as well?

                                     remark: the statements about the 
composition of the string that is md5-hashed are contradictory.
                                     (concatenating "bar" to "foo" 
results in foobar, not the other way round, as it is implied in the 
explanation of the md5 hashing), this however, is not really linked to 
the changes introduced with these patches.

                                     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*" ).
                                     (if this is a general convention 
thing, please ignore this comment, however I couldn't find anything in 
the relevant RFC's while skimming through them).

         50. Frontend/Backend Protocol
             50.2.1 Start-up:  add explanation for 
"AuthenticationSCRAMPassword" authentication request message. (?)
             50.5 message formats  see 50.2.1