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

On Mon, Feb 10, 2025 at 11:05:32AM -0800, Jacob Champion wrote:
> Bad regex escaping on my part; fixed in v8. Thanks for the report!
>
> While debugging, I also noticed that a poorly timed autovacuum could
> also show up in my new pg_stat_activity query, so I've increased the
> specificity.

Now this test is able to catch the reason why it has been added.
Thanks for the fix.

Allowing pgstat_bestart_security() to be reentrant worries me.

Anyway, when it comes to the fields that would show up in the catalogs
before authentication completes, are we sure that all of them are OK
and that it would not become an open door for pushing data into the
catalogs that other users could scan?

+ lsslstatus.ssl_bits = be_tls_get_cipher_bits(MyProcPort);
+ strlcpy(lsslstatus.ssl_version, be_tls_get_version(MyProcPort), NAMEDATALEN);
+ strlcpy(lsslstatus.ssl_cipher, be_tls_get_cipher(MyProcPort), NAMEDATALEN);

These three for SSL are OK, they rely on a hardcoded set of values.

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

+ lgssstatus.gss_auth = be_gssapi_get_auth(MyProcPort);
+ lgssstatus.gss_enc = be_gssapi_get_enc(MyProcPort);
+ lgssstatus.gss_delegation = be_gssapi_get_delegation(MyProcPort);

These three are booleans, hence OK. They are set when the connection
opens, before the first call of pgstat_bestart_security(), right?

+ if (princ)
+ strlcpy(lgssstatus.gss_princ, princ, NAMEDATALEN);

This is not going to be set, being part of pg_GSS_checkauth() that
happens later on.

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. Perhaps it would be OK to document what can be
set by the first call and/or the second call, but at the end it seems
that this is going to require a split, or just to move the fields that
are potentially unsafe into the final step where we know that we're
done with authentication, leaving in the security() call the fields
that we are definitely OK to have. The boolean states from the Port
copied into the backend entries are fine. The data coming from the
peer certificate when initializing the secure connection look
problematic. We should be careful.

Daniel, perhaps you could comment about the fact of showing all these
fields in the catalogs before performing any authentication? That
worries me.

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