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

> On Nov 1, 2021, at 2:00 PM, Stephen Frost <sfrost@snowman.net> wrote:

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

I am neutral on this.  I prefer not to break backward compatibility, but I also prefer to fix broken features rather
thanleave them as traps for the unwary.  The CREATEROLE attribute exists and is defined in a way that is broadly viewed
asa misfeature.  Fixing it has long term benefits, but short term compatibility concerns. 

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

I don't see an additional benefit beyond preserving compatibility with how CREATEROLE has historically worked.

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

I'm not seeing that.  If you create a role "role_admin" and give it CREATEROLE and other stuff, maybe CREATEDB,
pg_read_all_data,and so forth, then you grant "stephen" membership in "role_admin", doesn't that work?  Why would
"role_admin"being a member of some new role, say "pg_can_create_roles", work differently than "role_admin" having the
CREATEROLEattribute? 

>>> 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
thespec has a concept of ADMIN and I don't know that we can fix everything we want to fix and still be within
compliancewith the 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.

The WITH ADMIN OPTION feature has a really well defined meaning.  If you have ADMIN on a role, you can grant and revoke
thatrole to/from other roles.  That's it.  If we start tying a bunch of other stuff to that, we're breaking reasonable
expectationsabout how WITH ADMIN OPTION works, and since the spec defines how that works, we're then in violation of
thespec. 

CREATEROLE, on the other hand, has no defined meaning in the spec.  It's a postgres invention.  So if we change what it
means,we're not breaking compability with the spec, only backward compatibility with older version of postgres
vis-a-visthe CREATEROLE misfeature that most people presumably don't use.  I find that far preferable to breaking spec
compliance. It is strange to me that you view changing how WITH ADMIN OPTION functions as being motivated by spec
compliance,since I see it as going in the opposite direction. 

As you say above, we'd have to tie the ability to create, alter, and drop roles to the ADMIN option.  That already
soundslike a non-starter to me.  We'd further want to tie other stuff, like event triggers, to ADMIN option.  I don't
seehow this furthers spec compliance. 

Tying this stuff to CREATEROLE seems perfectly fair.  Nobody coming from another database vendor to postgres should
haveany spec-compatibility-based expectations about how CREATEROLE works. 

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

You're pushing at an open door.  If the community doesn't object to fixing the security problems with table triggers,
I'mnot going to object either. 

>> If roles were not cluster-wide, I might not have such a problem with leaving things mostly as they are.  But there
issomething really objectionable to having two separate databases in a cluster intended for two separate purposes and
withtwo separate 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..?).

The problem is that superusers can act in any database, so role administration in database A must be done by a
non-superuserif you want that administrator to be unable to mess with the roles used in database B.  But what mechanism
dowe have for that?  WITH ADMIN OPTION is too narrow to do it, and I've already argued why I don't want to expand that
power,and CREATEROLE as currently implemented is too broad. 

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

Yes, I took some interest in that conversation.  Like you, I'm not sure I see how it fixes the problems under
discussionhere. 

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

I don't care much about this.  You can implement that if you want, and I'm not going to have a reason to complain,
unlessit somehow allows people to cast too wide a net.  Narrowing the net is, to my mind, entirely orthogonal to this
discussion.

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

I understand why you want this, but I don't understand why you think it is tied to a security patch.  I'm not being
facetiouswhen I mention having syntax to support event triggers to fire only at certain times of day.  Plenty of
deploymentsI have encountered have exactly this type of restriction, limiting the time of day when certain sorts of
actionscan be performed.  Similarly, I have seen deployments which have their heaviest activity around the Christmas
shoppingseason.  They might want event triggers that fire between Black Friday and Boxing Day that prevent schema
changesduring such heavy load, but not the rest of the year.  And they might want them to fire for some roles and not
others.

The idea in the event trigger patch is to make it reasonable, from a security standpoint, to allow non-superusers to
createevent triggers.  The only thing that makes it *unreasonable* for them to do so is that an event trigger could
blockor alter actions performed by roles which the non-superuser trigger owner should not have the privilege to block
oralter.  So restrictions on when the event trigger fires to get around that problem seem on topic. Other filters, no
matterhow good the idea, are simply off topic.  Go and implement them with my blessing.  But I don't see why I should
haveto implement them as part of this patch set. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company