Re: pg_auth_members.grantor is bunk

Greetings,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Jul 26, 2022 at 12:46 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > I believe that these patches are mostly complete, but I think that
> > dumpRoleMembership() probably needs some more work. I don't know what
> > exactly, but there's nothing to cause it to dump the role grants in an
> > order that will create dependent grants after the things that they
> > depend on, which seems essential.
>
> OK, so I fixed that, and also updated the documentation a bit more. I
> think these patches are basically done, and I'd like to get them
> committed before too much more time goes by, because I have other
> things that depend on this which I also want to get done for this
> release. Anybody object?

Thanks for working on this.

Subject: [PATCH v3 1/2] Ensure that pg_auth_members.grantor is always valid.

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 94135fdd6b..258943094a 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -919,7 +920,7 @@ DropRole(DropRoleStmt *stmt)

/*
* Scan the pg_authid relation to find the Oid of the role(s) to be
- * deleted.
+ * deleted and perform prleliminary permissions and sanity checks.

Should be preliminary, I'm guessing.

Overall, this looks like a solid improvement.

Subject: [PATCH v3 2/2] Make role grant system more consistent with other
privileges.

> Previously, only the superuser could specify GRANTED BY with a user
> other than the current user. Relax that rule to allow the grantor
> to be any role whose privileges the current user posseses. This
> doesn't improve compatibility with what we do for other object types,
> where support for GRANTED BY is entirely vestigial, but it makes this
> feature more usable and seems to make sense to change at the same time
> we're changing related behaviors.

Presumably the GRANTED BY user in this case still has to have the
ability to have performed the GRANT themselves? Looks that way below
and it's just the commit message, but was the first question that came
to mind when I read through this.

diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index f744b05b55..1f828d386a 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -267,8 +267,14 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace

<para>
If <literal>GRANTED BY</literal> is specified, the grant is recorded as
- having been done by the specified role. Only database superusers may
- use this option, except when it names the same role executing the command.
+ having been done by the specified role. A user can only attribute a grant
+ to another role if they possess the privileges of that role. A role can
+ only be recorded as a grantor if has <literal>ADMIN OPTION</literal> on

Should be: if they have

+ a role or is the bootstrap superuser. When a grant is recorded as having

on *that* role seems like it'd be better. And maybe 'or if they are the
bootstrap superuser'?

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 258943094a..8ab2fecf3a 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -805,11 +842,12 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
if (stmt->action == +1) /* add members to role */
AddRoleMems(rolename, roleid,
rolemembers, roleSpecsToIds(rolemembers),
- GetUserId(), false);
+ InvalidOid, false);
else if (stmt->action == -1) /* drop members from role */
DelRoleMems(rolename, roleid,
rolemembers, roleSpecsToIds(rolemembers),
- false);
+ InvalidOid, false, DROP_RESTRICT); /* XXX sketchy - hint
+ * may mislead */
}

This comment seems a little concerning..? Also isn't very clear.

@@ -1027,7 +1065,7 @@ DropRole(DropRoleStmt *stmt)

while (HeapTupleIsValid(tmp_tuple = systable_getnext(sscan)))
{
- Form_pg_auth_members authmem_form;
+ Form_pg_auth_members authmem_form;

authmem_form = (Form_pg_auth_members) GETSTRUCT(tmp_tuple);
deleteSharedDependencyRecordsFor(AuthMemRelationId,

Some random whitespace changes that seems a bit odd given that they
should have been already correct thanks to pgindent- will these end up
just getting undone again?

@@ -1543,14 +1578,94 @@ AddRoleMems(const char *rolename, Oid roleid,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
errmsg("role \"%s\" is a member of role \"%s\"",
rolename, get_rolespec_name(memberRole))));
+ }
+
+ /*
+ * Disallow attempts to grant ADMIN OPTION back to a user who granted it
+ * to you, similar to what check_circularity does for ACLs. We want the
+ * chains of grants to remain acyclic, so that it's always possible to use
+ * REVOKE .. CASCADE to clean up all grants that depend on the one being
+ * revoked.
+ *
+ * NB: This check might look redundant with the check for membership
+ * loops above, but it isn't. That's checking for role-member loop (e.g.
+ * A is a member of B and B is a member of A) while this is checking for
+ * a member-grantor loop (e.g. A gave ADMIN OPTION to X to B and now B, who
+ * has no other source of ADMIN OPTION on X, tries to give ADMIN OPTION
+ * on X back to A).
+ */

With this exact scenario, wouldn't it just be a no-op as A must have
ADMIN OPTION already on X? The spec says that no cycles of role
authorizations are allowed. Presumably we'd continue this for other
GRANT'able things which can be further GRANT'd (should we add them) in
the future? Just trying to think ahead a bit here in case it's
worthwhile. Those would likely be ABC WITH GRANT OPTION too, right?

