Re: role self-revocation - Mailing list pgsql-hackers

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Mar 10, 2022 at 2:58 PM Stephen Frost <sfrost@snowman.net> wrote:
> > It'd be useful to have a better definition of exactly what a
> > 'mini-superuser' is, but at least for the moment when it comes to roles,
> > let's look at what the spec says:
>
> Gosh, I feel like I've spelled that out approximately 463,121 times
> already. That estimate might be slightly off though; I've been known
> to make mistakes from time to time....

If there's a specific message that details it closely on the lists
somewhere, I'm happy to go review it.  I admit that I didn't go back and
look for such.

> > CREATE ROLE
> >   - Who is allowed to run CREATE ROLE is implementation-defined
> >   - After creation, this is effictively run:
> >     GRANT new_role TO creator_role WITH ADMIN, GRANTOR "_SYSTEM"
> >
> > DROP ROLE
> >   - Any user who has been GRANT'd a role with ADMIN option is able to
> >     DROP that role.
> >
> > GRANT ROLE
> >   - No cycles allowed
> >   - A role must have ADMIN rights on the role to be able to GRANT it to
> >     another role.
> >
> > ALTER ROLE
> >   - Doesn't exist
> >
> > This actually looks to me like more-or-less what you're looking for, it
> > just isn't what we have today because CREATEROLE brings along with it a
> > bunch of other stuff, some of which we want and some that we don't, and
> > some things that the SQL spec says ADMIN should be allowed to do (DROP
> > ROLE) we don't allow today.
>
> The above is mostly fine with me, except for the part about ALTER ROLE
> not existing. I think it's always good to be able to change your mind
> post-CREATE.

Errr, just to be clear, ALTER ROLE doesn't exist *in the spec*.  I
wasn't suggesting that we get rid of it, just that it doesn't exist in
the spec and therefore the spec doesn't have anything to say about it.

> Basically, in this sketch, ADMIN OPTION on a role involves the ability
> to DROP it, which means we don't need a separate role owner concept.

Right.  The above doesn't include any specifics about what to do with
ALTER ROLE, but my thought would be to have it also be under ADMIN
OPTION rather than under CREATEROLE, as I tried to outline (though not
very well, I'll admit) below.

> It also involves membership, meaning that you can freely exercise the
> privileges of the role without SET ROLE. While I'm totally down with
> having other possible behaviors as options, that particular behavior
> seems very useful to me, so, sounds great.

Well, yes and no- by default you're right, presuming everything is set
as inheirited, but I'd wish for us to keep the option of creating roles
which are noinherit and having that work just as it does today.

> > It's also not quite what I want because it requires that membership and
> > ADMIN go together where I'd like to be able to have those be
> > independently GRANT'able- and then some.
> >
> > I don't think we're that far from having all of these though.  To start
> > with, we remove from CREATEROLE the random things that it does which go
> > beyond what folks tend to expect- remove the whole 'grant any role to
> > any other' stuff, remove the 'drop role' exception, remove the
> > 'alter role' stuff.  Do make it so that when you create a role, however,
> > the above GRANT is effectively done.  Now, for the items above where we
> > removed the checks against have_createrole_privilege() we go back and
> > add in checks using is_admin_of_role().  Of course, also remove the role
> > self-administration bug.
>
> What do you mean by the 'drop role' exception?

'ability' was probably a better word there.  What I'm talking about is
changing in DropRole:

    if (!have_createrole_privilege())
        ereport(ERROR,
                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                 errmsg("permission denied to drop role")));

to be, more or less:

    if (!is_admin_of_role(role))
        ereport(ERROR,
                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                 errmsg("permission denied to drop role")));

> I don't like removing 'alter role'.

Ditto above but for AlterRole.  Taking it away from users with
CREATEROLE being able to run those commands on $anyrole and instead
making it so that the role running DROP ROLE or ALTER ROLE needs to have
ADMIN on the role they're messing with.  I do think we may also need to
make some adjustments in terms of what a regular user WITH ADMIN on a
given role is able to do when it comes to ALTER ROLE, in particular, I
don't think we'll want to remove the existing is-superuser checks
against a user settings bypassrls or replication or superuser on some
other role.  Maybe we can provide a way for a non-superuser to be given
the ability to set those attributes for roles they create, but that
would be a separate thing.

> The rest sounds good.

Great.

> > That's step #1, but it gets us more-or-less what you're looking for, I
> > think, and brings us a lot closer to what the spec has.
>
> Great.

Awesome.

> > Step #2 is also in-line with the spec: track GRANTORs and care about
> > them, for everything.  We really should have been doing this all along.
> > Note that I'm not saying that an owner of a table can't REVOKE some
> > right that was GRANT'd on that table, but rather that a user who was
> > GRANT'd ADMIN rights on a table and then GRANT'd that right to some
> > other user shouldn't have some other user who only has ADMIN rights on
> > the table be able to remove that GRANT.  Same goes for roles, meaning
> > that you could GRANT rights in a role with ADMIN option and not have to
> > be afraid that the role you just gave that to will be able to remove
> > *your* ADMIN rights on that role.  In general, I don't think this
> > would actually have a very large impact on users because most users
> > don't, today, use the ADMIN option much.
>
> There are details to work out here, but in general, I like it.

Cool.  Note that superusers would still be able to do $anything,
including removing someone's ADMIN rights on a role even if that
superuser didn't GRANT it (at least, that's my thinking on this).

> > Step #3 starts going in the direction of what I'd like to see, which
> > would be to break out membership in a role as a separate thing from
> > admin rights on that role.  This is also what would help with the 'bot'
> > use-case that Joshua (not David Steele, btw) brought up.
>
> Woops, apologies for getting the name wrong. I also said Marc earlier
> when I meant Mark, because I work with people named Mark, Marc, and
> Marc, and Mark's spelling got outvoted by some distant corner of my
> brain.

Hah, no worries.

> I think this is a fine long-term direction, with the caveat that
> you've not provided enough specifics here for me to really understand
> how it would work. I fear the specifics might be hard to get right,
> both in terms of making it understandable to users and in terms of
> preserving as much backward-compatibility as we can. However, I am not
> opposed to the concept.

We can perhaps debate the specifics around this later.

> > Step #4 then breaks the 'admin' option on roles into pieces- a 'drop
> > role' right, a 'reset password' right, maybe separate rights for
> > different role attributes, etc.  We would likely still keep the
> > 'admin_option' column in pg_auth_members and just check that first
> > and then check the individual rights (similar to table-level vs.
> > column-level privileges) so that we stay in line with the spec's
> > expectation here and with what users are used to.
>
> Same comments as #3, plus I wonder whether it really makes sense to
> separate #3 and #4. But we can decide that when there's a fleshed-out
> design for this.

Ditto.  I don't know that they need to be independent either.

> > In some hyptothetical world, there's even a later step #5 which allows
> > us to define user profiles and then grant the ability for a user to
> > create a role with a certain profile (but not any arbitrary profile),
> > thus making things like the 'bot' even more constrained in terms of
> > what it's able to do (maybe it can then create a role that's a member of
> > a role without itself being a member of that role or explicitly having
> > admin rights in that role, as an example).
>
> Right. I don't object to this either, hypothetically, but I think
> we're a long way from understanding how to get there, and I don't want
> step #1 to get blocked behind all the rest of this. Particularly the
> part where we remove the role self-administration thing.

Sure.

Thanks,

Stephen