On 21/06/2024 02:32, Jacob Champion wrote:
On Thu, Jun 20, 2024 at 4:13 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:By "negotiation" I mean the server's response to the startup packet. I.e. "supported"/"not supported"/"error".Ok, I'm still a little confused, probably a terminology issue. The server doesn't respond with "supported" or "not supported" to the startup packet, that happens earlier. I think you mean the SSLRequst / GSSRequest packet, which is sent *before* the startup packet?Yes, sorry. (I'm used to referring to those as startup packets too, ha.)
Yeah I'm not sure what the right term would be.
Hmm, right, GSS encryption was introduced in v12, and older versions respond with an error to a GSSRequest. We probably could make the same assumption for GSS as we did for TLS in a49fbaaf, i.e. that an error means that something's wrong with the server, rather than that it's just very old and doesn't support GSS. But the case for that is a lot weaker case than with TLS. There are still pre-v12 servers out there in the wild.Right. Since we default to gssencmode=prefer, if you have Kerberos creds in your environment, I think this could potentially break existing software that connects to v11 servers once you upgrade libpq.
When you connect to a V11 server and attempt to perform GSSAPI authentication, it will respond with a V3 error that says: "unsupported frontend protocol 1234.5680: server supports 2.0 to 3.0". That was a surprise to me until I tested it just now. I thought that it would respond with a protocol V2 error, but it is not so. The backend sets FrontendProtocol to 1234.5680 before sending the error, and because it is >= 3, the error is sent with protocol version 3.
Given that, I think it is a good thing to fail the connection completely on receiving a V2 error.
Attached is a patch to fix the other issue, with falling back from SSL to plaintext. And some tests and comment fixes I spotted while at it.
0001: A small comment fix 0002: This is the main patch that fixes the SSL fallback issue0003: This adds fault injection tests to exercise these early error codepaths. It is not ready to be merged, as it contains a hack to skip locking. See thread at https://www.postgresql.org/message-id/e1ffb822-054e-4006-ac06-50532767f75b%40iki.fi.
0004: More tests, for what happens if the server sends an error after responding "yes" to the SSLRequest or GSSRequest, but before performing the SSL/GSS handshake.
Attached is also a little stand-alone perl program that listens on a socket, and when you connect to it, it immediately sends a V2 or V3 error, depending on the argument. That's useful for testing. It could be used as an alternative strategy to the injection points I used in the 0003-0004 patches, but for now I just used it for manual testing.
-- Heikki Linnakangas Neon (https://neon.tech)
From 4b988b1c74066fe8b5f98acb4ecd20150a47bc33 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 24 Jun 2024 20:41:41 +0300
Subject: [PATCH 1/4] Fix outdated comment after removal of direct SSL fallback
The option to fall back from direct SSL to negotiated SSL or a
plaintext connection was removed in commit fb5718f35f.
---
src/interfaces/libpq/fe-connect.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 071b1b34aa1..e003279fb6c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3564,8 +3564,8 @@ keep_going: /* We will come back to here until there is
if (pollres == PGRES_POLLING_FAILED)
{
/*
- * Failed direct ssl connection, possibly try a new
- * connection with postgres negotiation
+ * SSL handshake failed. We will retry with a plaintext
+ * connection, if permitted by sslmode.
*/
CONNECTION_FAILED();
}
--
2.39.2
From 7df581e902b76665de4c751ddbc1309143e6c0b8 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 24 Jun 2024 18:14:52 +0300
Subject: [PATCH 2/4] Fix fallback behavior when server sends an ERROR early at
startup
With sslmode=prefer, the desired behavior is to completely fail the
connection attempt, *not* fall back to a plaintext connection, if the
server responds to the SSLRequest with an error ('E') response instead
of rejecting SSL with an 'N' response. This was broken in commit
05fd30c0e7.
Discussion: https://www.postgresql.org/message-id/CAOYmi%2Bnwvu21mJ4DYKUa98HdfM_KZJi7B1MhyXtnsyOO-PB6Ww%40mail.gmail.com
---
src/interfaces/libpq/fe-connect.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index e003279fb6c..360d9a45476 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3521,6 +3521,12 @@ keep_going: /* We will come back to here until there is
* byte here.
*/
conn->status = CONNECTION_AWAITING_RESPONSE;
+
+ /*
+ * Don't fall back to a plaintext connection after
+ * reading the error.
+ */
+ conn->failed_enc_methods |= conn->allowed_enc_methods & (~conn->current_enc_method);
goto keep_going;
}
else
@@ -3612,6 +3618,13 @@ keep_going: /* We will come back to here until there is
* into AWAITING_RESPONSE state and let the code there
* deal with it. Note we have *not* consumed the "E"
* byte here.
+ *
+ * Note that unlike on an error response to
+ * SSLRequest, we allow falling back to SSL or
+ * plaintext connection here. GSS support was
+ * introduced in PostgreSQL version 12, so an error
+ * response might mean that we are connecting to a
+ * pre-v12 server.
*/
conn->status = CONNECTION_AWAITING_RESPONSE;
goto keep_going;
@@ -3659,6 +3672,10 @@ keep_going: /* We will come back to here until there is
}
else if (pollres == PGRES_POLLING_FAILED)
{
+ /*
+ * GSS handshake failed. We will retry with an SSL or
+ * plaintext connection, if permitted by the options.
+ */
CONNECTION_FAILED();
}
/* Else, return POLLING_READING or POLLING_WRITING status */
--
2.39.2
From e893d2e53a9bde6ddf4462d9f8b206c51ebd40cb Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 24 Jun 2024 18:15:53 +0300
Subject: [PATCH 3/4] WIP: Add injection points and test for early backend
startup errors
---
src/backend/tcop/backend_startup.c | 9 ++++
src/backend/utils/misc/injection_point.c | 33 +++++++++++-
src/include/utils/injection_point.h | 1 +
src/interfaces/libpq/meson.build | 1 +
.../libpq/t/005_negotiate_encryption.pl | 50 +++++++++++++++++++
5 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/src/backend/tcop/backend_startup.c b/src/backend/tcop/backend_startup.c
index cfa27551964..7b4a3e23458 100644
--- a/src/backend/tcop/backend_startup.c
+++ b/src/backend/tcop/backend_startup.c
@@ -33,6 +33,7 @@
#include "tcop/backend_startup.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/timeout.h"
@@ -160,6 +161,14 @@ BackendInitialize(ClientSocket *client_sock, CAC_state cac)
whereToSendOutput = DestRemote; /* now safe to ereport to client */
+ /* For testing client error handling */
+ INJECTION_POINT("backend-initialize");
+ if (IsInjectionPointAttached("backend-initialize-v2-error"))
+ {
+ FrontendProtocol = PG_PROTOCOL(2,0);
+ elog(FATAL, "protocol version 2 error triggered");
+ }
+
/* set these to empty in case they are needed before we set them up */
port->remote_host = "";
port->remote_port = "";
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 5c2a0d2297e..d3995dfd6fc 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -24,6 +24,7 @@
#include "port/pg_bitutils.h"
#include "storage/fd.h"
#include "storage/lwlock.h"
+#include "storage/proc.h"
#include "storage/shmem.h"
#include "utils/hsearch.h"
#include "utils/injection_point.h"
@@ -281,6 +282,31 @@ InjectionPointDetach(const char *name)
#endif
}
+/*
+ * Test if an injection point is defined.
+ */
+bool
+IsInjectionPointAttached(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+ bool found;
+
+ /* FIXME: just skip locking if we don't have a PGPROC entry yet. Obviously unsafe.. */
+ if (MyProc != NULL && IsUnderPostmaster)
+ LWLockAcquire(InjectionPointLock, LW_SHARED);
+
+ (void) hash_search(InjectionPointHash, name, HASH_FIND, &found);
+
+ if (MyProc != NULL && IsUnderPostmaster)
+ LWLockRelease(InjectionPointLock);
+
+ return found;
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+ return false; /* silence compiler */
+#endif
+}
+
/*
* Execute an injection point, if defined.
*
@@ -296,11 +322,14 @@ InjectionPointRun(const char *name)
InjectionPointCallback injection_callback;
const void *private_data;
- LWLockAcquire(InjectionPointLock, LW_SHARED);
+ /* FIXME: just skip locking if we don't have a PGPROC entry yet. Obviously unsafe.. */
+ if (MyProc != NULL && IsUnderPostmaster)
+ LWLockAcquire(InjectionPointLock, LW_SHARED);
entry_by_name = (InjectionPointEntry *)
hash_search(InjectionPointHash, name,
HASH_FIND, &found);
- LWLockRelease(InjectionPointLock);
+ if (MyProc != NULL && IsUnderPostmaster)
+ LWLockRelease(InjectionPointLock);
/*
* If not found, do nothing and remove it from the local cache if it
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index a61d5d44391..5715eefb31e 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -35,6 +35,7 @@ extern void InjectionPointAttach(const char *name,
const void *private_data,
int private_data_size);
extern void InjectionPointRun(const char *name);
+extern bool IsInjectionPointAttached(const char *name);
extern bool InjectionPointDetach(const char *name);
#endif /* INJECTION_POINT_H */
diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index ed2a4048d18..7623aeadab7 100644
--- a/src/interfaces/libpq/meson.build
+++ b/src/interfaces/libpq/meson.build
@@ -121,6 +121,7 @@ tests += {
't/005_negotiate_encryption.pl',
],
'env': {
+ 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
'with_ssl': ssl_library,
'with_gssapi': gssapi.found() ? 'yes' : 'no',
'with_krb_srvnam': 'postgres',
diff --git a/src/interfaces/libpq/t/005_negotiate_encryption.pl b/src/interfaces/libpq/t/005_negotiate_encryption.pl
index c3f70d31bc8..e6bea0f04f8 100644
--- a/src/interfaces/libpq/t/005_negotiate_encryption.pl
+++ b/src/interfaces/libpq/t/005_negotiate_encryption.pl
@@ -90,6 +90,8 @@ my $kerberos_enabled =
$ENV{PG_TEST_EXTRA} && $ENV{PG_TEST_EXTRA} =~ /\bkerberos\b/;
my $ssl_supported = $ENV{with_ssl} eq 'openssl';
+my $injection_points_supported = $ENV{enable_injection_points} eq 'yes';
+
###
### Prepare test server for GSSAPI and SSL authentication, with a few
### different test users and helper functions. We don't actually
@@ -155,6 +157,10 @@ $node->safe_psql('postgres', 'CREATE USER ssluser;');
$node->safe_psql('postgres', 'CREATE USER nossluser;');
$node->safe_psql('postgres', 'CREATE USER gssuser;');
$node->safe_psql('postgres', 'CREATE USER nogssuser;');
+if ($injection_points_supported != 0)
+{
+ $node->safe_psql('postgres', 'CREATE EXTENSION injection_points;')
+}
my $unixdir = $node->safe_psql('postgres', 'SHOW unix_socket_directories;');
chomp($unixdir);
@@ -320,6 +326,27 @@ nossluser . disable postgres connect, authok
\@all_sslmodes, \@all_sslnegotiations,
parse_table($test_table));
+ if ($injection_points_supported != 0)
+ {
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('backend-initialize', 'error');",
+ connstr => "user=localuser host=$unixdir");
+ connect_test(
+ $node,
+ "user=testuser sslmode=prefer",
+ 'backenderror -> fail');
+ $node->restart;
+
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('backend-initialize-v2-error', 'error');",
+ connstr => "user=localuser host=$unixdir");
+ connect_test(
+ $node,
+ "user=testuser sslmode=prefer",
+ 'v2error -> fail');
+ $node->restart;
+ }
+
# Disable SSL again
$node->adjust_conf('postgresql.conf', 'ssl', 'off');
$node->reload;
@@ -403,6 +430,27 @@ nogssuser disable disable postgres connect, authok
test_matrix($node, $server_config, [ 'testuser', 'gssuser', 'nogssuser' ],
\@all_gssencmodes, $sslmodes, $sslnegotiations,
parse_table($test_table));
+
+ if ($injection_points_supported != 0)
+ {
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('backend-initialize', 'error');",
+ connstr => "user=localuser host=$unixdir");
+ connect_test(
+ $node,
+ "user=testuser gssencmode=prefer sslmode=disable",
+ 'backenderror, backenderror -> fail');
+ $node->restart;
+
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('backend-initialize-v2-error', 'error');",
+ connstr => "user=localuser host=$unixdir");
+ connect_test(
+ $node,
+ "user=testuser gssencmode=prefer sslmode=disable",
+ 'v2error -> fail');
+ $node->restart;
+ }
}
###
@@ -750,6 +798,8 @@ sub parse_log_events
push @events, "gssreject" if $line =~ /GSSENCRequest rejected/;
push @events, "authfail" if $line =~ /no pg_hba.conf entry/;
push @events, "authok" if $line =~ /connection authenticated/;
+ push @events, "backenderror" if $line =~ /error triggered for injection point backend-/;
+ push @events, "v2error" if $line =~ /protocol version 2 error triggered/;
}
# No events at all is represented by "-"
--
2.39.2
From 572b3e8ba2ab33fa695cb557ab6d0b6ae0e0ae63 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 24 Jun 2024 23:13:28 +0300
Subject: [PATCH 4/4] WIP: Add tests for errors during ssl or gssapi handshake
---
src/backend/libpq/be-secure-gssapi.c | 3 +++
src/backend/libpq/be-secure.c | 3 +++
.../libpq/t/005_negotiate_encryption.pl | 18 ++++++++++++++++++
3 files changed, 24 insertions(+)
diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
index bc04e78abba..483636503c1 100644
--- a/src/backend/libpq/be-secure-gssapi.c
+++ b/src/backend/libpq/be-secure-gssapi.c
@@ -21,6 +21,7 @@
#include "libpq/pqformat.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
@@ -499,6 +500,8 @@ secure_open_gssapi(Port *port)
minor;
gss_cred_id_t delegated_creds;
+ INJECTION_POINT("backend-gssapi-startup");
+
/*
* Allocate subsidiary Port data for GSSAPI operations.
*/
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 1663f36b6b8..ef20ea755b7 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -30,6 +30,7 @@
#include "libpq/libpq.h"
#include "miscadmin.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/wait_event.h"
char *ssl_library;
@@ -129,6 +130,8 @@ secure_open_server(Port *port)
}
Assert(pq_buffer_remaining_data() == 0);
+ INJECTION_POINT("backend-ssl-startup");
+
r = be_tls_open_server(port);
if (port->raw_buf_remaining > 0)
diff --git a/src/interfaces/libpq/t/005_negotiate_encryption.pl b/src/interfaces/libpq/t/005_negotiate_encryption.pl
index e6bea0f04f8..991badf28ec 100644
--- a/src/interfaces/libpq/t/005_negotiate_encryption.pl
+++ b/src/interfaces/libpq/t/005_negotiate_encryption.pl
@@ -345,6 +345,15 @@ nossluser . disable postgres connect, authok
"user=testuser sslmode=prefer",
'v2error -> fail');
$node->restart;
+
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('backend-ssl-startup', 'error');",
+ connstr => "user=localuser host=$unixdir");
+ connect_test(
+ $node,
+ "user=testuser sslmode=prefer",
+ 'connect, sslaccept, backenderror, reconnect, authok -> plain');
+ $node->restart;
}
# Disable SSL again
@@ -450,6 +459,15 @@ nogssuser disable disable postgres connect, authok
"user=testuser gssencmode=prefer sslmode=disable",
'v2error -> fail');
$node->restart;
+
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('backend-gssapi-startup', 'error');",
+ connstr => "user=localuser host=$unixdir");
+ connect_test(
+ $node,
+ "user=testuser gssencmode=prefer sslmode=disable",
+ 'connect, gssaccept, backenderror, reconnect, authok -> plain');
+ $node->restart;
}
}
--
2.39.2
serve-pgsql-error.pl
Description: Perl program
