Re: CREATEROLE and role ownership hierarchies - Mailing list pgsql-hackers

> On Jan 31, 2022, at 12:43 AM, Michael Banck <michael.banck@credativ.de> wrote:

> Ok, sure. I think this topic is hugely important and as I read the
> patch anyway, I added some comments, but yeah, we need to figure out
> the fundamentals first.

Right.

Perhaps some background on this patch series will help.  The patch versions before v8 were creating an owner-owned
relationshipbetween the creator and the createe, and a lot of privileges were dependent on that ownership.  Stephen
objectedthat we were creating parallel tracks on which the privilege system was running; things like belonging to a
roleor having admin on a role were partially conflated with owning a role.  He also objected that the pre-v8 patch sets
alloweda creator role with the CREATEROLE privilege to give away any privilege the creator had, rather than needing to
haveGRANT or ADMIN option on the privilege being given. 

The v8-WIP patch is not a complete replacement for the pre-v8 patches.  It's just a balloon I'm floating to try out
candidatesolutions to some of Stephen's objections.  In the long run, I want the solution to Stephen's objections to
notcreate problems for anybody who liked the way the pre-v8 patches worked (Robert, Andrew, and to some extent me.) 

In this WIP patch, for a creator to give *anything* away to a createe, the creator must have GRANT or ADMIN on the
thingbeing given.  That includes attributes like BYPASSRLS, CREATEDB, LOGIN, etc., and also ADMIN on any role the
createeis granted into. 

I tried to structure things for backwards compatibility, considering which things roles with CREATEROLE could give away
historically. It turns out they can give away most everything, but not SUPERUSER, BYPASSRLS, or REPLICATION.  So I
structuredthe default privileges for CREATEROLE to match.  But I'm uncertain that design is any good, and your comments
belowsuggest that you find it pretty hard to use. 

Part of the problem with trying to be backwards compatible is that we must break compatibility anyway, to address the
problemthat historically having CREATEROLE meant you effectively had ADMIN on all non-superuser roles.  That's got to
change. So in part I'm asking pgsql-hackers if partial backwards compatibility is worth the bother. 

If we don't go with backwards compatibility, then CREATEROLE would only allow you to create a new role, but not to give
thatrole LOGIN, nor CREATEDB, etc.  You'd need to also have admin option on those things.  To create a role that can
givethose things away, you'd need to run something like: 

CREATE ROLE michael
    CREATEROLE WITH ADMIN OPTION    -- can further give away "createrole"
    CREATEDB WITH ADMIN OPTION    -- can further give away "createdb"
    LOGIN WITH ADMIN OPTION    -- can further give away "login"
    NOREPLICATION WITHOUT ADMIN OPTION    -- this would be implied anyway
    NOBYPASSRLS WITHOUT ADMIN OPTION    -- this would be implied anyway
    CONNECTION LIMIT WITH ADMIN OPTION    -- can specify connection limits
    PASSWORD WITH ADMIN OPTION    -- can specify passwords
    VALID UNTIL WITH ADMIN OPTION    -- can specify expiration

