Re: CREATEROLE and role ownership hierarchies

On Mon, Jan 31, 2022 at 1:50 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > > On Jan 31, 2022, at 8:53 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > > Yeah, we do need to have a way to determine who is allowed to drop
> > > roles and role ownership seems like it's one possible approach to that.
> >
> > Which other ways are on the table?  Having ADMIN on a role doesn't allow you to do that, but maybe it could?  What
else?
>
> Supporting that through ADMIN is one option, another would be a
> 'DROPROLE' attribute, though we'd want a way to curtail that from being
> able to be used for just any role and that does lead down a path similar
> to ownership or just generally the concept that some roles have certain
> rights over certain other roles (whether you create them or not...).
>
> I do think there's a lot of value in being able to segregate certain
> rights- consider that you may want a role that's able to create other
> roles, perhaps grant them into some set of roles, can lock those roles
> (prevent them from logging in, maybe do a password reset, something like
> that), but which *isn't* able to drop those roles (and all their
> objects) as that's dangerous and mistakes can certainly happen, or be
> able to become that role because the creating role simply doesn't have
> any need to be able to do that (or desire to in many cases, as we
> discussed in the landlord-vs-tenant sub-thread).
>

This is precisely the use case I am trying to accomplish with this
patchset, roughly:

- An automated bot that creates users and adds them to the employees role
- Bot cannot access any employee (or other roles) table data
- Bot cannot become any employee
- Bot can disable the login of any employee

Yes there are attack surfaces around the fringes of login, etc but
those can be mitigated with certificate authentication. My pg_hba
would require any role in the employees role to use cert auth.

This would adequately mitigate many threats while greatly enhancing
user management.

> Naturally, you'd want *some* role to be able to drop that role (and one
> that doesn't have full superuser access) but that might be a role that's
> not able to create new roles or take over accounts.

I suspect some kind of web backend to handle manual user pruning. I
don't expect Bot to automatically drop users because mistakes can
happen, and disabling the login ability seems like an adequate
tradeoff.

> Separation of concerns and powers and all of that is what we want to be
> going for here, more generically, which is why I was opposed to the
> blanket "owners have all rights of all roles they own" implementation.
> That approach doesn't support the ability to have a relatively
> unprivileged role that's able to create other roles, which seems like a
> pretty important use-case for us to be considering.

Agreed.

