Re: CREATEROLE and role ownership hierarchies

> On Jan 30, 2022, at 2:38 PM, Michael Banck <michael(dot)banck(at)credativ(dot)de> wrote:
>
> Hi,

Your review is greatly appreciated!

>> The attached WIP patch attempts to solve most of the CREATEROLE

I'm mostly looking for whether the general approach in this Work In Progress patch is acceptable, so I was a bit sloppy with whitespace and such....

> 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!

> Some light review of the patch (I haven't read all the previous ones, so
> please excuse me if I rehash old discussions):

Not a problem.

> Spaces vs. tabs here...
>
> ... and here, is that intentional?

> I think typdefs usually go at the top of the file, not at line 5441...

> I feel this function comment needs revision...

> Hrm, maybe also mention ...

All good comments, but I'm not doing code cleanup on this WIP patch just yet. Forgive me.

> Shouldn't this (and the following) be "must have admin option on
> CREATEROLE"?

Yes, there may be other places where I failed to replace the verbiage "grant option" with "admin option". Earlier drafts of the patch were using that language. I wouldn't mind review comments on which language people thinks is better.

>
>> @@ -311,7 +370,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>> stmt->role)));
>>
>> /* Convert validuntil to internal form */
>> - if (validUntil)
>> + if (validUntil && strcmp(validUntil, "always") != 0)
>
> This (there are other similar hunks further down) looks like an
> independent patch/feature?

Part of the problem with the grammar introduced in this patch is that you are not normally required to mention attributes like VALID UNTIL, but if you want to change whether the created role gets WITH ADMIN OPTION, you have to. That leaves the problem of what to do if you *only* want to specify the ADMIN part. The grammar needs some sort of "dummy" value that intentionally has no effect, but sets up for the WITH/WITHOUT ADMIN OPTION clause. I think I left a few bits of cruft around like that. But what I'd really like to know is if people think this sort of thing is even headed in the right direction? Are there problems with SQL spec compliance? Does it just feel icky? I don't have any pride-of-ownership in the grammar this WIP patch introduces. I just needed something to put out there for people to attack/improve.

>> +
>> + /* To mess with replication roles, must have admin on REPLICATION */
>> + if ((authform->rolreplication || disreplication) &&
>> + !may_admin_replication_privilege(GetUserId()))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> + errmsg("must have admin on replication to alter replication roles or change replication attribute")));
>
> "have admin" sounds a bit weird, but I understand the error message is
> too long already to spell out "must have admin option"? Or am I mistaken
> and "admin" is what it's actually called (same for the ones below)?

If it is the officially correct language, I arrived at it by accident. I didn't take any time to wordsmith those error messages. Improvements welcome!

> Also, I think those role options are usually capitalized like
> REPLICATION in other error messages.

Yeah, I noticed some amount of inconsistency there. For a brief time I was trying to make them all the same, but got a bit confused on what would be correct, and didn't waste the time. The sort of thing I'm thinking about is the pre-existing message text, "must be superuser to change bypassrls attribute". Note that neither "superuser" nor "bypassrls" are capitalized. If people like where this patch is going, I'll no doubt need to clean it up.

>> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
>> index b5966712ce..7503d3ead6 100644
>> --- a/src/backend/parser/gram.y
>> +++ b/src/backend/parser/gram.y
>> @@ -1131,67 +1140,111 @@ AlterOptRoleElem:
> [...]
>
>> + | VALID ALWAYS opt_admin_spec
>> + {
>> + RoleElem *n = makeNode(RoleElem);
>> + n->elem = makeDefElem("validUntil", (Node *)makeString("always"), @1);
>> + n->admin_spec = $3;
>> + $$ = (Node *)n;
>
> This one is from another patch as well I think.

That was an attempt at a "dummy" type value. I agree it probably doesn't belong.

>> }
>> /* Supported but not documented for roles, for use by ALTER GROUP. */
>> - | USER role_list
>> + | USER role_list opt_admin_spec
>> {
>> - $$ = makeDefElem("rolemembers", (Node *)$2, @1);
>> + RoleElem *n = makeNode(RoleElem);
>> + n->elem = makeDefElem("rolemembers", (Node *)$2, @1);
>> + n->admin_spec = $3;
>> + $$ = (Node *)n;
>> }
>> - | IDENT
>> + | IDENT opt_admin_spec
>> {
>> /*
>> * We handle identifiers that aren't parser keywords with
>> * the following special-case codes, to avoid bloating the
>> * size of the main parser.
>> */
>> + RoleElem *n = makeNode(RoleElem);
>> +
>> + /*
>> + * Record whether the user specified WITH GRANT OPTION.
>
> WITH ADMIN OPTION rather?

Yes.

>> + * Note that for some privileges this is always implied,
>> + * such as SUPERUSER, but we don't reflect that here.
>> + */
>> + n->admin_spec = $2;
>> +
>
>> diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
>> index 6c28119fa1..4829a6dbd2 100644
>> --- a/src/include/catalog/pg_authid.dat
>> +++ b/src/include/catalog/pg_authid.dat
>> @@ -22,67 +22,93 @@
>> { oid => '10', oid_symbol => 'BOOTSTRAP_SUPERUSERID',
>> rolname => 'POSTGRES', rolsuper => 't', rolinherit => 't',
>> rolcreaterole => 't', rolcreatedb => 't', rolcanlogin => 't',
>> - rolreplication => 't', rolbypassrls => 't', rolconnlimit => '-1',
>> + rolreplication => 't', rolbypassrls => 't', adminherit => 't', admcreaterole => 't',
>> + admcreatedb => 't', admcanlogin => 't', admreplication => 't', admbypassrls => 't',
>> + admconnlimit => 't', admpassword => 't', admvaliduntil => 't', rolconnlimit => '-1',
>> rolpassword => '_null_', rolvaliduntil => '_null_' },
>
> Those sure are a couple of new columns in pg_authid, but oh well...

Yes, that's also a big part of what people might object to. I think it's a reasonable objection, but I don't know where else to put the information, given the lack of an aclitem[]?

>> diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
>> index 4b65e39a1f..4acdcaa685 100644
>> --- a/src/include/catalog/pg_authid.h
>> +++ b/src/include/catalog/pg_authid.h
>> @@ -39,6 +39,16 @@ CATALOG(pg_authid,1260,AuthIdRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_OID(284
>> bool rolcanlogin; /* allowed to log in as session user? */
>> bool rolreplication; /* role used for streaming replication */
>> bool rolbypassrls; /* bypasses row-level security? */
>> +
>> + bool adminherit; /* allowed to administer inherit? */
>> + bool admcreaterole; /* allowed to administer createrole? */
>> + bool admcreatedb; /* allowed to administer createdb?? */
>> + bool admcanlogin; /* allowed to administer login? */
>> + bool admreplication; /* allowed to administer replication? */
>> + bool admbypassrls; /* allowed to administer bypassesrls? */
>> + bool admconnlimit; /* allowed to administer connlimit? */
>> + bool admpassword; /* allowed to administer password? */
>> + bool admvaliduntil; /* allowed to administer validuntil? */
>> int32 rolconnlimit; /* max connections allowed (-1=no limit) */
>
> It's cosmetic, but the space between rolbypassrls and adminherit is
> maybe not needed, and I'd put rolconnlimit first (even though it has a
> different type).

Oh, totally agree. I had that blank there during development because the "rol..." and "adm..." all started to blur together.

Thanks again! If the patch stays mostly like it is, I'll incorporate all your review comments into a next version.

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