Horton

Re: Password identifiers, protocol aging and SCRAM protocol

Michael Paquier <michael.paquier@gmail.com>

View thread View on postgr.es
From: Michael Paquier <michael.paquier@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
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-09T11:10:41Z
Lists: pgsql-hackers 
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

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.