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

On Fri, 2022-09-02 at 09:30 -0400, Robert Haas wrote:
> Thanks for having a look.

Thanks for doing the work.

> 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.
>

> 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.

>

> 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.


>
> 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 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.

> 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

> 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.

If superusers merely act on behalf of others, then:

   1. Why can the bootstrap superuser be a grantor?
   2. Why can non-bootstrap superusers specify themselves in GRANTED BY
if they are not suitable grantors?

>
> 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.

I like integrity constriants, too. But it feels like we're recording
the wrong information (losing the actual grantor) because it's easier
to keep it "consistent", which doesn't necessarily seem like a win.

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.

> >   * 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.

...

> it seems more theoretically sound than the previous two
> ideas, because it doesn't just throw the idea of integrity
> constraints
> out the window.

Perhaps it's worth considering further. Would be a separate patch, of
course.

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

...

> 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.

Regards,
    Jeff Davis