Обсуждение: BUG #17346: pg_upgrade fails with role granted by other role
The following bug has been logged on the website:
Bug reference: 17346
Logged by: Andrew Bille
Email address: andrewbille@gmail.com
PostgreSQL version: Unsupported/Unknown
Operating system: Ubuntu 20.04
Description:
Hello!
After the commit:
commit 371087d006e04991080bf17cf2287db38d3ea92e
Author: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri Nov 26 14:02:01 2021 +0100
Fix GRANTED BY support in REVOKE ROLE statements
Commit 6aaaa76bb added support for the GRANTED BY clause in GRANT and
REVOKE statements, but missed adding support for checking the role in
the REVOKE ROLE case. Fix by checking that the parsed role matches the
CURRENT_ROLE/CURRENT_USER requirement, and also add some tests for it.
Backpatch to v14 where GRANTED BY support was introduced.
Discussion:
https://postgr.es/m/B7F6699A-A984-4943-B9BF-CEB84C003527@yesql.se
Backpatch-through: 14
pg_upgrade for example from 10.19 version causes the error:
10/bin/initdb -D d10
14/bin/initdb -D d14
10/bin/pg_ctl -D d10 -l logfile start
10/bin/psql -c "CREATE ROLE user1; CREATE ROLE user2; GRANT user1 TO user2
GRANTED BY user1;"
10/bin/pg_ctl -D d10 -l logfile stop
14/bin/pg_upgrade -d d10 -D d14 -b 10.19/bin/ -B 14/bin/
.........
Copying old pg_multixact/members to new server ok
Setting next multixact ID and offset for new cluster ok
Resetting WAL archives ok
Setting frozenxid and minmxid counters in new cluster ok
Restoring global objects in the new cluster
*failure*
Consult the last few lines of "pg_upgrade_utility.log" for
the probable cause of the failure.
Failure, exiting
Last lines of pg_upgrade_utility.log:
...
CREATE ROLE "user2";
CREATE ROLE
ALTER ROLE "user2" WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN
NOREPLICATION NOBYPASSRLS;
ALTER ROLE
GRANT "user1" TO "user2" GRANTED BY "user1";
psql:pg_upgrade_dump_globals.sql:37: ERROR: grantor must be current user
Regards! Andrew Bille
PG Bug reporting form <noreply@postgresql.org> writes:
> After the commit:
> commit 371087d006e04991080bf17cf2287db38d3ea92e
> Author: Daniel Gustafsson <dgustafsson@postgresql.org>
> Date: Fri Nov 26 14:02:01 2021 +0100
> Fix GRANTED BY support in REVOKE ROLE statements
> pg_upgrade for example from 10.19 version causes the error:
Yeah, you don't even need pg_upgrade. Just do
regression=# CREATE ROLE user1; CREATE ROLE user2; GRANT user1 TO user2 GRANTED BY user1;
CREATE ROLE
CREATE ROLE
ERROR: grantor must be current user
A superuser, or really anyone who's a member of the user1 role,
ought to be able to do that (especially since it used to be allowed).
So it seems the permissions check was coded incorrectly.
regards, tom lane
> On 27 Dec 2021, at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > PG Bug reporting form <noreply@postgresql.org> writes: >> After the commit: > >> commit 371087d006e04991080bf17cf2287db38d3ea92e >> Author: Daniel Gustafsson <dgustafsson@postgresql.org> >> Date: Fri Nov 26 14:02:01 2021 +0100 >> Fix GRANTED BY support in REVOKE ROLE statements > >> pg_upgrade for example from 10.19 version causes the error: > > Yeah, you don't even need pg_upgrade. Just do > > regression=# CREATE ROLE user1; CREATE ROLE user2; GRANT user1 TO user2 GRANTED BY user1; > CREATE ROLE > CREATE ROLE > ERROR: grantor must be current user > > A superuser, or really anyone who's a member of the user1 role, > ought to be able to do that (especially since it used to be allowed). > So it seems the permissions check was coded incorrectly. > > regards, tom lane Thanks for the report, I’m OOO until late tonight but I’ll have a look when in. cheers ./daniel
> On 27 Dec 2021, at 17:02, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > PG Bug reporting form <noreply@postgresql.org> writes: >> After the commit: > >> commit 371087d006e04991080bf17cf2287db38d3ea92e >> Author: Daniel Gustafsson <dgustafsson@postgresql.org> >> Date: Fri Nov 26 14:02:01 2021 +0100 >> Fix GRANTED BY support in REVOKE ROLE statements > >> pg_upgrade for example from 10.19 version causes the error: > > Yeah, you don't even need pg_upgrade. Just do > > regression=# CREATE ROLE user1; CREATE ROLE user2; GRANT user1 TO user2 GRANTED BY user1; > CREATE ROLE > CREATE ROLE > ERROR: grantor must be current user > > A superuser, or really anyone who's a member of the user1 role, > ought to be able to do that (especially since it used to be allowed). > So it seems the permissions check was coded incorrectly. Reading the SQL spec for GRANT and REVOKE, and specifically the "Grantor Determination" subsection, it's not clear to me that this is wrong *per spec* and that any value except CURRENT_USER and CURRENT_ROLE is supported (which is what 6aaaa76bb implemented and the above referenced commit amended). Given the time of day I'm undercaffeinated for spec reading so I might be missing something though. Is <grantor> really handled differently for GRANT/REVOKE ROLE to PRIVILEGE? That being said, *iff* my spec reading is right, since this is something that was working, and the benefit in supporting this is slim, reverting might be the best (only) course. Question is then how far that revert should stretch? Is there value in being spec compliant for PRIVILEGE and not ROLE? If my spec reading is wrong then reverting is pretty obvious, but I would appreciate a second pair of eyes on this before ripping it out. -- Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
> On 27 Dec 2021, at 17:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A superuser, or really anyone who's a member of the user1 role,
>> ought to be able to do that (especially since it used to be allowed).
>> So it seems the permissions check was coded incorrectly.
> Reading the SQL spec for GRANT and REVOKE, and specifically the "Grantor
> Determination" subsection, it's not clear to me that this is wrong *per spec*
> and that any value except CURRENT_USER and CURRENT_ROLE is supported (which is
> what 6aaaa76bb implemented and the above referenced commit amended). Given the
> time of day I'm undercaffeinated for spec reading so I might be missing
> something though. Is <grantor> really handled differently for GRANT/REVOKE
> ROLE to PRIVILEGE?
Not sure. However, it is certainly true that we have long supported
other grantors in GRANT <role> ... GRANTED BY (at least back to 8.4,
I didn't try anything older). Whether that's within the spec or an
extension, breaking the case now isn't okay.
So the code change in b2a459edf is flat wrong AFAICS. The fact that
it didn't break any existing test cases says we've got a testing gap,
which probably ought to be filled.
ISTM that 6aaaa76bb left a lot on the table too; would it have been
that hard to support other roles in the cases it added, given
that (at least most of) the ACL infrastructure is there? I do agree
with the idea that GRANT <role> and GRANT <privilege> ought to
behave the same on this point --- but we have to do that by adding
capability, not subtracting it.
There do seem to be some bugs/limitations in the GRANT <role> case.
Trying this in v13:
regression=# create user user1;
CREATE ROLE
regression=# create user user2;
CREATE ROLE
regression=# create user user3;
CREATE ROLE
regression=# grant user3 to user2 with admin option;
GRANT ROLE
regression=# \c - user2
You are now connected to database "regression" as user "user2".
regression=> grant user3 to user1 granted by postgres;
ERROR: must be superuser to set grantor
regression=> grant user3 to user1 granted by user3;
ERROR: must have admin option on role "user3"
regression=> grant user3 to user1 granted by user2;
GRANT ROLE
So the "must be superuser" message is pretty misleading, it should
be more like "must be superuser to set grantor to another role".
And the second error is flat out wrong, since user2 *does* have that
admin option. Both of those errors are coming out of AddRoleMems,
but I didn't look at the code in detail.
regards, tom lane
> On 27 Dec 2021, at 23:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> On 27 Dec 2021, at 17:02, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> A superuser, or really anyone who's a member of the user1 role, >>> ought to be able to do that (especially since it used to be allowed). >>> So it seems the permissions check was coded incorrectly. > >> Reading the SQL spec for GRANT and REVOKE, and specifically the "Grantor >> Determination" subsection, it's not clear to me that this is wrong *per spec* >> and that any value except CURRENT_USER and CURRENT_ROLE is supported (which is >> what 6aaaa76bb implemented and the above referenced commit amended). Given the >> time of day I'm undercaffeinated for spec reading so I might be missing >> something though. Is <grantor> really handled differently for GRANT/REVOKE >> ROLE to PRIVILEGE? > > Not sure. However, it is certainly true that we have long supported > other grantors in GRANT <role> ... GRANTED BY (at least back to 8.4, > I didn't try anything older). Whether that's within the spec or an > extension, breaking the case now isn't okay. Agreed. > So the code change in b2a459edf is flat wrong AFAICS. The fact that > it didn't break any existing test cases says we've got a testing gap, > which probably ought to be filled. > > ISTM that 6aaaa76bb left a lot on the table too; would it have been > that hard to support other roles in the cases it added, given > that (at least most of) the ACL infrastructure is there? I do agree > with the idea that GRANT <role> and GRANT <privilege> ought to > behave the same on this point --- but we have to do that by adding > capability, not subtracting it. I'll craft a revert of b2a459edf when I'm less tired, a proper fix can then be worked on separately. -- Daniel Gustafsson https://vmware.com/
> On 28 Dec 2021, at 00:26, Daniel Gustafsson <daniel@yesql.se> wrote: > I'll craft a revert of b2a459edf when I'm less tired, a proper fix can then be > worked on separately. I prepared a revert patch tonight and ran it through CI, but won't have ample time to monitor the buildfarm so I have it slated for pushing in the morning. I opted for keeping the tests even though they aren't excercising all that much code but they at least show the current behavior. -- Daniel Gustafsson https://vmware.com/