Subject: Re: [PATCH] Expose port->authn_id to extensions and triggers

From: Julien Rouhaud <[email-masked]>
Date: Fri, 25 Feb 2022 01:15:38 +0800

Hi,

On Thu, Feb 24, 2022 at 04:50:59PM +0000, Jacob Champion wrote:
> On Thu, 2022-02-24 at 20:39 +0900, Michael Paquier wrote:
> > I don't quite see the additional value that this API brings as
> > MyProcPort is directly accessible, and contrib modules like
> > postgres_fdw and sslinfo just use that to find the data of the current
> > backend.
> 
> Right -- I just didn't know if third-party modules were actually able
> to rely on the internal layout of struct Port. Is that guaranteed to
> remain constant for a major release line? If so, this new API is
> superfluous.

Yes, third-party can rely on Port layout.  We don't break ABI between minor
release.  In some occasions we can add additional fields at the end of a
struct, but nothing more.

> > I could still see a use case for that at a more global level with
> > beentrys, but it looked like there was not much interest the last time
> > I dropped this idea.
> 
> I agree that this would be useful to have in the stats. From my outside
> perspective, it seems like it's difficult to get strings of arbitrary
> length in there; is that accurate?

Yes, as it's all in shared memory.  The only workaround is using something like
track_activity_query_size, but it's not great.

---

From: Jacob Champion <[email-masked]>
Date: Thu, 24 Feb 2022 20:44:08 +0000

> struct, but nothing more.

That simplifies things. PFA a smaller v2; if pgaudit can't make use of
libpq-be.h for some reason, then I guess we can tailor a fix to that
use case.

> track_activity_query_size, but it's not great.

Yeah... I was following a similar track with the initial work last
year, but I dropped it when the cost of implementation started to grow
considerably. At the time, though, it looked like some overhauls to the
stats framework were being pursued? I should read up on that thread.

--Jacob
From 92679a2109be5ba4d81bf58e8fb091c2d0020828 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email-masked]>
Date: Mon, 14 Feb 2022 08:10:53 -0800
Subject: [PATCH v2] Add API to retrieve authn_id from SQL

The authn_id field in MyProcPort is currently only accessible to the
backend itself.  Add a SQL function, session_authn_id(), to expose the
field to triggers that may want to make use of it.
---
 src/backend/utils/adt/name.c              | 12 +++++++++++-
 src/include/catalog/catversion.h          |  2 +-
 src/include/catalog/pg_proc.dat           |  3 +++
 src/test/authentication/t/001_password.pl | 11 +++++++++++
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c
index e8bba3670c..ff0e2829bb 100644
--- a/src/backend/utils/adt/name.c
+++ b/src/backend/utils/adt/name.c
@@ -23,6 +23,7 @@
 #include "catalog/namespace.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
+#include "libpq/libpq-be.h"
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -257,7 +258,7 @@ namestrcmp(Name name, const char *str)

 /*
- * SQL-functions CURRENT_USER, SESSION_USER
+ * SQL-functions CURRENT_USER, SESSION_USER, SESSION_AUTHN_ID
  */
 Datum
 current_user(PG_FUNCTION_ARGS)
@@ -271,6 +272,15 @@ session_user(PG_FUNCTION_ARGS)
 	PG_RETURN_DATUM(DirectFunctionCall1(namein, CStringGetDatum(GetUserNameFromId(GetSessionUserId(), false))));
 }
 
