incorrect pg_dump output due to not handling dropped roles correctly

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

incorrect pg_dump output due to not handling dropped roles correctly

Floris Van Nee

I noticed I wasn't able to apply my usual pg_dump schema output without errors anymore after I dropped some roles. After some digging, I found it has to do with Postgres not correctly updating the pg_init_privs table upon dropping roles. I can reproduce a similar scenario with the following steps (output from v13devel, but AFAIK all versions affected, I ran into the issue on v11.2):


postgres=# create role test;
CREATE ROLE
postgres=# alter default privileges in schema public grant all privileges on tables to test;
ALTER DEFAULT PRIVILEGES
postgres=# create extension pg_stat_statements;
CREATE EXTENSION
postgres=# select * from pg_catalog.pg_init_privs
where objoid=(select 'pg_stat_statements'::regclass);
 objoid | classoid | objsubid | privtype |                                   initprivs                                   
--------+----------+----------+----------+-------------------------------------------------------------------------------
  16409 |     1259 |        0 | e        | {florisvannee=arwdDxt/florisvannee,test=arwdDxt/florisvannee,=r/florisvannee}
(1 row)

postgres=# drop owned by test;
DROP OWNED
postgres=# drop role test;
DROP ROLE
postgres=# select * from pg_catalog.pg_init_privs
where objoid=(select 'pg_stat_statements'::regclass);
 objoid | classoid | objsubid | privtype |                                   initprivs                                    
--------+----------+----------+----------+--------------------------------------------------------------------------------
  16409 |     1259 |        0 | e        | {florisvannee=arwdDxt/florisvannee,16404=arwdDxt/florisvannee,=r/florisvannee}
(1 row)


If we do a pg_dump on this, there'll be a line like this in the output:


REVOKE ALL ON TABLE public.pg_stat_statements FROM "16404";


This fails when restoring, because there's no role '16404'.



Can I manually fix this by updating pg_init_privs catalog table? Eg. in the example, I could run something like?


update pg_catalog.pg_init_privs
set initprivs=(select array_agg(p) from unnest(initprivs) p where not (p::text like '16404%'))
where initprivs <> (select array_agg(p) from unnest(initprivs) p where not (p::text like '16404%'))
;

Dropping/recreating the extension seems to work too, but I'd like to avoid that if possible (that may be a solution for pg_stat_statements, but isn't necessarily possible for every extension).


I'm pretty sure I once ran into a similar issue before, when doing a pg_upgrade in-place from 10 to 11. I couldn't run pg_upgrade until - back then I fixed it by dropping/recreating the extension, but didn't know exactly what was causing it, so I didn't report it here. From glancing at the code, this seems to be following some similar code paths in dump/restore. Just so you know the impact may not be limited to manual pg_dump actions, but also potentially pg_upgrade if my memory is correct.



-Floris


Reply | Threaded
Open this post in threaded view
|

Re: incorrect pg_dump output due to not handling dropped roles correctly

Tom Lane-2
Floris Van Nee <[hidden email]> writes:
> I noticed I wasn't able to apply my usual pg_dump schema output without errors anymore after I dropped some roles. After some digging, I found it has to do with Postgres not correctly updating the pg_init_privs table upon dropping roles. I can reproduce a similar scenario with the following steps (output from v13devel, but AFAIK all versions affected, I ran into the issue on v11.2):

