Re: Allow non-superuser to cancel superuser tasks.

On Mon, Apr 01, 2024 at 02:29:29PM +0000, Leung, Anthony wrote:
> I've attached the updated patch with some improvements.

Thanks!

+ <row>
+ <entry>pg_signal_autovacuum</entry>
+ <entry>Allow terminating backend running autovacuum</entry>
+ </row>

I think we should be more precise here by calling out the exact types of
workers:

"Allow signaling autovacuum worker processes to..."

- if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) &&
- !superuser())
- return SIGNAL_BACKEND_NOSUPERUSER;
-
- /* 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))
- return SIGNAL_BACKEND_NOPERMISSION;
+ if (!superuser())
+ {
+ if (!OidIsValid(proc->roleId))
+ {
+ /*
+ * We only allow user with pg_signal_autovacuum role to terminate
+ * autovacuum worker as an exception.
+ */
+ if (!(pg_stat_is_backend_autovac_worker(proc->backendId) &&
+ has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM)))
+ return SIGNAL_BACKEND_NOSUPERUSER;
+ }
+ else
+ {
+ if (superuser_arg(proc->roleId))
+ return SIGNAL_BACKEND_NOSUPERUSER;
+
+ /* 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))
+ return SIGNAL_BACKEND_NOPERMISSION;
+ }
+ }

I don't think we should rely on !OidIsValid(proc->roleId) for signaling
autovacuum workers. That might not always be true, and I don't see any
need to rely on that otherwise. IMHO we should just add a few lines before
the existing code, which doesn't need to be changed at all:

if (pg_stat_is_backend_autovac_worker(proc->backendId) &&
!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM))
return SIGNAL_BACKEND_NOAUTOVACUUM;

I also think we need to return something besides SIGNAL_BACKEND_NOSUPERUSER
in this case. Specifically, we probably need to introduce a new value and
provide the relevant error messages in pg_cancel_backend() and
pg_terminate_backend().

+/* ----------
+ * pg_stat_is_backend_autovac_worker() -
+ *
+ * Return whether the backend of the given backend id is of type autovacuum worker.
+ */
+bool
+pg_stat_is_backend_autovac_worker(BackendId beid)
+{
+ PgBackendStatus *ret;
+
+ Assert(beid != InvalidBackendId);
+
+ ret = pgstat_get_beentry_by_backend_id(beid);
+
+ if (!ret)
+ return false;
+
+ return ret->st_backendType == B_AUTOVAC_WORKER;
+}

Can we have this function return the backend type so that we don't have to
create a new function for every possible type? That might be handy in the
future.

I haven't looked too closely, but I'm pretty skeptical that the test suite
in your patch would be stable. Unfortunately, I don't have any better
ideas at the moment besides not adding a test for this new role.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com