(I'm on the fence about the phrase "WITH ADMIN OPTION" vs. the phrase "WITH GRANT OPTION".)

Even then, when "michael" creates new roles, if he wants to be able to further administer those roles, he needs to
rememberto give himself ADMIN membership in that role at creation time.  After the role is created, if he doesn't have
ADMIN,he can't give it to himself.  So, at create time, he needs to remember to do this: 

SET ROLE michael;
CREATE ROLE mark ADMIN michael;

But that's still a bit strange, because "ADMIN michael" means that michael can grant other roles membership in "mark",
notthat michael can, for example, change mark's password.  If we don't want CREATEROLE to imply that you can mess
aroundwith arbitrary roles (rather than only roles that you created or have been transferred control over) then we need
theconcept of role ownership.  This patch doesn't go that far, so for now, only superusers can do those things.
Assumingsome form of this patch is acceptable, the v9 series will resurrect some of the pre-v7 logic for role ownership
andsay that the owner can do those things. 


>>> One thing I noticed (and which will likely make DBAs grumpy) is that it
>>> seems being able to create users (as opposed to non-login roles/groups)
>>> depends on when you get the CREATEROLE attribute (on role creation or
>>> later), viz:
>>>
>>> postgres=# CREATE USER admin CREATEROLE;
>>> CREATE ROLE
>>> postgres=# SET ROLE admin;
>>> SET
>>> postgres=> CREATE USER testuser; -- this works
>>> CREATE ROLE
>>> postgres=> RESET ROLE;
>>> RESET
>>> postgres=# CREATE USER admin2;
>>> CREATE ROLE
>>> postgres=# ALTER ROLE admin2 CREATEROLE; -- we get CREATEROLE after the fact
>>> ALTER ROLE
>>> postgres=# SET ROLE admin2;
>>> SET
>>> postgres=> CREATE USER testuser2; -- bam
>>> ERROR:  must have grant option on LOGIN privilege to create login users
>>> postgres=# SELECT rolname, admcreaterole, admcanlogin FROM
>>> pg_authid
>>> WHERE rolname LIKE 'admin%';
>>> rolname | admcreaterole | admcanlogin
>>> ---------+---------------+-------------
>>> admin   | t             | t
>>> admin2  | f             | f
>>> (2 rows)
>>>
>>> Is that intentional? If it is, I think it would be nice if this
>>> could be
>>> changed, unless I'm missing some serious security concerns or so.
>>
>> It's intentional, but part of what I wanted review comments about.
>> The issue is that historically:
>>
>>   CREATE USER michael CREATEROLE
>>
>> meant that you could go on to do things like create users with LOGIN
>> privilege.  I could take that away, which would be a backwards
>> compatibility break, or I can do the weird thing this patch does.  Or
>> I could have your
>>
>>   ALTER ROLE admin2 CREATEROLE;
>>
>> also grant the other privileges like LOGIN unless you explicitly say
>> otherwise with a bunch of explicit WITHOUT ADMIN OPTION clauses.
>> Finding out which of those this is preferred was a big part of why I
>> put this up for review.  Thanks for calling it out in under 24 hours!
>
> Ok, so what I would have needed to do in the above in order to have
> "admin2" and "admin" be the same as far as creating login users is (I
> believe):
>
> ALTER ROLE admin2 CREATEROLE LOGIN WITH ADMIN OPTION;

Yes, those it's more likely admin2 would have been created with these privileges to begin with, if the creator intended
admin2to do such things.  

> I think if possible, it would be nice to just have this part as default
> if possible. I.e. CREATEROLE and HASLOGIN are historically so much
> intertwined that I think the above should be implicit (again, if that
> is possible); I don't care and/or haven't made up my mind about any of
> the other options so far...

Possibily.  But then, if you really wanted to grant someone CREATEROLE but not anything else, you'd need to remember
whichother things are implicit, and explicitly disavow them, like: 

ALTER ROLE admin2 CREATEROLE (WITHOUT this, WITHOUT that, WITHOUT the other)

and I think that mostly stinks.

> Ok, so now that I had another look, I see we are going down Pandora's
> box: For any of the other things a role admin would like to do (change
> password, change conn limit), one would have to go with this weird
> disconnect between CREATE USER admin CREATEROLE and ALTER USER admin2
> CREATEROLE [massive list of WITH ADMIN OPTION], and then I'm not sure
> where we stop.

I agree.  That's a good argument for just breaking backward compatibility.

> By the way, is there now even a way to add admpassword to a role after
> it got created?
>
> postgres=# SET ROLE admin2;
> SET
> postgres=> \password test
> Enter new password for user "test":
> Enter it again:
> ERROR:  must have admin on password to change password attribute
> postgres=> RESET ROLE;
> RESET
> postgres=# ALTER ROLE admin2 PASSWORD WITH ADMIN OPTION;
> ERROR:  syntax error at or near "WITH"
> UPDATE pg_authid SET admpassword = 't' WHERE rolname = 'admin2';
> UPDATE 1
> postgres=# SET ROLE admin2;
> SET
> postgres=> \password test
> Enter new password for user "test":
> Enter it again:
> postgres=>

I don't really have this worked out yet.  That's mostly because I'm planning to fix it with role ownership, but perhaps
thereis a better way? 

> However, the next thing is:
>
> postgres=# SET ROLE admin;
> SET
> postgres=> CREATE GROUP testgroup;
> CREATE ROLE
> postgres=> GRANT testgroup TO test;
> ERROR:  must have admin option on role "testgroup"
>
> First off, what does "admin option" mean on a role?

From the docs for "CREATE ROLE", https://www.postgresql.org/docs/14/sql-createrole.html

  The ADMIN clause is like ROLE, but the named roles are added to the new role WITH ADMIN OPTION, giving them the right
togrant membership in this role to others. 

> I then tried this:
>
> postgres=# CREATE USER admin3 CREATEROLE WITH ADMIN OPTION;
> CREATE ROLE
> postgres=# SET ROLE admin3;
> SET
> postgres=> CREATE USER test3;
> CREATE ROLE
> postgres=> CREATE GROUP testgroup3;
> CREATE ROLE
> postgres=> GRANT testgroup3 TO test3;
> ERROR:  must have admin option on role "testgroup3"
>
> So I created both user and group, I have the CREATEROLE priv (with or
> without admin option), but I still can't assign the group. Is that
> (tracking who created a role and letting the creator do more thing) the
> part that got chopped away in your last patch in order to find a common
> ground?

You need ADMIN on the role, not on CREATEROLE.  To add members to a target role, you must have ADMIN on that target
role. To create new roles with CREATEROLE privilege, you must have ADMIN on the CREATEROLE privilege. 

> Is there now any way non-Superusers can assign groups to other users?

Yes, by having ADMIN on those groups.

> I
> feel this (next to creating users/groups) is the primary thing those
> CREATEROLE admins are supposed to do/where doing up to now.

Right.  In the past, having CREATEROLE implied having ADMIN on every role.  I'm intentionally breaking that.

> The way the adm* privs are now somewhere in the middle of the rol*
> privs also looks weird for the end-user and there does not seems to be
> some greater scheme behind it:

Because they are not variable length nor nullable, they must come before such fields (namely, rolpassword and
rolvaliduntil). They don't really need to come before rolconnlimit, but I liked the idea of packing twelve booleans
together,since with "bool" typedef'd to unsigned char, that's twelve contiguous bytes, starting after oid (4 bytes) and
rolname(64 bytes) and likely fitting nicely without padding bytes on at least some platforms.  If I split them on
eitherside of rolconnlimit (which is 4 bytes), there'd be seven bools before it and five bools after, which wouldn't
packnicely. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company