public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Andrey M. Borodin <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Leung, Anthony <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Allow non-superuser to cancel superuser tasks.
Date: Fri, 21 Jun 2024 14:36:17 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <ZmykQgj_hWCsGwgp@nathan>
References: <CALdSSPjVmZMcFm14dzzAGenssJAmV400wBYA=UNWMi9aJeXJMw@mail.gmail.com>
<[email protected]>
<20240410150034.GA1673069@nathanxps13>
<[email protected]>
<CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com>
<CALdSSPj60pTyiCH8C_UmcoHft5TLmzfXSMvf5+5vtZjt7PUuRA@mail.gmail.com>
<[email protected]>
<ZmoNRuO9-QlLUjRu@nathan>
<[email protected]>
<ZmykQgj_hWCsGwgp@nathan>
On Fri, Jun 14, 2024 at 03:12:50PM -0500, Nathan Bossart wrote:
> On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
> > This patch looks good to me.
>
> Thanks for looking.
While double-checking the whole, where I don't have much to say about
0001, I have fixed a few issues with the test presented upthread and
stabilized it (CI and my stuff are both OK). I'd suggest to move it
to test_misc/, because there is no clear category where to put it, and
we have another test with injection points there for timeouts so the
module dependency with EXTRA_INSTALL is already cleared.
What do you think?
--
Michael
Attachments:
[text/x-diff] v6-0001-Introduce-pg_signal_autovacuum_worker.patch (7.8K, ../[email protected]/2-v6-0001-Introduce-pg_signal_autovacuum_worker.patch)
download | inline diff:
From 2d3e219149b8dbdbaf2b465f619f898c61969b17 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 12 Jun 2024 15:38:14 -0500
Subject: [PATCH v6 1/2] Introduce pg_signal_autovacuum_worker.
Since commit 3a9b18b309, roles with privileges of pg_signal_backend
cannot signal autovacuum workers. Many users treated the ability
to signal autovacuum workers as a feature instead of a bug, so we
are reintroducing it via a new predefined role. Having privileges
of this new role, named pg_signal_autovacuum_worker, only permits
signaling autovacuum workers. It does not permit signaling other
types of superuser backends.
Author: Kirill Reshke
Reviewed-by: Anthony Leung, Michael Paquier
Discussion: https://postgr.es/m/CALdSSPhC4GGmbnugHfB9G0%3DfAxjCSug_-rmL9oUh0LTxsyBfsg%40mail.gmail.com
---
src/include/catalog/pg_authid.dat | 5 +++
src/backend/storage/ipc/signalfuncs.c | 46 +++++++++++++++++++++------
doc/src/sgml/func.sgml | 8 +++--
doc/src/sgml/user-manag.sgml | 5 +++
4 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index bf00815c14..0129f67eaa 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -99,5 +99,10 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '8916', oid_symbol => 'ROLE_PG_SIGNAL_AUTOVACUUM_WORKER',
+ rolname => 'pg_signal_autovacuum_worker', rolsuper => 'f', rolinherit => 't',
+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+ rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+ rolpassword => '_null_', rolvaliduntil => '_null_' },
]
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index 88e9bf8125..780a1c682a 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -34,8 +34,9 @@
* role as the backend being signaled. For "dangerous" signals, an explicit
* check for superuser needs to be done prior to calling this function.
*
- * Returns 0 on success, 1 on general failure, 2 on normal permission error
- * and 3 if the caller needs to be a superuser.
+ * Returns 0 on success, 1 on general failure, 2 on normal permission error,
+ * 3 if the caller needs to be a superuser, and 4 if the caller needs to have
+ * privileges of pg_signal_autovacuum_worker.
*
* In the event of a general failure (return code 1), a warning message will
* be emitted. For permission errors, doing that is the responsibility of
@@ -45,6 +46,7 @@
#define SIGNAL_BACKEND_ERROR 1
#define SIGNAL_BACKEND_NOPERMISSION 2
#define SIGNAL_BACKEND_NOSUPERUSER 3
+#define SIGNAL_BACKEND_NOAUTOVAC 4
static int
pg_signal_backend(int pid, int sig)
{
@@ -77,15 +79,27 @@ pg_signal_backend(int pid, int sig)
/*
* Only allow superusers to signal superuser-owned backends. Any process
* not advertising a role might have the importance of a superuser-owned
- * backend, so treat it that way.
+ * backend, so treat it that way. As an exception, we allow roles with
+ * privileges of pg_signal_autovacuum_worker to signal autovacuum workers
+ * (which do not advertise a role).
+ *
+ * Otherwise, users can signal backends for roles they have privileges of.
*/
- if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) &&
- !superuser())
- return SIGNAL_BACKEND_NOSUPERUSER;
+ if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId))
+ {
+ ProcNumber procNumber = GetNumberFromPGProc(proc);
+ PgBackendStatus *procStatus = pgstat_get_beentry_by_proc_number(procNumber);
- /* Users can signal backends they have role membership in. */
- if (!has_privs_of_role(GetUserId(), proc->roleId) &&
- !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
+ if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER)
+ {
+ if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM_WORKER))
+ return SIGNAL_BACKEND_NOAUTOVAC;
+ }
+ else if (!superuser())
+ return SIGNAL_BACKEND_NOSUPERUSER;
+ }
+ else if (!has_privs_of_role(GetUserId(), proc->roleId) &&
+ !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
return SIGNAL_BACKEND_NOPERMISSION;
/*
@@ -130,6 +144,13 @@ pg_cancel_backend(PG_FUNCTION_ARGS)
errdetail("Only roles with the %s attribute may cancel queries of roles with the %s attribute.",
"SUPERUSER", "SUPERUSER")));
+ if (r == SIGNAL_BACKEND_NOAUTOVAC)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to cancel query"),
+ errdetail("Only roles with privileges of the \"%s\" role may cancel queries of autovacuum workers.",
+ "pg_signal_autovacuum_worker")));
+
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -236,6 +257,13 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
errdetail("Only roles with the %s attribute may terminate processes of roles with the %s attribute.",
"SUPERUSER", "SUPERUSER")));
+ if (r == SIGNAL_BACKEND_NOAUTOVAC)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to terminate process"),
+ errdetail("Only roles with privileges of the \"%s\" role may terminate autovacuum worker processes.",
+ "pg_signal_autovacuum_worker")));
+
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2609269610..f73ba439c0 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28040,7 +28040,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<para>
Cancels the current query of the session whose backend process has the
specified process ID. This is also allowed if the
- calling role is a member of the role whose backend is being canceled or
+ calling role has privileges of the role whose backend is being canceled,
+ the backend process is an autovacuum worker and the calling role has
+ privileges of <literal>pg_signal_autovacuum_worker</literal>, or
the calling role has privileges of <literal>pg_signal_backend</literal>,
however only superusers can cancel superuser backends.
</para></entry>
@@ -28114,7 +28116,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<para>
Terminates the session whose backend process has the
specified process ID. This is also allowed if the calling role
- is a member of the role whose backend is being terminated or the
+ has privileges of the role whose backend is being terminated,
+ the backend process is an autovacuum worker and the calling role has
+ privileges of <literal>pg_signal_autovacuum_worker</literal>, or the
calling role has privileges of <literal>pg_signal_backend</literal>,
however only superusers can terminate superuser backends.
</para>
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 07a16247d7..340cefff70 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -661,6 +661,11 @@ DROP ROLE doomed_role;
<entry>pg_signal_backend</entry>
<entry>Signal another backend to cancel a query or terminate its session.</entry>
</row>
+ <row>
+ <entry>pg_signal_autovacuum_worker</entry>
+ <entry>Signal an autovacuum worker to cancel the current table's vacuum
+ or terminate its session.</entry>
+ </row>
<row>
<entry>pg_read_server_files</entry>
<entry>Allow reading files from any location the database can access on the server with COPY and
--
2.45.2
[text/x-diff] v6-0002-Add-tap-test-for-pg_signal_autovacuum-role.patch (4.6K, ../[email protected]/3-v6-0002-Add-tap-test-for-pg_signal_autovacuum-role.patch)
download | inline diff:
From 93b7d69585aa4a408749bee6239006d6494df1bb Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Fri, 21 Jun 2024 14:29:23 +0900
Subject: [PATCH v6 2/2] Add tap test for pg_signal_autovacuum role
---
src/backend/postmaster/autovacuum.c | 7 ++
.../test_misc/t/006_signal_autovacuum.pl | 100 ++++++++++++++++++
2 files changed, 107 insertions(+)
create mode 100644 src/test/modules/test_misc/t/006_signal_autovacuum.pl
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9a925a10cd..0d4e2c5140 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -100,6 +100,7 @@
#include "utils/fmgroids.h"
#include "utils/fmgrprotos.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -1902,6 +1903,12 @@ do_autovacuum(void)
/* Start a transaction so our commands have one to play into. */
StartTransactionCommand();
+ /*
+ * This injection point is put in a transaction block to work with a wait
+ * that uses a condition variable.
+ */
+ INJECTION_POINT("autovacuum-worker-start");
+
/*
* Compute the multixact age for which freezing is urgent. This is
* normally autovacuum_multixact_freeze_max_age, but may be less if we are
diff --git a/src/test/modules/test_misc/t/006_signal_autovacuum.pl b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
new file mode 100644
index 0000000000..98cbea3744
--- /dev/null
+++ b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
@@ -0,0 +1,100 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test signaling autovacuum worker backend by non-superuser role.
+#
+# Only non-superuser roles granted pg_signal_autovacuum_worker are allowed
+# to signal autovacuum workers. This test uses an injection point located
+# at the beginning of the autovacuum worker startup.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+# Initialize postgres
+my $psql_err = '';
+my $psql_out = '';
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+
+# This ensures a quick worker spawn.
+$node->append_conf(
+ 'postgresql.conf', 'autovacuum_naptime = 1');
+$node->start;
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+$node->safe_psql(
+ 'postgres', qq(
+ CREATE ROLE regular_role;
+ CREATE ROLE signal_autovacuum_worker_role;
+ GRANT pg_signal_autovacuum_worker TO signal_autovacuum_worker_role;
+));
+
+# From this point, autovacuum worker will wait at startup.
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('autovacuum-worker-start', 'wait');");
+
+# Create some content and set an aggressive autovacuum.
+$node->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_int(i int);
+ ALTER TABLE tab_int SET (autovacuum_vacuum_cost_limit = 1);
+ ALTER TABLE tab_int SET (autovacuum_vacuum_cost_delay = 100);
+));
+
+$node->safe_psql(
+ 'postgres', qq(
+ INSERT INTO tab_int VALUES(1);
+));
+
+# Wait until an autovacuum worker starts.
+$node->wait_for_event('autovacuum worker', 'autovacuum-worker-start');
+
+my $av_pid = $node->safe_psql(
+ 'postgres', qq(
+ SELECT pid FROM pg_stat_activity WHERE backend_type = 'autovacuum worker';
+));
+
+# Regular role cannot terminate autovacuum worker.
+my $terminate_with_no_pg_signal_av = $node->psql(
+ 'postgres', qq(
+ SET ROLE regular_role;
+ SELECT pg_terminate_backend($av_pid);
+),
+ stdout => \$psql_out,
+ stderr => \$psql_err);
+
+like(
+ $psql_err,
+ qr/ERROR: permission denied to terminate process\nDETAIL: Only roles with privileges of the "pg_signal_autovacuum_worker" role may terminate autovacuum worker processes./,
+ "autovacuum worker not signaled with regular role");
+
+my $offset = -s $node->logfile;
+
+# Role with pg_signal_autovacuum can terminate autovacuum worker.
+my $terminate_with_pg_signal_av = $node->psql(
+ 'postgres', qq(
+ SET ROLE signal_autovacuum_worker_role;
+ SELECT pg_terminate_backend($av_pid);
+),
+ stdout => \$psql_out,
+ stderr => \$psql_err);
+
+# Check that the primary server logs a FATAL indicating that autovacuum
+# is terminated.
+ok( $node->log_contains(
+ qr/FATAL: terminating autovacuum process due to administrator command/,
+ $offset),
+ "autovacuum worker signaled with pg_signal_autovacuum_worker granted"
+);
+
+# Release injection point.
+$node->safe_psql('postgres',
+ "SELECT injection_points_detach('autovacuum-worker-start');");
+
+done_testing();
--
2.45.2
[application/pgp-signature] signature.asc (833B, ../[email protected]/4-signature.asc)
download
view thread (19+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Allow non-superuser to cancel superuser tasks.
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox