Re: Allow placeholders in ALTER ROLE w/o superuser

On Wed, Jul 20, 2022 at 11:50:10AM -0400, Tom Lane wrote:
> 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.

I was imagining that the permissions checks would apply at ALTER ROLE/DB
SET time, not at login time.  Otherwise, changing a role's privileges might
impact other roles' parameters, and it's not clear (at least to me) what
should happen when the role is dropped.  Another reason I imagined it this
way is because that's basically how it works today.  We assume that the
pg_db_role_setting entry was added by a superuser, but we don't check that
the user that ran ALTER ROLE/DB SET is still superuser every time you log
in.

> 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).

Yeah, this is why I suggested storing something that equates to PGC_SUSET
any time a role is superuser or has grantable GUC permissions.

> 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.

Couldn't ProcessGUCArray() use set_config_option_ext() with the context
indicated by pg_db_role_setting?  Also, instead of using PGC_USERSET in all
the set_config_option() call sites, shouldn't we remove the "context"
argument altogether?  I am likely misunderstanding your proposal, but while
I think simplifying set_config_option() is worthwhile, I don't see why it
would preclude storing the context in pg_db_role_setting.

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