Обсуждение: 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)
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: https://www.postgresql.org/message-id/F27EEE9D-D04A-4B6B-B1F1-96EA4DD996D0@eggerapps.at Jakob
Вложения
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