Re: Bug in handling default privileges inside extension update scripts

Hi Again Stephen,

On Wed, Apr 28, 2021 at 8:23 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Greetings,
>
> * Mats Kindahl (mats(at)timescale(dot)com) wrote:
> > On Mon, Apr 26, 2021 at 7:29 PM Stephen Frost <sfrost(at)snowman(dot)net>
> wrote:
> > > * Mats Kindahl (mats(at)timescale(dot)com) wrote:
> > > > On Thu, Apr 22, 2021 at 5:15 PM Stephen Frost <sfrost(at)snowman(dot)net>
> > > wrote:
> > > > > * Mats Kindahl (mats(at)timescale(dot)com) wrote:
> > > > > > * To be able to read the configuration tables, "reader" need to
> have
> > > > > > SELECT privileges.
> > > > > >
> > > > > > * Since the new role is added by the user and not by the
> extension,
> > > > > > the grants have to be dumped as well. Otherwise, a restore of
> the
> > > > > > data will have wrong privileges.
> > > > > >
> > > > > > * Since new configuration tables could be added by an update of
> the
> > > > > > extension, it is necessary to make sure that these privileges
> are
> > > > > > added to new tables when updating. Typically, this means
> changing
> > > > > > the default privileges on the schema for the configuration
> files.
> > > > >
> > > > > If the extension is updated, I think it's entirely reasonable to
> expect
> > > > > an admin to have to go in and update the relevant permissions on
> any
> > > new
> > > > > tables that have come into existance and, as I've said elsewhere, I
> > > > > don't think that schema-level default privs should be applied to
> tables
> > > > > created by extensions. Sadly, no one else seems to have an opinion
> > > > > regarding that and so there hasn't been a change in that, yet, but
> > > > > that's the source of the issue imv.
> > > >
> > > > That is a different way to solve it, but I think that is a little
> > > > unintuitive. I am actually proposing to still assign default
> privileges,
> > > > but not add them to initprivs, to make sure that they are treated the
> > > same
> > > > way before and after an update.
> >
> > Before delving into the discussion below, my main issue originally is
> that
> > the default privileges are used to set the initprivs for objects created
> by
> > the extension, causing them to not be dumped after an update of the
> > extension. From the thread that you mentioned above, it seems like you
> > agree that this should indeed not be the case for the reasons I outlined.
> > Did I understand it correctly? If so, I can create a patch for that part
> at
> > least.
>
> If default privileges aren't set for objects created by an extension, as
> I feel should be the case, then there's no concern around them being set
> in initprivs or impacting the dump/restore case.

Agree that if default privileges are not applied to extension objects, the
problem goes away.

> I wouldn't agree with
> a patch that applied default privileges but then excluded them from
> initprivs, as I said before.
>

This I do not follow. If the default privileges are added to initprivs, it
means that any privileges assigned to new objects created when the
extension is updated will not be present in a dump after the update, which
is inconsistent because it behaves differently compared to a fresh install
of the same extension with a newer version.

Scenario #1:

1. Install extension v1 using a schema
2. GRANT privileges on objects in schema
3. ALTER DEFAULT PRIVILEGES for schema
4. Update extension to v2
5. pg_dump

Scenario #2:

1. Install extension v2 using a schema
2. GRANT privileges on objects in schema
3. ALTER DEFAULT PRIVILEGES for schema
4. pg_dump

The output of the two dumps will differ, even if the statements inside the
extensions for creating objects are the same.

Note that this part is only about initprivs, not about whether default
privileges should apply to objects created in extensions (which they
currently are).

> > > Yes, I understood your suggestion, but I did, and still do, disagree
> > > with that approach- how is an admin supposed to correctly guess what
> > > permissions would be appropriate for new tables being added during an
> > > upgrade of an extension?
> >
> > I am not really sure in what situation an admin will not be able to
> decide
> > on the grants for extension objects (beyond what the extension decides
> are
> > required).
>
> Consider this:
>
> An admin reviews v1 of extension ABC which installs some tables and
> functions and decides that all users should be allowed to modify those
> tables and so they set DEFAULT PRIVILEGES on the schema for everyone to
> be able to modify the installed tables.
>

If I understand it correctly, the DBA installs the extension and then
checks what tables are there (it is not clear if the DBA reviews the
extension before installing it or after, but the most natural case is that
the DBA checks the tables after installation). In that case, wouldn't the
DBA use GRANT in that case? After all, DEFAULT PRIVILEGES are for *future*
objects created in the schema, while GRANT is used to assign privileges to
currently existing tables. From
https://www.postgresql.org/docs/13/sql-alterdefaultprivileges.html:

ALTER DEFAULT PRIVILEGES allows you to set the privileges that will be
applied to objects created in the future. (It does not affect privileges
assigned to already-existing objects.)

> The ABC extension is then updated to add a new table in a v2 which
> should *not* be able to be modified by any user, and the extension
> author doesn't GRANT any privileges for that table since they know that
> the *default* for a table is that only the owning role can modify the
> table.

But under the assumption above (that the DBA does not want future tables
installed by the extension to get the privileges)...

> When that extension is updated, however, the DEFAULT PRIVILEGES
> will end up GRANT'ing everyone access to that table anyway. That
> doesn't strike me as at all sane.
>

... the DBA would not assign DEFAULT PRIVILEGES to the schema since that
applies to future objects, but rather just use GRANT on the specific tables
that she wants to add privileges for. In that case, the new table would not
get any new grants during the update (because the default privileges are
not set), which is what the DBA expected.

