Re: replacing role-level NOINHERIT with a grant-level option

On Tue, Aug 30, 2022 at 1:30 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> I've been staring at this stuff for a while, and I'm still rather confused.
>
> * You mention that you modelled the "new" function select_best_grantor() on
> is_admin_of_role(), but it looks like that function was introduced in 2005.
> IIUC you meant select_best_admin(), which was added much more recently.

Well, a combination of the two. It's modeled after is_admin_of_role()
in the sense that it also calls roles_is_member_of() with a
non-InvalidOid third argument and a non-NULL fourth argument. All
other callers pass InvalidOid/NULL.

> * I don't see why we should ignore inheritance until after checking admin
> option. Prior to your commit (e3ce2de), we skipped checking for admin
> option if the role had [NO]INHERIT, and AFAICT your commit aimed to retain
> that behavior. The comment above check_role_grantor() even mentions
> "inheriting the privileges of a role which does have ADMIN OPTION." Within
> the function, I see the following comment:
>
> * Otherwise, the grantor must either have ADMIN OPTION on the role or
> * inherit the privileges of a role which does. In the former case,
> * record the grantor as the current user; in the latter, pick one of
> * the roles that is "most directly" inherited by the current role
> * (i.e. fewest "hops").
>
> So, IIUC, the intent is for ADMIN OPTION to apply even if for non-inherited
> grants, but we still choose to recurse in roles_is_member_of() based on
> inheritance. This means that you can inherit the ADMIN OPTION to some
> degree, but if you have membership WITH INHERIT FALSE to a role that has
> ADMIN OPTION on another role, the current role effectively does not have
> ADMIN OPTION on the other role. Is this correct?

I think it's easiest if I get an example. Suppose role 'william'
inherits 'charles' and 'diana' and role 'charles' in turn inherits
from roles 'elizabeth' and 'philip'. Furthermore, 'william' has ADMIN
OPTION on 'cambridge', 'charles' on 'wales', and 'elizabeth' on 'uk'.
Now, because 'william' has ADMIN OPTION on role 'cambridge', he can
add members to that role and remove the ones he added. He can also
grant admin out option to others and later remove it (and all
dependent grants that those others have made). When he does this, he
is acting as himself, on his own authority. Because he inherits the
privileges of charles directly and of elizabeth via charles, he can
also add members to wales and uk. But if does this, he is not acting
as himself, because he himself does not possess the ADMIN OPTION on
either of those roles. Rather, he is acting on authority delegated
from some other role which does possess admin option on those roles.
Therefore, if 'william' adds a member to 'uk', the grantor MUST be
recorded as 'elizabeth', not 'william', because the grantor of record
must possess admin option on the role.

Now let's add another layer of complexity. Suppose that the 'uk' role
possesses ADMIN OPTION on some other role, let's say 'parliament'. Can
'william' use the fact that he inherits from 'elizabeth' to grant
membership in 'parliament'? That all depends on whether 'uk' was
granted to 'elizabeth' WITH INHERIT TRUE or WITH INHERIT FALSE. If it
was granted WITH INHERIT TRUE, then elizabeth freely exercises all the
powers of parliament, william freely exercises all of elizabeth's
powers, and thus william can grant membership in parliament. Such a
membership will be recorded as having been granted by uk, the role
that actually holds ADMIN OPTION on parliament. However, if elizabeth
was granted uk WITH INHERIT FALSE, then she can't mess with the
membership of parliament, and thus neither can william. At least, not
without first executing "SET ROLE uk", which in this scenario, either
could do, but perhaps the use of SET ROLE is logged, audited, and very
carefully scrutinized.

So, what does all of this mean for select_best_grantor() and its
helper function roles_is_member_of()? Well, first, we have to recurse.
william can act on his own behalf when administering cambridge, and on
behalf of charles or elizabeth when administering wales or uk
respectively, and there's no way to get that behavior without
recursing. Second, we have to stop the recursion when we reach a grant
that is not inherited. Otherwise, william might be able to act on
behalf of uk and administer membership in parliament even if uk is
granted to elizabeth WITH INHERIT FALSE. Third, and this is where it
gets tricky, we have to stop recursion *only after checking whether
we've found a valid grantor*. william is allowed to administer
membership cambridge whether or not it is granted to william WITH
INHERIT TRUE or WITH INHERIT FALSE. Either way, he possess ADMIN
OPTION on that role, and may administer membership in it. Likewise, he
can administer membership in wales or uk as charles or elizabeth,
respectively, because he definitely inherits the privileges of roles
which definitely have ADMIN OPTION on those roles.

Maybe a clearer way to look at is this: we're going to transit a
series of role-membership links going from william (who attempts some
operation) to the role in which he's trying to grant (or revoke)
membership. For the operation to succeed, we need every link but the
last in the chain to have INHERIT TRUE, and we need the last link in
the chain to have ADMIN TRUE. We don't care whether the links other
than the last one have ADMIN, and we don't care whether the last link
has INHERIT. Thus:

- william => cambridge is OK because the one and only grant involved
has ADMIN, whether or not it has INHERIT
- william => charles => wales is OK because the first hop has INHERIT
and the second hop has ADMIN
- william => charles => elizabeth => uk is OK because the first two
hops have ADMIN and the last hop has INHERIT
- william => charles => elizabeth => uk => parliament is probably not
OK because although the first two hops have ADMIN and the last has
INHERIT, the third hop probably lacks INHERIT

I hope this makes it clearer. select_best_grantor() can't completely
disregard links without INHERIT, because if it does, it can't pay any
attention to the last grant in the chain, which only needs to have
ADMIN, not INHERIT. But it must pay some attention to them, because
every earlier link in the chain does need to have INHERIT. By moving
the if-test up, the patch makes it behave just that way, or at least,
I think it does.

> As I'm writing this down, I think it's beginning to make more sense to me,
> so this might just be a matter of under-caffeination. But perhaps some
> extra comments or documentation wouldn't be out of the question...

I do not rule that out!

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