Re: CREATEROLE and role ownership hierarchies

> On Jan 22, 2022, at 1:20 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
>> Subject: [PATCH v4 1/5] Add tests of the CREATEROLE attribute.
>
> No particular issue with this one.

Andrew already committed this, forcing the remaining patches to be renumbered. Per your comments below, I have combined what was 0002+0003 into 0001, renumbered 0004 as 0002, and abandoned 0005. (It may come back as an independent patch.) Also owing to the fact that 0001 has been committed, I really need to post another patch set right away, to make the cfbot happy. I'm fixing non-controversial deficits you call out in your review, but leaving other things unchanged, in the interest of getting a patch posted sooner rather than later.

>> Subject: [PATCH v4 2/5] Add owners to roles
>>
>> All roles now have owners. By default, roles belong to the role
>> that created them, and initdb-time roles are owned by POSTGRES.
>
> ... database superuser, not 'POSTGRES'.

I rephrased this as "bootstrap superuser" in the commit message.

>> +++ b/src/backend/catalog/aclchk.c
>> @@ -5430,6 +5434,57 @@ pg_statistics_object_ownercheck(Oid stat_oid, Oid roleid)
>> return has_privs_of_role(roleid, ownerId);
>> }
>>
>> +/*
>> + * Ownership check for a role (specified by OID)
>> + */
>> +bool
>> +pg_role_ownercheck(Oid role_oid, Oid roleid)
>> +{
>> + HeapTuple tuple;
>> + Form_pg_authid authform;
>> + Oid owner_oid;
>> +
>> + /* Superusers bypass all permission checking. */
>> + if (superuser_arg(roleid))
>> + return true;
>> +
>> + /* Otherwise, look up the owner of the role */
>> + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(role_oid));
>> + if (!HeapTupleIsValid(tuple))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_UNDEFINED_OBJECT),
>> + errmsg("role with OID %u does not exist",
>> + role_oid)));
>> + authform = (Form_pg_authid) GETSTRUCT(tuple);
>> + owner_oid = authform->rolowner;
>> +
>> + /*
>> + * Roles must necessarily have owners. Even the bootstrap user has an
>> + * owner. (It owns itself). Other roles must form a proper tree.
>> + */
>> + if (!OidIsValid(owner_oid))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_DATA_CORRUPTED),
>> + errmsg("role \"%s\" with OID %u has invalid owner",
>> + authform->rolname.data, authform->oid)));
>> + if (authform->oid != BOOTSTRAP_SUPERUSERID &&
>> + authform->rolowner == authform->oid)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_DATA_CORRUPTED),
>> + errmsg("role \"%s\" with OID %u owns itself",
>> + authform->rolname.data, authform->oid)));
>> + if (authform->oid == BOOTSTRAP_SUPERUSERID &&
>> + authform->rolowner != BOOTSTRAP_SUPERUSERID)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_DATA_CORRUPTED),
>> + errmsg("role \"%s\" with OID %u owned by role with OID %u",
>> + authform->rolname.data, authform->oid,
>> + authform->rolowner)));
>> + ReleaseSysCache(tuple);
>> +
>> + return (owner_oid == roleid);
>> +}
>
> Do we really need all of these checks on every call of this function..?

