Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)

Le lundi 27 septembre 2021, 20:15:05 CEST Mark Dilger a écrit :
> > On Sep 21, 2021, at 12:58 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> >
> > This patch set is failing to apply for me - it fails on patch 2.
>
> Thanks for looking!  I have pulled together a new patch set which applies
> cleanly against current master.
> > I haven't dug terribly deeply into it yet, but I notice that there is a
> > very large increase in test volume, which appears to account for much of
> > the 44635 lines of the patch set. I think we're probably going to want
> > to reduce that. We've had complaints in the past from prominent hackers
> > about adding too much volume to the regression tests.
>
> The v8 patch set is much smaller, with the reduction being in the size of
> regression tests covering which roles can perform SET, RESET, ALTER SYSTEM
> SET, and ALTER SYSTEM RESET and on which GUCs.  The v7 patch set did
> exhaustive testing on this, which is why it was so big.  The v8 set does
> just a sampling of GUCs per role.  The total number of lines for the patch
> set drops from 44635 to 13026, with only 1960 lines total between the .sql
> and .out tests of GUC privileges.
> > I do like the basic thrust of reducing the power of CREATEROLE. There's
> > an old legal maxim I learned in my distant youth that says "nemo dat
> > quod non habet" - Nobody can give something they don't own. This seems
> > to be in that spirit, and I approve :-)
>
> Great!  I'm glad to hear the approach has some support.
>
>
> Other changes in v8:
>
> Add a new test for subscriptions owned by non-superusers to verify that the
> tablesync and apply workers replicating their subscription won't replicate
> into schemas and tables that the subscription owner lacks privilege to
> touch.  The logic to prevent that existed in the v7 patch, but I overlooked
> adding tests for it.  Fixed.
>
> Allow non-superusers to create event triggers.  The logic already existed
> and is unchanged to handle event triggers owned by non-superusers and
> conditioning those triggers firing on the (trigger-owner,
> role-performing-event) pair.  The v7 patch set didn't go quite so far as
> allowing non-superusers to create event triggers, but that undercuts much
> of the benefit of the changes for no obvious purpose.
>
>
> Not changed in v8, but worth discussing:
>
> Non-superusers are still prohibited from creating subscriptions, despite
> improvements to the security around that circumstance.  Improvements to the
> security model around event triggers does not have to also include
> permission for non-superuser to create event triggers, but v8 does both.
> These could be viewed as inconsistent choices, but I struck the balance
> this way because roles creating event triggers does not entail them doing
> anything that they can't already do, whereas allowing arbitrary users to
> create subscriptions would entail an ordinary user causing external network
> connections being initiated.  We likely need to create another privileged
> role and require a non-superuser to be part of that role before they can
> create subscriptions.  That seems, however, like something to do as a
> follow-on patch, since tightening up the security on subscriptions as done
> in this patch doesn't depend on that.

The changes proposed around subscription management make a lot of sense,
especially considering that now that we don't allow to run ALTER SUBSCRIPTION
REFRESH in a function anymore, there is no way to delegate this to a non
superuser (using a security definer function). Since it doesn't involve the
rest of the patchset, patches 16, 17 and 18 could be separated in another
thread / patchset.

--
Ronan Dunklau