Subject: Re: Serverside SNI support in libpq

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 3 Dec 2024 14:58:01 +0100

> On 25 Jul 2024, at 19:51, Jacob Champion <[email-masked]> wrote:

The attached rebased version adds proper list reset, a couple of bugfixes
around cert loading and the ability to set ssl_passhprase_command (and reload)
in the hosts file.

> Matt Caswell appears to be convinced that SSL_set_SSL_CTX() is
> fundamentally broken. So it might just be FUD, but I'm wondering if we
> should instead be using the SSL_ flavors of the API to reassign the
> certificate chain on the SSL pointer directly, inside the callback,
> instead of trying to set them indirectly via the SSL_CTX_ API.

Maybe, but I would feel better about changing if I can could reproduce the
issues (see below).

> Have you seen any weird behavior like this on your end? I'm starting
> to doubt my test setup...

I've not been able to reproduce any behaviour like what you describe.

> On the plus side, I now have a handful of
> debugging patches for a future commitfest.

Do you have them handy for running tests on this version?

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Tue, 3 Dec 2024 16:43:18 -0800

> I've not been able to reproduce any behaviour like what you describe.

Hm, v2 is different enough that I'm going to need to check my notes
and try to reproduce again. At first glance, I am still seeing strange
reload behavior (e.g. issuing `pg_ctl reload` a couple of times in a
row leads to the server disappearing without any log messages
indicating why).

> Do you have them handy for running tests on this version?

I'll work on cleaning them up. I'd meant to contribute them
individually by now, but I got a bit sidetracked...

Thanks!
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 4 Dec 2024 14:44:18 +0100

> On 4 Dec 2024, at 01:43, Jacob Champion <[email-masked]> wrote:
> 

No worries, I know you have a big path on your plate right now.  The attached
v3 fixes a small buglet in the tests and adds silly reload testing to try and
stress out any issues.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Wed, 11 Dec 2024 09:34:40 +0900

> v3 fixes a small buglet in the tests and adds silly reload testing to try and

Looks like this still fails quite heavily in the CI..  You may want to
look at that.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 11 Dec 2024 09:13:35 +0100

> On 11 Dec 2024, at 01:34, Michael Paquier <[email-masked]> wrote:
> 

Interestingly enough the CFBot hasn't picked up that there are new version
posted and the buildfailure is from the initial patch in the thread, which no
longer applies (as the CFBot righly points out).  I'll try posting another
version later today to see if that gets it unstuck.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 20 Feb 2025 00:12:50 +0100

Attached is a rebase which fixes a few smaller things (and a pgperltidy run);
and adds a paragraph to the docs about how HBA clientname settings can't be
made per certificate set in an SNI config.  As discussed with Jacob offlist,
there might be a case for supporting that but it will be a niche usecase within
a niche feature, so rather than complicating the code for something which might
never be used, it's likely better to document it and await feedback.

Are there any blockers for getting this in?

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Mon, 24 Feb 2025 13:51:02 -0800

> +           SSL_context = ssl_init_context(isServerStart, host);

I'm still not quite following the rationale behind the SSL_context
assignment. To maybe illustrate, attached are some tests that I
expected to pass, but don't.

After adding an additional host and reloading the config, the behavior
of the original fallback host seems to change. Am I misunderstanding
the designed fallback behavior, have I misdesigned my test, or is this
a bug?

Thanks,
--Jacob
diff --git a/src/test/ssl/t/004_sni.pl b/src/test/ssl/t/004_sni.pl
index 0542c59ebcb..e183a953ede 100644
--- a/src/test/ssl/t/004_sni.pl
+++ b/src/test/ssl/t/004_sni.pl
@@ -57,6 +57,32 @@ $node->connect_ok(
 	"$connstr sslrootcert=ssl/root+server_ca.crt sslmode=require",
 	"connect with correct server CA cert file sslmode=require");
 
+# This is added only for comparison with the same test case below.
+$node->connect_fails(
+	"$connstr sslrootcert=ssl/root_ca.crt sslmode=verify-ca",
+	"connect fails with fallback hostname, without intermediate",
+	expected_stderr => qr/certificate verify failed/);
+
+# example.org serves the server cert and its intermediate CA.
+$node->append_conf('pg_hosts.conf',
+	"example.org server-cn-only+server_ca.crt server-cn-only.key root_ca.crt"
+);
+$node->reload;
+
+$node->connect_ok(
+	"$connstr host=example.org sslrootcert=ssl/root_ca.crt sslmode=verify-ca",
+	"connect with configured hostname, serving intermediate server CA");
+
+# Why does this test fail?
+$node->connect_fails(
+	"$connstr sslrootcert=ssl/root_ca.crt sslmode=verify-ca",
+	"connect still fails with fallback hostname, without intermediate",
+	expected_stderr => qr/certificate verify failed/);
+
+$node->connect_ok(
+	"$connstr host=localhost sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca",
+	"connect with fallback hostname, intermediate included");
+
 ok(unlink($node->data_dir . '/pg_hosts.conf'));
 $node->append_conf('pg_hosts.conf',
 	"localhost server.crt server.key root.crt");

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 27 Feb 2025 14:38:24 +0100

> On 24 Feb 2025, at 22:51, Jacob Champion <[email-masked]> 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.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Tue, 4 Mar 2025 13:57:09 -0800

> 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
<[email-masked]> 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
>
> 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
commit a4d9cbf4d1228dcc17f2961b7811321a50e74617
Author: Jacob Champion <[email-masked]>
Date:   Tue Mar 4 13:17:12 2025 -0800

    Tests

diff --git a/src/test/ssl/t/004_sni.pl b/src/test/ssl/t/004_sni.pl
index f0ce048273a..72e64c6c00d 100644
--- a/src/test/ssl/t/004_sni.pl
+++ b/src/test/ssl/t/004_sni.pl
@@ -33,6 +33,11 @@ if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
 
 my $ssl_server = SSL::Server->new();
 
+sub sslkey
+{
+	return $ssl_server->sslkey(@_);
+}
+
 my $node = PostgreSQL::Test::Cluster->new('primary');
 $node->init;
 
@@ -161,4 +166,57 @@ $node->connect_fails(
 	"connect fails since the passphrase protected key cannot be reloaded",
 	expected_stderr => qr/tlsv1 unrecognized name/);
 