Since the function is following the ownership chain upwards, it seems necessary to check that the chain is wellformed, else we might get into an infinite loop or return the wrong answer. These would only happen under corrupt conditions, but it seems sensible to check for those, since they are cheap to check. (Actually, the check for nontrivial cycles included in the patch is not as efficient as it could be, but I'm punting the work of improving that algorithm from quadratic to linear until a later patch version, in the interest of posting the patch soon.)

> Also, there isn't much point in including the role OID twice in the last
> error message, is there? Unless things have gotten quite odd, it's
> goint to be the same value both times as we just proved to ourselves
> that it is, in fact, the same value (and that it's not the
> BOOTSTRAP_SUPERUSERID).

It is comparing the authform->oid against the authform->rolowner, which are not the same. The first is the owned role, the second is the owning role. We could hardcode the message to say something like "bootstrap superuser owned by role with Oid %u", but that hardcodes "bootstrap superuser" into the message, rather than something like "stephen". I don't feel strongly about the wording. Let me know if you still want me to change it.

> This function also doesn't actually do any kind of checking to see if
> the role ownership forms a proper tree, so it seems a bit odd to have
> the comment talking about that here where it's doing other checks.

Right. The comment simply explains the structure we expect, not the structure we are fully validating. The point is that each link in the hierarchy must be compatible with the expected structure. It would be overkill to validate the whole tree in this one function. I don't mind rewording the code comment, if you have a less confusing suggestion.

>> +++ b/src/backend/commands/user.c
>> @@ -77,6 +79,9 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>> Datum new_record[Natts_pg_authid];
>> bool new_record_nulls[Natts_pg_authid];
>> Oid roleid;
>> + Oid owner_uid;
>> + Oid saved_uid;
>> + int save_sec_context;
>
> Seems a bit odd to introduce 'uid' into this file, which hasn't got any
> such anywhere in it, and I'm not entirely sure that any of these are
> actually needed..?

Good catch! The implementation in v6 was wrong. It didn't enforce that the creating role was a member of the target owner, something this next patch set does.

>> @@ -108,6 +113,16 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>> DefElem *dvalidUntil = NULL;
>> DefElem *dbypassRLS = NULL;
>>
>> + GetUserIdAndSecContext(&saved_uid, &save_sec_context);
>> +
>> + /*
>> + * Who is supposed to own the new role?
>> + */
>> + if (stmt->authrole)
>> + owner_uid = get_rolespec_oid(stmt->authrole, false);
>> + else
>> + owner_uid = saved_uid;
>> +
>> /* The defaults can vary depending on the original statement type */
>> switch (stmt->stmt_type)
>> {
>> @@ -254,6 +269,10 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>> ereport(ERROR,
>> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> errmsg("must be superuser to create superusers")));
>> + if (!superuser_arg(owner_uid))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> + errmsg("must be superuser to own superusers")));
>> }
>> else if (isreplication)
>> {
>
> So, we're telling a superuser (which is the only way you could get to
> this point...) that they aren't allowed to create a superuser role which
> is owned by a non-superuser... Why?

The reason is one you won't like very much. Given that roles have the privileges of roles they own (which you don't like), allowing a non-superuser to own a superuser effectively promotes that owner to superuser status. That's a pretty obscure way of making someone a superuser, probably not what was intended, and quite a high-caliber foot-gun.

Even if roles didn't inherit privileges from roles they own, I think it would be odd for a non-superuser to own a superuser. The definition of "ownership" would have to be extremely restricted to prevent the owner from using their ownership to obtain superuser.

