On Fri, Sep 2, 2022 at 6:01 PM Jeff Davis <pg...@j-davis.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
