Re: Serverside SNI support in libpq

> On 13 Mar 2026, at 22:12, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:

> Originally I started looking at this thread because Jacob mentioned it
> relates to the custom OAuth / HBA variable question[1]. While I do see
> the relation, I don't have many practical ideas.

Thanks for reviewing!

> I was thinking about suggesting using a "key=value, key=value ..."
> file style instead of the fixed table, both for easier later
> generalization, and because it aligns better to modern configuration
> formats (and personally I always find these columnar config files
> harder to read)
> However, it would also differ from other existing postgres config
> files and wouldn't offer a clear initial advantage, so it doesn't seem
> like a good practical choice. I'm still mentioning this for
> completeness, but mostly I'll focus on a more practical review:

I can sympathise with wanting more expressive configuration formats, but I
think there is substantial value in being consistent across these files. If we
start to also accept another format, we should change for all files at once.

Such a change should be considered on its own merits though and not included in
any other patchset, since that will move the goalposts well out of sight.

> + check_nook => 'check_ssl_sni',
>
> This seems to be a typo?

Indeed it is, an embarrassing one at that. I'm a bit surprised that the .dat
file processing doesn't error out keys that aren't part of the DSL for defining
GUCs but clearly it doesn't. Fixed.

> + if (SSL_client_hello_get0_ext(ssl, TLSEXT_TYPE_server_name, &tlsext, &left))
> ...
> Since we are already inside the if which verifies that the extension
> is present, shouldn't all of these report decode_error?

Re-reading the relevant portions of RFC8446 I think you are right, fixed.

> + if (!ssl_update_ssl(ssl, install_config))
> + {
> + ereport(COMMERROR,
> + errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("failed to switch to SSL configuration for host, terminating
> connection"));
> + return SSL_CLIENT_HELLO_ERROR;
> + }
>
> Isn't there a missing *al = assignment here?

Good catch, it should set SSL_AD_INTERNAL_ERROR here.

> The comment suggests that this aims to prevent any additional text on
> the line, but this parses:
>
> localhost server.crt server.key server.crt "cmd" on TRAILING_TEXT MORE_TEXT

Ugh, that was a silly bug, I was testing the wrong thing. Fixed, and test
cases added for this.

> + /* SSL Passphrase Command (optional) */
> + field = lnext(tok_line->fields, field);
> + if (field)
> + {
> + tokens = lfirst(field);
> + token = linitial(tokens);
>
> Isn't a length > 1 error check missing from here?

I remember having a rationale for not checking on these optional fields but I
can't for the life of me remember, and I can't see a reason not to, so added.

--
Daniel Gustafsson