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

On Mon, 2021-04-05 at 14:47 +0900, Michael Paquier wrote:
> On Sat, Apr 03, 2021 at 09:30:25PM +0900, Michael Paquier wrote:
> > Slight rebase for this one to take care of the updates with the SSL
> > error messages.
>
> I have been looking again at that and applied it as c50624cd after
> some slight modifications.

This loses the test fixes I made in my v19 [1]; some of the tests on
HEAD aren't testing anything anymore. I've put those fixups into 0001,
attached.

> Attached is the main, refactored, patch
> that plugs on top of the existing infrastructure. connect_ok() and
> connect_fails() gain two parameters each to match or to not match the
> logs of the backend, with a truncation of the logs done before any
> connection attempt.

It looks like this is a reimplementation of v19, but it loses the
additional tests I wrote? Not sure. Maybe my v19 was sent to spam?

In any case I have attached my Friday patch as 0002.

> I have spent more time reviewing the backend code while on it and
> there was one thing that stood out:
> + ereport(FATAL,
> + (errmsg("connection was re-authenticated"),
> + errdetail_log("previous ID: \"%s\"; new ID: \"%s\"",
> + port->authn_id, id)));
> This message would not actually trigger because auth_failed() is the
> code path in charge of showing an error here

It triggers just fine for me (you can duplicate one of the
set_authn_id() calls to see):

FATAL: connection was re-authenticated
DETAIL: previous ID: "uid=test2,dc=example,dc=net"; new ID: "uid=test2,dc=example,dc=net"

> so this could just be
> replaced by an assertion on authn_id being NULL?

An assertion seems like the wrong way to go; in the event that a future
code path accidentally performs a duplicated authentication, the FATAL
will just kill off an attacker's connection, while an assertion will
DoS the server.

> The contents of this
> log were a bit in contradiction with the comments a couple of lines
> above anyway.

What do you mean by this? I took another look at the comment and it
seems to match the implementation.

v21 attached, which is just a rebase of my original v19.

--Jacob

[1] https://www.postgresql.org/message-id/8c08c6402051b5348d599c0e07bbd83f8614fa16.camel%40vmware.com