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

On Tue, 2021-04-06 at 14:15 +0900, Michael Paquier wrote:
> 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.

I assumed Tom added those checks to catch a particular failure mode for
the GSS encryption case. (I guess Tom would know for sure.)

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

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.

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

As for the client knowing: an active attacker is probably going to know
that they're triggering the reauthentication anyway. So the primary
disadvantage I see is that a more passive attacker could scan for some
vulnerability by looking for that error message.

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.

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

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.

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

Fixed in v22, thanks.

--Jacob