Re: Proposal: Save user's original authenticated identity for logging - Mailing list pgsql-hackers

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@%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