Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Serverside SNI support in libpq
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <0C53C316-C24E-4307-807B-D825CA3F7254@yesql.se>
Date: Tue, 25 Nov 2025 07:28:00 +0800
Cc: Jacob Champion <jacob.champion@enterprisedb.com>,
 Michael Paquier <michael@paquier.xyz>,
 Andres Freund <andres@anarazel.de>,
 Pgsql Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <DD77A37A-BFCA-4819-A029-1A41E2443D8E@gmail.com>
References: <88986722-5A72-4DEC-8750-BDBF67FF8C01@yesql.se>
 <CAOYmi+nYV6Rr9BY4YfYyVdiQ5TzMZray6QPXwiO3pYSaow+-Tg@mail.gmail.com>
 <7E77028B-5A3A-436B-9046-8E9992E9F94A@yesql.se>
 <Z1jeINLtB05YMVOY@paquier.xyz>
 <E51258CD-C7B2-4BB1-AB9C-A3DD2B6592D0@yesql.se>
 <F4222274-15C0-40E4-84FB-8FC1355BB8B2@yesql.se>
 <CAOYmi+mSrV8hRaQkvGDf1Df4cmpv5SeTbTxppyxeonMe6MW8nA@mail.gmail.com>
 <0BC5B9B1-6503-4563-AAC6-33DEF264AE3F@yesql.se>
 <aa7gx3mychf3m2g67mbslzbxjy3if4enpcflstoa5pol3432x5@ugqz45gsvurq>
 <F585452A-1F69-4658-A7E8-D1273AC663ED@yesql.se>
 <aLT9xVsTVp0j3zWy@paquier.xyz>
 <80F4F8F4-8E4F-4B6F-866B-D837057C1192@yesql.se>
 <D9F6F14D-FCB1-48B5-9CBC-AE89FA0AEF4C@yesql.se>
 <CAOYmi+m2Ks7D4obtXay3y-UNn6CkTNrmr_zWC25vKTdesatafA@mail.gmail.com>
 <0C53C316-C24E-4307-807B-D825CA3F7254@yesql.se>
To: Daniel Gustafsson <daniel@yesql.se>
Archived-At: 
 <https://www.postgresql.org/message-id/DD77A37A-BFCA-4819-A029-1A41E2443D8E%40gmail.com>
Precedence: bulk

Hi Daniel,

None of my comment on v9 are addressed in v10:

>=20
> 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
> ```
>=20
> Typo: certicate -> certificate
>=20
> 2 - be-secure-common.c
> ```
> +run_ssl_passphrase_command(const char *prompt, bool is_server_start, =
char *buf, int size, void *userdata)
> {
> int loglevel =3D is_server_start ? ERROR : LOG;
> char    *command;
> FILE    *fh;
> int pclose_rc;
> size_t len =3D 0;
> + char    *cmd =3D (char *) userdata;
> ```
>=20
> Cmd is only passed to replace_percent_placeholders(), and the function =
take a "const char *=E2=80=9D argument, so we can define cmd as =E2=80=9Cc=
onst char *=E2=80=9D.
>=20
> 2 - be-secure-common.c
> ```
> + tokenize_auth_file(HostsFileName, file, &hosts_lines, LOG, 0);
> +
> + foreach(line, hosts_lines)
> + {
> + TokenizedAuthLine *tok_line =3D (TokenizedAuthLine *) lfirst(line);
> +
> + if (tok_line->err_msg !=3D NULL)
> + {
> + ok =3D false;
> + continue;
> + }
> +
> + if ((newline =3D parse_hosts_line(tok_line, LOG)) =3D=3D NULL)
> + {
> + ok =3D false;
> + continue;
> + }
> +
> + parsed_lines =3D lappend(parsed_lines, newline);
> + }
> +
> + free_auth_file(file, 0);
> ```
>=20
> 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=E2=80=99t =
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.
>=20
> 3 - be-secure-openssl.c
> ```
> int
> @@ -759,6 +933,9 @@ be_tls_close(Port *port)
> pfree(port->peer_dn);
> port->peer_dn =3D NULL;
> }
> +
> + Host_context =3D NULL;
> + SSL_context =3D NULL;
> }
> ```
>=20
> 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?
>=20
> 4 - guc_parameters.dat
> ```
> +{ name =3D> 'hosts_file', type =3D> 'string', context =3D> =
'PGC_POSTMASTER', group =3D> 'FILE_LOCATIONS',
> +  short_desc =3D> 'Sets the server\'s "hosts" configuration file.',
> +  flags =3D> 'GUC_SUPERUSER_ONLY',
> +  variable =3D> 'HostsFileName',
> +  boot_val =3D> 'NULL',
> +},
>=20
> +{ name =3D> 'ssl_snimode', type =3D> 'enum', context =3D> =
'PGC_SIGHUP', group =3D> 'CONN_AUTH_SSL',
> +  short_desc =3D> 'Sets the SNI mode to use for the server.',
> +  flags =3D> 'GUC_SUPERUSER_ONLY',
> +  variable =3D> 'ssl_snimode',
> +  boot_val =3D> 'SSL_SNIMODE_DEFAULT',
> +  options =3D> 'ssl_snimode_options',
> +},
> ```
>=20
> 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:
>=20
>> On 12 Nov 2025, at 23:44, Jacob Champion =
<jacob.champion@enterprisedb.com> wrote:
>=20
>> Did you have any thoughts on my earlier review [2]? The test patch
>> attached there still fails on my machine with v9.
>=20
> 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.
>=20
> The other comments from your review are also addressed, as well as =
additional
> cleanup and improved error handling.
>=20
> --
> Daniel Gustafsson
>=20
> <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 =E2=80=9C*=E2=80=9D, =
should we explain =E2=80=9C*=E2=80=9D 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 =3D> cluster=E2=80=99s

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

=E2=80=9CUsed to for=E2=80=9D =3D> =E2=80=9Cused for"

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

Typo: agsinst =3D> against

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