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

On Wed, Mar 24, 2021 at 04:45:35PM +0000, Jacob Champion wrote:
> With low-coverage test suites, I think it's useful to allow as little
> strange behavior as possible -- in this case, printing authorization
> before authentication could signal a serious bug -- but I don't feel
> too strongly about it.

I got to look at the DN patch yesterday, so now's the turn of this
one. Nice work.

+ * Sets the authenticated identity for the current user. The provided string
+ * will be copied into the TopMemoryContext. The ID will be logged if
+ * log_connections is enabled.
[...]
+ port->authn_id = MemoryContextStrdup(TopMemoryContext, id);
It may not be obvious that all the field is copied to TopMemoryContext
because the Port requires that.

+$node->stop('fast');
+my $log_contents = slurp_file($log);
Like 11e1577, let's just truncate the log files in all those tests.

+ 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?

+like(
+ $log_contents,
+ qr/connection authenticated: identity="ssltestuser"
method=scram-sha-256/,
+ "Basic SCRAM sets the username as the authenticated identity");
+
+$node->start;
It looks wrong to me to include in the SSL tests some checks related
to SCRAM authentication. This should remain in 001_password.pl, as of
src/test/authentication/.

port->gss->princ = MemoryContextStrdup(TopMemoryContext, port->gbuf.value);
+ set_authn_id(port, gbuf.value);
I don't think that this position is right for GSSAPI. Shouldn't this
be placed at the end of pg_GSS_checkauth() and only if the status is
OK?

- ret = check_usermap(port->hba->usermap, port->user_name, peer_user, false);
-
- pfree(peer_user);
+ ret = check_usermap(port->hba->usermap, port->user_name, port->authn_id, false);
I would also put this one after checking the usermap for peer.

+ /*
+ * We have all of the information necessary to construct the authenticated
+ * identity.
+ */
+ if (port->hba->compat_realm)
+ {
+ /* SAM-compatible format. */
+ authn_id = psprintf("%s\\%s", domainname, accountname);
+ }
+ else
+ {
+ /* Kerberos principal format. */
+ authn_id = psprintf("%s(at)%s", accountname, domainname);
+ }
+
+ set_authn_id(port, authn_id);
+ pfree(authn_id);
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.

Reading through the thread, the consensus is to add the identity
information with log_connections. One question I have is that if we
just log the information with log_connectoins, there is no real reason
to add this information to the Port, except the potential addition of
some system function, a superuser-only column in pg_stat_activity or
to allow extensions to access this information. I am actually in
favor of keeping this information in the Port with those pluggability
reasons. How do others feel about that?
--
Michael