Re: Allow placeholders in ALTER ROLE w/o superuser

Thanks a lot for the feedback Nathan.

Taking your options into consideration, for me the correct behaviour should
be:

- The ALTER ROLE placeholder should always be stored with a PGC_USERSET
GucContext. It's a placeholder anyway, so it should be the less restrictive
one. If the user wants to define it as PGC_SUSET or other this should be
done through a custom extension.
- When an extension claims the placeholder, we should check the
DefineCustomXXXVariable GucContext with PGC_USERSET. If there's a match,
then the value gets applied, otherwise WARN or ERR.
The role GUCs get applied at login time right? So at this point we can
WARN or ERR about the defined role GUCs.

What do you think?

On Mon, 18 Jul 2022 at 19:03, Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:

> On Fri, Jul 01, 2022 at 04:40:27PM -0700, Nathan Bossart wrote:
> > On Sun, Jun 05, 2022 at 11:20:38PM -0500, Steve Chavez wrote:
> >> However, defining placeholders at the role level require superuser:
> >>
> >> alter role myrole set my.username to 'tomas';
> >> ERROR: permission denied to set parameter "my.username"
> >>
> >> Which is inconsistent and surprising behavior. I think it doesn't make
> >> sense since you can already set them at the session or transaction
> >> level(SET LOCAL my.username = 'tomas'). Enabling this would allow
> sidecar
> >> services to store metadata scoped to its pertaining role.
> >>
> >> I've attached a patch that removes this restriction. From my testing,
> this
> >> doesn't affect permission checking when an extension defines its custom
> GUC
> >> variables.
> >>
> >> DefineCustomStringVariable("my.custom", NULL, NULL, &my_custom,
> NULL,
> >> PGC_SUSET, ..);
> >>
> >> Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
> >> when doing "make installcheck".
> >
> > IIUC you are basically proposing to revert a6dcd19 [0], but it is not
> clear
> > to me why that is safe. Am I missing something?
>
> I spent some more time looking into this, and I think I've constructed a
> simple example that demonstrates the problem with removing this
> restriction.
>
> postgres=# CREATE ROLE test CREATEROLE;
> CREATE ROLE
> postgres=# CREATE ROLE other LOGIN;
> CREATE ROLE
> postgres=# GRANT CREATE ON DATABASE postgres TO other;
> GRANT
> postgres=# SET ROLE test;
> SET
> postgres=> ALTER ROLE other SET plperl.on_plperl_init = 'test';
> ALTER ROLE
> postgres=> \c postgres other
> You are now connected to database "postgres" as user "other".
> postgres=> CREATE EXTENSION plperl;
> CREATE EXTENSION
> postgres=> SHOW plperl.on_plperl_init;
> plperl.on_plperl_init
> -----------------------
> test
> (1 row)
>
> In this example, the non-superuser role sets a placeholder GUC for another
> role. This GUC becomes a PGC_SUSET GUC when plperl is loaded, so a
> non-superuser role will have successfully set a PGC_SUSET GUC. If we had a
> record of who ran ALTER ROLE, we might be able to apply appropriate
> permissions checking when the module is loaded, but this information
> doesn't exist in pg_db_role_setting. IIUC we have the following options:
>
> 1. Store information about who ran ALTER ROLE. I think there are a
> couple of problems with this. For example, what happens if the
> grantor was dropped or its privileges were altered? Should we
> instead store the context of the user (i.e., PGC_USERSET or
> PGC_SUSET)? Do we need to add entries to pg_depend?
> 2. Ignore or ERROR for any ALTER ROLE settings for custom GUCs.
> Since
> we don't know who ran ALTER ROLE, we can't trust the value.
> 3. Require superuser to use ALTER ROLE for a placeholder. This is
> what
> we do today. Since we know a superuser set the value, we can
> always
> apply it when the custom GUC is finally defined.
>
> If this is an accurate representation of the options, it seems clear why
> the superuser restriction is in place.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>