On Thu, 28 Mar 2024, 13:37 Heikki Linnakangas, <hlinn...@iki.fi> wrote: > > On 28/03/2024 13:15, Matthias van de Meent wrote: > > On Tue, 5 Mar 2024 at 15:08, Heikki Linnakangas <hlinn...@iki.fi> 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 } /*