Re: pg_auth_members.grantor is bunk

On Fri, Jun 24, 2022 at 4:46 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Interesting. I hadn't thought about changing the behavior of DROP
> OWNED BY and REASSIGN OWNED BY. A quick experiment supports your
> interpretation:

Here is a minimal patch fixing exactly $SUBJECT. Granting a role to
another role now creates a dependency on the grantor, so if you try to
drop the grantor you get an ERROR. You can resolve that by revoking
the grant, or by using DROP OWNED BY or REASSIGN OWNED BY. To make
this work, I had to make role memberships participate in the
dependency system, which means pg_auth_members gains an OID column.
The tricky part is that removing either of the two roles directly
involved in a grant currently does, and should still, silently remove
the grant. So, if you do "GRANT foo TO bar GRANTED BY baz", and then
try to "DROP ROLE baz", that should fail, but if you instead try to
"DROP ROLE baz, bar", that should work, because when bar is removed,
the grant is silently removed, and then it's OK to drop baz. If these
were database-local objects I think this could have all been sorted
out quite easily by creating dependencies on all three roles involved
in the GRANT and using the right deptype for each, but shared objects
have their own set of deptypes which seemed to present no easy
solution to this problem. I resolved the issue by having DropRole()
make two loops over the list of roles to be dropped rather than one;
see patch for details.

There are several things that I think ought to be changed which this
patch does not change. Most likely, I'll try to write separate patches
for those things rather than making this one bigger.

First, as discussed upthread, I think we ought to change things so
that you can have multiple simultaneous grants of role A to role B
each with a different grantor. That is what we do for other types of
grants and Stephen at least thinks it's what the SQL standard
specifies.

Second, I think we ought to enforce that the grantor has to be a role
which has the ability to perform the grant, just as we do for other
object types. This is a little thorny, though, because we play some
tricks with other types of objects that don't work for roles. If
superuser alice executes "GRANT SELECT ON bobs_table TO fred" we
record the owner of the grant as being the table owner and update the
ownership of the grant each time the table owner is changed. That way,
even if alice ceases to be a superuser, we maintain the invariant that
the grantor of record must have privileges to perform the grant. But
if superuser alice executes "GRANT accounting TO fred", we can't use
the same trick, because the "accounting" role doesn't have an owner.
If we attribute the grant to alice and she ceases to be a superuser
(and also doesn't have CREATEROLE) then the invariant is violated.
Attributing the grant to the bootstrap superuser doesn't help, as that
user can also be made not a superuser. Attributing the grant to
accounting is no good, as accounting doesn't and can't have ADMIN
OPTION on itself; and fred doesn't have to have ADMIN OPTION on
accounting either.

One way to fix this problem would be to prohibit the removal of
superuser privileges from the booststrap superuser. Then, we could
attribute grants made by users who lack ADMIN OPTION on the granted
role to the bootstrap superuser. Grants made by users who do possess
ADMIN OPTION would be attributed to the actual grantor (unless GRANTED
BY was used) and removing ADMIN OPTION from such a user could be made
to fail if they had outstanding role grants. I think that's probably
the nearest analogue of what we do for other object types, but if
you've got another idea what to do here, I'd love to hear it.

Thoughts on this patch would be great, too.

Thanks,

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