Re: pg_auth_members.grantor is bunk
Robert Haas writes: > On Thu, Aug 18, 2022 at 1:26 PM Robert Haas wrote: >> CI is happier with this version, so I've committed 0001. If no major >> problems emerge, I'll proceed with 0002 as well. > Done. Shouldn't the CF entry [1] be closed as committed? regards, tom lane [1] https://commitfest.postgresql.org/39/3745/
Re: pg_auth_members.grantor is bunk
On Wed, Sep 7, 2022 at 10:56 AM Jeff Davis wrote: > OK. I suppose the best path forward is to just try to improve the > ability to administer the system without relying as much on superusers, > which will allow us to safely ignore some of the weirdness caused by > superusers issuing grants. Yeah, and I think we might not even be that far away from making that happen. There are still a few thorny design issues to work out, I believe, and there's also some complexity that is introduced by the fact that different people want different things. For example, last release cycle, I believed that the NOINHERIT behavior was a weird wart that probably nobody cared about. That turned out to be false, really false. What I *personally* want most as an alternative to superuser is an account that inherits all the privileges of the other accounts that it manages, which might not be all the accounts on the system, and which can also SET ROLE to those accounts. If you're logged into such an account, you can do many of the things a superuser can do and in the same ways that a superuser can do them. For example, if you've got some pg_dump output, you could probably restore the dump using such an account and privilege restoration would work, provided that the required accounts exist and that they're among the accounts managed by your account. However, I think that other people want different things. For example, I think that Joshua Brindle mentioned wanting to have a user-creation bot that should be able to make new accounts but not access them in any way, and I think Stephen Frost was interested in semantics where you could make accounts and be able to SET ROLE into them but not inherit their privileges. Or maybe they were both proposing the same thing: not quite sure. Anyway, it will perhaps turn out to be impossible to give everybody 100% of everything they would like, but I'm thinking about a few ideas that might enable us to cater to a few different scenarios - and I'm hopeful that it will be possible to propose something in time for inclusion in v16, but my ideas aren't quite well enough formulated yet to make a concrete proposal just yet, and when I do make such a proposal I want to do it on a new thread for better visibility. In the meantime, I think that what has already been committed is clearly a step in the right direction. The patch which is the subject of this thread has basically brought the role grant code up to the level of other object types. I don't think it's an overstatement to say that the previous state of affairs was that this feature just didn't work properly and no one had cared enough to bother fixing it. That always makes discussions about future enhancements harder. The patch to add grant-level control to the INHERIT option also seems to me to be a step in the right direction, since, at least IMHO, it is really hard to reason about behavior when the heritability of a particular grant is a property of the grantee rather than something which can be controlled by the grantor, or the system. If we can reach agreement on some of the other things that I have proposed, specifically sorting out the issues under discussion on the "has_privs_of_role vs. is_member_of_role, redux" thread and adding the new capability discussed on the "allowing for control over SET ROLE" thread, I think will be a further, useful step. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Wed, 2022-09-07 at 09:39 -0400, Robert Haas wrote: > Now that is not to say that we couldn't decide that > select_best_grantor() got it wrong and choose to break backward > compatibility in order to fix it ... but I'm not even convinced that > the alternative behavior you propose is clearly better, let alone > that > it's enough better to justify changing things. OK. I suppose the best path forward is to just try to improve the ability to administer the system without relying as much on superusers, which will allow us to safely ignore some of the weirdness caused by superusers issuing grants. Regards, Jeff Davis
Re: pg_auth_members.grantor is bunk
On Tue, Sep 6, 2022 at 7:26 PM Jeff Davis wrote: > There's at least one other difference: if you specify "GRANTED BY su1" > for a table grant, it still selects the table owner as the grantor; > whereas if you specify "GRANTED BY su1" for a role grant, it selects > "su1". Right. Personally, I'm inclined to view that as a defect in the "GRANTED BY whoever" implementation for other object types, and I think it should be resolved by making other object types error out if the user explicitly mentioned in the "GRANTED BY" clause isn't a valid grantor. It also seems possible to view it as a defect in the new implementation, and argue that inference should always be performed starting at the named user. I find that a POLA violation, but someone could disagree. Parenthetically, I think we should also fix GRANTED BY for other object types so that it actually works, but that is a bit of headache because it doesn't seem like that code is relying as heavily on common infrastructure as some things, so I believe it's actually a fair amount of work to make that happen. > In other words, try to issue the grant normally if at all possible, and > play the superuser card as a last resort. I believe that will lead to > the fewest surprising cases, and make them easiest to explain, because > superuser-ness doesn't influence the outcome in as many cases. It seems to me that this policy would reverse select_best_grantor()'s decision about whether we should prefer to rely on superuser privileges or on privileges actually granted to the current user. I think either behavior is defensible, but the existing precedent is to prefer relying on superuser privileges. Like you, I found that a bit weird when I realized that's what it was doing, but it does have some advantages. In particular, it means that the privileges granted by a superuser don't depend on any other grants, which is something that a user might value. Now that is not to say that we couldn't decide that select_best_grantor() got it wrong and choose to break backward compatibility in order to fix it ... but I'm not even convinced that the alternative behavior you propose is clearly better, let alone that it's enough better to justify changing things. However, I don't personally have a strong preference about it one way or the other; if there's a strong consensus to change it, so be it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Tue, 2022-09-06 at 16:26 -0700, Jeff Davis wrote: > In other words, omitting > GRANTED BY is the same as specifying "GRANTED BY current_user". Let me correct this thinko to distinguish between specifying GRANTED BY and not: * Let the granting user be the one specified in the GRANTED BY clause if it exists; otherwise the current user. * If the granting user has privileges to be the grantor (ADMIN OPTION for roles, GRANT OPTION for other objects) then the granting user is the grantor. * Else if GRANTED BY was *not* specified, infer the grantor: - If the granting user inherits from a role with the privileges to be the grantor, then it selects a role with the fewest inheritance hops as the grantor. - Else if the current user is any superuser, the grantor is the top "owner" (bootstrap superuser for roles; object owner for other objects) * Else error (or if an error would break important backwards compatibility, silently make it work like before and perhaps issue a WARNING). The basic idea is to use superuser privileges as a last resort in order to maximize the cases that work normally (independent of superuser- ness). Regards, Jeff Davis
Re: pg_auth_members.grantor is bunk
On Tue, 2022-09-06 at 13:15 -0400, Robert Haas wrote: > 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. There's at least one other difference: if you specify "GRANTED BY su1" for a table grant, it still selects the table owner as the grantor; whereas if you specify "GRANTED BY su1" for a role grant, it selects "su1". grant all privileges on schema public to public; create user su1 superuser; create user u1; create user u2; create user aa; grant u2 to su1 with admin option; \c - aa create table t_aa(i int); grant all privileges on t_aa to su1 with grant option; \c - su1 grant select on t_aa to u1 granted by su1; -- grantor aa select relname, relacl from pg_class where relname='t_aa'; grant u2 to u1 granted by su1; -- grantor su1 -- grantor su1 select grantor::regrole from pg_auth_members where member='u1'::regrole; [ If you run the same example but where su1 is not a superuser, then both select "su1" as the grantor because that's the only valid grantor that can be inferred. ] Now that I understand the underlying philosophy better, and I've experimented with more cases, I propose the following grantor inference behavior which I believe is in the spirit of your changes: * Let the granting user be the one specified in the GRANTED BY clause if it exists; otherwise the current user. In other words, omitting GRANTED BY is the same as specifying "GRANTED BY current_user". * If the granting user has privileges to be the grantor (ADMIN OPTION for roles, GRANT OPTION for other objects) then the granting user is the grantor. * Else if the granting user inherits from a user with the privileges to be the grantor, then it selects a role with the fewest inheritance hops as the grantor. * Else if the current user is any superuser: - If the grant is a role grant, it selects the bootstrap superuser as the grantor. - Else the object owner is the grantor. * Else error (or if an error would break important backwards compatibility, silently make it work like before or perhaps issue a WARNING). In other words, try to issue the grant normally if at all possible, and play the superuser card as a last resort. I believe that will lead to the fewest surprising cases, and make them easiest to explain, because superuser-ness doesn't influence the outcome in as many cases. It cements the idea that the bootstrap superuser is the "real" superuser, and must always remain so, and that all other superusers are temporary stand-ins (kind of but not quite the same as inheritance). And it leaves the ugliness that we lose the information about the "real" grantor when we play the superuser card, but, as I say above, that would be a last resort. The proposal would be a slight behavior change from v15 in the following case: grant all privileges on schema public to public; create user su1 superuser; create user u1; create user aa; \c - aa create table t_aa(i int); grant all privileges on t_aa to su1 with grant option; \c - su1 grant select on t_aa to u1 granted by su1; -- grantor "aa" in v15, grantor "su1" after my proposal select relname, relacl from pg_class where relname='t_aa'; Another change in behavior would be that the bootstrap superuser could be the grantor for table privileges, if the bootstrap superuser has WITH GRANT OPTION privileges. But those seems minor to me. Regards, Jeff Davis
Re: pg_auth_members.grantor is bunk
On Fri, Sep 2, 2022 at 6:01 PM Jeff Davis 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
Re: pg_auth_members.grantor is bunk
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
Re: pg_auth_members.grantor is bunk
Thanks for having a look. On Thu, Sep 1, 2022 at 4:34 PM Jeff Davis 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 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
Re: pg_auth_members.grantor is bunk
On Mon, 2022-08-22 at 11:47 -0400, Robert Haas wrote: > On Thu, Aug 18, 2022 at 1:26 PM Robert Haas > wrote: > > On Wed, Aug 10, 2022 at 4:28 PM Robert Haas > > wrote: > > > Well, CI isn't happy with this, and for good reason: > > > > CI is happier with this version, so I've committed 0001. If no > > major > > problems emerge, I'll proceed with 0002 as well. > > Done. It's still on the CF, so I took a look. There's still some weirdness around superusers: 1. "GRANTED BY current_user" differs from not specifying "GRANTED BY" at all. a. With GRANTED BY current_user, weird because current_user is a superuser: CREATE USER su1 SUPERUSER; CREATE ROLE u1; CREATE ROLE u2; \c - su1 GRANT u2 TO u1 GRANTED BY current_user; ERROR: grantor must have ADMIN OPTION on "u2" b. Without GRANTED BY: CREATE USER su1 SUPERUSER; CREATE ROLE u1; CREATE ROLE u2; \c - su1 GRANT u2 TO u1; -- grantor is bootstrap superuser 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 3. Another case where "GRANTED BY current_user" differs from no "GRANTED BY" at all, with slightly different consequences: a. GRANTED BY current_user, throws error: CREATE USER su1 SUPERUSER; CREATE ROLE u1; CREATE ROLE u2; GRANT u2 TO su1 WITH ADMIN OPTION; \c - su1 GRANT u2 TO u1 GRANTED BY current_user; -- grantor is su1 \c - bootstrap_superuser REVOKE ADMIN OPTION FOR u2 FROM su1; ERROR: dependent privileges exist b. No GRANTED BY, no error: 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 \c - boostrap_superuser REVOKE ADMIN OPTION FOR u2 FROM su1; 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 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 * 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. Also, it would be nice to have REASSIGN OWNED work with grants, perhaps by adding a "WITH[OUT] GRANT" or something. Regards, Jeff Davis
Re: pg_auth_members.grantor is bunk
On Thu, Aug 18, 2022 at 1:26 PM Robert Haas wrote: > On Wed, Aug 10, 2022 at 4:28 PM Robert Haas wrote: > > Well, CI isn't happy with this, and for good reason: > > CI is happier with this version, so I've committed 0001. If no major > problems emerge, I'll proceed with 0002 as well. Done. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Wed, Aug 10, 2022 at 4:28 PM Robert Haas wrote: > Well, CI isn't happy with this, and for good reason: CI is happier with this version, so I've committed 0001. If no major problems emerge, I'll proceed with 0002 as well. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Mon, Aug 1, 2022 at 3:51 PM Robert Haas wrote: > On Mon, Aug 1, 2022 at 1:38 PM Tom Lane wrote: > >> I think the latter --- the cfbot thinks the July CF is no longer relevant, > > but Jacob hasn't yet moved your patches forward. You could wait for > > him to do that, or do it yourself. > > Done. New patches attached. Well, CI isn't happy with this, and for good reason: ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2; -- duplicate -NOTICE: role "regress_priv_user2" has already been granted membership in role "regress_priv_group2" by role "rhaas" +NOTICE: role "regress_priv_user2" has already been granted membership in role "regress_priv_group2" by role "postgres" The problem here is that I revised the error message to include the name of the grantor, since that's now a part of the identity of the grant. It would be misleading to say, as we did previously... NOTICE: role "regress_priv_user2" is already a member of role "regress_priv_group2" ...because them being in the group isn't relevant so much as them being in the group by means of the same grantor. However, I suspect that I can't persuade all of you that we should hard-code the name of the bootstrap superuser as "rhaas", so this test case needs some alteration. I found, however, that the original intent of the test case couldn't be preserved with the patch as written, because when you grant membership in one role to another role as the superuser or a CREATEROLE user, the grant is attributed to the bootstrap superuser, whose name is variable, as this test failure shows. Therefore, to fix the test, I needed to use ALTER GROUP as a non-CREATEROLE user, some user created as part of the test, for the results to be stable. But that was impossible, because even though "GRANT user_name TO group_name" requires *either* CREATEROLE *or* ADMIN OPTION on the group, the equivalent command "ALTER GROUP group_name ADD USER user_name" requires specifically CREATEROLE. I debated whether to fix that inconsistency or just remove this test case and eventually came down on the side of fixing the inconsistency, so the attached version does it that way. -- Robert Haas EDB: http://www.enterprisedb.com v5-0001-Ensure-that-pg_auth_members.grantor-is-always-val.patch Description: Binary data v5-0002-Make-role-grant-system-more-consistent-with-other.patch Description: Binary data
Re: pg_auth_members.grantor is bunk
On Mon, Aug 1, 2022 at 10:38 AM Tom Lane wrote: > > I can't see this on cfbot - either I don't know how to use it > > properly, which is quite possible, or the results aren't showing up > > because of the close of the July CommitFest. > > I think the latter --- the cfbot thinks the July CF is no longer relevant, > but Jacob hasn't yet moved your patches forward. You could wait for > him to do that, or do it yourself. > > (Probably our nonexistent SOP manual for CFMs ought to say "don't > close the old CF till you've moved everything forward".) Sorry about that. I've made a note to add this to the manual later. --Jacob
Re: pg_auth_members.grantor is bunk
On Mon, Aug 1, 2022 at 1:38 PM Tom Lane wrote: >> I think the latter --- the cfbot thinks the July CF is no longer relevant, > but Jacob hasn't yet moved your patches forward. You could wait for > him to do that, or do it yourself. Done. New patches attached. Changes in v4, for 0001: - Typo fix. - Whitespace fixes. Changes in v4, for 0002: - Remove "XXX sketchy" comment because the thing in question turns out not to be sketchy. It has to do with the behavior of ALTER GROUP .. DROP USER and, having investigated the situation, I think the messaging is clear enough. - But just to be sure, add a note to the ALTER GROUP documentation to try to make things more clear. - Wording fixes to the "If GRANTED BY is specified..." paragraph of the GRANT documentation. I reworded this a bit more extensively than what Stephen proposed. Hopefully this is clearer now, or at least no longer missing any words. - Change message to "admin option cannot be granted back to your own grantor". The choice of message is intended to be consistent with the existing message "grant options cannot be granted back to your own grantor," but while there's one grant option per privilege, there's only one admin option. Stephen suggested adopting a message that I had meant to take out of the version I posted, but which ended up surviving in one place, "grants with admin options cannot be circular". And we could still decide to do something like that, but my enthusiasm for that direction was considerably reduced when I realized that "circular" is not very clear at all, because there are multiple kinds of circularities (role-member, member-grantor). - Fix comment to say DROP_RESTRICT instead of DROP_RECURSE. - Make the comment for check_role_grantor() longer so that it can better explain itself. - Rephrase part of the header comment for initialize_revoke_actions() because Stephen found it awkward. - Whitespace fixes. -- Robert Haas EDB: http://www.enterprisedb.com v4-0002-Make-role-grant-system-more-consistent-with-other.patch Description: Binary data v4-0001-Ensure-that-pg_auth_members.grantor-is-always-val.patch Description: Binary data
Re: pg_auth_members.grantor is bunk
Robert Haas writes: > On Sun, Jul 31, 2022 at 2:34 PM Tom Lane wrote: >> Indeed. I've not read the patch, but I just wanted to mention that >> the cfbot shows it as failing regression tests on all platforms. >> Possibly a conflict with some recent commit? > I can't see this on cfbot - either I don't know how to use it > properly, which is quite possible, or the results aren't showing up > because of the close of the July CommitFest. I think the latter --- the cfbot thinks the July CF is no longer relevant, but Jacob hasn't yet moved your patches forward. You could wait for him to do that, or do it yourself. (Probably our nonexistent SOP manual for CFMs ought to say "don't close the old CF till you've moved everything forward".) regards, tom lane
Re: pg_auth_members.grantor is bunk
On Sun, Jul 31, 2022 at 2:34 PM Tom Lane wrote: > Indeed. I've not read the patch, but I just wanted to mention that > the cfbot shows it as failing regression tests on all platforms. > Possibly a conflict with some recent commit? I can't see this on cfbot - either I don't know how to use it properly, which is quite possible, or the results aren't showing up because of the close of the July CommitFest. I tried a rebase locally and it didn't seem to change anything material, not even context lines. Can you provide a link or something that I can look at? -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Sun, Jul 31, 2022 at 2:18 PM Stephen Frost wrote:
> Thanks for working on this.
Thanks for the review.
> > Previously, only the superuser could specify GRANTED BY with a user
> > other than the current user. Relax that rule to allow the grantor
> > to be any role whose privileges the current user posseses. This
> > doesn't improve compatibility with what we do for other object types,
> > where support for GRANTED BY is entirely vestigial, but it makes this
> > feature more usable and seems to make sense to change at the same time
> > we're changing related behaviors.
>
> Presumably the GRANTED BY user in this case still has to have the
> ability to have performed the GRANT themselves? Looks that way below
> and it's just the commit message, but was the first question that came
> to mind when I read through this.
Yes. The previous paragraph in this commit message seems to cover this
point pretty thoroughly.
>
> If GRANTED BY is specified, the grant is recorded as
> - having been done by the specified role. Only database superusers may
> - use this option, except when it names the same role executing the command.
> + having been done by the specified role. A user can only attribute a grant
> + to another role if they possess the privileges of that role. A role can
> + only be recorded as a grantor if has ADMIN OPTION on
>
> Should be: if they have
>
> + a role or is the bootstrap superuser. When a grant is recorded as having
>
> on *that* role seems like it'd be better. And maybe 'or if they are the
> bootstrap superuser'?
Will fix.
> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> index 258943094a..8ab2fecf3a 100644
> --- a/src/backend/commands/user.c
> +++ b/src/backend/commands/user.c
> @@ -805,11 +842,12 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
> if (stmt->action == +1) /* add members to role */
> AddRoleMems(rolename, roleid,
> rolemembers,
> roleSpecsToIds(rolemembers),
> - GetUserId(), false);
> + InvalidOid, false);
> else if (stmt->action == -1)/* drop members from role */
> DelRoleMems(rolename, roleid,
> rolemembers,
> roleSpecsToIds(rolemembers),
> - false);
> + InvalidOid, false,
> DROP_RESTRICT); /* XXX sketchy - hint
> +
>* may mislead */
> }
>
> This comment seems a little concerning..? Also isn't very clear.
Oh right. That was a note to myself to look into that more. And then I
didn't. I'll look into that more and report back.
> @@ -1027,7 +1065,7 @@ DropRole(DropRoleStmt *stmt)
>
> while (HeapTupleIsValid(tmp_tuple = systable_getnext(sscan)))
> {
> - Form_pg_auth_membersauthmem_form;
> + Form_pg_auth_members authmem_form;
>
> authmem_form = (Form_pg_auth_members)
> GETSTRUCT(tmp_tuple);
> deleteSharedDependencyRecordsFor(AuthMemRelationId,
>
> Some random whitespace changes that seems a bit odd given that they
> should have been already correct thanks to pgindent- will these end up
> just getting undone again?
Will fix.
> @@ -1543,14 +1578,94 @@ AddRoleMems(const char *rolename, Oid roleid,
>
> (errcode(ERRCODE_INVALID_GRANT_OPERATION),
> errmsg("role \"%s\" is a member of
> role \"%s\"",
> rolename,
> get_rolespec_name(memberRole;
> + }
> +
> + /*
> +* Disallow attempts to grant ADMIN OPTION back to a user who granted
> it
> +* to you, similar to what check_circularity does for ACLs. We want
> the
> +* chains of grants to remain acyclic, so that it's always possible
> to use
> +* REVOKE .. CASCADE to clean up all grants that depend on the one
> being
> +* revoked.
> +*
> +* NB: This check might look redundant with the check for membership
> +* loops above, but it isn't. That's checking for role-member loop
> (e.g.
> +* A is a member of B and B is a member of A) while this is checking
> for
> +* a member-grantor loop (e.g. A gave ADMIN OPTION to X to B and now
> B, who
> +* has no other source of ADMIN OPTION on X, tries to give ADMIN
> OPTION
> +* on X back to A).
> +*/
>
> With this exact scenario, wouldn't it just be a no-op as A must have
> ADMIN OPTION already on X? The spec says that no cycles of role
> authorizations are allowe
Re: pg_auth_members.grantor is bunk
Greetings, On Sun, Jul 31, 2022 at 11:44 David G. Johnston wrote: > On Sun, Jul 31, 2022 at 11:18 AM Stephen Frost wrote: > >> Greetings, >> >> * Robert Haas (robertmh...@gmail.com) wrote: >> > On Tue, Jul 26, 2022 at 12:46 PM Robert Haas >> wrote: >> >> + } >> + >> + /* >> +* Disallow attempts to grant ADMIN OPTION back to a user who >> granted it >> +* to you, similar to what check_circularity does for ACLs. We >> want the >> +* chains of grants to remain acyclic, so that it's always >> possible to use >> +* REVOKE .. CASCADE to clean up all grants that depend on the >> one being >> +* revoked. >> +* >> +* NB: This check might look redundant with the check for >> membership >> +* loops above, but it isn't. That's checking for role-member >> loop (e.g. >> +* A is a member of B and B is a member of A) while this is >> checking for >> +* a member-grantor loop (e.g. A gave ADMIN OPTION to X to B and >> now B, who >> +* has no other source of ADMIN OPTION on X, tries to give ADMIN >> OPTION >> +* on X back to A). >> +*/ >> >> With this exact scenario, wouldn't it just be a no-op as A must have >> ADMIN OPTION already on X? The spec says that no cycles of role >> authorizations are allowed. > > I’ve realized that what I hadn’t been contemplating here is actually that the GRANT from B to A for X wouldn’t be redundant because grantor is part of the key (A got the right from someone else, but this would be giving it to A from B and therefore would be distinct and would also create a loop which is no good). Haven’t got a good idea on how to improve on the comment based off of that though it still feels like it could be clearer. If I think of something, I’ll share it. Role A must have admin option for X to grant membership in X (with or > without admin option) to B. But that doesn't preclude A from getting > another admin option from someone else. That someone else cannot be > someone to whom they gave admin option to however. So B cannot grant admin > option back to A but role P could if it was basically a sibling of A (i.e., > both getting their initial admin option from someone else). > Right but that wasn’t what I had been trying to get at above. If they do have admin option twice it should be possible to drop one of > them, the prohibition should be on dropping the only admin option > permission a role has for some other role. The commit message for 2 > contemplates this though I haven't gone through the revocation code in > detail. > Yes, think I agree with this also- if A has been given the WITH ADMIN right from Q and P to GRANT X to other roles, and uses that to GRANT X to B, then the GRANT of X to B should be retained even if Q decides to revoke their GRANT as A still has the right from P. If both remove the right, however, either B should lose the right (if CASCADE was passed in) or an error should be returned saying that there’s a dependent GRANT and CASCADE wasn’t given. I'm trying to get this review out sooner than later and so I might be >> missing something, but looking at the regression test for this and these >> error messages, feels like the 'circular' error message makes more sense >> than the 'your own grantor' message that actually ends up being >> returned in that regression test. >> > > Having a more specific error seems reasonable, faster to track down what > the problem is. > Yeah, but also making sure that all the error messages we have in this area are in the regression test output would be good. Makes me wonder if we might try to figure out a way to globally check for that. I suppose one could review coverage.p.o for any ereport() calls that aren’t ever called. I wonder what that would turn up. I think that the whole graph dynamic of this might need some presentation > work (messages and/or psql and/or functions) ; but assuming the errors are > handled improved messages and/or presentation of graphs can be a separate > enhancement. > Yes, we can further improve this later too but that doesn’t mean we should just commit this as-is when some deficiencies have been pointed out. If the only comments were “would be good to improve this error message but I haven’t got a great idea how”, then sure, but there were other items pointed out which were clear corrections and we should make sure to cover in the regression tests all these scenarios that we are checking for and erroring on, lest we end up breaking them unintentionally later. Thanks, Stephen >
Re: pg_auth_members.grantor is bunk
On Sun, Jul 31, 2022 at 11:18 AM Stephen Frost wrote: > Greetings, > > * Robert Haas (robertmh...@gmail.com) wrote: > > On Tue, Jul 26, 2022 at 12:46 PM Robert Haas > wrote: > > + } > + > + /* > +* Disallow attempts to grant ADMIN OPTION back to a user who > granted it > +* to you, similar to what check_circularity does for ACLs. We > want the > +* chains of grants to remain acyclic, so that it's always > possible to use > +* REVOKE .. CASCADE to clean up all grants that depend on the one > being > +* revoked. > +* > +* NB: This check might look redundant with the check for > membership > +* loops above, but it isn't. That's checking for role-member loop > (e.g. > +* A is a member of B and B is a member of A) while this is > checking for > +* a member-grantor loop (e.g. A gave ADMIN OPTION to X to B and > now B, who > +* has no other source of ADMIN OPTION on X, tries to give ADMIN > OPTION > +* on X back to A). > +*/ > > With this exact scenario, wouldn't it just be a no-op as A must have > ADMIN OPTION already on X? The spec says that no cycles of role > authorizations are allowed. Role A must have admin option for X to grant membership in X (with or without admin option) to B. But that doesn't preclude A from getting another admin option from someone else. That someone else cannot be someone to whom they gave admin option to however. So B cannot grant admin option back to A but role P could if it was basically a sibling of A (i.e., both getting their initial admin option from someone else). If they do have admin option twice it should be possible to drop one of them, the prohibition should be on dropping the only admin option permission a role has for some other role. The commit message for 2 contemplates this though I haven't gone through the revocation code in detail. > I'm trying to get this review out sooner than later and so I might be > missing something, but looking at the regression test for this and these > error messages, feels like the 'circular' error message makes more sense > than the 'your own grantor' message that actually ends up being > returned in that regression test. > Having a more specific error seems reasonable, faster to track down what the problem is. I think that the whole graph dynamic of this might need some presentation work (messages and/or psql and/or functions) ; but assuming the errors are handled improved messages and/or presentation of graphs can be a separate enhancement. David J.
Re: pg_auth_members.grantor is bunk
Stephen Frost writes: > * Robert Haas (robertmh...@gmail.com) wrote: >> OK, so I fixed that, and also updated the documentation a bit more. I >> think these patches are basically done, and I'd like to get them >> committed before too much more time goes by, because I have other >> things that depend on this which I also want to get done for this >> release. Anybody object? > Thanks for working on this. Indeed. I've not read the patch, but I just wanted to mention that the cfbot shows it as failing regression tests on all platforms. Possibly a conflict with some recent commit? regards, tom lane
Re: pg_auth_members.grantor is bunk
Greetings,
* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Jul 26, 2022 at 12:46 PM Robert Haas wrote:
> > I believe that these patches are mostly complete, but I think that
> > dumpRoleMembership() probably needs some more work. I don't know what
> > exactly, but there's nothing to cause it to dump the role grants in an
> > order that will create dependent grants after the things that they
> > depend on, which seems essential.
>
> OK, so I fixed that, and also updated the documentation a bit more. I
> think these patches are basically done, and I'd like to get them
> committed before too much more time goes by, because I have other
> things that depend on this which I also want to get done for this
> release. Anybody object?
Thanks for working on this.
Subject: [PATCH v3 1/2] Ensure that pg_auth_members.grantor is always valid.
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 94135fdd6b..258943094a 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -919,7 +920,7 @@ DropRole(DropRoleStmt *stmt)
/*
* Scan the pg_authid relation to find the Oid of the role(s) to be
-* deleted.
+* deleted and perform prleliminary permissions and sanity checks.
Should be preliminary, I'm guessing.
Overall, this looks like a solid improvement.
Subject: [PATCH v3 2/2] Make role grant system more consistent with other
privileges.
> Previously, only the superuser could specify GRANTED BY with a user
> other than the current user. Relax that rule to allow the grantor
> to be any role whose privileges the current user posseses. This
> doesn't improve compatibility with what we do for other object types,
> where support for GRANTED BY is entirely vestigial, but it makes this
> feature more usable and seems to make sense to change at the same time
> we're changing related behaviors.
Presumably the GRANTED BY user in this case still has to have the
ability to have performed the GRANT themselves? Looks that way below
and it's just the commit message, but was the first question that came
to mind when I read through this.
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index f744b05b55..1f828d386a 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -267,8 +267,14 @@ GRANT role_name [, ...] TO
If GRANTED BY is specified, the grant is recorded as
- having been done by the specified role. Only database superusers may
- use this option, except when it names the same role executing the command.
+ having been done by the specified role. A user can only attribute a grant
+ to another role if they possess the privileges of that role. A role can
+ only be recorded as a grantor if has ADMIN OPTION on
Should be: if they have
+ a role or is the bootstrap superuser. When a grant is recorded as having
on *that* role seems like it'd be better. And maybe 'or if they are the
bootstrap superuser'?
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 258943094a..8ab2fecf3a 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -805,11 +842,12 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
if (stmt->action == +1) /* add members to role */
AddRoleMems(rolename, roleid,
rolemembers,
roleSpecsToIds(rolemembers),
- GetUserId(), false);
+ InvalidOid, false);
else if (stmt->action == -1)/* drop members from role */
DelRoleMems(rolename, roleid,
rolemembers,
roleSpecsToIds(rolemembers),
- false);
+ InvalidOid, false,
DROP_RESTRICT); /* XXX sketchy - hint
+
* may mislead */
}
This comment seems a little concerning..? Also isn't very clear.
@@ -1027,7 +1065,7 @@ DropRole(DropRoleStmt *stmt)
while (HeapTupleIsValid(tmp_tuple = systable_getnext(sscan)))
{
- Form_pg_auth_membersauthmem_form;
+ Form_pg_auth_members authmem_form;
authmem_form = (Form_pg_auth_members)
GETSTRUCT(tmp_tuple);
deleteSharedDependencyRecordsFor(AuthMemRelationId,
Some random whitespace changes that seems a bit odd given that they
should have been already correct thanks to pgindent- will these end up
just getting undone again?
@@ -1543,14 +1578,94 @@ AddRoleMems(const char *rolename, Oid roleid,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
errmsg("role \"%s\"
Re: pg_auth_members.grantor is bunk
On Thu, Jul 28, 2022 at 5:17 PM David G. Johnston
wrote:
> I suggest changing \du memberof to output something like this:
>
> rolname | memberof
> -+
> vagrant | {}
> r | {q:admin/vagrant}
> t | {q:admin/vagrant,s:member/vagrant}
>
> (needs sorting, tried to model it after ACL - column privileges specifically)
I don't know. I agree with you that we should probably think about
changing the \du output, but I'm not sure if I like this particular
idea about how to do it. I mean, the ACL format that we use for tables
and other objects is basically an internal format which we throw at
the user, hoping they'll know how to interpret it. I don't know if
it's what we should pick when we don't have that kind of internal
format already. On the other hand, consistency is worth something, and
I'm not sure that I have a better idea.
https://commitfest.postgresql.org/38/3744/ might affect what we want
to do here, too.
> If we aren't dead set on having \du and \dg be aliases for each other I'd
> rather redesign \dg (or add a new meta-command) to be a group-centric view of
> this exact same data instead of user-centric one. Namely it has a "members"
> column instead of "memberof" and have it output, one line per member:
>
> user=[admin|member]/grantor
That seems like a topic for a separate thread, but I agree that a
flipped view of this data would be more useful than using two letters
of the alphabet for exactly the same thing, especially given that
we're pretty short on unused letters.
> I don't have any meaningful insight as to breaking things with these changes
> but I am strongly in favor of tightening this up and formalizing it.
Cool.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Thu, Jul 28, 2022 at 12:09 PM Robert Haas wrote:
> On Tue, Jul 26, 2022 at 12:46 PM Robert Haas
> wrote:
> > I believe that these patches are mostly complete, but I think that
> > dumpRoleMembership() probably needs some more work. I don't know what
> > exactly, but there's nothing to cause it to dump the role grants in an
> > order that will create dependent grants after the things that they
> > depend on, which seems essential.
>
> OK, so I fixed that, and also updated the documentation a bit more. I
> think these patches are basically done, and I'd like to get them
> committed before too much more time goes by, because I have other
> things that depend on this which I also want to get done for this
> release. Anybody object?
>
> I'm hoping not, because, while this is a behavior change, the current
> state of play in this area is just terrible. To my knowledge, this is
> the only place in the system where we allow a dangling OID reference
> in a catalog table to persist after the object to which it refers has
> been dropped. I believe it's also the object type where multiple
> grants by different grantors aren't tracked separately, and where the
> grantor need not themselves have the permission being granted. It
> doesn't really look like any of these things were intentional behavior
> so much as just ... nobody ever bothered to write the code to make it
> work properly. I'm hoping the fact that I have now done that will be
> viewed as a good thing, but maybe that won't turn out to be the case.
>
>
I suggest changing \du memberof to output something like this:
select r.rolname,
array(
select format('%s:%s/%s',
b.rolname,
case when m.admin_option then 'admin' else 'member' end,
g.rolname)
from pg_catalog.pg_auth_members m
join pg_catalog.pg_roles b on (m.roleid = b.oid)
join pg_catalog.pg_roles g on (m.grantor = g.oid)
where m.member = r.oid
) as memberof
from pg_catalog.pg_roles r where r.rolname !~ '^pg_';
rolname | memberof
-+
vagrant | {}
o | {}
a | {o:admin/p,o:admin/vagrant}
x | {o:admin/a,p:member/vagrant}
b | {o:admin/a}
p | {o:admin/vagrant}
y | {x:member/vagrant}
q | {}
r | {q:admin/vagrant}
s | {}
t | {q:admin/vagrant,s:member/vagrant}
(needs sorting, tried to model it after ACL - column privileges
specifically)
=> \dp mytable
Access privileges
Schema | Name | Type | Access privileges | Column privileges |
Policies
+-+---+---+---+--
public | mytable | table | miriam=arwdDxt/miriam+| col1:+|
| | | =r/miriam+| miriam_rw=rw/miriam |
| | | admin=arw/miriam | |
(1 row)
If we aren't dead set on having \du and \dg be aliases for each other I'd
rather redesign \dg (or add a new meta-command) to be a group-centric view
of this exact same data instead of user-centric one. Namely it has a
"members" column instead of "memberof" and have it output, one line per
member:
user=[admin|member]/grantor
I looked over the rest of the patch and played with the circularity a bit,
which motivated the expanded info in \du, and the confirmation that two
separate admin grants that are not circular can exist.
I don't have any meaningful insight as to breaking things with these
changes but I am strongly in favor of tightening this up and formalizing it.
David J.
Re: pg_auth_members.grantor is bunk
On Tue, Jul 26, 2022 at 12:46 PM Robert Haas wrote: > I believe that these patches are mostly complete, but I think that > dumpRoleMembership() probably needs some more work. I don't know what > exactly, but there's nothing to cause it to dump the role grants in an > order that will create dependent grants after the things that they > depend on, which seems essential. OK, so I fixed that, and also updated the documentation a bit more. I think these patches are basically done, and I'd like to get them committed before too much more time goes by, because I have other things that depend on this which I also want to get done for this release. Anybody object? I'm hoping not, because, while this is a behavior change, the current state of play in this area is just terrible. To my knowledge, this is the only place in the system where we allow a dangling OID reference in a catalog table to persist after the object to which it refers has been dropped. I believe it's also the object type where multiple grants by different grantors aren't tracked separately, and where the grantor need not themselves have the permission being granted. It doesn't really look like any of these things were intentional behavior so much as just ... nobody ever bothered to write the code to make it work properly. I'm hoping the fact that I have now done that will be viewed as a good thing, but maybe that won't turn out to be the case. -- Robert Haas EDB: http://www.enterprisedb.com v3-0001-Ensure-that-pg_auth_members.grantor-is-always-val.patch Description: Binary data v3-0002-Make-role-grant-system-more-consistent-with-other.patch Description: Binary data
Re: pg_auth_members.grantor is bunk
On Wed, Jul 20, 2022 at 3:11 PM Robert Haas wrote: > The previous version of the patch makes both DROP OWNED and REASSIGN > OWNED cascade to grantors, but I now think that, for consistency, I'd > better look into changing it so that only DROP OWNED cascades. I think > perhaps I should be using SHARED_DEPENDENCY_ACL instead of > SHARED_DEPENDENCY_OWNER. All right, here's a new patch set, now with a second patch added to the series. 0001, as before, is a minimal fix for $SUBJECT, but it now uses SHARED_DEPENDENCY_ACL instead of SHARED_DEPENDENCY_OWNER, because that gives behavior which is more like what we do for other object types. However, it confines itself to making sure that pg_auth_members.grantor is a valid user, and that's it. 0002 then revises the behavior substantially further to make role grants work like other grants. The grantor of record is required to be a user with ADMIN OPTION on the grant, or the bootstrap superuser, just as for other object types the grantor of record must have GRANT OPTION or be the object owner (but roles don't have owners). Dependent grants are tracked and must be revoked before the grants upon which they depend, but REVOKE .. CASCADE now works. Dependent grants must be acyclic: you can't have alice getting ADMIN OPTION from bob and bob getting it from alice; somebody's got to get it from the bootstrap superuser. This is all just by analogy with what we do for grants on object types, and making role grants do something similar instead of the completely random treatment we have at present. I believe that these patches are mostly complete, but I think that dumpRoleMembership() probably needs some more work. I don't know what exactly, but there's nothing to cause it to dump the role grants in an order that will create dependent grants after the things that they depend on, which seems essential. -- Robert Haas EDB: http://www.enterprisedb.com v2-0001-Ensure-that-pg_auth_members.grantor-is-always-val.patch Description: Binary data v2-0002-Make-role-grant-system-more-consistent-with-other.patch Description: Binary data
Re: pg_auth_members.grantor is bunk
On Fri, Jun 24, 2022 at 4:46 PM Robert Haas wrote: > On Fri, Jun 24, 2022 at 4:30 PM David G. Johnston > wrote: > >> Upthread, I proposed that "drop role baz" should fail here > > > > I concur with this. > > > > I think that the grantor owns the grant, and that REASSIGNED OWNED should > > be able to move those grants to someone else. > > > > By extension, DROP OWNED should remove them. > > Interesting. I hadn't thought about changing the behavior of DROP > OWNED BY and REASSIGN OWNED BY. A quick experiment supports your > interpretation: This experiment was insufficiently thorough. I see now that, for other object types, DROP OWNED BY does work in the way that you propose, but REASSIGN OWNED BY does not. Here's a better test: rhaas=# create table foo(); CREATE TABLE rhaas=# create role bar; CREATE ROLE rhaas=# create role baz; CREATE ROLE rhaas=# grant select on table foo to bar with grant option; GRANT rhaas=# set role bar; SET rhaas=> grant select on table foo to baz; GRANT rhaas=> reset role; RESET rhaas=# drop role bar; ERROR: role "bar" cannot be dropped because some objects depend on it DETAIL: privileges for table foo rhaas=# create role quux; CREATE ROLE rhaas=# reassign owned by bar to quux; REASSIGN OWNED rhaas=# drop role bar; ERROR: role "bar" cannot be dropped because some objects depend on it DETAIL: privileges for table foo rhaas=# drop owned by bar; DROP OWNED rhaas=# drop role bar; DROP ROLE This behavior might look somewhat bizarre, but there's actually a good reason for it: the system guarantees that whoever is listed as the grantor of a privilege has the *current* right to grant that privilege. It can't categorically change the grantor of every privilege given by bar to quux because quux might not and in fact does not have the right to grant select on table foo to baz. Now, you might be thinking, ah, but what if the superuser performed the grant? They could cease to be the superuser later, and then the rule would be violated! But actually not, because a grant by the superuser is imputed to the table owner, who always has the right to grant all rights on the table, and if the table owner is ever changed, all the grants imputed to the old table owner are changed to have their grantor as the new table owner. Similarly, trying to revoke select, or the grant option on it, from bar would fail. So it looks pretty intentional, and pretty tightly-enforced, that every role listed as a grantor must be one which is currently able to grant that privilege. And that means that REASSIGN OWNED can't just do a blanket change to the recorded grantor. It could try to do so, I suppose, and just throw an error if it doesn't work out, but that might make REASSIGN OWNED fail a lot more often, which could suck. In any event, the implemented behavior is that REASSIGN OWNED does nothing about permissions, but DROP OWNED cascades to grantors. This is SORT OF documented, although the documentation only mentions that DROP OWNED cascades to privileges granted *to* the target role, and does not mention that it also cascades to privileges granted *by* the target role. The previous version of the patch makes both DROP OWNED and REASSIGN OWNED cascade to grantors, but I now think that, for consistency, I'd better look into changing it so that only DROP OWNED cascades. I think perhaps I should be using SHARED_DEPENDENCY_ACL instead of SHARED_DEPENDENCY_OWNER. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Fri, Jun 24, 2022 at 4:46 PM Robert Haas 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 v1-0001-Ensure-that-pg_auth_members.grantor-is-always-val.patch Description: Binary data
Re: pg_auth_members.grantor is bunk
On Fri, Jun 24, 2022 at 4:30 PM David G. Johnston wrote: >> Upthread, I proposed that "drop role baz" should fail here > > I concur with this. > > I think that the grantor owns the grant, and that REASSIGNED OWNED should be > able to move those grants to someone else. > > By extension, DROP OWNED should remove them. Interesting. I hadn't thought about changing the behavior of DROP OWNED BY and REASSIGN OWNED BY. A quick experiment supports your interpretation: rhaas=# grant select on table foo to bar; GRANT rhaas=# revoke select on table foo from bar; REVOKE rhaas=# grant select on table foo to bar with grant option; GRANT rhaas=# set role bar; SET rhaas=> grant select on table foo to baz; GRANT rhaas=> reset role; RESET rhaas=# drop role bar; ERROR: role "bar" cannot be dropped because some objects depend on it DETAIL: privileges for table foo rhaas=# drop owned by bar; DROP OWNED rhaas=# drop role bar; DROP ROLE So, privileges on tables (and presumably all other SQL objects) already work the way that you propose here. If we choose to make role memberships work in some other way then the two will be inconsistent. Probably we shouldn't do that. There is still the question of what the SQL specification says about this, but I would guess that it mandates the same behavior for all kinds of privileges rather than treating role memberships and table permissions in different ways. I could be wrong, though. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
On Fri, Jun 24, 2022 at 1:19 PM Robert Haas wrote: > On Mon, Jun 6, 2022 at 7:41 PM Stephen Frost wrote: > > > > In terms of how that's then used, yeah, it's during REVOKE because a > > REVOKE is only able to 'find' role authorization descriptors which match > > the triple of role revoked, grantee, grantor (though there's a caveat in > > that the 'grantor' role could be the current role, or the current user). > > What is supposed to happen if someone tries to execute DROP ROLE on a > role that has previously been used as a grantor? > > Upthread, I proposed that "drop role baz" should fail here > I concur with this. I think that the grantor owns the grant, and that REASSIGNED OWNED should be able to move those grants to someone else. By extension, DROP OWNED should remove them. David J.
Re: pg_auth_members.grantor is bunk
On Mon, Jun 6, 2022 at 7:41 PM Stephen Frost wrote: > Thankfully, at least from my reading, the spec isn't all that > complicated on this particular point. The spec talks about "role > authorization descriptor"s and those are "created with role name, > grantee, and grantor" and then further says "redundant duplicate role > authorization descriptors are destroyed", presumably meaning that the > entire thing has to be identical. In other words, yeah, the PK should > include the grantor. There's a further comment that the 'set of > involved grantees' is the union of all the 'grantees', clearly > indicating that you can have multiple GRANT 'foo' to 'bar's with > distinct grantees. > > In terms of how that's then used, yeah, it's during REVOKE because a > REVOKE is only able to 'find' role authorization descriptors which match > the triple of role revoked, grantee, grantor (though there's a caveat in > that the 'grantor' role could be the current role, or the current user). What is supposed to happen if someone tries to execute DROP ROLE on a role that has previously been used as a grantor? Consider: create role foo; create role bar; create role baz; grant foo to bar granted by baz; drop role baz; Upthread, I proposed that "drop role baz" should fail here, but there's at least one other option: it could silently remove the grant, as we would do if either foo or bar were dropped. The situation is not quite comparable, though: a grant from foo to bar makes no logical sense if either of those roles cease to exist, but it does make at least some sense if baz ceases to exist. Therefore I think someone could argue either for an error or for removing the grant -- or possibly even for some other behavior, though the other behaviors that I can think of don't make much sense in a world where the primary key of pg_auth_members is (roleid, member, grantor). -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Jun 2, 2022 at 3:51 PM Tom Lane wrote: > > > I sort of thought http://postgr.es/m/3981966.1646429...@sss.pgh.pa.us > > > constituted a completed investigation of this sort. No? > > > > I didn't think so. It's clear that the spec expects us to track the > > grantor, but I didn't chase down what it expects us to *do* with that > > information, nor what it thinks the rules are for merging multiple > > authorizations. > > Hmm, OK. Well, one problem is that I've never had any luck > interpreting what the spec says about anything, and I've sort of given > up. But even if that were not so, I'm a little unclear what other > conclusion is possible here. The spec either wants the same behavior > that we already have for other object types, which is what I am here > proposing that we do, or it wants something different. If it wants > something different, it probably wants that for all object types, not > just roles. Since I doubt we would want the behavior for roles to be > inconsistent with what we do for all other object types, in that case > we would probably either change the behavior for all other object > types to something new, and then clean up the role stuff afterwards, > or else first do what I proposed here and then later change it all at > once. In which case the proposal that I've made is as good a way to > start as any. > > Now, if it happens to be the case that the spec proposes a different > behavior for roles than for non-role objects, and if the behavior for > roles is something other than the only we currently have for non-role > objects, then I'd agree that the plan I propose here needs revision. I > suspect that's unlikely but I can't make anything of the spec so > maybe? Thankfully, at least from my reading, the spec isn't all that complicated on this particular point. The spec talks about "role authorization descriptor"s and those are "created with role name, grantee, and grantor" and then further says "redundant duplicate role authorization descriptors are destroyed", presumably meaning that the entire thing has to be identical. In other words, yeah, the PK should include the grantor. There's a further comment that the 'set of involved grantees' is the union of all the 'grantees', clearly indicating that you can have multiple GRANT 'foo' to 'bar's with distinct grantees. In terms of how that's then used, yeah, it's during REVOKE because a REVOKE is only able to 'find' role authorization descriptors which match the triple of role revoked, grantee, grantor (though there's a caveat in that the 'grantor' role could be the current role, or the current user). Interestingly, at least in my looking it over today, it doesn't seem that the 'grantor' could be 'any applicable role' (which is what's usually used to indicate that it could be any role that the current role inherits), meaning you have to include the GRANTED BY in the REVOKE statement or do a SET ROLE first when doing a REVOKE if it's for a role that you aren't currently running as (but which you are a member of). Anyhow, in other words, I do think Robert's got it right here. Happy to discuss further though if there are doubts. Thanks, Stephen signature.asc Description: PGP signature
Re: pg_auth_members.grantor is bunk
On Thu, Jun 2, 2022 at 3:51 PM Tom Lane wrote: > > I sort of thought http://postgr.es/m/3981966.1646429...@sss.pgh.pa.us > > constituted a completed investigation of this sort. No? > > I didn't think so. It's clear that the spec expects us to track the > grantor, but I didn't chase down what it expects us to *do* with that > information, nor what it thinks the rules are for merging multiple > authorizations. Hmm, OK. Well, one problem is that I've never had any luck interpreting what the spec says about anything, and I've sort of given up. But even if that were not so, I'm a little unclear what other conclusion is possible here. The spec either wants the same behavior that we already have for other object types, which is what I am here proposing that we do, or it wants something different. If it wants something different, it probably wants that for all object types, not just roles. Since I doubt we would want the behavior for roles to be inconsistent with what we do for all other object types, in that case we would probably either change the behavior for all other object types to something new, and then clean up the role stuff afterwards, or else first do what I proposed here and then later change it all at once. In which case the proposal that I've made is as good a way to start as any. Now, if it happens to be the case that the spec proposes a different behavior for roles than for non-role objects, and if the behavior for roles is something other than the only we currently have for non-role objects, then I'd agree that the plan I propose here needs revision. I suspect that's unlikely but I can't make anything of the spec so maybe? -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
Robert Haas writes: > On Thu, Jun 2, 2022 at 3:15 PM Tom Lane wrote: >> Maybe. What I was pointing out is that this is SQL-standard syntax >> and there are SQL-standard semantics that it ought to be implementing. >> Probably those semantics match what you describe here, but we ought >> to dive into the spec and make sure before we spend a lot of effort. >> It's not quite clear to me whether the spec defines any particular >> unique key (identity) for the set of role authorizations. > I sort of thought http://postgr.es/m/3981966.1646429...@sss.pgh.pa.us > constituted a completed investigation of this sort. No? I didn't think so. It's clear that the spec expects us to track the grantor, but I didn't chase down what it expects us to *do* with that information, nor what it thinks the rules are for merging multiple authorizations. regards, tom lane
Re: pg_auth_members.grantor is bunk
On Thu, Jun 2, 2022 at 3:15 PM Tom Lane wrote: > Maybe. What I was pointing out is that this is SQL-standard syntax > and there are SQL-standard semantics that it ought to be implementing. > Probably those semantics match what you describe here, but we ought > to dive into the spec and make sure before we spend a lot of effort. > It's not quite clear to me whether the spec defines any particular > unique key (identity) for the set of role authorizations. I sort of thought http://postgr.es/m/3981966.1646429...@sss.pgh.pa.us constituted a completed investigation of this sort. No? -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_auth_members.grantor is bunk
Robert Haas writes: > On Fri, Mar 4, 2022 at 4:34 PM Tom Lane wrote: >> If we are not tracking the grantors of role authorizations, >> then we are doing it wrong and we ought to fix that. > So let's talk about how we could fix this. In a vacuum I'd say this is > just a feature that never got finished and we should rip the whole > thing out. That is, remove pg_auth_members.grantor entirely and at > most keep some do-nothing syntax around for backward compatibility. > However, what Tom is saying in the text quoted above is that we ought > to have something that actually works, which is more challenging. > Apparently, the desired behavior here is for this to work like grants > on non-role objects, where executing "GRANT SELECT ON TABLE s1 TO foo" > under two different user accounts bar and baz that both have > permissions to grant that privilege creates two independent grants > that can be independently revoked. Maybe. What I was pointing out is that this is SQL-standard syntax and there are SQL-standard semantics that it ought to be implementing. Probably those semantics match what you describe here, but we ought to dive into the spec and make sure before we spend a lot of effort. It's not quite clear to me whether the spec defines any particular unique key (identity) for the set of role authorizations. regards, tom lane
pg_auth_members.grantor is bunk
On Fri, Mar 4, 2022 at 4:34 PM Tom Lane wrote: > If we are not tracking the grantors of role authorizations, > then we are doing it wrong and we ought to fix that. We are definitely doing it wrong. It's not that we aren't doing it at all, but we are doing it incorrectly. If user foo executes "GRANT foo TO bar GRANTED BY quux", pg_auth_members.grantor gets the OID of role "quux". Without the "GRANTED BY" clause, it gets the OID of role "foo". But no dependency is created; therefore, the OID of that column can point to a role that no longer exists, or potentially to a role that did exist at one point, was dropped, and was later replaced by some other role that happens to get the same OID. pg_dump handles this by dumping "GRANTED BY whoever" if pg_auth_members.grantor is a still-extant role and omitting the clause if not. This would be a security vulnerability if there were any logic in the backend that actually did anything with pg_auth_members.grantor, because if the original grantor is removed and replaced by another role with the same OID, a dump/restore could change the notional grantor. Since there is no such logic, I don't think it's insecure, but it's still really lame. So let's talk about how we could fix this. In a vacuum I'd say this is just a feature that never got finished and we should rip the whole thing out. That is, remove pg_auth_members.grantor entirely and at most keep some do-nothing syntax around for backward compatibility. However, what Tom is saying in the text quoted above is that we ought to have something that actually works, which is more challenging. Apparently, the desired behavior here is for this to work like grants on non-role objects, where executing "GRANT SELECT ON TABLE s1 TO foo" under two different user accounts bar and baz that both have permissions to grant that privilege creates two independent grants that can be independently revoked. To get there, we'll have to change a good few things -- not only will we need a dependency to prevent a grantor from being dropped without revoking the grant, but we need to change the primary key of pg_auth_members from (roleid, member) to (roleid, member, grantor). Then we'll also have to change the behavior of the GRANT and REVOKE commands at the SQL level, and also the behavior of pg_dump, which will need to dump and restore all grants. I'm open to other proposals, but my thought is that it might be simplest to try to clean this up in two steps. In step one, the only goal would be to make pg_auth_members.grantor reliably sane. In other words, we'd add a dependency on the grantor when a role is granted to another role. You could still only have one grant of role A to role B, but the notional grantor C would always be a user that actually exists. I suspect it would be a really good idea to also patch pg_dump to not ever dump the grantor when working from an older release, because the information is not necessarily reliable and I fear that propagating it forward could lead to broken stuff or maybe even security hazards as noted above. Then, in step two, we change things around to allow multiple grants of the same role to the same other role, one per grantor. Now you've achieved parity between the behavior we have for roles and the behavior we have for permissions on other kinds of SQL objects. There may be other improvements we want to make in this area - previous discussions have suggested various ideas - but it seems to me that making the behavior sane and consistent with other types of objects would be a good start. That way, if we decide we do want to change anything else, we will be starting from a firm foundation, rather than building on sand. Thoughts? -- Robert Haas EDB: http://www.enterprisedb.com
