Re: pg_auth_members.grantor is bunk

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_auth_members.grantor is bunk
Date: 2022-07-20 19:11:09
Message-ID: CA+Tgmob29PESpCrnZCX7LjiXwzMsibxdsiYpuSPhvcV45O=7TA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Jun 24, 2022 at 4:46 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jun 24, 2022 at 4:30 PM David G. Johnston
> <david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> >> Upthread, I proposed that "drop role baz" should fail here
> >
> > I concur with this.
> >
> > I think that the grantor owns the grant, and that REASSIGNED OWNED should be able to move those grants to someone else.
> >
> > By extension, DROP OWNED should remove them.
>
> Interesting. I hadn't thought about changing the behavior of DROP
> OWNED BY and REASSIGN OWNED BY. A quick experiment supports your
> interpretation:

This experiment was insufficiently thorough. I see now that, for other
object types, DROP OWNED BY does work in the way that you propose, but
REASSIGN OWNED BY does not. Here's a better test:

rhaas=# create table foo();
CREATE TABLE
rhaas=# create role bar;
CREATE ROLE
rhaas=# create role baz;
CREATE ROLE
rhaas=# grant select on table foo to bar with grant option;
GRANT
rhaas=# set role bar;
SET
rhaas=> grant select on table foo to baz;
GRANT
rhaas=> reset role;
RESET
rhaas=# drop role bar;
ERROR: role "bar" cannot be dropped because some objects depend on it
DETAIL: privileges for table foo
rhaas=# create role quux;
CREATE ROLE
rhaas=# reassign owned by bar to quux;
REASSIGN OWNED
rhaas=# drop role bar;
ERROR: role "bar" cannot be dropped because some objects depend on it
DETAIL: privileges for table foo
rhaas=# drop owned by bar;
DROP OWNED
rhaas=# drop role bar;
DROP ROLE

This behavior might look somewhat bizarre, but there's actually a good
reason for it: the system guarantees that whoever is listed as the
grantor of a privilege has the *current* right to grant that
privilege. It can't categorically change the grantor of every
privilege given by bar to quux because quux might not and in fact does
not have the right to grant select on table foo to baz. Now, you might
be thinking, ah, but what if the superuser performed the grant? They
could cease to be the superuser later, and then the rule would be
violated! But actually not, because a grant by the superuser is
imputed to the table owner, who always has the right to grant all
rights on the table, and if the table owner is ever changed, all the
grants imputed to the old table owner are changed to have their
grantor as the new table owner. Similarly, trying to revoke select, or
the grant option on it, from bar would fail. So it looks pretty
intentional, and pretty tightly-enforced, that every role listed as a
grantor must be one which is currently able to grant that privilege.

And that means that REASSIGN OWNED can't just do a blanket change to
the recorded grantor. It could try to do so, I suppose, and just throw
an error if it doesn't work out, but that might make REASSIGN OWNED
fail a lot more often, which could suck. In any event, the implemented
behavior is that REASSIGN OWNED does nothing about permissions, but
DROP OWNED cascades to grantors. This is SORT OF documented, although
the documentation only mentions that DROP OWNED cascades to privileges
granted *to* the target role, and does not mention that it also
cascades to privileges granted *by* the target role.

The previous version of the patch makes both DROP OWNED and REASSIGN
OWNED cascade to grantors, but I now think that, for consistency, I'd
better look into changing it so that only DROP OWNED cascades. I think
perhaps I should be using SHARED_DEPENDENCY_ACL instead of
SHARED_DEPENDENCY_OWNER.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2022-07-20 19:46:46 Re: Is select_outer_pathkeys_for_merge() too strict now we have Incremental Sorts?
Previous Message Andres Freund 2022-07-20 19:08:49 Re: shared-memory based stats collector - v70