Re: Password identifiers, protocol aging and SCRAM protocol

From: David Steele <david(at)pgmasters(dot)net>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, David Fetter <david(at)fetter(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Magnus Hagander <magnus(at)hagander(dot)net>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Julian Markwort <julian(dot)markwort(at)uni-muenster(dot)de>, Stephen Frost <sfrost(at)snowman(dot)net>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Valery Popov <v(dot)popov(at)postgrespro(dot)ru>
Subject: Re: Password identifiers, protocol aging and SCRAM protocol
Date: 2016-09-27 14:55:55
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

> On Mon, Sep 26, 2016 at 2:15 AM, David Steele <david(at)pgmasters(dot)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(at)aarongifford(dot)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...


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


In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-09-27 15:39:55 Re: Fix some corner cases that cube_in rejects
Previous Message Kevin Grittner 2016-09-27 14:55:18 Re: Redesigning parallel dump/restore's wait-for-workers logic