> The terminology seems to also be driving us in a certain direction and I
> don't know that it's necessarily a good one.  That is- the term 'owner'
> implies certain things and maybe that's where some of the objection to
> my argument that owners shouldn't necessarily have all rights of the
> roles they 'own' comes from (ok- I'll also put out there for general
> consideration that since we're talking about roles, and login roles are
> generally associated with people, that maybe 'owner' isn't a great term
> to use for this anyway ...).  I feel like the 'owner' concept came from
> the way we have table owners and function owners and database owners
> today rather than from a starting point of what do we wish to
> specifically enable.
>
> Perhaps instead of starting from the 'owner' concept, we start from the
> question about the kinds of things we want roles to be able to do and
> perhaps that will help inform the terminology.
>
> - Create new roles
> - Drop an existing role
> - Drop objects which belong to a role
> - Lock existing roles
> - Change/reset the PW of existing roles
> - Give roles to other roles
> - Revoke access to some roles from other roles
> - Give select role attributes to a role
> - Revoke role attributes from a role
> - Traditional role-based access control (group memberships, SET ROLE)
>
> Certain of the above are already covered by the existing role membership
> system and with the admin option, though there's definitely an argument
> to be made as to if that is as capable as we'd like it to be (there's no
> way to, today at least, GRANT *just* the admin option, for example, and
> maybe that's something that it would actually be sensible to support).
>
> Perhaps there is a need to have a user who has all of the above
> capabilities and maybe that would be an 'owner' or 'manager', but as I
> tried to illustrate above, there's definitely use-cases for giving
> a role only some of the above capabilities rather than all of them
> together at once.
>
> > >> The main idea here is that having CREATEROLE doesn't give you ADMIN on roles, nor on role attributes.  For role
attributes,the syntax has been extended.  An excerpt from the patch's regression test illustrates some of that concept: 
> > >>
> > >> -- ok, superuser can create a role that can create login replication users, but
> > >> -- cannot itself login, nor perform replication
> > >> CREATE ROLE regress_role_repladmin
> > >>    CREATEROLE WITHOUT ADMIN OPTION     -- can create roles, but cannot give it away
> > >>    NOCREATEDB WITHOUT ADMIN OPTION     -- cannot create db, nor give it away
> > >>    NOLOGIN WITH ADMIN OPTION           -- cannot log in, but can give it away
> > >>    NOREPLICATION WITH ADMIN OPTION     -- cannot replicate, but can give it away
> > >>    NOBYPASSRLS WITHOUT ADMIN OPTION;   -- cannot bypassrls, nor give it away
> > >>
> > >> -- ok, superuser can create a role with CREATEROLE but restrict give-aways
> > >> CREATE ROLE regress_role_minoradmin
> > >>    NOSUPERUSER                         -- WITHOUT ADMIN OPTION is implied
> > >>    CREATEROLE WITHOUT ADMIN OPTION
> > >>    NOCREATEDB WITHOUT ADMIN OPTION
> > >>    NOLOGIN WITHOUT ADMIN OPTION
> > >>    NOREPLICATION                       -- WITHOUT ADMIN OPTION is implied
> > >>    NOBYPASSRLS                         -- WITHOUT ADMIN OPTION is implied
> > >>    NOINHERIT WITHOUT ADMIN OPTION
> > >>    CONNECTION LIMIT NONE WITHOUT ADMIN OPTION
> > >>    VALID ALWAYS WITHOUT ADMIN OPTION
> > >>    PASSWORD NULL WITHOUT ADMIN OPTION;
> > >
> > > Right, this was one of the approaches that I was thinking could work for
> > > managing role attributes and it's very similar to roles and the admin
> > > option for them.  As I suggested at least once, another possible
> > > approach could be to have login users not be able to create roles but
> > > for them to be able to SET ROLE to a role which is able to create roles,
> > > and then, using your prior method, only allow the attributes which that
> > > role has to be able to be given to other roles.
> >
> > I'm not sure how that works.  If I have a group named "administrators" which as multiple attributes like BYPASSRLS
andsuch, and user "stephen" is a member of "administrators", then stephen can not only give away bypassrls to new users
butalso has it himself.  How is that an improvement?  (I mean this as a question, not as criticism.) 
>
> That's not how role attributes work though- "stephen" only has the
> 'bypassrls' role attribute after a 'set role administrators'.  This has
> been one of the issues with role attributes in general as there's no way
> to change that (unlike the 'inherit' option for roles themselves) but in
> this particular case it might be to our advantage.
>
> > >  That essentially makes
> > > a role be a proxy for the per-attribute admin options.  There's pros and
> > > cons for each approach and so I'm curious as to which you feel is the
> > > better approach?  I get the feeling that you're more inclined to go with
> > > the approach of having an admin option for each role attribute (having
> > > written this WIP patch) but I'm not sure if that is because you
> > > contempltaed both and felt this was better for some reason or more
> > > because I wasn't explaining the other approach very well, or if there
> > > was some other reason.
> >
> > I need more explanation of the other option you are contemplating.  My apologies if I'm being thick-headed.
>
> Hopefully the above helps.  Note that in order to not allow the
> "stephen" role simply alter itself to have the bypassrls role attribute,
> we'd need to consider roles to not have 'ownership' (or whatever) over
> themselves, which leads into the prior complaint I made around roles
> having 'admin' rights on themselves which I generally don't feel is
> correct either.
>
> > >> -- fail, having CREATEROLE is not enough to create roles in privileged roles
> > >> SET SESSION AUTHORIZATION regress_role_minoradmin;
> > >> CREATE ROLE regress_nosuch_read_all_data IN ROLE pg_read_all_data;
> > >> ERROR:  must have admin option on role "pg_read_all_data"
> > >
> > > I would say not just privileged roles, but any roles that the user
> > > doesn't have admin rights on.
> >
> > Yes, that's how it works.  But this portion of the test is only checking the interaction between CREATEROLE and
built-inprivileged roles, hence the comment. 
>
> But.. predefined roles aren't actually different in this regard from any
> other role, so I disagree that such a test of explicitly predefined
> roles makes sense..?
>
> > >> Whether "WITH ADMIN OPTION" or "WITHOUT ADMIN OPTION" is implied hinges on whether the role is given CREATEROLE.
That hackery is necessary to preserve backwards compatibility.  If we don't care about compatibility, I could change
thepatch to make "WITHOUT ADMIN OPTION" implied for all attributes when not specified. 
> > >
> > > Given the relative size of the changes we're talking about regarding
> > > CREATEROLE, I don't really think we need to stress about backwards
> > > compatibility too much.
> >
> > Yeah, I'm leaning pretty strongly that way, too.
>
> Great.
>
> Thanks,
>
> Stephen