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)
[email protected]
www.highgo.ca
