Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove hard dependency on creation in the pg_catalog schema #289

Closed

Conversation

hanefi
Copy link
Member

@hanefi hanefi commented Oct 13, 2023

Users were able to create the extension on any schema on versions earlier than 1.5. At version 1.5 we started to force users to create the extension in pg_catalog. This set of events cause hardship when doing a pg_dump&pg_restore.

Fixes: #274

@onderkalaci onderkalaci requested a review from marcoslot October 16, 2023 05:01
@JelteF
Copy link

JelteF commented Oct 16, 2023

As far as I know this change was made because of security reasons. Placing an extension in an already existing schema can cause a lot of trickery with search_path to have extensions execute unintended code with high permissions.

I don't think this added security layer is worth breaking upgrades though.

@@ -2,4 +2,3 @@ comment = 'Job scheduler for PostgreSQL'
default_version = '1.6'
module_pathname = '$libdir/pg_cron'
relocatable = false
schema = pg_catalog
Copy link
Collaborator

@marcoslot marcoslot Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I don't think this is an acceptable change. This is likely to cause privilege escalation issues on platforms that allow regular users to create the extension, but deliberately do not provide superuser access.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if we do a very careful audit:

e.g. the unqualified nextval default in
https://github.com/citusdata/pg_cron/blob/main/pg_cron--1.2--1.3.sql looks dodgy

(It's easy to miss things)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading through the issue #113, I no longer think that we should update our control file.

I opened a separate PR that qualifies the nextval call in the migration script in #291 and I plan to close this one without merging.

@marcoslot
Copy link
Collaborator

I don't think this added security layer is worth breaking upgrades though.

can you clarify the scenarios that break?

it's worth noting that pg_dump & pg_restore has never worked due to #113

@JelteF
Copy link

JelteF commented Oct 16, 2023

can you clarify the scenarios that break?

it's worth noting that pg_dump & pg_restore has never worked due to #113

The pg_dump+pg_restore issue described in #274, but if pg_dump+pg_restore doesn't work without either, then I don't think this should be merged.

@philip-harvey
Copy link

Why was this closed without being merged? It's a breaking and undocumented change. Our Terraform is now breaking and can't create a database in GCP CloudSQL due to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants