Обсуждение: [HACKERS] 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

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 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.
>
>
> 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_catalogor the ACLs   for objects which are members of extensions, but only if they have   been changed from their
originalvalues.
 
   The original values are tracked in pg_init_privs.  When pg_dump'ing   9.6-and-above databases, we will dump out the
ACLsfor all objects   in pg_catalog and the ACLs for all extension members, where the ACL   has been changed from the
originalvalue 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

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 -0400
>
> Thanks!  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

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

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 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 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

... 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

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

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 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.

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

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

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