Re: role self-revocation - Mailing list pgsql-hackers
| From | Stephen Frost |
|---|---|
| Subject | Re: role self-revocation |
| Date | |
| Msg-id | CAOuzzgqQW6cdb6L1N8kk=XLXEMh4z-d4yPU_ni+ESRxJzwewNg@mail.gmail.com Whole thread Raw |
| In response to | Re: role self-revocation (Mark Dilger <mark.dilger@enterprisedb.com>) |
| Responses |
Re: role self-revocation
(Mark Dilger <mark.dilger@enterprisedb.com>)
|
| List | pgsql-hackers |
Greetings,
On Fri, Mar 11, 2022 at 12:32 Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> On Mar 11, 2022, at 8:48 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> I agree that it would have an impact on backwards compatibility to
> change how WITH ADMIN works- but it would also get us more in line with
> what the SQL standard says for how WITH ADMIN is supposed to work and
> that seems worth the change to me.
I'm fine with giving up some backwards compatibility to get some SQL standard compatibility, as long as we're clear that is what we're doing. What you say about the SQL spec isn't great, though, because too much power is vested in "ADMIN". I see "ADMIN" as at least three separate privileges together. Maybe it would be spec compliant to implement "ADMIN" as a synonym for a set of separate privileges?
I do think that’s reasonable … and believe I suggested it about 3 messages ago in this thread. ;) (step #3 I think it was? Or maybe 4).
> On Mar 11, 2022, at 8:41 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> That we aren't discussing the issues with the current GRANT ... WITH
> ADMIN OPTION and how we deviate from what the spec calls for when it
> comes to DROP ROLE, which seems to be the largest thing that's
> 'solved' with this ownership concept, is concerning to me.
Sure, let's discuss that a bit more. Here is my best interpretation of your post about the spec, when applied to postgres with an eye towards not doing any more damage than necessary:
> On Mar 10, 2022, at 11:58 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> let's look at what the spec says:
>
> CREATE ROLE
> - Who is allowed to run CREATE ROLE is implementation-defined
This should be anyone with membership in pg_create_role.
That could be the case if we wished to go that route. I’d think in such case we’d then also remove CREATEROLE as otherwise the documentation feels like it’d be quite confusing.
> - After creation, this is effictively run:
> GRANT new_role TO creator_role WITH ADMIN, GRANTOR "_SYSTEM"
This should internally be implemented as three separate privileges, one which means you can grant the role, another which means you can drop the role, and a third that means you're a member of the role. That way, they can be independently granted and revoked. We could make "WITH ADMIN" a short-hand for "WITH G, D, M" where G, D, and M are whatever we name the independent privileges Grant, Drop, and Member-of.
I mean, sure, we can get there, and possibly add more like if you’re allowed to change or reset that role’s password and other things, but I don’t see that this piece is required as part of the very first change in this area. Further, WITH ADMIN already gives grant and member today, so you’re saying the only thing this change does that makes “WITH ADMIN” too powerful is adding DROP to it, yet that’s explicitly what the spec calls for. In short, I disagree that moving the DROP ROLE right from CREATEROLE roles having that across the entire system (excluding superusers) to WITH ADMIN where the role who has that right can:
A) already become that role and drop all their objects
B) already GRANT that role to some other role
is a big issue.
Splitting G and D helps with backwards compatibility, because it gives people who want the traditional postgres "admin" a way to get there, by granting "G+M". Splitting M from G and D makes it simpler to implement the "bot" idea, since the bot shouldn't have M. But it does raise a question about always granting G+D+M to the creator, since the bot is the creator and we don't want the bot to have M. This isn't a problem I've invented from thin air, mind you, as G+D+M is just the definition of ADMIN per the SQL spec, if I've understood you correctly. So we need to think a bit more about the pg_create_role built-in role and whether that needs to be further refined to distinguish those who can get membership in roles they create vs. those who cannot. This line of reasoning takes me in the direction of what I think you were calling #5 upthread, but you'd have to elaborate on that, and how it interacts with the spec, for us to have a useful conversation about it.
All that said, as I said before, I’m in favor of splitting things up and so if you want to do that as part of this initial work, sure. Idk that it’s absolutely required as part of this but I’m not going to complain if it’s included either. I agree that would allow folks to get something similar to what they could get today if they want.
I agree that the split up helps with the “bot” idea, as we could at least then create a security definer function that the bot runs and which creates roles that the bot then has G for but not M or D. Even better would be to also provide a way for the “bot” to be able to create roles without the need for a security definer function where it doesn’t automatically get all three, and that was indeed what I was thinking about with the template idea. The general thought there being that an admin could define a template along the lines of:
CREATE TEMPLATE employee_template
CREATOR WITH ADMIN, NOMEMBERSHIP
ROLE IN employee;
And then provide a way for the bot to be given the right to use this template. Thinking on it a bit further, I’m guessing that we wouldn’t actually give the bot pg_create_role in this case and instead would leave that to mean “able to create arbitrary roles and have all privs in that” similar to what we are talking about where ADMIN implies the full set of rights.
> DROP ROLE
> - Any user who has been GRANT'd a role with ADMIN option is able to
> DROP that role.
Change this to "Any role who has D on the role". That's spec compliant, because anyone granted ADMIN necessarily has D.
Yeah.
> GRANT ROLE
> - No cycles allowed
> - A role must have ADMIN rights on the role to be able to GRANT it to
> another role.
Change this to "Any role who has G on the role". That's spec compliant, because anyone grant ADMIN necessarily has G.
Sure.
We should also fix the CREATE ROLE command to require the grantor have G on a role in order to give it to the new role as part of the command.
… or just get rid of it, which seems saner to me.
Changing the CREATEROLE, CREATEDB, REPLICATION, and BYPASSRLS attributes into pg_create_role, pg_create_db, pg_replication, and pg_bypassrls, the creator could only give them to the created role if the creator has G on the roles. If we do this, we could keep the historical privilege bits and their syntax support for backward compatibility, or we could rip them out, but the decision between those two options is independent of the rest of the design.
Yeah, turning those into predefined roles which an admin can then decide to give out (and to allow ADMIN on them to be given to folks who could then pass that along if they wanted) is another thought I’ve had though one that’s somewhat independent of the rest of this, but also shows how we could make those be things that a superuser could choose to give out, or not, to some set of roles who would then be able to create roles of their own with those privileges.
On the whole, using predefined roles as the source of certain capabilities, and the options discussed here which would allow an admin to grant those capabilities out with or without the ability to grant them further, plus the splitting out of the individual role-relationship rights (membership, grantable, drop, etc) strikes me as being quite flexible and extendable and generally in the direction that we’ve been trending and which seems to be reasonably successful so far.
Thanks,
Stephen
pgsql-hackers by date: