Greetings, * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Nov 1, 2021, at 12:44 PM, Stephen Frost <sfr...@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 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. > > 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. > > 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 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..?). 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 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. > > 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. Thanks, Stephen
signature.asc
Description: PGP signature