Hm, looks like we forgot to teach the dependency mechanism about
pg_init_privs privileges?  (I didn't look yet.)

> Can I manually fix this by updating pg_init_privs catalog table?

Yeah, I think that would work.  Obviously best to test in a throwaway
database ...

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: incorrect pg_dump output due to not handling dropped roles correctly

Floris Van Nee
> Yeah, I think that would work.  Obviously best to test in a throwaway
> database ...

Thanks, Tom. Fixed it with that query.

As for the proper fix, indeed it looks like the 'drop role' needs to be taught to update pg_init_privs.

-Floris



Reply | Threaded
Open this post in threaded view
|

Re: incorrect pg_dump output due to not handling dropped roles correctly

Michael Paquier-2
On Sun, Nov 17, 2019 at 08:56:13PM +0000, Floris Van Nee wrote:
> As for the proper fix, indeed it looks like the 'drop role' needs to
> be taught to update pg_init_privs.

The dependencies related to the ACL entries exist in pg_shdepend
between the role and the revoked objects, and these get removed when
issuing DROP OWNED BY.  So it seems to me that the cleanup needs to
happen when issuing the DROP OWNED BY query, and not DROP ROLE.
Looking at the code, it seems to me that we should just patch
shdepDropOwned() to handle properly the removal of the role in ACL
objects in pg_init_privs for all the objects we are removing a
dependency on.  I am just diving into a patch..
--
Michael

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

Re: incorrect pg_dump output due to not handling dropped roles correctly

Michael Paquier-2
On Mon, Nov 18, 2019 at 12:10:59PM +0900, Michael Paquier wrote:
> The dependencies related to the ACL entries exist in pg_shdepend
> between the role and the revoked objects, and these get removed when
> issuing DROP OWNED BY.  So it seems to me that the cleanup needs to
> happen when issuing the DROP OWNED BY query, and not DROP ROLE.
> Looking at the code, it seems to me that we should just patch
> shdepDropOwned() to handle properly the removal of the role in ACL
> objects in pg_init_privs for all the objects we are removing a
> dependency on.  I am just diving into a patch..

Okay, I have been looking more at the code and as CREATE EXTENSION has
been creating the entry depending on the role, I would tend to think
that the simplest solution is that for each SHARED_DEPENDENCY_ACL we
should call a new routine, say RemoveRoleFromInitPriv(), which would
check for the presence of the object whose dependency is removed in
pg_init_privs and then remove from the ACL item list any trace of the
role whose ownerships are dropped.  The removal would require a logic
similar to what is done in RemoveRoleFromObjectPolicy(), where the
previous ACL is rebuilt but without the role removed.

It may be cleaner to invent a new type of dependency for pg_shdepend,
say SHARED_DEPENDENCY_INIT_PRIVS which would remove the dependency to
the object in pg_init_privs but that would not be backpatchable :(

Tom, any thoughts perhaps?
--
Michael

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

Re: incorrect pg_dump output due to not handling dropped roles correctly

Floris Van Nee

> The dependencies related to the ACL entries exist in pg_shdepend
> between the role and the revoked objects, and these get removed when
> issuing DROP OWNED BY.  So it seems to me that the cleanup needs to
> happen when issuing the DROP OWNED BY query, and not DROP ROLE.
> Looking at the code, it seems to me that we should just patch
> shdepDropOwned() to handle properly the removal of the role in ACL
> objects in pg_init_privs for all the objects we are removing a
> dependency on.  I am just diving into a patch..

Forgive me for not following the logic here completely, as I haven't done a deep dive into the code.
I agree doing it in the DROP OWNED BY makes more sense, however I was suggesting to do it during 'DROP ROLE', because it is at least not enough to do it *only* in the DROP OWNED BY. For example, we can also manually remove the permissions and then drop the role, without using DROP OWNED BY.
So, if we do it during DROP OWNED BY, we should also handle it during one of the below REVOKE commands. Perhaps DROP OWNED BY already calls one of these functions internally - in that case you can ignore my comment. Just wanted to make sure we catch all possible cases this can occur.

-- before this, create role role, assign default privs and then create extension, then:

postgres=# select * from pg_catalog.pg_init_privs
where objoid=(select 'pg_stat_statements'::regclass);
 objoid | classoid | objsubid | privtype |                                   initprivs                                  
--------+----------+----------+----------+-------------------------------------------------------------------------------
  24583 |     1259 |        0 | e        | {florisvannee=arwdDxt/florisvannee,test=arwdDxt/florisvannee,=r/florisvannee}
(1 row)

postgres=# alter default privileges in schema public revoke all privileges on tables from test;
ALTER DEFAULT PRIVILEGES

postgres=# revoke all on pg_stat_statements from test;
REVOKE

postgres=# drop role test;
DROP ROLE
postgres=# select * from pg_catalog.pg_init_privs
where objoid=(select 'pg_stat_statements'::regclass);
 objoid | classoid | objsubid | privtype |                                   initprivs                                    
--------+----------+----------+----------+--------------------------------------------------------------------------------
  24583 |     1259 |        0 | e        | {florisvannee=arwdDxt/florisvannee,24578=arwdDxt/florisvannee,=r/florisvannee}
(1 row)


-Floris


Reply | Threaded
Open this post in threaded view
|

Re: incorrect pg_dump output due to not handling dropped roles correctly

Michael Paquier-2
On Mon, Nov 18, 2019 at 09:16:06AM +0000, Floris Van Nee wrote:
> So, if we do it during DROP OWNED BY, we should also handle it
> during one of the below REVOKE commands. Perhaps DROP OWNED BY
> already calls one of these functions internally - in that case you
> can ignore my comment. Just wanted to make sure we catch all
> possible cases this can occur.

One problem here is that you would need to scan and track down all the
entries in pg_init_privs if you'd like to make sure that all the
traces of the role have been removed, so that's really not good for
performance.  The fix I was actually imagining upthread would be to
add a new type of entry in pg_shdepend linking the role with
pg_init_privs for the object where an ACL is added.  This would
prevent DROP ROLE to complete with your scenarios, and if the role
ownerships are dropped we could use the link between the object and
the role to remove directly the ACL for the role in the object.
That's not really backpatchable unfortunately.  On top of the new
pg_depend type we would need a new cleanup routine similar to what
happens for policy ACLs when dropping a role.
--
Michael

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

Re: incorrect pg_dump output due to not handling dropped roles correctly

Stephen Frost
In reply to this post by Floris Van Nee
Greetings,

* Floris Van Nee ([hidden email]) wrote:
> I noticed I wasn't able to apply my usual pg_dump schema output without errors anymore after I dropped some roles. After some digging, I found it has to do with Postgres not correctly updating the pg_init_privs table upon dropping roles. I can reproduce a similar scenario with the following steps (output from v13devel, but AFAIK all versions affected, I ran into the issue on v11.2):

Ok, this is ... interesting.

> postgres=# create role test;
> CREATE ROLE
> postgres=# alter default privileges in schema public grant all privileges on tables to test;
> ALTER DEFAULT PRIVILEGES
> postgres=# create extension pg_stat_statements;
> CREATE EXTENSION

So- in this case, the 'test' role is being granted these privileges
because it was given default privs in the public schema for objects that
are created by the superuser, even though the 'test' role never shows up
in the actual pg_stat_statements sql script.

I'm on the fence about if all of the objects which are created by an
extension should actually be subject to default privileges or not, but I
definitely don't think that the pg_init_privs system should be treating
those privileges that come from default privileges, instead of from the
extension's sql script, as being part of the 'initial privileges' for
the extension.

In short, I don't think any of the downthread discussion about how to
fix this is at all correct- the problem, as I view it, is that these
entries are getting into pg_init_privs in the first place and they
really shouldn't be because these privileges aren't coming from the
*extension* which is what pg_init_privs is trying to track.

Another way to view this is that I think the way we should be thinking
about the order of operations here is:

create role test;
alter default privs;
create extension;
  -- extension .sql runs WITHOUT any default privs being applied
  -- ACLs are recorded into pg_init_privs from the .sql script
  -- default privs are applied to the objects from the extension

Maybe we implement it that way, maybe we don't, but the above is my
feeling as to what the perception should be.

This would also mean that pg_dump would automatically figure out that
these privileges have been added AFTER the extension was created (and
aren't part of the extension's .sql script) and therefore there should
be some independent GRANT commands to add those privileges back included
in the pg_dump file.

Maybe something else to point out is that if we keep these entries in
pg_init_privs the way the downthread discussion seems to be assuming,
then pg_dump would *not* include the GRANT commands to add them back and
therefore you'd have to imagine re-ordering things in pg_dump so that
the default privileges are installed before the extension gets created,
and I'm hoping everyone here agress that'd be pretty crazy to try and
do.

Thanks,

Stephen

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

Re: incorrect pg_dump output due to not handling dropped roles correctly

Stephen Frost
In reply to this post by Michael Paquier-2
Greetings Michael,

* Michael Paquier ([hidden email]) wrote:

> On Mon, Nov 18, 2019 at 09:16:06AM +0000, Floris Van Nee wrote:
> > So, if we do it during DROP OWNED BY, we should also handle it
> > during one of the below REVOKE commands. Perhaps DROP OWNED BY
> > already calls one of these functions internally - in that case you
> > can ignore my comment. Just wanted to make sure we catch all
> > possible cases this can occur.
>
> One problem here is that you would need to scan and track down all the
> entries in pg_init_privs if you'd like to make sure that all the
> traces of the role have been removed, so that's really not good for
> performance.  The fix I was actually imagining upthread would be to
> add a new type of entry in pg_shdepend linking the role with
> pg_init_privs for the object where an ACL is added.  This would
> prevent DROP ROLE to complete with your scenarios, and if the role
> ownerships are dropped we could use the link between the object and
> the role to remove directly the ACL for the role in the object.
> That's not really backpatchable unfortunately.  On top of the new
> pg_depend type we would need a new cleanup routine similar to what
> happens for policy ACLs when dropping a role.
Please review the message I just posted on this thread in response to
the original complaint.  I really don't think we want these entries to
end up in pg_init_privs in the first place and all of this around the
dependency handling is, at best, an independent thing and not actually
the root cause of this problem that we should be addressing.

I'm not sure that there's a case where this dependency handling would
actually be needed that's being described here, assuming we address the
root issue that these entries are being added in the first place- maybe
there is if an extension's .sql script has a built-in assumption that
some role already exists and then a user goes and drops that role, but
it seems like the right answer there might be to require the user to
drop the extension itself and not the role and we should track the
dependency that way.

Thanks,

Stephen

signature.asc (836 bytes) Download Attachment