Re: Password identifiers, protocol aging and SCRAM protocol

On 9/26/16 2:02 AM, Michael Paquier wrote:

> On Mon, Sep 26, 2016 at 2:15 AM, David Steele <david@pgmasters.net> wrote:
>
> Thanks for the review and the comments!
> 
>> I notice that the copyright from pgcrypto/sha1.c was carried over but
>> not the copyright from pgcrypto/sha2.c.  I'm no expert on how this
>> works, but I believe the copyright from sha2.c must be copied over.
> 
> Right, those copyright bits are missing:
> - * AUTHOR: Aaron D. Gifford <me@aarongifford.com>
> [...]
> - * Copyright (c) 2000-2001, Aaron D. Gifford
> The license block being the same, it seems to me that there is no need
> to copy it over. The copyright should be enough.

Looks fine to me.

>> Also, are there any plans to expose these functions directly to the user
>> without loading pgcrypto?  Now that the functionality is in core it
>> seems that would be useful.  In addition, it would make this patch stand
>> on its own rather than just being a building block.
> 
> There have been discussions about avoiding enabling those functions by
> default in the distribution. We'd rather not do that...

OK.

>> * [PATCH 2/8] Move encoding routines to src/common/
>>
>> I wonder if it is confusing to have two of encode.h/encode.c.  Perhaps
>> they should be renamed to make them distinct?
> 
> Yes it may be a good idea to rename that, like encode_utils.[c|h] for
> the new files.

I like that better.

>> Couldn't md5_crypt_verify() be made more general and take the hash type?
>>  For instance, password_crypt_verify() with the last param as the new
>> password type enum.
> 
> This would mean incorporating the whole SASL message exchange into
> this routine because the password string is part of the scram
> initialization context, and it seems to me that it is better to just
> do once a lookup at the entry in pg_authid. So we'd finish with a more
> confusing code I am afraid. At least that's the conclusion I came up
> with when doing that.. md5_crypt_verify does only the work on a
> received password.

Ah, yes, I see now.  I missed that when I reviewed patch 6.

-- 
-David
david@pgmasters.net