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

From: Jacob Champion <[email-masked]>
Date: Mon, 28 Feb 2022 18:58:00 +0000

On Fri, Feb 25, 2022 at 01:23:49PM -0800, Andres Freund wrote:
> 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.

Thanks for the catch. It looks like MyProcPort is left empty, and other
functions that rely on like inet_server_addr() are marked parallel-
restricted, so I've done the same in v4.

On Sat, 2022-02-26 at 14:39 +0900, Michael Paquier wrote:
> 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'm primarily motivated by the linked thread -- if the gap between
builtin roles and authn_id are going to be used as ammo against other
security features, then let's close that gap. But I think it's fair to
say that if someone is already using triggers to exhaustively audit a
table, it'd be nice to have this info in the same place too.

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

Fixed.

> > Perhaps a few tests for less trivial authn_ids could be worthwhile?
> > E.g. certificate DNs.
> 
> 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.

It was easy enough to add, so I added it. I suppose it does protect
against any reimplementations of pg_session_authn_id() that can't
handle longer ID strings, though I admit that's a stretch.

Thanks,
--Jacob
commit efec9f040843d1de2fc52f5ce0d020478a5bc75d
Author: Jacob Champion <[email-masked]>
Date:   Mon Feb 28 10:28:51 2022 -0800

    squash! Add API to retrieve authn_id from SQL

diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c
index b892d25c29..662a7943ed 100644
--- a/src/backend/utils/adt/name.c
+++ b/src/backend/utils/adt/name.c
@@ -258,7 +258,7 @@ namestrcmp(Name name, const char *str)

 /*
- * SQL-functions CURRENT_USER, SESSION_USER, SESSION_AUTHN_ID
+ * SQL-functions CURRENT_USER, SESSION_USER, PG_SESSION_AUTHN_ID
  */
 Datum
 current_user(PG_FUNCTION_ARGS)
@@ -273,7 +273,7 @@ session_user(PG_FUNCTION_ARGS)
 }
 
 Datum
-session_authn_id(PG_FUNCTION_ARGS)
+pg_session_authn_id(PG_FUNCTION_ARGS)
 {
 	if (!MyProcPort || !MyProcPort->authn_id)
 		PG_RETURN_NULL();
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 14194afe1c..3787b8edaf 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202202251
+#define CATALOG_VERSION_NO	202202281
 
 #endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 51e0d24f01..45326a2fe5 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1509,8 +1509,8 @@
   proname => 'session_user', provolatile => 's', prorettype => 'name',
   proargtypes => '', prosrc => 'session_user' },
 { oid => '9774', descr => 'session authenticated identity',
-  proname => 'session_authn_id', provolatile => 's', prorettype => 'text',
-  proargtypes => '', prosrc => 'session_authn_id' },
+  proname => 'pg_session_authn_id', provolatile => 's', proparallel => 'r',
+  prorettype => 'text', proargtypes => '', prosrc => 'pg_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 2aa28ed547..1edac8d588 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -83,7 +83,7 @@ test_role($node, 'md5_role', 'trust', 0,
 	log_unlike => [qr/connection authenticated:/]);
 
 my $res = $node->safe_psql('postgres',
-	"SELECT session_authn_id() IS NULL;"
+	"SELECT pg_session_authn_id() IS NULL;"
 );
 is($res, 't', "users with trust authentication have NULL authn_id");
 
@@ -97,7 +97,7 @@ test_role($node, 'md5_role', 'password', 0,
 	  [qr/connection authenticated: identity="md5_role" method=password/]);
 
 $res = $node->safe_psql('postgres',
-	"SELECT session_authn_id();",
+	"SELECT pg_session_authn_id();",
 	connstr      => "user=md5_role"
 );
 is($res, 'md5_role', "users with md5 authentication have authn_id matching role name");
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 5c5b16fbe7..79ef7b46f1 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -443,6 +443,13 @@ $node->connect_ok(
 		qr/connection authenticated: identity="CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG" method=cert/
 	],);
 
+# Sanity-check pg_session_authn_id() for long ID strings
+my $res = $node->safe_psql('postgres',
+	"SELECT pg_session_authn_id();",
+	connstr => "$dn_connstr user=ssltestuser sslcert=ssl/client-dn.crt sslkey=$key{'client-dn.key'}",
+);
+is($res, "CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG", "users with cert authentication have entire DN as authn_id");
+
 # same thing but with a regex
 $dn_connstr = "$common_connstr dbname=certdb_dn_re";
 
From 8a313bb19ad9d2748edc38ce91464c123642046b Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email-masked]>
Date: Mon, 14 Feb 2022 08:10:53 -0800
Subject: [PATCH v4] 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, pg_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 +++++++++++
 src/test/ssl/t/001_ssltests.pl            |  7 +++++++
 5 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c
index e8bba3670c..662a7943ed 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, PG_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
+pg_session_authn_id(PG_FUNCTION_ARGS)
+{
+	if (!MyProcPort || !MyProcPort->authn_id)
+		PG_RETURN_NULL();
+
+	PG_RETURN_TEXT_P(cstring_to_text(MyProcPort->authn_id));
+}
+
 
 /*
  * SQL-functions CURRENT_SCHEMA, CURRENT_SCHEMAS
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 14194afe1c..3787b8edaf 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202202251
+#define CATALOG_VERSION_NO	202202281
 
 #endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7de8cfc7e9..45326a2fe5 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 => 'pg_session_authn_id', provolatile => 's', proparallel => 'r',
+  prorettype => 'text', proargtypes => '', prosrc => 'pg_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..1edac8d588 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 pg_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 pg_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(
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 5c5b16fbe7..79ef7b46f1 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -443,6 +443,13 @@ $node->connect_ok(
 		qr/connection authenticated: identity="CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG" method=cert/
 	],);
 
+# Sanity-check pg_session_authn_id() for long ID strings
+my $res = $node->safe_psql('postgres',
+	"SELECT pg_session_authn_id();",
+	connstr => "$dn_connstr user=ssltestuser sslcert=ssl/client-dn.crt sslkey=$key{'client-dn.key'}",
+);
+is($res, "CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG", "users with cert authentication have entire DN as authn_id");
+
 # same thing but with a regex
 $dn_connstr = "$common_connstr dbname=certdb_dn_re";
 
-- 
2.25.1

---

From: Stephen Frost <[email-masked]>
Date: Mon, 28 Feb 2022 16:00:36 -0500

Greetings,

> restricted, so I've done the same in v4.

That's probably alright.

> table, it'd be nice to have this info in the same place too.

Yeah, we really should make this available to trigger-based auditing
systems too and not just through log_connections which involves a great
deal more log parsing and work external to the database to figure out
who did what.

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

That's fine.

>     squash! Add API to retrieve authn_id from SQL

Bleh. :)  Squash indeed.

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

Only did a quick look but generally looks reasonable to me.

Thanks,

Stephen