On Thu, 2022-03-17 at 18:33 -0400, Tom Lane wrote:
> Yeah. It seems to me that putting the auth info into struct Port was
> a fairly random thing to do in the first place, and we are now dealing
> with the fallout of that.
>
> I think what we ought to do here is separate out the data that we think
> parallel workers need access to. It does not seem wise to say "workers
> can access fields A,B,C of MyPort but not fields X,Y,Z". I do not have
> a concrete proposal for fixing it though.
v6-0002 has my first attempt at this. I moved authn_id into its own
substruct inside Port, which gets serialized with the parallel key
machinery. (My name selection of "SharedPort" is pretty bland.)
Over time, we could move more fields into the shared struct and fill
out the serialization logic as needed, and then maybe eventually
SharedPort can be broken off into its own thing with its own
allocation. But I don't know if we should do it all at once, yet.
WDYT?
--Jacob
From c1323d7694deedf1fa829772083977c18c7f77f6 Mon Sep 17 00:00:00 2001
From: Jacob Champion <pchamp...@vmware.com>
Date: Mon, 14 Feb 2022 08:10:53 -0800
Subject: [PATCH v6 1/3] 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.
---
doc/src/sgml/func.sgml | 26 +++++++++++++++++++++++
src/backend/utils/adt/name.c | 12 ++++++++++-
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, 58 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 89a5e17884..45df4ff158 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -22280,6 +22280,32 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
</para></entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_session_authn_id</primary>
+ </indexterm>
+ <function>pg_session_authn_id</function> ()
+ <returnvalue>text</returnvalue>
+ </para>
+ <para>
+ Returns the authenticated identity for the current connection, or
+ <literal>NULL</literal> if the user has not been authenticated.
+ </para>
+ <para>
+ The authenticated identity is an immutable identifier for the user
+ presented during the connection handshake; the exact format depends on
+ the authentication method in use. (For example, when using the
+ <literal>scram-sha-256</literal> auth method, the authenticated identity
+ is simply the username. When using the <literal>cert</literal> auth
+ method, the authenticated identity is the Distinguished Name of the
+ client certificate.) Even for auth methods which use the username as
+ the authenticated identity, this function differs from
+ <literal>session_user</literal> in that its return value cannot be
+ changed after login.
+ </para></entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
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/pg_proc.dat b/src/include/catalog/pg_proc.dat
index d8e8715ed1..a1bf898476 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..f0bdeda52d 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -82,6 +82,10 @@ 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 +95,13 @@ 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 c96254c0656ffd7fc9ddf5cea524be42cce71c77 Mon Sep 17 00:00:00 2001
From: Jacob Champion <pchamp...@vmware.com>
Date: Wed, 23 Mar 2022 15:07:05 -0700
Subject: [PATCH v6 2/3] Allow parallel workers to use pg_session_authn_id()
Move authn_id into a substruct, SharedPort, which is intended to hold
all the information that can be shared between the backend and any
parallel workers. SharedPort is serialized and restored using a new
parallel key.
With this change, the parallel restriction can be removed from
pg_session_authn_id().
---
src/backend/access/transam/parallel.c | 91 ++++++++++++++++++++++-
src/backend/libpq/auth.c | 12 +--
src/backend/utils/adt/name.c | 4 +-
src/include/catalog/pg_proc.dat | 2 +-
src/include/libpq/libpq-be.h | 38 +++++++---
src/test/authentication/t/001_password.pl | 33 ++++++++
6 files changed, 161 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index df0cd77558..dda2aab7b1 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -76,6 +76,7 @@
#define PARALLEL_KEY_REINDEX_STATE UINT64CONST(0xFFFFFFFFFFFF000C)
#define PARALLEL_KEY_RELMAPPER_STATE UINT64CONST(0xFFFFFFFFFFFF000D)
#define PARALLEL_KEY_UNCOMMITTEDENUMS UINT64CONST(0xFFFFFFFFFFFF000E)
+#define PARALLEL_KEY_SHAREDPORT UINT64CONST(0xFFFFFFFFFFFF000F)
/* Fixed-size parallel state. */
typedef struct FixedParallelState
@@ -212,6 +213,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
Size reindexlen = 0;
Size relmapperlen = 0;
Size uncommittedenumslen = 0;
+ Size sharedportlen = 0;
Size segsize = 0;
int i;
FixedParallelState *fps;
@@ -272,8 +274,10 @@ InitializeParallelDSM(ParallelContext *pcxt)
shm_toc_estimate_chunk(&pcxt->estimator, relmapperlen);
uncommittedenumslen = EstimateUncommittedEnumsSpace();
shm_toc_estimate_chunk(&pcxt->estimator, uncommittedenumslen);
+ sharedportlen = EstimateSharedPortSpace();
+ shm_toc_estimate_chunk(&pcxt->estimator, sharedportlen);
/* If you add more chunks here, you probably need to add keys. */
- shm_toc_estimate_keys(&pcxt->estimator, 11);
+ shm_toc_estimate_keys(&pcxt->estimator, 12);
/* Estimate space need for error queues. */
StaticAssertStmt(BUFFERALIGN(PARALLEL_ERROR_QUEUE_SIZE) ==
@@ -352,6 +356,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
char *session_dsm_handle_space;
char *entrypointstate;
char *uncommittedenumsspace;
+ char *sharedportspace;
Size lnamelen;
/* Serialize shared libraries we have loaded. */
@@ -422,6 +427,12 @@ InitializeParallelDSM(ParallelContext *pcxt)
shm_toc_insert(pcxt->toc, PARALLEL_KEY_UNCOMMITTEDENUMS,
uncommittedenumsspace);
+ /* Serialize our SharedPort. */
+ sharedportspace = shm_toc_allocate(pcxt->toc, sharedportlen);
+ SerializeSharedPort(sharedportlen, sharedportspace);
+ shm_toc_insert(pcxt->toc, PARALLEL_KEY_SHAREDPORT,
+ sharedportspace);
+
/* Allocate space for worker information. */
pcxt->worker = palloc0(sizeof(ParallelWorkerInfo) * pcxt->nworkers);
@@ -467,6 +478,79 @@ InitializeParallelDSM(ParallelContext *pcxt)
MemoryContextSwitchTo(oldcontext);
}
+/*
+ * Calculate the space needed to serialize MyProcPort->shared.
+ */
+Size
+EstimateSharedPortSpace(void)
+{
+ SharedPort *shared = &MyProcPort->shared;
+ Size size = 1;
+
+ if (shared->authn_id)
+ size = add_size(size, strlen(shared->authn_id) + 1);
+
+ return size;
+}
+
+/*
+ * Serialize MyProcPort->shared for use by parallel workers.
+ */
+void
+SerializeSharedPort(Size maxsize, char *start_address)
+{
+ SharedPort *shared = &MyProcPort->shared;
+
+ /*
+ * First byte is an indication of whether or not authn_id has been set to
+ * non-NULL, to differentiate that case from the empty string.
+ */
+ Assert(maxsize > 0);
+ start_address[0] = shared->authn_id ? 1 : 0;
+ start_address++;
+ maxsize--;
+
+ if (shared->authn_id)
+ {
+ Size len;
+
+ len = strlcpy(start_address, shared->authn_id, maxsize) + 1;
+ Assert(len <= maxsize);
+ maxsize -= len;
+ start_address += len;
+ }
+}
+
+/*
+ * Restore MyProcPort->shared from its serialized representation, allocating
+ * MyProcPort if necessary.
+ */
+void
+RestoreSharedPort(char *sharedport)
+{
+ /* First make sure we have a place to put the information. */
+ if (!MyProcPort)
+ {
+ if (!(MyProcPort = calloc(1, sizeof(Port))))
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+ }
+
+ if (sharedport[0] == 0)
+ {
+ MyProcPort->shared.authn_id = NULL;
+ sharedport++;
+ }
+ else
+ {
+ sharedport++;
+ MyProcPort->shared.authn_id = MemoryContextStrdup(TopMemoryContext,
+ sharedport);
+ sharedport += strlen(sharedport) + 1;
+ }
+}
+
/*
* Reinitialize the dynamic shared memory segment for a parallel context such
* that we could launch workers for it again.
@@ -1270,6 +1354,7 @@ ParallelWorkerMain(Datum main_arg)
char *reindexspace;
char *relmapperspace;
char *uncommittedenumsspace;
+ char *sharedportspace;
StringInfoData msgbuf;
char *session_dsm_handle_space;
Snapshot tsnapshot;
@@ -1479,6 +1564,10 @@ ParallelWorkerMain(Datum main_arg)
false);
RestoreUncommittedEnums(uncommittedenumsspace);
+ /* Restore the SharedPort. */
+ sharedportspace = shm_toc_lookup(toc, PARALLEL_KEY_SHAREDPORT, false);
+ RestoreSharedPort(sharedportspace);
+
/* Attach to the leader's serializable transaction, if SERIALIZABLE. */
AttachSerializableXact(fps->serializable_xact_handle);
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index efc53f3135..40384a31b0 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -350,7 +350,7 @@ set_authn_id(Port *port, const char *id)
{
Assert(id);
- if (port->authn_id)
+ if (port->shared.authn_id)
{
/*
* An existing authn_id should never be overwritten; that means two
@@ -361,17 +361,18 @@ set_authn_id(Port *port, const char *id)
ereport(FATAL,
(errmsg("authentication identifier set more than once"),
errdetail_log("previous identifier: \"%s\"; new identifier: \"%s\"",
- port->authn_id, id)));
+ port->shared.authn_id, id)));
}
- port->authn_id = MemoryContextStrdup(TopMemoryContext, id);
+ port->shared.authn_id = MemoryContextStrdup(TopMemoryContext, id);
if (Log_connections)
{
ereport(LOG,
errmsg("connection authenticated: identity=\"%s\" method=%s "
"(%s:%d)",
- port->authn_id, hba_authname(port->hba->auth_method), HbaFileName,
+ port->shared.authn_id,
+ hba_authname(port->hba->auth_method), HbaFileName,
port->hba->linenumber));
}
}
@@ -1908,7 +1909,8 @@ auth_peer(hbaPort *port)
*/
set_authn_id(port, pw->pw_name);
- ret = check_usermap(port->hba->usermap, port->user_name, port->authn_id, false);
+ ret = check_usermap(port->hba->usermap, port->user_name,
+ port->shared.authn_id, false);
return ret;
#else
diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c
index 662a7943ed..9000ad05f8 100644
--- a/src/backend/utils/adt/name.c
+++ b/src/backend/utils/adt/name.c
@@ -275,10 +275,10 @@ session_user(PG_FUNCTION_ARGS)
Datum
pg_session_authn_id(PG_FUNCTION_ARGS)
{
- if (!MyProcPort || !MyProcPort->authn_id)
+ if (!MyProcPort || !MyProcPort->shared.authn_id)
PG_RETURN_NULL();
- PG_RETURN_TEXT_P(cstring_to_text(MyProcPort->authn_id));
+ PG_RETURN_TEXT_P(cstring_to_text(MyProcPort->shared.authn_id));
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index a1bf898476..b044a71c93 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1509,7 +1509,7 @@
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',
+ proname => 'pg_session_authn_id', provolatile => 's',
prorettype => 'text', proargtypes => '', prosrc => 'pg_session_authn_id' },
{ oid => '744',
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index dd3e5efba3..0a9dc61d04 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -99,6 +99,27 @@ typedef struct
} pg_gssinfo;
#endif
+/*
+ * Fields from Port that need to be copied over to parallel workers go into the
+ * SharedPort. The same rules apply for allocations here as for Port (must be
+ * malloc'd or palloc'd in TopMemoryContext).
+ */
+typedef struct SharedPort
+{
+ /*
+ * Authenticated identity. The meaning of this identifier is dependent on
+ * hba->auth_method; it is the identity (if any) that the user presented
+ * during the authentication cycle, before they were assigned a database
+ * role. (It is effectively the "SYSTEM-USERNAME" of a pg_ident usermap
+ * -- though the exact string in use may be different, depending on pg_hba
+ * options.)
+ *
+ * authn_id is NULL if the user has not actually been authenticated, for
+ * example if the "trust" auth method is in use.
+ */
+ const char *authn_id;
+} SharedPort;
+
/*
* This is used by the postmaster in its communication with frontends. It
* contains all state information needed during this communication before the
@@ -160,17 +181,10 @@ typedef struct Port
HbaLine *hba;
/*
- * Authenticated identity. The meaning of this identifier is dependent on
- * hba->auth_method; it is the identity (if any) that the user presented
- * during the authentication cycle, before they were assigned a database
- * role. (It is effectively the "SYSTEM-USERNAME" of a pg_ident usermap
- * -- though the exact string in use may be different, depending on pg_hba
- * options.)
- *
- * authn_id is NULL if the user has not actually been authenticated, for
- * example if the "trust" auth method is in use.
+ * Information that's copied between the backend and any parallel workers.
+ * This is the only part of the Port that a parallel worker may access!
*/
- const char *authn_id;
+ SharedPort shared;
/*
* TCP keepalive and user timeout settings.
@@ -341,4 +355,8 @@ extern int pq_setkeepalivesinterval(int interval, Port *port);
extern int pq_setkeepalivescount(int count, Port *port);
extern int pq_settcpusertimeout(int timeout, Port *port);
+extern Size EstimateSharedPortSpace(void);
+extern void SerializeSharedPort(Size maxsize, char *start_address);
+extern void RestoreSharedPort(char *sharedport);
+
#endif /* LIBPQ_BE_H */
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index f0bdeda52d..3f8629b3a6 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -74,6 +74,14 @@ $node->safe_psql('postgres',
);
$ENV{"PGPASSWORD"} = 'pass';
+# Set up a table for parallel worker testing.
+$node->safe_psql('postgres',
+ 'CREATE TABLE nulls (n) AS SELECT NULL FROM generate_series(1, 200000);'
+);
+$node->safe_psql('postgres',
+ 'GRANT SELECT ON nulls TO md5_role;'
+);
+
# For "trust" method, all users should be able to connect. These users are not
# considered to be authenticated.
reset_pg_hba($node, 'trust');
@@ -86,6 +94,19 @@ my $res =
$node->safe_psql('postgres', "SELECT pg_session_authn_id() IS NULL;");
is($res, 't', "users with trust authentication have NULL authn_id");
+# Test pg_session_authn_id() with parallel workers.
+$res = $node->safe_psql(
+ 'postgres', '
+ SET min_parallel_table_scan_size TO 0;
+ SET parallel_setup_cost TO 0;
+ SET parallel_tuple_cost TO 0;
+ SET max_parallel_workers_per_gather TO 2;
+
+ SELECT bool_and(pg_session_authn_id() IS NOT DISTINCT FROM n) FROM nulls;
+ ',
+ connstr => "user=md5_role");
+is($res, 't', "parallel workers return a null authn_id when not authenticated");
+
# For plain "password" method, all users should also be able to connect.
reset_pg_hba($node, 'password');
test_role($node, 'scram_role', 'password', 0,
@@ -102,6 +123,18 @@ $res = $node->safe_psql(
is($res, 'md5_role',
"users with md5 authentication have authn_id matching role name");
+$res = $node->safe_psql(
+ 'postgres', '
+ SET min_parallel_table_scan_size TO 0;
+ SET parallel_setup_cost TO 0;
+ SET parallel_tuple_cost TO 0;
+ SET max_parallel_workers_per_gather TO 2;
+
+ SELECT bool_and(pg_session_authn_id() IS DISTINCT FROM n) FROM nulls;
+ ',
+ connstr => "user=md5_role");
+is($res, 't', "parallel workers return a non-null authn_id when authenticated");
+
# 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
