Обсуждение: incorrect pg_dump output due to not handling dropped roles correctly

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

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?

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

Floris Van Nee <florisvannee@Optiver.com> writes:
> I noticed I wasn't able to apply my usual pg_dump schema output without errors anymore after I dropped some roles.
Aftersome 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

> 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

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

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

> 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
itis at least not enough to do it *only* in the DROP OWNED BY. For example, we can also manually remove the permissions
andthen 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
OWNEDBY already calls one of these functions internally - in that case you can ignore my comment. Just wanted to make
surewe 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

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

Greetings,

* Floris Van Nee (florisvannee@Optiver.com) wrote:
> I noticed I wasn't able to apply my usual pg_dump schema output without errors anymore after I dropped some roles.
Aftersome 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

Greetings Michael,

* Michael Paquier (michael@paquier.xyz) 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

Stephen, are you intending to get this bug fixed?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Stephen Frost <sfrost@snowman.net> writes:
> In particular- should we really be setting default privileges for objects
> that are being created by an extension, at all?  The more I think about it,
> the less I like it, particularly when we start to think about the trusted
> extension work that Tom’s doing.

> In short, I’m leaning towards the argument that this was a missing check in
> the default ACL code to explicitly *not* add default privileges on objects
> that are being created by an extension.  With such a check, the entire
> wouldn’t end up in pg_init_privs and the issue that started this thread
> wouldn’t happen.

Hmm.  I think we'd still wish pg_upgrade to preserve whatever privileges
have been granted on extension objects, but that doesn't necessarily
have to happen through pg_init_privs.  I've forgotten the details ---
would the change you suggest be enough to make upgrade work correctly,
or would we need additional hacking?

            regards, tom lane

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> On Wed, Jan 29, 2020 at 14:46 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> > > In particular- should we really be setting default privileges for objects
> > > that are being created by an extension, at all?  The more I think about
> > it,
> > > the less I like it, particularly when we start to think about the trusted
> > > extension work that Tom’s doing.
> >
> > > In short, I’m leaning towards the argument that this was a missing check
> > in
> > > the default ACL code to explicitly *not* add default privileges on
> > objects
> > > that are being created by an extension.  With such a check, the entire
> > > wouldn’t end up in pg_init_privs and the issue that started this thread
> > > wouldn’t happen.
> >
> > Hmm.  I think we'd still wish pg_upgrade to preserve whatever privileges
> > have been granted on extension objects, but that doesn't necessarily
> > have to happen through pg_init_privs.  I've forgotten the details ---
> > would the change you suggest be enough to make upgrade work correctly,
> > or would we need additional hacking?
>
> Err, privileges on upgrade *are* preserved, even with this. Guess I wasn’t
> clear.
>
> Right now:
>
> If someone sets up default privileges for a user, and that user then
> creates an extension, the default privs are applied to all of the
> individual objects that are created by the extension, and then get recorded
> in pg_init_privs, as if it was the extension setting up those privileges.
>
> Having those privileges end up in pg_init_privs is pretty clearly wrong
> because only those privileges that are installed by the extension should
> end up in init privs, because all OTHER privileges are what will be dumped
> out.
>
> However, I’m not sure that those objects, which are inside the extension,
> should really be having default privs applied to them in the first place!
> If I have default privs set up to grant EXECUTE to some user on functions,
> and then I install pg adminpack, is it really right for all the functions
> in pg adminpack to now have execute rights granted to whomever this user
> is- even when the extension revoked the grant to public because the
> function is unsafe for general users to have access to..?
>
> I feel like default privs should apply when the user is EXPLICITLY creating
> a specific object, not when it’s being created as part of an extension, and
> therefore we should have a check in the creation system, where the default
> privs are handled, to basically make the default privs code be a no-op when
> the object is being created by an extension. Users will have to go grant
> access to those objects in the extension explicitly instead, but that seems
> perfectly reasonable to me.
>
> Currently traveling and so not actually looking at the code, but that’s my
> recollection of things.

Back from said travel now and reviewed the thread and discussion and
this looks like exactly what's happening- so, does someone want to
comment on this, or..?

Specifically, if one does:

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

Should the pg_stat_statements view, that's actually created by the
extension as part of its install, end up with the default privileges of
"grant all privileges on tables to test"?

At this point, my vote is 'no, that is a bug that we should fix.', but
it's also clearly a behavior change and maybe others have a different
opinion..?  If not, it should at least be a relatively easy fix..

Thanks,

Stephen