Re: pg_parameter_aclcheck() and trusted extensions

From: Nathan Bossart <nathandbossart(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_parameter_aclcheck() and trusted extensions
Date: 2022-07-14 22:57:35
Message-ID: 20220714225735.GB3173833@nathanxps13
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, Jul 14, 2022 at 06:03:45PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
>> On Thu, Jul 14, 2022 at 04:02:30PM -0400, Tom Lane wrote:
>>> I noted something that ought to be looked at separately:
>>> validate_option_array_item() seems like it needs to be taught about
>>> grantable permissions on GUCs. I think that right now it may report
>>> permissions failures in some cases where it should succeed.
>> Which cases do you think might be inappropriately reporting permissions
>> failures? It looked to me like this stuff was mostly used for
>> pg_db_role_setting, which wouldn't be impacted by the current set of
>> grantable GUC permissions. Is the idea that you should be able to do ALTER
>> ROLE SET for GUCs that you have SET permissions for?
> Well, that's what I'm wondering. Obviously that wouldn't *alone* be
> enough permissions, but it seems like it could be a component of it.
> Specifically, this bit:
> /* manual permissions check so we can avoid an error being thrown */
> if (gconf->context == PGC_USERSET)
> /* ok */ ;
> else if (gconf->context == PGC_SUSET && superuser())
> /* ok */ ;
> else if (skipIfNoPermissions)
> return false;
> seems like it's trying to duplicate what set_config_option would do,
> and it's now missing a component of that. If it shouldn't check
> per-GUC permissions along with superuser(), we should add a comment
> explaining why not.

I looked into this a bit closer. I found that having SET permissions on a
GUC seems to allow you to ALTER ROLE SET it to others.

postgres=# CREATE ROLE other;
postgres=# GRANT SET ON PARAMETER zero_damaged_pages TO test;
postgres=# SET ROLE test;
postgres=> ALTER ROLE other SET zero_damaged_pages = 'on';
postgres=> SELECT * FROM pg_db_role_setting;
setdatabase | setrole | setconfig
0 | 16385 | {zero_damaged_pages=on}
(1 row)

However, ALTER ROLE RESET ALL will be blocked, while resetting only the
individual GUC will go through.

postgres=> ALTER ROLE other RESET ALL;
postgres=> SELECT * FROM pg_db_role_setting;
setdatabase | setrole | setconfig
0 | 16385 | {zero_damaged_pages=on}
(1 row)

postgres=> ALTER ROLE other RESET zero_damaged_pages;
postgres=> SELECT * FROM pg_db_role_setting;
setdatabase | setrole | setconfig
(0 rows)

I think this is because GUCArrayReset() is the only caller of
validate_option_array_item() that sets skipIfNoPermissions to true. The
others fall through to set_config_option(), which does a
pg_parameter_aclcheck(). So, you are right.

Regarding whether SET privileges should be enough to allow ALTER ROLE SET,
I'm not sure I have an opinion yet. You would need WITH GRANT OPTION to be
able to grant SET to that role, but that's a bit different than altering
the setting for the role. You'll already have privileges to alter the role
(e.g., CREATEROLE), so requiring extra permissions to set GUCs on roles
feels like it might be excessive. But there might be a good argument for

Nathan Bossart
Amazon Web Services:

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2022-07-14 22:58:09 Re: doc: New cumulative stats subsystem obsoletes comment in maintenance.sgml
Previous Message Tom Lane 2022-07-14 22:57:29 Re: Move Section 9.27.7 (Data Object Management Functions) to System Information Chapter