Re: Serverside SNI support in libpq

> On 25 Nov 2025, at 00:28, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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