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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Jacob Champion <pchampion(at)vmware(dot)com>
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-03-31 07:42:32
Message-ID: YGQk74rdCdb8F9gZ@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Mar 30, 2021 at 11:15:48PM +0000, Jacob Champion wrote:
> Rather than putting Postgres log data into the Perl logs, I rotate the
> logs exactly once at the beginning -- so that there's an
> old 001_ssltests_primary.log, and a new 001_ssltests_primary_1.log --
> and then every time we truncate the logfile, I shuffle the bits from
> the new logfile into the old one. So no one has to learn to find the
> log entries in a new place, we don't get an explosion of rotated logs,
> we don't lose the log data, we don't match incorrect portions of the
> logs, and we only pay the restart price once. This is wrapped into a
> small Perl module, LogCollector.

Hmm. I have dug today into that and I am really not convinced that
this is necessary, as a connection attempt combined with the output
sent to stderr gives you the stability needed. If we were to have
anything like that, perhaps a sub-class of PostgresNode would be
adapted instead, with an internal log integration.

After thinking about it, the new wording in config.sgml looks fine
as-is.

Anyway, I have not been able to convince myself that we need those
slowdowns and that many server restarts as there is no
reload-dependent timing here, and things have been stable on
everything I have tested (including a slow RPI). I have found a
couple of things that can be simplified in the tests:
- In src/test/authentication/, except for the trust method where there
is no auth ID, all the other tests wrap a like() if $res == 0, or
unlike() otherwise. I think that it is cleaner to make the matching
pattern an argument of test_role(), and adapt the tests to that.
- src/test/ldap/ can also embed a single logic within test_access().
- src/test/ssl/ is a different beast, but I think that there is more
refactoring possible here in parallel of the recent work I have sent
to have equivalents of test_connect_ok() and test_connect_fails() in
PostgresNode.pm. For now, I think that we should just live in this
set with a small routine able to check for pattern matches in the
logs.

Attached is an updated patch, with a couple of comments tweaks, the
reworked tests and an indentation done.
--
Michael

Attachment Content-Type Size
v15-0001-Log-authenticated-identity-from-all-auth-backend.patch text/x-diff 28.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fabien COELHO 2021-03-31 08:02:42 Re: pgbench - add pseudo-random permutation function
Previous Message Kyotaro Horiguchi 2021-03-31 07:10:31 Re: Issue with point_ops and NaN