Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Declare load_hosts() as returning HostsFileLoadResult.

  2. ssl: Skip passphrase reload tests in EXEC_BACKEND builds

  3. ssl: Serverside SNI support for libpq

  4. ssl: Add tests for client CA

  1. Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2024-05-10T14:22:45Z

    SNI was brought up the discussions around the ALPN work, and I have had asks
for it off-list, so I decided to dust off an old patch I started around the
time we got client-side SNI support but never finished (until now).  Since
there is discussion and thinking around how we handle SSL right now I wanted to
share this early even though it will be parked in the July CF for now.  There
are a few usecases for serverside SNI, allowing for completely disjoint CAs for
different hostnames is one that has come up.  Using strict SNI mode (elaborated
on below) as a cross-host attack mitigation was mentioned in [0].

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.

The way multiple certificates are handled is that libpq creates one SSL_CTX for
each at startup, and switch to the appropriate one when the connection is
inspected.  Configuration handling is done in secure-common to not tie it to a
specific TLS backend (should we ever support more), but the validation of the
config values is left for the TLS backend.

There are a few known open items with this patch:

* There are two OpenSSL callbacks which can be used to inspect the hostname TLS
extension: SSL_CTX_set_tlsext_servername_callback and
SSL_CTX_set_client_hello_cb.  The documentation for the latter says you
shouldn't use the former, and the docs for the former says you need it even if
you use the latter.  For now I'm using SSL_CTX_set_tlsext_servername_callback
mainly because the OpenSSL tools themselves use that for SNI.

* The documentation is not polished at all and will require a more work to make
it passable I think.  There are also lot's more testing that can be done, so
far it's pretty basic.

* I've so far only tested with OpenSSL and haven't yet verified how LibreSSL
handles this.

--
Daniel Gustafsson

[0] https://www.postgresql.org/message-id/e782e9f4-a0cd-49f5-800b-5e32a1b29183%40eisentraut.org

  2. Re: Serverside SNI support in libpq

    Cary Huang <cary.huang@highgo.ca> — 2024-05-24T19:54:49Z

    The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

This is an interesting feature on PostgreSQL server side where it can swap the
certificate settings based on the incoming hostnames in SNI field in client
hello message.