+# Test client CAs.
+
+$connstr =
+  "user=ssltestuser dbname=certdb hostaddr=$SERVERHOSTADDR sslmode=require sslsni=1";
+
+ok(unlink($node->data_dir . '/pg_hosts.conf'));
+# example.org has an unconfigured CA.
+$node->append_conf('pg_hosts.conf', 'example.org server-cn-only.crt server-cn-only.key ""');
+# example.com uses the client CA.
+$node->append_conf('pg_hosts.conf', 'example.com server-cn-only.crt server-cn-only.key root+client_ca.crt');
+# example.net uses the server CA (which is wrong).
+$node->append_conf('pg_hosts.conf', 'example.net server-cn-only.crt server-cn-only.key root+server_ca.crt');
+$node->reload;
+
+my @cases = ( "", "root+client_ca", "root+server_ca" );
+foreach my $default_ca (@cases)
+{
+	# The default CA should, ideally, not matter for the purposes of these
+	# tests, since we connect to the other hosts explicitly.
+	$ssl_server->switch_server_cert(
+		$node,
+		certfile => 'server-cn-only',
+		cafile => $default_ca);
+
+	# example.org is unconfigured and should fail.
+	$node->connect_fails(
+		"$connstr host=example.org sslcertmode=require sslcert=ssl/client.crt " . sslkey('client.key'),
+		"example.org, $default_ca: connect with sslcert, no client CA configured",
+		expected_stderr => qr/client certificates can only be checked if a root certificate store is available/);
+
+	# example.com is configured and should require a valid client cert.
+	$node->connect_fails(
+		"$connstr host=example.com sslcertmode=disable",
+		"example.com, $default_ca: connect fails if no client certificate sent",
+		expected_stderr => qr/connection requires a valid client certificate/);
+
+	$node->connect_ok(
+		"$connstr host=example.com sslcertmode=require sslcert=ssl/client.crt " . sslkey('client.key'),
+		"example.com, $default_ca: connect with sslcert, client certificate sent");
+
+	# example.net is configured and should require a client cert, but will
+	# always fail verification.
+	$node->connect_fails(
+		"$connstr host=example.net sslcertmode=disable",
+		"example.net, $default_ca: connect fails if no client certificate sent",
+		expected_stderr => qr/connection requires a valid client certificate/);
+
+	$node->connect_fails(
+		"$connstr host=example.net sslcertmode=require sslcert=ssl/client.crt " . sslkey('client.key'),
+		"example.net, $default_ca: connect with sslcert, client certificate sent",
+		expected_stderr => qr/unknown ca/);
+}
+
 done_testing();
diff --git a/src/test/ssl/t/SSL/Backend/OpenSSL.pm b/src/test/ssl/t/SSL/Backend/OpenSSL.pm
index e044318531f..bdcce84003e 100644
--- a/src/test/ssl/t/SSL/Backend/OpenSSL.pm
+++ b/src/test/ssl/t/SSL/Backend/OpenSSL.pm
@@ -71,6 +71,7 @@ sub init
 	chmod(0600, glob "$pgdata/server-*.key")
 	  or die "failed to change permissions on server keys: $!";
 	_copy_files("ssl/root+client_ca.crt", $pgdata);
+	_copy_files("ssl/root+server_ca.crt", $pgdata);
 	_copy_files("ssl/root_ca.crt", $pgdata);
 	_copy_files("ssl/root+client.crl", $pgdata);
 	mkdir("$pgdata/root+client-crldir")
@@ -145,7 +146,8 @@ following parameters are supported:
 =item cafile => B<value>
 
 The CA certificate file to use for the C<ssl_ca_file> GUC. If omitted it will
-default to 'root+client_ca.crt'.
+default to 'root+client_ca.crt'. If empty, no C<ssl_ca_file> configuration
+parameter will be set.
 
 =item certfile => B<value>
 
@@ -180,10 +182,11 @@ sub set_server_cert
 	  unless defined $params->{keyfile};
 
 	my $sslconf =
-		"ssl_ca_file='$params->{cafile}.crt'\n"
-	  . "ssl_cert_file='$params->{certfile}.crt'\n"
+		"ssl_cert_file='$params->{certfile}.crt'\n"
 	  . "ssl_key_file='$params->{keyfile}.key'\n"
 	  . "ssl_crl_file='$params->{crlfile}'\n";
+	$sslconf .= "ssl_ca_file='$params->{cafile}.crt'\n"
+	  if $params->{cafile} ne "";
 	$sslconf .= "ssl_crl_dir='$params->{crldir}'\n"
 	  if defined $params->{crldir};

---

From: Andres Freund <[email-masked]>
Date: Tue, 13 May 2025 09:46:32 -0400

Hi,

On 2025-02-27 14:38:24 +0100, Daniel Gustafsson wrote:
> The attached v6 rebase contains this as well as your tests as well as
> general cleanup and comment writing etc.

This is not passing CI on windows...
https://cirrus-ci.com/build/4765059278176256

Greetings,

Andres

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 27 Aug 2025 21:49:34 +0200

> On 13 May 2025, at 15:46, Andres Freund <[email-masked]> wrote:

> https://cirrus-ci.com/build/4765059278176256

When looking into why the SNI tests failed on Windows I think I found a
pre-existing issue that we didn't have tests for, which my patch added tests
for and thus broke.

The test I added was to check restarting and reloading with ssl passphrase
commands (which we do have testcoverage for) with a subsequent connection test
to ensure it didn't just work to start the cluster.

When ssl_passphrase_command_supports_reload is set to 'off', the cluster should
allow connections until a reload has been issued.  That works fine except on
Windows where our process-model is such that a new connection will re-run the
passphrase command, which inevitably fails as it's not configured for reload.
The test in my patch exposed this out of (happy) accident, but it can be
reproduced in HEAD as well.  The attached version modifies the ssl tests to
cover this with a connection attempt.  If I'm not mistaken though, there should
probably be a docs patch to make it clear how this works on Windows.

No codechanges on top of the test fix.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Mon, 1 Sep 2025 10:58:29 +0900

> to ensure it didn't just work to start the cluster.

Would this part be better if extracted from the main patch and then
backpatched?  Even if not backpatched, a split would be cleaner on
HEAD, I assume, leading to less fuzz with the main patch.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 2 Sep 2025 14:48:43 +0200

> On 1 Sep 2025, at 03:58, Michael Paquier <[email-masked]> wrote:
> 

Yes, that's my plan, just wanted to float it here first to see if I was
thinking about it all wrong.  I will raise it on its own thread on -hackers.
The backpatchable portion is probably limited to a docs entry clarifying the
behaviour on Windows.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 10 Nov 2025 23:32:43 +0100

Attached is a cleaned up rebase with improved memory handling, additional code
documentation, removed passphrase test (sent as a separate thread), and some
general cleanup and additional testing.

--
Daniel Gustafsson

---

From: Chao Li <[email-masked]>
Date: Tue, 11 Nov 2025 17:06:49 +0800

Hi Daniel,

I just reviewed the patch and got a few comments:

> On Nov 11, 2025, at 06:32, Daniel Gustafsson <[email-masked]> wrote:
> 
> <v9-0001-Serverside-SNI-support-for-libpq.patch>

1 - commit message
```
Experimental support for serverside SNI support in libpq, a new config
file $datadir/pg_hosts.conf is used for configuring which certicate and
```

Typo: certicate -> certificate

2 - be-secure-common.c
```
+run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size, void *userdata)
 {
 	int			loglevel = is_server_start ? ERROR : LOG;
 	char	   *command;
 	FILE	   *fh;
 	int			pclose_rc;
 	size_t		len = 0;
+	char	   *cmd = (char *) userdata;
```

Cmd is only passed to replace_percent_placeholders(), and the function take a "const char *” argument, so we can define cmd as “const char *”.

2 - be-secure-common.c
```
+	tokenize_auth_file(HostsFileName, file, &hosts_lines, LOG, 0);
+
+	foreach(line, hosts_lines)
+	{
+		TokenizedAuthLine *tok_line = (TokenizedAuthLine *) lfirst(line);
+
+		if (tok_line->err_msg != NULL)
+		{
+			ok = false;
+			continue;
+		}
+
+		if ((newline = parse_hosts_line(tok_line, LOG)) == NULL)
+		{
+			ok = false;
+			continue;
+		}
+
+		parsed_lines = lappend(parsed_lines, newline);
+	}
+
+	free_auth_file(file, 0);
```

