Date: Tue, 30 Aug 2022 10:30:17 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: tushar <tushar.ahuja@enterprisedb.com>, Joe Conway <mail@joeconway.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Stephen Frost <sfrost@snowman.net>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: replacing role-level NOINHERIT with a grant-level option
Message-ID: <20220830173017.GB544233@nathanxps13>
References: <bf371eff-a98b-2b8f-6c28-95c7f3cdd523@enterprisedb.com>
 <CA+Tgmob6pVopLZ+NJuy_AxDS0uvgHhjjEBYQTBZXek-vM9dmGw@mail.gmail.com>
 <030c1dbf-5a8e-706d-698c-b1cdfcb6ce0f@enterprisedb.com>
 <CA+TgmobWFL5eU=60egYuzL0C0cgaVFEOVriTk_Rbs_cpj=MreQ@mail.gmail.com>
 <dab97d44-3859-f58f-ff4c-aa79d3278869@enterprisedb.com>
 <CA+TgmoYkbcH=njVN-vbRDorj9VqTGNzGHCYizO_5k1RWxGOQfQ@mail.gmail.com>
 <CA+TgmoaKMnde2W_=u7CqeCKi=FKnfbNQPwOR=c_3c8qD7b2nhQ@mail.gmail.com>
 <20220828211421.GA213902@nathanxps13>
 <CA+TgmobWjtFEsZ0vfNoCJVipB6WVc5BUPYq9m8eEE-4zMkgzug@mail.gmail.com>
 <CA+TgmoZSU1h6hU4u_T_fMbefEaKWyaWjreubuyGeR057dPnLkg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoZSU1h6hU4u_T_fMbefEaKWyaWjreubuyGeR057dPnLkg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220830173017.GB544233%40nathanxps13>
Precedence: bulk

On Mon, Aug 29, 2022 at 03:38:57PM -0400, Robert Haas wrote:
> Argh, I found a bug, and one that I should have caught during testing,
> too. I modelled the new function select_best_grantor() on
> is_admin_of_role(), but it differs in that it calls
> roles_is_member_of() with ROLERECURSE_PRIVS rather than
> ROLECURSE_MEMBERS. Sadly, roles_is_member_of() handles
> ROLERECURSE_PRIVS by completely ignoring non-inherited grants, which
> is wrong, because then calls to select_best_grantor() treat a member
> of a role with INHERIT FALSE, ADMIN TRUE is if they were not an admin
> at all, which is incorrect.

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.

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

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

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com