Re: Password identifiers, protocol aging and SCRAM protocol

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