When I read this function, I thought to raise a comment for freeing hosts_lines. However, then I read be-secure-openssl.c, I saw the load_hosts() is called within a transient hostctx, so it doesn’t have to free memory pieces. Can we explain that in the function comment? Otherwise other reviewers and future code readers may have the same confusion.

3 - be-secure-openssl.c
```
 int
@@ -759,6 +933,9 @@ be_tls_close(Port *port)
 		pfree(port->peer_dn);
 		port->peer_dn = NULL;
 	}
+
+	Host_context = NULL;
+	SSL_context = NULL;
 }
```

Do we need to free_contexts() here? When be_tls_init() is called again, contexts will anyway be freed, so I guess earlier free might be better?

4 - guc_parameters.dat
```
+{ name => 'hosts_file', type => 'string', context => 'PGC_POSTMASTER', group => 'FILE_LOCATIONS',
+  short_desc => 'Sets the server\'s "hosts" configuration file.',
+  flags => 'GUC_SUPERUSER_ONLY',
+  variable => 'HostsFileName',
+  boot_val => 'NULL',
+},

+{ name => 'ssl_snimode', type => 'enum', context => 'PGC_SIGHUP', group => 'CONN_AUTH_SSL',
+  short_desc => 'Sets the SNI mode to use for the server.',
+  flags => 'GUC_SUPERUSER_ONLY',
+  variable => 'ssl_snimode',
+  boot_val => 'SSL_SNIMODE_DEFAULT',
+  options => 'ssl_snimode_options',
+},
```
 
If ssl_snimode is PGC_SIGHUP that allows to reload without a server reset, why hosts_file cannot?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Jacob Champion <[email-masked]>
Date: Wed, 12 Nov 2025 14:44:39 -0800

> general cleanup and additional testing.

Thanks! Builds and passes back to OpenSSL 1.1.1 and LibreSSL 3.4
(except for the unrelated known issue with "depth 0"/"depth 1", which
this patch did not introduce [1]).

Did you have any thoughts on my earlier review [2]? The test patch
attached there still fails on my machine with v9.

Thanks,
--Jacob

[1] https://postgr.es/m/CA%2BhUKG%2BfLqyweHqFSBcErueUVT0vDuSNWui-ySz3%2Bd_APmq7dw%40mail.gmail.com
[1] https://postgr.es/m/CAOYmi%2Bk%3DVF-2BCqfR49A92tx%3D_QNuL%3D3iT3w6FysOffKw9cxDQ%40mail.gmail.com

---

From: Jacob Champion <[email-masked]>
Date: Wed, 12 Nov 2025 14:50:36 -0800

> If ssl_snimode is PGC_SIGHUP that allows to reload without a server reset, why hosts_file cannot?

I think all of our FILE_LOCATIONS GUCs are handled similarly.

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 13 Nov 2025 00:03:11 +0100

> On 12 Nov 2025, at 23:44, Jacob Champion <[email-masked]> wrote:

> attached there still fails on my machine with v9.

Oh shoot, I missed that when going back over the thread.  Will have a look.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 24 Nov 2025 15:53:31 +0100

> attached there still fails on my machine with v9.

The attached incorporates your tests, fixes them to make them pass.  The
culprit seemed to be a combination of a bug in the code (the verify callback
need to be defined in the default context even if there is no CA for it to be
called in an SNI setting because OpenSSL), and that the tests were matching
backend errors against frontend messages.

The other comments from your review are also addressed, as well as additional
cleanup and improved error handling.

--
Daniel Gustafsson

---

From: Chao Li <[email-masked]>
Date: Tue, 25 Nov 2025 07:28:00 +0800

Hi Daniel,

> If ssl_snimode is PGC_SIGHUP that allows to reload without a server reset, why hosts_file cannot?

Comment 4 can be ignored as Jacob has answered.

> On Nov 24, 2025, at 22:53, Daniel Gustafsson <[email-masked]> wrote:
> 
> <v10-0001-Serverside-SNI-support-for-libpq.patch>

I reviewed v10 again, and got some a few more comments:

5 - runtime.sgml
```
+    in <filename>pg_hba.conf</filename>.  <replaceable>hostname</replaceable>
+    is matched against the hostname TLS extension in the SSL handshake.
```

In the patch code, default context uses hostname “*”, should we explain “*” here in the doc?

6 - runtime.sgml
```
+    <filename>pg_hosts.conf</filename>, which is stored in the clusters
+    data directory.  The hosts configuration file contains lines of the general
```

Typo: clusters => cluster’s

7 - runtime.sgml
```
+    will only be used to for the handshake until the hostname is inspected, it
```

“Used to for” => “used for"

8 - Cluster.pm
```
+matching the specified pattern. If the pattern matches agsinst the logfile a
```

Typo: agsinst => against

Best regards,

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 25 Nov 2025 15:39:22 +0100

> On 25 Nov 2025, at 00:28, Chao Li <[email-masked]> wrote:
> 
> Hi Daniel,
> 

I do apologise, I was so focused on fixing Jacob's tests that I forgot about
addressing these.  Please find the attached v11 with your comments addressed.
Thank you for all your review, much appreciated!

>> Typo: certicate -> certificate

Fixed.  I also reworded the commit message from saying experimental since we
don't have a concept of experimental features really.

>> Cmd is only passed to replace_percent_placeholders(), and the function take a "const char *” argument, so we can define cmd as “const char *”.

Fixed.

>> When I read this function, I thought to raise a comment for freeing hosts_lines. However, then I read be-secure-openssl.c, I saw the load_hosts() is called within a transient hostctx, so it doesn’…

I expanded the comment, and while there also improved the error reporting from
the function by returning a bool indicating status as well as the list (since
NIL was both empty-file and error).

>> Do we need to free_contexts() here? When be_tls_init() is called again, contexts will anyway be freed, so I guess earlier free might be better?

I don't think so, be_tls_close is only for closing the session.

> In the patch code, default context uses hostname “*”, should we explain “*” here in the doc?

I don't think we should since we don't want anyone to configure a host with
'*'.  That does bring up a good point though, and I added a check in the
parsing to ensure that such wildcard hostnames cause failures in parsing if
found in pg_hosts.

> Typo: clusters => cluster’s

Fixed.

> “Used to for” => “used for"

Fixed.

> +matching the specified pattern. If the pattern matches agsinst the logfile a

Fixed.

--
Daniel Gustafsson

---

From: "Dewei Dai" <[email-masked]>
Date: Wed, 26 Nov 2025 17:14:52 +0800

Hi Daniel,
   I just reviewed the v11 patch and got a few comments:

1  - commit message
```This adds support for serverside SNI such that certficate/key handling
```
Typo: certficate ->  certificate 

2  -be-secure-openssl.c
```* host/snimode match, but we need something to drive the hand- shake till
```
Typo: hand- shake ->handshake

3 - be-secure-openssl.c
```
errhint("In strict ssl_snimode there need to be at least one entry in pg_hosts.conf."));
there needs to be
```
Typo: There need to be  -> there needs to be

4 - src/backend/makefile
   It is recommended to delete pg_hosts.conf.sample during the `make uninstall`  command

5  - be-secure-openssl.c
```
be_tls_destroy(void)
 {
+ ListCell   *cell;
+
+ foreach(cell, contexts)
+ {
+ HostContext *host_context = lfirst(cell);
+
+ SSL_CTX_free(host_context->context);
+ pfree(host_context);
+ }
`````
In the `be_tls_destroy` function, the context is released, but it is not set to null. 
         This is similar to the `free_context` function, and it seems that it can be called directly.

 Best regards

