Code review for MD5 authorization patch. Clean up some breakage

(salts were always zero!?), add much missing documentation.

8 files changed, 57 insertions, 54 deletions

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index e3c2a04a9be..96bb8f0c572 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/libpq/auth.c,v 1.66 2001/09/07 19:52:53 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/libpq/auth.c,v 1.67 2001/09/21 20:31:45 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -594,15 +594,11 @@ sendAuthRequest(Port *port, AuthRequest areq)
 	/* Add the salt for encrypted passwords. */
 	if (areq == AUTH_REQ_MD5)
 	{
-		pq_sendint(&buf, port->md5Salt[0], 1);
-		pq_sendint(&buf, port->md5Salt[1], 1);
-		pq_sendint(&buf, port->md5Salt[2], 1);
-		pq_sendint(&buf, port->md5Salt[3], 1);
+		pq_sendbytes(&buf, port->md5Salt, 4);
 	}
-	if (areq == AUTH_REQ_CRYPT)
+	else if (areq == AUTH_REQ_CRYPT)
 	{
-		pq_sendint(&buf, port->cryptSalt[0], 1);
-		pq_sendint(&buf, port->cryptSalt[1], 1);
+		pq_sendbytes(&buf, port->cryptSalt, 2);
 	}
 
 	pq_endmessage(&buf);
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 8f2a1f92439..1d6b80a2642 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -9,7 +9,7 @@
  * Dec 17, 1997 - Todd A. Brandys
  *	Orignal Version Completed.
  *
- * $Id: crypt.c,v 1.37 2001/08/17 15:40:07 momjian Exp $
+ * $Id: crypt.c,v 1.38 2001/09/21 20:31:45 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -282,7 +282,7 @@ md5_crypt_verify(const Port *port, const char *user, const char *pgpass)
 	{
 		snprintf(PQerrormsg, PQERRORMSG_LENGTH,
 			"Password is stored MD5 encrypted.  "
-			"Only pg_hba.conf's MD5 protocol can be used for this user.\n");
+			"'password' and 'crypt' auth methods cannot be used.\n");
 		fputs(PQerrormsg, stderr);
 		pqdebug("%s", PQerrormsg);
 		return STATUS_ERROR;
@@ -339,7 +339,7 @@ md5_crypt_verify(const Port *port, const char *user, const char *pgpass)
 			break;
 	}
 
