Обсуждение: [MASSMAIL]RFC: Additional Directory for Extensions

Hackers,

In the Security lessons from liblzma thread[1], walther broached the subject of an extension directory path[1]:

> Also a configurable directoy to look up extensions, possibly even to be
> changed at run-time like [2]. The patch says this:
>
>> This directory is prepended to paths when loading extensions (control and SQL files), and to the '$libdir' directive
whenloading modules that back functions. The location is made configurable to allow build-time testing of extensions
thatdo not have been installed to their proper location yet. 
>
> This seems like a great thing to have. This might also be relevant in
> light of recent discussions in the ecosystem around extension management.


That quotation comes from this Debian patch[2] maintained by Christoph Berg. I’d like to formally propose integrating
thispatch into the core. And not only because it’s overhead for package maintainers like Christoph, but because a
numberof use cases have emerged since we originally discussed something like this back in 2013[3]: 

Docker Immutability
-------------------

Docker images must be immutable. In order for users of a Docker image to install extensions that persist, they must
createa persistent volume, map it to SHAREDIR/extensions, and copy over all the core extensions (or muck with symlink
magic[4]).This makes upgrades trickier, because the core extensions are mixed in with third party extensions.  

By supporting a second directory pretended to the list of directories to search, as the Debian patch does, users of
Dockerimages can keep extensions they install separate from core extensions, in a directory mounted to a persistent
volumewith none of the core extensions. Along with tweaking dynamic_library_path to support additional directories for
sharedobject libraries, which can also be mounted to a separate path, we can have a persistent and clean separation of
immutablecore extensions and extensions installed at runtime. 

Postgres.app
------------

The Postgres.app project also supports installing extensions. However, because they must go into the
SHAREDIR/extensions,once a user installs one the package has been modified and the Apple bundle signature will be
broken.The OS will no longer be able to validate that the app is legit. 

If the core supported an additional extension (and PKGLIBDIR), it would allow an immutable PostgreSQL base package and
stillallow extensions to be installed into directories outside the app bundle, and thus preserve bundle signing on
macOS(and presumably other systems --- is this the nix issue, too?) 

RFC
---

I know there was some objection to changes like this in the past, but the support I’m seeing in the liblzma thread for
makingpkglibdir configurable  me optimistic that this might be the right time to support additional configuration for
theextension directory, as well, starting with the Debian patch, perhaps. 

Thoughts?

I would be happy to submit a clean version of the Debian patch[2].

Best,

David

[1] https://www.postgresql.org/message-id/99c41b46-616e-49d0-9ffd-a29432cec818%40technowledgy.de
[2] https://salsa.debian.org/postgresql/postgresql/-/blob/17/debian/patches/extension_destdir?ref_type=heads
[3] https://www.postgresql.org/message-id/flat/51AE0845.8010600@ocharles.org.uk
[4] https://speakerdeck.com/ongres/postgres-extensions-in-kubernetes?slide=14

On 2024-Apr-02, David E. Wheeler wrote:

> That quotation comes from this Debian patch[2] maintained by Christoph
> Berg. I’d like to formally propose integrating this patch into the
> core. And not only because it’s overhead for package maintainers like
> Christoph, but because a number of use cases have emerged since we
> originally discussed something like this back in 2013[3]:

I support the idea of there being a second location from where to load
shared libraries ... but I don't like the idea of making it
runtime-configurable.  If we want to continue to tighten up what
superuser can do, then one of the things that has to go away is the
ability to load shared libraries from arbitrary locations
(dynamic_library_path).  I think we should instead look at making those
locations hardcoded at compile time.  The packager can then decide where
those things go, and the superuser no longer has the ability to load
arbitrary code from arbitrary locations.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

Alvaro Herrera:
> I support the idea of there being a second location from where to load
> shared libraries ... but I don't like the idea of making it
> runtime-configurable.  If we want to continue to tighten up what
> superuser can do, then one of the things that has to go away is the
> ability to load shared libraries from arbitrary locations
> (dynamic_library_path).  I think we should instead look at making those
> locations hardcoded at compile time.  The packager can then decide where
> those things go, and the superuser no longer has the ability to load
> arbitrary code from arbitrary locations.

The use-case for runtime configuration of this seems to be build-time 
testing of extensions against an already installed server. For this 
purpose it should be enough to be able to set this directory at startup 
- it doesn't need to be changed while the server is actually running. 
Then you could spin up a temporary postgres instance with the extension 
directory pointing a the build directory and test.

Would startup-configurable be better than runtime-configurable regarding 
your concerns?

I can also imagine that it would be very helpful in a container setup to 
be able to set an environment variable with this path instead of having 
to recompile all of postgres to change it.

Best,