[email-masked]

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 26 Nov 2025 15:33:07 +0100

> On 26 Nov 2025, at 10:14, Dewei Dai <[email-masked]> wrote:
> 
> Hi Daniel,

Thanks!

Fixed.

Fixed.

> Typo: There need to be  -> there needs to be

AFAIK "need to be" is the correct spelling for referring to a singular thing,
and "needs to be" is correct for plural.  I've been thinking about this in a
singular context but maybe "needs to be" is the right wording since the hint is
"at least one".  Changed to "needs to be" just in case.

>    It is recommended to delete pg_hosts.conf.sample during the `make uninstall`  command

Nice catch, fixed.

>          This is similar to the `free_context` function, and it seems that it can be called directly.

That's a good point, be_tls_destroy can just call free_contexts directly and
save some code while making sure it's consistent.  Fixed.

--
Daniel Gustafsson

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 3 Dec 2025 11:57:32 +0200

Sorry for jumping in so late.

On Fri, May 10, 2024 at 7:23 AM Daniel Gustafsson 
<daniel(at)yesql(dot)se> wrote:
> The attached patch adds serverside SNI support to libpq, it is still a bit
> rough around the edges but I'm sharing it early to make sure I'm not designing
> it in a direction that the community doesn't like.  A new config file
> $datadir/pg_hosts.conf is used for configuring which certicate and key should
> be used for which hostname.  The file is parsed in the same way as pg_ident
> et.al so it allows for the usual include type statements we support.  A new
> GUC, ssl_snimode, is added which controls how the hostname TLS extension is
> handled.  The possible values are off, default and strict:
> 
> 
>       - off: pg_hosts.conf is not parsed and the hostname TLS extension is
>         not inspected at all. The normal SSL GUCs for certificates and keys
>         are used.
>       - default: pg_hosts.conf is loaded as well as the normal GUCs. If no
>         match for the TLS extension hostname is found in pg_hosts the cert
>         and key from the postgresql.conf GUCs is used as the default (used
>         as a wildcard host).
>       - strict: only pg_hosts.conf is loaded and the TLS extension hostname
>         MUST be passed and MUST have a match in the configuration, else the
>         connection is refused.
> 
> 
> As of now the patch use default as the initial value for the GUC

Do we need the GUC? It feels a little confusing that a GUC affects how 
the settings in the pg_hosts.conf are interepreted. It'd be nice if you 
could open pg_hosts.conf in an editor, and see at one glance everything 
that affects this.

I propose that there is no GUC. In 'pg_hosts.conf', you can specify a 
wildcard '*' host that matches anything. You can also specify a "no sni" 
line which matches connections with no SNI specified. (Or something 
along those lines, I didn't think too hard about all the interactions).

Should we support wildcards like "*.example.com* too?

For backwards-compatibility, if you specify a certificate and key in 
postgresql.conf, they are treated the same as if you had a "*" line in 
pg_hosts.conf.

- Heikki

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 3 Dec 2025 17:52:09 +0100

> On 3 Dec 2025, at 10:57, Heikki Linnakangas <[email-masked]> wrote:
> 

Not at all, thanks for looking!

> Do we need the GUC? It feels a little confusing that a GUC affects how the settings in the pg_hosts.conf are interepreted. It'd be nice if you could open pg_hosts.conf in an editor, and see at one glance everything that affects this.

I added the GUC for two reasons; as a way to opt-out of this feature if it's
something that the admin doesn't want; and as a way to set the SNI mode.  There
are currently the two modes of STRICT and DEFAULT which affects how incoming
connections are handled.  The first motivation might be unfounded, and the
second one could be encoded in a pg_hosts configuration though implicitly
rather than explicitly. 

Having all the details in pg_hosts.conf is appealing, no disagreement there,
but it does pose some challenges in the interaction with the postgresql.conf
GUCS (more later).

> I propose that there is no GUC. In 'pg_hosts.conf', you can specify a wildcard '*' host that matches anything. You can also specify a "no sni" line which matches connections with no SNI specified. (Or something along those lines, I didn't think too hard about all the interactions).

So basically reserving a hostname,"no_sni" or something, which indicates that
it's for non sslsni connections?  That should work, with the parsing rule that
there can only be one in the file.

> Should we support wildcards like "*.example.com* too?

I have that on my if-it-gets-committed TODO but I kept it out of the initial
proposal to keep complexity down and goalposts in sight.

> For backwards-compatibility, if you specify a certificate and key in postgresql.conf, they are treated the same as if you had a "*" line in pg_hosts.conf.

That's a bit trickier though, since the cert/key have a default boot_val so
they will always be set to something unless the user enables ssl=on and at the
same time uncomments ssl_cert_file/ssl_key_file and set them to '' before
proceeding to add configuration in pg_hosts.conf.  This is pretty unintuitive I
think.  unintuitive.  This backwards comatibility is one of the reasons I kept
the postgresl.conf values for the default context config.

I really want to make it possible for anyone who don't want SNI to keep using
postgresql.conf and get the exact behavior they've always had.  Do you agree
with that design goal?

--
Daniel Gustafsson

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 3 Dec 2025 18:56:59 +0200

> there can only be one in the file.

Yeah, something like that. And to implement the "strict" mode, you could 
have a "no_sni" line with no cert/key specified.

> postgresql.conf and get the exact behavior they've always had.  Do you agree

Yeah, that's fair.

- Heikki

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 3 Dec 2025 22:27:43 +0100

> > postgresql.conf and get the exact behavior they've always had.  Do you agree

What if we make it so that if a pg_hosts.conf file exists, then the
ssl_cert_file/ssl_key_file configs are ignored? And by default initdb
would not create a file (or it would, but with the same default
settings that we have now). Then we don't need the new GUC. Basically
it would be:
1. If the file does not exist, use the "off" behaviour
2. If the file exists, use the "strict" behaviour

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 4 Dec 2025 00:27:53 +0100

> On 3 Dec 2025, at 22:27, Jelte Fennema-Nio <[email-masked]> wrote:
> 
> settings that we have now).

Maybe.  I'm not a big fan of magic-file-exist configurations but..  I'm trying
out a few different options to see which seems the most reasonable, and this is
for one of them.

> Basically it would be:

It will really be "strict" *or* "default" based on whether or not '*' is set as
a wildcard hostname (which can be argued is just a version of strict).

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 11 Dec 2025 09:47:28 -0800

Hi!

> backend errors against frontend messages.

The new v12 tests still don't pass for me (they all use "certificate
verify failed", but the failure modes should be different).

> +           if (host->ssl_ca && host->ssl_ca[0] != '\0')

The comment for HostsLine.ssl_ca, and the code that assigns it,
implies to me that host->ssl_ca should never be NULL. Am I missing a
case where it could be?

> along those lines, I didn't think too hard about all the interactions).

That seems to position SNI as a feature that every DBA should have to
think about by default. ("learn this file. you can't turn it off.") Is
it, yet?

Web servers enable SNI implicitly because name-based hosting is a
top-level concept for users over there (hostnames are baked into the
application layer). I would argue that we don't have that here. Maybe
in the future someone will ask for that, but at that point don't you
want a very different, name-based, config system?

On Wed, Dec 3, 2025 at 3:28 PM Daniel Gustafsson <[email-masked]> wrote:
> Maybe.  I'm not a big fan of magic-file-exist configurations

Me neither. (I especially don't like the idea of ignoring a
certificate+key setting that a user has taken the time to put into a
config.)

Thanks,
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 11 Dec 2025 18:51:53 +0100

> On 11 Dec 2025, at 18:47, Jacob Champion <[email-masked]> wrote:

> verify failed", but the failure modes should be different).

In which version of OpenSSL (or LibreSSL)?

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 11 Dec 2025 11:40:58 -0800

> In which version of OpenSSL (or LibreSSL)?

1.1.1 through 3.6. The CI for this commitfest entry shows it too:

    https://cirrus-ci.com/task/5648027525840896

Local diff that missed `git add`, maybe?

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 12 Dec 2025 12:41:36 +0100

> verify failed", but the failure modes should be different).

I'm still not sure why they pass for me locally with that error, but I've
updated to patch to match CI.

> implies to me that host->ssl_ca should never be NULL. Am I missing a

The attached version allows ssl_ca to be omitted from the pg_host config to
match the ssl_ca GUC.

>> along those lines, I didn't think too hard about all the interactions).

The attached version removes the GUC and instead sets a precedence rule that if
pg_hosts exists and is non-empty, it is exclusively used.  If it doesn't exist,
or is empty, then the regular SSL GUCs are used.

Further, pg_hosts is extended with handling * for default fallback, and no_sni
for rules targeting connections with no hostname.  The docs changes were harder
than implementing the code, suggestions on how to improve that part would be
greatly appreciated.

But, see below.

> certificate+key setting that a user has taken the time to put into a

I wonder if the way forward is to do both?  Heikki has a good point that when
working with pg_hosts.conf it should be clear from just that file what the
final config will be, and in the previous version that wasn't the case since
the ssl_snimode GUC set operation modes.  At the same time, Jacob has a point
that overriding configuration just because pg_hosts exists isn't transparent.

Adding a boolean GUC which turns ph_hosts (and thus SNI) on or off can perhaps
fix both complaints?  If the GUC is on, pg_hosts - and only pg_hosts - is used
for configuring secrets.  By using the * fallback and no_sni rule in pg_hosts
all variations of configs can be achieved.  If the GUC is off, then the regular
SSL GUCs are used and pg_host is never considered (and thus SNI is not
possible).

Such a GUC wouldn't make the patch all that much different from what it is
right now. What do you think about that middleground proposal?

--
Daniel Gustafsson

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 17 Dec 2025 11:03:07 +0200

>> certificate+key setting that a user has taken the time to put into a

+1

> right now. What do you think about that middleground proposal?

I like that.

Instead of a boolean GUC, it could perhaps be a path to the pg_hosts 
file. I haven't thought this through but somehow it feels more natural 
to me than a "read this file or not" setting.

- Heikki

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 17 Dec 2025 11:06:36 +0200

> to me than a "read this file or not" setting.

I was thinking that the boolean GUC would be called something like 
"read_pg_hosts_file = on / off", which feels unnatural. But thinking 
about this more, if the GUC is called something like "enable_sni = on / 
off", that feels much better, and I like that more than my suggestion of 
specifying the path to the pg_hosts file.

- Heikki

---

From: Jacob Champion <[email-masked]>
Date: Wed, 17 Dec 2025 15:58:10 -0800

> The attached version allows ssl_ca to be omitted from the pg_host config to

Aha! I think ssl_ca should be moved into the "Optional fields" section
of `struct HostsLine` now.

> updated to patch to match CI.

There's one diff remaining from my old tests patch: the example.org
line doesn't set ssl_ca, so I expect

> -       expected_stderr => qr/unknown ca/);

