Re: Password identifiers, protocol aging and SCRAM protocol

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.

Here are some comments.

> @@ -720,12 +721,16 @@ CheckMD5Auth(Port *port, char **logdetail)
> sendAuthRequest(port, AUTH_REQ_MD5, md5Salt, 4);
>
> passwd = recv_password_packet(port);
> -
> if (passwd == NULL)
> return STATUS_EOF; /* client wouldn't send password */

This looks like useless noise.

> - 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.
--
Michael