Re: pg_auth_members.grantor is bunk

On Sun, Jul 31, 2022 at 2:18 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Thanks for working on this.

Thanks for the review.

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

Yes. The previous paragraph in this commit message seems to cover this
point pretty thoroughly.

> <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'?

Will fix.

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

Oh right. That was a note to myself to look into that more. And then I
didn't. I'll look into that more and report back.

> @@ -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?

Will fix.

> @@ -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?

I don't believe there's anything novel here - at least there isn't
supposed to be. Here's the equivalent with table privileges:

rhaas=# create table t1();
CREATE TABLE
rhaas=# create role foo;
CREATE ROLE
rhaas=# create role bar;
CREATE ROLE
rhaas=# grant select on t1 to foo with grant option;
GRANT
rhaas=# set role foo;
SET
rhaas=> grant select on t1 to bar with grant option;
GRANT
rhaas=> set role bar;
SET
rhaas=> grant select on t1 to foo with grant option;
ERROR: grant options cannot be granted back to your own grantor

> + 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?

Do we guarantee that the regression tests are running as the bootstrap
superuser, or just as some superuser? I am a bit reluctant to add a
regression test that assumes the former unless we're assuming it
already. For 'make check' it doesn't matter but 'make installcheck' is
another story.

The memberid == BOOTSTRAP_SUPERUSERID case is very much intentional.
The code will detect any loops in the catalog, but the implicit grant
to the bootstrap superuser doesn't exist in the catalog
representation, so it needs a separate check. I think I should sync
the two error messages though, i.e. this should say "admin options
cannot be granted back to your own grantor" like the other one just
below.

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

I think it's a bit off too, but I didn't invent it. See check_circularity():

if ((ACLITEM_GET_GOPTIONS(*mod_aip) & ~own_privs) != 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
errmsg("grant options cannot be granted back to your
own grantor")));

Looks like Tom Lane, vintage 2004, 4b2dafcc0b1a579ef5daaa2728223006d1ff98e9.

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

Will fix.

> @@ -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.

I'm pretty sure you are right.

> +/*
> + * 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.

I'll think about this some more, but I don't want to commit to
changing it very much. IMHO, the whole split into AddRoleMems() and
DelRoleMems() for what is basically the same operation seems pretty
dubious, but this commit's intended purpose is to clean up the
behavior rather than to rewrite the code. So I left the existing logic
in AddRoleMems() and DelRoleMems() alone, and when I realized I needed
something else that was mostly common to both, I made this function
instead of duplicating the logic in two places. I realize there are
other ways that it could be split up, and maybe some of those are
better in theory, but they'd likely also expand the scope of the patch
to things that it doesn't quite need to touch. I'm not real keen to go
there. That can be done later, in a separate patch, or never, and I
don't think we'll really be any the worse for it.

> +/*
> + * 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"?

It seems completely fine to me, but I'll change it somehow to avoid
annoying you. :-)

--
Robert Haas
EDB: http://www.enterprisedb.com