because host_context->ssl_loaded_verify_locations should be false. But
that doesn't happen... Why?

> fix both complaints?

Sounds reasonable, I think.

--

Just checking my understanding: is the use case for no_sni primarily
that you should be able to strictly refuse clients who say they're
talking to someone else -- so you don't want a wildcard -- but you
still want to gracefully handle clients who don't speak SNI at all?

> +           else if (strcmp(host->hostname, "no_sni") == 0)
> +               no_sni_context = host_context;

Will anyone be mad at us for camping on the "no_sni" identifier? I
know technically underscore isn't allowed in DNS hostnames, buuuut [1,
2]

> +   /* Hostname */
> +   field = list_head(tok_line->fields);
> +   tokens = lfirst(field);
> +   token = linitial(tokens);
> +   parsedline->hostname = pstrdup(token->string);

We should probably check tokens->length to make sure that the user
hasn't passed more than one token for each field, similar to how
parse_hba_line() does it.

Should we support multiple hostname tokens in a single line, though,
and just copy the settings that follow across all of them? That would
allow you to collapse

    example.org  server.crt  server.key
    example.com  server.crt  server.key
    sub.example.com  server.crt  server.key
    *  other.crt  other.key

into

    example.org,example.com,sub.example.com  server.crt  server.key
    *  other.crt  other.key

or even

    @my-hostnames.txt  server.crt  server.key
    *  other.crt  other.key

Then you'd have a fighting chance at automatically generating the
lists, especially since we don't do wildcards yet.

--Jacob

[1] https://github.com/netty/netty/pull/8150
[2] https://github.com/openssl/openssl/issues/12566

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 18 Dec 2025 01:07:41 +0100

> On 18 Dec 2025, at 00:58, Jacob Champion <[email-masked]> wrote:
> 

Ah, yes.

> that doesn't happen... Why?

I'll have a look.

> still want to gracefully handle clients who don't speak SNI at all?

Yeah, pretty much.

> know technically underscore isn't allowed in DNS hostnames, buuuut [1,

Maybe, but I think that regardless of what we do someone will be mad.  The
other option would be to use another single character like '?' or something.
Not sure that will improve readability though.

> parse_hba_line() does it.

Good point, will do that.

> and just copy the settings that follow across all of them?

I've been hesitant to add too much complexity, but perhaps just allowing a
comma separated list is a good middle ground to avoid going full regex?

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 18 Dec 2025 09:06:19 -0800

> Not sure that will improve readability though.

Hm, I agree that's not readable. Especially since other famous server
implementations use ? to match a single character in server alias
names.

Maybe we could enclose no_sni with something that's emphatically not
DNS. Braces, brackets, etc.? If we had control over the lower level
tokenizer, we could tell people to double-quote it to disambiguate,
but I don't think we have access to that information at our level.

> comma separated list is a good middle ground to avoid going full regex?

I think it could be a pretty good bump in usability. Wildcards seem
ideal but the cost is much higher. Hopefully the cost of
comma-separated hosts is just an extra inner loop in the parser, plus
the extra tests?

I'm trying to put on my "what could we possibly regret" hat for these
next ones. They may be uselessly speculative:

- If the goal is to eventually support wildcards, will the use of a
bare catch-all asterisk conflict with your plans (if any)?
- What kind of normalization should we do? Currently, `example.com`
will not match `example.COM` and it seems like that might be a problem
for somebody.
- Do we need to consider IDNs and A-labels and U-labels? (Do we
support the latter today, at all?)

A nice-to-have v2ish feature might be to warn if the host configured
for a certificate cannot in fact match that certificate according to
OpenSSL.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 18 Dec 2025 10:20:21 -0800

> for a certificate cannot in fact match that certificate according to

Another wishlist item: the logs (both server- and client-side) are
pretty inscrutable when things fail right now. Server's relatively
easy to change, but I wonder if we can do something along the lines of
0b5d1fb36 to provide an extra hint on the client side?

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 13 Jan 2026 10:57:27 +0100