Wolfgang

> On 3 Apr 2024, at 09:13, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2024-Apr-02, David E. Wheeler wrote:
>
>> That quotation comes from this Debian patch[2] maintained by Christoph
>> Berg. I’d like to formally propose integrating this patch into the
>> core. And not only because it’s overhead for package maintainers like
>> Christoph, but because a number of use cases have emerged since we
>> originally discussed something like this back in 2013[3]:
>
> I support the idea of there being a second location from where to load
> shared libraries

Agreed, the case made upthread that installing an extension breaks the app
signing seems like a compelling reason to do this.

The implementation of this need to make sure the directory is properly set up
however to avoid similar problems that CVE 2019-10211 showed.

--
Daniel Gustafsson

On Apr 3, 2024, at 3:57 AM, walther@technowledgy.de wrote:

> I can also imagine that it would be very helpful in a container setup to be able to set an environment variable with
thispath instead of having to recompile all of postgres to change it. 

Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whatever
theperson who compiled it thought would make sense. 

Best,

David

On Apr 3, 2024, at 8:54 AM, David E. Wheeler <david@justatheory.com> wrote:

> Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to
whateverthe person who compiled it thought would make sense. 

Or SIGHUP?

D

On Apr 3, 2024, at 8:54 AM, David E. Wheeler <david@justatheory.com> wrote:

> Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to
whateverthe person who compiled it thought would make sense. 

Here’s a revision of the Debian patch that requires a server start.

However, in studying the patch, it appears that the `extension_directory` is searched for *all* shared libraries, not
justthose being loaded for an extension. Am I reading the `expand_dynamic_library_name()` function right? 

If so, this seems like a good way for a bad actor to muck with things, by putting an exploited libpgtypes library into
theextension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploit
theoriginal. 

I’m thinking it would be better to have the dynamic library lookup for extension libraries (and LOAD libraries?)
separate,so that the `extension_directory` would not be used for core libraries. 

This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which
wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single
directoryfor an extension. 

Thoughts?

Best,

David

Re: David E. Wheeler
> > Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to
whateverthe person who compiled it thought would make sense.
 
> 
> Here’s a revision of the Debian patch that requires a server start.

Thanks for bringing this up, I should have submitted this years ago.
(The patch is originally from September 2020.)

I designed the patch to require a superuser to set it, so it doesn't
matter very much by which mechanism it gets updated. There should be
little reason to vary it at run-time, so I'd be fine with requiring a
restart, but otoh, why restrict the superuser from reloading it if
they know what they are doing?

> However, in studying the patch, it appears that the `extension_directory` is searched for *all* shared libraries, not
justthose being loaded for an extension. Am I reading the `expand_dynamic_library_name()` function right?
 
> 
> If so, this seems like a good way for a bad actor to muck with things, by putting an exploited libpgtypes library
intothe extension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t
exploitthe original.
 
> 
> I’m thinking it would be better to have the dynamic library lookup for extension libraries (and LOAD libraries?)
separate,so that the `extension_directory` would not be used for core libraries.
 

I'm not sure the concept of "core libraries" exists. PG happens to
dlopen things at run time, and it doesn't know/care if they were
installed by users or by the original PG server. Also, an exploited
libpgtypes library is not worse than any other untrusted "user"
library, so you really don't want to allow users to provide their own
.so files, no matter by what mechanism.

> This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which
wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single
directoryfor an extension.
 

Nice idea, but that would mean installing .so files into PGSHAREDIR.
Perhaps the whole extension stuff would have to move to PKGLIBDIR
instead.


Fwiw, I wrote this patch to solve the problem of testing extensions at
build-time where the build process does not have write access to
PGSHAREDIR. It solves that problem quite well, almost all PG
extensions have build-time test coverage now (where there was
basically 0 before).

Security is not a concern at this point as everything is running as
the same user, and the test cluster will be wiped right after the
test. I figured marking the setting as "super user" only was enough
security at that point, but I would recommend another audit before
using it together with "trusted" extensions and other things in
production.

Christoph

> Fwiw, I wrote this patch to solve the problem of testing extensions at
> build-time where the build process does not have write access to
> PGSHAREDIR. It solves that problem quite well, almost all PG
> extensions have build-time test coverage now (where there was
> basically 0 before).

Also, it's called extension "destdir" because it behaves like DESTDIR
in Makefiles: It prepends the given path to the path that PG is trying
to open when set. So it doesn't allow arbitrary new locations as of
now, just /home/build/foo-1/debian/foo/usr/share/postgresql/17/extension
in addition to /usr/share/postgresql/17/extension. (That is what the
Debian package build process needs, so that restriction/design choice
made sense.)