I think this patch resonate with a patch I shared awhile ago
( https://commitfest.postgresql.org/48/4924/ ) that adds multiple certificate
support on the libpq client side while this patch adds multiple certificate
support on the server side. My patch allows user to supply multiple certs, keys,
sslpasswords in comma separated format and the libpq client will pick one that
matches the CA issuer names sent by the server. In relation with your patch,
this CA issuer name would match the CA certificate configured in pg_hosts.cfg.

I had a look at the patch and here's my comments:

+   <para>
+    <productname>PostgreSQL</productname> can be configured for
+    <acronym>SNI</acronym> using the <filename>pg_hosts.conf</filename>
+    configuration file. <productname>PostgreSQL</productname> inspects the TLS
+    hostname extension in the SSL connection handshake, and selects the right
+    SSL certificate, key and CA certificate to use for the connection.
+   </para>

pg_hosts should also have sslpassword_command just like in the postgresql.conf in
case the sslkey for a particular host is encrypted with a different password.

+	/*
+	 * Install SNI TLS extension callback in case the server is configured to
+	 * validate hostnames.
+	 */
+	if (ssl_snimode != SSL_SNIMODE_OFF)
+		SSL_CTX_set_tlsext_servername_callback(context, sni_servername_cb);

If libpq client does not provide SNI, this callback will not be called, so there
is not a chance to check for a hostname match from pg_hosts, swap the TLS CONTEXT,
or possibly reject the connection even in strict mode. The TLS handshake in such
case shall proceed and server will use the certificate specified in
postgresql.conf (if these are loaded) to complete the handshake with the client.
There is a comment in the patch that reads:

>  - 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.

I am not sure if it implies that if ssl_snimode is strict, then the normal ssl_cert,
ssl_key and ca_cert…etc settings in postgresql.conf are ignored?

thank you

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca

  3. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-07-25T17:51:05Z

    On Fri, May 10, 2024 at 7:23 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> The way multiple certificates are handled is that libpq creates one SSL_CTX for
> each at startup, and switch to the appropriate one when the connection is
> inspected.

I fell in a rabbit hole while testing this patch, so this review isn't
complete, but I don't want to delay it any more. I see a few
possibly-related problems with the handling of SSL_context.

The first is that reloading the server configuration doesn't reset the
contexts list, so the server starts behaving in really strange ways
the longer you test. That's an easy enough fix, but things got weirder
when I did. Part of that weirdness is that SSL_context gets set to the
last initialized context, so fallback doesn't always behave in a
deterministic fashion. But we do have to set it to something, to
create the SSL object itself...

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 (?!).

Since I can't reproduce the weirdest bits under a debugger yet, I
don't really know what's happening. Maybe my patches are buggy. Or
maybe we're running into some chicken-and-egg madness? The order of
operations looks like this:

1. Create a list of contexts, selecting one as an arbitrary default
2. Create an SSL object from our default context
3. During the servername_callback, reparent that SSL object (which has
an active connection underway) to the actual context we want to use
4. Complete the connection

It's step 3 that I'm squinting at. I wondered how, exactly, that
worked in practice, and based on this issue the answer might be "not
well":

    https://github.com/openssl/openssl/issues/6109

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.

Have you seen any weird behavior like this on your end? I'm starting
to doubt my test setup... On the plus side, I now have a handful of
debugging patches for a future commitfest.

Thanks,
--Jacob

  4. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-07-25T18:00:41Z

    On Fri, May 24, 2024 at 12:55 PM Cary Huang <cary.huang@highgo.ca> wrote:
> pg_hosts should also have sslpassword_command just like in the postgresql.conf in
> case the sslkey for a particular host is encrypted with a different password.

Good point. There is also the HBA-related handling of client
certificate settings (such as pg_ident)...

I really dislike that these things are governed by various different
files, but I also feel like I'm opening up a huge can of worms by
requesting nestable configurations.

> +       if (ssl_snimode != SSL_SNIMODE_OFF)
> +               SSL_CTX_set_tlsext_servername_callback(context, sni_servername_cb);
>
> If libpq client does not provide SNI, this callback will not be called, so there
> is not a chance to check for a hostname match from pg_hosts, swap the TLS CONTEXT,
> or possibly reject the connection even in strict mode.

I'm mistrustful of my own test setup (see previous email to the
thread), but I don't seem to be able to reproduce this. With sslsni=0
set, strict mode correctly shuts down the connection for me. Can you
share your setup?

(The behavior you describe might be a useful setting in practice, to
let DBAs roll out strict protection for new clients gracefully without
immediately blocking older ones.)

Thanks,
--Jacob

  5. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2024-12-03T13:58:01Z

    > On 25 Jul 2024, at 19:51, Jacob Champion <jacob.champion@enterprisedb.com> 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

  6. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-12-04T00:43:18Z

    On Tue, Dec 3, 2024 at 5:58 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > 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.

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).

> > 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?

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

Thanks!
--Jacob

  7. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2024-12-04T13:44:18Z

    > On 4 Dec 2024, at 01:43, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Tue, Dec 3, 2024 at 5:58 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>> 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.
> 
> 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).
> 
>>> 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?
> 
> I'll work on cleaning them up. I'd meant to contribute them
> individually by now, but I got a bit sidetracked...

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

  8. Re: Serverside SNI support in libpq

    Michael Paquier <michael@paquier.xyz> — 2024-12-11T00:34:40Z

    On Wed, Dec 04, 2024 at 02:44:18PM +0100, Daniel Gustafsson 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.

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

  9. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2024-12-11T08:13:35Z

    > On 11 Dec 2024, at 01:34, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Wed, Dec 04, 2024 at 02:44:18PM +0100, Daniel Gustafsson 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.
> 
> Looks like this still fails quite heavily in the CI..  You may want to
> look at that.

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

  10. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-02-19T23:12:50Z

    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

  11. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-02-24T21:51:02Z

    On Wed, Feb 19, 2025 at 3:13 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> Are there any blockers for getting this in?

> +           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

  12. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-02-27T13:38:24Z

    > On 24 Feb 2025, at 22:51, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Wed, Feb 19, 2025 at 3:13 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>> Are there any blockers for getting this in?