> of `struct HostsLine` now.

Done, and removed the now unused default_host member from the struct as well
which I had missed in the previous version.

> parse_hba_line() does it.

Done.

> but I don't think we have access to that information at our level.

I've changed to /no_sni/ in the attached patch which should make it safer, but
it can easily be changed to braces or brackets or something else entirely.

> comma-separated hosts is just an extra inner loop in the parser, plus

I've added support for lists of hostnames along with tests and docs for the
same.  The limitation is that one cannot specify '*' or '/no_sni/' in a list,
it must be just hostnames.  I haven't added support for @hostnames.txt yet to
keep scope under control, but it can be added as well (in the future if this
patch is committed).

> next ones. They may be uselessly speculative:

I really appreciate thinking about this!

> bare catch-all asterisk conflict with your plans (if any)?

Possibly, I guess it depends on how we define a wildcard scheme.  One solution
could perhaps be to use an enclosed name like the non-SNI case, like /default/
or something similar.

> will not match `example.COM` and it seems like that might be a problem

The attached use case insensitive comparison.  RFC 952 makes it clear that
hostnames are case insensitive, and RFC 921/1035 does the same for DNS.

> support the latter today, at all?)

There is nothing in the current patch which prevents supporting it in a future
update is there?

> for a certificate cannot in fact match that certificate according to

That would be quite nifty indeed.

I think the attached is pretty clear improvement over the previous version so
thanks for the review suggestions.  That being said, the test which was
reported to still fail upstream is failing here as well (it does the right
thing with the connection, but terminates the handshake in a different place).
In an attempt to fix that I moved to using the ClientHello callback which
OpenSSL document to be the right one (yet they use the servername callback
themselves), but it renders the same result.  I hope that your eagle eyes (or
someone elses) can figure out either what is wrong, or if this is a different
form of right.  The same failing test is added to 0001 to run it in a strictly
non-SNI config as well.

The attached also simplifies the tests you provided since there is no longer
any need to run the tests for different default values, as we no longer have
that mixed configfile handling it was intended to test.  The actual connection
tests remain though.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 16 Jan 2026 15:44:55 -0800

[skipping right to the weird part, will circle back to the other
questions later]

> form of right.  The same failing test is added to 0001 to run it in a strictly

I hadn't realized that this also regressed without SNI! That helped a lot.

With 0001, the bug is this diff, which runs the verify_cb regardless
of the ssl_ca setting:

> -       SSL_CTX_set_verify(context,
> -                          (SSL_VERIFY_PEER |
> -                           SSL_VERIFY_CLIENT_ONCE),
> -                          verify_cb);
>     }
>
> +   /*
> +    * Always ask for SSL client cert, but don't fail if it's not presented.
> +    * We might fail such connections later, depending on what we find in
> +    * pg_hba.conf.
> +    */
> +   SSL_CTX_set_verify(context,
> +                      (SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE),
> +                      verify_cb);

0002 undid that but reintroduced it in be_tls_open_server():

>     /* enable ALPN */
>     SSL_CTX_set_alpn_select_cb(SSL_context, alpn_cb, port);
>
> +   SSL_CTX_set_verify(SSL_context,
> +                      (SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE),
> +                      verify_cb);

If I remove that diff, the regression goes away. But then we start
failing SNI tests:

> [14:24:05.901](0.000s) not ok 72 - host: 'example.com', ca: 'root+client_ca.crt': connect with sslcert, client certificate sent: no stderr
> [14:24:05.902](0.000s) #   Failed test 'host: 'example.com', ca: 'root+client_ca.crt': connect with sslcert, client certificate sent: no stderr'
> #   at src/test/ssl/t/004_sni.pl line 314.
> [14:24:05.902](0.000s) #          got: 'psql: error: connection to server at "127.0.0.1", port 15428 failed: FATAL:  connection requires a valid client certificate'
> #     expected: ''
> [14:24:05.917](0.000s) not ok 76 - host: 'example.net', ca: 'root+server_ca.crt': connect with sslcert, client certificate sent: matches
> [14:24:05.917](0.000s) #   Failed test 'host: 'example.net', ca: 'root+server_ca.crt': connect with sslcert, client certificate sent: matches'
> #   at ssl/t/004_sni.pl line 327.
> [14:24:05.917](0.000s) #                   'psql: error: connection to server at "127.0.0.1", port 15428 failed: FATAL:  connection requires a valid client certificate'
> #     doesn't match '(?^:unknown ca)'

I think the root problem probably comes back to SSL_set_SSL_CTX [1].
That copies the certificate over from the new SSL_CTX, but it doesn't
really seem to care about much else, and there are a _lot_ of settings
copied into the SSL pointer during initial connection [2] that are
ignored there.

