Re: Allow non-superuser to cancel superuser tasks.

Leung, Anthony <antholeu@amazon.com>

From: "Leung, Anthony" <antholeu@amazon.com>
To: Nathan Bossart <nathandbossart@gmail.com>, Kirill Reshke <reshkekirill@gmail.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2024-03-10T04:13:50Z
Lists: pgsql-hackers
Hi, 

I'm new to reviewing postgres patches, but I took an interest in reviewing this patch as recommended by Nathan.

I have the following comments:

> 	if (!superuser()) {
>		if (!OidIsValid(proc->roleId)) {
>			LocalPgBackendStatus *local_beentry;
>			local_beentry = pgstat_get_local_beentry_by_backend_id(proc->backendId);
>
>			if (!(local_beentry && local_beentry->backendStatus.st_backendType == B_AUTOVAC_WORKER && 
>				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;
>		}
>	}
>
1. I would suggest not to do nested if blocks since it's becoming harder to read. Also, does it make sense to have a utilities function in backend_status.c to check if a backend of a given backend id is of a certain backend_type. And should we check if proc->backendId is invalid?

>    ALTER SYSTEM SET autovacuum_vacuum_cost_limit TO 1;
>    ALTER SYSTEM SET autovacuum_vacuum_cost_delay TO 100;
>    ALTER SYSTEM SET autovacuum_naptime TO 1;    
2. Could we set these parameters at the beginning of the test before $node->start with $node->append_conf ? That way we can avoid restarting the node and doing the sleep later on.

> my $res_pid = $node_primary->safe_psql(
>.           'regress',
>	"SELECT pid FROM pg_stat_activity WHERE backend_type = 'autovacuum worker' and datname = 'regress';"
> );
>
> my ($res_reg_psa_1, $stdout_reg_psa_1, $stderr_reg_psa_1)  = $node_primary->psql('regress', qq[
     SET ROLE psa_reg_role_1;
>   SELECT pg_terminate_backend($res_pid);
>  ]);
>
> ok($res_reg_psa_1 != 0, "should fail for non pg_signal_autovacuum");
> like($stderr_reg_psa_1, qr/Only roles with the SUPERUSER attribute may terminate processes of roles with the SUPERUSER attribute./, "matches");
>
> my ($res_reg_psa_2, $stdout_reg_psa_2, $stderr_reg_psa_2) = $node_primary->psql('regress', qq[
>    SET ROLE psa_reg_role_2;
>    SELECT pg_terminate_backend($res_pid);
> ]");
3. Some nits on styling 

4. According to Postgres styles, I believe open brackets should be in a new line 

Commits

  1. Look up backend type in pg_signal_backend() more cheaply.

  2. Add tap test for pg_signal_autovacuum role

  3. Introduce pg_signal_autovacuum_worker.

  4. Add a slot synchronization function.

  5. Ban role pg_signal_backend from more superuser backend types.