Horton

Thread

Commits

  1. Fix ordering of GRANT commands in pg_dumpall for database creation

  2. Fix ordering of GRANT commands in pg_dump for database creation

  1. BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    The Post Office <noreply@postgresql.org> — 2019-05-01T23:18:16Z

    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

  2. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Nathan Bossart <bossartn@amazon.com> — 2019-05-01T23:28:03Z

    It looks like the bug reporting form destroyed the patch.  I've
attached it again here.

Nathan

  3. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-05-02T04:08:59Z

    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

  4. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Nathan Bossart <bossartn@amazon.com> — 2019-05-03T17:22:47Z

    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

  5. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Michael Paquier <michael@paquier.xyz> — 2019-05-04T12:32:21Z

    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

  6. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Nathan Bossart <bossartn@amazon.com> — 2019-05-06T16:08:47Z

    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

  7. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Michael Paquier <michael@paquier.xyz> — 2019-05-20T08:27:05Z

    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

  8. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Nathan Bossart <bossartn@amazon.com> — 2019-05-20T22:37:50Z

    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

  9. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Michael Paquier <michael@paquier.xyz> — 2019-05-22T06:26:26Z

    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

  10. Re: BUG #15788: 'pg_dump --create' orders database GRANTs incorrectly

    Nathan Bossart <bossartn@amazon.com> — 2019-05-22T16:59:53Z

    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