public inbox for [email protected]  

help / color / mirror / Atom feed

From: Leung, Anthony <[email protected]>
To: Michael Paquier <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Andrey M. Borodin <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Allow non-superuser to cancel superuser tasks.
Date: Mon, 8 Apr 2024 17:42:05 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <20240401202146.GA2354284@nathanxps13>
	<[email protected]>
	<[email protected]>
	<20240404181533.GA3885243@nathanxps13>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<20240405125656.GA4102502@nathanxps13>
	<[email protected]>
	<20240406010751.GA167028@nathanxps13>
	<[email protected]>

>>> There is pg_read_all_stats as well, so I don't see a big issue in
>>> requiring to be a member of this role as well for the sake of what's
>>> proposing here.
>>
>> Well, that tells you quite a bit more than just which PIDs correspond to
>> autovacuum workers, but maybe that's good enough for now.
>
> That may be a good initial compromise, for now.

Sounds good to me. I will update the documentation.


> And we should try to reshape things so as we get an ERROR like
> "permission denied to terminate process" or "permission denied to
> cancel query" for all the error paths, including autovacuum workers 
> and backends, so as we never leak any information about the backend
> types involved when a role has no permission to issue the signal.
> Perhaps that's the most intuitive thing as well, because autovacuum
> workers are backends. One thing that we could do is to mention both
> pg_signal_backend and pg_signal_autovacuum in the errdetail, and have
> both cases be handled by SIGNAL_BACKEND_NOPERMISSION on failure.

I understand your concern that we should avoid exposing the fact that the backend which the user is attempting to terminate is an AV worker unless the user has pg_signal_backend privileges and pg_signal_autovacuum privileges. 
But Im not following how we can re-use SIGNAL_BACKEND_NOPERMISSION for this. If we return SIGNAL_BACKEND_NOPERMISSION here as the following, it'll stay return the "permission denied to terminate / cancel query" errmsg and errdetail in pg_cancel/terminate_backend.

	/*
	 * If the backend is autovacuum worker, allow user with the privileges of
	 * pg_signal_autovacuum role to signal the backend.
	 */
	if (pgstat_get_backend_type(GetNumberFromPGProc(proc)) == B_AUTOVAC_WORKER)
	{
		if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM))
			return SIGNAL_BACKEND_NOPERMISSION;
	}

Are you suggesting that we check if the backend is B_AUTOVAC in pg_cancel/ terminate_backend? That seems a bit unclean to me since pg_cancel_backend & pg_cancel_backend does not access to the procNumber to check the type of the backend.

IMHO, we can keep SIGNAL_BACKEND_NOAUTOVACUUM but just improve the errmsg / errdetail to not expose that the backend is an AV worker. It'll also be helpful if you can suggest what errdetail we should use here.

Thanks
--
Anthony Leung
Amazon Web Services: https://aws.amazon.com

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