Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
ssl: Serverside SNI support for libpq
- 4f433025f666 19 (unreleased) landed
-
ssl: Add tests for client CA
- 25e568ba7ce7 19 (unreleased) landed
-
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 -
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
-
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 -
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
-
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
-
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
-
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
-
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
-
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
-
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
-
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
-
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
-
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 -
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
-
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
-
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
-
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
-
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
-
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/ -
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
-
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
-
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
-
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
-
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/ -
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 -
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 -
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
-
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
-
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
-
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
-
Re: Serverside SNI support in libpq
Jelte Fennema-Nio <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
-
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
-
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 -
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
-
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 -
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
-
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
-
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
-
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 -
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
-
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
-
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