Обсуждение: REVOKE FROM warning on grantor
Hi, Since ldap2pg 6, I'm working on running by default as non-super role with CREATEDB. Robert Haas made this a viable solution as of Postgres 16. I got a case where ldap2pg tries to remove a role from a group. But ldap2pg user is not the grantor of this membership. This triggers a warning: $ REVOKE owners FROM alice; WARNING: role "alice" has not been granted membership in role "owners" by role "ldap2pg" I'll add a condition on grantor when listing manageable membership to simply avoid this. However, I'd prefer if Postgres fails properly. Because the GRANT is actually not revoked. This prevent ldap2pg to report an issue in handling privileges on such roles. What do you think of make this warning an error ?
On Thursday, March 14, 2024, Étienne BERSAC <etienne.bersac@dalibo.com> wrote:
However, I'd prefer if Postgres fails properly. Because the GRANT is
actually not revoked. This prevent ldap2pg to report an issue in
handling privileges on such roles.
What do you think of make this warning an error ?
The choice of warning is made because after the command ends the grantmin question does not exist. The revoke was a no-op and the final state is as the user intended. Historically doing this didn’t give any message at all which was confusing so we added a warning so the semantics of not failing were preserved but there was some indication that something was amiss. I don’t have a compelling argument to,change the long-standing behavior. Client code can and probably should look for a show errors reported by the backend. It is indeed possibly to treat this warning more serverly than the server chooses to.
David J.
Hi David, Thanks for your answer. > The choice of warning is made because after the command ends the > grantmin question does not exist. The revoke was a no-op and the > final state is as the user intended. Sorry, can you explain me what's the grantmin question is ? Regards, Étienne
On Sat, Mar 16, 2024 at 1:00 PM Étienne BERSAC <etienne.bersac@dalibo.com> wrote:
> The choice of warning is made because after the command ends the
> grantmin question does not exist. The revoke was a no-op and the
> final state is as the user intended.
Sorry, can you explain me what's the grantmin question is ?
That should have read: the granted permission does not exist
David J.
Hi David, > That should have read: the granted permission does not exist Thanks, its clear. However, I'm hitting the warning when removing a role from a group. But the membership remains after the warning. In this case, I expect an error. I'll try to patch the behaviour to ensure an error if the REVOKE is ineffective. Regards, Étienne
=?ISO-8859-1?Q?=C9tienne?= BERSAC <etienne.bersac@dalibo.com> writes: > I'll try to patch the behaviour to ensure an error if the REVOKE is > ineffective. I think we're unlikely to accept such a patch. By my reading, the way we do it now is required by the SQL standard. The standard doesn't seem to say that in so many words; what it does say (from SQL99) is b) If the <revoke statement> is a <revoke role statement>, then for every <grantee> specified, a set of role authorization descriptors is identified. A role authorization descriptor is said to be identified if it defines the grant of any of the specified <role revoked>s to <grantee> with grantor A. It does not say that that set must be nonempty. Admittedly it's not very clear from this one point. However, if you look around in the standard it seems clear that they expect no-op revokes to be no-ops not errors. As an example, every type of DROP command includes an explicit step to drop privileges attached to the object, with wording like (this is for ALTER TABLE DROP COLUMN): 3) Let A be the <authorization identifier> that owns T. The following <revoke statement> is effectively executed with a current authorization identifier of "_SYSTEM" and without further Access Rule checking: REVOKE INSERT(CN), UPDATE(CN), SELECT(CN), REFERENCES(CN) ON TABLE TN FROM A CASCADE There is no special rule for the case that all (or any...) of those privileges were previously revoked; but if that case is supposed to be an error, there would have to be an exception here, or you couldn't drop such columns. Even taking the position that this is an unspecified point that we could implement how we like, I don't think there's a sufficient argument for changing behavior that's stood for a couple of decades. (The spelling of the message has changed over the years, but giving a warning not an error appears to go all the way back to 99b8f8451 where we implemented user groups.) It is certain that there are applications out there that rely on this behavior and would break. regards, tom lane
Hi Tom, Thanks for your anwser. > It does not say that that set must be nonempty. Admittedly it's not > very clear from this one point. However, if you look around in the > standard it seems clear that they expect no-op revokes to be no-ops > not errors. Postgres actually identifies memberhips to revoke. The list is not empty. Event if revoker has USAGE privilege on parent role, the membership is protected by a new check on grantor of membership. This is a new semantic for me. I guess this may obfuscate other people too. I would compare denied revoking of role with revoking privilege on denied table: > REVOKE SELECT ON TABLE toto FROM PUBLIC ; ERROR: permission denied for table toto > Even taking the position that this is an unspecified point that we > could implement how we like, I don't think there's a sufficient > argument for changing behavior that's stood for a couple of decades. In Postgres 15, revoking a membership granted by another role is accepted. I suspect this is related to the new CREATEROLE behaviour implemented by Robert Haas (which is great job anyway). Attached is a script to reproduce. Here is the output on Postgres 15: SET DROP ROLE DROP ROLE DROP ROLE CREATE ROLE CREATE ROLE CREATE ROLE GRANT ROLE SET REVOKE ROLE DO Here is the output of the same script on Postgres 16: SET DROP ROLE DROP ROLE DROP ROLE CREATE ROLE CREATE ROLE CREATE ROLE GRANT ROLE SET psql:ldap2pg/my-revoke.sql:12: WARNING: role "r" has not been granted membership in role "g" by role "m" REVOKE ROLE psql:ldap2pg/my-revoke.sql:18: ERROR: REVOKE failed CONTEXTE : PL/pgSQL function inline_code_block line 4 at RAISE Can you confirm this ? Regards, Étienne
Вложения
On Sat, Mar 16, 2024 at 8:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Even taking the position that this is an unspecified point that we > could implement how we like, I don't think there's a sufficient > argument for changing behavior that's stood for a couple of decades. > (The spelling of the message has changed over the years, but giving a > warning not an error appears to go all the way back to 99b8f8451 > where we implemented user groups.) It is certain that there are > applications out there that rely on this behavior and would break. I got curious about the behavior of other database systems. https://dev.mysql.com/doc/refman/8.0/en/revoke.html documents an "IF EXISTS" option whose documentation reads, in relevant part, "Otherwise, REVOKE executes normally; if the user does not exist, the statement raises an error." https://community.snowflake.com/s/article/Access-Control-Error-Message-When-Revoking-a-Non-existent-Role-Grant-From-a-Role-or-User is kind of interesting. It says that such commands used to fail with an error but that's been changed; now they don't. I couldn't find a clear reference for Oracle or DB-2 or SQL server, but it doesn't look like any of them have an IF EXISTS option, and the examples they show don't mention this being an issue AFAICS, so I'm guessing that all of them accept commands of this type without error. On the whole, it seems like we might be taking the majority position here, but I can't help but feel some sympathy with people who don't like it. Maybe we need a REVOKE OR ELSE command. :-) -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Sat, Mar 16, 2024 at 8:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Even taking the position that this is an unspecified point that we >> could implement how we like, I don't think there's a sufficient >> argument for changing behavior that's stood for a couple of decades. > I got curious about the behavior of other database systems. Yeah, I was mildly curious about that too; it'd be unlikely to sway my bottom-line opinion, but it would be interesting to check. > https://dev.mysql.com/doc/refman/8.0/en/revoke.html documents an "IF > EXISTS" option whose documentation reads, in relevant part, > "Otherwise, REVOKE executes normally; if the user does not exist, the > statement raises an error." Hmm, I don't think that's quite what's at stake here. We do throw error if either named role doesn't exist: regression=# revoke foo from joe; ERROR: role "joe" does not exist regression=# create user joe; CREATE ROLE regression=# revoke foo from joe; ERROR: role "foo" does not exist regression=# create role foo; CREATE ROLE regression=# revoke foo from joe; WARNING: role "joe" has not been granted membership in role "foo" by role "postgres" REVOKE ROLE What the OP is on about is that that last case issues WARNING not ERROR. Reading further down in the mysql page you cite, it looks like their IF EXISTS conflates "role doesn't exist" with "role wasn't granted", and suppresses errors for both those cases. I'm not in favor of changing things here, but if we did, I sure wouldn't want to adopt those exact semantics. regards, tom lane
On 18.03.24 22:30, Tom Lane wrote: > regression=# revoke foo from joe; > WARNING: role "joe" has not been granted membership in role "foo" by role "postgres" > REVOKE ROLE > > What the OP is on about is that that last case issues WARNING not > ERROR. Another point is that granting a role that has already been granted is not an error. So it makes some sense that revoking a role that has not been granted is also not an error. Both of these operations are idempotent in the same way.
Hi, > https://dev.mysql.com/doc/refman/8.0/en/revoke.html documents an "IF > EXISTS" option whose documentation reads, in relevant part, > "Otherwise, REVOKE executes normally; if the user does not exist, the > statement raises an error." > > https://community.snowflake.com/s/article/Access-Control-Error-Message-When-Revoking-a-Non-existent-Role-Grant-From-a-Role-or-User > is kind of interesting. It says that such commands used to fail with > an error but that's been changed; now they don't. It's not about inexistant user. It's not about inexistant membership. It's about membership you are not allowed to revoke. ldap2pg goals is to revoke spurious privileges. If ldap2pg find a spurious membership, it revokes it. Postgres 16 does not revoke some membership revoked before, and does not fail. The usual case is: a superuser grants writers role to alice. In directory, alice is degraded to readers. ldap2pg is not superuser but has CREATEROLE. ldap2pg applies the changes. In Postgres 15, revocation is completed. In Postgres 16, alice still has writers privileges and ldap2pg is not aware of this without clunky checks. Do you see a security concern here ? Regards, Étienne
On Wed, Mar 20, 2024 at 1:26 PM Étienne BERSAC <etienne.bersac@dalibo.com> wrote: > The usual case is: a superuser grants writers role to alice. In > directory, alice is degraded to readers. ldap2pg is not superuser but > has CREATEROLE. ldap2pg applies the changes. In Postgres 15, revocation > is completed. In Postgres 16, alice still has writers privileges and > ldap2pg is not aware of this without clunky checks. In previous versions of PostgreSQL, the grantor column of pg_auth_members was not meaningful. Commit ce6b672e4455820a0348214be0da1a024c3f619f changed that, for reasons explained in the commit message. So now, to revoke a particular grant, ldap2pg really ought to issue REVOKE x FROM y GRANTED BY z. Notice that pg_auth_members now has a unique constraint on (roleid, member, grantor) instead of (roleid, member) as it did previously, so if you specify all of those things, you're identifying a unique grant; if you specify only two of them, you aren't. I think in a case like the one you described here, the REVOKE would fail with an error, because ldap2pg, as a non-superuser, would not have permission to revoke a superuser's grant. That's intentional. I don't really understand why you describe the checks as "clunky". The structure of pg_auth_members is straightforward. If you issue a command to try to make a row go away from that table, it shouldn't be that difficult to figure out whether or not it actually vanished. I *think* that if you use GRANTED BY and don't have any other mistakes in your SQL construction, you'll either succeed in getting rid of the row or you'll get an error; the non-error case is when REVOKE would have targeted a row that's not actually present. But even if I'm wrong about that, running the REVOKE and then checking whether the row you wanted to eliminate is gone seems straightforward. The main thing, to me, seems like you want to make sure that you actually are targeting a specific row. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, > ldap2pg really ought to issue REVOKE x FROM y GRANTED BY z. Thanks for this. I missed this notation and it is exactly what I need. You could consider this subject as closed. Thanks for your time and explanation. Regards, Étienne
On Tue, Mar 26, 2024 at 5:22 AM Étienne BERSAC <etienne.bersac@dalibo.com> wrote: > > ldap2pg really ought to issue REVOKE x FROM y GRANTED BY z. > > Thanks for this. I missed this notation and it is exactly what I need. > > You could consider this subject as closed. Thanks for your time and > explanation. No problem. Glad it helped! -- Robert Haas EDB: http://www.enterprisedb.com