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

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Nov 1, 2021, at 12:44 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > I can generally get behind the idea that a user who has been allowed to
> > create other roles should be able to do various other things with that
> > role, but should also be limited by what rights they themselves have
> > (unlike how CREATEROLE works today).
>
> I intend to rearrange the role ownership patch set to have the 0004-Restrict-power-granted-via-CREATEROLE.patch come
before,and be independent of, the patches that introduce role ownership.  That would put the less controversial patch
first,and might get committed what we all agree. 

I've not directly looked at that patch, but I like it based on the name,
if we think we can actually get folks to agree to is as it's quite a
change from the current situation.  Previously I've felt that we
wouldn't have support for breaking backwards compatibility in such a
manner but perhaps I'm wrong on that.  There's also something to be
said, in my view anyway, of having a predefined role be used for what we
want CREATEROLE to be rather than changing the existing CREATEROLE role
attribute.  Reason being that such a predefined role could then be
GRANT'd to some other role along with whatever other generally-relevant
privileges there are and then that GRANT'd to whomever should have those
rights.  That's not really possible with the current CREATEROLE role
attribute.

> > That said, I have a hard time seeing why we're drawing this distinction
> > of 'ownership' as being ultimately different from simple 'admin' rights
> > on a role.  In other words, beyond the ability to actually create/drop
> > roles, having 'admin' rights on a role already conveys just about
> > everything 'ownership' would.  The things that are getting in the way
> > there are:
> >
> > - Ability to actually create/alter/drop roles, this needs to be
> >   addressed somehow but doesn't necessarily imply a need for
> >   'ownership' as a concept.
> >
> > - Restriction of a role from being able to implicitly have 'admin'
> >   rights on itself, as I started a discussion about elsewhere.
> >
> > - Some system for deciding who event triggers should fire for.  I don't
> >   think this should really be tied into the question of who has admin
> >   rights on whom except to the extent that maybe you can only cause
> >   event triggers to fire for roles you've got admin rights on (or maybe
> >   membership in).
>
> You and I are not that far apart on this issue.  The reason I wanted to use "ownership" rather than ADMIN is that the
spechas a concept of ADMIN and I don't know that we can fix everything we want to fix and still be within compliance
withthe spec. 

There's no concept in the spec of event triggers, I don't believe
anyway, so I'm not really buying this particular argument.  Seems like
we'd be more likely to run afoul of some future spec by creating a
concept of role ownership and creating a definition around what that
means than using something existing in the spec as controlling what some
other not-in-spec thing does.

> > One thing that comes to mind is that event triggers aren't the only
> > thing out there and I have to wonder if we should be thinking about
> > other things.  As a thought exercise- how is an event trigger really
> > different from a table-level trigger?  Anyone who has the ability to
> > create objects is able to create tables, create functions, create
> > operators, and a user logging in and running SQL can certainly end up
> > running those things with their privileges.
>
> The difference in my mind is that table triggers owned by non-superusers have been around for a long time and are in
heavyuse, so changing how that behaves is a huge backwards compatibility break.  Event triggers owned by non-superusers
areonly a fluke, not an intentional feature, and only occur when a superuser creates an event trigger and later has
superuserprivileges revoked.  We can expect that far fewer users are really depending on that to work compared with
tabletriggers. 
>
> In a green field, I would not create table triggers to work as they do.

I don't think we're entirely beholden to having table-level triggers
work the way they do today.  I agree that we can't simply stop having
them fire for some users while letting things continue to happen on the
table but throwing an error and rolling back a transaction with an error
saying "you were about to run this trigger which runs this function with
your privileges and you don't trust the person who wrote it" seems
entirely within reason, were we to have such a concept.

> >  We've generally recognized
> > that that's not great and there's been work to get it so that the
> > 'public' schema that everyone has in their search_path by default won't
> > be world-writable but that isn't exactly a cure-all for the general
> > problem.
>
> I agree.
>
> > One of the interesting bits is that there's two sides to this.  On the
> > one hand, as a user, maybe I don't want to run functions of people who I
> > don't trust.  As an admin/superuser/landlord, maybe I want to require
> > everyone who I have authority over to run these functions/event
> > triggers.  I'm not sure that we can find a solution to everything with
> > this but figure I'd share these thoughts.
>
> If roles were not cluster-wide, I might not have such a problem with leaving things mostly as they are.  But there is
somethingreally objectionable to having two separate databases in a cluster intended for two separate purposes and with
twoseparate sets of roles, and the set of roles in one database can mess with the roles intended for the other
database. I think some kind of partitioning is needed, and I saw role ownership as the cleanest solution to it.  I
shareyour intuitions that perhaps the WITH ADMIN OPTION stuff could be used instead, but I don't see quite how. 

I agree that roles existing cluster-level is an issue in some instances
though I'm not quite sure what the concern here is (how could a role in
database A mess with roles in database B unless the first role had some
kind of access on those roles, in which case, what's the issue..?).

Another thread/patch under discussion is around role membership being
made to be able to be per-database, which could be pretty interesting,
though I don't think it directly helps with what you're suggesting
above, unfortunately.

> > Last thought I'll share is that I do believe we're going to want to
> > provide flexibility when it comes to defining who event triggers run
> > for, as a given admin may wish for that set to be different from the set
> > of roles that they ultimately have control over.  I dislike tying these
> > two things together at such a core level and therefore continue to feel
> > that CREATE EVENT TRIGGER should be extended in some fashion to allow
> > individuals who can create them to specify who they are to run for.
>
> Within reason, sure.  It is fine by me if we support CREATE EVENT TRIGGER...AUTHORIZATION... in order to accomplish
that. But the role running that command still needs to be limited to just a (proper or otherwise) subset of their own
privileges.
>
> I thought about this some when originally writing the event trigger patch.  The author of the event trigger is free
toadd a preamble to the trigger that exits early if the user, time of day, phase of the moon, etc., are inappropriate
perthe reasoning of the trigger author.  We only need the system to prevent the event trigger from casting too wide a
net. The event trigger author can limit the scope of the net further if desired. 

I don't know that all such event triggers will necessarily be able to be
modified by everyone who will want to use them in the way you're
suggesting.  Consider that there's things which require the event
trigger to be a C function as a simple example.

> > Open to different ideas as to how a user could express that, but it
> > feels to me like that should be a core part of the definition of a
> > user-defined event trigger (ie: could be "FOR ALL ROLES I OWN" or
> > whatever, and maybe that's the default, but having that be the only
> > option isn't appealing).
>
> I am not strongly against adding syntactic support for FOR ALL ROLES vs. FOR role, role, ..., so long as that syntax
cannotexpand the net.  It does seem a bit arbitrary to me, though, since you could also say FOR HOURS OF DAY 11PM
through3AM, and once you open the door to supporting all that in the syntax, and tracking it in the catalogs, you've
openeda can of worms. 

I disagree that it's a "can of worms" that one would be opening.  Sure,
folks can ask for all kinds of things and that's true today, but
ultimately we're the arbitrators of what is a sensible and common enough
use-case and what's not.  We seem to be in pretty clear agreement that
it's a sensible and reasonably common use-case for an event trigger
definer to wish for it to only be run for some subset of individuals and
that subset might not always be the exact subset of individuals that a
given role has 'ownership' or 'admin' rights over.  Your approach puts
the onus of limiting that on the trigger author, who might not even be
involved if it's some function that's provided from an extension and
written in C and distributed in a packaged form from PGDG.  There's also
no way to tie together privileges between who is allowed to do some
action and the individuals who the event trigger fires for, which seems
unfortuante to me.

Thanks,

Stephen