Skip to content

fix: grant Vault privs to service_role #1539

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

soedirgo
Copy link
Member

service_role used to be able to manage secrets in Vault <=0.2.8 because it had privileges to pgsodium functions; we do the grants for Vault functions instead

@soedirgo soedirgo marked this pull request as ready for review April 11, 2025 07:28
@soedirgo soedirgo requested review from a team as code owners April 11, 2025 07:28
Copy link
Member

@steve-chavez steve-chavez left a comment

Choose a reason for hiding this comment

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

We need more tests as explained above.

@samrose
Copy link
Collaborator

samrose commented Apr 11, 2025

If it helps, I had a moment and created some proposed tests that I think cover this PR, as a PR to this one here

The tests are found in #1540

Feel free to reject that if it doesn't do what you need/you are implementing your own

@soedirgo
Copy link
Member Author

@supabase/postgres tests added, please +1. Aiming to release this early next week

Copy link
Member

@steve-chavez steve-chavez left a comment

Choose a reason for hiding this comment

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

Tests need changing as mentioned on #1540 (comment)

@steve-chavez steve-chavez force-pushed the fix/grant-vault-privs-to-service_role branch from 9bdefbc to aacdfb4 Compare April 14, 2025 20:21
@@ -52,8 +58,9 @@ ORDER BY object_name, grantee, privilege_type;
vault | secrets | supabase_admin | TRUNCATE
vault | secrets | supabase_admin | UPDATE
vault | update_secret | postgres | EXECUTE
vault | update_secret | service_role | EXECUTE
Copy link
Member

Choose a reason for hiding this comment

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

Effects of the changes now can be clearly visualized.

Copy link
Member

@steve-chavez steve-chavez left a comment

Choose a reason for hiding this comment

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

PR was rebased on top of #1544, test now look good.

The chore: bump versions commit didn't make the rebase as some other commit already used the same versions on https://github.com/supabase/postgres/blob/develop/ansible/vars.yml#L11-L14.

@samrose
Copy link
Collaborator

samrose commented Apr 14, 2025

PR was rebased on top of #1544, test now look good.

The chore: bump versions commit didn't make the rebase as some other commit already used the same versions on https://github.com/supabase/postgres/blob/develop/ansible/vars.yml#L11-L14.

I think in this case we just need to advance those versions one more than

https://github.com/supabase/postgres/blob/develop/ansible/vars.yml#L11

..unless this PR has not been tested in local infra for relevant scenarios. If not, we should add a suffix to the version and test, then remove that and merge.

@steve-chavez
Copy link
Member

I think in this case we just need to advance those versions one more than

Did that, although not sure if correct.

@samrose
Copy link
Collaborator

samrose commented Apr 14, 2025

I think in this case we just need to advance those versions one more than

Did that, although not sure if correct.

version bump looks correct, but also we need to make sure we have tested these changed before we merge into develop. If you're not able to do so, either myself or @soedirgo can for the short term.

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.

3 participants