Re: Serverside SNI support in libpq

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
definecmd 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
earlierfree 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/