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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

PG Bug reporting form
The following bug has been logged on the website:

Bug reference:      15788
Logged by:          Nathan Bossart
Email address:      [hidden email]
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

Reply | Threaded
Open this post in threaded view
|

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

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

Nathan



v1-0001-Fix-ordering-of-GRANTs-for-databases-in-pg_dump.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Haribabu Kommi-2


On Thu, May 2, 2019 at 9:28 AM Bossart, Nathan <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Bossart, Nathan
On 5/1/19, 9:10 PM, "Haribabu Kommi" <[hidden email]> wrote:
> On Thu, May 2, 2019 at 9:28 AM Bossart, Nathan <[hidden email]> 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


v2-0001-Fix-ordering-of-GRANTs-for-databases-in-pg_dump-master-v11.patch (2K) Download Attachment
v2-0001-Fix-ordering-of-GRANTs-for-databases-in-pg_dump-v9.6.patch (2K) Download Attachment
v2-0001-Fix-ordering-of-GRANTs-for-databases-in-pg_dump-v10.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Bossart, Nathan
On 5/4/19, 5:33 AM, "Michael Paquier" <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

database-acls-v3.patch (2K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Bossart, Nathan
On 5/20/19, 1:27 AM, "Michael Paquier" <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
On Mon, May 20, 2019 at 10:37:50PM +0000, Bossart, Nathan wrote:
> On 5/20/19, 1:27 AM, "Michael Paquier" <[hidden email]> 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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Bossart, Nathan
On 5/21/19, 11:26 PM, "Michael Paquier" <[hidden email]> wrote:

> On Mon, May 20, 2019 at 10:37:50PM +0000, Bossart, Nathan wrote:
>> On 5/20/19, 1:27 AM, "Michael Paquier" <[hidden email]> 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