Horton

Thread

  1. Re: Add ssl_(supported|shared)_groups to sslinfo

    Cary Huang <cary.huang@highgo.ca> — 2026-05-08T21:36:10Z

    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

  2. Re: Add ssl_(supported|shared)_groups to sslinfo

    Dmitry Dolgov <9erthalion6@gmail.com> — 2026-05-11T15:50:01Z

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

  3. Re: Add ssl_(supported|shared)_groups to sslinfo

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-12T21:48:20Z

    Hello!

+#define HAVE_SSL_GROUPS \
+ defined(HAVE_DECL_SSL_GET1_GROUPS) && \
+ defined(HAVE_DECL_SSL_GET_NEGOTIATED_GROUP) && \
+ defined(HAVE_SSL_GROUP_TO_NAME)

I don't think this check works properly
1. autoconf/meson always defines HAVE_DECL to 0/1, so defined() always
returns true
2. in practice it should work, but using defined() in a macro
expansion is undefined behavior

  4. Re: Add ssl_(supported|shared)_groups to sslinfo

    Dmitry Dolgov <9erthalion6@gmail.com> — 2026-05-13T18:08:02Z

    > On Tue, May 12, 2026 at 10:48:20PM +0100, Zsolt Parragi wrote:
> +#define HAVE_SSL_GROUPS \
> + defined(HAVE_DECL_SSL_GET1_GROUPS) && \
> + defined(HAVE_DECL_SSL_GET_NEGOTIATED_GROUP) && \
> + defined(HAVE_SSL_GROUP_TO_NAME)
> 
> I don't think this check works properly
> 1. autoconf/meson always defines HAVE_DECL to 0/1, so defined() always
> returns true
> 2. in practice it should work, but using defined() in a macro
> expansion is undefined behavior

That's indeed a faux pas. I was testing this on openssl 1.1.1w, which
was lacking both SSL_get_negotiated_group and SSL_group_to_name, and
this was hiding the problem. The new version should address this
problem, and includes documentation fix from Cary.