On Fri, Feb 16, 2024 at 8:55 PM Andres Freund <and...@anarazel.de> wrote:
>
> Hi,
>
> On 2024-01-12 17:16:53 +0100, Magnus Hagander wrote:
> > On Thu, Jan 11, 2024 at 5:55 PM Bertrand Drouvot
> > <bertranddrouvot...@gmail.com> wrote:
> > > On Thu, Jan 11, 2024 at 02:24:58PM +0100, Magnus Hagander wrote:
> > > > On Wed, Jan 10, 2024 at 3:12 PM Bertrand Drouvot
> > > > <bertranddrouvot...@gmail.com> wrote:
> > > > >
> > > > > If we go the 2 fields way, then what about auth_identity and
> > > > > auth_method then?
> > > >
> > > >
> > > > Here is an updated patch based on this idea.
> > >
> > > Thanks!
> > >
> > > + <row>
> > > + <entry role="catalog_table_entry"><para role="column_definition">
> > > + <structfield>auth_method</structfield> <type>text</type>
> > > + </para>
> > > + <para>
> > > + The authentication method used for authenticating the connection,
> > > or
> > > + NULL for background processes.
> > > + </para></entry>
> > >
> > > I'm wondering if it would make sense to populate it for parallel workers
> > > too.
> > > I think it's doable thanks to d951052, but I'm not sure it's worth it
> > > (one could
> > > join based on the leader_pid though). OTOH that would be consistent with
> > > how the SYSTEM_USER behaves with parallel workers (it's populated).
> >
> > I guess one could conceptually argue that "authentication happens int
> > he leader". But we do populate it with the other user records, and
> > it'd be weird if this one was excluded.
> >
> > The tricky thing is that pgstat_bestart() is called long before we
> > deserialize the data. But from what I can tell it should be safe to
> > change it per the attached? That should be AFAICT an extremely short
> > window of time longer before we report it, not enough to matter.
>
> I don't like that one bit. The whole subsystem initialization dance already is
> quite complicated, particularly for pgstat, we shouldn't make it more
> complicated. Especially not when the initialization is moved quite a bit away
> from all the other calls.
>
> Besides just that, I also don't think delaying visibility of the worker in
> pg_stat_activity until parallel worker initialization has completed is good,
> that's not all cheap work.
>
>
> Maybe I am missing something, but why aren't we just getting the value from
> the leader's entry, instead of copying it?
The answer to that would be "because I didn't think of it" :)
Were you thinking of something like the attached?
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index cf3de80394..9403df5886 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23891,7 +23891,7 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
<row>
<entry role="func_table_entry"><para role="func_signature">
- <indexterm>
+ <indexterm id="system-user">
<primary>system_user</primary>
</indexterm>
<function>system_user</function>
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 5cf9363ac8..51ed656b17 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -784,6 +784,28 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>auth_method</structfield> <type>text</type>
+ </para>
+ <para>
+ The authentication method used for authenticating the connection, or
+ NULL for background processes.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>auth_identity</structfield> <type>text</type>
+ </para>
+ <para>
+ The identity (if any) that the user presented during the authentication
+ cycle before they were assigned a database role. Contains the same
+ value as the identity part in <xref linkend="system-user" />, or NULL
+ for background processes.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>application_name</structfield> <type>text</type>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 04227a72d1..bb1060fc66 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -873,6 +873,8 @@ CREATE VIEW pg_stat_activity AS
S.leader_pid,
S.usesysid,
U.rolname AS usename,
+ S.auth_method,
+ S.auth_identity,
S.application_name,
S.client_addr,
S.client_hostname,
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 9bbdc4beb0..6621969154 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -625,9 +625,20 @@ ClientAuthentication(Port *port)
status = CheckRADIUSAuth(port);
break;
case uaCert:
- /* uaCert will be treated as if clientcert=verify-full (uaTrust) */
+
+ /*
+ * uaCert will be treated as if clientcert=verify-full further
+ * down
+ */
+ break;
case uaTrust:
status = STATUS_OK;
+
+ /*
+ * Trust doesn't set_authn_id(), but we still need to store the
+ * auth_method
+ */
+ MyClientConnectionInfo.auth_method = uaTrust;
break;
}
@@ -645,6 +656,12 @@ ClientAuthentication(Port *port)
#endif
}
+ /*
+ * All auth methods should have either called set_authn_id() or manually
+ * set the auth_method if they were successful.
+ */
+ Assert(status != STATUS_OK || MyClientConnectionInfo.auth_method != 0);
+
if (Log_connections && status == STATUS_OK &&
!MyClientConnectionInfo.authn_id)
{
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 1a1050c8da..119319e85a 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -357,6 +357,17 @@ pgstat_bestart(void)
else
MemSet(&lbeentry.st_clientaddr, 0, sizeof(lbeentry.st_clientaddr));
+ lbeentry.st_auth_method = MyClientConnectionInfo.auth_method;
+ if (MyClientConnectionInfo.authn_id)
+ {
+ int len = pg_mbcliplen(MyClientConnectionInfo.authn_id, strlen(MyClientConnectionInfo.authn_id), NAMEDATALEN - 1);
+
+ memcpy(lbeentry.st_auth_identity, MyClientConnectionInfo.authn_id, len);
+ lbeentry.st_auth_identity[len] = '\0';
+ }
+ else
+ lbeentry.st_auth_identity[0] = '\0';
+
#ifdef USE_SSL
if (MyProcPort && MyProcPort->ssl_in_use)
{
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 30a2063505..91d2a406c0 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -304,7 +304,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
Datum
pg_stat_get_activity(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_ACTIVITY_COLS 31
+#define PG_STAT_GET_ACTIVITY_COLS 33
int num_backends = pgstat_fetch_stat_numbackends();
int curr_backend;
int pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
@@ -398,6 +398,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
/* leader_pid */
nulls[29] = true;
+ /* authenticated user */
+ nulls[31] = nulls[32] = true;
+
proc = BackendPidGetProc(beentry->st_procpid);
if (proc == NULL && (beentry->st_backendType != B_BACKEND))
@@ -435,6 +438,22 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
{
values[29] = Int32GetDatum(leader->pid);
nulls[29] = false;
+
+ /*
+ * The authenticated user in a parallel worker is the same as the one in
+ * the leader, so look it up there.
+ */
+ if (leader->backendId)
+ {
+ LocalPgBackendStatus *leaderstat = pgstat_get_local_beentry_by_backend_id(leader->backendId);
+
+ if (leaderstat->backendStatus.st_auth_method != uaReject && leaderstat->backendStatus.st_auth_method != uaImplicitReject)
+ {
+ nulls[31] = nulls[32] = false;
+ values[31] = CStringGetTextDatum(hba_authname(leaderstat->backendStatus.st_auth_method));
+ values[32] = CStringGetTextDatum(leaderstat->backendStatus.st_auth_identity);
+ }
+ }
}
else if (beentry->st_backendType == B_BG_WORKER)
{
@@ -617,6 +636,14 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[30] = true;
else
values[30] = UInt64GetDatum(beentry->st_query_id);
+
+ /* Authenticated user in regular processes */
+ if (beentry->st_auth_method != uaReject && beentry->st_auth_method != uaImplicitReject)
+ {
+ nulls[31] = nulls[32] = false;
+ values[31] = CStringGetTextDatum(hba_authname(beentry->st_auth_method));
+ values[32] = CStringGetTextDatum(beentry->st_auth_identity);
+ }
}
else
{
@@ -646,6 +673,8 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[28] = true;
nulls[29] = true;
nulls[30] = true;
+ nulls[31] = true;
+ nulls[32] = true;
}
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9c120fc2b7..8c55765497 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5440,9 +5440,9 @@
proname => 'pg_stat_get_activity', prorows => '100', proisstrict => 'f',
proretset => 't', provolatile => 's', proparallel => 'r',
prorettype => 'record', proargtypes => 'int4',
- proallargtypes => '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,bool,int4,int8}',
- proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
- proargnames => '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,gss_delegation,leader_pid,query_id}',
+ proallargtypes => '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,bool,int4,int8,text,text}',
+ proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
+ proargnames => '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,gss_delegation,leader_pid,query_id,auth_method,auth_identity}',
prosrc => 'pg_stat_get_activity' },
{ oid => '8403', descr => 'describe wait events',
proname => 'pg_get_wait_events', procost => '10', prorows => '250',
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 7b8a34f64f..cfc45a2224 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -11,6 +11,7 @@
#define BACKEND_STATUS_H
#include "datatype/timestamp.h"
+#include "libpq/hba.h"
#include "libpq/pqcomm.h"
#include "miscadmin.h" /* for BackendType */
#include "storage/backendid.h"
@@ -132,6 +133,10 @@ typedef struct PgBackendStatus
SockAddr st_clientaddr;
char *st_clienthostname; /* MUST be null-terminated */
+ /* Information about the authenticated user */
+ UserAuth st_auth_method;
+ char st_auth_identity[NAMEDATALEN];
+
/* Information about SSL connection */
bool st_ssl;
PgBackendSSLStatus *st_sslstatus;
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 87e180af3d..7d9c409066 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -176,6 +176,22 @@ is($res, 't',
"users with trust authentication use SYSTEM_USER = NULL in parallel workers"
);
+# pg_stat_activity should contain trust and empty string for trust auth
+$res = $node->safe_psql(
+ 'postgres',
+ "SELECT auth_method, auth_identity='' FROM pg_stat_activity WHERE pid=pg_backend_pid()",
+ connstr => "user=scram_role");
+is($res, 'trust|t', 'Users with trust authentication should show correctly in pg_stat_activity');
+
+# pg_stat_activity should contain NULL for auth of background processes
+# (test is a bit out of place here, but this is where the other pg_stat_activity.auth* tests are)
+$res = $node->safe_psql(
+ 'postgres',
+ "SELECT auth_method IS NULL, auth_identity IS NULL FROM pg_stat_activity WHERE backend_type='checkpointer'",
+);
+is($res, 't|t', 'Background processes should show NULL for auth in pg_stat_activity');
+
+
# Explicitly specifying an empty require_auth (the default) should always
# succeed.
$node->connect_ok("user=scram_role require_auth=",
@@ -483,6 +499,13 @@ is($res, 't',
"users with md5 authentication use SYSTEM_USER = md5:role in parallel workers"
);
+# Users with md5 auth should show both auth method and identity in pg_stat_activity
+$res = $node->safe_psql(
+ 'postgres',
+ "SELECT auth_method, auth_identity FROM pg_stat_activity WHERE pid=pg_backend_pid()",
+ connstr => "user=scram_role");
+is($res, 'md5|scram_role', 'Users with md5 authentication should show correctly in pg_stat_activity');
+
# Tests for channel binding without SSL.
# Using the password authentication method; channel binding can't work
reset_pg_hba($node, 'all', 'all', 'password');
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b7488d760e..f81b4fee7f 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1746,6 +1746,8 @@ pg_stat_activity| SELECT s.datid,
s.leader_pid,
s.usesysid,
u.rolname AS usename,
+ s.auth_method,
+ s.auth_identity,
s.application_name,
s.client_addr,
s.client_hostname,
@@ -1762,7 +1764,7 @@ pg_stat_activity| SELECT s.datid,
s.query_id,
s.query,
s.backend_type
- FROM ((pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id)
+ FROM ((pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id, auth_method, auth_identity)
LEFT JOIN pg_database d ON ((s.datid = d.oid)))
LEFT JOIN pg_authid u ON ((s.usesysid = u.oid)));
pg_stat_all_indexes| SELECT c.oid AS relid,
@@ -1882,7 +1884,7 @@ pg_stat_gssapi| SELECT pid,
gss_princ AS principal,
gss_enc AS encrypted,
gss_delegation AS credentials_delegated
- FROM pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id)
+ FROM pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id, auth_method, auth_identity)
WHERE (client_port IS NOT NULL);
pg_stat_io| SELECT backend_type,
object,
@@ -2085,7 +2087,7 @@ pg_stat_replication| SELECT s.pid,
w.sync_priority,
w.sync_state,
w.reply_time
- FROM ((pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id)
+ FROM ((pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id, auth_method, auth_identity)
JOIN pg_stat_get_wal_senders() w(pid, state, sent_lsn, write_lsn, flush_lsn, replay_lsn, write_lag, flush_lag, replay_lag, sync_priority, sync_state, reply_time) ON ((s.pid = w.pid)))
LEFT JOIN pg_authid u ON ((s.usesysid = u.oid)));
pg_stat_replication_slots| SELECT s.slot_name,
@@ -2119,7 +2121,7 @@ pg_stat_ssl| SELECT pid,
ssl_client_dn AS client_dn,
ssl_client_serial AS client_serial,
ssl_issuer_dn AS issuer_dn
- FROM pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id)
+ FROM pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, backend_type, ssl, sslversion, sslcipher, sslbits, ssl_client_dn, ssl_client_serial, ssl_issuer_dn, gss_auth, gss_princ, gss_enc, gss_delegation, leader_pid, query_id, auth_method, auth_identity)
WHERE (client_port IS NOT NULL);
pg_stat_subscription| SELECT su.oid AS subid,
su.subname,
