On Tue, Feb 11, 2025 at 11:23 PM Michael Paquier <mich...@paquier.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
