Re: CREATEROLE and role ownership hierarchies

> On Jan 22, 2022, at 1:20 PM, Stephen Frost <sfrost@snowman.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
combinedwhat 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
makethe 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,
elsewe might get into an infinite loop or return the wrong answer.  These would only happen under corrupt conditions,
butit seems sensible to check for those, since they are cheap to check.  (Actually, the check for nontrivial cycles
includedin the patch is not as efficient as it could be, but I'm punting the work of improving that algorithm from
quadraticto 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,
thesecond is the owning role.  We could hardcode the message to say something like "bootstrap superuser owned by role
withOid %u", but that hardcodes "bootstrap superuser" into the message, rather than something like "stephen".  I don't
feelstrongly 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
thateach link in the hierarchy must be compatible with the expected structure.  It would be overkill to validate the
wholetree 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
obscureway 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
ownershipto 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
mightwant 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
theinformation, too. 

I'm not saying these are super strong arguments.  If people generally feel that CREATE ROLE ... AUTHORIZATION shouldn't
callSetUserIdAndSecContext, 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
thisexplicitly, but that's because it has already checked that the new owner is superuser, so the check must
necessarilysucceed.  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
explanationin functions which handle other object types?  I don't see why this one function would require the
explanationif 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
changeit to be less powerful, and (2) add a new built-in role, say "pg_create_role", and have membership in that role
bewhat we use.  Option (2) was generally viewed less favorably, or that was my sense of people's opinions, on the
theorythat 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
thenumber of diffs the next patch creates. 

>> From 1784a5b51d4dbebf99798b5832d92b0f585feb08 Mon Sep 17 00:00:00 2001
>> From: Mark Dilger <mark.dilger@enterprisedb.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_internalto 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
whatthe right behavior should be.  Whatever we come up with, I think it should allow the use case that Robert has been
talkingabout.  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
canthink of "login role" as a synonym for "user", and "non-login role" as a synonym for "group", and that language
makesit 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
favorof what you suggest, excepting that I'd accommodate Robert's requirements by having the owner of a role have ADMIN
onthat role by default, with grammar for requesting the alternative.  Maybe there is something I'm forgetting to
considerjust now, but I'd think that would handle Robert's "tenant" type argument while also making it easy to operate
theway that you want.  But, again, it does require having users be members of users, something which was rejected in
thediscussion 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.
Ifrole 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
whatroles 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
privilegesthat 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
WITHGRANT OPTION".  I can see some advantages in doing it all in one go, but also some advantage in being incremental.
Morediscussion 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
allyour concerns in the v7 patch set: 



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