Re: Password identifiers, protocol aging and SCRAM protocol

On Tue, Mar 15, 2016 at 6:38 PM, David Steele <david(at)pgmasters(dot)net> wrote:
> Hi Michael,
>
> On 3/14/16 7:07 PM, Michael Paquier wrote:
>
>> On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>>
>>> On Mon, Mar 14, 2016 at 4:32 PM, David Steele <david(at)pgmasters(dot)net> wrote:
>>>
>>>> Could you provide an updated set of patches for review? Meanwhile I am
>>>> marking this as "waiting for author".
>>>
>>> Sure. I'll provide them shortly with all the comments addressed. Up to
>>> now I just had a couple of comments about docs and whitespaces, so I
>>> didn't really bother sending a new set, but this meritates a rebase.
>>
>> And here they are. I have addressed the documentation and the
>> whitespaces reported up to now at the same time.
>
> For this first review I would like to focus on the user visible changes
> introduced in 0001-0002.

Thanks for the input!

> 1) I see that rolvaliduntil is still in pg_authid:
> I think that's OK if we now define it to be "role validity" (it's still
> password validity in the patched docs). I would also like to see a
> validuntil column in pg_auth_verifiers so we can track password
> expiration for each verifier separately. For now I think it's enough to
> copy the same validity both places since there can only be one verifier.

FWIW, this is an intentional change, and my goal is to focus on only
the protocol aging for now. We will need to move rolvaliduntil to
pg_auth_verifiers if we want to allow rolling updates of password
verifiers for a given role, but that's a different patch, and we need
to think about the SQL interface carefully. This infrastructure makes
the move easier by the way to do that, and honestly I don't really see
what we gain now by copying the same value to two different system
catalogs.

> 2) I don't think the column naming in pg_auth_verifiers is consistent
> with other catalogs:
> postgres=# select * from pg_auth_verifiers;
> roleid | verimet | verival
> --------+---------+-------------------------------------
> 16387 | m | md505a671c66aefea124cc08b76ea6d30bb
> 16388 | p | testu
>
> System catalogs generally use a 3 character prefix so I would expect the
> columns to be (if we pick avr as a prefix):

OK, this makes sense.

> avrrole
> avrmethod
> avrverifier

Assuming "ver" is the prefix, we get: verroleid, vermethod, vervalue.
I kind of like those ones, more than with "avr" as prefix actually.
Other ideas are of course welcome.

> I'm not a big fan in abbreviating too much so you can see I've expanded
> the names a bit.

Sure.

> 3) rolpassword is still in pg_shadow even though it is not useful anymore:
> postgres=# select usename, passwd, valuntil from pg_shadow;
>
> usename | passwd | valuntil
> ---------+----------+------------------------
> vagrant | ******** |
> test | ******** |
> testu | ******** | 2017-01-01 00:00:00+00
>
> If anyone is actually using this column in a meaningful way they are in
> for a nasty surprise when trying use the value in passwd as a verifier.
> I would prefer to drop the column entirely and produce a clear error.
>
> Perhaps a better option would be to drop pg_shadow entirely since it
> seems to have no further purpose in life.

We discussed that on the previous thread and the conclusion was to
keep pg_shadow, but to clobber the password value with "***",
explaining this choice:
http://www.postgresql.org/message-id/6174.1455501497@sss.pgh.pa.us
--
Michael