Re: Allow placeholders in ALTER ROLE w/o superuser

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 19 Jul 2022 09:53:39 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
>> Hm.  I would expect ALTER ROLE to store the PGC_SUSET context when executed
>> by a superuser or a role with privileges via pg_parameter_acl.  Storing all
>> placeholder GUC settings as PGC_USERSET would make things more restrictive
>> than they are today.  For example, it would no longer be possible to apply
>> any ALTER ROLE settings from superusers for placeholders that later become
>> custom GUCS.

> Returning to the topic, that operation can be allowed in PG15, having
> being granted by superuser using the GRANT SET ON PARMETER command.

I think that 13d838815 has completely changed the terms that this
discussion needs to be conducted under.  It seems clear to me now
that if you want to relax this only-superusers restriction, what
you have to do is store the OID of the role that issued ALTER ROLE/DB SET,
and then apply the same checks that would be used in the ordinary case
where a placeholder is being filled in after being set intra-session.
That is, we'd no longer assume that a value coming from pg_db_role_setting
was set with superuser privileges, but we'd know exactly who did set it.

This might also tie into Nathan's question in another thread about
exactly what permissions should be required to issue ALTER ROLE/DB SET.
In particular I'm wondering if different permissions should be needed to
override an existing entry than if there is no existing entry.  If not,
we could find ourselves downgrading a superuser-set entry to a
non-superuser-set entry, which might have bad consequences later
(eg, by rendering the entry nonfunctional because when we actually
load the extension we find out the GUC is SUSET).

Possibly related to this: I felt while working on 13d838815 that
PGC_SUSET and PGC_SU_BACKEND should be usable as GucContext
values for GUC variables, indicating that the GUC requires special
privileges to be set, but we should no longer use them as passed-in
GucContext values.  That is, we should remove privilege tests from
the call sites, like this:

            (void) set_config_option(stmt->name,
                                     ExtractSetVariableArgs(stmt),
-                                    (superuser() ? PGC_SUSET : PGC_USERSET),
+                                    PGC_USERSET,
                                     PGC_S_SESSION,
                                     action, true, 0, false);

and instead put that behavior inside set_config_option_ext, which
would want to look at superuser_arg(srole) instead, and indeed might
not need to do anything because pg_parameter_aclcheck would subsume
the test.  I didn't pursue this further because it wasn't essential
to fixing the bug.  But it seems relevant here, because that line of
thought leads to the conclusion that storing PGC_SUSET vs PGC_USERSET
is entirely the wrong approach.

There is a bunch of infrastructure work that has to be done if anyone
wants to make this happen:

* redesign physical representation of pg_db_role_setting

* be sure to clean up if a role mentioned in pg_db_role_setting is dropped

* pg_dump would need to be taught to dump the state of affairs correctly.

            regards, tom lane