Date: Fri, 08 May 2026 14:36:10 -0700
From: Cary Huang <cary.huang@highgo.ca>
To: "Dmitry Dolgov" <9erthalion6@gmail.com>
Cc: "Jacob Champion" <jacob.champion@enterprisedb.com>,
	"Daniel Gustafsson" <daniel@yesql.se>,
	"PostgreSQL Hackers" <pgsql-hackers@postgresql.org>
Message-ID: <19e0984fc10.339087cd498909.6340608720119447495@highgo.ca>
In-Reply-To: <tuobeew3dd3eubtelwroocwgac3jhlselwshs2km5ovei77tao@wghkpdvvthbi>
References: <d57duqvzkxe43oons3jkdq7pj2wacidg7qorxommri74evu3l2@4x53she7mf77>
 <D1EC4B00-9593-488F-916D-090A73E69B4E@yesql.se>
 <CAOYmi+nkT7rkbNd6que0wtz=epOikgBKSDR88DQ=cyNJwiUw8Q@mail.gmail.com>
 <srua2tidoiztaytmxlwjfpjhntxelmxpfta4lhulvlker444yg@sf232zqm3qvs>
 <CAOYmi+k7v6hP5nM7BQdKu37TJFi-X=d7_SDswZBV5q0awxPVYg@mail.gmail.com>
 <qyw7l5ztbqouluctxgbxc2aty43suulka2q4ybpaew4tey7rlw@l66rjb7vxxhk>
 <CAOYmi+=r50Kk1c7A7O8yXwJzALyyqDmQE7FiCZvZmt_3WRBGwQ@mail.gmail.com>
 <izopeja2suconbqpbswmtto5czajnc4p7rsml5gwwrs56lexaj@kmqdrqiepays>
 <rf5vuhdo4ub2qz3eivighhjs4tfkfclbosptnhzdpzibx5aq6z@faylol54ro5s>
 <slgx6krxgs2sb6m7j6z6qht25geaa2jbwwiizwimudrxqxxg3n@2qq5eslriowi>
 <tuobeew3dd3eubtelwroocwgac3jhlselwshs2km5ovei77tao@wghkpdvvthbi>
Subject: Re: Add ssl_(supported|shared)_groups to sslinfo
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Importance: Medium
User-Agent: Zoho Mail
Archived-At: 
 <https://www.postgresql.org/message-id/19e0984fc10.339087cd498909.6340608720119447495%40highgo.ca>
Precedence: bulk

Hi

Given that sslinfo is designed to expose diagnostic information
about the current TLS connection, I am supportive of extending
its functionality.

Just some comments about the patch:

> /* Send the negotiated group first */
> if (call_cntr == 0)
> {
> 	nid = SSL_get_negotiated_group(ssl);
> 	group_type = CStringGetTextDatum("negotiated");
> }
> /* Then the shared groups */
> else if (call_cntr < fctx->nshared + 1)
> {
> 	nid = SSL_get_shared_group(ssl, call_cntr - 1);
> 	group_type = CStringGetTextDatum("shared");
> }
> /* And finally the supported groups */
> else if (call_cntr < fctx->nsupported + fctx->nshared + 1)
> {
> 	nid = fctx->supported_groups[call_cntr - fctx->nshared - 1];
> 	group_type = CStringGetTextDatum("supported");
> }
> else
> 	SRF_RETURN_DONE(funcctx);
> 
> /*
>  * SSL_group_to_name can return NULL in case of an error, e.g. when no
>  * such name was registered for some reason.
>  */
> group_name = SSL_group_to_name(ssl, nid);
> if (group_name == NULL)
> 	ereport(ERROR,
> 			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> 			 errmsg("unknown OpenSSL group at position %d",
> 					call_cntr)));

It is possible that SSL_get_negotiated_group() and
SSL_get_shared_group() would return NID_undef when there is no
negotiated group. The current code will pass that to
SSL_group_to_name() and raise an error if it returns NULL.

Instead of failing the whole function, would it be better to
just omit that row since the function returns a SETOF record?

if nid == NID_undef, we could just omit the row instead of
making a call to SSL_group_to_name(), which most likely will
fail.

Also, I found a small typo on documentation:

> Lisf of named groups shared with the server side.

should be corrected to:

List of named groups shared with the server side.

thanks!

Cary Huang
-------------
HighGo Software Inc. (Canada)
cary.huang@highgo.ca
www.highgo.ca