Re: Allow placeholders in ALTER ROLE w/o superuser

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