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)