Am I misunderstanding something?

> The example I gave is because a non-superuser should be able to back up
> the
> > tables and I think that the statements are quite clear in what they mean:
> > assigning grants to tables directly is for the purpose of the currently
> > existing objects, and default privileges are for future objects.
>
> Allowing a non-superuser to back up a database is actually something
> that's been added to v14 in the form of the pg_read_all_data role.
>

That is useful, but the same situation occurs if you want to ensure that
all configuration tables can be written as part of, e.g., a pg_restore. In
that case, these privileges would be dumped before an update, but not
after, which is inconsistent in the same way as when only assigning select
privileges, so unless I am mistaken, having a dedicated role helps with the
backup, but not with the restore.

When it comes to default privileges, there's no shortage of extensions
> which modify the privileges as part of the script that is run- but they
> can only do that sanely when they know what privileges the object will
> have initially. If we have DEFAULT PRIVILEGES going on, there's no way
> for the extension to know what privileges the object has during the
> initial or the upgrade script.
>
> > In the example given, the reason adding additional grants and/or default
> > privileges is because the user needs to read the objects for backup
> > purpose. It is quite clearly expressed using the statements in the
> example.
> > To help me understand what problem you see, do you have an example where
> > this is not as clear and where the water is muddier about what privileges
> > to assign?
>
> Any DEFAULT PRIVILEGES that get assigned behind the back of the
> extension when the object is created means that the extension isn't able
> to properly set the privileges on the objects that the extension has
> created and that can lead to cases like the extension having a private
> table that users are able to modify anyway, which is not a good thing.
>
> > > Not to mention that extensions routinely get
> > > added to existing schemas and I don't think it's at all obvious to
> > > users that tables, functions, etc, added by an extension into a schema
> > > should get the default privileges for that schema (and that could even
> > > lead to security issues, I suspect...),
> >
> > Well... a user that wants to assign specific privileges to extension
> > objects and keep them "under control" is probably going to use a
> dedicated
> > schema for the extension(s) to separate user data from extension data
> (for
> > example, in a hosted environment). Sure, users *can* use the public
> schema
> > for all extensions (I think that is what you meant by "extensions
> routinely
> > gets added to existing schemas"), but that will quickly be unmanageable
> for
> > precisely the reason that it is hard to separate extension data from user
> > data, so not sure if that is a good reason to not allow default
> privileges
> > on extension objects. It feels like optimizing for the rare case.
>
> It's not an optimization, it's a matter of doing what makes sense and is
> not going to be a surprise to extension authors and users, and I don't
> buy that having DEFAULT PRIVILEGES applied to objects created by
> extensions is sensible to either of those groups.
>
> > I am not sure how this could cause a security issue though. My reasoning
> is
> > this: escalations of privileges can happen for objects created by the
> > extension if you have default privileges on the schema, so this would
> mean
> > that some role can get access to new objects created by the extension,
> but
> > this was the purpose of assigning default privileges on the schema in the
> > first place so it cannot be unexpected.
>
> > Do you have some ideas or suspicions for how unexpected escalations could
> > occur?
>
> I've outlined at least one way this could cause problems above. I don't
> agree that having DEFAULT PRIVILEGES assigned to objects created by
> extensions is expected. Indeed, I'm fairly confident that it's entirely
> *unexpected* by extension authors, including for core extensions like
> adminpack, that DEFAULT PRIVILEGES might interfere with the privileges
> that it's attempting to explicitly assign (such as REVOKE'ing EXECUTE on
> sensitive functions from PUBLIC).
>
> At least, when I wrote those REVOKEs into that exact extension, I sure
> wasn't imagining that DEFAULT PRIVILEGES might be assigned to the schema
> where those functions were being created and which would end up
> GRANT'ing access to those very sensitive functions. Maybe you can argue
> that the user installing that extension into such a schema intended it,
> but as an extension author, I surely didn't and that's enough of a point
> of surprise that I'm fairly well convinced that we shouldn't have
> objects being created by extensions have DEFAULT PRIVILEGES applied.
>

Ok. I see what you're aiming for.

> > not to mention that you have to
> > > wonder if the privileges installed by the extension should be applied
> > > *first*, and default privs after, or if the default privileges should
> > > be first and the extension's privileges after.
> >
> > Forgive my ignorance, but I am not sure how that makes a difference.
> Could
> > you elaborate?
>
> The order in which privileges are applied makes a difference in terms of
> what the resulting privileges on the object end up being..
>

Ah, you mean like if there is a default privilege that is GRANT ALL and the
extension does a REVOKE of some privileges? Yes, in that case, the end
result would be different.

> > > As it's currently the
> > > latter, it's rather complicated as the extension has no idea what to
> > > expect the privileges on the object to be and so how can it sensibly
> set
> > > privileges on it..?
> >
> > Again, forgive my ignorance, but isn't the expectation of the extension
> > that it has superuser privileges to the database where it is being
> created?
>
> First, no, extensions can be installed by non-superusers in the first
> place, and second, I don't see how that has much to do with the question
> at hand.
>

My fault, I was thinking about this comment from
https://www.postgresql.org/docs/13/sql-createextension.html (and the
following discussion about trusted extensions):

Loading an extension ordinarily requires the same privileges that would be
required to create its component objects. For many extensions this means
superuser privileges are needed.

... but as you say, it notes that superuser privileges are not required.

Thanks,

Mats Kindahl

>
> Thanks,
>
> Stephen
>