Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible

On Tue, Feb 11, 2025 at 11:23 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> + be_tls_get_peer_subject_name(MyProcPort, lsslstatus.ssl_client_dn, NAMEDATALEN);
> + be_tls_get_peer_serial(MyProcPort, lsslstatus.ssl_client_serial, NAMEDATALEN);
> + be_tls_get_peer_issuer_name(MyProcPort, lsslstatus.ssl_issuer_dn, NAMEDATALEN);
>
> But are these three actually OK to have showing up in the catalogs
> this early? This data comes from a peer certificate, that we may know
> nothing about, become set at a very early stage with
> secure_initialize().

I guess I'm going to zero in on your definition of "may know nothing
about". If that is true, something is very wrong IMO.

My understanding of the backend code was that port->peer is only set
after OpenSSL has verified that the certificate has been issued by an
explicitly trusted CA. (Our verify_cb() doesn't override the internal
checks to allow failed certificates through.) That may not be enough
to authorize entry into the server, but it also shouldn't be untrusted
data. If a CA is issuing Subject data that is somehow dangerous to the
operation of the server, I think that's a security problem in and of
itself: there are clientcert HBA modes that don't validate the
Subject, but they're still going to push that data into the catalogs,
aren't they?

So if we're concerned that Subject data is dangerous at this point in
the code, I agree that my patch makes it even more dangerous and I'll
modify it -- but then I'm going to split off another thread to try to
fix that underlying issue. A user should not have to be authorized to
access the server in order for signed authentication data from the
transport layer to be considered trustworthy. Being able to monitor
those separately is important for auditability.

> As a whole we still have a gap between what could be OK, what could
> not be OK, and the fact that pgstat_bestart_security() is called twice
> makes that confusing.

My end goal is that all of this _should_ always be OK, so calling it
once or twice or thirty times should be safe. (But again, if that's
not actually true today, I'll remove it for now.)

> > v8-0003 shows this approach. For the record, I think it's materially
> > worse than v7-0003. IMO it increases the cognitive load for very
> > little benefit and makes it more work for a newcomer to refactor the
> > cleanup code for those routines. I think it's enough that you can see
> > a separate LOG message for each failure case, if you want to know why
> > we're unbinding.
>
> That's more verbose, as well. As Robert said, that makes the output
> easier to debug with a 1:1 mapping between the event and a code path.

I agree with Robert's goal in general. I just think that following
that rule to *this* extent is counterproductive. But I won't die on
that hill; in the end, I just want to be able to see when LDAP calls
hang.

Thanks!
--Jacob