From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: David Steele <david@pgmasters.net>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Fetter <david@fetter.org>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Magnus Hagander <magnus@hagander.net>
Cc: Julian Markwort <julian.markwort@uni-muenster.de>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Cc: Valery Popov <v.popov@postgrespro.ru>
Subject: Re: Password identifiers, protocol aging and SCRAM protocol
Date: Fri, 9 Dec 2016 11:51:45 +0200
Message-ID: <3029e460-d47c-710e-507e-d8ba759d7cbb@iki.fi> (raw)
In-Reply-To: <CAB7nPqQtD7jOpYCsB4Ga=Cz+tgh4Ngac6NAV-WLs9zxaw0Uu7w@mail.gmail.com>
References: <947b9aff-8fdb-dbf5-a99c-0ffd4523a73f@2ndquadrant.com>
<CA+TgmoaeBqkXhmuu5n1AbTyC7Lz9WPHMkOxnF_hvbqpz-Sf_sA@mail.gmail.com>
<20161117003628.xonaz4p3lavrif6u@alap3.anarazel.de>
<CA+TgmoZjjvDvdwkDDwVdi5D+GkDheU87y0YRug5wHwODrmcyAQ@mail.gmail.com>
<CAB7nPqRkUxmNzkho_5hNQhoH-CutPvSM7pvMToZkJt8QVRM08g@mail.gmail.com>
<CAB7nPqQ3SasLdKfUDFFCdpDLxXG6+4+_8MEgivWzGGaZP4MkAQ@mail.gmail.com>
<CA+TgmoYydfT0ZwPaCMZannstRG207Ymf3s_aWTxkNJx9SBfm4Q@mail.gmail.com>
<CAB7nPqTAKNSBT6zzX8H0R8EwJ24Y8YsPmOqb3j38uzCoU_3+=A@mail.gmail.com>
<CAB7nPqSNMvJO50nJFoprf-wMMa281gJ-G6hfOr+krm8Zh8WLBg@mail.gmail.com>
<CAB7nPqRF=kVxqtC+Ei_t7eTrMti_0P+xUugSOd_7-rcLvREYzw@mail.gmail.com>
<e139fcbc-cfb2-3a64-93ac-afd5ba342960@iki.fi>
<CAB7nPqSqEwRgmVCS_OW3sJn6baymQ6MsO3kKMRvHLC0RZaKaYA@mail.gmail.com>
<21eecdb8-365b-8347-1ae2-5278d1fe798c@iki.fi>
<CAB7nPqT2UGWm0ukdomC+mG-co6nmKer62vuSGRxwtc+jS8uQPA@mail.gmail.com>
<CAB7nPqQtD7jOpYCsB4Ga=Cz+tgh4Ngac6NAV-WLs9zxaw0Uu7w@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:0001-Refactor-the-code-for-verifying-user-s-password.patch (text/x-diff, 11.2K) download
From 30be98cf09e8807d477827257a1e55c979dbe877 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 9 Dec 2016 11:49:36 +0200
Subject: [PATCH 1/1] Refactor the code for verifying user's password.
Split md5_crypt_verify() into three functions:
* get_role_password() to fetch user's password from pg_authid, and check
its expiration.
* md5_crypt_verify() to check an MD5 authentication challenge
* plain_crypt_verify() to check a plaintext password.
get_role_password() will be needed as a separate function by the upcoming
SCRAM authentication patch set. Most of the remaining functionality in
md5_crypt_verify() was different for MD5 and plaintext authentication, so
split that for readability.
While we're at it, simplify the *_crypt_verify functions by using
stack-allocated buffers to hold the temporary MD5 hashes, instead of
pallocing.
---
src/backend/libpq/auth.c | 18 +++-
src/backend/libpq/crypt.c | 214 ++++++++++++++++++++++++++++------------------
src/include/libpq/crypt.h | 9 +-
3 files changed, 151 insertions(+), 90 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.cindex f8bffe3..5c9ee06 100644--- a/src/backend/libpq/auth.c+++ b/src/backend/libpq/auth.c@@ -707,6 +707,7 @@ CheckMD5Auth(Port *port, char **logdetail)
{
char md5Salt[4]; /* Password salt */
char *passwd;
+ char *shadow_pass;
int result;
if (Db_user_namespace)
@@ -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 */
- result = md5_crypt_verify(port->user_name, passwd, md5Salt, 4, logdetail);+ result = get_role_password(port->user_name, &shadow_pass, logdetail);+ if (result == STATUS_OK)+ result = md5_crypt_verify(port->user_name, shadow_pass, passwd,+ md5Salt, 4, logdetail);+ if (shadow_pass)+ pfree(shadow_pass);
pfree(passwd);
return result;
@@ -741,16 +746,21 @@ CheckPasswordAuth(Port *port, char **logdetail)
{
char *passwd;
int result;
+ char *shadow_pass;
sendAuthRequest(port, AUTH_REQ_PASSWORD, NULL, 0);
passwd = recv_password_packet(port);
-
if (passwd == NULL)
return STATUS_EOF; /* client wouldn't send password */
- result = md5_crypt_verify(port->user_name, passwd, NULL, 0, logdetail);+ result = get_role_password(port->user_name, &shadow_pass, logdetail);+ if (result == STATUS_OK)+ result = plain_crypt_verify(port->user_name, shadow_pass, passwd,+ logdetail);+ if (shadow_pass)+ pfree(shadow_pass);
pfree(passwd);
return result;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.cindex b4ca174..fb6d1af 100644--- a/src/backend/libpq/crypt.c+++ b/src/backend/libpq/crypt.c@@ -30,28 +30,28 @@
/*
- * Check given password for given user, and return STATUS_OK or STATUS_ERROR.+ * Fetch stored password for a user, for authentication.
*
- * 'client_pass' is the password response given by the remote user. If- * 'md5_salt' is not NULL, it is a response to an MD5 authentication- * challenge, with the given salt. Otherwise, it is a plaintext password.+ * Returns STATUS_OK on success. On error, returns STATUS_ERROR, and stores+ * a palloc'd string describing the reason, for the postmaster log, in+ * *logdetail. The error reason should *not* be sent to the client, to avoid+ * giving away user information!
*
- * In the error case, optionally store a palloc'd string at *logdetail- * that will be sent to the postmaster log (but not the client).+ * If the password is expired, it is still returned in *shadow_pass, but the+ * return code is STATUS_ERROR. On other errors, *shadow_pass is set to+ * NULL.
*/
int
-md5_crypt_verify(const char *role, char *client_pass,- char *md5_salt, int md5_salt_len, char **logdetail)+get_role_password(const char *role, char **shadow_pass, char **logdetail)
{
int retval = STATUS_ERROR;
- char *shadow_pass,- *crypt_pwd;
TimestampTz vuntil = 0;
- char *crypt_client_pass = client_pass;
HeapTuple roleTup;
Datum datum;
bool isnull;
+ *shadow_pass = NULL;+
/* Get role info from pg_authid */
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(role));
if (!HeapTupleIsValid(roleTup))
@@ -70,7 +70,7 @@ md5_crypt_verify(const char *role, char *client_pass,
role);
return STATUS_ERROR; /* user has no password */
}
- 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 */
}
/*
- * Compare with the encrypted or plain password depending on the- * authentication method being used for this connection. (We do not- * bother setting logdetail for pg_md5_encrypt failure: the only possible- * error is out-of-memory, which is unlikely, and if it did happen adding- * a psprintf call would only make things worse.)+ * Password OK, now check to be sure we are not past rolvaliduntil
*/
- if (md5_salt)+ if (isnull)+ retval = STATUS_OK;+ else if (vuntil < GetCurrentTimestamp())
{
- /* MD5 authentication */- Assert(md5_salt_len > 0);- crypt_pwd = palloc(MD5_PASSWD_LEN + 1);- if (isMD5(shadow_pass))- {- /* stored password already encrypted, only do salt */- if (!pg_md5_encrypt(shadow_pass + strlen("md5"),- md5_salt, md5_salt_len,- crypt_pwd))- {- pfree(crypt_pwd);- return STATUS_ERROR;- }- }- else+ *logdetail = psprintf(_("User \"%s\" has an expired password."),+ role);+ retval = STATUS_ERROR;+ }+ else+ retval = STATUS_OK;++ return retval;+}++/*+ * Check MD5 authentication response, and return STATUS_OK or STATUS_ERROR.+ *+ * 'shadow_pass' is the user's correct password or password hash, as stored+ * in pg_authid.rolpassword.+ * 'client_pass' is the response given by the remote user to the MD5 challenge.+ * 'md5_salt' is the salt used in the MD5 authentication challenge.+ *+ * In the error case, optionally store a palloc'd string at *logdetail+ * that will be sent to the postmaster log (but not the client).+ */+int+md5_crypt_verify(const char *role, const char *shadow_pass,+ const char *client_pass,+ const char *md5_salt, int md5_salt_len,+ char **logdetail)+{+ int retval;+ char crypt_pwd[MD5_PASSWD_LEN + 1];+ char crypt_pwd2[MD5_PASSWD_LEN + 1];++ Assert(md5_salt_len > 0);++ /*+ * Compute the correct answer for the MD5 challenge.+ *+ * We do not bother setting logdetail for any pg_md5_encrypt failure+ * below: the only possible error is out-of-memory, which is unlikely, and+ * if it did happen adding a psprintf call would only make things worse.+ */+ if (isMD5(shadow_pass))+ {+ /* stored password already encrypted, only do salt */+ if (!pg_md5_encrypt(shadow_pass + strlen("md5"),+ md5_salt, md5_salt_len,+ crypt_pwd))
{
- /* stored password is plain, double-encrypt */- char *crypt_pwd2 = palloc(MD5_PASSWD_LEN + 1);-- if (!pg_md5_encrypt(shadow_pass,- role,- strlen(role),- crypt_pwd2))- {- pfree(crypt_pwd);- pfree(crypt_pwd2);- return STATUS_ERROR;- }- if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),- md5_salt, md5_salt_len,- crypt_pwd))- {- pfree(crypt_pwd);- pfree(crypt_pwd2);- return STATUS_ERROR;- }- pfree(crypt_pwd2);+ return STATUS_ERROR;
}
}
else
{
- /* Client sent password in plaintext */- if (isMD5(shadow_pass))+ /* stored password is plain, double-encrypt */+ if (!pg_md5_encrypt(shadow_pass,+ role,+ strlen(role),+ crypt_pwd2))
{
- /* Encrypt user-supplied password to match stored MD5 */- crypt_client_pass = palloc(MD5_PASSWD_LEN + 1);- if (!pg_md5_encrypt(client_pass,- role,- strlen(role),- crypt_client_pass))- {- pfree(crypt_client_pass);- return STATUS_ERROR;- }+ return STATUS_ERROR;+ }+ if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),+ md5_salt, md5_salt_len,+ crypt_pwd))+ {+ return STATUS_ERROR;
}
- crypt_pwd = shadow_pass;
}
- if (strcmp(crypt_client_pass, crypt_pwd) == 0)+ if (strcmp(client_pass, crypt_pwd) == 0)+ retval = STATUS_OK;+ else
{
- /*- * Password OK, now check to be sure we are not past rolvaliduntil- */- if (isnull)- retval = STATUS_OK;- else if (vuntil < GetCurrentTimestamp())+ *logdetail = psprintf(_("Password does not match for user \"%s\"."),+ role);+ retval = STATUS_ERROR;+ }++ return retval;+}++/*+ * Check given password for given user, and return STATUS_OK or STATUS_ERROR.+ *+ * 'shadow_pass' is the user's correct password or password hash, as stored+ * in pg_authid.rolpassword.+ * 'client_pass' is the password given by the remote user+ *+ * In the error case, optionally store a palloc'd string at *logdetail+ * that will be sent to the postmaster log (but not the client).+ */+int+plain_crypt_verify(const char *role, const char *shadow_pass,+ const char *client_pass,+ char **logdetail)+{+ int retval;+ char crypt_client_pass[MD5_PASSWD_LEN + 1];++ /*+ * Client sent password in plaintext. If we have an MD5 hash stored, hash+ * the password the client sent, and compare the hashes. Otherwise+ * compare the plaintext passwords directly.+ */+ if (isMD5(shadow_pass))+ {+ if (!pg_md5_encrypt(client_pass,+ role,+ strlen(role),+ crypt_client_pass))
{
- *logdetail = psprintf(_("User \"%s\" has an expired password."),- role);- retval = STATUS_ERROR;+ /*+ * We do not bother setting logdetail for pg_md5_encrypt failure:+ * the only possible error is out-of-memory, which is unlikely,+ * and if it did happen adding a psprintf call would only make+ * things worse.+ */+ return STATUS_ERROR;
}
- else- retval = STATUS_OK;+ client_pass = crypt_client_pass;
}
++ if (strcmp(client_pass, shadow_pass) == 0)+ retval = STATUS_OK;
else
+ {
*logdetail = psprintf(_("Password does not match for user \"%s\"."),
role);
-- if (crypt_pwd != shadow_pass)- pfree(crypt_pwd);- if (crypt_client_pass != client_pass)- pfree(crypt_client_pass);+ retval = STATUS_ERROR;+ }
return retval;
}
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.hindex 4ca8a75..229ce76 100644--- a/src/include/libpq/crypt.h+++ b/src/include/libpq/crypt.h@@ -15,7 +15,12 @@
#include "datatype/timestamp.h"
-extern int md5_crypt_verify(const char *role, char *client_pass,- char *md5_salt, int md5_salt_len, char **logdetail);+extern int get_role_password(const char *role, char **shadow_pass, char **logdetail);++extern int md5_crypt_verify(const char *role, const char *shadow_pass,+ const char *client_pass, const char *md5_salt,+ int md5_salt_len, char **logdetail);+extern int plain_crypt_verify(const char *role, const char *shadow_pass,+ const char *client_pass, char **logdetail);
#endif
--
2.10.2
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, michael.paquier@gmail.com, robertmhaas@gmail.com, andres@anarazel.de, peter.eisentraut@2ndquadrant.com, david@pgmasters.net, tgl@sss.pgh.pa.us, david@fetter.org, alvherre@2ndquadrant.com, magnus@hagander.net, julian.markwort@uni-muenster.de, sfrost@snowman.net, v.popov@postgrespro.ru
Subject: Re: Password identifiers, protocol aging and SCRAM protocol
In-Reply-To: <3029e460-d47c-710e-507e-d8ba759d7cbb@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox