On 16/07/2024 09:54, Michael Paquier wrote:
On Mon, Jun 24, 2024 at 11:30:53PM +0300, Heikki Linnakangas wrote:
0002: This is the main patch that fixes the SSL fallback issue
+ conn->failed_enc_methods |= conn->allowed_enc_methods &
(~conn->current_enc_method);
Sounds reasonable to me.
It's a bit annoying to have to guess that current_enc_method is
tracking only one method at a time (aka these three fields are not
documented in libpq-int.h), while allowed_enc_methods and
failed_enc_methods is a bitwise combination of the methods that are
still allowed or that have already failed.
Yeah. In hindsight I'm still not very happy with the code structure with
"allowed_enc_methods" and "current_enc_methods" and all that. The
fallback logic is still complicated. It's better than in v16, IMHO, but
still not great. This patch seems like the best fix for v17, but I
wouldn't mind another round of refactoring for v18, if anyone's got some
good ideas on how to structure it better. All these new tests are a
great asset when refactoring this again.
+ if (IsInjectionPointAttached("backend-initialize-v2-error"))
+ {
+ FrontendProtocol = PG_PROTOCOL(2,0);
+ elog(FATAL, "protocol version 2 error triggered");
+ }
This is an attempt to do stack manipulation with an injection point
set. FrontendProtocol is a global variable, so you could have a new
callback setting up this global variable directly, then FATAL (I
really don't mind is modules/injection_points finishes with a library
of callbacks).
Not sure to like much this new IsInjectionPointAttached() that does a
search in the existing injection point pool, though. This leads to
more code footprint in the core backend, and I'm trying to minimize
that. Not everybody agrees with this view, I'd guess, which is also
fine.
Yeah, I'm also not too excited about the additional code in the backend,
but I'm also not excited about writing another test C module just for
this. I'm inclined to commit this as it is, but we can certainly revisit
this later, since it's just test code.
Here's a new rebased version with some minor cleanup. Notably, I added
docs for the new IS_INJECTION_POINT_ATTACHED() macro.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 651db616fcb19b89c94cacb30c27267ac7c17070 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 23 Jul 2024 20:03:27 +0300
Subject: [PATCH v2 1/3] 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.
Reported-by: Jacob Champion
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 e003279fb6..360d9a4547 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 649e2c89f62484940c55b02635e393bdd85654af Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 23 Jul 2024 20:20:37 +0300
Subject: [PATCH v2 2/3] Add test for early backend startup errors
The new test tests the libpq fallback behavior on an early error.
This adds an IS_INJECTION_POINT_ATTACHED() macro, to allow writing the
test code alongside the normal source code. In principle, the new test
could've been written by an extra test module with a callback that
sets the FrontendProtocol global variable, but I think it's more
straightforward to have the test code
Discussion: https://www.postgresql.org/message-id/CAOYmi%2Bnwvu21mJ4DYKUa98HdfM_KZJi7B1MhyXtnsyOO-PB6Ww%40mail.gmail.com
---
doc/src/sgml/xfunc.sgml | 25 ++++++++++
src/backend/tcop/backend_startup.c | 16 ++++++
src/backend/utils/misc/injection_point.c | 15 ++++++
src/include/utils/injection_point.h | 3 ++
src/interfaces/libpq/Makefile | 4 +-
src/interfaces/libpq/meson.build | 1 +
.../libpq/t/005_negotiate_encryption.pl | 50 +++++++++++++++++++
7 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 7e92e89846..5b584a4f14 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3672,6 +3672,31 @@ custom_injection_callback(const char *name, const void *private_data)
logic.
</para>
+ <para>
+ An alternative way to define the action to take when an injection point
+ is reached is to add the testing code alongside the normal source
+ code. This can be useful if the action e.g. depends on local variables
+ that are not accessible to loaded modules. The
+ <function>IS_INJECTION_POINT_ATTACHED</function> macro can then be used
+ to check if an injection point is attached, for example:
+<programlisting>
+#ifdef USE_INJECTION_POINTS
+if (IS_INJECTION_POINT_ATTACHED("before-foobar"))
+{
+ /* change a local variable if injection point is attached */
+ local_var = 123;
+
+ /* also execute the callback */
+ INJECTION_POINT_CACHED("before-foobar");
+}
+#endif
+</programlisting>
+ Note that the callback attached to the injection point will not be
+ executed by the <function>IS_INJECTION_POINT_ATTACHED</function>
+ macro. If you want to execute the callback, you must also call
+ <function>INJECTION_POINT_CACHED</function> like in the above example.
+ </para>
+
<para>
Optionally, it is possible to detach an injection point by calling:
<programlisting>
diff --git a/src/backend/tcop/backend_startup.c b/src/backend/tcop/backend_startup.c
index a2f94b1050..b840d95e4d 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"
@@ -213,6 +214,21 @@ BackendInitialize(ClientSocket *client_sock, CAC_state cac)
remote_host)));
}
+ /* For testing client error handling */
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT("backend-initialize");
+ if (IS_INJECTION_POINT_ATTACHED("backend-initialize-v2-error"))
+ {
+ /*
+ * This simulates an early error from a pre-v14 server, which used the
+ * version 2 protocol for any errors that occurred before processing
+ * the startup packet.
+ */
+ FrontendProtocol = PG_PROTOCOL(2, 0);
+ elog(FATAL, "protocol version 2 error triggered");
+ }
+#endif
+
/*
* If we did a reverse lookup to name, we might as well save the results
* rather than possibly repeating the lookup during authentication.
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 8ad0c27bc8..ec23f1f4d5 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -23,6 +23,7 @@
#include "miscadmin.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"
@@ -570,3 +571,17 @@ InjectionPointCached(const char *name)
elog(ERROR, "Injection points are not supported by this build");
#endif
}
+
+/*
+ * Test if an injection point is defined.
+ */
+bool
+IsInjectionPointAttached(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+ return InjectionPointCacheRefresh(name) != NULL;
+#else
+ elog(ERROR, "Injection points are not supported by this build");
+ return false; /* silence compiler */
+#endif
+}
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index a385e3df64..6be20b4857 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -18,10 +18,12 @@
#define INJECTION_POINT_LOAD(name) InjectionPointLoad(name)
#define INJECTION_POINT(name) InjectionPointRun(name)
#define INJECTION_POINT_CACHED(name) InjectionPointCached(name)
+#define IS_INJECTION_POINT_ATTACHED(name) IsInjectionPointAttached(name)
#else
#define INJECTION_POINT_LOAD(name) ((void) name)
#define INJECTION_POINT(name) ((void) name)
#define INJECTION_POINT_CACHED(name) ((void) name)
+#define IS_INJECTION_POINT_ATTACHED(name) (false)
#endif
/*
@@ -41,6 +43,7 @@ extern void InjectionPointAttach(const char *name,
extern void InjectionPointLoad(const char *name);
extern void InjectionPointRun(const char *name);
extern void InjectionPointCached(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/Makefile b/src/interfaces/libpq/Makefile
index b36a765764..27f8499d8a 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -9,11 +9,13 @@
#
#-------------------------------------------------------------------------
+EXTRA_INSTALL=src/test/modules/injection_points
+
subdir = src/interfaces/libpq
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
-export with_ssl with_gssapi with_krb_srvnam
+export with_ssl with_gssapi with_krb_srvnam enable_injection_points
PGFILEDESC = "PostgreSQL Access Library"
diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index ed2a4048d1..7623aeadab 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 251c405926..e21c883ab4 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);
@@ -312,6 +318,27 @@ nossluser . disable postgres connect, authok
['disable'], \@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",
+ 'connect, 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",
+ 'connect, v2error -> fail');
+ $node->restart;
+ }
+
# Disable SSL again
$node->adjust_conf('postgresql.conf', 'ssl', 'off');
$node->reload;
@@ -393,6 +420,27 @@ nogssuser disable disable postgres connect, authok
test_matrix($node, [ '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;
+ }
}
###
@@ -738,6 +786,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 87d8fbd5e65a7298eeb3335280e15c6da2d2a4bc Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 23 Jul 2024 20:21:54 +0300
Subject: [PATCH v2 3/3] Add tests for errors during SSL or GSSAPI handshake
These test that libpq correctly falls back to a plaintext connection
on handshake error, in the "prefer" modes.
---
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 bc04e78abb..483636503c 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 1663f36b6b..ef20ea755b 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 e21c883ab4..eadec9145f 100644
--- a/src/interfaces/libpq/t/005_negotiate_encryption.pl
+++ b/src/interfaces/libpq/t/005_negotiate_encryption.pl
@@ -337,6 +337,15 @@ nossluser . disable postgres connect, authok
"user=testuser sslmode=prefer",
'connect, 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
@@ -440,6 +449,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