> 
>> +           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 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

  13. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-04T21:57:09Z

    On Thu, Feb 27, 2025 at 5:38 AM Daniel Gustafsson <daniel@yesql.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.champion@enterprisedb.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

  14. Re: Serverside SNI support in libpq

    Andres Freund <andres@anarazel.de> — 2025-05-13T13:46:32Z

    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

  15. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-08-27T19:49:34Z

    > On 13 May 2025, at 15:46, Andres Freund <andres@anarazel.de> wrote:

> This is not passing CI on windows...
> 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

  16. Re: Serverside SNI support in libpq

    Michael Paquier <michael@paquier.xyz> — 2025-09-01T01:58:29Z

    On Wed, Aug 27, 2025 at 09:49:34PM +0200, Daniel Gustafsson wrote:
> 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.

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

  17. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-09-02T12:48:43Z

    > On 1 Sep 2025, at 03:58, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Wed, Aug 27, 2025 at 09:49:34PM +0200, Daniel Gustafsson wrote:
>> 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.
> 
> 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.

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

  18. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-11-10T22:32:43Z

    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

  19. Re: Serverside SNI support in libpq

    Chao Li <li.evan.chao@gmail.com> — 2025-11-11T09:06:49Z

    Hi Daniel,

I just reviewed the patch and got a few comments:

> On Nov 11, 2025, at 06:32, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> 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
> 
> <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/

  20. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-11-12T22:44:39Z

    On Mon, Nov 10, 2025 at 2:33 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> 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.

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

  21. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-11-12T22:50:36Z

    On Tue, Nov 11, 2025 at 1:07 AM Chao Li <li.evan.chao@gmail.com> wrote:
> 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

  22. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-11-12T23:03:11Z

    > On 12 Nov 2025, at 23:44, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> Did you have any thoughts on my earlier review [2]? The test patch
> 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

  23. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-11-24T14:53:31Z

    > On 12 Nov 2025, at 23:44, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> Did you have any thoughts on my earlier review [2]? The test patch
> 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

  24. Re: Serverside SNI support in libpq

    Chao Li <li.evan.chao@gmail.com> — 2025-11-24T23:28:00Z

    Hi Daniel,

None of my comment on v9 are addressed in v10:

> 
> 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?

Comment 4 can be ignored as Jacob has answered.


> On Nov 24, 2025, at 22:53, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 12 Nov 2025, at 23:44, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
>> Did you have any thoughts on my earlier review [2]? The test patch
>> 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
> 
> <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,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  25. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-11-25T14:39:22Z

    > On 25 Nov 2025, at 00:28, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> Hi Daniel,
> 
> None of my comment on v9 are addressed in v10:

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!

>> 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

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

>> 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 *”.

Fixed.

>> 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.

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).

>> 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?

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

> 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?

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.

> 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

Fixed.

> 7 - runtime.sgml
> ```
> +    will only be used to for the handshake until the hostname is inspected, it
> ```
> 
> “Used to for” => “used for"

Fixed.

> 8 - Cluster.pm
> ```
> +matching the specified pattern. If the pattern matches agsinst the logfile a
> ```
> 
> Typo: agsinst => against

Fixed.

--
Daniel Gustafsson

  26. Re: Re: Serverside SNI support in libpq

    Dewei Dai <daidewei1970@163.com> — 2025-11-26T09:14:52Z

    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


daidewei1970@163.com

  27. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-11-26T14:33:07Z

    > On 26 Nov 2025, at 10:14, Dewei Dai <daidewei1970@163.com> wrote:
> 
> Hi Daniel,
>    I just reviewed the v11 patch and got a few comments:

Thanks!

> Typo: certficate ->  certificate 

Fixed.

> Typo: hand- shake ->handshake

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.

> 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.

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

  28. Re: Serverside SNI support in libpq

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-03T09:57:32Z

    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

  29. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-12-03T16:52:09Z

    > On 3 Dec 2025, at 10:57, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
> Sorry for jumping in so late.

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

  30. Re: Serverside SNI support in libpq

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-03T16:56:59Z

    On 03/12/2025 18:52, Daniel Gustafsson wrote:
>> On 3 Dec 2025, at 10:57, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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.

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

>> 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?

Yeah, that's fair.

- Heikki

  31. Re: Serverside SNI support in libpq

    Jelte Fennema <postgres@jeltef.nl> — 2025-12-03T21:27:43Z

    On Wed, 3 Dec 2025 at 17:57, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > 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?
>
> Yeah, that's fair.

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

  32. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-12-03T23:27:53Z

    > On 3 Dec 2025, at 22:27, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> 
