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

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-02 18:18:44
Message-ID: 8c08c6402051b5348d599c0e07bbd83f8614fa16.camel@vmware.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, 2021-04-02 at 13:45 +0900, Michael Paquier wrote:
> Attached is what I have come up with as the first building piece,
> which is basically a combination of 0001 and 0002, except that I
> modified things so as the number of arguments remains minimal for all
> the routines. This avoids the manipulation of the list of parameters
> passed down to PostgresNode::psql. The arguments for the optional
> query, the expected stdout and stderr are part of the parameter set
> (0001 was not doing that).

I made a few changes, highlighted in the since-v18 diff:

> + # The result is assumed to match "true", or "t", here.
> + $node->connect_ok($connstr, $test_name, sql => $query,
> + expected_stdout => qr/t/);

I've anchored this as qr/^t$/ so we don't accidentally match a stray
"t" in some larger string.

> - is($res, 0, $test_name);
> - like($stdoutres, $expected, $test_name);
> - is($stderrres, "", $test_name);
> + my ($stdoutres, $stderrres);
> +
> + $node->connect_ok($connstr, $test_name, $query, $expected);

$query and $expected need to be given as named parameters. We also lost
the stderr check from the previous version of the test, so I added
expected_stderr to connect_ok().

> @@ -446,14 +446,14 @@ TODO:
> # correct client cert in encrypted PEM with empty password
> $node->connect_fails(
> "$common_connstr user=ssltestuser sslcert=ssl/client.crt sslkey=ssl/client-encrypted-pem_tmp.key sslpassword=''",
> - qr!\Qprivate key file "ssl/client-encrypted-pem_tmp.key": processing error\E!,
> + expected_stderr => qr!\Qprivate key file "ssl/client-encrypted-pem_tmp.key": processing error\E!,
> "certificate authorization fails with correct client cert and empty password in encrypted PEM format"
> );

These tests don't run yet inside the TODO block, but I've put the
expected_stderr parameter at the end of the list for them.

> For the main patch, this will need to be
> extended with two more parameters in each routine: log_like and
> log_unlike to match for the log patterns, handled as arrays of
> regexes. That's what 0003 is basically doing already.

Rebased on top of your patch as v19, attached. (v17 disappeared into
the ether somewhere, I think. :D)

Now that it's easy to add log_like to existing tests, I fleshed out the
LDAP tests with a few more cases. They don't add code coverage, but
they pin the desired behavior for a few more types of LDAP auth.

--Jacob

Attachment Content-Type Size
since-v18.diff.txt text/plain 3.3 KB
v19-0001-Plug-more-TAP-test-suites-with-new-PostgresNode-.patch text/x-patch 25.5 KB
v19-0002-Log-authenticated-identity-from-all-auth-backend.patch text/x-patch 33.6 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Zhihong Yu 2021-04-02 19:09:12 Re: Parallel Full Hash Join
Previous Message David Steele 2021-04-02 18:09:21 Re: invalid data in file backup_label problem on windows