Re: Serverside SNI support in libpq

On Thu, Feb 27, 2025 at 5:38 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> Thanks for the tests, they did in fact uncover a bug in how fallback was
> handled which is now fixed. In doing so I revamped how the default context
> handling is done, it now always use the GUCs in postgresql.conf for
> consistency. The attached v6 rebase contains this as well as your tests as
> well as general cleanup and comment writing etc.

Great, thanks!

Revisiting my concerns from upthread:

On Thu, Jul 25, 2024 at 10:51 AM Jacob Champion
<jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> I tried patching all that, but I continue to see nondeterministic
> behavior, including the wrong certificate chain occasionally being
> served, and the servername callback being called twice for each
> connection (?!).

1) The wrong chain being served was due to the fallback bug, now fixed.
2) The servername callback happening twice is due to the TLS 1.3
HelloRetryRequest problem with our ssl_groups (which reminded me to
ping that thread [1]). Switching to TLSv1.2 in order to more easily
see the handshake on the wire makes the problem go away, which
probably did not help my sense of growing insanity last July.

> https://github.com/openssl/openssl/issues/6109
>
> Matt Caswell appears to be convinced that SSL_set_SSL_CTX() is
> fundamentally broken.

We briefly talked about this in Brussels, and I've been trying to find
proof. Attached are some (very rough) tests that might highlight an
issue.

Basically, the new tests set up three hosts in pg_hosts.conf: one with
no client CA, one with a valid client CA, and one with a
malfunctioning CA (root+server_ca, which can't verify our client
certs). Then it switches out the default CA underneath to make sure it
does not affect the visible behavior, since that CA should not
actually be used in the end.

Unfortunately, the failure modes change depending on the default CA.
If it's not a bug in my tests, I think this may be an indication that
SSL_set_SSL_CTX() isn't fully switching out the client verification
behavior? For example, if the default CA isn't set, the other hosts
don't appear to ask for a client certificate even if they need one.
And vice versa.

--

> + /*
> + * Set flag to remember whether CA store has been loaded into this
> + * SSL_context.
> + */
> + if (host->ssl_ca)

I think this should be `if (host->ssl_ca[0])` -- which, incidentally,
fixes one of the new failing tests on my machine.

> int
> be_tls_init(bool isServerStart)
> +{
> + SSL_CTX *ctx;
> + List *sni_hosts = NIL;
> + HostsLine line;

A pointer to `line` is passed down to ssl_init_context(), but it's
only been partially initialized on the stack. Can it be
zero-initialized here instead?

> + if (ssl_snimode == SSL_SNIMODE_STRICT)
> + {
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("no hostname provided in callback")));
> + return SSL_TLSEXT_ERR_ALERT_FATAL;
> + }

At the moment we're sending an `unrecognized_name` alert in strict
mode if the client doesn't send SNI. RFC 8446 suggests
`missing_extension`:

Additionally, all implementations MUST support the use of the
"server_name" extension with applications capable of using it.
Servers MAY require clients to send a valid "server_name" extension.
Servers requiring this extension SHOULD respond to a ClientHello
lacking a "server_name" extension by terminating the connection with
a "missing_extension" alert.

Should we do that, or should we ignore the suggestion? The problem
with missing_extension, IMO, is that there's absolutely no indication
to the client as to which extension is missing. unrecognized_name is a
little confusing in this case (there was no name sent), but at least
the end user will be able to link that to an SNI problem via search
engine.

> +#hosts_file = 'ConfigDir/pg_hosts.conf' # hosts configuration file
> + # (change requires restart)

Nitpickiest nitpick: looks like the other lines use a tab instead of a
space between the setting and the trailing comment.

Thanks,
--Jacob

[1] https://postgr.es/m/CAOYmi%2BnTwu7%3DaUGCkf6L-ULqS8itNP7uc9nUmNLOvbXf2TCgBA%40mail.gmail.com