Re: Proposal: Save user's original authenticated identity for logging

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