From: | Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> |
---|---|
To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
Cc: | Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Greg Stark <stark(at)mit(dot)edu>, Andrey Borodin <amborodin86(at)gmail(dot)com>, Vladimir Sitnikov <sitnikov(dot)vladimir(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz> |
Subject: | Re: Experiments with Postgres and SSL |
Date: | 2024-04-04 11:08:08 |
Message-ID: | CAEze2Wja8VUoZygCepwUeiCrWa4jP316k0mvJrOW4PFmWP0Tcw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, 28 Mar 2024, 13:37 Heikki Linnakangas, <hlinnaka(at)iki(dot)fi> wrote:
>
> On 28/03/2024 13:15, Matthias van de Meent wrote:
> > On Tue, 5 Mar 2024 at 15:08, Heikki Linnakangas <hlinnaka(at)iki(dot)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 | Content-Type | Size |
---|---|---|
SELECT_NEXT_METHOD.diff.txt | text/plain | 2.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Melih Mutlu | 2024-04-04 11:08:45 | Re: Flushing large data immediately in pqcomm |
Previous Message | Amit Kapila | 2024-04-04 11:05:01 | Re: Introduce XID age and inactive timeout based replication slot invalidation |