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
signature.asc
Description: PGP signature
