Frontend/Backend Protocol: SSL / GSS Protocol Negotiation Problem

From 8e7f4e0326421e5a9a70f8d20c90423f31910cd4 Mon Sep 17 00:00:00 2001
From: Jakob Egger <jakob@eggerapps.at>
Date: Fri, 6 Dec 2019 12:33:44 +0100
Subject: [PATCH 1/2] Allow multiple ssl/gss negotiation attempts

---
 src/backend/postmaster/postmaster.c | 36 ++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9ff2832c00..490130691b 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -404,7 +404,7 @@ static void BackendRun(Port *port) pg_attribute_noreturn();
 static void ExitPostmaster(int status) pg_attribute_noreturn();
 static int	ServerLoop(void);
 static int	BackendStartup(Port *port);
-static int	ProcessStartupPacket(Port *port, bool secure_done);
+static int	ProcessStartupPacket(Port *port, bool secure_done, bool allow_ssl, bool allow_gss);
 static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
 static void processCancelRequest(Port *port, void *pkt);
 static int	initMasks(fd_set *rmask);
@@ -1918,7 +1918,7 @@ initMasks(fd_set *rmask)
  * GSSAPI) is already completed.
  */
 static int
-ProcessStartupPacket(Port *port, bool secure_done)
+ProcessStartupPacket(Port *port, bool secure_done, bool allow_ssl, bool allow_gss)
 {
 	int32		len;
 	void	   *buf;
@@ -2009,7 +2009,7 @@ ProcessStartupPacket(Port *port, bool secure_done)
 
 #ifdef USE_SSL
 		/* No SSL when disabled or on Unix sockets */
-		if (!LoadedSSL || IS_AF_UNIX(port->laddr.addr.ss_family))
+		if (!LoadedSSL || IS_AF_UNIX(port->laddr.addr.ss_family) || !allow_ssl)
 			SSLok = 'N';
 		else
 			SSLok = 'S';		/* Support for SSL */
@@ -2029,19 +2029,26 @@ retry1:
 		}
 
 #ifdef USE_SSL
-		if (SSLok == 'S' && secure_open_server(port) == -1)
-			return STATUS_ERROR;
+		if (SSLok == 'S') {
+			if (secure_open_server(port) == -1) {
+				return STATUS_ERROR;
+			}
+			else
+			{
+				secure_done = true;
+			}
+		}
 #endif
 		/* regular startup packet, cancel, etc packet should follow... */
 		/* but not another SSL negotiation request */
-		return ProcessStartupPacket(port, true);
+		return ProcessStartupPacket(port, secure_done, false, allow_gss);
 	}
 	else if (proto == NEGOTIATE_GSS_CODE && !secure_done)
 	{
 		char		GSSok = 'N';
 #ifdef ENABLE_GSS
 		/* No GSSAPI encryption when on Unix socket */
-		if (!IS_AF_UNIX(port->laddr.addr.ss_family))
+		if (!IS_AF_UNIX(port->laddr.addr.ss_family) && allow_gss)
 			GSSok = 'G';
 #endif
 
@@ -2056,11 +2063,18 @@ retry1:
 		}
 
 #ifdef ENABLE_GSS
-		if (GSSok == 'G' && secure_open_gssapi(port) == -1)
-			return STATUS_ERROR;
+		if (GSSok == 'G') {
+			if (secure_open_gssapi(port) == -1) {
+				return STATUS_ERROR;
+			}
+			else
+			{
+				secure_done = true;
+			}
+		}
 #endif
 		/* Won't ever see more than one negotiation request */
-		return ProcessStartupPacket(port, true);
+		return ProcessStartupPacket(port, secure_done, allow_ssl, false);
 	}
 
 	/* Could add additional special packet types here */
@@ -4400,7 +4414,7 @@ BackendInitialize(Port *port)
 	 * Receive the startup packet (which might turn out to be a cancel request
 	 * packet).
 	 */
-	status = ProcessStartupPacket(port, false);
+	status = ProcessStartupPacket(port, false, true, true);
 
 	/*
 	 * Stop here if it was bad or a cancel packet.  ProcessStartupPacket
-- 
2.23.0

From b60a18d2bb7e9a45d33155099459b41938f06323 Mon Sep 17 00:00:00 2001
From: Jakob Egger <jakob@eggerapps.at>
Date: Fri, 6 Dec 2019 12:34:23 +0100
Subject: [PATCH 2/2] libpq: Retry after failed ssl/gss negotiation

When attempting to negotiate both GSS and SSL,
detect errors from old servers that don't support
multiple negotiation attempts.
---
 src/interfaces/libpq/fe-connect.c | 86 +++++++++++++++++++++----------
 src/interfaces/libpq/libpq-int.h  |  3 ++
 2 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 5c786360a9..e88771b7e6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1965,11 +1965,6 @@ connectDBStart(PGconn *conn)
 	 */
 	resetPQExpBuffer(&conn->errorMessage);
 
