Re: Backward compat issue with v16 around ROLEs

On Thu, Sep 12, 2024 at 3:40 PM Dominique Devienne <ddevienne(at)gmail(dot)com> wrote:
> Another way to look at it is this:
>
> === v14 ===
> ddevienne=> create role dd_child;
> CREATE ROLE
> ddevienne=> select pg_has_role(current_role, 'dd_child', 'MEMBER');
> pg_has_role
> -------------
> f
> (1 row)
>
> === v16 ===
> ddevienne=> create role dd_child;
> CREATE ROLE
> ddevienne=> select pg_has_role(current_role, 'dd_child', 'MEMBER');
> pg_has_role
> -------------
> t
> (1 row)
>
> Any existing ROLE graph which had "back-edges" (GRANTs) from a ROLE
> back to the ROLE that created it, valid in pre-v16, becomes invalid in v16+.
> And there's no work-around. Tough luck, take a hike...
>
> And our security model and its implementation basically requires such
> back-edges.
>
> My contention is that if this is an ADMIN-only edge, it shouldn't be
> deemed circular.
> Kind of the same way you break cycles in FKs by making one side DEFERRED,
> ADMIN edges should be "weaker" than SET ones, and break cycles.
>
> Maybe I'm the only one in the world using PostgreSQL in that situation?
> Somehow I doubt that. Most people and organization are slow to upgrade,
> and v16 is new enough that it wasn't exposed to enough real world usage yet.
> So this is issue is only get bigger as time passes IMHO.

Hi,

I don't normally read pgsql-general, but Tom Lane drew my attention to
this thread. I made the changes that you're complaining about here, so
everything you're unhappy about is my fault. First of all, I'm sorry
that you're frustrated. I was aware that there was a possibility that
some people's use cases were going to get broken by these changes, and
I tried to minimize the amount of stuff that would get broken, but
it's still a bummer to hear that the situation is so frustrating for
you. Second, it's over a year too late to think about making any
changes to the behavior of v16. Even if everything I did here is
terrible, we're kind of stuck with it at this point. We can fix bugs,
but it's too late to redefine the semantics of a released branch.
However, I don't think that what I did here is terrible, because
CREATEROLE is totally insecure in earlier releases. If I understand
your setup correctly, dd_user can do this:

set role dd_owner;
grant pg_execute_server_program to dd_user;
grant pg_write_server_files to dd_user;

At this point, dd_user should easily be able to make themselves
superuser, or just erase/corrupt the entire database cluster. In other
words, in v14, every account that has CREATEROLE and every account
that can SET ROLE to an account that has CREATEROLE can very easily
escalate to superuser. So I guess I would respectfully disagree with
the idea that this works on v14 and v16 broke it. It doesn't really
work on v14, or at least not any better than just using SUPERUSER
everywhere that you're currently using CREATEROLE. And if you choose
to do that, I think your example will work pretty much the same way on
v16 as it does on v14.

But that is not to say that you don't raise a good point here. The
prohibition against circular grants is really annoying in your use
case. If dd_owner creates dd_user, then dd_user is granted to
dd_owner, which means that dd_owner cannot be granted (directly or
indirectly) to dd_user. The reason why we have this sort of
prohibition is that we don't want to end up with grants that become
disconnected from their original source. Suppose the boss grants a
privilege to alice, and later revokes it. But suppose that between the
time the privilege is granted and the time it is revoked, alice grants
the privilege to bob, and bob in turn grants it back to alice. Now,
when the boss revokes the privilege, alice still has it, because she's
still getting it from bob, who in turn is getting it from her. This
would be very bad: the boss has every right to expect that when they
revoke a privilege they have previously granted, the former recipient
no longer has it. The prohibition against circular grants keeps this
from happening.

And we now apply that same principle to role grants. Consider this:

robert.haas=# create role alice createrole;
CREATE ROLE
robert.haas=# set session authorization alice;
SET
robert.haas=> create role accounting;
CREATE ROLE
robert.haas=> create role bob;
CREATE ROLE
robert.haas=> grant accounting to bob;
GRANT ROLE
robert.haas=> set session authorization bob;
SET
robert.haas=> set role accounting;
SET
robert.haas=> reset session authorization;
RESET
robert.haas=# drop role alice;
ERROR: role "alice" cannot be dropped because some objects depend on it
DETAIL: privileges for membership of role bob in role accounting

Privilege grants made by alice cannot survive alice's termination.
Prior to v16, this principle applied to grants of everything except
role; now, it also applies to role grants. Whether that's correct is
an arguable point, but it seems very strange to me to argue that role
grants should work differently from every other type of grant in the
system, and it does have some nice properties. But that means that the
anti-circularity provisions that we apply in other cases also need to
be applied to roles. Otherwise, in your example, if the ddevienne role
were removed, dd_admin and dd_owner would retain the ability to
administer each other even though that grant would now have no source.
That administrative authority would have come from ddevienne
originally but, by making a set of circular grants, dd_admin and
dd_owner could arrange to retain that privilege even after ddevienne
was gone. We now forbid that just as we do for other object types.

However, it seems like we might be able to fix this by just making the
code smarter. Maybe there's a problem that I'm not seeing, but if the
boss grants a privilege to alice and alice grants it to bob and bob
grants it back to alice and then the boss revokes the privilege, why
can't we figure out that alice no longer has a source for that
privilege *aside from the one involved in the cycle* and undo the
reciprocal grants that bob and alice made to each other? Right now I
believe we just ask "is the number of sources that alices has for this
privilege still greater than zero" which only works if there are no
cycles but maybe we can do better. We'd probably need to think
carefully about concurrency issues, though, and whether pg_dump is
smart enough to handle this case. Also, there are separate code paths
for role grants and non-role grants, and since I went to a lot of
trouble to make them work the same way, I'd really prefer it if we
didn't go back to having them work differently...

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