Re: Extension security improvement: Add support for extensions with an owned schema

On Fri, 27 Sept 2024 at 14:00, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> One thing that's not quite clear to me is what's the correct way for
> existing extensions to switch to an "owned schema". Let's say you have
> an extension. How do you transition to this? Can you just add it to the
> control file and then some magic happens?

Sadly no. As currently implemented this feature can only really be
used by new extensions. There is no upgrade path to it for existing
extensions. This is fairly common btw, the only field in the control
file that ever gets used after the taken from the control file for
ALTER EXTENSION is the version field. e.g. if you change the schema
field in the control file of an already installed extension, the
extension will not be moved to the new schema unless you DROP + CREATE
EXTENSION.

After this message I tried messing around a bit with changing an
existing extension to become an owned schema (either with a new
command or as part of ALTER EXTENSION UPDATE). But it's not as trivial
as I hoped for a few reasons:
1. There are multiple states that extensions are currently in all of
which need somewhat different conversion strategies. Specifically:
relocatable/non-relocatable &
schema=pg_catalog/public/actual_extension_schema.
2. There are two important assumptions on the schema for an
owned_schema, which we would need to check on an existing schema
converting it:
a. The owner of the extension should be the owner of the schema
b. There are no other objects in the schema appart from the extension.

I'll probably continue some efforts to allow for migration, because I
agree it's useful. But I don't think it's critical for this feature to
be committable. Even if this can only be used by new extensions (that
target PG18+ only), I think that would already be very useful. i.e. if
in 5 years time I don't have to worry about these security pitfalls
for any new extensions that I write then I'll be very happy. Also if
an extension really wants to upgrade to an owned schema, then that
should be possible by doing some manual catalog hackery in its
extension update script, because that's effectively what any automatic
conversion would also do.

> A couple minor comments:
> Doesn't "extension is owned_schema" sound a bit weird?

Updated the docs to address all of your feedback I think.

> Also, perhaps "dedicated_schema" would be better than "owned_schema"? I
> mean, the point is not that it's "owned" by the extension, but that
> there's nothing else in it. But that's nitpicking.

I agree that's probably a better name. Given the amount of effort
necessary to update everything I haven't done that yet. I'll think a
bit if there's a name I like even better in the meantime, and I'm
definitely open to other suggestions.

> 3) src/backend/commands/extension.c
>
> I'm not sure why AlterExtensionNamespace moves the privilege check. Why
> should it not check the privilege for owned schema too?

Because for an owned schema we're not creating objects in an existing
schema. We're only renaming the schema that the extension is already
in using RenameSchema, so there's no point in checking if the user can
create objects in that schema, since the objects are already there.
(RenameSchema also checks internally if the current user is the owner
of the schema)

> Shouldn't binary_upgrade_extension_member still set ext=NULL in the for
> loop, the way the original code does that?

I don't think that's necessary. It was necessary before to set extobj
to NULL, because that variable was set every loop. But now extobj is
scoped to the loop, and ext is only set right before the break. So
either we set ext in the loop and break out of the loop right away, or
ext is still set to the NULL value that's was assigned at the top of
the function.

> The long if conditions might need some indentation, I guess. pgindent
> leaves them like this, but 100 columns seems a bit too much. I'd do a
> line break after each condition, I guess.

Done