Re: pg_stat_ssl additions

Hello.

At Thu, 18 Oct 2018 00:05:15 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<398754d8-6bb5-c5cf-e7b8-22e5f0983caf@2ndquadrant.com>
> During discussions of alternative SSL implementations, contrib/sslinfo
> is usually mentioned as something that something needs to be done about.
>  I've looked into adapting some functionality from sslinfo into the
> pg_stat_ssl view.  These two facilities have a lot of overlap but seem
> mostly oblivious to each other.
> 
> The attached patch series
> 
> - Adds a documentation link from sslinfo to pg_stat_ssl.
> 
> - Adds tests under src/test/ssl/ for the pg_stat_ssl view.

With this change the feature mapping between the two is as
follows.

  ssl_is_used()             : .ssl
  ssl_version()             : .version
  ssl_cipher()              : .cipher
  (none)                    : .bits
  (none)                    : .compression
  ssl_client_cert_present() : .clientdn IS NOT NULL
  ssl_client_serial()       : .clientserial
  ssl_client_dn()           : .clientdn
  ssl_issuer_dn()           : .issuerdn
  ssl_client_dn_field()     : (none)
  ssl_issuer_field()        : (none)
  ssl_extension_info()      : (none)

# I couldn't create x509 v3 certs for uncertain reasons..

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.

I don't look through all the similar points but it might be
better fix it.

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

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

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center