The verify mode and callback are two such settings. So is the password
callback (which may mean that the new per-host-line logic for
openssl_tls_init_hook won't work correctly either).

So unless Matt Caswell knows of an existing API that does this right,
I think I'm coming back to the idea that we should keep a single
SSL_CTX, and then use the selected HostsLine to override individual
connection settings during the clienthello/servername callback. Do we
give anything up with that approach?

--Jacob

[1] https://postgr.es/m/CAOYmi%2Bk%3DVF-2BCqfR49A92tx%3D_QNuL%3D3iT3w6FysOffKw9cxDQ%40mail.gmail.com
[2] https://github.com/openssl/openssl/blob/5d401004a0/ssl/ssl_lib.c#L731

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 6 Mar 2026 23:11:49 +0100

> On 17 Jan 2026, at 00:44, Jacob Champion <[email-masked]> wrote:

> give anything up with that approach?

After discussing this more off-list we collaborated on rewriting the mechanics
for switching out the SSL_CTX settings during SNI selection in the clienthello
callback. The attached version implements this modified approach.

The code now has a single main SSL_CTX object which is reconfigured rather than
swapped out.  The HostsLine struct, which keeps the parsed pg_hosts.conf
information, gains an SSL_CTX object which contains the host specific settings,
and this is where they are then copied to the single main during
reconfiguration.  The interface with Postgres and how SNI is configured has not
been changed at all.  Users who don't enable ssl_sni and configure SSL in the
usual way in postgresql.conf will not notice any difference from today (and
ssl_sni is set to off by default).

As discussed above, the tls_init hook will not work very well for a multi host
setup so in the attached it will only be executed when ssl_sni is set to off.
When ssl_sni is on the ssl_passphrase_cmd parameter will still be honored for
handling passphrases.

We also realized that LibreSSL doesn't support a lot of the functionality
required, as it is *IMHO* falling further and further behind OpenSSL in it's
compatibility layer.  The patch adds meson/autoconf checks for required API's
and require these to be present for ssl_sni to be enabled.  Longer term I think
we need to start thinking about splitting be-secure-openssl.c into a
be-secure-libressl.c to keep the ifdef soup from getting too bad.  Thats for
another patch however.

Some of the new tests added for this patchset turned out to be valuable on
their own as they fill a gap in coverage, they have been pulled out into 0001.
0002 has a few small TODO comments left but is feature complete.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 10 Mar 2026 14:11:00 +0100

The attached rebase adds a check, and testcase, for duplicated hostname entries
in pg_hosts.conf and errors out in case a host is configured multiple times.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 12 Mar 2026 15:36:00 +0100

> On 10 Mar 2026, at 14:11, Daniel Gustafsson <[email-masked]> wrote:
> 

And another small update to SKIP the newly added tests on LibreSSL since they
use sslmode require which is only available in OpenSSL.  No other changes to
the patchset in this version (apart from a freshly brewed rebase of course).

--
Daniel Gustafsson

---

From: Zsolt Parragi <[email-masked]>
Date: Fri, 13 Mar 2026 21:12:43 +0000

Hello

Originally I started looking at this thread because Jacob mentioned it
relates to the custom OAuth / HBA variable question[1]. While I do see
the relation, I don't have many practical ideas.

I was thinking about suggesting using a "key=value, key=value ..."
file style instead of the fixed table, both for easier later
generalization, and because it aligns better to modern configuration
formats (and personally I always find these columnar config files
harder to read)
However, it would also differ from other existing postgres config
files and wouldn't offer a clear initial advantage, so it doesn't seem
like a good practical choice. I'm still mentioning this for
completeness, but mostly I'll focus on a more practical review:

+  check_nook => 'check_ssl_sni',

This seems to be a typo?

+ if (SSL_client_hello_get0_ext(ssl, TLSEXT_TYPE_server_name, &tlsext, &left))
+ {
+ if (left <= 2)
+ {
+ *al = SSL_AD_MISSING_EXTENSION;
+ return 0;
+ }

 ... and later error returns in this if block seem to use the wrong
error code to me: truncated length, length mismatch, empty list,
length exceeding remaining data...

missing_extension:  Sent by endpoints that receive a handshake
      message not containing an extension that is mandatory to send for
      the offered TLS version or other negotiated parameters.

decode_error:  A message could not be decoded because some field was
      out of the specified range or the length of the message was
      incorrect.  This alert is used for errors where the message does
      not conform to the formal protocol syntax.  This alert should
      never be observed in communication between proper implementations,
      except when messages were corrupted in the network.

Since we are already inside the if which verifies that the extension
is present, shouldn't all of these report decode_error?

+ if (!ssl_update_ssl(ssl, install_config))
+ {
+ ereport(COMMERROR,
+ errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("failed to switch to SSL configuration for host, terminating
connection"));
+ return SSL_CLIENT_HELLO_ERROR;
+ }

Isn't there a missing *al = assignment here?

+ /*
+ * There should be no more tokens after this, if there are break
+ * parsing and report error to avoid silently accepting incorrect
+ * config.
+ */
+ if (tokens->length > 1)
+ {
+ ereport(elevel,
+ errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("extra fields at end of line"),
+ errcontext("line %d of configuration file \"%s\"",
+    tok_line->line_num, tok_line->file_name));
+ return NULL;
+ }

The comment suggests that this aims to prevent any additional text on
the line, but this parses:

localhost server.crt server.key server.crt "cmd" on TRAILING_TEXT MORE_TEXT

+ /* SSL Passphrase Command (optional) */
+ field = lnext(tok_line->fields, field);
+ if (field)
+ {
+ tokens = lfirst(field);
+ token = linitial(tokens);

Isn't a length > 1 error check missing from here?

[1]: https://www.postgresql.org/message-id/CAN4CZFM3b8u5uNNNsY6XCya257u%2BDofms3su9f11iMCxvCacag%40mail.gmail.com

---

From: Zsolt Parragi <[email-masked]>
Date: Tue, 17 Mar 2026 06:22:53 +0000

> I'm a bit surprised that the .dat
> file processing doesn't error out keys that aren't part of the DSL for defining
> GUCs but clearly it doesn't. Fixed.

This also surprised me, I wrote a patch to improve this [1].

I only have a few mostly stylistic comments, otherwise the patch looks good.

+ if (isServerStart)
+ {
+ if (host->ssl_passphrase_cmd && host->ssl_passphrase_cmd[0])
+ {
+ SSL_CTX_set_default_passwd_cb(ctx, ssl_external_passwd_cb);
+ SSL_CTX_set_default_passwd_cb_userdata(ctx, host->ssl_passphrase_cmd);
+ }
+ }
+ else
+ {
+ if (host->ssl_passphrase_reload && host->ssl_passphrase_cmd[0])
+ {
+ SSL_CTX_set_default_passwd_cb(ctx, ssl_external_passwd_cb);
+ SSL_CTX_set_default_passwd_cb_userdata(ctx, host->ssl_passphrase_cmd);
+ }

The start path checks ssl_passphrase_cmd for null, the reload doesn't.

+ if (openssl_tls_init_hook != default_openssl_tls_init)
+ {
+ ereport(WARNING,
+ errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("SNI is enabled; installed TLS init hook will be ignored"),

Won't this spam the log with one warning per hosts line? It might be
okay/acceptable, but there isn't anything line specific in this
warning.

+ * file.  The list is returned in the hosts parameter. The function will return
+ * a HostsFileLoadResult value detailing the result of the operation.  When
+ * the hosts configuration failed to load, the err_msg variable may have more
+ * information in case it was passed as non-NULL.
+ */
+int
+load_hosts(List **hosts, char **err_msg)

Comment says HostsFileLoadResult, but the return type is int.

+typedef enum HostsFileLoad
+{
+ HOSTSFILE_LOAD_OK = 0,
+ HOSTSFILE_LOAD_FAILED,
+ HOSTSFILE_EMPTY,
+ HOSTSFILE_MISSING,
+ HOSTSFILE_DISABLED,
+} HostsFileLoadResult;

Is the HostsFileLoad vs HostsFileLoadResult difference intentional?

+#ifdef USE_ASSERT_CHECKING
+ if (res == HOSTSFILE_DISABLED)
+ Assert(ssl_sni == false);
+#endif

Do we need this ifdef?

And I also found a typo (distncnt):

+ /*
+ * At this point we know we have a configuration with a list
+ * of distnct 1..n hostnames for literal string matching with
+ * the SNI extension from the user.

[1]: https://www.postgresql.org/message-id/CAN4CZFP%3D3xUoXb9jpn5OWwicg%2Brbyrca8-tVmgJsQAa4%2BOExkw%40mail.gmail.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 18 Mar 2026 13:19:01 +0100

After staring at this version more, and testing on various platforms with
various OpenSSL versions, I went ahead and pushed it.  Thanks for all the
reviews!

longfin has so far reported a test failure which I am looking into.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Wed, 18 Mar 2026 06:01:45 -0700

> longfin has so far reported a test failure which I am looking into.

I took a quick look at culicidae and I think that's just due to the
use of EXEC_BACKEND. Rather than $windows_os the SKIP logic should
probably use something like 001_server's $exec_backend.

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 18 Mar 2026 15:25:13 +0100

> On 18 Mar 2026, at 14:01, Jacob Champion <[email-masked]> wrote:
> 

That's a bit embarrassing, I spent some time investigating passphrase reloading
under EXEC_BACKEND as part of this patchset..

The longfin issue is a bit more odd, I can reproduce it on macOS with OpenSSL
1.1.1 but nowhere else.  Rather than reporting an SSL error for aborted
handshake it reports a SYSCALL error.  Using SYSCALL error for when the server
close the connection abruptly is documented, but not really this case where it
does so with no error codes at all (which given OpenSSL documentation doesn't
really say much..).  The change in the attached diff does fix it for me but I'm
a bit hesitant to apply something like that, I would be more inclined to the
change the expected output in the test.  What are your thoughts?

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 18 Mar 2026 15:58:02 +0100

> On 18 Mar 2026, at 15:33, Tom Lane <[email-masked]> wrote:
> 
> Daniel Gustafsson <[email-masked]> writes:
>> handshake it reports a SYSCALL error.
> 
> IIRC longfin is using some fairly old hand-built openssl installation.

Thanks for confirming, that matches with my local repro which required building
1.1.1 on macOS. Did you build with Apple clang or a a stock clang/gcc?

> Maybe I should just update it.  Do you want to hold off and see if
> that changes anything?

Let's wait a little while I research little more.  I can reproduce it locally
but it's better with two independent cases.

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Wed, 18 Mar 2026 11:35:50 -0400

> 1.1.1 on macOS. Did you build with Apple clang or a a stock clang/gcc?

It's been awhile, but I can't imagine that I didn't use Apple's
compiler of the time.  [ Checks longfin's host... ]  The files
in that openssl tree are all dated Nov 20 2018, so it's probably
due for a refresh in any case.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 18 Mar 2026 09:14:02 -0700

> handshake it reports a SYSCALL error.

Do you know yet why the handshake is aborted on macOS, as opposed to a
polite handshake_failure alert?

> The change in the attached diff does fix it for me but I'm

I think that patch might effectively shadow the `else` branch, which
is supposed to be reporting the EOF. (I wouldn't mind a better error
message than "SYSCALL error: EOF detected", but that's not something
this patch did.)

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 18 Mar 2026 14:39:10 -0400

Daniel Gustafsson <[email-masked]> writes:
> I've done more testing now and I can only reproduce this with downgraded
> versions of OpenSSL 1.1.1, when running the latest 1.1.1x the error goes away
> and the error is reported as expected.  Can you try to upgrade your machine,
> which I assume isn't running bleeding edge 1.1.1 by the sounds of it.

Nope, it was still on 1.1.1a.  I've now updated it to 3.0.19,
which appears to be the oldest available-to-the-public supported
version.  I doubt that continuing to test 1.1.1-anything is really
useful.

... and longfin is now green.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 19 Mar 2026 10:45:07 +0100

> On 19 Mar 2026, at 10:39, Michael Banck <[email-masked]> wrote:

> I'm really late to the part, but I did not see it discussed elsewhere on
> a quick glance: Isn't pg_hosts.conf a really (too) generic name for this
> feature? I don't want to open a huge bikeshedding sub-thread, but was a
> more specific filename considered?

I don't recall any discussion on that, and I don't really see a problem off the
cuff.  As it is a config file for defining hostnames and their config, in which
way do you feel its too generic and what would "claiming that name" for this
prevent (or how would it confuse)? Do you have any alternative suggestions?

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Mon, 04 May 2026 15:22:37 -0400

In preparation for our annual pgindent update, I checked what happens
when I install the buildfarm's version of typedefs.list, and I found
that the typedef HostsFileLoadResult (from 4f433025f) gets
misformatted because it's not in the buildfarm's list.  That's because
the buildfarm mechanism only captures typedefs that are used to
declare some object (variable, function, field) and this one isn't.

It seems quite odd to me that load_host(), which in fact returns
HostsFileLoadResult codes, is declared to return int.  That seems
to have been done because HostsFileLoadResult wasn't declared in
the same header, but there is no visible reason why it shouldn't be.
Any objection to the attached fixup?

As a side matter, "load_host" seems like a remarkably generic name
that conveys little about what it actually does, and to the extent
that it does convey anything the implication is wrong: it returns
(potentially) info about multiple hosts not just one.  Can't we do
better?

			regards, tom lane

diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index ad04bedaa1d..6ec887b8a47 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -361,7 +361,7 @@ parse_hosts_line(TokenizedAuthLine *tok_line, int elevel)
  * the hosts configuration failed to load, the err_msg variable may have more
  * information in case it was passed as non-NULL.
  */
-int
+HostsFileLoadResult
 load_hosts(List **hosts, char **err_msg)
 {
 	FILE	   *file;
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index f64b2787f66..b978497b5d4 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -156,7 +156,7 @@ be_tls_init(bool isServerStart)
 	MemoryContext host_memcxt = NULL;
 	MemoryContextCallback *host_memcxt_cb;
 	char	   *err_msg = NULL;
-	int			res;
+	HostsFileLoadResult res;
 	struct hosts *new_hosts;
 	SSL_CTX    *context = NULL;
 	int			ssl_ver_min = -1;
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 29e2a6c5b3d..4aa6258a345 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -165,15 +165,6 @@ typedef struct HostsLine
 	void	   *ssl_ctx;		/* associated SSL_CTX* for the above settings */
 } HostsLine;
 
-typedef enum HostsFileLoadResult
-{
-	HOSTSFILE_LOAD_OK = 0,
-	HOSTSFILE_LOAD_FAILED,
-	HOSTSFILE_EMPTY,
-	HOSTSFILE_MISSING,
-	HOSTSFILE_DISABLED,
-} HostsFileLoadResult;
-
 /*
  * TokenizedAuthLine represents one line lexed from an authentication
  * configuration file.  Each item in the "fields" list is a sub-list of
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index c9b934d2321..d15073a0a93 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -156,6 +156,15 @@ enum ssl_protocol_versions
 	PG_TLS1_3_VERSION,
 };
 
+typedef enum HostsFileLoadResult
+{
+	HOSTSFILE_LOAD_OK = 0,
+	HOSTSFILE_LOAD_FAILED,
+	HOSTSFILE_EMPTY,
+	HOSTSFILE_MISSING,
+	HOSTSFILE_DISABLED,
+} HostsFileLoadResult;
+
 /*
  * prototypes for functions in be-secure-common.c
  */
@@ -164,6 +173,6 @@ extern int	run_ssl_passphrase_command(const char *cmd, const char *prompt,
 									   char *buf, int size);
 extern bool check_ssl_key_file_permissions(const char *ssl_key_file,
 										   bool isServerStart);
-extern int	load_hosts(List **hosts, char **err_msg);
+extern HostsFileLoadResult load_hosts(List **hosts, char **err_msg);
 
 #endif							/* LIBPQ_H */

---

From: Tom Lane <[email-masked]>
Date: Mon, 04 May 2026 15:27:20 -0400

> (potentially) info about multiple hosts not just one.  Can't we do

Sigh ... brain fade there, of course the function is load_hosts
not load_host.  It's still too generic IMO, but at least the
pluralization is right.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 4 May 2026 22:31:36 +0200

> On 4 May 2026, at 21:22, Tom Lane <[email-masked]> wrote:

> Any objection to the attached fixup?

At some point during the development of the patch there was a reason (which I
cannot remember right now) for the declaration being in hba.h, but I clearly
missed moving it when that no longrer applied.  No objections to the patch,
thanks!

> (potentially) info about multiple hosts not just one.  Can't we do

It's following the naming convention of load_hba() which reads pg_hba.conf, and
load_ident() which reads pg_ident.conf - thus load_hosts() for the function
that reads pg_hosts.conf.  Perhaps load_pg_hosts_conf() or load_hosts_config()
would convey more meaning?

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Mon, 04 May 2026 18:00:02 -0400

Daniel Gustafsson <[email-masked]> writes:
>> that conveys little about what it actually does,

> would convey more meaning?

Hmm, okay.  I'd prefer a more specific name, but it wouldn't make
much sense unless we also rename those two.  That's probably more
code churn than is justified.

I'll push the thing for moving/using the typedef, but leave the
function name alone.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 5 May 2026 09:40:19 +0200

> On 5 May 2026, at 00:00, Tom Lane <[email-masked]> wrote:

Thanks!

--
Daniel Gustafsson