Re: Missing warning on revokes with grant options
Sorry for the multiple consecutive emails. I just came across this comment that explains the current behavior in restrict_and_check_grant /* * Restrict the operation to what we can actually grant or revoke, and * issue a warning if appropriate. (For REVOKE this isn't quite what the * spec says to do: the spec seems to want a warning only if no privilege * bits actually change in the ACL. In practice that behavior seems much * too noisy, as well as inconsistent with the GRANT case.) */ However, I still think the current behavior is a bit strange since holding a grant option is not directly required to issue a revoke. Perhaps for revoke the logic should be: - for each specified privilege: - if the set of acl items on the specified object that includes this privilege is non empty - and none of those acl items have the current role as the grantor - then issue a warning. Thanks, Joe Koshakow
Re: Missing warning on revokes with grant options
I've been thinking about this some more and reading the SQL99 spec. In the original thread that added these warnings [0], which was linked earlier in this thread by Nathan, the following assertion was made: > After that, you get to the General Rules, which pretty clearly say that > trying to grant privileges you don't have grant option for is just a > warning and not an error condition. (Such privileges will not be in the > set of "identified privilege descriptors".) > > AFAICS the specification for REVOKE is exactly parallel. I think it is true that for both GRANT and REVOKE, if a privilege was specified in the statement and a corresponding privilege does not exist in the identified set then a warning should be issued. However, the meaning of "identified set" is different between GRANT and REVOKE. In GRANT the identified set is defined as 4) A set of privilege descriptors is identified. The privilege descriptors identified are those defining, for each explicitly or implicitly in , that on O held by A with grant option. Essentially it is all privileges specified in the GRANT statement on O **where by A is the grantee with a grant option**. In REVOKE the identified set is defined as 1) Case: a) If the is a , then for every specified, a set of privilege descriptors is identified. A privilege descriptor P is said to be identified if it belongs to the set of privilege descriptors that defined, for any explicitly or implicitly in , that on O, or any of the objects in S, granted by A to . Essentially it is all privileges specified in the REVOKE statement on O **where A is the grantor and the grantee is one of the grantees specified in the REVOKE statement**. In fact as far as I can tell, the ability to revoke a privilege does not directly depend on having a grant option for that privilege, it only depends on being the grantor of the specified privilege. However, our code in restrict_and_check_grant doesn't match this. It treats the rules for GRANTs and REVOKEs the same, in that you need a grant option to execute either. It's possible that due to the abandoned privilege rules that it is impossible for a privilege to exist where the grantor doesn't also have a grant option on that privilege. I haven't read that part of the spec closely enough. As a consequence of how the identified set is defined for REVOKE, not only should a warning be issued in the example from my previous email, but I think a warning should also be issued even if the grantee has no privileges on O. For example, ``` test=# SELECT current_role; current_role -- joe (1 row) test=# CREATE TABLE t (); CREATE TABLE test=# CREATE ROLE r1; CREATE ROLE test=# SELECT relacl FROM pg_class WHERE relname = 't'; relacl (1 row) test=# REVOKE SELECT ON t FROM r1; REVOKE ``` Here the identified set for the REVOKE statement is empty. So there is no corresponding privilege descriptor in the identified set for the SELECT privilege in the REVOKE statement. So a warning should be issued. Recall: 18) If the is a , then: a) For every combination of and on O specified in , if there is no corresponding privilege descriptor in the set of identified privilege descriptors, then a completion condition is raised: warning — privilege not revoked Essentially the meaning of the warning for REVOKE does not mean "you tried to revoke a privilege but you don't have a grant option", it means "you tried to revoke a privilege (where you are the grantor), but such a privilege does not exist". Thanks, Joe Koshakow [0] https://postgr.es/m/20040511091816.E9887CF519E%40www.postgresql.com
Re: Missing warning on revokes with grant options
On Thu, May 18, 2023 at 7:17 PM Joseph Koshakow wrote: > >I looked into this function and that is correct. We fail to find a >match for the revoked privilege here: > >/* >* Search the ACL for an existing entry for this grantee and grantor. If >* one exists, just modify the entry in-place (well, in the same position, >* since we actually return a copy); otherwise, insert the new entry at >* the end. >*/ > >for (dst = 0; dst < num; ++dst) >{ >if (aclitem_match(mod_aip, old_aip + dst)) >{ >/* found a match, so modify existing item */ >new_acl = allocacl(num); >new_aip = ACL_DAT(new_acl); >memcpy(new_acl, old_acl, ACL_SIZE(old_acl)); >break; >} >} > >Seeing that there was no match, we add a new empty privilege to the end >of the existing ACL list here: > >if (dst == num) >{ >/* need to append a new item */ >new_acl = allocacl(num + 1); >new_aip = ACL_DAT(new_acl); >memcpy(new_aip, old_aip, num * sizeof(AclItem)); > >/* initialize the new entry with no permissions */ >new_aip[dst].ai_grantee = mod_aip->ai_grantee; >new_aip[dst].ai_grantor = mod_aip->ai_grantor; >ACLITEM_SET_PRIVS_GOPTIONS(new_aip[dst], > ACL_NO_RIGHTS, ACL_NO_RIGHTS); >num++; /* set num to the size of new_acl */ >} > >We then try and revoke the specified privileges from the new empty >privilege, leaving it empty (modechg will equal ACL_MODECHG_DEL here): > >old_rights = ACLITEM_GET_RIGHTS(new_aip[dst]); >old_goptions = ACLITEM_GET_GOPTIONS(new_aip[dst]); > >/* apply the specified permissions change */ >switch (modechg) >{ >case ACL_MODECHG_ADD: >ACLITEM_SET_RIGHTS(new_aip[dst], > old_rights | ACLITEM_GET_RIGHTS(*mod_aip)); >break; >case ACL_MODECHG_DEL: >ACLITEM_SET_RIGHTS(new_aip[dst], > old_rights & ~ACLITEM_GET_RIGHTS(*mod_aip)); >break; >case ACL_MODECHG_EQL: >ACLITEM_SET_RIGHTS(new_aip[dst], > ACLITEM_GET_RIGHTS(*mod_aip)); >break; >} > >Then since the new privilege remains empty, we remove it from the ACL >list: > >new_rights = ACLITEM_GET_RIGHTS(new_aip[dst]); >new_goptions = ACLITEM_GET_GOPTIONS(new_aip[dst]); > >/* >* If the adjusted entry has no permissions, delete it from the list. >*/ >if (new_rights == ACL_NO_RIGHTS) >{ >memmove(new_aip + dst, >new_aip + dst + 1, >(num - dst - 1) * sizeof(AclItem)); >/* Adjust array size to be 'num - 1' items */ >ARR_DIMS(new_acl)[0] = num - 1; >SET_VARSIZE(new_acl, ACL_N_SIZE(num - 1)); >} Sorry about the unformatted code, here's the entire quoted section again with proper formatting: I looked into this function and that is correct. We fail to find a match for the revoked privilege here: /* * Search the ACL for an existing entry for this grantee and grantor. If * one exists, just modify the entry in-place (well, in the same position, * since we actually return a copy); otherwise, insert the new entry at * the end. */ for (dst = 0; dst < num; ++dst) { if (aclitem_match(mod_aip, old_aip + dst)) { /* found a match, so modify existing item */ new_acl = allocacl(num); new_aip = ACL_DAT(new_acl); memcpy(new_acl, old_acl, ACL_SIZE(old_acl)); break; } } Seeing that there was no match, we add a new empty privilege to the end of the existing ACL list here: if (dst == num) { /* need to append a new item */ new_acl = allocacl(num + 1); new_aip = ACL_DAT(new_acl); memcpy(new_aip, old_aip, num * sizeof(AclItem)); /* initialize the new entry with no permissions */ new_aip[dst].ai_grantee = mod_aip->ai_grantee; new_aip[dst].ai_grantor = mod_aip->ai_grantor; ACLITEM_SET_PRIVS_GOPTIONS(new_aip[dst], ACL_NO_RIGHTS, ACL_NO_RIGHTS); num++;/* set num to the size of new_acl */ } We then try and revoke the specified privileges from the new empty privilege, leaving it empty (modechg will equal ACL_MODECHG_DEL here): old_rights = ACLITEM_GET_RIGHTS(new_aip[dst]); old_goptions = ACLITEM_GET_GOPTIONS(new_aip[dst]); /* apply the specified permissions change */ switch (modechg) { case ACL_MODECHG_ADD: ACLITEM_SET_RIGHTS(new_aip[dst], old_rights | ACLITEM_GET_RIGHTS(*mod_aip)); break; case ACL_MODECHG_DEL: ACLITEM_SET_RIGHTS(new_aip[dst], old_rights & ~ACLITEM_GET_RIGHTS(*mod_aip)); break; case ACL_MODECHG_EQL: ACLITEM_SET_RIGHTS(new_aip[dst], ACLITEM_GET_RIGHTS(*mod_aip)); break; } Then since the new privilege remains empty, we remove it from the ACL list:
Re: Missing warning on revokes with grant options
On Wed, May 17, 2023 at 11:48 PM Nathan Bossart wrote: > >The thread for the aforementioned change [0] mentions the standard quite a >bit, which might explain the current behavior. I went through that thread and the quoted parts of the SQL standard. It seems clear that if a user tries to REVOKE some privilege and they don't have a grant option on that privilege, then a warning should be issued. There was some additional discussion on when there should be an error vs a warning, but I don't think it's that relevant to this discussion. However, I was not able to find any discussion about the restriction that a revoker can only revoke privileges that they granted themselves. The restriction was added to PostgreSQL at the same time as GRANT OPTIONs were introduced. The commit [0] and mailing thread [1] don't provide much details on this specific restriction. The SQL99 standard for REVOKE is very dense and I may have misunderstood parts, but here's my interpretation of how this restriction might come from the standard and what it says about issuing a warning (section 12.6). Let's start with the Syntax Rules: 1) Let O be the object identified by the contained in In my example O is the table t. 3) Let U be the current user identifier and R be the current role name. 4) Case: a) If GRANTED BY is not specified, then Case: i) If U is not the null value, then let A be U. ii) Otherwise, let A be R. In my example A is the role r1. 9) Case: a) If the is a , then for every specified, a set of privilege descriptors is identified. A privilege descriptor P is said to be identified if it belongs to the set of privilege descriptors that defined, for any explicitly or implicitly in , that on O, or any of the objects in S, granted by A to In my example, is the role r1, is the list of privileges that only contain SELECT, is SELECT. Therefore the set of identified privilege descriptors would be a single privilege descriptor on table t where the privileges contain SELECT, the grantor is r1, and the grantee is r1. Such a privilege does not exist, so the identified privilege set is empty. Now onto the General Rules: 1) Case: a) If the is a , then Case: i) If neither WITH HIERARCHY OPTION nor GRANT OPTION FOR is specified, then: 2) The identified privilege descriptors are destroyed. In my example, the identified set of privileges is empty, so no privileges are destroyed (which I'm interpreting to mean the same thing as revoked). 18) If the is a , then: a) For every combination of and on O specified in , if there is no corresponding privilege descriptor in the set of identified privilege descriptors, then a completion condition is raised: warning — privilege not revoked. In my example the identified privileges set is empty, therefore it cannot contain a corresponding privilege descriptor, therefore we should be issuing a warning. So I think our current behavior is not in spec. Would you agree with this evaluation or do you think I've misunderstood something? >> I wasn't able to locate where the check for >>> A user can only revoke privileges that were granted directly by that >>> user. >> is in the code, but we should probably just add a warning there. > >І'm not certain, but I suspect the calls to aclupdate() in >merge_acl_with_grant() take care of this because the grantors will never >match. I looked into this function and that is correct. We fail to find a match for the revoked privilege here: /* * Search the ACL for an existing entry for this grantee and grantor. If * one exists, just modify the entry in-place (well, in the same position, * since we actually return a copy); otherwise, insert the new entry at * the end. */ for (dst = 0; dst < num; ++dst) { if (aclitem_match(mod_aip, old_aip + dst)) { /* found a match, so modify existing item */ new_acl = allocacl(num); new_aip = ACL_DAT(new_acl); memcpy(new_acl, old_acl, ACL_SIZE(old_acl)); break; } } Seeing that there was no match, we add a new empty privilege to the end of the existing ACL list here: if (dst == num) { /* need to append a new item */ new_acl = allocacl(num + 1); new_aip = ACL_DAT(new_acl); memcpy(new_aip, old_aip, num * sizeof(AclItem)); /* initialize the new entry with no permissions */ new_aip[dst].ai_grantee = mod_aip->ai_grantee; new_aip[dst].ai_grantor = mod_aip->ai_grantor; ACLITEM_SET_PRIVS_GOPTIONS(new_aip[dst], ACL_NO_RIGHTS, ACL_NO_RIGHTS); num++; /* set num to the size of new_acl */ } We then try and revoke the specified privileges from the new empty privilege, leaving it empty (modechg will equal ACL_MODECHG_DEL here): old_rights = ACLITEM_GET_RIGHTS(new_aip[dst]); old_goptions = ACLITEM_GET_GOPTIONS(new_aip[dst]); /* apply the specified permissions change */ switch (modechg) { case ACL_MODECHG_ADD:
Re: Missing warning on revokes with grant options
On Mon, May 15, 2023 at 11:23:22PM -0400, Joseph Koshakow wrote: > Reading through the docs [0], I'm not actually sure if the REVOKE > in the second example should succeed or not. At first it says: > >> A user can only revoke privileges that were granted directly by that >> user. If, for example, user A has granted a privilege with grant >> option to user B, and user B has in turn granted it to user C, then >> user A cannot revoke the privilege directly from C. > > Which seems pretty clear that you can only revoke privileges that you > directly granted. However later on it says: > >> As long as some privilege is available, the command will proceed, but >>it will revoke only those privileges for which the user has grant >> options. > ... >> while the other forms will issue a warning if grant options for any >> of the privileges specifically named in the command are not held. > > Which seems to imply that you can revoke a privilege as long as you > have a grant option on that privilege. I believe the "can only revoke privileges that were granted directly by that user" rule still applies. However, I can see how the section about non-owners attempting to revoke privileges might cause confusion about this. The text in question has been around since 2004 (4b2dafc) and might be worth revisiting. IMO the most confusing part is that the warnings won't appear if you have the grant option on the privilege in question but aren't the grantor. My (possibly naive) expectation would be that you'd see warnings when a privilege cannot be revoked because you are not the grantor. > Either way I think the REVOKE should either fail and emit a warning > OR succeed and emit no warning. The thread for the aforementioned change [0] mentions the standard quite a bit, which might explain the current behavior. > I wasn't able to locate where the check for >> A user can only revoke privileges that were granted directly by that >> user. > is in the code, but we should probably just add a warning there. І'm not certain, but I suspect the calls to aclupdate() in merge_acl_with_grant() take care of this because the grantors will never match. [0] https://postgr.es/m/20040511091816.E9887CF519E%40www.postgresql.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Missing warning on revokes with grant options
Hi Hackers, I noticed some confusing behavior with REVOKE recently. Normally if REVOKE fails to revoke anything a warning is printed. For example, see the following scenario: ``` test=# SELECT current_role; current_role -- joe (1 row) test=# CREATE ROLE r1; CREATE ROLE test=# CREATE TABLE t (); CREATE TABLE test=# GRANT SELECT ON TABLE t TO r1; GRANT test=# SET ROLE r1; SET test=> REVOKE SELECT ON TABLE t FROM r1; WARNING: no privileges could be revoked for "t" WARNING: no privileges could be revoked for column "tableoid" of relation "t" WARNING: no privileges could be revoked for column "cmax" of relation "t" WARNING: no privileges could be revoked for column "xmax" of relation "t" WARNING: no privileges could be revoked for column "cmin" of relation "t" WARNING: no privileges could be revoked for column "xmin" of relation "t" WARNING: no privileges could be revoked for column "ctid" of relation "t" REVOKE test=> SELECT relacl FROM pg_class WHERE relname = 't'; relacl - {joe=arwdDxtm/joe,r1=r/joe} (1 row) ``` However, if the REVOKE fails and the revoker has a grant option on the privilege, then no warning is emitted. For example, see the following scenario: ``` test=# SELECT current_role; current_role -- joe (1 row) test=# CREATE ROLE r1; CREATE ROLE test=# CREATE TABLE t (); CREATE TABLE test=# GRANT SELECT ON TABLE t TO r1 WITH GRANT OPTION; GRANT test=# SET ROLE r1; SET test=> REVOKE SELECT ON TABLE t FROM r1; REVOKE test=> SELECT relacl FROM pg_class WHERE relname = 't'; relacl -- {joe=arwdDxtm/joe,r1=r*/joe} (1 row) ``` The warnings come from restrict_and_check_grant() in aclchk.c. The psuedo code is if (revoked_privileges & available_grant_options == 0) emit_warning() In the second example, `r1` does have the proper grant options so no warning is emitted. However, the revoke has no actual effect. Reading through the docs [0], I'm not actually sure if the REVOKE in the second example should succeed or not. At first it says: > A user can only revoke privileges that were granted directly by that > user. If, for example, user A has granted a privilege with grant > option to user B, and user B has in turn granted it to user C, then > user A cannot revoke the privilege directly from C. Which seems pretty clear that you can only revoke privileges that you directly granted. However later on it says: > As long as some privilege is available, the command will proceed, but >it will revoke only those privileges for which the user has grant > options. ... > while the other forms will issue a warning if grant options for any > of the privileges specifically named in the command are not held. Which seems to imply that you can revoke a privilege as long as you have a grant option on that privilege. Either way I think the REVOKE should either fail and emit a warning OR succeed and emit no warning. I wasn't able to locate where the check for > A user can only revoke privileges that were granted directly by that > user. is in the code, but we should probably just add a warning there. - Joe Koshakow [0] https://www.postgresql.org/docs/15/sql-revoke.html