On Thu, 28 Mar 2024, 13:37 Heikki Linnakangas, <[email protected]> wrote:
>
> On 28/03/2024 13:15, Matthias van de Meent wrote:
> > On Tue, 5 Mar 2024 at 15:08, Heikki Linnakangas <[email protected]> wrote:
> >>
> >> I hope I didn't joggle your elbow reviewing this, Jacob, but I spent
> >> some time rebase and fix various little things:
> >
> > With the recent changes to backend startup committed by you, this
> > patchset has gotten major apply failures.
> >
> > Could you provide a new version of the patchset so that it can be
> > reviewed in the context of current HEAD?
>
> Here you are.
Sorry for the delay. I've run some tests and didn't find any specific
issues in the patchset.
I did get sidetracked on trying to further improve the test suite,
where I was trying to find out how to use Test::More::subtests, but
have now decided it's not worth the lost time now vs adding this as a
feature in 17.
Some remaining comments:
patches 0001/0002: not reviewed in detail.
Patch 0003:
The read size in secure_raw_read is capped to port->raw_buf_remaining
if the raw buf has any data. While the user will probably call into
this function again, I think that's a waste of cycles.
pq_buffer_has_data now doesn't have any protections against
desynchronized state between PqRecvLength and PqRecvPointer. An
Assert(PqRecvLength >= PqRecvPointer) to that value would be
appreciated.
(in backend_startup.c)
> + elog(LOG, "Detected direct SSL handshake");
I think this should be gated at a lower log level, or a GUC, as this
wouls easily DOS a logfile by bulk sending of SSL handshake bytes.
0004:
backend_startup.c
> + if (!ssl_enable_alpn)
> + {
> + elog(WARNING, "Received direct SSL connection without
> ssl_enable_alpn enabled");
This is too verbose, too.
> + if (!port->alpn_used)
> + {
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("Received direct SSL connection request without
> required ALPN protocol negotiation extension")));
If ssl_enable_alpn is disabled, we shouln't report a COMMERROR when
the client does indeed not have alpn enabled.
0005:
As mentioned above, I'd have loved to use subtests here for the cube()
of tests, but I got in too much of a rabbit hole to get that done.
0006:
In CONNECTION_FAILED, we use connection_failed() to select whether we
need a new connection or stop trying altogether, but that function's
description states:
> + * Out-of-line portion of the CONNECTION_FAILED() macro
> + *
> + * Returns true, if we should retry the connection with different encryption
> method.
Which to me reads like we should reuse the connection, and try a
different method on that same connection. Maybe we can improve the
wording to something like
+ * Returns true, if we should reconnect with a different encryption method.
to make the reconnect part more clear.
In select_next_encryption_method, there are several copies of this pattern:
if ((remaining_methods & ENC_METHOD) != 0)
{
conn->current_enc_method = ENC_METHOD;
return true;
}
I think a helper macro would reduce the verbosity of the scaffolding,
like in the attached SELECT_NEXT_METHOD.diff.txt.
Kind regards,
Matthias van de Meent
src/interfaces/libpq/fe-connect.c | 51 +++++++++++++++------------------------
1 file changed, 20 insertions(+), 31 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index edc324dad0..ef95b07978 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4344,6 +4344,15 @@ select_next_encryption_method(PGconn *conn, bool
have_valid_connection)
{
int remaining_methods;
+#define SELECT_NEXT_METHOD(method) \
+ do { \
+ if ((remaining_methods & method) != 0) \
+ { \
+ conn->current_enc_method = method; \
+ return true; \
+ } \
+ } while (false)
+
remaining_methods = conn->allowed_enc_methods &
~conn->failed_enc_methods;
/*
@@ -4373,20 +4382,14 @@ select_next_encryption_method(PGconn *conn, bool
have_valid_connection)
}
}
}
- if ((remaining_methods & ENC_GSSAPI) != 0)
- {
- conn->current_enc_method = ENC_GSSAPI;
- return true;
- }
}
+
+ SELECT_NEXT_METHOD(ENC_GSSAPI);
#endif
/* With sslmode=allow, try plaintext connection before SSL. */
- if (conn->sslmode[0] == 'a' && (remaining_methods & ENC_PLAINTEXT) != 0)
- {
- conn->current_enc_method = ENC_PLAINTEXT;
- return true;
- }
+ if (conn->sslmode[0] == 'a')
+ SELECT_NEXT_METHOD(ENC_PLAINTEXT);
/*
* Try SSL. If enabled, try direct SSL. Unless we have a valid TCP
@@ -4396,33 +4399,19 @@ select_next_encryption_method(PGconn *conn, bool
have_valid_connection)
* roundtrip from the negotiation, but reconnecting would also incur a
* roundtrip.
*/
- if (have_valid_connection && (remaining_methods & ENC_NEGOTIATED_SSL)
!= 0)
- {
- conn->current_enc_method = ENC_NEGOTIATED_SSL;
- return true;
- }
-
- if ((remaining_methods & ENC_DIRECT_SSL) != 0)
- {
- conn->current_enc_method = ENC_DIRECT_SSL;
- return true;
- }
+ if (have_valid_connection)
+ SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL);
- if ((remaining_methods & ENC_NEGOTIATED_SSL) != 0)
- {
- conn->current_enc_method = ENC_NEGOTIATED_SSL;
- return true;
- }
+ SELECT_NEXT_METHOD(ENC_DIRECT_SSL);
+ SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL);
- if (conn->sslmode[0] != 'a' && (remaining_methods & ENC_PLAINTEXT) != 0)
- {
- conn->current_enc_method = ENC_PLAINTEXT;
- return true;
- }
+ if (conn->sslmode[0] != 'a')
+ SELECT_NEXT_METHOD(ENC_PLAINTEXT);
/* No more options */
conn->current_enc_method = ENC_ERROR;
return false;
+#undef SELECT_NEXT_METHOD
}
/*
