michelp / pgsodium

Modern cryptography for PostgreSQL using libsodium.
Other
546 stars 32 forks source link

Make extension relocatable during installation #44

Closed ioguix closed 1 year ago

ioguix commented 1 year ago

Hi,

While studying pgsodium, I found that the extension schema location is using sometime the relocatable form @extschema@ and sometime the fixed one pgsodium. Sometime, both forms appear in the same function.

If the extension is truly non relocatable, using @extschema@ is a useless pain. If it can be relocatable, I suppose we just have to fix it in the installation script and adjust tests to make sure it works as expected.

I took the liberty to implement the relocatable form, just to start the discussion. If you want to run your tests with a non-default schema, just run: psql -f test/test.sql -v extschema=anotherschema.

If the extension must not be relocatable, I can handle the cleanup as well.

michelp commented 1 year ago

Hello!

Yes the code has a bit of mess in it, pgsodium started life relocatable, then I decided to use a static name. My concern with relocation is set_path style attacks where users can interpose malicious functions in front, and also that pgsodium has a key table that should be referenced possibly from other extensions, and third, if the schema can be located, then when CREATE EXTENSION something_that_uses_pgsodium WITH foo_schema CASCADE happens, pgsodium ends up in foo_schema and nothing else in the system can know that a-priory. All that stuff makes my head hurt so I like the fully qualified names everywhere, and set_path = '' on all my functions.

I could be talked out of it of course if there's something I'm missing about the above issues.

ioguix commented 1 year ago

Hi,

My concern with relocation is set_path style attacks where users can interpose malicious functions in front

This is true for a lot of extension anyway. This is the responsibility of the dev/admin to use fully qualified name for security reason.

pgsodium has a key table that should be referenced possibly from other extensions

The key table is accessible through funcs as well. And other extension can find pgsodium schema/funcs/tables/views by querying the catalog anyway. I admit it might not be straight forward, but if the user need to relocate elsewhere, I suppose he's ready to handle this as well.

pgsodium ends up in foo_schema and nothing else in the system can know that a-priory

I'm not sure to understand this one.

I like the fully qualified names everywhere, and set_path = '' on all my functions.

This is a good habit anyway, and as far as I understand your point, relocatable doesn't forbid this.

But anyway, the PR conflicts now. I don't have strong feelings/needs about the extension being relocatable or not, feel free to close the PR. My main objective was to clean this surprising half-backed situation while studying the code. The work is not really hard to achieve if pgsodium need to be relocatable in the future anyway...

michelp commented 1 year ago

Hi,

My concern with relocation is set_path style attacks where users can interpose malicious functions in front

This is true for a lot of extension anyway. This is the responsibility of the dev/admin to use fully qualified name for security reason.

While true for a lot of extensions, I still think it's a trap to avoid, especially for a security related extension that other extensions will want to depend on (see last response below).

pgsodium has a key table that should be referenced possibly from other extensions

The key table is accessible through funcs as well. And other extension can find pgsodium schema/funcs/tables/views by querying the catalog anyway. I admit it might not be straight forward, but if the user need to relocate elsewhere, I suppose he's ready to handle this as well.

Yes, extension schemas can be found in the catalog, but now the average extension programmer is meta programming, which I think is a pretty high bar which no current extensions bother doin, but you're right it is possible.

pgsodium ends up in foo_schema and nothing else in the system can know that a-priory

I'm not sure to understand this one.

Let's presume pgsodium is relocatable and that extension A depends on pgsodium (with a requires statement in its control file). A user wants to install extension A in schema A, they do so with CREATE EXTENSION A WITH SCHEMA A CASCADE then pgsodium will get installed into schema A. Now we have extension B that also requires pgsodium. Creating the extension with CASCADE will fail as pgsodium is already installed, not cascading works but now B can't know where pgsodium is (without, as you pointed out, meta-programming the catalog).

This is why everyone dumps extensions into public or some other catch-all schema, there's no good way for relocatable extensions to interoperate their shared names without meta-programming. What is an entirely boring and standard feature of say Python and Javascript ("import pgsodium as foo") does not exist in postgres. I consider it an anachronism, and the only way to avoid it is to declare your schema name up front and stick with it. If I could have anything in the postgres world, it would be a true modular namespace of importable objects, but here we are.

My main objective was to clean this surprising half-backed situation while studying the code. The work is not really hard to achieve if pgsodium need to be relocatable in the future anyway...

I'll go through and clean up all the old extschema invocations laying around, they don't cause any problems (they become 'pgsodium' anyway) but I agree it can be off-putting.

I'll leave this PR open for a bit, maybe someone else wants to weigh in.

ioguix commented 1 year ago

Thanks for the explanation around create extension cascade.

I'll go through and clean up all the old extschema invocations laying around

I can handle this and forge a PR if you prefer. I didn't start this discussion to give you more work ;)