Обсуждение: 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 ?

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 ?

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

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