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

On Thu, Feb 13, 2025 at 09:53:52AM -0800, Jacob Champion wrote:
> 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?

Is that the case before we finish authentication now? I was not sure
how much of this data is set before and after finishing
authentication, tracking that this was part of the init phase of the
connection, something we do earlier than the actual authentication.
It feels like this should be documented more clearly in the patch if
pgstat_bestart_security() is allowed to be called multiple times (I
think we should not allow that). That's quite unclear now in the
patch; on HEAD everything is set after authentication completes.

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

Making this information visible in the catalogs for already logged in
users increases the potential of problems, and this is a sensitive
area of the code, so..

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

The concept of making pgstat_bestart_security() callable multiple
times relates also to the issue pointed upthread by Andres, somewhat:
allowing this pattern may lead to errors in the future regarding this
that should or should not be set this early in the authentication
process. Sounds just saner to me to do pgstat_bestart_security()
once for now once we're OK with authentication, and it does not
prevent to address your initial point of being able to know if
backends with a given PID are stuck at a certain point. At least
that's a step towards more debuggability as the backend entries are
available earlier than they are now.

Getting ready for tomatoes now, please aim freely.

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

Understood.
--
Michael