+ if (admin_opt && grantorId != BOOTSTRAP_SUPERUSERID)
+ {
+ CatCList *memlist;
+ RevokeRoleGrantAction *actions;
+ int i;
+
+ /* Get the list of members for this role. */
+ memlist = SearchSysCacheList1(AUTHMEMROLEMEM,
+ ObjectIdGetDatum(roleid));
+
+ /*
+ * Figure out what would happen if we removed all existing grants to
+ * every role to which we've been asked to make a new grant.
+ */
+ actions = initialize_revoke_actions(memlist);
+ foreach(iditem, memberIds)
+ {
+ Oid memberid = lfirst_oid(iditem);
+
+ if (memberid == BOOTSTRAP_SUPERUSERID)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("grants with admin options cannot be circular")));
+ plan_member_revoke(memlist, actions, memberid);
+ }

I don't see a regression test added which produces the above error
message. The memberid == BOOTSTRAP_SUPERUSERID seems odd too?

+ /*
+ * If the result would be that the grantor role would no longer have
+ * the ability to perform the grant, then the proposed grant would
+ * create a circularity.
+ */
+ for (i = 0; i < memlist->n_members; ++i)
+ {
+ HeapTuple authmem_tuple;
+ Form_pg_auth_members authmem_form;
+
+ authmem_tuple = &memlist->members[i]->tuple;
+ authmem_form = (Form_pg_auth_members) GETSTRUCT(authmem_tuple);
+
+ if (actions[i] == RRG_NOOP &&
+ authmem_form->member == grantorId &&
+ authmem_form->admin_option)
+ break;
+ }
+ if (i >= memlist->n_members)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("admin options cannot be granted back to your own grantor")));

I do see this in the regression tests. There though, the GRANTs are
being performed by someone else, so saying 'your' isn't quite right.
I'm trying to get this review out sooner than later and so I might be
missing something, but looking at the regression test for this and these
error messages, feels like the 'circular' error message makes more sense
than the 'your own grantor' message that actually ends up being
returned in that regression test.

@@ -1637,17 +1737,22 @@ AddRoleMems(const char *rolename, Oid roleid,
* roleid: OID of role to del from
* memberSpecs: list of RoleSpec of roles to del (used only for error messages)
* memberIds: OIDs of roles to del
+ * grantorId: who is revoking the membership
* admin_opt: remove admin option only?
*/
static void
DelRoleMems(const char *rolename, Oid roleid,
List *memberSpecs, List *memberIds,
- bool admin_opt)
+ Oid grantorId, bool admin_opt, DropBehavior behavior)

The comment above DropRoleMems missed adding a description for
the 'behavior' parameter.

@@ -1669,40 +1774,69 @@ DelRoleMems(const char *rolename, Oid roleid,
+ /*
+ * We may need to recurse to dependent privileges if DROP_CASCADE was
+ * specified, or refuse to perform the operation if dependent privileges
+ * exist and DROP_RECURSE was specified. plan_single_revoke() will
+ * figure out what to do with each catalog tuple.
+ */

Pretty sure that should be DROP_RESTRICT, not DROP_RECURSE.

+/*
+ * Sanity-check, or infer, the grantor for a GRANT or REVOKE statement
+ * targeting a role.
+ *
+ * The grantor must always be either a role with ADMIN OPTION on the role in
+ * which membership is being granted, or the bootstrap superuser. This is
+ * similar to the restriction enforced by select_best_grantor, except that
+ * roles don't have owners, so we regard the bootstrap superuser as the
+ * implicit owner.
+ *
+ * The return value is the OID to be regarded as the grantor when executing
+ * the operation.
+ */
+static Oid
+check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId, bool is_grant)

As this also does some permission checks, it seems like it'd be good to
mention that in the function description. Maybe also in the places that
call into this function with the expectation that the privilege check
will be taken care of here.

Indeed, I wonder if maybe we should really split this function into two
as the "give me what the best grantor is" is a fair bit different from
"check if this user has permission to grant as this role". As noted in
the comments, the current function also only does privilege checking in
some case- when InvalidOid is passed in we've also already done
permissions checks to make sure that the GRANT will succeed.

+/*
+ * Initialize an array of RevokeRoleGrantAction objects.
+ *
+ * 'memlist' should be a list of all grants for the target role.
+ *
+ * We here construct an array indicating that no actions are to be performed;
+ * that is, every element is intiially RRG_NOOP.
+ */

"We here construct" seems odd wording to me. Maybe "Here we construct"?

Thanks,

Stephen