Re: Experiments with Postgres and SSL - Mailing list pgsql-hackers
From | Matthias van de Meent |
---|---|
Subject | Re: Experiments with Postgres and SSL |
Date | |
Msg-id | CAEze2Wja8VUoZygCepwUeiCrWa4jP316k0mvJrOW4PFmWP0Tcw@mail.gmail.com Whole thread Raw |
In response to | Re: Experiments with Postgres and SSL (Heikki Linnakangas <hlinnaka@iki.fi>) |
Responses |
Re: Experiments with Postgres and SSL
|
List | pgsql-hackers |
On Thu, 28 Mar 2024, 13:37 Heikki Linnakangas, <hlinnaka@iki.fi> wrote: > > On 28/03/2024 13:15, Matthias van de Meent wrote: > > On Tue, 5 Mar 2024 at 15:08, Heikki Linnakangas <hlinnaka@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
Attachment
pgsql-hackers by date: