Обсуждение: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly
The following bug has been logged on the website:
Bug reference: 15788
Logged by: Nathan Bossart
Email address: bossartn@amazon.com
PostgreSQL version: 11.2
Operating system: Linux
Description:
Hello,
Currently, 'pg_dump --create' will generate database GRANTs in the
wrong order, which can lead to WARNINGs or ERRORs when attempting to
restore its output. Here is a simple way to reproduce the issue:
1. As a superuser, run the following SQL commands.
CREATE ROLE a_user;
CREATE ROLE b_user WITH CREATEROLE CREATEDB;
CREATE ROLE c_user;
SET SESSION AUTHORIZATION b_user;
CREATE DATABASE mydb;
\c mydb
SET SESSION AUTHORIZATION b_user;
REVOKE ALL ON DATABASE mydb FROM public;
GRANT TEMPORARY ON DATABASE mydb TO c_user WITH GRANT OPTION;
SET SESSION AUTHORIZATION c_user;
GRANT TEMPORARY ON DATABASE mydb TO a_user;
2. Then, execute the following pg_dump and psql commands.
pg_dump mydb -C -s -f dump.sql
psql postgres -c "DROP DATABASE mydb;"
psql postgres -q -c "\\set ON_ERROR_STOP" -f dump.sql
The last psql command will fail with the following ERROR:
ERROR: permission denied for database mydb
I think the underlying issue is that the pg_dump query is sorting the
ACLs, which may not be the natural ordering. I was able to fix this
by making a very similar change to 68a7c24f in dumpDatabase().
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index db8ca40a78..28e78756a8 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2672,13 +2672,23 @@ dumpDatabase(Archive *fout)
"(%s datdba) AS dba, "
"pg_encoding_to_char(encoding) AS encoding, "
"datcollate, datctype,
datfrozenxid, datminmxid, "
- "(SELECT array_agg(acl
ORDER BY acl::text COLLATE \"C\") FROM ( "
- " SELECT
unnest(coalesce(datacl,acldefault('d',datdba))) AS acl "
- " EXCEPT SELECT
unnest(acldefault('d',datdba))) as datacls)"
+ "(SELECT array_agg(acl
ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM
"
+
"unnest(coalesce(datacl,acldefault('d',datdba))) "
+ "WITH ORDINALITY AS
perm(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+
"unnest(acldefault('d',datdba)) "
+ "AS init(init_acl) WHERE
acl = init_acl)) as datacls)"
" AS datacl, "
- "(SELECT array_agg(acl
ORDER BY acl::text COLLATE \"C\") FROM ( "
- " SELECT
unnest(acldefault('d',datdba)) AS acl "
- " EXCEPT SELECT
unnest(coalesce(datacl,acldefault('d',datdba)))) as rdatacls)"
+ "(SELECT array_agg(acl
ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM
"
+
"unnest(acldefault('d',datdba)) "
+ "WITH ORDINALITY AS
initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+
"unnest(coalesce(datacl,acldefault('d',datdba))) "
+ "AS permp(orig_acl) WHERE
acl = orig_acl)) as rdatacls)"
" AS rdatacl, "
"datistemplate,
datconnlimit, "
"(SELECT spcname FROM
pg_tablespace t WHERE t.oid = dattablespace) AS tablespace, "
Nathan
It looks like the bug reporting form destroyed the patch. I've attached it again here. Nathan
Вложения
On Thu, May 2, 2019 at 9:28 AM Bossart, Nathan <bossartn@amazon.com> wrote:
It looks like the bug reporting form destroyed the patch. I've
attached it again here.
Thanks, for the patch. Just to make sure that we didn't miss, a similar problem
can occur in v10 and v9.6 pg_dumpall. In v11, the database acl's dumping
mechanism is moved from pg_dumpall to pg_dump.
a similar change in pg_dumpall "dumpCreateDB" function works for 10 and 9.6.
Regards,
Haribabu Kommi
Fujitsu Australia
On 5/1/19, 9:10 PM, "Haribabu Kommi" <kommi.haribabu@gmail.com> wrote: > On Thu, May 2, 2019 at 9:28 AM Bossart, Nathan <bossartn@amazon.com> wrote: >> It looks like the bug reporting form destroyed the patch. I've >> attached it again here. > > Thanks, for the patch. Just to make sure that we didn't miss, a similar problem > can occur in v10 and v9.6 pg_dumpall. In v11, the database acl's dumping > mechanism is moved from pg_dumpall to pg_dump. > > a similar change in pg_dumpall "dumpCreateDB" function works for 10 and 9.6. Thanks for taking a look. I've attached all the versions of the patch needed to back-patch this down to 9.6, where this query was introduced. Nathan
Вложения
On Fri, May 03, 2019 at 05:22:47PM +0000, Bossart, Nathan wrote: > Thanks for taking a look. I've attached all the versions of the patch > needed to back-patch this down to 9.6, where this query was > introduced. This requires a careful lookup, and it may be too late for the next round of minor releases. Could you add it to the next commit fest [1] so as we don't forget about it? [1]: https://commitfest.postgresql.org/23/ -- Michael
Вложения
On 5/4/19, 5:33 AM, "Michael Paquier" <michael@paquier.xyz> wrote: > On Fri, May 03, 2019 at 05:22:47PM +0000, Bossart, Nathan wrote: >> Thanks for taking a look. I've attached all the versions of the patch >> needed to back-patch this down to 9.6, where this query was >> introduced. > > This requires a careful lookup, and it may be too late for the next > round of minor releases. Could you add it to the next commit fest [1] > so as we don't forget about it? Added here: https://commitfest.postgresql.org/23/2111/ Nathan
On Mon, May 06, 2019 at 04:08:47PM +0000, Bossart, Nathan wrote: > Added here: https://commitfest.postgresql.org/23/2111/ Thanks for adding the patch to the CF. With your patch attached the difference in the dump is plain: REVOKE CONNECT,TEMPORARY ON DATABASE mydb FROM PUBLIC; +GRANT TEMPORARY ON DATABASE mydb TO c_user WITH GRANT OPTION; SET SESSION AUTHORIZATION c_user; GRANT TEMPORARY ON DATABASE mydb TO a_user; RESET SESSION AUTHORIZATION; -GRANT TEMPORARY ON DATABASE mydb TO c_user WITH GRANT OPTION; So what happens is that the GRANT command to a_user fails when switching to the session context to c_user as this user does not have yet the authorization to perform this command. If the GRANT permissions assigned to c_user are moved prior its actual actions then the restore is able to work. I have been looking at it, and wondered first if we could have just used buildACLQueries(), until I noticed that we don't support initial privileges on databases, so the patch you have sent looks fine to me. I had first a hard time parsing the subqueries added, so I have tweaked your patch with more indentation, and a comment block with more details about why we need to preserve the ACL ordering (you will note that I don't have a lot of imagination here). v12 beta1 is going to ship soon, so let's wait for the version to be tagged before committing it. -- Michael
Вложения
On 5/20/19, 1:27 AM, "Michael Paquier" <michael@paquier.xyz> wrote: > I had first a hard time parsing the subqueries added, so I have > tweaked your patch with more indentation, and a comment block with > more details about why we need to preserve the ACL ordering (you will > note that I don't have a lot of imagination here). Thanks. Your version looks good to me. > v12 beta1 is going to ship soon, so let's wait for the version to be > tagged before committing it. Sounds good. Nathan
On Mon, May 20, 2019 at 10:37:50PM +0000, Bossart, Nathan wrote: > On 5/20/19, 1:27 AM, "Michael Paquier" <michael@paquier.xyz> wrote: >> I had first a hard time parsing the subqueries added, so I have >> tweaked your patch with more indentation, and a comment block with >> more details about why we need to preserve the ACL ordering (you will >> note that I don't have a lot of imagination here). > > Thanks. Your version looks good to me. Okay, I have done more tests, checks and tweaks on this stuff, and pushed the fix for databases down to 9.6. Now, I have noticed two separate issues while testing: 1) We need a similar fix for tablespaces and pg_dumpall 1-1) Take for example this set of commands, reusing your previous example: \! rm -rf /tmp/tbspc/ \! mkdir -p /tmp/tbspc/ CREATE ROLE a_user; CREATE ROLE b_user WITH SUPERUSER; CREATE ROLE c_user; CREATE TABLESPACE poo LOCATION '/tmp/tbspc/'; SET SESSION AUTHORIZATION b_user; REVOKE ALL ON TABLESPACE poo FROM public; GRANT CREATE ON TABLESPACE poo TO c_user WITH GRANT OPTION; SET SESSION AUTHORIZATION c_user; GRANT CREATE ON TABLESPACE poo TO a_user; 1-2) And then run pg_dumpall -g (-t is fine but including roles ensures a correct test), where you will notice the following GRANT ordering: CREATE TABLESPACE poo OWNER ioltas LOCATION '/tmp/tbspc'; SET SESSION AUTHORIZATION c_user; GRANT ALL ON TABLESPACE poo TO a_user; -- breaks here RESET SESSION AUTHORIZATION; GRANT ALL ON TABLESPACE poo TO c_user WITH GRANT OPTION; The problem comes from dumpTablespaces() which still has a broken ACL ordering logic. I'll start a new thread about that as this thread is actually now fixed. It would be interesting to refactor all that stuff, but I'd like to think that it would be much more interesting if we could get pg_init_privs to work with databases and tablespace instead, but that would not be back-patchable. So I'll focus on the bug fix. 2) On HEAD, before the patch, I am able to trigger the following failure in libpq when using for example pg_dumpall and a 9.5 backend: pg_dump: column number -1 is out of range 0..36 I am not sure yet what's exactly causing that, but this is just a heads-up. -- Michael
Вложения
On 5/21/19, 11:26 PM, "Michael Paquier" <michael@paquier.xyz> wrote: > On Mon, May 20, 2019 at 10:37:50PM +0000, Bossart, Nathan wrote: >> On 5/20/19, 1:27 AM, "Michael Paquier" <michael@paquier.xyz> wrote: >>> I had first a hard time parsing the subqueries added, so I have >>> tweaked your patch with more indentation, and a comment block with >>> more details about why we need to preserve the ACL ordering (you will >>> note that I don't have a lot of imagination here). >> >> Thanks. Your version looks good to me. > > Okay, I have done more tests, checks and tweaks on this stuff, and > pushed the fix for databases down to 9.6. Thanks! > The problem comes from dumpTablespaces() which still has a broken ACL > ordering logic. I'll start a new thread about that as this thread is > actually now fixed. It would be interesting to refactor all that > stuff, but I'd like to think that it would be much more interesting if > we could get pg_init_privs to work with databases and tablespace > instead, but that would not be back-patchable. So I'll focus on the > bug fix. I'll take a look at this. Nathan