> Security is not a concern at this point as everything is running as
> the same user, and the test cluster will be wiped right after the
> test. I figured marking the setting as "super user" only was enough
> security at that point, but I would recommend another audit before
> using it together with "trusted" extensions and other things in
> production.

That's also included in the current GUC description:

   This directory is prepended to paths when loading extensions
   (control and SQL files), and to the '$libdir' directive when
   loading modules that back functions. The location is made
   configurable to allow build-time testing of extensions that do not
   have been installed to their proper location yet.

Perhaps I should have included a more verbose "NOT FOR PRODUCTION"
there.

As for compatibility, the patch has been part of the PG 9.5..17 now
for several years, and I'm very happy with extra test coverage it
provides, especially on the Debian architectures that don't have
"autopkgtest" runners yet.

Christoph

On Apr 3, 2024, at 12:46 PM, Christoph Berg <myon@debian.org> wrote:

> Thanks for bringing this up, I should have submitted this years ago.
> (The patch is originally from September 2020.)

That’s okay, it’s still 2020 in some ways. 😂

> I designed the patch to require a superuser to set it, so it doesn't
> matter very much by which mechanism it gets updated. There should be
> little reason to vary it at run-time, so I'd be fine with requiring a
> restart, but otoh, why restrict the superuser from reloading it if
> they know what they are doing?

I think that’s fair. I’ll keep it requiring a restart now, on the theory it would be easier to loosen it later than
haveto tighten it later. 

> I'm not sure the concept of "core libraries" exists. PG happens to
> dlopen things at run time, and it doesn't know/care if they were
> installed by users or by the original PG server. Also, an exploited
> libpgtypes library is not worse than any other untrusted "user"
> library, so you really don't want to allow users to provide their own
> .so files, no matter by what mechanism.

Yes, I guess my concern is whether it could be used to “shadow” core libraries. Maybe it’s no different, really.

>> This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which
wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single
directoryfor an extension. 
>
> Nice idea, but that would mean installing .so files into PGSHAREDIR.
> Perhaps the whole extension stuff would have to move to PKGLIBDIR
> instead.

Yes, I was just poking around the code, and realized that, when extension functions are created they may or may not not
use`MODULE_PATHNAME`, but in any event, there is nothing different about loading an extension DSO than any other DSO. I
washoping to find a path where it knows it’s opening a DSO for the purpose of an extension, so we could limit the
lookupthere. But that does not (currently) exist. 

Maybe we could add an `$extensiondir` variable to complement `$libdir`?

Or is PGKLIBDIR is the way to go? I’m not familiar with it. It looks like extension JIT files are put there already.

> Fwiw, I wrote this patch to solve the problem of testing extensions at
> build-time where the build process does not have write access to
> PGSHAREDIR. It solves that problem quite well, almost all PG
> extensions have build-time test coverage now (where there was
> basically 0 before).

Yeah, good additional use case.

On Apr 3, 2024, at 1:03 PM, Christoph Berg <myon@debian.org> wrote:

> Also, it's called extension "destdir" because it behaves like DESTDIR
> in Makefiles: It prepends the given path to the path that PG is trying
> to open when set. So it doesn't allow arbitrary new locations as of
> now, just /home/build/foo-1/debian/foo/usr/share/postgresql/17/extension
> in addition to /usr/share/postgresql/17/extension. (That is what the
> Debian package build process needs, so that restriction/design choice
> made sense.

Right, this makes perfect sense, in that you don’t have to copy all the extension files from the destdir to the
SHAREDIRto test them, which I imagine could be a PITA. 

> That's also included in the current GUC description:
>
>   This directory is prepended to paths when loading extensions
>   (control and SQL files), and to the '$libdir' directive when
>   loading modules that back functions. The location is made
>   configurable to allow build-time testing of extensions that do not
>   have been installed to their proper location yet.
>
> Perhaps I should have included a more verbose "NOT FOR PRODUCTION"
> there.

The use cases I described upthread are very much production use cases. Do you think it’s not for production just
becausewe need to really think it through? 

I’ve added some docs based on your GUC description; updated patch attached.

Best,

David

On Apr 4, 2024, at 1:20 PM, David E. Wheeler <david@justatheory.com> wrote:

> I’ve added some docs based on your GUC description; updated patch attached.

Here’s a rebase.

I realize this probably isn’t going to happen for 17, given the freeze, but I would very much welcome feedback and
pointersto address concerns about providing a second directory for extensions and DSOs. Quite a few people have talked
aboutthe need for this in the Extension Mini Summits[1], so I’m sure I could get some collaborators to make
improvementsor look at a different approach. 

Best,

David

[1] https://justatheory.com/2024/02/extension-ecosystem-summit/#extension-ecosystem-mini-summit