On Mon, Apr 06, 2020 at 04:25:57PM +0900, Michael Paquier wrote: > It is possible to enforce this flag to false by using > gssencmode=disable, but that's not really user-friendly in my opinion > because nobody is going to remember that for connection strings with > SSL settings so a lot of application are taking a performance hit at > connection because of that in my opinion. I think that's also a bad > idea from the start to assume that we have to try GSS by default, as > any new code path opening a secured connection may fail into the trap > of attempting to use GSS if this flag is not reset. Shouldn't we try > to set this flag to false by default, and set it to true only if > necessary depending on gssencmode? A quick hack switching this flag > to false in makeEmptyPGconn() gives back the past performance to > src/test/ssl/, FWIW. > > Looking around, it seems to me that there is a second issue as of > PQconnectPoll(), where we don't reset the state machine correctly for > try_gss within reset_connection_state_machine, and instead HEAD does > it in connectDBStart().
So, a lot of things come down to PQconnectPoll() here. Once the connection state reached is CONNECTION_MADE, we first try a GSS connection if try_gss is true, and a SSL connection attempt follows just after. This makes me wonder about the following things: - gssencmode is prefer by default, the same as sslmode. Shouldn't we issue an error if any of them is not disabled to avoid any conflicts in the client, making the choice of gssencmode=prefer by default a bad choice? It seems to me that there could be an argument to make gssencmode disabled by default, and issue an error if somebody attempts a connection without at least gssencode or sslmode set as disabled. - The current code tries a GSS connection first, and then it follows with SSL, which is annoying because gssencmode=prefer by default means that any user would pay the cost of attempting a GSS connection for nothing (with or even without SSL). Shouldn't we do the opposite here, by trying SSL first, and then GSS? For now, I am attaching a WIP patch which seems like a good angle of attack for libpq, meaning that if sslmode and gssencmode are both set, then we would attempt a SSL connection before attempting GSS, so as any user of SSL does not pay a performance hit compared to past versions (I know that src/test/kerberos/ fails with that because sslmode=prefer is triggered first in PQconnectPoll(), but that's just to show the idea I had in mind). Any thoughts? -- Michael
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0157c619aa..f1b74e8351 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2023,11 +2023,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
@@ -2464,6 +2459,9 @@ 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
+ conn->try_gss = (conn->gssencmode[0] != 'd'); /* disable */
+#endif
reset_connection_state_machine = false;
need_new_connection = true;
@@ -2861,6 +2859,38 @@ keep_going: /* We will come back to here until there is
#endif
}
+#ifdef USE_SSL
+
+ /*
+ * If SSL is enabled and we haven't already got it running,
+ * request it instead of sending the startup message.
+ */
+ if (conn->allow_ssl_try && !conn->wait_ssl_try &&
+ !conn->ssl_in_use)
+ {
+ ProtocolVersion pv;
+
+ /*
+ * Send the SSL request packet.
+ *
+ * Theoretically, this could block, but it really
+ * shouldn't since we only got here if the socket is
+ * write-ready.
+ */
+ pv = pg_hton32(NEGOTIATE_SSL_CODE);
+ if (pqPacketSend(conn, 0, &pv, sizeof(pv)) != STATUS_OK)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not send SSL negotiation packet: %s\n"),
+ SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+ goto error_return;
+ }
+ /* Ok, wait for response */
+ conn->status = CONNECTION_SSL_STARTUP;
+ return PGRES_POLLING_READING;
+ }
+#endif /* USE_SSL */
+
#ifdef ENABLE_GSS
/*
@@ -2897,38 +2927,6 @@ keep_going: /* We will come back to here until there is
}
#endif
-#ifdef USE_SSL
-
- /*
- * If SSL is enabled and we haven't already got it running,
- * request it instead of sending the startup message.
- */
- if (conn->allow_ssl_try && !conn->wait_ssl_try &&
- !conn->ssl_in_use)
- {
- ProtocolVersion pv;
-
- /*
- * Send the SSL request packet.
- *
- * Theoretically, this could block, but it really
- * shouldn't since we only got here if the socket is
- * write-ready.
- */
- pv = pg_hton32(NEGOTIATE_SSL_CODE);
- if (pqPacketSend(conn, 0, &pv, sizeof(pv)) != STATUS_OK)
- {
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("could not send SSL negotiation packet: %s\n"),
- SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- goto error_return;
- }
- /* Ok, wait for response */
- conn->status = CONNECTION_SSL_STARTUP;
- return PGRES_POLLING_READING;
- }
-#endif /* USE_SSL */
-
/*
* Build the startup packet.
*/
@@ -3902,9 +3900,6 @@ makeEmptyPGconn(void)
conn->verbosity = PQERRORS_DEFAULT;
conn->show_context = PQSHOW_CONTEXT_ERRORS;
conn->sock = PGINVALID_SOCKET;
-#ifdef ENABLE_GSS
- conn->try_gss = true;
-#endif
/*
* We try to send at least 8K at a time, which is the usual size of pipe
signature.asc
Description: PGP signature
