On Sat, 2021-03-06 at 18:33 +0100, Magnus Hagander wrote: > It looks like patch 0001 has some leftover debuggnig code at the end? > Or did you intend for that to be included permanently?
I'd intended to keep it -- it works hand-in-hand with the existing "current_logfiles" log line on 219 and might keep someone from tearing their hair out. But I can certainly remove it, if it's cluttering up the logs too much. > As for log escaping, we report port->user_name already unescaped -- > surely this shouldn't be a worse case than that? Ah, that's a fair point. I'll remove the TODO. > I wonder if it wouldn't be better to keep the log line on the existing > "connection authorized" line, just as a separate field. I'm kind of > split on it though, because I guess it might make that line very long. > But it's also a lot more convenient to parse it on a single line than > across multiple lines potentially overlapping with other sessions. Authentication can succeed even if authorization fails, and it's useful to see that in the logs. In most cases that looks like a failed user mapping, but there are other corner cases where we fail the connection after a successful authentication, such as when using krb_realm. Currently you get little to no feedback when that happens, but with a separate log line, it's a lot easier to piece together what's happened. (In general, I feel pretty strongly that Postgres combines/conflates authentication and authorization in too many places.) > With this we store the same value as the authn and as > port->gss->princ, and AFAICT it's only used once. Seems we could just > use the new field for the gssapi usage as well? Especially since that > usage only seems to be there in order to do the gssapi specific > logging of, well, the same thing. > > Same goes for peer_user? In fact, if we're storing it in the Port, why > are we even passing it as a separate parameter to check_usermap -- > shouldn't that one always use this same value? ISTM that it could be > quite confusing if the logged value is different from whatever we > apply to the user mapping? Seems reasonable; I'll consolidate them. --Jacob
