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

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