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(at)snowman(dot)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 than leave them as traps for the unwary. The CREATEROLE attribute exists and is defined in a way that is broadly viewed as a 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 CREATEROLE 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 spec has a concept of ADMIN and I don't know that we can fix everything we want to fix and still be within compliance with 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 that role to/from other roles. That's it. If we start tying a bunch of other stuff to that, we're breaking reasonable expectations about how WITH ADMIN OPTION works, and since the spec defines how that works, we're then in violation of the spec.

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-vis the 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 sounds like a non-starter to me. We'd further want to tie other stuff, like event triggers, to ADMIN option. I don't see how this furthers spec compliance.

Tying this stuff to CREATEROLE seems perfectly fair. Nobody coming from another database vendor to postgres should have any 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 heavy use, so changing how that behaves is a huge backwards compatibility break. Event triggers owned by non-superusers are only a fluke, not an intentional feature, and only occur when a superuser creates an event trigger and later has superuser privileges revoked. We can expect that far fewer users are really depending on that to work compared with table triggers.
>>
>> 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'm not 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 is something really objectionable to having two separate databases in a cluster intended for two separate purposes and with two 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 share your 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-superuser if you want that administrator to be unable to mess with the roles used in database B. But what mechanism do we 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 discussion here.

>>> 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 to add a preamble to the trigger that exits early if the user, time of day, phase of the moon, etc., are inappropriate per the 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, unless it 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 cannot expand the net. It does seem a bit arbitrary to me, though, since you could also say FOR HOURS OF DAY 11PM through 3AM, and once you open the door to supporting all that in the syntax, and tracking it in the catalogs, you've opened a 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 facetious when I mention having syntax to support event triggers to fire only at certain times of day. Plenty of deployments I have encountered have exactly this type of restriction, limiting the time of day when certain sorts of actions can be performed. Similarly, I have seen deployments which have their heaviest activity around the Christmas shopping season. They might want event triggers that fire between Black Friday and Boxing Day that prevent schema changes during 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 create event triggers. The only thing that makes it *unreasonable* for them to do so is that an event trigger could block or alter actions performed by roles which the non-superuser trigger owner should not have the privilege to block or alter. So restrictions on when the event trigger fires to get around that problem seem on topic. Other filters, no matter how good the idea, are simply off topic. Go and implement them with my blessing. But I don't see why I should have to implement them as part of this patch set.

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