On Tue, Sep 22, 2026 at 2:56 PM Daniel Gustafsson <[email protected]> wrote:
> In the v3 the ssl_sni value isn't reverted at all, which albeit confusing is
> in
> line with how we treat (and document) SSL configuration so I think thats the
> better option. Flipping it in existing sessions would require a lot more
> infrastructure for little gain.
> + # Grant pg_read_all_settings to ssltestuser so that relevant GUCs can be
> + # examined during tests
> + $node->psql('postgres', "GRANT pg_read_all_settings TO ssltestuser");
(This patch didn't introduce it, but $node->psql() can fail silently.
Something for later, maybe.)
> +my $log =
> + PostgreSQL::Test::Utils::slurp_file($node->logfile, $node_loglocation);
> +like(
> + $log,
> + qr/SNI is still on/,
> + 'SSL reload triggered WARNING on ssl_sni state');
Should be equivalent to
$node->log_check('SSL reload triggered WARNING on ssl_sni state',
$node_loglocation, log_like => [qr/SNI is still on/]);
> - if (!ssl_sni)
> + if (!SSL_hosts->sni_enabled)
> {
> + /* A configuration loaded without SNI must have a default host */
> install_config = SSL_hosts->default_host;
> + Assert(install_config != NULL);
> goto found;
> }
Idle thoughts from staring at this today: I was playing with an
alternative implementation like
if (SSL_hosts->default_host
&& SSL_hosts->sni == NIL
&& !SSL_hosts->no_sni)
{
/* If all we have is a default host, just use that. */
install_config = SSL_hosts->default_host;
goto found;
}
in the hopes that we could remove sni_enabled entirely. The logic
afterwards is roughly equivalent, except that we wouldn't perform
verification of the sent server_name extension if SNI is enabled with
only a "*" entry, and I'm not sure I like that. Getting rid of
sni_enabled also makes it difficult to log a WARNING...
> - if (ssl_sni)
> + if (new_hosts->sni_enabled)
> - if (!ssl_sni)
> + if ((SSL_hosts && !SSL_hosts->sni_enabled) || !ssl_sni)
This maybe overlaps with Zsolt's review, but these diffs don't feel
right. (In the second, I think SSL_hosts refers to the prior config?)
IMO we should continue to use ssl_sni directly here, since it's safe
to do so (IIUC) and the surrounding code still references it, and then
we can explicitly call out places where it's not safe to consult any
of the GUCs (such as the clienthello callback).
While I was reviewing this, I noticed separately that we leave
SSL_hosts and its memory context around after be_tls_destroy(). I
don't think it's leaked (if you turn ssl back on, the previous hosts
should be cleared out), but it is unused. I can't remember, did we do
that on purpose?
Thanks!
--Jacob
