Re: [HACKERS] [PATCH] Refactoring: rename md5Salt to pwsalt

2016-09-30 Thread Michael Paquier
On Fri, Sep 30, 2016 at 9:40 PM, Tom Lane  wrote:
> Aleksander Alekseev  writes:
>> Suggested patch (first of many, I hope) renames `md5Salt` to more
>> general `pwsalt`.
>> Does it sound reasonable?
>
> I'm dubious.  The main problem with supposing that port->md5Salt
> can serve other purposes is its fixed size.  I think you're likely
> going to have to change that representation at some point (eg
> make it a separately-palloc'd field).  My inclination would be to
> do the field renaming at the same time you change the representation,
> since that provides a convenient way to ensure you've caught every
> place that has to change.

SCRAM is going to use more than 4 bytes here. RFC5802 does not given
directly a length, the last set of patches has been using 10 bytes,
but at the end we are very likely to use more than that, and not 4 for
sure.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Refactoring: rename md5Salt to pwsalt

2016-09-30 Thread Tom Lane
Aleksander Alekseev  writes:
> Suggested patch (first of many, I hope) renames `md5Salt` to more
> general `pwsalt`.
> Does it sound reasonable?

I'm dubious.  The main problem with supposing that port->md5Salt
can serve other purposes is its fixed size.  I think you're likely
going to have to change that representation at some point (eg
make it a separately-palloc'd field).  My inclination would be to
do the field renaming at the same time you change the representation,
since that provides a convenient way to ensure you've caught every
place that has to change.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] [PATCH] Refactoring: rename md5Salt to pwsalt

2016-09-30 Thread Aleksander Alekseev
Hello.

Since there are plans/efforts to introduce additional authorization
methods in nearest feature I suggest to refactor the code so it
wouldn't mention md5 when it possible. `md5Salt` for instance could be
not only "md5 salt" but also "sha2 salt", etc - depending on what
authorization method was chosen.

Suggested patch (first of many, I hope) renames `md5Salt` to more
general `pwsalt`.

Does it sound reasonable?

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 0ba8530..25bb4c2 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -536,7 +536,7 @@ ClientAuthentication(Port *port)
 		(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 		 errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled")));
 			/* include the salt to use for computing the response */
-			sendAuthRequest(port, AUTH_REQ_MD5, port->md5Salt, 4);
+			sendAuthRequest(port, AUTH_REQ_MD5, port->pwsalt, 4);
 			status = recv_and_check_password_packet(port, &logdetail);
 			break;
 
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index d84a180..98f3315 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -96,8 +96,8 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass,
 			{
 /* stored password already encrypted, only do salt */
 if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
-	port->md5Salt,
-	sizeof(port->md5Salt), crypt_pwd))
+	port->pwsalt,
+	sizeof(port->pwsalt), crypt_pwd))
 {
 	pfree(crypt_pwd);
 	return STATUS_ERROR;
@@ -118,8 +118,8 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass,
 	return STATUS_ERROR;
 }
 if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
-	port->md5Salt,
-	sizeof(port->md5Salt),
+	port->pwsalt,
+	sizeof(port->pwsalt),
 	crypt_pwd))
 {
 	pfree(crypt_pwd);
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 0c0a609..b7ab8dd 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2350,7 +2350,7 @@ ConnCreate(int serverFd)
 	 * after.  Else the postmaster's random sequence won't get advanced, and
 	 * all backends would end up using the same salt...
 	 */
-	RandomSalt(port->md5Salt, sizeof(port->md5Salt));
+	RandomSalt(port->pwsalt, sizeof(port->pwsalt));
 
 	/*
 	 * Allocate GSSAPI specific state struct
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index b91eca5..6b7935c 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -144,7 +144,7 @@ typedef struct Port
 	 * Information that needs to be held during the authentication cycle.
 	 */
 	HbaLine*hba;
-	char		md5Salt[4];		/* Password salt */
+	char		pwsalt[4];		/* Password salt */
 
 	/*
 	 * Information that really has no business at all being in struct Port,
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 404bc93..9123d5b 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -522,8 +522,8 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
 	free(crypt_pwd);
 	return STATUS_ERROR;
 }
-if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"), conn->md5Salt,
-	sizeof(conn->md5Salt), crypt_pwd))
+if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"), conn->pwsalt,
+	sizeof(conn->pwsalt), crypt_pwd))
 {
 	free(crypt_pwd);
 	return STATUS_ERROR;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f3a9e5a..7529fd5 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2441,8 +2441,8 @@ keep_going:		/* We will come back to here until there is
 /* Get the password salt if there is one. */
 if (areq == AUTH_REQ_MD5)
 {
-	if (pqGetnchar(conn->md5Salt,
-   sizeof(conn->md5Salt), conn))
+	if (pqGetnchar(conn->pwsalt,
+   sizeof(conn->pwsalt), conn))
 	{
 		/* We'll come back when there are more data */
 		return PGRES_POLLING_READING;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index be6c370..1e18688 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -389,7 +389,7 @@ struct pg_conn
 	/* Miscellaneous stuff */
 	int			be_pid;			/* PID of backend --- needed for cancels */
 	int			be_key;			/* key of backend --- needed for cancels */
-	char		md5Salt[4];		/* password salt received from backend */
+	char		pwsalt[4];		/* password salt received from backend */
 	pgParameterStatus *pstatus; /* ParameterStatus data */
 	int			client_encoding;	/* encoding id */
 	bool		std_strings;	/* standard_conforming_strings */


pgpHAHfrE45d_.pgp
Description: OpenPGP digital signature