Re: CREATEROLE and role ownership hierarchies

On Thu, Feb 17, 2022 at 12:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Jan 31, 2022 at 1:57 PM Joshua Brindle
> <joshua(dot)brindle(at)crunchydata(dot)com> wrote:
> > 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.
>
> So, where do we go from here?
>
> I've been thinking about this comment a bit. On the one hand, I have
> some reservations about the phrase "the use case I am trying to
> accomplish with this patchset," because in the end, this is not your
> patch set. It's not reasonable to complain that a patch someone else
> wrote doesn't solve your problem; of course everyone writes patches to
> solve their own problems, or those of their employer, not other
> people's problems. And that's as it should be, else we will have few
> contributors. On the other hand, to the extent that this patch set
> makes things worse for a reasonable use case which you have in mind,
> that's an entirely legitimate complaint.

Yes, absolutely. It is my understanding that generally a community
consensus is attempted, I was throwing my (and Crunchy's) use case out
there as a possible goal, and I have spent time reviewing and testing
the patch, so I think that is fair. Obviously I am not in the position
to stipulate hard requirements.

> After a bit of testing, it seems to me that as things stand today,
> things are nearly perfect for the use case that you have in mind. I
> would be interested to know whether you agree. If I set up an account
> and give it CREATEROLE, it can create users, and it can put them into
> the employees role, but it can't SET ROLE to any of those accounts. It
> can also ALTER ROLE ... NOLOGIN on any of those accounts. The only gap
> I see is that there are certain role-based flags which the CREATEROLE
> account cannot set: SUPERUSER, BYPASSRLS, REPLICATION. You might
> prefer a system where your bot account had the option to grant those
> privileges also, and I think that's a reasonable thing to want.

I believe the only issue in the existing patchset was that membership
was required in employees was required for the Bot, but I can apply
the current patchset and test it out more in a bit.

> However, I *also* think it's reasonable to want an account that can
> create roles but can't give to those roles membership in roles that it
> does not itself possess. Likewise, I think it's reasonable to want an
> account that can only drop roles which that account itself created.
> These kinds of requirements stem from a different use case than what
> you are talking about here, but they seem like fine things to want,
> and as far as I know we have pretty broad agreement that they are
> reasonable. It seems extremely difficult to make a convincing argument
> that this is not a thing which anyone should want to block:
>
> rhaas=> create role bob role pg_execute_server_program;
> CREATE ROLE
>
> Honestly, that seems like a big yikes from here. How is it OK to block
> "create role bob superuser" yet allow that command? I'm inclined to
> think that's just broken. Even if the role were pg_write_all_data
> rather than pg_execute_server_program, it's still a heck of a lot of
> power to be handing out, and I don't see how anyone could make a
> serious argument that we shouldn't have an option to restrict that.

Yes, agreed 100%. To be clear, I do not want Bot in the above use case
to be able to add any role other than employees to new roles it
creates. So we are in complete agreement there, the only difference is
that I do not want Bot to be able to become those roles (or use any
access granted via those roles), it's only job is to manage roles, not
look at data.

> Let me separate the two features that I just mentioned and talk about
> them individually:
>
> 1. Don't allow a CREATEROLE user to give out membership in groups
> which that user does not possess. Leaving aside the details of any
> previously-proposed patches and just speaking theoretically, how can
> this be implemented? I can think of a few ideas. We could (1A) just
> change CREATEROLE to work that way, but IIUC that would break the use
> case you outline here, so I guess that's off the table unless I am
> misunderstanding the situation. We could also (1B) add a second role
> attribute with a different name, like, err, CREATEROLEWEAKLY, that
> behaves in that way, leaving the existing one untouched. But we could
> also take it a lot further, because someone might want to let an
> account hand out a set of privileges which corresponds neither to the
> privileges of that account nor to the full set of available
> privileges. That leads to another idea: (1C) implement an in-database
> system that lets you specify which privileges an account has, and,
> separately, which ones it can assign to others. I am skeptical of that
> idea because it seems really, really complicated, not only from an
> implementation standpoint but even just from a user-experience
> standpoint. Suppose user 'userbot' has rights to grant a suitable set
> of groups to the new users that it creates -- but then someone creates
> a new group. Should that also be added to the things 'userbot' can
> grant or not? What if we have 'userbot1' through 'userbot6' and each
> of them can grant a different set of roles? I wouldn't mind (1D)
> providing a hook that allows the system administrator to install a
> loadable module that can enforce any rules it likes, but it seems way
> too complicated to me to expose all of this configuration as SQL,
> especially because for what I want to do, either (1A) or (1B) is
> adequate, and (1B) is a LOT simpler than (1C). It also caters to what
> I believe to be a common thing to want, without prejudice to the
> possibility that other people want other things.

if 1A worked for admins, or members I think it may work (i.e., Bot is
admin of employees but not a member of employees and therefore can
manage employees but not become them or read their tables)

For example, today this works (in master):

postgres=# CREATE USER creator password 'a';
CREATE ROLE
postgres=# CREATE ROLE employees ADMIN creator NOLOGIN;
CREATE ROLE

as creator:
postgres=> CREATE USER joshua IN ROLE employees PASSWORD 'a';
ERROR: permission denied to create role

as superuser:
postgres=# CREATE USER joshua LOGIN PASSWORD 'a';
CREATE ROLE

as creator:
postgres=> GRANT employees TO joshua;
GRANT ROLE
postgres=> SET ROLE joshua;
ERROR: permission denied to set role "joshua"
postgres=> SET ROLE employees;
SET

So ADMIN of a role can add membership, but not create, and
unfortunately can SET ROLE to employees.

Can ADMIN mean "can create and drop roles with membership of this role
but not implicitly be a member of the role"?

I think Stephen was advocating for this but wanted to look at the SQL
spec to see if it conflicts.

The current (v8) patch conflates membership and admin:

postgres=# CREATE USER user_creator CREATEROLE WITHOUT ADMIN OPTION
PASSWORD 'a';
CREATE ROLE
postgres=# CREATE ROLE employees ADMIN user_creator NOLOGIN;
CREATE ROLE

(Note I never GRANTED employees to user_creator):

postgres=# \du
List of roles
Role name | Attributes
| Member of
--------------+------------------------------------------------------------+-------------
employees | Cannot login | {}
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
user_creator | Create role
| {employees}

postgres=# REVOKE employees FROM user_creator;
REVOKE ROLE

as user_creator:
postgres=> CREATE USER joshua2 IN ROLE employees;
ERROR: must have admin option on role "employees"

This seems non-intuitive to me, employees was never granted, but after
being revoked the admin option is gone.

> Joshua, what is your opinion on this point?
>
> 2. Only allow a CREATEROLE user to drop users which that account
> created, and not just any role that isn't a superuser. Again leaving
> aside previous proposals, this cannot be implemented without providing
> some means by which we know which CREATEROLE user created which other
> user. I believe there are a variety of words we could use to describe
> that linkage, and I don't deeply care which ones we pick, although I
> have my own preferences. We could speak of the CREATEROLE user being
> the owner, manager, or administrator of the created role. We could
> speak of a new kind of object, a TENANT, of which the CREATEROLE user
> is the administrator and to which the created user is linked. I
> proposed this previously and it's still my favorite idea. There are no
> doubt other options as well. But it's axiomatic that we cannot
> restrict the rights of a CREATEROLE user to drop other roles to a
> subset of roles without having some way to define which subset is at
> issue.
>
> Now, my motivation for wanting this feature is pretty simple: I want
> to have something that feels like a superuser but isn't a full
> superuser, and can't interfere with accounts set up by the service
> provider, but can do whatever they want to the other ones. But I think
> this is potentially useful in the userbot case that you (Joshua)
> mention as well, because it seems like it could be pretty desirable to
> have a certain list of users which the userbot can't remove, just for
> safety, either to limit the damage if somebody gets into that account,
> or just to keep the bot from going nuts and doing something it
> shouldn't in the event of a programming error. Now, if you DON'T care
> about the userbot being able to access this functionality, that's fine
> with me, because then there's nothing left to do but argue about what
> to call the linkage between the CREATEROLE user and the created user.
> Your userbot need not participate in whatever system we decide on, and
> things are no worse for that use case than they are today.

Not being able to drop roles that weren't created or managed by the
Bot is good. Being able to specify exactly what roles the Bot can drop
is ideal, we may want no automated drops whatsoever (just automated
disabling, to constrain possible damage).

> But if you DO want the userbot to be able to access that
> functionality, then things are more complicated, because now the
> linkage has to be special-purpose. In that scenario, we can't say that
> the right of a CREATEROLE user to drop a certain other role implies
> having the privileges of that other role, because in your use case,
> you don't want that, whereas in mine, I do. What makes this
> particularly ugly is that we can't, as things currently stand, use a
> role as the grouping mechanism, because of the fact that a role can
> revoke membership in itself from some other role. It will not do for
> roles to remove themselves from the set of roles that the CREATEROLE
> user can drop. If we changed that behavior, then perhaps we could just
> define a way to say that role X can drop roles if they are members of
> group G. In my tenant scenario, G would be granted to X, and in your
> userbot scenario, it wouldn't. Everybody wins, except for any people
> who like the ability of roles to revoke themselves from any group
> whatsoever.
>
> So that leads to these questions: (2A) Do you care about restricting
> which roles the userbot can drop? (2B) If yes, do you endorse
> restricting the ability of roles to revoke themselves from other
> roles?

2A, yes
2B, yes, and IIUC this already exists:
postgres=> select current_user;
current_user
--------------
joshua
(1 row)

postgres=> REVOKE employees FROM joshua;
ERROR: must have admin option on role "employees"

> I think that we don't have any great problems here, at least as far as
> this very specific issue is concerned, if either the answer to (2A) is
> no or the answer to (2B) is yes. However, if the answer to (2A) is yes
> and the answer to (2B) is no, there are difficulties. Evidently in
> that case we need some new kind of thing that behaves mostly likes a
> group of roles but isn't actually a group of roles -- and that thing
> needs to prohibit self-revocation. Given what I've written above, you
> may be able to guess my preferred solution: let's call it a TENANT.
> Then, my pseudo-super-user can have permission to (i) create roles in
> that tenant, (ii) drop roles in that tenant, and (iii) assume the
> privileges of roles in that tenant -- and your userbot can have
> privileges to do (i) and (ii) but not (iii). All we need do is add a
> roltenant column to pg_authid and find three bits someplace
> corresponding to (i)-(iii), and we are home.

I believe this works.

> Thoughts?
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com