Date: Wed, 12 Jun 2024 16:04:06 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kirill Reshke <reshkekirill@gmail.com>,
	"Leung, Anthony" <antholeu@amazon.com>,
	"Andrey M. Borodin" <x4mmm@yandex-team.ru>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Allow non-superuser to cancel superuser tasks.
Message-ID: <ZmoNRuO9-QlLUjRu@nathan>
References: <ZhNv2yixxyfxD7Iy@paquier.xyz>
 <E35F20FF-4E2A-440C-8097-79B5748AD2DF@amazon.com>
 <ZhTXvFKMkdWDXqG0@paquier.xyz>
 <CALdSSPjVmZMcFm14dzzAGenssJAmV400wBYA=UNWMi9aJeXJMw@mail.gmail.com>
 <ZhXIH3JuKX34r905@paquier.xyz>
 <20240410150034.GA1673069@nathanxps13>
 <ZhcQ8Ki3Ck6VlYqu@paquier.xyz>
 <CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com>
 <CALdSSPj60pTyiCH8C_UmcoHft5TLmzfXSMvf5+5vtZjt7PUuRA@mail.gmail.com>
 <ZhyxTNbkw7PR6BZf@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8o9oIr3uRRuBRCEH"
Content-Disposition: inline
In-Reply-To: <ZhyxTNbkw7PR6BZf@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/ZmoNRuO9-QlLUjRu%40nathan>
Precedence: bulk


--8o9oIr3uRRuBRCEH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

I adjusted 0001 based on my upthread feedback.

-- 
nathan

--8o9oIr3uRRuBRCEH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v5-0001-Introduce-pg_signal_autovacuum_worker.patch"

From 118f95346fcf8099ab28d2f9186185171e3b88af Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 12 Jun 2024 15:38:14 -0500
Subject: [PATCH v5 1/1] 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
---
 doc/src/sgml/func.sgml                |  8 +++--
 doc/src/sgml/user-manag.sgml          |  5 +++
 src/backend/storage/ipc/signalfuncs.c | 46 +++++++++++++++++++++------
 src/include/catalog/pg_authid.dat     |  5 +++
 4 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 17c44bc338..7d5b7acb68 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28037,7 +28037,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>
@@ -28111,7 +28113,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
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/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_' },
 
 ]
-- 
2.39.3 (Apple Git-146)


--8o9oIr3uRRuBRCEH--