public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Jacob Champion <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Pgsql Hackers <[email protected]>
Subject: Re: Serverside SNI support in libpq
Date: Tue, 25 Nov 2025 07:28:00 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAOYmi+nYV6Rr9BY4YfYyVdiQ5TzMZray6QPXwiO3pYSaow+-Tg@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAOYmi+mSrV8hRaQkvGDf1Df4cmpv5SeTbTxppyxeonMe6MW8nA@mail.gmail.com>
<[email protected]>
<aa7gx3mychf3m2g67mbslzbxjy3if4enpcflstoa5pol3432x5@ugqz45gsvurq>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAOYmi+m2Ks7D4obtXay3y-UNn6CkTNrmr_zWC25vKTdesatafA@mail.gmail.com>
<[email protected]>
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 <[email protected]> wrote:
>
>> On 12 Nov 2025, at 23:44, Jacob Champion <[email protected]> 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/
view thread (58+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Serverside SNI support in libpq
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox