From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <pchampion@vmware.com>
Cc: magnus@hagander.net <magnus@hagander.net>
Cc: stark@mit.edu <stark@mit.edu>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: sfrost@snowman.net <sfrost@snowman.net>
Cc: tgl@sss.pgh.pa.us <tgl@sss.pgh.pa.us>
Subject: Re: Proposal: Save user's original authenticated identity for logging
Date: Thu, 25 Mar 2021 14:41:59 +0900
Message-ID: <YFwip2On8XmGlfIA@paquier.xyz> (raw)
In-Reply-To: <4c90814d2f0018ba835c3d8e2eb67b99e6a45578.camel@vmware.com>
References: <679382aaff23627a2e53541322c7a3158d402394.camel@vmware.com>
<e3f7433d3e2bf31e8f67a9fb3790f660b7af586e.camel@vmware.com>
<YFML4MDZVWWuIBTF@paquier.xyz>
<YFRfD6h+iosaiQY7@paquier.xyz>
<65354d28ec024d610ee7fe0128a69075d2092043.camel@vmware.com>
<d88b52ac2539caa7eb28bde6db589926e187ab76.camel@vmware.com>
<YFg2QJolwvV7K33B@paquier.xyz>
<3d0e903ae67cf27240f1c392534951e86c939cb7.camel@vmware.com>
<YFl68dieZi3F4q/N@paquier.xyz>
<4c90814d2f0018ba835c3d8e2eb67b99e6a45578.camel@vmware.com>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, pchampion@vmware.com, magnus@hagander.net, stark@mit.edu, sfrost@snowman.net, tgl@sss.pgh.pa.us
Subject: Re: Proposal: Save user's original authenticated identity for logging
In-Reply-To: <YFwip2On8XmGlfIA@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox