Re: Extension security improvement: Add support for extensions with an owned schema - Mailing list pgsql-hackers

From Tomas Vondra
Subject Re: Extension security improvement: Add support for extensions with an owned schema
Date
Msg-id 585e996c-a5c6-4e61-acc4-d92b7a1458ea@vondra.me
Whole thread Raw
In response to Re: Extension security improvement: Add support for extensions with an owned schema  (Jelte Fennema-Nio <me@jeltef.nl>)
List pgsql-hackers
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




pgsql-hackers by date:

Previous
From: Andrew Dunstan
Date:
Subject: Re: SQL:2023 JSON simplified accessor support
Next
From: Robert Haas
Date:
Subject: Re: pg_verifybackup: TAR format backup verification