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

feat: remove dependency on pgsodium #37

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

soedirgo
Copy link
Member

@soedirgo soedirgo commented Oct 31, 2024

Add a new version 0.3.0 that doesn't depend on pgsodium. All the needed pgsodium functionalities are baked directly into supabase_vault.

Tested flows:

  1. pause & restore w/o vault
    • create a new project
    • drop extension supabase_vault cascade;
    • pause project
    • restore project
    • select * from vault.decrypted_secrets
  2. pause & restore w/ existing secrets
    • create a new project
    • create a new secret in Settings > Vault
    • pause project
    • restore project
    • select * from vault.decrypted_secrets
  3. pause & restore w/ existing secrets w/o pgsodium
    • follow steps in 2
    • drop extension pgsodium
    • pause project
    • restore project
    • select * from vault.decrypted_secrets
  4. pg_upgrade
    • create a new project on an earlier AMI version (e.g. 15.6.1.139)
    • create a new secret in Settings > Vault
    • upgrade project in Settings > Infrastructure
    • select * from vault.decrypted_secrets

@soedirgo soedirgo marked this pull request as ready for review October 31, 2024 12:43
@soedirgo soedirgo force-pushed the feat/vault-sans-pgsodium branch 3 times, most recently from ac58861 to f525592 Compare November 14, 2024 12:41
Copy link

Copy link
Member Author

Choose a reason for hiding this comment

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

Creating a separate version here as an alternative to 0.2.8 -> 0.3.0. This lets the extension be created without first creating pgsodium, which would be necessary when creating 0.2.8.

@@ -0,0 +1 @@
requires = pgsodium
Copy link
Member Author

Choose a reason for hiding this comment

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

Only 0.2.8 (the current latest version) requires pgsodium - future versions won't require it.

shell.nix Outdated Show resolved Hide resolved
@soedirgo soedirgo force-pushed the feat/vault-sans-pgsodium branch 2 times, most recently from a8db719 to 72cde61 Compare November 15, 2024 07:51
@soedirgo soedirgo requested a review from imor November 15, 2024 10:18
@@ -188,3 +190,52 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

# pgsodium License
Copy link

Choose a reason for hiding this comment

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

is this necessary if the dep is getting pulled out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we use pgsodium code in pgsodium.c and pgsodium.h; we can do away with them once we move from libsodium (still needed to decrypt existing secrets)

shell.nix Outdated Show resolved Hide resolved
sql/supabase_vault--0.2.8.sql Outdated Show resolved Hide resolved
Comment on lines 53 to 55
GRANT ALL ON SCHEMA vault TO pgsodium_keyiduser;
GRANT ALL ON TABLE vault.secrets TO pgsodium_keyiduser;
GRANT ALL ON vault.decrypted_secrets TO pgsodium_keyiduser;
Copy link

Choose a reason for hiding this comment

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

these roles come from pg_sodium. for new installs on 0.3.0 is pg_sodium still required?

there is no exception in tests currently due to fixtures.sql creating these roles but wouldn't that need to be directly in this setup script for prod?

Copy link
Member Author

Choose a reason for hiding this comment

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

The roles will stay when pgsodium is dropped (Postgres doesn't track extension dependents for roles).

I'll see if I can change how the permissions is set up so we can further remove pgsodium-isms, but at worst we just create these roles at AMI creation once we stop creating pgsodium by default.

Copy link

Choose a reason for hiding this comment

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

@soedirgo if it would help once I have a pg 17 in https://github.com/supabase/postgres I could set up a flake here that would let you isolate and use it, so that you could end up with 1:1 sync with what is built and used downstream.

Copy link

Choose a reason for hiding this comment

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

This would help you match exact build + avoid having to maintain your own build of postgres in this project too.

Copy link

Choose a reason for hiding this comment

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

...it could also then use the cached versions we have, and would save time on local machine and CI too.

Copy link
Member Author

@soedirgo soedirgo Nov 15, 2024

Choose a reason for hiding this comment

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

Yeah that'd be fantastic 👍 it'd halve the size of this PR

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

Successfully merging this pull request may close these issues.

4 participants