MIME-Version: 1.0
References: 
 <CAOYmi+=KTLd+XsEP=TDiZ48iVf-CEc7JrZd5uhWPYWKEfOgyyQ@mail.gmail.com>
 <CAOYmi+n_hw=SC5V1i3BmqfZPfPBRaUSJc+BeOXEKDwRue+WYrg@mail.gmail.com>
In-Reply-To: 
 <CAOYmi+n_hw=SC5V1i3BmqfZPfPBRaUSJc+BeOXEKDwRue+WYrg@mail.gmail.com>
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Date: Mon, 6 Jul 2026 16:33:03 +0530
Message-ID: 
 <CAJTYsWXbrewUuiMekCmah3V3fA+ZvcASyV6CJgaUQ2Wc-M0MLw@mail.gmail.com>
Subject: Re: REVOKE's CASCADE protection doesn't work with INHERITed table
 owners
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000874c2b0655ef383d"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJTYsWXbrewUuiMekCmah3V3fA%2BZvcASyV6CJgaUQ2Wc-M0MLw%40mail.gmail.com>
Precedence: bulk

--000000000000874c2b0655ef383d
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Fri, 26 Jun 2026 at 05:43, Jacob Champion <
jacob.champion@enterprisedb.com> wrote:

> [moving to -hackers]
>
> On Wed, Jun 24, 2026 at 2:57=E2=80=AFPM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
> > TL;DR: The protection in recursive_revoke() against broken GRANT
> > OPTION chains doesn't seem to work properly when the grantee also
> > holds the privileges of the grantor.
>
> More accurately: "when an intermediate grantor in the chain only
> indirectly holds the ability to grant."
>
> > I think the issue is in recursive_revoke()'s usage of aclmask(), which
> > in turn uses has_privs_of_role(). It doesn't seem like that's what was
> > wanted in this particular case... thoughts?
>
> I propose changing that to aclmask_direct(), as in the attached, and
> backpatching all the way down.
>
> To try to prove to myself that this works, I added tests to pin each
> of the three cases that are treated differently by aclmask_direct():
> 1. the grantor has indirect ownership privileges
> 2. the grantor has indirect grant options via INHERIT
> 3. the grantor has indirect grant options via PUBLIC (which is already
> disallowed in practice)
>

Thanks, this looks right to me.  I traced recursive_revoke() and the
aclmask() -> aclmask_direct() switch makes sense to me, since a
grant is only ever attributed to a role that holds the option directly
(or the owner), it seems right that recursive_revoke() should judge
"still has it" the same way, rather than counting inherited/superuser
access?  The three new test cases line up with that too.

I also tried to expand the existing comment, both to point out the
> pitfall and to explain why the short-circuit works. But I've rewritten
> it at least a dozen times, so if anyone can tell me whether I've made
> sense and/or used the terminology appropriately, I'd appreciate it.


One tiny comment question: the phrase "granted by any role on the chain"
in the new comment reads a little oddly to me, would something like
"still holds the option directly via another grantor" be closer to what
the code checks?  Could just be me misreading it.

Apart from the above, patch LGTM.

