Horton

Re: Password identifiers, protocol aging and SCRAM protocol

Heikki Linnakangas <hlinnaka@iki.fi>

View thread View on postgr.es
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, David Steele <david@pgmasters.net>, Tom Lane <tgl@sss.pgh.pa.us>, David Fetter <david@fetter.org>, Alvaro Herrera <alvherre@2ndquadrant.com>, Magnus Hagander <magnus@hagander.net>, Julian Markwort <julian.markwort@uni-muenster.de>, Stephen Frost <sfrost@snowman.net>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>, Valery Popov <v.popov@postgrespro.ru>
Date: 2016-12-12T10:52:21Z
Lists: pgsql-hackers 
On 12/09/2016 01:10 PM, Michael Paquier wrote:
> On Fri, Dec 09, 2016 at 11:51:45AM +0200, Heikki Linnakangas wrote:
>> On 12/09/2016 05:58 AM, Michael Paquier wrote:
>>>
>>> One thing is: when do we look up at pg_authid? After receiving the
>>> first message from client or before beginning the exchange? As the
>>> first message from client has the user name, it would make sense to do
>>> the lookup after receiving it, but from PG prospective it would just
>>> make sense to use the data already present in the startup packet. The
>>> current patch does the latter. What do you think?
>>
>> While hacking on this, I came up with the attached refactoring, against
>> current master. I think it makes the current code more readable, anyway, and
>> it provides a get_role_password() function that SCRAM can use, to look up
>> the stored password. (This is essentially the same refactoring that was
>> included in the SCRAM patch set, that introduced the get_role_details()
>> function.)
>>
>> Barring objections, I'll go ahead and commit this first.

Ok, committed.

>> -	shadow_pass = TextDatumGetCString(datum);
>> +	*shadow_pass = TextDatumGetCString(datum);
>>
>>  	datum = SysCacheGetAttr(AUTHNAME, roleTup,
>>  							Anum_pg_authid_rolvaliduntil, &isnull);
>> @@ -83,100 +83,146 @@ md5_crypt_verify(const char *role, char *client_pass,
>>  	{
>>  		*logdetail = psprintf(_("User \"%s\" has an empty password."),
>>  							  role);
>> +		*shadow_pass = NULL;
>>  		return STATUS_ERROR;	/* empty password */
>>  	}
>
> Here the password is allocated by text_to_cstring(), that's only 1 byte
> but it should be free()'d.

Fixed. Thanks, good catch! It doesn't matter in practice as we'll 
disconnect shortly afterwards anyway, but given that the callers pfree() 
other things on error, let's be tidy.

- Heikki

Commits

  1. Support SCRAM-SHA-256 authentication (RFC 5802 and 7677).

  2. Refactor SHA2 functions and move them to src/common/.

  3. Replace isMD5() with a more future-proof way to check if pw is encrypted.

  4. Remove bogus notice that older clients might not work with MD5 passwords.

  5. Refactor the code for verifying user's password.

  6. Replace PostmasterRandom() with a stronger source, second attempt.

  7. Remove support for (insecure) crypt authentication.