Re: pg_stat_ssl additions

On 20/11/2018 09:31, Kyotaro HORIGUCHI wrote:
> Comparing the two codes in pgstatfuncs.c and sslinfo.c, It seems
> that ssl_client_serial() can be largely simplified using
> be_tls_get_peer_serial(). X509_NAME_to_text() can be simplified
> using X509_NAME_to_cstring(). But this would be another patch.
>
> By the way, though it is not by this patch, X509_NAME_to_cstring
> doesn't handle NID_undef from OBJ_obj2nid() and NULL from
> OBJ_nid2ln(). The latter case feeds NULL to BIO_printf() . I'm
> not sure it can actually happen.

Yes, that seems problematic, at least in theory.

>> - Changes pg_stat_ssl.clientdn to be null if there is no client
>> certificate (as documented, but not implemented). (bug fix)
>
> This reveals DN, serial and issuer DN of the cert to
> non-superusres. pg_stat_activity hides at least
> client_addr. Aren't they needed to be hidden? (This will change
> the existing behavior.)

Yes. Arguably, nothing in this view other than your own session should
be seen by normal users. Thoughts from others?

>> - Adds new fields to pg_stat_ssl: issuerdn and clientserial. These
>> allow uniquely identifying the client certificate. AFAICT, these are
>> the most interesting pieces of information provided by sslinfo but not
>> in pg_stat_ssl. (I don't like the underscore-free naming of these
>> fields, but it matches the existing "clientdn".)
>
> clientdn, clientserial, issuerdn are the fields about client
> certificates. Only the last one omits prefixing "client". But
> "clientissuerdn" seems somewhat rotten... Counldn't we rename
> clientdn to client_dn?

I'd prefer renaming as well, but some people might not like that.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services