> On Wed, 3 Dec 2025 at 17:57, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> 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?
>> 
>> Yeah, that's fair.
> 
> 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).

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:
> 1. If the file does not exist, use the "off" behaviour
> 2. If the file exists, use the "strict" behaviour

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

  33. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-11T17:47:28Z

    Hi!

On Mon, Nov 24, 2025 at 6:53 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> 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 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?

On Wed, Dec 3, 2025 at 1:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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).

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 <daniel@yesql.se> wrote:
> > On 3 Dec 2025, at 22:27, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > 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).
>
> 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

  34. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-12-11T17:51:53Z

    > On 11 Dec 2025, at 18:47, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

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

In which version of OpenSSL (or LibreSSL)?

--
Daniel Gustafsson

  35. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-11T19:40:58Z

    On Thu, Dec 11, 2025 at 9:52 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > The new v12 tests still don't pass for me (they all use "certificate
> > verify failed", but the failure modes should be different).
>
> 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

  36. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-12-12T11:41:36Z

    > On 11 Dec 2025, at 18:47, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> On Mon, Nov 24, 2025 at 6:53 AM Daniel Gustafsson <daniel@yesql.se> wrote:

>> 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 new v12 tests still don't pass for me (they all use "certificate
> 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.

>> +           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?

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

> On Wed, Dec 3, 2025 at 1:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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).

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.

>> 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.)

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

  37. Re: Serverside SNI support in libpq

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-17T09:03:07Z

    On 12/12/2025 13:41, Daniel Gustafsson wrote:
>> On Wed, Dec 3, 2025 at 1:57 AM Heikki Linnakangas <hlinnaka@iki.fi> 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.)

+1

> 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?

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

  38. Re: Serverside SNI support in libpq

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-17T09:06:36Z

    On 17/12/2025 11:03, Heikki Linnakangas wrote:
> On 12/12/2025 13:41, Daniel Gustafsson wrote:
>> 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?
> 
> 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.

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

  39. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-17T23:58:10Z

    On Fri, Dec 12, 2025 at 3:41 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > 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?
>
> The attached version allows ssl_ca to be omitted from the pg_host config to
> match the ssl_ca GUC.

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

> I'm still not sure why they pass for me locally with that error, but I've
> 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/);
> +       expected_stderr => qr/client certificates can only be checked if a root certificate store is available/);

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

> Adding a boolean GUC which turns ph_hosts (and thus SNI) on or off can perhaps
> 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

  40. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2025-12-18T00:07:41Z

    > On 18 Dec 2025, at 00:58, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Fri, Dec 12, 2025 at 3:41 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>> 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?
>> 
>> The attached version allows ssl_ca to be omitted from the pg_host config to
>> match the ssl_ca GUC.
> 
> Aha! I think ssl_ca should be moved into the "Optional fields" section
> of `struct HostsLine` now.

Ah, yes.

>> I'm still not sure why they pass for me locally with that error, but I've
>> 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/);
>> +       expected_stderr => qr/client certificates can only be checked if a root certificate store is available/);
> 
> because host_context->ssl_loaded_verify_locations should be false. But
> that doesn't happen... Why?

I'll have a look.

> 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?

Yeah, pretty much.

>> +           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]

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.

