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

On Thu, Mar 25, 2021 at 06:51:22PM +0000, Jacob Champion wrote:
> On Thu, 2021-03-25 at 14:41 +0900, Michael Paquier wrote:
>> + port->authn_id = MemoryContextStrdup(TopMemoryContext, id);
>> It may not be obvious that all the field is copied to TopMemoryContext
>> because the Port requires that.
>
> I've expanded the comment. (v11 attached, with incremental changes over
> v10 in since-v10.diff.txt.)

That's the addition of "to match the lifetime of the Port". Looks
good.

>> +$node->stop('fast');
>> +my $log_contents = slurp_file($log);
>> Like 11e1577, let's just truncate the log files in all those tests.
>
> Hmm... having the full log file contents for the SSL tests has been
> incredibly helpful for me with the NSS work. I'd hate to lose them; it
> can be very hard to recreate the test conditions exactly.

Does it really matter to have the full contents of the file from the
previous tests though? like() would report the contents of
slurp_file() when it fails if the generated output does not match the
expected one, so you actually get less noise this way.

>> + if (auth_method < 0 || USER_AUTH_LAST < auth_method)
>> + {
>> + Assert((0 <= auth_method) && (auth_method <= USER_AUTH_LAST));
>> What's the point of having the check and the assertion? NULL does not
>> really seem like a good default here as this should never really
>> happen. Wouldn't a FATAL be actually safer?
>
> I think FATAL makes more sense. Changed, thanks.

Thanks. FWIW, one worry I had here was a corrupted stack that calls
this code path that would remain undetected.

>> For SSPI, I think that this should be moved down once we are sure that
>> there is no error and that pg_SSPI_recvauth() reports STATUS_OK to the
>> caller. There is a similar issue with CheckCertAuth(), and
>> set_authn_id() is documented so as it should be called only when we
>> are sure that authentication succeeded.
>
> Authentication *has* succeeded already; that's what the SSPI machinery
> has done above. Likewise for CheckCertAuth, which relies on the TLS
> subsystem to validate the client signature before setting the peer_cn.
> The user mapping is an authorization concern: it answers the question,
> "is an authenticated user allowed to use a particular Postgres user
> name?"

Okay. Could you make the comments in those various areas more
explicit about the difference and that it is intentional to register
the auth ID before checking the user map? Anybody reading this code
in the future may get confused with the differences in handling all
that according to the auth type involved if that's not clearly
stated.

> That was my intent, yeah. Getting this into the stats framework was
> more than I could bite off for this first patchset, but having it
> stored in a central location will hopefully help people do more with
> it.

No problem with that.
--
Michael