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

On Tue, Mar 30, 2021 at 05:06:51PM +0000, Jacob Champion wrote:
> The key there is "if there is a failure" -- false positives need to be
> debugged too. Tests I've worked with recently for the NSS work were
> succeeding for the wrong reasons. Overly generic logfile matches ("SSL
> error"), for example.

Indeed, so that's a test stability issue. It looks like a good idea
to make those tests more picky with the sub-errors they expect. I see
most "certificate verify failed" a lot, two "sslv3 alert certificate
revoked" and one "tlsv1 alert unknown ca" with 1.1.1, but it is not
something that this patch has to address IMO.

> modules/ssl_passphrase_callback/t/001_testfunc.pl is where I pulled
> this pattern from.

I see. For this case, I see no issue as the input caught is from
_PG_init() so that seems better than a wait on the logs generated.

> Does unilateral log truncation play any nicer with parallel test runs?
> I understand not wanting to make an existing problem worse, but it
> doesn't seem like the existing tests were written for general
> parallelism.

TAP tests running in parallel use their own isolated backend, wiht
dedicated paths and ports.

> Would it be acceptable to adjust the tests for live rotation using the
> logging collector, rather than a full restart? It would unfortunately
> mean that we have to somehow wait for the rotation to complete, since
> that's asynchronous.
>
> (Speaking of asynchronous: how does the existing check-and-truncate
> code make sure that the log entries it's looking for have been flushed
> to disk? Shutting down the server guarantees it.)

stderr redirection looks to be working pretty well with
issues_sql_like().

> I took a stab at this in v13: "Causes each attempted connection to the
> server to be logged, as well as successful completion of both client
> authentication (if necessary) and authorization." (IMO any further in-
> depth explanation of authn/z and user mapping probably belongs in the
> auth method documentation, and this patch doesn't change any authn/z
> behavior.)
>
> v13 also incorporates the latest SSL cert changes, so it's just a
> single patch now. Tests now cover the CN and DN clientname modes. I
> have not changed the log capture method yet; I'll take a look at it
> next.

Thanks, I am looking into that and I am digging into the code now.
--
Michael