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: <D9F6F14D-FCB1-48B5-9CBC-AE89FA0AEF4C@yesql.se>
Date: Tue, 11 Nov 2025 17:06:49 +0800
Cc: Michael Paquier <michael@paquier.xyz>,
 Andres Freund <andres@anarazel.de>,
 Jacob Champion <jacob.champion@enterprisedb.com>,
 Pgsql Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <1C36B747-628D-4B66-B1DC-657CB15F8AEB@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>
To: Daniel Gustafsson <daniel@yesql.se>
Archived-At: 
 <https://www.postgresql.org/message-id/1C36B747-628D-4B66-B1DC-657CB15F8AEB%40gmail.com>
Precedence: bulk

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:
>=20
> 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.
>=20
> --
> Daniel Gustafsson
>=20
> <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 =3D is_server_start ? ERROR : =
LOG;
 	char	   *command;
 	FILE	   *fh;
 	int			pclose_rc;
 	size_t		len =3D 0;
+	char	   *cmd =3D (char *) userdata;
```

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.

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);
```

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.

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;
 }
```

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 =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',
+},

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

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