>> +   /* 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.

Good point, will do that.

> Should we support multiple hostname tokens in a single line, though,
> 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

  41. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-18T17:06:19Z

    On Wed, Dec 17, 2025 at 4:07 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> > 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]
>
> 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.

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.

> > Should we support multiple hostname tokens in a single line, though,
> > 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?

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

  42. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-18T18:20:21Z

    On Thu, Dec 18, 2025 at 9:06 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> 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.

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

  43. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-01-13T09:57:27Z

    >> The attached version allows ssl_ca to be omitted from the pg_host config to
>> match the ssl_ca GUC.
> 
> Aha! I think ssl_ca should be moved into the "Optional fields" section
> 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.

> 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.

Done.

>>> 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]
>> 
>> 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.
> 
> 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.

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.

>>> Should we support multiple hostname tokens in a single line, though,
>>> 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?
> 
> 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'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).

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

I really appreciate thinking about this!

> - If the goal is to eventually support wildcards, will the use of a
> 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.

> - 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.

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.

> - Do we need to consider IDNs and A-labels and U-labels? (Do we
> support the latter today, at all?)

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

> 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.

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

  44. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-01-16T23:44:55Z

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

On Tue, Jan 13, 2026 at 1:57 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> 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.

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

  45. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-06T22:11:49Z

    > On 17 Jan 2026, at 00:44, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> 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?

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

  46. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-10T13:11:00Z

    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

  47. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-12T14:36:00Z

    > On 10 Mar 2026, at 14:11, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> 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.

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

  48. Re: Serverside SNI support in libpq

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-03-13T21:12:43Z

    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

  49. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-13T23:06:56Z

    > On 13 Mar 2026, at 22:12, Zsolt Parragi <zsolt.parragi@percona.com> wrote:

> 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.

Thanks for reviewing!

> 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:

I can sympathise with wanting more expressive configuration formats, but I
think there is substantial value in being consistent across these files.  If we
start to also accept another format, we should change for all files at once.

Such a change should be considered on its own merits though and not included in
any other patchset, since that will move the goalposts well out of sight.

> +  check_nook => 'check_ssl_sni',
> 
> This seems to be a typo?

Indeed it is, an embarrassing one at that.  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.

> + if (SSL_client_hello_get0_ext(ssl, TLSEXT_TYPE_server_name, &tlsext, &left))
> ...
> Since we are already inside the if which verifies that the extension
> is present, shouldn't all of these report decode_error?

Re-reading the relevant portions of RFC8446 I think you are right, fixed.

> + 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?

Good catch, it should set SSL_AD_INTERNAL_ERROR here.

> 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

Ugh, that was a silly bug, I was testing the wrong thing.  Fixed, and test
cases added for this.

> + /* 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?

I remember having a rationale for not checking on these optional fields but I
can't for the life of me remember, and I can't see a reason not to, so added.

--
Daniel Gustafsson

  50. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-16T14:24:28Z

    The attached rebase brings a few more tests by Jacob for ensuring that a) CA's
entirely replace when a host config is matched; b) that CRL's work as intended
for certificates configured in pg_hosts.conf; and c) ssl_sni and TLS init
aren't configured at the same time.

This concludes all TODOs in the code, unless there are review objections I plan
to go ahead with this version within a few days.

--
Daniel Gustafsson

  51. Re: Serverside SNI support in libpq

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-03-17T06:22:53Z

    > 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

  52. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-17T13:07:14Z

    > On 17 Mar 2026, at 07:22, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> 
>> 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].

That's great, thank you!

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

Thanks for reviewing!

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

The ssl_passphrase_reload can only be true if ssl_passphrase_cmd is non-null
since it otherwise won't be parsed.  That being said, it's an implementation
detail bleeding through and the null check is cheap enough so added.

> + 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.

It will, and I've been waffling a bit on if that's good or bad.  The WARNING
does apply to all lines in pg_hosts but it is as you say also not reporting any
specific context.  In the end I settled for reporting it only once and added a
test for it (as well as tweaked the existing test which needed to take LibreSSL
into account, which I had missed).

> + * 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.

HostsFileLoadResult is an enum and can thus be represented by an int, and it's
done like that to avoid having to include hba.h in libpq.h for the prototype.

> +typedef enum HostsFileLoad
> +{
> + HOSTSFILE_LOAD_OK = 0,
> + HOSTSFILE_LOAD_FAILED,
> + HOSTSFILE_EMPTY,
> + HOSTSFILE_MISSING,
> + HOSTSFILE_DISABLED,
> +} HostsFileLoadResult;
> 
> Is the HostsFileLoad vs HostsFileLoadResult difference intentional?

It isn't, fixed.

> +#ifdef USE_ASSERT_CHECKING
> + if (res == HOSTSFILE_DISABLED)
> + Assert(ssl_sni == false);
> +#endif
> 
> Do we need this ifdef?

Assert(..); will become (void)true; in production builds so technically they
shouldn't be required to avoid compiler complaints.  Personally I don't like to
have code which is only useful in assert-enabled builds be present in
non-assert builds so I ifdef-wrapped the conditional.

> And I also found a typo (distncnt):

Fixed.

--
Daniel Gustafsson

  53. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-18T12:19:01Z

    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

  54. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-03-18T13:01:45Z

    On Wed, Mar 18, 2026 at 5:19 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> 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

  55. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-18T14:25:13Z

    > On 18 Mar 2026, at 14:01, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Wed, Mar 18, 2026 at 5:19 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> 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.

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

  56. Re: Serverside SNI support in libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-03-18T14:33:36Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> 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.

IIRC longfin is using some fairly old hand-built openssl installation.
Maybe I should just update it.  Do you want to hold off and see if
that changes anything?

			regards, tom lane

  57. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-18T14:58:02Z

    > On 18 Mar 2026, at 15:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>> 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.
> 
> 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

  58. Re: Serverside SNI support in libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-03-18T15:35:50Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 18 Mar 2026, at 15:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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?

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

  59. Re: Serverside SNI support in libpq

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-03-18T16:14:02Z

    On Wed, Mar 18, 2026 at 7:25 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> 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.

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
> 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?

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

  60. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-18T17:09:22Z

    > On 18 Mar 2026, at 16:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 18 Mar 2026, at 15:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 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?
> 
> 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.

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.

--
Daniel Gustafsson

  61. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-18T17:13:34Z

    > On 18 Mar 2026, at 17:14, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Wed, Mar 18, 2026 at 7:25 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> 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.
> 
> Do you know yet why the handshake is aborted on macOS, as opposed to a
> polite handshake_failure alert?

As mentioned upthread it seems the error is likely the result of a bug in older
versions of OpenSSL, using the latest from the 1.1.1 series makes it go away.
I will go ahead with the EXEC_BACKEND fix in a bit to turn that animal green as
well.

> (I wouldn't mind a better error
> message than "SYSCALL error: EOF detected", but that's not something
> this patch did.)

+1

--
Daniel Gustafsson

  62. Re: Serverside SNI support in libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-03-18T18:39:10Z

    Daniel Gustafsson <daniel@yesql.se> 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

  63. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-18T19:03:49Z

    > On 18 Mar 2026, at 19:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> 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.

Wow, that was almost an Antiques Roadshow material candidate =)

> 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.

I don't disagree, 1.1.1 went EOL a long time ago, but since we still support
1.1.1 we need to test it somewhere.  I believe we have sufficient coverage
though, and I personally test on 1.1.1 through 3.6 (and will start testing 4
soon).

> ... and longfin is now green.

Great, thanks!

--
Daniel Gustafsson

  64. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-18T22:51:00Z

    > On 18 Mar 2026, at 14:01, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Wed, Mar 18, 2026 at 5:19 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> 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.

Done, and culicidae is back to being green.

--
Daniel Gustafsson

  65. Re: Serverside SNI support in libpq

    Michael Banck <mbanck@gmx.net> — 2026-03-19T09:39:25Z

    Hi,

On Wed, Mar 18, 2026 at 01:19:01PM +0100, Daniel Gustafsson wrote:
> 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!

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?


Michael

  66. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-19T09:45:07Z

    > On 19 Mar 2026, at 10:39, Michael Banck <mbanck@gmx.net> 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

  67. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-03-19T10:38:22Z

    > On 19 Mar 2026, at 10:55, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> 
>> Isn't pg_hosts.conf a really (too) generic name for this
>> feature?
> 
> My question earlier about a different key=value format (json5?), was
> related to this, but approached it from a different direction: maybe
> it could be more generic in the future, supporting different settings,
> maybe even in an extensible way? With the current SNI handling already
> relates to multi tenancy, and allowing custom per hosts settings would
> move in the same direction.

Introducing a new config format is for sure an interesting idea, but it's a
much bigger body of work which needs it's own discussion and patchset, it
should not be as part of another feature.

--
Daniel Gustafsson

  68. Re: Serverside SNI support in libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-05-04T19:22:37Z

    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

  69. Re: Serverside SNI support in libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-05-04T19:27:20Z

    I wrote:
> 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?

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

  70. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-05-04T20:31:36Z

    > On 4 May 2026, at 21:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> 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?

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!

> 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?

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

  71. Re: Serverside SNI support in libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-05-04T22:00:02Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 4 May 2026, at 21:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As a side matter, "load_host" seems like a remarkably generic name
>> that conveys little about what it actually does,

> 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?

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

  72. Re: Serverside SNI support in libpq

    Daniel Gustafsson <daniel@yesql.se> — 2026-05-05T07:40:19Z

    > On 5 May 2026, at 00:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Thanks!

--
Daniel Gustafsson