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

Hi,

I've spent a bit of time looking at this patch. It seems there's a clear
consensus that having "owned schemas" for extensions would be good for
security. To me it also seems as a convenient way to organize stuff. It
was possible to create extensions in a separate schema before, ofc, but
that's up to the DBA. With this the extension author to enforce that.

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?

A couple minor comments:

1) doc/src/sgml/extend.sgml

An extension is <firstterm>owned_schema</firstterm> if it requires a
new dedicated schema for its objects. Such a requirement can make
security concerns related to <literal>search_path</literal> injection
much easier to reason about. The default is <literal>false</literal>,
i.e., the extension can be installed into an existing schema.

Doesn't "extension is owned_schema" sound a bit weird? I'd probably say
"extension may own a schema" or something like that.

Also, "requires a new dedicated schema" is a bit ambiguous. It's not
clear if it means the schema is expected to exist, or if it creates the
schema itself.

And perhaps it should clarify what "much easier to reason about" means.
That's pretty vague, and as a random extension author I wouldn't know
about the risks to consider. Maybe there's a section about this stuff
that we could reference?

2) doc/src/sgml/ref/create_extension.sgml

relocated. The named schema must already exist if the extension's
control file does not specify <literal>owned_schema</literal>.

Seems a bit unclear, I'd say having "owned_schema = false" in the
control file still qualifies as "specifies owned_schema". So might be
better to say it needs to be set to true?

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.

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?

4) src/bin/pg_dump/pg_dump.c

checkExtensionMembership has typo "owned_schem".

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

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.

regards

--
Tomas Vondra