-#ifdef ENABLE_GSS
-	if (conn->gssencmode[0] == 'd') /* "disable" */
-		conn->try_gss = false;
-#endif
-
 	/*
 	 * Set up to try to connect to the first host.  (Setting whichhost = -1 is
 	 * a bit of a cheat, but PQconnectPoll will advance it to 0 before
@@ -2409,6 +2404,13 @@ keep_going:						/* We will come back to here until there is
 		conn->allow_ssl_try = (conn->sslmode[0] != 'd');	/* "disable" */
 		conn->wait_ssl_try = (conn->sslmode[0] == 'a'); /* "allow" */
 #endif
+		
+#ifdef ENABLE_GSS
+	if (conn->gssencmode[0] == 'd') /* "disable" */
+		conn->try_gss = false;
+#endif
+		
+		conn->did_negotiate = false;
 
 		reset_connection_state_machine = false;
 		need_new_connection = true;
@@ -2431,6 +2433,8 @@ keep_going:						/* We will come back to here until there is
 		/* Reset conn->status to put the state machine in the right state */
 		conn->status = CONNECTION_NEEDED;
 
+		conn->did_negotiate = false;
+
 		need_new_connection = false;
 	}
 
@@ -2971,24 +2975,37 @@ keep_going:						/* We will come back to here until there is
 							goto error_return;
 						}
 						/* Otherwise, proceed with normal startup */
+						conn->did_negotiate = true;
 						conn->allow_ssl_try = false;
 						conn->status = CONNECTION_MADE;
 						return PGRES_POLLING_WRITING;
 					}
 					else if (SSLok == 'E')
 					{
-						/*
-						 * Server failure of some sort, such as failure to
-						 * fork a backend process.  We need to process and
-						 * report the error message, which might be formatted
-						 * according to either protocol 2 or protocol 3.
-						 * Rather than duplicate the code for that, we flip
-						 * into AWAITING_RESPONSE state and let the code there
-						 * deal with it.  Note we have *not* consumed the "E"
-						 * byte here.
-						 */
-						conn->status = CONNECTION_AWAITING_RESPONSE;
-						goto keep_going;
+						if (conn->did_negotiate) {
+							/*
+							 * Postmaster used to allow only a single negotiation attempt.
+							 * If we get an error on the second negotiation attempt,
+							 * we reconnect and try again.
+							 */
+							need_new_connection = true;
+							goto keep_going;
+						}
+						else
+						{
+							/*
+							 * Server failure of some sort, such as failure to
+							 * fork a backend process.  We need to process and
+							 * report the error message, which might be formatted
+							 * according to either protocol 2 or protocol 3.
+							 * Rather than duplicate the code for that, we flip
+							 * into AWAITING_RESPONSE state and let the code there
+							 * deal with it.  Note we have *not* consumed the "E"
+							 * byte here.
+							 */
+							conn->status = CONNECTION_AWAITING_RESPONSE;
+							goto keep_going;
+						}
 					}
 					else
 					{
@@ -3061,17 +3078,29 @@ keep_going:						/* We will come back to here until there is
 
 					if (gss_ok == 'E')
 					{
-						/*
-						 * Server failure of some sort.  Assume it's a
-						 * protocol version support failure, and let's see if
-						 * we can't recover (if it's not, we'll get a better
-						 * error message on retry).  Server gets fussy if we
-						 * don't hang up the socket, though.
-						 */
-						conn->try_gss = false;
-						pqDropConnection(conn, true);
-						conn->status = CONNECTION_NEEDED;
-						goto keep_going;
+						if (conn->did_negotiate) {
+							/*
+							 * Postmaster used to allow only a single negotiation attempt.
+							 * If we get an error on the second negotiation attempt,
+							 * we reconnect and try again.
+							 */
+							need_new_connection = true;
+							goto keep_going;
+						}
+						else
+						{
+							/*
+							 * Server failure of some sort.  Assume it's a
+							 * protocol version support failure, and let's see if
+							 * we can't recover (if it's not, we'll get a better
+							 * error message on retry).  Server gets fussy if we
+							 * don't hang up the socket, though.
+							 */
+							conn->try_gss = false;
+							pqDropConnection(conn, true);
+							conn->status = CONNECTION_NEEDED;
+							goto keep_going;
+						}
 					}
 
 					/* mark byte consumed */
@@ -3087,6 +3116,7 @@ keep_going:						/* We will come back to here until there is
 							goto error_return;
 						}
 
+						conn->did_negotiate = true;
 						conn->try_gss = false;
 						conn->status = CONNECTION_MADE;
 						return PGRES_POLLING_WRITING;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 7f5be7db7a..e787ab82da 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -507,6 +507,9 @@ struct pg_conn
 								 * connection */
 #endif
 
+	/* Did we try negotiating SSL or GSS? Postmaster used to allow only a single attempt */
+	bool did_negotiate;
+	
 	/* Buffer for current error message */
 	PQExpBufferData errorMessage;	/* expansible string */
 
-- 
2.23.0