>> @@ -310,6 +329,19 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>> errmsg("role \"%s\" already exists",
>> stmt->role)));
>>
>> + /*
>> + * If the requested authorization is different from the current user,
>> + * temporarily set the current user so that the object(s) will be created
>> + * with the correct ownership.
>> + *
>> + * (The setting will be restored at the end of this routine, or in case of
>> + * error, transaction abort will clean things up.)
>> + */
>> + if (saved_uid != owner_uid)
>> + SetUserIdAndSecContext(owner_uid,
>> + save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
>
> Err, why is this needed? This looks copied from the CreateSchemaCommand
> but, unlike with the create schema command, CreateRole doesn't actually
> allow sub-commands to be run to create other objects in the way that
> CreateSchemaCommand does.

Not quite. There are still the check_password_hook and RunObjectPostCreateHook() to consider. The check_password_hook might want to validate the validuntil_time parameter against the owner's validuntil time, or some other property of the owner. And the RunObjectPostCreateHook (called via InvokeObjectPostCreateHook(AuthIdRelationId, roleid, 0)) may want the information, too.

I'm not saying these are super strong arguments. If people generally feel that CREATE ROLE ... AUTHORIZATION shouldn't call SetUserIdAndSecContext, feel free to argue that.

>> @@ -1675,3 +1714,110 @@ DelRoleMems(const char *rolename, Oid roleid,
>> +static void
>> +AlterRoleOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
>> +{
>> + Form_pg_authid authForm;
>> +
>> + Assert(tup->t_tableOid == AuthIdRelationId);
>> + Assert(RelationGetRelid(rel) == AuthIdRelationId);
>> +
>> + authForm = (Form_pg_authid) GETSTRUCT(tup);
>> +
>> + /*
>> + * If the new owner is the same as the existing owner, consider the
>> + * command to have succeeded. This is for dump restoration purposes.
>> + */
>> + if (authForm->rolowner != newOwnerId)
>> + {
>> + /* Otherwise, must be owner of the existing object */
>> + if (!pg_role_ownercheck(authForm->oid, GetUserId()))
>> + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_ROLE,
>> + NameStr(authForm->rolname));
>> +
>> + /* Must be able to become new owner */
>> + check_is_member_of_role(GetUserId(), newOwnerId);
>
> Feels like we should be saying a bit more about why we check for role
> membership vs. has_privs_of_role() here. I'm generally of the opinion
> that membership is the right thing to check here, just feel like we
> should try to explain more why that's the right thing.

For orthogonality with how ALTER .. OWNER TO works for everything else? AlterEventTriggerOwner_internal doesn't check this explicitly, but that's because it has already checked that the new owner is superuser, so the check must necessarily succeed. I'm not aware of any ALTER .. OWNER TO commands that don't require this, at least implicitly.

We could explain this in AlterRoleOwner_internal, as you suggest, but if we need it there, do we need to put the same explanation in functions which handle other object types? I don't see why this one function would require the explanation if other equivalent functions do not.

>> + /*
>> + * must have CREATEROLE rights
>> + *
>> + * NOTE: This is different from most other alter-owner checks in that
>> + * the current user is checked for create privileges instead of the
>> + * destination owner. This is consistent with the CREATE case for
>> + * roles. Because superusers will always have this right, we need no
>> + * special case for them.
>> + */
>> + if (!have_createrole_privilege())
>> + aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_ROLE,
>> + NameStr(authForm->rolname));
>> +
>
> I would think we'd be trying to get away from the role attribute stuff.

That's not a bad idea, but I thought it was discussed months ago. The two options were (1) keep using CREATEROLE but change it to be less powerful, and (2) add a new built-in role, say "pg_create_role", and have membership in that role be what we use. Option (2) was generally viewed less favorably, or that was my sense of people's opinions, on the theory that we'd be better off fixing how CREATEROLE works than having two different ways of doing roughly the same thing.

>> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
>
>> + CREATE ROLE RoleId AUTHORIZATION RoleSpec opt_with OptRoleList
>> + {
>> + CreateRoleStmt *n = makeNode(CreateRoleStmt);
>> + n->stmt_type = ROLESTMT_ROLE;
>> + n->role = $3;
>> + n->authrole = $5;
>> + n->options = $7;
>> + $$ = (Node *)n;
>> + }
>> ;
>
> ...
>
>> @@ -1218,6 +1229,10 @@ CreateOptRoleElem:
>> {
>> $$ = makeDefElem("addroleto", (Node *)$3, @1);
>> }
>> + | OWNER RoleSpec
>> + {
>> + $$ = makeDefElem("owner", (Node *)$2, @1);
>> + }
>> ;
>
> Not sure why we'd have both AUTHORIZATION and OWNER for CREATE ROLE..?
> We don't do that for other objects.

Good catch! The "OWNER RoleSpec" here was unused. I have removed it from the new patch set.

>> diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
>
>> @@ -1,6 +1,7 @@
>> -- ok, superuser can create users with any set of privileges
>> CREATE ROLE regress_role_super SUPERUSER;
>> CREATE ROLE regress_role_1 CREATEDB CREATEROLE REPLICATION BYPASSRLS;
>> +GRANT CREATE ON DATABASE regression TO regress_role_1;
>
> Seems odd to add this as part of this patch, or am I missing something?

It's not used much in patch 0001 where it gets introduced, but gets used more in patch 0002. I put it here to reduce the number of diffs the next patch creates.

>> From 1784a5b51d4dbebf99798b5832d92b0f585feb08 Mon Sep 17 00:00:00 2001
>> From: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
>> Date: Tue, 4 Jan 2022 11:42:27 -0800
>> Subject: [PATCH v4 3/5] Give role owners control over owned roles
>>
>> Create a role ownership hierarchy. The previous commit added owners
>> to roles. This goes further, making role ownership transitive. If
>> role A owns role B, and role B owns role C, then role A can act as
>> the owner of role C. Also, roles A and B can perform any action on
>> objects belonging to role C that role C could itself perform.
>>
>> This is a preparatory patch for changing how CREATEROLE works.
>
> This feels odd to have be an independent commit.

Reworked the v6-0002 and v6-0003 patches into just one, as discussed at the top of this email.

>> diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
>
>> @@ -363,7 +363,7 @@ AlterSchemaOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
>> /*
>> * must have create-schema rights
>> *
>> - * NOTE: This is different from other alter-owner checks in that the
>> + * NOTE: This is different from most other alter-owner checks in that the
>> * current user is checked for create privileges instead of the
>> * destination owner. This is consistent with the CREATE case for
>> * schemas. Because superusers will always have this right, we need
>
> Not a fan of just dropping 'most' in here, doesn't really help someone
> understand what is being talked about. I'd suggest adjusting the
> comment to talk about alter-owner checks for objects which exist in
> schemas, as that's really what is being referred to.

Yeah, that's a better approach. This next patch set changes the comment in both AlterSchemaOwner_internal and AlterRoleOwner_internal to make that clear.

...<snip>...

> Whoah, really? No, I don't agree with this, it's throwing away the
> entire concept around inheritance of role rights and how you can have
> roles which you can get the privileges of by doing a SET ROLE to them
> but you don't automatically have those rights.

I didn't change any of this for the next patch set, not because I'm ignoring you, but because we're still arguing out what the right behavior should be. Whatever we come up with, I think it should allow the use case that Robert has been talking about. Doing that and also doing what you are talking about might be hard, but I'm still hoping to find some solution.

Recall that upthread, months ago, we discussed that it is abnormal for any role to be a member of a login role. You can think of "login role" as a synonym for "user", and "non-login role" as a synonym for "group", and that language makes it easier to think about how weird it is for users to be members of other users.

It's perfectly sensible to have users own users, but not for users to be members of users. If not for that, I'd be in favor of what you suggest, excepting that I'd accommodate Robert's requirements by having the owner of a role have ADMIN on that role by default, with grammar for requesting the alternative. Maybe there is something I'm forgetting to consider just now, but I'd think that would handle Robert's "tenant" type argument while also making it easy to operate the way that you want. But, again, it does require having users be members of users, something which was rejected in the discussion months ago.

>> +/*
>> + * Is owner a direct or indirect owner of the role, not considering
>> + * superuserness?
>> + */
>> +bool
>> +is_owner_of_role_nosuper(Oid owner, Oid role)
>> +{
>> + return list_member_oid(roles_is_owned_by(role), owner);
>> +}
>
>
> Surely if you're a member of a role which owns another role, you should
> be considered to be an owner of that role too..? Just checking if the
> current role is a member of the roles which directly own the specified
> role misses that case.
>
> That is:
>
> CREATE ROLE r1;
> CREATE ROLE r2;
>
> GRANT r2 to r1;
>
> CREATE ROLE r3 AUTHORIZATION r2;
>
> Surely, r1 is to be considered an owner of r3 in this case, but the
> above check wouldn't consider that to be the case- it would only return
> true if the current role is r2.
>
> We do need some kind of direct membership check in the list of owners to
> avoid creating loops, so maybe this function is kept as that and the
> pg_role_ownership() check is changed to address the above case, but I
> don't think we should just ignore role membership when it comes to role
> ownership- we don't do that for any other kind of ownership check.

I like this line of reasoning, and it appears to be an argument in your favor where the larger question is concerned. If role ownership is transitive, and role membership is transitive, it gets weird trying to work out larger relationship chains.

This deserves more attention.

>> Subject: [PATCH v4 4/5] Restrict power granted via CREATEROLE.
>
> I would think this would be done independently of the other patches and
> probably be first.

The way I'm trying to fix CREATEROLE is first by introducing the concept of role owners, then second by restricting what roles can do based on whether they own a target role. I don't see how I can reverse the order.

>> diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
>
>> @@ -70,18 +70,18 @@ ALTER ROLE { <replaceable class="parameter">role_specification</replaceable> | A
>> <link linkend="sql-revoke"><command>REVOKE</command></link> for that.)
>> Attributes not mentioned in the command retain their previous settings.
>> Database superusers can change any of these settings for any role.
>> - Roles having <literal>CREATEROLE</literal> privilege can change any of these
>> - settings except <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>,
>> - and <literal>BYPASSRLS</literal>; but only for non-superuser and
>> - non-replication roles.
>> - Ordinary roles can only change their own password.
>> + Role owners can change any of these settings on roles they own except
>> + <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>, and
>> + <literal>BYPASSRLS</literal>; but only for non-superuser and non-replication
>> + roles, and only if the role owner does not alter the target role to have a
>> + privilege which the role owner itself lacks. Ordinary roles can only change
>> + their own password.
>> </para>
>
> Having contemplated this a bit more, I don't like it, and it's not how
> things work when it comes to regular privileges.
>
> Consider that I can currently GRANT someone UPDATE privileges on an
> object, but they can't GRANT that privilege to someone else unless I
> explicitly allow it. The same could certainly be said for roles-
> perhaps I want to allow someone the privilege to create non-login roles,
> but I don't want them to be able to create new login roles, even if they
> themselves have LOGIN.

This comment conflates privileges like LOGIN for which there isn't any "with grant option" logic with privileges that do. Granting someone UPDATE privileges on a relation will be tracked in an aclitem including whether the "with grant option" bit is set. Nothing like that will exist for LOGIN. I'm not dead-set against having that functionality for the privileges that currently lack it, but we'd have to do so in a way that doesn't gratuitously break backward compatibility, and how to do so has not been discussed.

> As another point, I might want to have an 'admin' role that I want
> admins to SET ROLE to before they go creating other roles, because I
> don't want them to be creating roles as their regular user and so that
> those other roles are owned by the 'admin' role, but I don't want that
> role to have the 'login' attribute.

Same problem. We don't have aclitem bits for this.

> In other words, we should really consider what role attributes a given
> role has to be independent of what role attributes that role is allowed
> to set on roles they create. I appreciate that "just whatever the
> current role has" is simpler and less work but also will be difficult to
> walk back from once it's in the wild.

I don't feel there is any fundamental disagreement here, except perhaps whether it needs to be done as part of this patch, vs. implemented in a future development cycle. We don't currently have any syntax for "CREATE ROLE bob LOGIN WITH GRANT OPTION". I can see some advantages in doing it all in one go, but also some advantage in being incremental. More discussion is needed here.

>> @@ -1457,7 +1449,7 @@ AddRoleMems(const char *rolename, Oid roleid,
>
>> /*
>> - * Check permissions: must have createrole or admin option on the role to
>> + * Check permissions: must be owner or have admin option on the role to
>> * be changed. To mess with a superuser role, you gotta be superuser.
>> */
>> if (superuser_arg(roleid))
>
> ...
>
>> @@ -1467,9 +1459,9 @@ AddRoleMems(const char *rolename, Oid roleid,
>> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> errmsg("must be superuser to alter superusers")));
>> }
>> - else
>> + else if (!superuser())
>> {
>> - if (!have_createrole_privilege() &&
>> + if (!pg_role_ownercheck(roleid, grantorId) &&
>> !is_admin_of_role(grantorId, roleid))
>> ereport(ERROR,
>> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>
> I'm not entirely sure about including owners here though I'm not
> completely against it either. This conflation of what the 'admin'
> privileges on a role means vs. the 'ownership' of a role is part of what
> I dislike about having two distinct systems for saying who is allowed to
> GRANT one role to another.
>
> Also, if we're going to always consider owners to be admins of roles
> they own, why not push that into is_admin_of_role()?

Unchanged in this patch set, but worth further discussion and evaluation.

>> Subject: [PATCH v4 5/5] Remove grantor field from pg_auth_members

...<snip>...

> If we're going to do this, it should also be done independently of the
> role ownership stuff too.

I've withdrawn 0005 from this patch set, and we can come back to it separately.

> Thanks,
>
> Stephen

Thanks for the review! I hope we can keep pushing this forward. Again, no offense is intended in having not addressed all your concerns in the v7 patch set: