pg_dump dump catalog ACLs

Stephen Frost <sfrost@snowman.net> writes:

Per discussion about the best approach to reduce the amount of
superuser-only capabilities, this patch modifies pg_dump to dump out
all ACLs which exist on objects in the pg_catalog schema.

Um ... surely there are some of those that are installed by default?
To make this work, you'd need a way to distinguish privileges installed
by initdb from those changed later.

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 Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

Per discussion about the best approach to reduce the amount of
superuser-only capabilities, this patch modifies pg_dump to dump out
all ACLs which exist on objects in the pg_catalog schema.

Um ... surely there are some of those that are installed by default?

There are a few, but not terribly many currently.

To make this work, you'd need a way to distinguish privileges installed
by initdb from those changed later.

To replicate whatever the current ACL is, we don't actually need to
make such a differentiation. I'm not against doing so, but the only
point of it would be to eliminate a few extra lines being dumped out
which re-run those commands that initdb runs on restore.

The downside of doing so would be having to keep track of the exact ACLs
set for every object in pg_catalog which has a non-NULL ACL at initdb
time for every version of PG that the latest version of pg_dump
supports, and making sure that any changes to those get updated in
pg_dump in addition to the relevant system_views.sql change.

That's possible, but I wasn't sure it was worth it.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

To make this work, you'd need a way to distinguish privileges installed
by initdb from those changed later.

To replicate whatever the current ACL is, we don't actually need to
make such a differentiation. I'm not against doing so, but the only
point of it would be to eliminate a few extra lines being dumped out
which re-run those commands that initdb runs on restore.

No, the point of it would be to not have pg_dump scripts overriding
installed-by-default ACLs. A newer PG version might have different
ideas about what those should be. I don't think this is exactly an
academic concern, either: wouldn't a likely outcome of your default-roles
work be that some built-in functions have different initial ACLs than
they do today? Good luck with that, if pg_upgrade overwrites those
ACLs with the previous-version values.

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 Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

To make this work, you'd need a way to distinguish privileges installed
by initdb from those changed later.

To replicate whatever the current ACL is, we don't actually need to
make such a differentiation. I'm not against doing so, but the only
point of it would be to eliminate a few extra lines being dumped out
which re-run those commands that initdb runs on restore.

No, the point of it would be to not have pg_dump scripts overriding
installed-by-default ACLs. A newer PG version might have different
ideas about what those should be. I don't think this is exactly an
academic concern, either: wouldn't a likely outcome of your default-roles
work be that some built-in functions have different initial ACLs than
they do today? Good luck with that, if pg_upgrade overwrites those
ACLs with the previous-version values.

As it turns out, there isn't such an issue as the default for functions
is to allow PUBLIC to EXECUTE and therefore we don't dump out ACLs for
most functions. The follow-on change to this patch is to modify those
functions to *not* have the default/NULL ACL (and also drop the explicit
if (!superuser()) ereport() checks in those functions), which will work
just fine and won't be overwritten during pg_upgrade because those
functions currently just have the default ACL, which we don't dump out.

Of course, it's a different story if the user changes the ACL on objects
in pg_catalog and then we change what we think the default ACL should
be, but in such a case, I'm guessing we should probably go with what the
user explicitly asked for anyway and if there's a serious enough change
in the permissions of the function then perhaps we should have a
different function instead of re-defining the existing one.

We do have some fun issues with pg_upgrade by going with this approach
of having pg_dump dump out ACLs- what happens when there's a function or
column which goes away? If there's a non-NULL ACL on them, the restore
will just outright fail, if we don't do something more. I'm not a huge
fan of coding into pg_dump the knowledge of every object which exists
for every version of PG we support for pg_dump though.

Regarding the default roles patch, now that it's down to only one
default role, based on the assumption that this approach with pg_dump
will solve all the other concerns, there isn't really much overlap
between the default roles and the function ACLs. Those functions which
will be able to work with just ACLs won't have a default role and the
functions which we need a default role for will have the default NULL
ACL.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

No, the point of it would be to not have pg_dump scripts overriding
installed-by-default ACLs. A newer PG version might have different
ideas about what those should be. I don't think this is exactly an
academic concern, either: wouldn't a likely outcome of your default-roles
work be that some built-in functions have different initial ACLs than
they do today? Good luck with that, if pg_upgrade overwrites those
ACLs with the previous-version values.

As it turns out, there isn't such an issue as the default for functions
is to allow PUBLIC to EXECUTE and therefore we don't dump out ACLs for
most functions. The follow-on change to this patch is to modify those
functions to *not* have the default/NULL ACL (and also drop the explicit
if (!superuser()) ereport() checks in those functions), which will work
just fine and won't be overwritten during pg_upgrade because those
functions currently just have the default ACL, which we don't dump out.

Yes, so it would probably manage to not fail during 9.6 -> 9.7 migration.
But you *won't ever again* get to change the default ACLs on those
functions. That does not seem like a great bet from 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 02/29/2016 08:52 PM, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

As it turns out, there isn't such an issue as the default for functions
is to allow PUBLIC to EXECUTE and therefore we don't dump out ACLs for
most functions. The follow-on change to this patch is to modify those
functions to *not* have the default/NULL ACL (and also drop the explicit
if (!superuser()) ereport() checks in those functions), which will work
just fine and won't be overwritten during pg_upgrade because those
functions currently just have the default ACL, which we don't dump out.

Yes, so it would probably manage to not fail during 9.6 -> 9.7 migration.
But you *won't ever again* get to change the default ACLs on those
functions. That does not seem like a great bet from here.

Would it be a terrible idea to add some attribute to ACLs which can be
used to indicate they should not be dumped (and supporting syntax)?

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway <mail@joeconway.com> writes:

Would it be a terrible idea to add some attribute to ACLs which can be
used to indicate they should not be dumped (and supporting syntax)?

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

Actually, wouldn't you need to mark individual aclitems as built-in
or not? Consider a situation where we have some function foo() that
by default has EXECUTE permission granted to some built-in "pg_admin"
role. If a given installation then also grants EXECUTE to "joe",
what you really want to have happen is for pg_dump to dump only the
grant to "joe". Mentioning pg_admin's grant would tie the dump to
a particular major PG version's idea of what the built-in roles are,
which is what I'm arguing we need to avoid.

I guess this could also be addressed by having two separate aclitem[]
columns, one that is expected to be frozen after initdb and one for
user-added grants.

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 03/01/2016 08:00 AM, Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

Would it be a terrible idea to add some attribute to ACLs which can be
used to indicate they should not be dumped (and supporting syntax)?

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

I was thinking the supporting syntax might be used by extensions, for
example.

Actually, wouldn't you need to mark individual aclitems as built-in
or not? Consider a situation where we have some function foo() that
by default has EXECUTE permission granted to some built-in "pg_admin"
role. If a given installation then also grants EXECUTE to "joe",
what you really want to have happen is for pg_dump to dump only the
grant to "joe". Mentioning pg_admin's grant would tie the dump to
a particular major PG version's idea of what the built-in roles are,
which is what I'm arguing we need to avoid.

Yes, I guess it would need to be a per aclitem attribute.

I guess this could also be addressed by having two separate aclitem[]
columns, one that is expected to be frozen after initdb and one for
user-added grants.

Yeah, that would work, but seems kind of ugly.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

* Joe Conway (mail@joeconway.com) wrote:

On 03/01/2016 08:00 AM, Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

Would it be a terrible idea to add some attribute to ACLs which can be
used to indicate they should not be dumped (and supporting syntax)?

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

I was thinking the supporting syntax might be used by extensions, for
example.

I tend to agree with Tom that we don't really need SQL syntax for this.

Actually, wouldn't you need to mark individual aclitems as built-in
or not? Consider a situation where we have some function foo() that
by default has EXECUTE permission granted to some built-in "pg_admin"
role. If a given installation then also grants EXECUTE to "joe",
what you really want to have happen is for pg_dump to dump only the
grant to "joe". Mentioning pg_admin's grant would tie the dump to
a particular major PG version's idea of what the built-in roles are,
which is what I'm arguing we need to avoid.

Yes, I guess it would need to be a per aclitem attribute.

Agreed.

I guess this could also be addressed by having two separate aclitem[]
columns, one that is expected to be frozen after initdb and one for
user-added grants.

Yeah, that would work, but seems kind of ugly.

Rather than have two aclitem[] columns in every catalog, since this
information is only used by pg_dump and not during normal operation, we
could use the approach that pg_description took and have an independent
catalog table which just contains all non-NULL "system" ACLs. We could
populate it at the bottom of system_views.sql, so that we don't have to
explicitly think about updating that table whenever there's a change to
what the default ACLs are.

I don't see any reason it couldn't be used by extensions also, though
we'd have to do a bit more work on pg_dump to make it actually dump
out any non-default ACLs for extension-owned objects.

Thoughts?

Thanks!

Stephen

On Wed, Mar 2, 2016 at 1:54 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Joe Conway (mail@joeconway.com) wrote:

On 03/01/2016 08:00 AM, Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

Would it be a terrible idea to add some attribute to ACLs which can be
used to indicate they should not be dumped (and supporting syntax)?

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

I was thinking the supporting syntax might be used by extensions, for
example.

I tend to agree with Tom that we don't really need SQL syntax for this.

Actually, wouldn't you need to mark individual aclitems as built-in
or not? Consider a situation where we have some function foo() that
by default has EXECUTE permission granted to some built-in "pg_admin"
role. If a given installation then also grants EXECUTE to "joe",
what you really want to have happen is for pg_dump to dump only the
grant to "joe". Mentioning pg_admin's grant would tie the dump to
a particular major PG version's idea of what the built-in roles are,
which is what I'm arguing we need to avoid.

Yes, I guess it would need to be a per aclitem attribute.

Agreed.

I guess this could also be addressed by having two separate aclitem[]
columns, one that is expected to be frozen after initdb and one for
user-added grants.

Yeah, that would work, but seems kind of ugly.

Rather than have two aclitem[] columns in every catalog, since this
information is only used by pg_dump and not during normal operation, we
could use the approach that pg_description took and have an independent
catalog table which just contains all non-NULL "system" ACLs. We could
populate it at the bottom of system_views.sql, so that we don't have to
explicitly think about updating that table whenever there's a change to
what the default ACLs are.

I don't see any reason it couldn't be used by extensions also, though
we'd have to do a bit more work on pg_dump to make it actually dump
out any non-default ACLs for extension-owned objects.

​It sounds like this train of thought would resolve this complaint?

​
/messages/by-id/CADmxfmmz-ATwptaidTSAF0XE=cPeikMyc00sj6t9xF6KCV5jCQ@mail.gmail.com

​Namely allowing users to edit permissions on extension objects and have
those changes dumped and then restored after the dependent CREATE EXTENSION
command is executed during pg_restore.

Did I interpret that right?

David J.
​

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Wed, Mar 2, 2016 at 1:54 PM, Stephen Frost <sfrost@snowman.net> wrote:

Rather than have two aclitem[] columns in every catalog, since this
information is only used by pg_dump and not during normal operation, we
could use the approach that pg_description took and have an independent
catalog table which just contains all non-NULL "system" ACLs. We could
populate it at the bottom of system_views.sql, so that we don't have to
explicitly think about updating that table whenever there's a change to
what the default ACLs are.

I don't see any reason it couldn't be used by extensions also, though
we'd have to do a bit more work on pg_dump to make it actually dump
out any non-default ACLs for extension-owned objects.

​It sounds like this train of thought would resolve this complaint?

​
/messages/by-id/CADmxfmmz-ATwptaidTSAF0XE=cPeikMyc00sj6t9xF6KCV5jCQ@mail.gmail.com

​Namely allowing users to edit permissions on extension objects and have
those changes dumped and then restored after the dependent CREATE EXTENSION
command is executed during pg_restore.

Did I interpret that right?

Yes, I was following that thread also (as was Joe, I imagine) and that's
the idea.

Thanks!

Stephen

On 03/02/2016 12:54 PM, Stephen Frost wrote:

* Joe Conway (mail@joeconway.com) wrote:

On 03/01/2016 08:00 AM, Tom Lane wrote:

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

I was thinking the supporting syntax might be used by extensions, for
example.

I tend to agree with Tom that we don't really need SQL syntax for this.

I don't see any reason it couldn't be used by extensions also, though
we'd have to do a bit more work on pg_dump to make it actually dump
out any non-default ACLs for extension-owned objects.

Without any syntax, what does the extension do, directly insert into
this special catalog table?

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Wed, Mar 2, 2016 at 2:44 PM, Joe Conway <mail@joeconway.com> wrote:

On 03/02/2016 12:54 PM, Stephen Frost wrote:

* Joe Conway (mail@joeconway.com) wrote:

On 03/01/2016 08:00 AM, Tom Lane wrote:

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

I was thinking the supporting syntax might be used by extensions, for
example.

I tend to agree with Tom that we don't really need SQL syntax for this.

I don't see any reason it couldn't be used by extensions also, though
we'd have to do a bit more work on pg_dump to make it actually dump
out any non-default ACLs for extension-owned objects.

Without any syntax, what does the extension do, directly insert into
this special catalog table?

​The desire in the thread I linked was for the user, not the extension, to
alter the permissions. In that context (and possibly here as well)
PostgreSQL would (somehow?) recognize the ​target as being special and thus
requiring adding or updating an entry into the supplemental catalog table
when the usual GRANT/REVOKE SQL command is issued.

​In effect any object dependent upon an EXTENSION or that already exists in
this special catalog table would need to have the supplemental procedure
executed.

David J.
​

* Joe Conway (mail@joeconway.com) wrote:

On 03/02/2016 12:54 PM, Stephen Frost wrote:

* Joe Conway (mail@joeconway.com) wrote:

On 03/01/2016 08:00 AM, Tom Lane wrote:

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

I was thinking the supporting syntax might be used by extensions, for
example.

I tend to agree with Tom that we don't really need SQL syntax for this.

I don't see any reason it couldn't be used by extensions also, though
we'd have to do a bit more work on pg_dump to make it actually dump
out any non-default ACLs for extension-owned objects.

Without any syntax, what does the extension do, directly insert into
this special catalog table?

Perhaps nothing- we already are tracking everything they've created, at
the end of the extension's script, we could simply go over all of the
objects which are part of the extension and save off the non-NULL ACLs
which exist.

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Wed, Mar 2, 2016 at 2:44 PM, Joe Conway <mail@joeconway.com> wrote:

On 03/02/2016 12:54 PM, Stephen Frost wrote:

* Joe Conway (mail@joeconway.com) wrote:

On 03/01/2016 08:00 AM, Tom Lane wrote:

Yes, we'd need some way to mark non-null ACLs as being "built-in
defaults". I do not see the need to have SQL syntax supporting that
though.

I was thinking the supporting syntax might be used by extensions, for
example.

I tend to agree with Tom that we don't really need SQL syntax for this.

I don't see any reason it couldn't be used by extensions also, though
we'd have to do a bit more work on pg_dump to make it actually dump
out any non-default ACLs for extension-owned objects.

Without any syntax, what does the extension do, directly insert into
this special catalog table?

​The desire in the thread I linked was for the user, not the extension, to
alter the permissions. In that context (and possibly here as well)
PostgreSQL would (somehow?) recognize the ​target as being special and thus
requiring adding or updating an entry into the supplemental catalog table
when the usual GRANT/REVOKE SQL command is issued.

Not quite.

The idea here is for us to track in the catalog what the ACLs were at
the "start" (being "initdb" time for the database as a whole, and
"CREATE EXTENSION" time for the extension) and then dump out any ACLs
which have been changed since then.

That strikes me as much simpler than having to track every GRANT/REVOKE
done against some special set of objects..

Thanks!

Stephen