+Datum
+session_authn_id(PG_FUNCTION_ARGS)
+{
+	if (!MyProcPort || !MyProcPort->authn_id)
+		PG_RETURN_NULL();
+
+	PG_RETURN_CSTRING(MyProcPort->authn_id);
+}
+
 
 /*
  * SQL-functions CURRENT_SCHEMA, CURRENT_SCHEMAS
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 1addb568ef..ca52e6889c 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202202221
+#define CATALOG_VERSION_NO	202202231
 
 #endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7f1ee97f55..b76d357bee 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1508,6 +1508,9 @@
 { oid => '746', descr => 'session user name',
   proname => 'session_user', provolatile => 's', prorettype => 'name',
   proargtypes => '', prosrc => 'session_user' },
+{ oid => '9774', descr => 'session authenticated identity',
+  proname => 'session_authn_id', provolatile => 's', prorettype => 'cstring',
+  proargtypes => '', prosrc => 'session_authn_id' },
 
 { oid => '744',
   proname => 'array_eq', prorettype => 'bool',
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 3e3079c824..2aa28ed547 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -82,6 +82,11 @@ test_role($node, 'scram_role', 'trust', 0,
 test_role($node, 'md5_role', 'trust', 0,
 	log_unlike => [qr/connection authenticated:/]);
 
+my $res = $node->safe_psql('postgres',
+	"SELECT session_authn_id() IS NULL;"
+);
+is($res, 't', "users with trust authentication have NULL authn_id");
+
 # For plain "password" method, all users should also be able to connect.
 reset_pg_hba($node, 'password');
 test_role($node, 'scram_role', 'password', 0,
@@ -91,6 +96,12 @@ test_role($node, 'md5_role', 'password', 0,
 	log_like =>
 	  [qr/connection authenticated: identity="md5_role" method=password/]);
 
+$res = $node->safe_psql('postgres',
+	"SELECT session_authn_id();",
+	connstr      => "user=md5_role"
+);
+is($res, 'md5_role', "users with md5 authentication have authn_id matching role name");
+
 # For "scram-sha-256" method, user "scram_role" should be able to connect.
 reset_pg_hba($node, 'scram-sha-256');
 test_role(
-- 
2.25.1

---

From: Jacob Champion <[email-masked]>
Date: Fri, 25 Feb 2022 16:28:39 +0000

> libpq-be.h for some reason, then I guess we can tailor a fix to that

Ha, opr_sanity caught my use of cstring. I'll work on a fix later
today.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 25 Feb 2022 20:19:24 +0000

> Ha, opr_sanity caught my use of cstring. I'll work on a fix later

Fixed in v3.

--Jacob
From 2fde60a6bc3739f1894c8c264120e4fa0f04df64 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email-masked]>
Date: Mon, 14 Feb 2022 08:10:53 -0800
Subject: [PATCH v3] Add API to retrieve authn_id from SQL

index e8bba3670c..b892d25c29 100644

 /*
 
+Datum
+session_authn_id(PG_FUNCTION_ARGS)
+{
+	if (!MyProcPort || !MyProcPort->authn_id)
+		PG_RETURN_NULL();
+
+	PG_RETURN_TEXT_P(cstring_to_text(MyProcPort->authn_id));
+}
+
 
 /*
index 1addb568ef..14194afe1c 100644
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202202221
+#define CATALOG_VERSION_NO	202202251
 
 #endif
index 7f1ee97f55..3ddcbae55e 100644
+  proname => 'session_authn_id', provolatile => 's', prorettype => 'text',
 
 { oid => '744',

---

From: Andres Freund <[email-masked]>
Date: Fri, 25 Feb 2022 13:23:49 -0800

> field to triggers that may want to make use of it.

Looks to me like authn_id isn't synchronized to parallel workers right now. So
the function will return the wrong thing when executed as part of a parallel
query.

I don't think we should add further functions not prefixed with pg_.

Perhaps a few tests for less trivial authn_ids could be worthwhile?
E.g. certificate DNs.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Sat, 26 Feb 2022 14:39:17 +0900

> the function will return the wrong thing when executed as part of a parallel

FWIW, I am not completely sure what's the use case for being able to
see the authn of the current session through a trigger.  We expose
that when log_connections is enabled, for audit purposes.  I can also
get behind something more central so as one can get a full picture of
the authn used by a bunch of session, particularly with complex HBA
policies, but this looks rather limited to me in usability.  Perhaps
that's not enough to stand as an objection, though, and the patch is
dead simple.

> I don't think we should add further functions not prefixed with pg_.

Yep.

> Perhaps a few tests for less trivial authn_ids could be worthwhile?

Yes, src/test/ssl would handle that just fine.  Now, this stuff
already looks after authn results with log_connections=on, so that
feels like a duplicate.
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 1 Mar 2022 19:56:45 +0100

>> Ha, opr_sanity caught my use of cstring. I'll work on a fix later

This patch contains no documentation.  I'm having a hard time 
understanding what the name "session_authn_id" is supposed to convey. 
The comment for the Port.authn_id field says this is the "system 
username", which sounds like a clearer terminology.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 1 Mar 2022 22:05:37 +0000

> username", which sounds like a clearer terminology.

"System username" may help from an internal development perspective,
especially as it relates to pg_ident.conf, but I don't think that's
likely to be a useful descriptor to an end user. (I don't think of a
client certificate's Subject Distinguished Name as a "system
username".) Does my attempt in v5 help?

--Jacob

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 2 Mar 2022 09:18:17 +0100

> username".) Does my attempt in v5 help?

Yeah, maybe there are better names.  But I have no idea what the letter 
combination "authn_id" is supposed to stand for.  Is it an 
"authentication identifier"?  What does it identify?  Maybe I'm missing 
something here, but I don't find it clear.