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