Protocol problem with GSSAPI encryption?
This came up recently on IRC, not sure if the report there was passed on
at all.
ProcessStartupPacket assumes that there will be only one negotiation
request for an encrypted connection, but libpq is capable of issuing
two: it will ask for GSS encryption first, if it looks like it will be
able to do GSSAPI, and if the server refuses that it will ask (on the
same connection) for SSL.
But ProcessStartupPacket assumes that the packet after a failed
negotiation of either kind will be the actual startup packet, so the SSL
connection request is rejected with "unsupported version 1234.5679".
I'm guessing this usually goes unnoticed because most people are
probably not set up to do GSSAPI, and those who are are probably ok with
using it for encryption. But if the client is set up for GSSAPI and the
server not, then trying to do an SSL connection will fail when it should
succeed, and PGGSSENCMODE=disable in the environment (or connect string)
is necessary to get the connection to succeed.
It seems to me that this is a bug in ProcessStartupPacket, which should
accept both GSS or SSL negotiation requests on a connection (in either
order). Maybe secure_done should be two flags rather than one?
I'm not really familiar with the GSSAPI stuff so probably someone who is
should take a look.
--
Andrew (irc:RhodiumToad)
On 2019-12-01 02:13, Andrew Gierth wrote:
But ProcessStartupPacket assumes that the packet after a failed
negotiation of either kind will be the actual startup packet, so the SSL
connection request is rejected with "unsupported version 1234.5679".I'm guessing this usually goes unnoticed because most people are
probably not set up to do GSSAPI, and those who are are probably ok with
using it for encryption. But if the client is set up for GSSAPI and the
server not, then trying to do an SSL connection will fail when it should
succeed, and PGGSSENCMODE=disable in the environment (or connect string)
is necessary to get the connection to succeed.It seems to me that this is a bug in ProcessStartupPacket, which should
accept both GSS or SSL negotiation requests on a connection (in either
order). Maybe secure_done should be two flags rather than one?
I have also seen reports of that. I think your analysis is correct.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
It seems to me that this is a bug in ProcessStartupPacket, which
should accept both GSS or SSL negotiation requests on a connection
(in either order). Maybe secure_done should be two flags rather than
one?
Peter> I have also seen reports of that. I think your analysis is
Peter> correct.
I figure something along these lines for the fix. Anyone in a position
to test this?
--
Andrew (irc:RhodiumToad)
Attachments:
gssenc.patchtext/x-patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9ff2832c00..1b51d4916d 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 ssl_done, bool gss_done);
static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
static void processCancelRequest(Port *port, void *pkt);
static int initMasks(fd_set *rmask);
@@ -1914,11 +1914,15 @@ initMasks(fd_set *rmask)
* send anything to the client, which would typically be appropriate
* if we detect a communications failure.)
*
- * Set secure_done when negotiation of an encrypted layer (currently, TLS or
- * GSSAPI) is already completed.
+ * Set ssl_done and/or gss_done when negotiation of an encrypted layer
+ * (currently, TLS or GSSAPI) is completed. A successful negotiation of either
+ * encryption layer sets both flags, but a rejected negotiation sets only the
+ * flag for that layer, since the client may wish to try the other one. We
+ * should make no assumption here about the order in which the client may make
+ * requests.
*/
static int
-ProcessStartupPacket(Port *port, bool secure_done)
+ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
{
int32 len;
void *buf;
@@ -1951,7 +1955,7 @@ ProcessStartupPacket(Port *port, bool secure_done)
if (pq_getbytes(((char *) &len) + 1, 3) == EOF)
{
/* Got a partial length word, so bleat about that */
- if (!secure_done)
+ if (!ssl_done && !gss_done)
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("incomplete startup packet")));
@@ -2003,7 +2007,7 @@ ProcessStartupPacket(Port *port, bool secure_done)
return STATUS_ERROR;
}
- if (proto == NEGOTIATE_SSL_CODE && !secure_done)
+ if (proto == NEGOTIATE_SSL_CODE && !ssl_done)
{
char SSLok;
@@ -2032,11 +2036,14 @@ retry1:
if (SSLok == 'S' && secure_open_server(port) == -1)
return STATUS_ERROR;
#endif
- /* regular startup packet, cancel, etc packet should follow... */
- /* but not another SSL negotiation request */
- return ProcessStartupPacket(port, true);
+ /*
+ * regular startup packet, cancel, etc packet should follow, but not
+ * another SSL negotiation request, and a GSS request should only
+ * follow if SSL was rejected (client may negotiate in either order)
+ */
+ return ProcessStartupPacket(port, true, SSLok == 'S');
}
- else if (proto == NEGOTIATE_GSS_CODE && !secure_done)
+ else if (proto == NEGOTIATE_GSS_CODE && !gss_done)
{
char GSSok = 'N';
#ifdef ENABLE_GSS
@@ -2059,8 +2066,12 @@ retry1:
if (GSSok == 'G' && secure_open_gssapi(port) == -1)
return STATUS_ERROR;
#endif
- /* Won't ever see more than one negotiation request */
- return ProcessStartupPacket(port, true);
+ /*
+ * regular startup packet, cancel, etc packet should follow, but not
+ * another GSS negotiation request, and an SSL request should only
+ * follow if GSS was rejected (client may negotiate in either order)
+ */
+ return ProcessStartupPacket(port, GSSok == 'G', true);
}
/* Could add additional special packet types here */
@@ -4400,7 +4411,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, false);
/*
* Stop here if it was bad or a cancel packet. ProcessStartupPacket
Greetings,
* Andrew Gierth (andrew@tao11.riddles.org.uk) wrote:
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
It seems to me that this is a bug in ProcessStartupPacket, which
should accept both GSS or SSL negotiation requests on a connection
(in either order). Maybe secure_done should be two flags rather than
one?Peter> I have also seen reports of that. I think your analysis is
Peter> correct.I figure something along these lines for the fix. Anyone in a position
to test this?
At least at first blush, I tend to agree with your analysis and patch.
I'll see about getting this actually set up and tested in the next week
or so (and maybe there's some way to also manage to have a regression
test for it..).
Thanks!
Stephen
On 4. Dec 2019, at 06:24, Stephen Frost <sfrost@snowman.net> wrote:
Greetings,
* Andrew Gierth (andrew@tao11.riddles.org.uk) wrote:
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
It seems to me that this is a bug in ProcessStartupPacket, which
should accept both GSS or SSL negotiation requests on a connection
(in either order). Maybe secure_done should be two flags rather than
one?Peter> I have also seen reports of that. I think your analysis is
Peter> correct.I figure something along these lines for the fix. Anyone in a position
to test this?At least at first blush, I tend to agree with your analysis and patch.
I agree with the patch, but this also needs to be fixed on the client side.
Otherwise libpq won't be able to connect to older servers.
I'm attaching a proposed second patch to detect the error on the client side and reconnect to this message.
This patch was first submitted as a separate thread here:
/messages/by-id/F27EEE9D-D04A-4B6B-B1F1-96EA4DD996D0@eggerapps.at
Jakob
Attachments:
0002-libpq-Retry-after-failed-ssl-gss-negotiation.patchapplication/octet-stream; name=0002-libpq-Retry-after-failed-ssl-gss-negotiation.patch; x-unix-mode=0644Download
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
On Sun, Dec 1, 2019 at 01:13:31AM +0000, Andrew Gierth wrote:
This came up recently on IRC, not sure if the report there was passed on
at all.ProcessStartupPacket assumes that there will be only one negotiation
request for an encrypted connection, but libpq is capable of issuing
two: it will ask for GSS encryption first, if it looks like it will be
able to do GSSAPI, and if the server refuses that it will ask (on the
same connection) for SSL.
Are you saying that there is an additional round-trip for starting all
SSL connections because we now support GSSAPI, or this only happens if
libpq asks for GSSAPI?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +Greetings,
* Bruce Momjian (bruce@momjian.us) wrote:
On Sun, Dec 1, 2019 at 01:13:31AM +0000, Andrew Gierth wrote:
This came up recently on IRC, not sure if the report there was passed on
at all.ProcessStartupPacket assumes that there will be only one negotiation
request for an encrypted connection, but libpq is capable of issuing
two: it will ask for GSS encryption first, if it looks like it will be
able to do GSSAPI, and if the server refuses that it will ask (on the
same connection) for SSL.Are you saying that there is an additional round-trip for starting all
SSL connections because we now support GSSAPI, or this only happens if
libpq asks for GSSAPI?
The way that this is intended to work is if, and only if, there's is a
valid GSS credentical cache (on the client side) will GSSAPI encryption
be attempted and then if that fails because the server doesn't support
GSSAPI encryption of it's not possible to acquire credentials for
whatever reason then we'll fall back to other methods.
I have heard, however, that the Applie GSS libraries are both outright
broken (they lie about a valid credential cache existing- claiming one
does even when that's clearly not the case, based on klist..), and
deprecated (so they aren't likely going to fix them either..). We're
currently looking to see if there's a way to basically detect the Apple
GSS libraries and refuse to build if we discover that's what we're
building against. I'm not sure what other choice we really have...
If you gdb psql, without a Kerberos credential cache, on a system that
has a working GSS library, you'll note that pg_GSS_have_cred_cache()
returns false, meaning we skip over the GSS startup code in
PQconnectPoll() (and drop down to trying to do SSL next).
Thanks,
Stephen
"Bruce" == Bruce Momjian <bruce@momjian.us> writes:
This came up recently on IRC, not sure if the report there was
passed on at all.ProcessStartupPacket assumes that there will be only one negotiation
request for an encrypted connection, but libpq is capable of issuing
two: it will ask for GSS encryption first, if it looks like it will
be able to do GSSAPI, and if the server refuses that it will ask (on
the same connection) for SSL.
Bruce> Are you saying that there is an additional round-trip for
Bruce> starting all SSL connections because we now support GSSAPI, or
Bruce> this only happens if libpq asks for GSSAPI?
The problem only occurs if libpq thinks it might be able to do GSSAPI,
but the server does not. Without the patch I proposed or something like
it, this case fails to connect at all; with it, there will be an extra
round-trip. Explicitly disabling GSSAPI encryption in the connection
string or environment avoids the issue.
The exact condition for libpq seems to be a successful call to
gss_acquire_cred, but I'm not familiar with GSS in general.
--
Andrew (irc:RhodiumToad)
On Fri, Dec 20, 2019 at 06:14:09PM +0000, Andrew Gierth wrote:
"Bruce" == Bruce Momjian <bruce@momjian.us> writes:
This came up recently on IRC, not sure if the report there was
passed on at all.ProcessStartupPacket assumes that there will be only one negotiation
request for an encrypted connection, but libpq is capable of issuing
two: it will ask for GSS encryption first, if it looks like it will
be able to do GSSAPI, and if the server refuses that it will ask (on
the same connection) for SSL.Bruce> Are you saying that there is an additional round-trip for
Bruce> starting all SSL connections because we now support GSSAPI, or
Bruce> this only happens if libpq asks for GSSAPI?The problem only occurs if libpq thinks it might be able to do GSSAPI,
but the server does not. Without the patch I proposed or something like
it, this case fails to connect at all; with it, there will be an extra
round-trip. Explicitly disabling GSSAPI encryption in the connection
string or environment avoids the issue.The exact condition for libpq seems to be a successful call to
gss_acquire_cred, but I'm not familiar with GSS in general.
Thanks for the clarification from you and Stephen.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +"Stephen" == Stephen Frost <sfrost@snowman.net> writes:
I figure something along these lines for the fix. Anyone in a
position to test this?
Stephen> At least at first blush, I tend to agree with your analysis
Stephen> and patch.
Stephen> I'll see about getting this actually set up and tested in the
Stephen> next week or so (and maybe there's some way to also manage to
Stephen> have a regression test for it..).
*poke*
--
Andrew (irc:RhodiumToad)
On Fri, Feb 21, 2020 at 12:35:03AM +0000, Andrew Gierth wrote:
"Stephen" == Stephen Frost <sfrost@snowman.net> writes:
I figure something along these lines for the fix. Anyone in a
position to test this?Stephen> At least at first blush, I tend to agree with your analysis
Stephen> and patch.Stephen> I'll see about getting this actually set up and tested in the
Stephen> next week or so (and maybe there's some way to also manage to
Stephen> have a regression test for it..).*poke*
Second *poke*.
--
Michael
Greetings,
* Stephen Frost (sfrost@snowman.net) wrote:
* Andrew Gierth (andrew@tao11.riddles.org.uk) wrote:
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
It seems to me that this is a bug in ProcessStartupPacket, which
should accept both GSS or SSL negotiation requests on a connection
(in either order). Maybe secure_done should be two flags rather than
one?Peter> I have also seen reports of that. I think your analysis is
Peter> correct.I figure something along these lines for the fix. Anyone in a position
to test this?At least at first blush, I tend to agree with your analysis and patch.
I'll see about getting this actually set up and tested in the next week
or so (and maybe there's some way to also manage to have a regression
test for it..).
After testing this and playing around with it a bit, I've gone ahead and
pushed it.
Thanks!
Stephen