Date: Mon, 11 May 2026 17:50:01 +0200
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Cary Huang <cary.huang@highgo.ca>
Cc: Jacob Champion <jacob.champion@enterprisedb.com>,
	Daniel Gustafsson <daniel@yesql.se>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add ssl_(supported|shared)_groups to sslinfo
Message-ID: <gz7ze35lm5ldwsrtxzmvuimrywdhale22glrrjntfbrbanrz7x@7ji3zamvay4m>
References: 
 <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>
 <19e0984fc10.339087cd498909.6340608720119447495@highgo.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <19e0984fc10.339087cd498909.6340608720119447495@highgo.ca>
Archived-At: 
 <https://www.postgresql.org/message-id/gz7ze35lm5ldwsrtxzmvuimrywdhale22glrrjntfbrbanrz7x%407ji3zamvay4m>
Precedence: bulk

> On Fri, May 08, 2026 at 02:36:10PM -0700, Cary Huang wrote:
> Hi
> 
> Given that sslinfo is designed to expose diagnostic information
> about the current TLS connection, I am supportive of extending
> its functionality.

Thanks for looking into it.

> > /* 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.

It makes sense to me in general, but it looks there are some arguments
against this:

* ssl_extension_info function is also an SRF and returns an error if
  faced NID_undef. It's probably a good idea to be concistent with the
  existing functionality. 

* From what I see SRF API doesn't allow to "skip" a record, available
  options are either to finish the set with SRF_RETURN_DONE, or to
  return a NULL record with SRF_RETURN_NEXT_NULL. That means that when
  facing NID_undef, we can stop altogether and skip all the records
  after this, or have a NULL record in the output, both don't sound
  fitting the purpose here.

With this in mind I'm inclined to leave it as it is, but open for
suggestions.