Обсуждение: 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
- Joe Koshakow
[0] https://www.postgresql.org/docs/15/sql-revoke.html
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.
> 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
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
On Wed, May 17, 2023 at 11:48 PM Nathan Bossart <nathandbossart@gmail.com> 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 <object name> contained in <privileges>
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 <grantor> 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 <revoke statement> is a <revoke privileges statement>, then for every <grantee>
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 <action>
explicitly or implicitly in <privileges>, that <action> on O, or any of the objects in S, granted
by A to <grantee>
In my example, <grantee> is the role r1, <privileges> is the list of
privileges that only contain SELECT, <action> 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 <revoke statement> is a <revoke privilege statement>, 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 <revoke statement> is a <revoke privileges statement>, then:
a) For every combination of <grantee> and <action> on O specified in <privileges>, 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:
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));
}
Thanks,
Joe Koshakow
[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ef7422510e93266e5aa9bb926d6747d5f2ae21f4
[1] https://www.postgresql.org/message-id/Pine.LNX.4.44.0301191916160.789-100000%40localhost.localdomain
>
> 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 <object name> contained in <privileges>
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 <grantor> 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 <revoke statement> is a <revoke privileges statement>, then for every <grantee>
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 <action>
explicitly or implicitly in <privileges>, that <action> on O, or any of the objects in S, granted
by A to <grantee>
In my example, <grantee> is the role r1, <privileges> is the list of
privileges that only contain SELECT, <action> 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 <revoke statement> is a <revoke privilege statement>, 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 <revoke statement> is a <revoke privileges statement>, then:
a) For every combination of <grantee> and <action> on O specified in <privileges>, 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:
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));
}
Thanks,
Joe Koshakow
[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ef7422510e93266e5aa9bb926d6747d5f2ae21f4
[1] https://www.postgresql.org/message-id/Pine.LNX.4.44.0301191916160.789-100000%40localhost.localdomain
On Thu, May 18, 2023 at 7:17 PM Joseph Koshakow <koshy44@gmail.com> 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:
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));
}
Thanks,
Joe Koshakow
>
> 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:
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));
}
Thanks,
Joe Koshakow
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 <action> explicitly or implicitly in <privileges>, that <action> 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 <revoke statement> is a <revoke privileges statement>, then for every <grantee>
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 <action>
explicitly or implicitly in <privileges>, that <action> on O, or any of the objects in S, granted
by A to <grantee>.
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 <revoke statement> is a <revoke privileges statement>, then:
a) For every combination of <grantee> and <action> on O specified in <privileges>, 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
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 <action> explicitly or implicitly in <privileges>, that <action> 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 <revoke statement> is a <revoke privileges statement>, then for every <grantee>
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 <action>
explicitly or implicitly in <privileges>, that <action> on O, or any of the objects in S, granted
by A to <grantee>.
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 <revoke statement> is a <revoke privileges statement>, then:
a) For every combination of <grantee> and <action> on O specified in <privileges>, 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
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
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