Re: pg_auth_members.grantor is bunk - Mailing list pgsql-hackers

Thanks for having a look.

On Thu, Sep 1, 2022 at 4:34 PM Jeff Davis <pgsql@j-davis.com> wrote:
> There's still some weirdness around superusers:
>
> 1. "GRANTED BY current_user" differs from not specifying "GRANTED BY"
> at all.

Yes. I figured that, when GRANTED BY is not specified, it is OK to
infer a valid grantor, but if it is specified, it does not seem right
to infer a grantor other than the one specified. Admittedly, this case
is without precedent elsewhere in the system, because nobody has made
GRANTED BY work for other object types, outside of trivial cases.
Still, it seems like the right behavior to me.

> 2. Grantor can depend on the path to get there:
>
>   a. Already superuser:
>
>      CREATE USER su1 SUPERUSER;
>      CREATE ROLE u1;
>      CREATE ROLE u2;
>      GRANT u2 TO su1 WITH ADMIN OPTION;
>      \c - su1
>      GRANT u2 TO u1;
>      -- grantor is bootstrap superuser
>
>   b. Becomes superuser after GRANT:
>
>      CREATE USER su1;
>      CREATE ROLE u1;
>      CREATE ROLE u2;
>      GRANT u2 TO su1 WITH ADMIN OPTION;
>      \c - su1
>      GRANT u2 TO u1;
>      \c - bootstrap_superuser
>      ALTER ROLE su1 SUPERUSER;
>      -- grantor is su1

This also seems correct to me, and here I believe you could construct
similar examples with other object types. We infer the grantor based
on the state of the system at the time the grant was performed. We
can't change our mind later even if things have changed that would
cause us to make a different inference. In the case of a table, for
example, consider:

create role p1;
create role p2;
create role a;
create table t1 (a int);
create role b;
grant select on table t1 to p1 with grant option;
grant select on table t1 to p2 with grant option;
grant p1 to a;
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. But if you had done "GRANT p2 TO a" then the grantor of
record would be p2 rather than p1. And you can still "REVOKE p1 FROM
a;" and then "GRANT p2 to a;". As in your example, doing so won't
change the grantor recorded for the grant already made.

> 3. Another case where "GRANTED BY current_user" differs from no
> "GRANTED BY" at all, with slightly different consequences:

It's extremely difficult for me to imagine what other behavior would
be sane here. In this example, the inferred best grantor is different
from the current user, so forcing the grantor to be the current user
changes the behavior. There are only two ways that anything different
can happen: either we'd have to change the algorithm for inferring the
best grantor, or we'd have to be willing to disregard the user's
explicit specification that the grantor be the current user rather
than somebody else.

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. Maybe that's
the best policy and maybe it isn't, but I can't see it being
reasonable to have one policy for grants on tables, functions, etc.
and another policy for grants on roles.

As to the second, this is somewhat similar to the case you already
raised in your example #1. However, in that case, the
explicitly-specified grantor wasn't valid, so the grant failed. I
don't think it's right to allow inference in the presence of an
explicit specification, but if the consensus was that we really ought
to make that case succeed, I suppose we could. Here, however, the
explicitly-specified grantor *is a legal grantor*. I think it would be
extremely surprising if we just ignored that and selected some other
valid grantor instead.

> We seem to be trying very hard to satisfy two things that seem
> impossible to satisfy:
>
>    i. "ALTER ROLE ... NOSUPERUSER" must always succeed, and probably
> execute quickly, too.
>    ii. We want to maintain catalog invariants that are based, in part,
> on roles having superuser privileges or not.
>
> The hacks we are using to try to make this work are just that: hacks.
> And it's all to satisfy a fairly rare case: removing superuser
> privileges and expecting the catalogs to be consistent.

I guess I don't really agree with that view of it. The primary purpose
of the patch was to make the handing of role grants consistent with
the handling of grants on other object types. I did extend the
existing functionality, because the GRANTED BY <whoever> clause works
for role grants and does not work for other grants. However, that also
worked for role grants before these patches, whereas it's never worked
for other object types. So I chose to restrict that functionality as
little as possible, and basically make it work, rather than removing
it completely, which would have been the most consistent with what we
do elsewhere.

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. GRANT role1 TO role2
doesn't really give role2 the privileges of role1; what it does is
allow role2 to act on behalf of role1. Similarly, ALTER ROLE ...
SUPERUSER lets the target role act on behalf of any user at all,
including the bootstrap superuser. In either case, actions are
attributed to the user on behalf of whom they were performed, not the
user who actually typed the command.

As another example, consider a superuser (the bootstrap superuser or
any other one) who executes GRANT SELECT ON some_random_table TO
some_random_user. Who will be recorded as the grantor? The answer is
that the table owner will be recorded as the grantor, because the
table owner is the one who actually has permission to perform the
operation. The superuser doesn't, except by virtue of their ability to
act on behalf of any other user in the system. In most cases, that's
just an academic distinction, because the question is only whether or
not the operation can be performed, and not who has to perform it. But
grants are different: it matters who does it, and when someone uses
superuser powers or other special privileges to perform an operation,
we have to ask on whose behalf they are acting.

> I think we'd be better off without these hacks. I'm not sure exactly
> how, but the benefit doesn't seem to be worth the cost. Some
> alternative ideas:
>
>   * Have a "safe" version of removing superuser that can error or
> cascade, and an "unsafe" version that always succeeds but might leave
> inconsistent catalogs.
>   * Ignore the problems with removing superuser, but issue a WARNING

I don't like either of these. I think the fact that we have strong
integrity constraints around who can be recorded as the grantor of a
privilege is a good thing, and, again, the purpose of this patch was
to bring role grants up to the level of other parts of the system.

>   * Superusers would auto-grant themselves the privileges that a normal
> user would need to do something before doing it. For instance, if a
> superuser did "GRANT u2 TO u1", it would first automatically issue a
> "GRANT u2 TO current_user WITH ADMIN OPTION GRANTED BY
> bootstrap_superuser", then do the grant normally. If the superuser
> privileges are removed, then the catalogs would still be consistent.
> This is a new idea and I didn't think it through very carefully, but
> might be an interesting approach.

If we did this, we ought to do it for all object types, so that if a
superuser grants privileges on a table they don't own, they implicitly
grant themselves those privileges with grant option and then grant
them to the requested recipient. I doubt that behavior change would be
popular, and I bet somebody would complain about the SQL standard or
something, but it seems more theoretically sound than the previous two
ideas, because it doesn't just throw the idea of integrity constraints
out the window.

> Also, it would be nice to have REASSIGN OWNED work with grants, perhaps
> by adding a "WITH[OUT] GRANT" or something.

I thought about this, too. It's a bit tricky. Right now, DROP OWNED
drops grants, but REASSIGN OWNED doesn't change their owner. On first
glance, this seems inconsistent: either grants are a kind of object
and DROP OWNED and REASSIGN OWNED ought to apply to them like anything
else, or they are not a type of object and neither command should
touch them. However, there's a pretty significant difference between
(1) a table and (2) a grant of privileges on a table. Ownership on the
table itself can be freely changed to any role in the system at any
time. We rewrite the table's ACL on the fly to preserve the invariants
about who can be listed as the grantor. But for the grant of
privileges on the table, we can't freely change the grantor of record
to an arbitrary user at any time: the set of valid grantors is
constrained.

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.

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