Re: role self-revocation

Greetings,

On Fri, Mar 11, 2022 at 12:32 Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
wrote:

>
>
> > On Mar 11, 2022, at 8:48 AM, Stephen Frost <sfrost(at)snowman(dot)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(at)snowman(dot)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(at)snowman(dot)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

>