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

On Mon, Apr 05, 2021 at 04:40:41PM +0000, Jacob Champion wrote:
> 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.

Argh, Thanks. The part about not checking after the error output when
the connection should pass is wanted to be more consistent with the
other test suites. So I have removed this part and applied the rest
of 0001.

> It looks like this is a reimplementation of v19, but it loses the
> additional tests I wrote? Not sure.

So, what you have here are three extra tests for ldap with
search+bind and search filters. This looks like a good idea.

> Maybe my v19 was sent to spam?

Indeed. All those messages are finishing in my spam folder. I am
wondering why actually. That's a bit surprising.

> 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"

Hmm. It looks like I did something wrong here.

> 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.

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.

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

This requires a perltidy run from what I can see, but that's no big
deal.

+ my (@log_like, @log_unlike);
+ if (defined($params{log_like}))
+ {
+ @log_like = @{ delete $params{log_like} };
+ }
+ if (defined($params{log_unlike}))
+ {
+ @log_unlike = @{ delete $params{log_unlike} };
+ }
There is no need for that? This removal was done as %params was
passed down directly as-is to PostgresNode::psql, but that's not the
case anymore.
--
Michael