Re: Experiments with Postgres and SSL

On 05/07/2023 02:33, Michael Paquier wrote:
> On Tue, Jul 04, 2023 at 05:15:49PM +0300, Heikki Linnakangas wrote:
>> I don't see the point of the libpq 'sslalpn' option either. Let's send ALPN
>> always.
>>
>> Admittedly having the options make testing different of combinations of old
>> and new clients and servers a little easier. But I don't think we should add
>> options for the sake of backwards compatibility tests.
>
> Hmm. I would actually argue in favor of having these with tests in
> core to stress the previous SSL hanshake protocol, as not having these
> parameters would mean that we rely only on major version upgrades in
> the buildfarm to test the backward-compatible code path, making issues
> much harder to catch. And we still need to maintain the
> backward-compatible path for 10 years based on what pg_dump and
> pg_upgrade need to support.

Ok, let's keep it.

I started to review this again. There's a lot of little things to fix
before this is ready for commit, but overall this looks pretty good. A
few notes / questions on the first two patches (in addition to the few
comments I made earlier):

If the client sends TLS HelloClient directly, but the server does not
support TLS, it just closes the connection. It would be nice to still
send some kind of an error to the client. Maybe a TLS alert packet? I
don't want to start implementing TLS, but I think a TLS alert packet
with a suitable error code would be just a constant.

The new CONNECTION_DIRECT_SSL_STARTUP state needs to be moved to end of
the enum. We cannot change the integer values of existing of enum
values, or clients compiled with old libpq version would mix up the states.

> /*
> * validate sslnegotiation option, default is "postgres" for the postgres
> * style negotiated connection with an extra round trip but more options.
> */

What "more options" does the negotiated connection provide?

> if (conn->sslnegotiation)
> {
> if (strcmp(conn->sslnegotiation, "postgres") != 0
> && strcmp(conn->sslnegotiation, "direct") != 0
> && strcmp(conn->sslnegotiation, "requiredirect") != 0)
> {
> conn->status = CONNECTION_BAD;
> libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
> "sslnegotiation", conn->sslnegotiation);
> return false;
> }
>
> #ifndef USE_SSL
> if (conn->sslnegotiation[0] != 'p') {
> conn->status = CONNECTION_BAD;
> libpq_append_conn_error(conn, "sslnegotiation value \"%s\" invalid when SSL support is not compiled in",
> conn->sslnegotiation);
> return false;
> }
> #endif
> }

At the same time, the patch allows the combination of "sslmode=disable"
and "sslnegotiation=requiredirect". Seems inconsistent to error out if
compiled without SSL support.

> else
> {
> libpq_append_conn_error(conn, "sslnegotiation missing?");
> return false;
> }

In the other similar settings, like 'channel_binding' and 'sslcertmode',
we strdup() the compiled-in default if the option is NULL. I'm not sure
if that's necessary, I think the compiled-in defaults should get filled
in conninfo_add_defaults(). If so, then those other places could be
turned into errors like this too. This seems to be a bit of a mess even
before this patch.

In pg_conn struct:

> + bool allow_direct_ssl_try; /* Try to make a direct SSL connection
> + * without an "SSL negotiation packet" */
> bool allow_ssl_try; /* Allowed to try SSL negotiation */
> bool wait_ssl_try; /* Delay SSL negotiation until after
> * attempting normal connection */

It's getting hard to follow what combinations of these booleans are
valid and what they're set to at different stages. I think it's time to
turn all these into one enum, or something like that.

I intend to continue reviewing this after Jan 8th. I'd still like to get
this into v17.

--
Heikki Linnakangas
Neon (https://neon.tech)