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

On Tue, Apr 06, 2021 at 06:31:16PM +0000, Jacob Champion wrote:
> On Tue, 2021-04-06 at 14:15 +0900, Michael Paquier wrote:
> > Hmm. You are making a good point here, but is that really the best
> > thing we can do? We lose the context of the authentication type being
> > done with this implementation, and the client would know that it did a
> > re-authentication even if the logdetail goes only to the backend's
> > logs. Wouldn't it be better, for instance, to generate a LOG message
> > in this code path, switch to STATUS_ERROR to let auth_failed()
> > generate the FATAL message? set_authn_id() could just return a
> > boolean to tell if it was OK with the change in authn_id or not.
>
> My concern there is that we already know the code is wrong in this
> (hypothetical future) case, and then we'd be relying on that wrong code
> to correctly bubble up an error status. I think that, once you hit this
> code path, the program flow should be interrupted immediately -- do not
> pass Go, collect $200, or let the bad implementation continue to do
> more damage.

Sounds fair to me.

> I agree that losing the context is not ideal. To avoid that, I thought
> it might be nice to add errbacktrace() to the ereport() call -- but
> since the functions we're interested in are static, the backtrace
> doesn't help. (I should check to see whether libbacktrace is better in
> this situation. Later.)

Perhaps, but that does not seem strongly necessary to me either here.

> If that's a major concern, we could call auth_failed() directly from
> this code. But that means that the auth_failed() logic must not give
> them more ammunition, in this hypothetical scenario where the authn
> system is already messed up. Obscuring the failure mode helps buy
> people time to update Postgres, which definitely has value, but it
> won't prevent any actual exploit by the time we get to this check. A
> tricky trade-off.

Nah. I don't like much a solution that involves calling auth_failed()
in more code paths than now.

>> This requires a perltidy run from what I can see, but that's no big
>> deal.
>
> Is that done per-patch? It looks like there's a large amount of
> untidied code in src/test in general, and in the files being touched.

Committers take care of that usually, but if you can do it that
helps :)

From what I can see, most of the indent diffs are coming from the
tests added with the addition of the log_(un)like parameters. See
pgindent's README for all the details related to the version of
perltidy, for example. The trick is that some previous patches may
not have been indented, causing the apparitions of extra diffs
unrelated to a patch. Usually that's easy enough to fix on a
file-basis.

Anyway, using a FATAL in this code path is fine by me at the end, so I
have applied the patch. Let's see now what the buildfarm thinks about
it.
--
Michael