From: | Jacob Champion <pchampion(at)vmware(dot)com> |
---|---|
To: | "michael(at)paquier(dot)xyz" <michael(at)paquier(dot)xyz> |
Cc: | "magnus(at)hagander(dot)net" <magnus(at)hagander(dot)net>, "stark(at)mit(dot)edu" <stark(at)mit(dot)edu>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "sfrost(at)snowman(dot)net" <sfrost(at)snowman(dot)net>, "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: Proposal: Save user's original authenticated identity for logging |
Date: | 2021-04-05 16:40:41 |
Message-ID: | 4f5b0b3dc0b6fe9ae6a34886b4d4000f61eb567e.camel@vmware.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v21-0001-test-kerberos-fix-up-test_query.patch | text/x-patch | 3.9 KB |
v21-0002-Log-authenticated-identity-from-all-auth-backend.patch | text/x-patch | 33.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2021-04-05 17:16:27 | Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view? |
Previous Message | Justin Pryzby | 2021-04-05 16:33:10 | Re: Have I found an interval arithmetic bug? |