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
signature.asc
Description: PGP signature
