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
isalways 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
testuserWITH 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
listSCRAM 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
PasswordAuthentication: 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
aboutmd5 
 
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