Re: pg_auth_members.grantor is bunk

On Fri, Sep 2, 2022 at 6:01 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> > Yes. I figured that, when GRANTED BY is not specified, it is OK to
> > infer a valid grantor
>
> The spec is clear that the grantor should be either the current user or
> the current role. We also have a concept of INHERIT, which allows us to
> choose a role we're a member of if the current one does not suffice.
>
> But to choose a different role (the bootstrap superuser) even when the
> current (super) user role *does* suffice seems like an outright
> violation of both the spec and the principle of least surprise.

I don't think that the current superuser role suffices. For non-role
objects, privileges originate in the table owner and can then be
granted to others. Roles don't have an explicit owner, so I treated
the bootstrap superuser as the implicit owner of every role. Perhaps
there is some other way we could go here - e.g. it's been proposed by
multiple people that maybe roles should have owners - but I do not
think it is viable to regard the owner of a role as being anyone who
happens to be a superuser right at the moment. To some extent that's
related to your concern about whether ALTER USER .. NOSUPERUSER should
be fast and immune to failure, but I also think that it is a good idea
to have all of the privileges originating from a single owner. That
ensures, for example, that anyone who can act as the object owner can
revoke any privilege, which wouldn't necessarily be true if the object
had multiple owners. Now if all of the owners are themselves
superusers who all have the power to become any of the other owners
then perhaps it wouldn't end up mattering too much, but it doesn't
seem like a good idea to rely on that. In fact, part of my goal here
is to get to a world where there's less need to rely on superuser
powers to do system administration. I also just think it's less
confusing if objects have single owners rather than nebulous groups of
owners.

> > set session authorization a;
> > grant select on table t1 to b;
> >
> > At this point, b has SELECT permission on table t1 and the grantor of
> > record is p1
>
> That's because "a" does not have permision to grant select on t1, so
> INHERIT kicks in to implicitly "SET ROLE p1". What keeps INHERIT sane
> is that it only kicks in when required (i.e. it would otherwise result
> in failure).
>
> But in the case I raised, the current user is an entirely valid
> grantor, so it doesn't make sense to me to infer a different grantor.

See above, but also, see the first stanza of select_best_grantor(). If
alice is a table owner, and grants permissions to bob WITH GRANT
OPTION, and bob is a superuser and grants permissions on the table,
the grantor will be alice, not bob.

> > As to the first, the algorithm being used to select the best grantor
> > here is analogous to the one we use for privileges on other object
> > types, such as tables, namely, we prefer to create a grant that is
> > not
> > dependent on some other grant, rather than one that is.
>
> I don't quite follow. It seems like we're conflating a policy based on
> INHERIT with the policy around grants by superusers.
>
> In the case of role membership and INHERIT, our current behavior seems
> wise (and closer to the standard): to prefer a grantor that is closer
> to the current user/role, and therefore less dependent on other grants.
>
> But for the new policy around superusers, the current superuser is a
> completely valid grantor, and we instead preferring the bootstrap
> superuser. That doesn't seem consistent or wise to me.

I hope that the above comments on treating the bootstrap superuser as
the object owner explain why it works this way.

> I certainly don't want to pin every weird thing about our privilege
> system on you just because you're the last one to touch it. But your
> changes did extend the behavior, and create some new analogous
> behavior, so it seems like a reasonable time to discuss whether those
> extensions are in the right direction.

Sure.

> > When you view this in the context of how other types of grants work,
> > ALTER ROLE ... NOSUPERUSER isn't as much of a special case. Just as
> > we
> > want ALTER ROLE ... NOSUPERUSER to succeed quickly, we also insist
> > that REVOKE role1 FROM role2 to succeed quickly. It isn't allowed to
> > fail due to the existence of dependent privileges, because there
> > aren't allowed to be any dependent privileges.
>
> create user u1;
> create user u2;
> create user u3;
> grant u2 to u1 with admin option;
> \c - u1
> grant u2 to u3;
> \c - bootstrap_superuser
> revoke u2 from u1;
> ERROR: dependent privileges exist

Hmm, I stand corrected. I was thinking of a case in which the grant
was used to perform an action on behalf of an inherited role. Here the
grant from u2 to u3 is performed as u1 and attributed to u1.

> And the whole reason we are jumping through all of these hoops is
> because we want to allow the removal of superuser privileges quickly
> without the possibility of failure. In other words, we don't have time
> to do the work of cascading to dependent objects, or erroring when we
> find them. I'm not entirely sure I agree that's a hard requirement,
> because dropping a superuser can fail. But even if it is a requirement,
> are we even meeting it if we preserve the grants that the former
> superuser created? I'd like to know more about this requirement, and
> whether we are still meeting it, and whether there are alternatives.
>
> It just feels like this edge case requirement about dropping superuser
> privileges is driving the whole design, and that feels wrong to me.

I'm struggling to figure out how to reply to this exactly. I do agree
that the way ALTER ROLE .. [NO]SUPERUSER thing works is something of a
wart, and if we were designing SQL from scratch all over again in
2022, I think it's reasonably likely that a lot of things would end up
working quite a bit differently than they actually do. But, at the
same time, it also seems to me that (1) the way ALTER ROLE ..
[NO]SUPERUSER works is pretty firmly entrenched at this point and we
can't easily get away with changing it; (2) I don't really see an easy
way of changing it that wouldn't cause more problems than it solves;
and (3) it all seems relatively unrelated to this patch.

Like, the logic to infer the grantor in check_role_grantor() and
select_best_admin() is intended to be, and as far as I know actually
is, an exact clone of the logic in select_best_grantor(). It is
different only in that we regard the bootstrap superuser as the object
owner because there is no other owner stored in the catalogs; and in
that we check CREATEROLE permission rather than SUPERUSER permission.
Everything else is the same. To be unhappy with the patch, you have to
think either that (1) treating the bootstrap superuser as the owner of
every role is the wrong idea or (2) that role grants should not choose
an implicit grantor in the same way that other types of grants do or
(3) that the code has a bug.

If you don't think any of those things but believe that the way we've
made superusers interact with the grant system is lame in general, I
somewhat agree, but if we came up with some new paradigm for how it
should work, we'd have to explain why it was sufficiently better than
the status quo to justify breaking backward compatibility, and I think
that would be a hard argument to make. The current system feels kind
of old-fashioned and awkward, but it's self-consistent on its terms
and I bet a lot of people are relying on it to keep working. And I
think if we were going to replace it with something that feels fresh
and modern, focusing on the behavior of ALTER ROLE .. [NO]SUPERUSER
would be the wrong place to start. That, to me, seems like it's a
*mostly* a consequence of much broader design choices, like:

- Having hard-coded superuser checks in many places instead of making
everything a capability.
- Having potentially any number of superusers, instead of just one root user.
- Having granted privileges depend on the grantor continuing to hold
the granted privilege, instead of existing independently.

If I were designing a privilege system for a new piece of software
that didn't need to comply with the SQL standard, I think I'd throw at
least some and maybe all of those things right out the window. But I
designed a system that had to work within that set of assumptions, I
think I'd make it work pretty much the way it actually does.

> > What might be useful is a command that says "OK, for every existing
> > grant that is attributed to user A, change the recorded grantor to
> > user B, if that's allowable, for the others, do nothing". Or maybe
> > there's some possible idea where we try to somehow make B into a
> > valid
> > grantor, but it's not clear to me what the algorithm would be.
>
> I was thinking that if the new grantor is not allowable, and "WITH
> GRANT" (or whatever) was specified, then it would throw an error.

That could be done too, but then every grant attributed to the target
role would have to be validly reattributable to the same new grantor.

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