pg_dump does not handle indirectly-granted permissions properly
AFAICT, pg_dump has no notion that it needs to be careful about the order
in which permissions are granted. I did
regression=# create user joe;
CREATE ROLE
regression=# create user bob;
CREATE ROLE
regression=# create user alice;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> create table joestable(f1 int);
CREATE TABLE
regression=> grant select on joestable to alice with grant option;
GRANT
regression=> \c - alice
You are now connected to database "regression" as user "alice".
regression=> grant select on joestable to bob;
GRANT
and then pg_dump'd that. The ACL entry for joestable looks like this:
--
-- TOC entry 5642 (class 0 OID 0)
-- Dependencies: 606
-- Name: joestable; Type: ACL; Schema: public; Owner: joe
--
SET SESSION AUTHORIZATION alice;
GRANT SELECT ON TABLE joestable TO bob;
RESET SESSION AUTHORIZATION;
GRANT SELECT ON TABLE joestable TO alice WITH GRANT OPTION;
Unsurprisingly, that fails to restore:
db2=# SET SESSION AUTHORIZATION alice;
SET
db2=> GRANT SELECT ON TABLE joestable TO bob;
ERROR: permission denied for relation joestable
I am not certain whether this is a newly introduced bug or not.
However, I tried it in 9.2-9.6, and all of them produce the
GRANTs in the right order:
GRANT SELECT ON TABLE joestable TO alice WITH GRANT OPTION;
SET SESSION AUTHORIZATION alice;
GRANT SELECT ON TABLE joestable TO bob;
RESET SESSION AUTHORIZATION;
That might be just chance, but my current bet is that Stephen
broke it sometime in the v10 cycle --- especially since we
haven't heard any complaints like this from the field.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom,
On Tue, Jul 25, 2017 at 16:43 Tom Lane <tgl@sss.pgh.pa.us> wrote:
AFAICT, pg_dump has no notion that it needs to be careful about the order
in which permissions are granted. I didregression=# create user joe;
CREATE ROLE
regression=# create user bob;
CREATE ROLE
regression=# create user alice;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> create table joestable(f1 int);
CREATE TABLE
regression=> grant select on joestable to alice with grant option;
GRANT
regression=> \c - alice
You are now connected to database "regression" as user "alice".
regression=> grant select on joestable to bob;
GRANTand then pg_dump'd that. The ACL entry for joestable looks like this:
--
-- TOC entry 5642 (class 0 OID 0)
-- Dependencies: 606
-- Name: joestable; Type: ACL; Schema: public; Owner: joe
--SET SESSION AUTHORIZATION alice;
GRANT SELECT ON TABLE joestable TO bob;
RESET SESSION AUTHORIZATION;
GRANT SELECT ON TABLE joestable TO alice WITH GRANT OPTION;Unsurprisingly, that fails to restore:
db2=# SET SESSION AUTHORIZATION alice;
SET
db2=> GRANT SELECT ON TABLE joestable TO bob;
ERROR: permission denied for relation joestableI am not certain whether this is a newly introduced bug or not.
However, I tried it in 9.2-9.6, and all of them produce the
GRANTs in the right order:GRANT SELECT ON TABLE joestable TO alice WITH GRANT OPTION;
SET SESSION AUTHORIZATION alice;
GRANT SELECT ON TABLE joestable TO bob;
RESET SESSION AUTHORIZATION;That might be just chance, but my current bet is that Stephen
broke it sometime in the v10 cycle --- especially since we
haven't heard any complaints like this from the field.
Hmmm. I'll certainly take a look when I get back to a laptop, but I can't
recall offhand any specific code for handling that, nor what change I might
have made in the v10 cycle which would have broken it (if anything, I would
have expected an issue from the rework in 9.6...).
I should be able to look at this tomorrow though.
Thanks!
Stephen
On 25 July 2017 at 21:47, Stephen Frost <sfrost@snowman.net> wrote:
Tom,
On Tue, Jul 25, 2017 at 16:43 Tom Lane <tgl@sss.pgh.pa.us> wrote:
AFAICT, pg_dump has no notion that it needs to be careful about the order
in which permissions are granted. I didregression=# create user joe;
CREATE ROLE
regression=# create user bob;
CREATE ROLE
regression=# create user alice;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> create table joestable(f1 int);
CREATE TABLE
regression=> grant select on joestable to alice with grant option;
GRANT
regression=> \c - alice
You are now connected to database "regression" as user "alice".
regression=> grant select on joestable to bob;
GRANTand then pg_dump'd that. The ACL entry for joestable looks like this:
--
-- TOC entry 5642 (class 0 OID 0)
-- Dependencies: 606
-- Name: joestable; Type: ACL; Schema: public; Owner: joe
--SET SESSION AUTHORIZATION alice;
GRANT SELECT ON TABLE joestable TO bob;
RESET SESSION AUTHORIZATION;
GRANT SELECT ON TABLE joestable TO alice WITH GRANT OPTION;Unsurprisingly, that fails to restore:
db2=# SET SESSION AUTHORIZATION alice;
SET
db2=> GRANT SELECT ON TABLE joestable TO bob;
ERROR: permission denied for relation joestableI am not certain whether this is a newly introduced bug or not.
However, I tried it in 9.2-9.6, and all of them produce the
GRANTs in the right order:GRANT SELECT ON TABLE joestable TO alice WITH GRANT OPTION;
SET SESSION AUTHORIZATION alice;
GRANT SELECT ON TABLE joestable TO bob;
RESET SESSION AUTHORIZATION;That might be just chance, but my current bet is that Stephen
broke it sometime in the v10 cycle --- especially since we
haven't heard any complaints like this from the field.Hmmm. I'll certainly take a look when I get back to a laptop, but I can't
recall offhand any specific code for handling that, nor what change I might
have made in the v10 cycle which would have broken it (if anything, I would
have expected an issue from the rework in 9.6...).I should be able to look at this tomorrow though.
This is the culprit:
commit 23f34fa4ba358671adab16773e79c17c92cbc870
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Apr 6 21:45:32 2016 -0400
In pg_dump, include pg_catalog and extension ACLs, if changed
Now that all of the infrastructure exists, add in the ability to
dump out the ACLs of the objects inside of pg_catalog or the ACLs
for objects which are members of extensions, but only if they have
been changed from their original values.
The original values are tracked in pg_init_privs. When pg_dump'ing
9.6-and-above databases, we will dump out the ACLs for all objects
in pg_catalog and the ACLs for all extension members, where the ACL
has been changed from the original value which was set during either
initdb or CREATE EXTENSION.
This should not change dumps against pre-9.6 databases.
Reviews by Alexander Korotkov, Jose Luis Tallon
Thom
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thom,
* Thom Brown (thom@linux.com) wrote:
This is the culprit:
commit 23f34fa4ba358671adab16773e79c17c92cbc870
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Apr 6 21:45:32 2016 -0400
Thanks! I'll take a look tomorrow.
Stephen
On 26 July 2017 at 00:52, Stephen Frost <sfrost@snowman.net> wrote:
Thom,
* Thom Brown (thom@linux.com) wrote:
This is the culprit:
commit 23f34fa4ba358671adab16773e79c17c92cbc870
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Apr 6 21:45:32 2016 -0400Thanks! I'll take a look tomorrow.
I should point out that this commit was made during the 9.6 cycle, and
I get the same issue with 9.6.
Thom
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thom,
On Tue, Jul 25, 2017 at 20:29 Thom Brown <thom@linux.com> wrote:
On 26 July 2017 at 00:52, Stephen Frost <sfrost@snowman.net> wrote:
Thom,
* Thom Brown (thom@linux.com) wrote:
This is the culprit:
commit 23f34fa4ba358671adab16773e79c17c92cbc870
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Apr 6 21:45:32 2016 -0400Thanks! I'll take a look tomorrow.
I should point out that this commit was made during the 9.6 cycle, and
I get the same issue with 9.6.
Interesting that Tom didn't. Still, that does make more sense to me.
Thanks!
Stephen
Show quoted text
Stephen Frost <sfrost@snowman.net> writes:
On Tue, Jul 25, 2017 at 20:29 Thom Brown <thom@linux.com> wrote:
I should point out that this commit was made during the 9.6 cycle, and
I get the same issue with 9.6.
Interesting that Tom didn't. Still, that does make more sense to me.
Yeah, it makes more sense to me too, but nonetheless that's the result
I get. I suspect there is an element of indeterminacy here.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/26/2017 02:12 AM, Tom Lane wrote:
AFAICT, pg_dump has no notion that it needs to be careful about the order
in which permissions are granted. I didregression=# create user joe;
CREATE ROLE
regression=# create user bob;
CREATE ROLE
regression=# create user alice;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> create table joestable(f1 int);
CREATE TABLE
regression=> grant select on joestable to alice with grant option;
GRANT
regression=> \c - alice
You are now connected to database "regression" as user "alice".
regression=> grant select on joestable to bob;
GRANTand then pg_dump'd that. The ACL entry for joestable looks like this:
--
-- TOC entry 5642 (class 0 OID 0)
-- Dependencies: 606
-- Name: joestable; Type: ACL; Schema: public; Owner: joe
--SET SESSION AUTHORIZATION alice;
GRANT SELECT ON TABLE joestable TO bob;
RESET SESSION AUTHORIZATION;
GRANT SELECT ON TABLE joestable TO alice WITH GRANT OPTION;Unsurprisingly, that fails to restore:
db2=# SET SESSION AUTHORIZATION alice;
SET
db2=> GRANT SELECT ON TABLE joestable TO bob;
ERROR: permission denied for relation joestableI am not certain whether this is a newly introduced bug or not.
However, I tried it in 9.2-9.6, and all of them produce the
GRANTs in the right order:GRANT SELECT O
I am also getting the same error while doing pg_upgrade from v9.6 to v10
,although not able to reproduce v9.5->v9.6 pg_upgrade.
v9.6
CREATE TABLE deptest (f1 serial primary key, f2 text);
\set VERBOSITY default
CREATE USER regress_dep_user0;
CREATE USER regress_dep_user1;
CREATE USER regress_dep_user2;
SET SESSION AUTHORIZATION regress_dep_user0;
REASSIGN OWNED BY regress_dep_user0 TO regress_dep_user1;
REASSIGN OWNED BY regress_dep_user1 TO regress_dep_user0;
CREATE TABLE deptest1 (f1 int unique);
GRANT ALL ON deptest1 TO regress_dep_user1 WITH GRANT OPTION;
SET SESSION AUTHORIZATION regress_dep_user1;
GRANT ALL ON deptest1 TO regress_dep_user2;
v10 - run pg_upgrade.
--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
... btw, while you're working on this, it'd be nice if you fixed the
header comment for dumpACL(). It is unintelligible as to what racls
is, and apparently feels that it need not discuss initacls or initracls
at all. I can't say that the reference to "fooacl" is really obvious
either.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
AFAICT, pg_dump has no notion that it needs to be careful about the order
in which permissions are granted. I did
I'm afraid that's correct, though I believe that's always been the case.
I spent some time looking into this today and from what I've gathered so
far, there's essentially an implicit (or at least, I couldn't find any
explicit reference to it) ordering in ACLs whereby a role which was
given a GRANT OPTION always appears in the ACL list before an ACL entry
where that role is GRANT'ing that option to another role, and this is
what pg_dump was (again, implicitly, it seems) depending on to get this
correct in prior versions.
Pulling apart the ACL list and rebuilding it to handle adding/revoking
of default options on objects ends up, in some cases, changing the
ordering around for the ACLs and that's how pg_dump comes to emit the
GRANT commands in the wrong order.
Looks like what is needed is an explicit ordering to the ACLs in
pg_dump to ensure that it emits the GRANTs in the correct order, which
should be that a given GRANTOR's rights must be before any ACL which
that GRATOR granted. Ideally, that could be crafted into the SQL query
which is sent to the server, but I haven't quite figured out if that'll
be possible or not. If not, it shouldn't be too hard to implement in
pg_dump directly.
I don't, at the moment, think we actually need to do any checks in the
backend code to make sure that the implicit ordering is always held,
assuming we make this change in pg_dump. I do wonder if it might be
possible, with the correct set of GRANTs (perhaps having role
memberships coming into play also, as discussed in the header of
select_best_grantor()) to generate an ACL list with an "incorrect"
ordering which would end up causing issues in older releases with
pg_dump. We've had precious little complaints from the field about this
and so, even if we were to generate such a case, I'm not sure that we'd
want to add all the code necessary to avoid it and then back-patch it.
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
AFAICT, pg_dump has no notion that it needs to be careful about the order
in which permissions are granted. I did
I'm afraid that's correct, though I believe that's always been the case.
I spent some time looking into this today and from what I've gathered so
far, there's essentially an implicit (or at least, I couldn't find any
explicit reference to it) ordering in ACLs whereby a role which was
given a GRANT OPTION always appears in the ACL list before an ACL entry
where that role is GRANT'ing that option to another role, and this is
what pg_dump was (again, implicitly, it seems) depending on to get this
correct in prior versions.
Yeah, I suspected that was what made it work before. I think the ordering
just comes from the fact that new ACLs are added to the end, and you can't
add an entry as a non-owner unless there's a grant WGO there already.
Pulling apart the ACL list and rebuilding it to handle adding/revoking
of default options on objects ends up, in some cases, changing the
ordering around for the ACLs and that's how pg_dump comes to emit the
GRANT commands in the wrong order.
Right.
I don't, at the moment, think we actually need to do any checks in the
backend code to make sure that the implicit ordering is always held,
assuming we make this change in pg_dump. I do wonder if it might be
possible, with the correct set of GRANTs (perhaps having role
memberships coming into play also, as discussed in the header of
select_best_grantor()) to generate an ACL list with an "incorrect"
ordering which would end up causing issues in older releases with
pg_dump. We've had precious little complaints from the field about this
and so, even if we were to generate such a case, I'm not sure that we'd
want to add all the code necessary to avoid it and then back-patch it.
I suspect it would be easier to modify the backend code that munges ACL
lists so that it takes care to preserve that property, if we ever find
there are cases where it does not. It seems plausible to me that
pg_dump is not the only code that depends on that ordering.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Stephen Frost <sfrost@snowman.net> writes:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
AFAICT, pg_dump has no notion that it needs to be careful about the order
in which permissions are granted. I didI'm afraid that's correct, though I believe that's always been the case.
I spent some time looking into this today and from what I've gathered so
far, there's essentially an implicit (or at least, I couldn't find any
explicit reference to it) ordering in ACLs whereby a role which was
given a GRANT OPTION always appears in the ACL list before an ACL entry
where that role is GRANT'ing that option to another role, and this is
what pg_dump was (again, implicitly, it seems) depending on to get this
correct in prior versions.Yeah, I suspected that was what made it work before. I think the ordering
just comes from the fact that new ACLs are added to the end, and you can't
add an entry as a non-owner unless there's a grant WGO there already.
Right.
I suspect it would be easier to modify the backend code that munges ACL
lists so that it takes care to preserve that property, if we ever find
there are cases where it does not. It seems plausible to me that
pg_dump is not the only code that depends on that ordering.
Hm, well, if we're alright with that then I think the attached would
address the pg_dump issue, and I believe this would work as this code is
only run for 9.6+ servers anyway.
This needs more cleanup, testing, and comments explaining why we're
doing this (and then perhaps comments, somewhere.. in the backend ACL
code that explains that the ordering needs to be preserved), but the
basic idea seems sound to me and the case you presented does work with
this patch (for me, at least) whereas what's in master didn't.
Thoughts?
Thanks!
Stephen
Attachments:
fix_pg_dump_grant_ordering_v1-master.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
new file mode 100644
index dfc6118..8686ed0
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
*************** buildACLQueries(PQExpBuffer acl_subquery
*** 723,742 ****
* these are run the initial privileges will be in place, even in a binary
* upgrade situation (see below).
*/
! printfPQExpBuffer(acl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
! "(SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) AS acl "
! "EXCEPT "
! "SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s)))) as foo)",
acl_column,
obj_kind,
acl_owner,
obj_kind,
acl_owner);
! printfPQExpBuffer(racl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
! "(SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) AS acl "
! "EXCEPT "
! "SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s)))) as foo)",
obj_kind,
acl_owner,
acl_column,
--- 723,742 ----
* these are run the initial privileges will be in place, even in a binary
* upgrade situation (see below).
*/
! printfPQExpBuffer(acl_subquery, "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
! "(SELECT acl, row_n FROM pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) WITH ORDINALITY AS perm(acl,row_n) "
! "WHERE NOT EXISTS ( "
! "SELECT 1 FROM pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) AS init(init_acl) WHERE acl = init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
obj_kind,
acl_owner);
! printfPQExpBuffer(racl_subquery, "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
! "(SELECT acl, row_n FROM pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) WITH ORDINALITY AS initp(acl,row_n) "
! "WHERE NOT EXISTS ( "
! "SELECT 1 FROM pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) AS permp(orig_acl) WHERE acl = orig_acl)) as foo)",
obj_kind,
acl_owner,
acl_column,
*************** buildACLQueries(PQExpBuffer acl_subquery
*** 761,779 ****
{
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e' THEN "
! "(SELECT pg_catalog.array_agg(acl) FROM "
! "(SELECT pg_catalog.unnest(pip.initprivs) AS acl "
! "EXCEPT "
! "SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s))) as foo) END",
obj_kind,
acl_owner);
printfPQExpBuffer(init_racl_subquery,
"CASE WHEN privtype = 'e' THEN "
"(SELECT pg_catalog.array_agg(acl) FROM "
! "(SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) AS acl "
! "EXCEPT "
! "SELECT pg_catalog.unnest(pip.initprivs)) as foo) END",
obj_kind,
acl_owner);
}
--- 761,779 ----
{
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e' THEN "
! "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
! "(SELECT acl, row_n FROM pg_catalog.unnest(pip.initprivs) WITH ORDINALITY AS initp(acl,row_n) "
! "WHERE NOT EXISTS ( "
! "SELECT 1 FROM pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) AS privm(orig_acl) WHERE acl = orig_acl)) as foo) END",
obj_kind,
acl_owner);
printfPQExpBuffer(init_racl_subquery,
"CASE WHEN privtype = 'e' THEN "
"(SELECT pg_catalog.array_agg(acl) FROM "
! "(SELECT acl, row_n FROM pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) WITH ORDINALITY AS privp(acl,row_n) "
! "WHERE NOT EXISTS ( "
! "SELECT 1 FROM pg_catalog.unnest(pip.initprivs) AS initp(init_acl) WHERE acl = init_acl)) as foo) END",
obj_kind,
acl_owner);
}
Tom, all,
* Stephen Frost (sfrost@snowman.net) wrote:
This needs more cleanup, testing, and comments explaining why we're
doing this (and then perhaps comments, somewhere.. in the backend ACL
code that explains that the ordering needs to be preserved), but the
basic idea seems sound to me and the case you presented does work with
this patch (for me, at least) whereas what's in master didn't.
Alright, here's an updated patch which cleans things up a bit and adds
comments to explain what's going on. I also updated the comments in
acl.h to explain that ordering actually does matter.
I've tried a bit to break the ordering in the backend a bit but there
could probably be more effort put into that, if I'm being honest.
Still, this definitely fixes the case which was being complained about
and therefore is a step in the right direction.
It's a bit late here, so I'll push this in the morning and watch the
buildfarm.
Thanks!
Stephen
Attachments:
fix_pg_dump_grant_ordering_v2-master.patchtext/x-diff; charset=us-asciiDownload
From cbca78a387ecfed9570bd079541ec7906c990f0f Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Thu, 3 Aug 2017 21:23:37 -0400
Subject: [PATCH] Fix ordering in pg_dump of GRANTs
The order in which GRANTs are output is important as GRANTs which have
been GRANT'd by individuals via WITH GRANT OPTION GRANTs have to come
after the GRANT which included the WITH GRANT OPTION. This happens
naturally in the backend during normal operation as we only change
existing ACLs in-place, only add new ACLs to the end, and when removing
an ACL we remove any which depend on it also.
Also, adjust the comments in acl.h to make this clear.
Unfortunately, the updates to pg_dump to handle initial privileges
involved pulling apart ACLs and then combining them back together and
could end up putting them back together in an invalid order, leading to
dumps which wouldn't restore.
Fix this by adjusting the queries used by pg_dump to ensure that the
ACLs are rebuilt in the same order in which they were originally.
Back-patch to 9.6 where the changes for initial privileges were done.
---
src/bin/pg_dump/dumputils.c | 51 ++++++++++++++++++++++++++++++++-------------
src/include/utils/acl.h | 14 ++++++++++---
2 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index dfc6118..67049a6 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -722,21 +722,36 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
* We always perform this delta on all ACLs and expect that by the time
* these are run the initial privileges will be in place, even in a binary
* upgrade situation (see below).
+ *
+ * Finally, the order in which privileges are in the ACL string (the order
+ * they been GRANT'd in, which the backend maintains) must be preserved to
+ * ensure that GRANTs WITH GRANT OPTION and subsequent GRANTs based on
+ * those are dumped in the correct order.
*/
- printfPQExpBuffer(acl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(acl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS perm(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "AS init(init_acl) WHERE acl = init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
obj_kind,
acl_owner);
- printfPQExpBuffer(racl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(racl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "AS permp(orig_acl) WHERE acl = orig_acl)) as foo)",
obj_kind,
acl_owner,
acl_column,
@@ -761,19 +776,25 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
{
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e' THEN "
- "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pip.initprivs) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s))) as foo) END",
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM pg_catalog.unnest(pip.initprivs) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "AS privm(orig_acl) WHERE acl = orig_acl)) as foo) END",
obj_kind,
acl_owner);
printfPQExpBuffer(init_racl_subquery,
"CASE WHEN privtype = 'e' THEN "
"(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pip.initprivs)) as foo) END",
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "WITH ORDINALITY AS privp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM pg_catalog.unnest(pip.initprivs) "
+ "AS initp(init_acl) WHERE acl = init_acl)) as foo) END",
obj_kind,
acl_owner);
}
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 43273ea..316eef9 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -12,9 +12,17 @@
* NOTES
* An ACL array is simply an array of AclItems, representing the union
* of the privileges represented by the individual items. A zero-length
- * array represents "no privileges". There are no assumptions about the
- * ordering of the items, but we do expect that there are no two entries
- * in the array with the same grantor and grantee.
+ * array represents "no privileges".
+ *
+ * The order of items in the array is important as client utilities (in
+ * particular, pg_dump, though possibly other clients) expect to be able
+ * to issue GRANTs in the ordering of the items in the array. The reason
+ * this matters is that GRANTs WITH GRANT OPTION must be before any GRANTs
+ * which depend on it. This happens naturally in the backend during
+ * operations as we update ACLs in-place, new items are appended, and
+ * existing entries are only removed if there's no dependency on them (no
+ * GRANT can been based on it, or, if there was, those GRANTs are also
+ * removed).
*
* For backward-compatibility purposes we have to allow null ACL entries
* in system catalogs. A null ACL will be treated as meaning "default
--
2.7.4
Tom, all,
* Stephen Frost (sfrost@snowman.net) wrote:
Alright, here's an updated patch which cleans things up a bit and adds
comments to explain what's going on. I also updated the comments in
acl.h to explain that ordering actually does matter.
Getting back to this, here's rebased patches for master/v10 and 9.6
(which only had whitespace differences, probably pgindent to blame
there..).
I'm going to push these later today unless there's other comments on it.
Thanks!
Stephen
Attachments:
fix_pg_dump_ordering_v2-master.patchtext/x-diff; charset=us-asciiDownload
From ac86eb5451492bcb72f25cceab4ae467716ea073 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Thu, 3 Aug 2017 21:23:37 -0400
Subject: [PATCH] Fix ordering in pg_dump of GRANTs
The order in which GRANTs are output is important as GRANTs which have
been GRANT'd by individuals via WITH GRANT OPTION GRANTs have to come
after the GRANT which included the WITH GRANT OPTION. This happens
naturally in the backend during normal operation as we only change
existing ACLs in-place, only add new ACLs to the end, and when removing
an ACL we remove any which depend on it also.
Also, adjust the comments in acl.h to make this clear.
Unfortunately, the updates to pg_dump to handle initial privileges
involved pulling apart ACLs and then combining them back together and
could end up putting them back together in an invalid order, leading to
dumps which wouldn't restore.
Fix this by adjusting the queries used by pg_dump to ensure that the
ACLs are rebuilt in the same order in which they were originally.
Back-patch to 9.6 where the changes for initial privileges were done.
---
src/bin/pg_dump/dumputils.c | 51 ++++++++++++++++++++++++++++++++-------------
src/include/utils/acl.h | 14 ++++++++++---
2 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index dfc611848b..e4c95feb63 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -722,21 +722,36 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
* We always perform this delta on all ACLs and expect that by the time
* these are run the initial privileges will be in place, even in a binary
* upgrade situation (see below).
+ *
+ * Finally, the order in which privileges are in the ACL string (the order
+ * they been GRANT'd in, which the backend maintains) must be preserved to
+ * ensure that GRANTs WITH GRANT OPTION and subsequent GRANTs based on
+ * those are dumped in the correct order.
*/
- printfPQExpBuffer(acl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(acl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS perm(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "AS init(init_acl) WHERE acl = init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
obj_kind,
acl_owner);
- printfPQExpBuffer(racl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(racl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "AS permp(orig_acl) WHERE acl = orig_acl)) as foo)",
obj_kind,
acl_owner,
acl_column,
@@ -761,19 +776,25 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
{
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e' THEN "
- "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pip.initprivs) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s))) as foo) END",
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM pg_catalog.unnest(pip.initprivs) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "AS privm(orig_acl) WHERE acl = orig_acl)) as foo) END",
obj_kind,
acl_owner);
printfPQExpBuffer(init_racl_subquery,
"CASE WHEN privtype = 'e' THEN "
"(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pip.initprivs)) as foo) END",
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "WITH ORDINALITY AS privp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM pg_catalog.unnest(pip.initprivs) "
+ "AS initp(init_acl) WHERE acl = init_acl)) as foo) END",
obj_kind,
acl_owner);
}
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 43273eaab5..254a811aff 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -12,9 +12,17 @@
* NOTES
* An ACL array is simply an array of AclItems, representing the union
* of the privileges represented by the individual items. A zero-length
- * array represents "no privileges". There are no assumptions about the
- * ordering of the items, but we do expect that there are no two entries
- * in the array with the same grantor and grantee.
+ * array represents "no privileges".
+ *
+ * The order of items in the array is important as client utilities (in
+ * particular, pg_dump, though possibly other clients) expect to be able
+ * to issue GRANTs in the ordering of the items in the array. The reason
+ * this matters is that GRANTs WITH GRANT OPTION must be before any GRANTs
+ * which depend on it. This happens naturally in the backend during
+ * operations as we update ACLs in-place, new items are appended, and
+ * existing entries are only removed if there's no dependency on them (no
+ * GRANT can been based on it, or, if there was, those GRANTs are also
+ * removed).
*
* For backward-compatibility purposes we have to allow null ACL entries
* in system catalogs. A null ACL will be treated as meaning "default
--
2.11.0
fix_pg_dump_ordering_v2-96.patchtext/x-diff; charset=us-asciiDownload
From 73523ccf2c1701797d47ee1c608ccd29c233a6dc Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Thu, 3 Aug 2017 21:23:37 -0400
Subject: [PATCH] Fix ordering in pg_dump of GRANTs
The order in which GRANTs are output is important as GRANTs which have
been GRANT'd by individuals via WITH GRANT OPTION GRANTs have to come
after the GRANT which included the WITH GRANT OPTION. This happens
naturally in the backend during normal operation as we only change
existing ACLs in-place, only add new ACLs to the end, and when removing
an ACL we remove any which depend on it also.
Also, adjust the comments in acl.h to make this clear.
Unfortunately, the updates to pg_dump to handle initial privileges
involved pulling apart ACLs and then combining them back together and
could end up putting them back together in an invalid order, leading to
dumps which wouldn't restore.
Fix this by adjusting the queries used by pg_dump to ensure that the
ACLs are rebuilt in the same order in which they were originally.
Back-patch to 9.6 where the changes for initial privileges were done.
---
src/bin/pg_dump/dumputils.c | 51 ++++++++++++++++++++++++++++++++-------------
src/include/utils/acl.h | 14 ++++++++++---
2 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 11870a33fa..c171978060 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -729,21 +729,36 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
* We always perform this delta on all ACLs and expect that by the time
* these are run the initial privileges will be in place, even in a binary
* upgrade situation (see below).
+ *
+ * Finally, the order in which privileges are in the ACL string (the order
+ * they been GRANT'd in, which the backend maintains) must be preserved to
+ * ensure that GRANTs WITH GRANT OPTION and subsequent GRANTs based on
+ * those are dumped in the correct order.
*/
- printfPQExpBuffer(acl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(acl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS perm(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "AS init(init_acl) WHERE acl = init_acl)) as foo)",
acl_column,
obj_kind,
acl_owner,
obj_kind,
acl_owner);
- printfPQExpBuffer(racl_subquery, "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s)))) as foo)",
+ printfPQExpBuffer(racl_subquery,
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(%s,%s))) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(coalesce(%s,pg_catalog.acldefault(%s,%s))) "
+ "AS permp(orig_acl) WHERE acl = orig_acl)) as foo)",
obj_kind,
acl_owner,
acl_column,
@@ -768,19 +783,25 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
{
printfPQExpBuffer(init_acl_subquery,
"CASE WHEN privtype = 'e' THEN "
- "(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pip.initprivs) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s))) as foo) END",
+ "(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM "
+ "(SELECT acl, row_n FROM pg_catalog.unnest(pip.initprivs) "
+ "WITH ORDINALITY AS initp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "AS privm(orig_acl) WHERE acl = orig_acl)) as foo) END",
obj_kind,
acl_owner);
printfPQExpBuffer(init_racl_subquery,
"CASE WHEN privtype = 'e' THEN "
"(SELECT pg_catalog.array_agg(acl) FROM "
- "(SELECT pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) AS acl "
- "EXCEPT "
- "SELECT pg_catalog.unnest(pip.initprivs)) as foo) END",
+ "(SELECT acl, row_n FROM "
+ "pg_catalog.unnest(pg_catalog.acldefault(%s,%s)) "
+ "WITH ORDINALITY AS privp(acl,row_n) "
+ "WHERE NOT EXISTS ( "
+ "SELECT 1 FROM pg_catalog.unnest(pip.initprivs) "
+ "AS initp(init_acl) WHERE acl = init_acl)) as foo) END",
obj_kind,
acl_owner);
}
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 5a4f1714f3..14279f79da 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -12,9 +12,17 @@
* NOTES
* An ACL array is simply an array of AclItems, representing the union
* of the privileges represented by the individual items. A zero-length
- * array represents "no privileges". There are no assumptions about the
- * ordering of the items, but we do expect that there are no two entries
- * in the array with the same grantor and grantee.
+ * array represents "no privileges".
+ *
+ * The order of items in the array is important as client utilities (in
+ * particular, pg_dump, though possibly other clients) expect to be able
+ * to issue GRANTs in the ordering of the items in the array. The reason
+ * this matters is that GRANTs WITH GRANT OPTION must be before any GRANTs
+ * which depend on it. This happens naturally in the backend during
+ * operations as we update ACLs in-place, new items are appended, and
+ * existing entries are only removed if there's no dependency on them (no
+ * GRANT can been based on it, or, if there was, those GRANTs are also
+ * removed).
*
* For backward-compatibility purposes we have to allow null ACL entries
* in system catalogs. A null ACL will be treated as meaning "default
--
2.11.0