> I'm pretty sure the following is unintended behavior. It looks
> > potentially related to [1] as well.
>
> (To fix [1] I suspect we need to make a similar tweak to
> check_circularity(), but I haven't looked into that yet.)
>
> [1]
> https://postgr.es/m/CAM6Zo8wD7RtQNhbQHODc9DobiW+GpT=3DtnqOSMz4+mnzA9m0zMg=
@mail.gmail.com


On check_circularity() for [1]: I tried the same aclmask_direct() swap,
but since it runs on every GRANT ... WITH GRANT OPTION, which
pg_dump/restore replays, erroring there could make restore/pg_upgrade of
an existing cluster (one already holding the [1] self-grant) fail. Feels
like it may need a companion dump/restore change first.

Regards,
Ayush

--000000000000874c2b0655ef383d
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi,</div><br><div class=3D"gmail_quote gm=
ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, 26 Jun 2=
026 at 05:43, Jacob Champion &lt;<a href=3D"mailto:jacob.champion@enterpris=
edb.com">jacob.champion@enterprisedb.com</a>&gt; wrote:<br></div><blockquot=
e class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px s=
olid rgb(204,204,204);padding-left:1ex">[moving to -hackers]<br>
<br>
On Wed, Jun 24, 2026 at 2:57=E2=80=AFPM Jacob Champion<br>
&lt;<a href=3D"mailto:jacob.champion@enterprisedb.com" target=3D"_blank">ja=
cob.champion@enterprisedb.com</a>&gt; wrote:<br>
&gt; TL;DR: The protection in recursive_revoke() against broken GRANT<br>
&gt; OPTION chains doesn&#39;t seem to work properly when the grantee also<=
br>
&gt; holds the privileges of the grantor.<br>
<br>
More accurately: &quot;when an intermediate grantor in the chain only<br>
indirectly holds the ability to grant.&quot;<br>
<br>
&gt; I think the issue is in recursive_revoke()&#39;s usage of aclmask(), w=
hich<br>
&gt; in turn uses has_privs_of_role(). It doesn&#39;t seem like that&#39;s =
what was<br>
&gt; wanted in this particular case... thoughts?<br>
<br>
I propose changing that to aclmask_direct(), as in the attached, and<br>
backpatching all the way down.<br>
<br>
To try to prove to myself that this works, I added tests to pin each<br>
of the three cases that are treated differently by aclmask_direct():<br>
1. the grantor has indirect ownership privileges<br>
2. the grantor has indirect grant options via INHERIT<br>
3. the grantor has indirect grant options via PUBLIC (which is already<br>
disallowed in practice)<br></blockquote><div><br>Thanks, this looks right t=
o me.=C2=A0 I traced recursive_revoke() and the<br>aclmask() -&gt; aclmask_=
direct() switch makes sense to me, since a <br>grant is only ever attribute=
d to a role that holds the option directly <br>(or the owner), it seems rig=
ht that recursive_revoke() should judge <br>&quot;still has it&quot; the sa=
me way, rather than counting inherited/superuser <br>access?=C2=A0 The thre=
e new test cases line up with that too.=C2=A0 =C2=A0<br><br></div><blockquo=
te class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px =
solid rgb(204,204,204);padding-left:1ex">
I also tried to expand the existing comment, both to point out the<br>
pitfall and to explain why the short-circuit works. But I&#39;ve rewritten<=
br>
it at least a dozen times, so if anyone can tell me whether I&#39;ve made<b=
r>
sense and/or used the terminology appropriately, I&#39;d appreciate it.</bl=
ockquote><div>=C2=A0<br>One tiny comment question: the phrase &quot;granted=
 by any role on the chain&quot;<br>in the new comment reads a little oddly =
to me, would something like<br>&quot;still holds the option directly via an=
other grantor&quot; be closer to what<br>the code checks?=C2=A0 Could just =
be me misreading it.<br><br>Apart from the above, patch LGTM.<br><br></div>=
<blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-=
left:1px solid rgb(204,204,204);padding-left:1ex">
&gt; I&#39;m pretty sure the following is unintended behavior. It looks<br>
&gt; potentially related to [1] as well.<br>
<br>
(To fix [1] I suspect we need to make a similar tweak to<br>
check_circularity(), but I haven&#39;t looked into that yet.)<br>
<br>
[1] <a href=3D"https://postgr.es/m/CAM6Zo8wD7RtQNhbQHODc9DobiW+GpT=3DtnqOSM=
z4+mnzA9m0zMg@mail.gmail.com" rel=3D"noreferrer" target=3D"_blank">https://=
postgr.es/m/CAM6Zo8wD7RtQNhbQHODc9DobiW+GpT=3DtnqOSMz4+mnzA9m0zMg@mail.gmai=
l.com</a></blockquote><div><br>On check_circularity() for [1]: I tried the =
same aclmask_direct() swap,<br>but since it runs on every GRANT ... WITH GR=
ANT OPTION, which<br>pg_dump/restore replays, erroring there could make res=
tore/pg_upgrade of<br>an existing cluster (one already holding the [1] self=
-grant) fail. Feels<br>like it may need a companion dump/restore change fir=
st.<br><br>Regards,<br>Ayush=C2=A0</div></div></div>

--000000000000874c2b0655ef383d--