-	if (!strcmp(pgpass, crypt_pwd))
+	if (strcmp(pgpass, crypt_pwd) == 0)
 	{
 		/*
 		 * check here to be sure we are not past valuntil
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 1fa235bd2ac..891fcb4317f 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/libpq/hba.c,v 1.71 2001/09/07 19:59:04 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/libpq/hba.c,v 1.72 2001/09/21 20:31:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -208,8 +208,8 @@ free_lines(List **lines)
  *  *error_p.  line points to the next token of the line.
  */
 static void
-parse_hba_auth(List *line, ProtocolVersion proto, UserAuth *userauth_p,
-				char *auth_arg, bool *error_p)
+parse_hba_auth(List *line, UserAuth *userauth_p, char *auth_arg,
+			   bool *error_p)
 {
 	char		*token;
 
@@ -295,8 +295,7 @@ parse_hba(List *line, hbaPort *port, bool *found_p, bool *error_p)
 		line = lnext(line);
 		if (!line)
 			goto hba_syntax;
-		parse_hba_auth(line, port->proto, &port->auth_method,
-					   port->auth_arg, error_p);
+		parse_hba_auth(line, &port->auth_method, port->auth_arg, error_p);
 		if (*error_p)
 			goto hba_syntax;
 
@@ -365,8 +364,7 @@ parse_hba(List *line, hbaPort *port, bool *found_p, bool *error_p)
 		line = lnext(line);
 		if (!line)
 			goto hba_syntax;
-		parse_hba_auth(line, port->proto, &port->auth_method,
-					   port->auth_arg, error_p);
+		parse_hba_auth(line, &port->auth_method, port->auth_arg, error_p);
 		if (*error_p)
 			goto hba_syntax;
 
diff --git a/src/backend/libpq/md5.c b/src/backend/libpq/md5.c
index 16a0ed5817c..ad5b4c91ec4 100644
--- a/src/backend/libpq/md5.c
+++ b/src/backend/libpq/md5.c
@@ -9,27 +9,20 @@
  *  generating hashed passwords from limited input.
  *
  *  Sverre H. Huseby <sverrehu@online.no>
+ *
+ * $Header: /cvsroot/pgsql/src/backend/libpq/md5.c,v 1.6 2001/09/21 20:31:47 tgl Exp $
  */
 
+#include "postgres.h"
 
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
 #include <errno.h>
 
-#include "postgres.h"
 #include "libpq/crypt.h"
 
 /*
  *	PRIVATE FUNCTIONS
  */
 
-#ifdef FRONTEND
-#undef palloc
-#define palloc malloc
-#undef pfree
-#define pfree free
-#endif
 
 /*
  *	The returned array is allocated using malloc.  the caller should free it
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e8c9ae70efd..40ff1a661b3 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -37,7 +37,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v 1.242 2001/09/21 17:06:12 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v 1.243 2001/09/21 20:31:48 tgl Exp $
  *
  * NOTES
  *
@@ -1235,6 +1235,14 @@ ConnCreate(int serverFd)
 	}
 	else
 	{
+		/*
+		 * Precompute password salt values to use for this connection.
+		 * It's slightly annoying to do this long in advance of knowing
+		 * whether we'll need 'em or not, but we must do the random()
+		 * calls before we fork, not after.  Else the postmaster's random
+		 * sequence won't get advanced, and all backends would end up
+		 * using the same salt...
+		 */
 		RandomSalt(port->cryptSalt, port->md5Salt);
 		port->pktInfo.state = Idle;
 	}
@@ -2145,16 +2153,16 @@ schedule_checkpoint(SIGNAL_ARGS)
 
 
 /*
- * CharRemap
+ * CharRemap: given an int in range 0..61, produce textual encoding of it
+ * per crypt(3) conventions.
  */
 static char
-CharRemap(long int ch)
+CharRemap(long ch)
 {
-
 	if (ch < 0)
 		ch = -ch;
-
 	ch = ch % 62;
+
 	if (ch < 26)
 		return 'A' + ch;
 
@@ -2176,13 +2184,22 @@ RandomSalt(char *cryptSalt, char *md5Salt)
 
 	cryptSalt[0] = CharRemap(rand % 62);
 	cryptSalt[1] = CharRemap(rand / 62);
-	/* Grab top 16-bits of two random runs so as not to send full
-	   random value over the network.  The high-order bits are more random. */
-	md5Salt[0] = rand & 0xff000000;
-	md5Salt[1] = rand & 0x00ff0000;
+	/*
+	 * It's okay to reuse the first random value for one of the MD5 salt bytes,
+	 * since only one of the two salts will be sent to the client.  After that
+	 * we need to compute more random bits.
+	 *
+	 * We use % 255, sacrificing one possible byte value, so as to ensure
+	 * that all bits of the random() value participate in the result.  While
+	 * at it, add one to avoid generating any null bytes.
+	 */
+	md5Salt[0] = (rand % 255) + 1;
+	rand = PostmasterRandom();
+	md5Salt[1] = (rand % 255) + 1;
+	rand = PostmasterRandom();
+	md5Salt[2] = (rand % 255) + 1;
 	rand = PostmasterRandom();
-	md5Salt[2] = rand & 0xff000000;
-	md5Salt[3] = rand & 0x00ff0000;
+	md5Salt[3] = (rand % 255) + 1;
 }
 
 /*
@@ -2200,7 +2217,7 @@ PostmasterRandom(void)
 		initialized = true;
 	}
 
-	return random() ^ random_seed;
+	return random();
 }
 
 /*
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 6525d5ecb3f..840bc3e05a4 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -4,7 +4,7 @@
  *	  Interface to hba.c
  *
  *
- * $Id: hba.h,v 1.26 2001/09/06 03:23:38 momjian Exp $
+ * $Id: hba.h,v 1.27 2001/09/21 20:31:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -31,9 +31,6 @@
 
 typedef enum UserAuth
 {
-#ifdef USE_PAM
-	uaPAM,
-#endif /* USE_PAM */
 	uaReject,
 	uaKrb4,
 	uaKrb5,
@@ -41,7 +38,10 @@ typedef enum UserAuth
 	uaIdent,
 	uaPassword,
 	uaCrypt,
-	uaMD5
+	uaMD5,
+#ifdef USE_PAM
+	uaPAM
+#endif /* USE_PAM */
 } UserAuth;
 
 typedef struct Port hbaPort;
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index a13292c70f3..c603a2b6a83 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -4,7 +4,7 @@
 #
 # Copyright (c) 1994, Regents of the University of California
 #
-# $Header: /cvsroot/pgsql/src/interfaces/libpq/Makefile,v 1.57 2001/09/06 04:57:30 ishii Exp $
+# $Header: /cvsroot/pgsql/src/interfaces/libpq/Makefile,v 1.58 2001/09/21 20:31:48 tgl Exp $
 #
 #-------------------------------------------------------------------------
 
@@ -84,5 +84,5 @@ uninstall: uninstall-lib
 	rm -f $(DESTDIR)$(includedir)/libpq-fe.h $(DESTDIR)$(includedir_internal)/libpq-int.h $(includedir_internal)/pqexpbuffer.h
 
 clean distclean maintainer-clean: clean-lib
-	rm -f $(OBJS) dllist.c md5.c md5.h wchar.c encnames.c
+	rm -f $(OBJS) dllist.c md5.c wchar.c encnames.c
 	rm -f $(OBJS) inet_aton.c snprintf.c strerror.c
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index e496f7c3ee5..bb60bb1ceb9 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -10,7 +10,7 @@
  * exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes).
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v 1.59 2001/09/07 19:52:54 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v 1.60 2001/09/21 20:31:49 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -30,12 +30,6 @@
 
 #include "postgres_fe.h"
 
-/* XXX is there a reason these appear before the system defines? */
-#include "libpq-fe.h"
-#include "libpq-int.h"
-#include "fe-auth.h"
-#include "libpq/crypt.h"
-
 #ifdef WIN32
 #include "win32.h"
 #else
@@ -59,6 +53,11 @@
 #include <crypt.h>
 #endif
 
+#include "libpq-fe.h"
+#include "libpq-int.h"
+#include "fe-auth.h"
+#include "libpq/crypt.h"
+
 
 /*
  * common definitions for generic fe/be routines