Re: Allow non-superuser to cancel superuser tasks.

From: Nathan Bossart <nathandbossart(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: "Leung, Anthony" <antholeu(at)amazon(dot)com>, "Andrey M(dot) Borodin" <x4mmm(at)yandex-team(dot)ru>, Kirill Reshke <reshkekirill(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Allow non-superuser to cancel superuser tasks.
Date: 2024-04-05 12:56:56
Message-ID: 20240405125656.GA4102502@nathanxps13
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Apr 05, 2024 at 02:39:05PM +0900, Michael Paquier wrote:
> + /*
> + * 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_NOAUTOVACUUM;
> + }
>
> I was wondering why this is not done after we've checked that we have
> a superuser-owned backend, and this is giving me a pause. @Nathan,
> why do you think we should not rely on the roleId for an autovacuum
> worker? In core, do_autovacuum() is only called in a process without
> a role specified, and I've noticed your remark here:
> https://www.postgresql.org/message-id/20240401202146.GA2354284@nathanxps13
> It's feeling more natural here to check that we have a superuser-owned
> backend first, and then do a lookup of the process type.

I figured since there's no reason to rely on that behavior, we might as
well do a bit of future-proofing in case autovacuum workers are ever not
run as InvalidOid. It'd be easy enough to fix this code if that ever
happened, so I'm not too worried about this.

> One thing that we should definitely not do is letting any user calling
> pg_signal_backend() know that a given PID maps to an autovacuum
> worker. This information is hidden in pg_stat_activity. And
> actually, doesn't the patch leak this information to all users when
> calling pg_signal_backend with random PID numbers because of the fact
> that SIGNAL_BACKEND_NOAUTOVACUUM exists? Any role could guess which
> PIDs are used by an autovacuum worker because of the granularity
> required for the error related to pg_signal_autovacuum.

Hm. I hadn't considered that angle. IIUC right now they'll just get the
generic superuser error for autovacuum workers. I don't know how concerned
to be about users distinguishing autovacuum workers from other superuser
backends, _but_ if roles with pg_signal_autovacuum can't even figure out
the PIDs for the autovacuum workers, then this feature seems kind-of
useless. Perhaps we should allow roles with privileges of
pg_signal_autovacuum to see the autovacuum workers in pg_stat_activity.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dagfinn Ilmari Mannsåker 2024-04-05 12:57:04 Re: IPC::Run::time[r|out] vs our TAP tests
Previous Message Robert Haas 2024-04-05 12:55:41 Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs