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: add secrets to sessions #3591

Merged
merged 21 commits into from
May 10, 2024
Merged

Conversation

lorenzo-cavazzi
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi commented Apr 17, 2024

Feature branch for the build "Secrets in sessions"

https://www.notion.so/Session-Secrets-Build-Team-c6d62656018841109dfe52d831b97f9f

/deploy renku-ui=lorenzo/shape-up-secrets-in-sessions renku-data-services=main renku-gateway=limit-user-secret renku-notebooks=secrets-sidecar extra-values=secretsStorage.encryptionKey=vMqdVXcSIwjwONvGiNE2vjzRjmi8NJ8Y9lTB8m3l/Xg=

* wip: charts for secrets storage

* add platform init script

* fix init container mount, add helm hook

* fix dockerfile

* comment secret storage for now

* update secrets storage chart

* update deploy action

* fix secret mount

* fix env var name

---------
Co-authored-by: Ralf Grubenmann <[email protected]>
@RenkuBot
Copy link
Collaborator

You can access the deployment of this PR at https://ci-renku-3591.dev.renku.ch

@Panaetius Panaetius changed the base branch from master to release-0.52.x May 8, 2024 13:46
Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

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

A few questions and minor changes.

docs/topic-guides/secrets/secrets.rst Outdated Show resolved Hide resolved
helm-chart/renku/templates/setup-job-platform-init.yaml Outdated Show resolved Hide resolved
helm-chart/renku/templates/setup-job-platform-init.yaml Outdated Show resolved Hide resolved
helm-chart/renku/values.yaml Show resolved Hide resolved
scripts/platform-init/platform-init.py Outdated Show resolved Hide resolved
helm-chart/renku/values.yaml Show resolved Hide resolved
existing_private_key != config.secret_service_private_key
and config.secret_service_private_key is not None
):
# update private key
Copy link
Member

Choose a reason for hiding this comment

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

Can we actually support updating the private key? What happens to all the secrets that are sitting in k8s secrets or in the database that are encrypted with the old key?

Same question for the public key.

Copy link
Member

Choose a reason for hiding this comment

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

Also I think we should support two cases only:

  1. Both private and public key are specified in the config and they are proper and valid
  2. Neither public nor private key are specified in the config, in this case we generate the secret only if the secret does not already exist

Right now we are independently checking for the pivate and public keys and also generating them independently. I think this is too complicated and I dont think anyone would or should be using this option. I.e. provide a public key but let renku generate the private one.

Lastly there is no documentation on what algorithm/standard should be used for the keys. And I assume this matters or it does not? Like RSA, eliptic curve, etc...

Copy link
Member

Choose a reason for hiding this comment

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

rotating keys is a nice to have that still needs to be handled.

I guess we could also just not require a public key at all and always generate it.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could also just not require a public key at all and always generate it.

I think that is fine

rotating keys is a nice to have that still needs to be handled.

For this you would have to have some kind of watch in the secret service to restart it when the secret changes. I dont know how easy it is to implement this though. An easier way is to take the sha256 hash of the secret and make it an environment variable in the secret service. Then when the secret changes the secret service will automatically update. See https://helm.sh/docs/chart_template_guide/function_list/#sha256sum

Copy link
Member

Choose a reason for hiding this comment

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

And taking the sha256 digest still does not address the issue of how you decrypt already encrypted secrets that may be still lying around. So perhaps it is better to keep this simple in the first implementation and improve this later during cooldown.

helm-chart/renku/templates/secrets-storage/pdb.yaml Outdated Show resolved Hide resolved
helm-chart/renku/templates/secrets-storage/secret.yaml Outdated Show resolved Hide resolved
@Panaetius Panaetius temporarily deployed to ci-renku-3591 May 8, 2024 16:11 — with GitHub Actions Inactive
@Panaetius Panaetius marked this pull request as ready for review May 10, 2024 08:44
@Panaetius Panaetius requested review from a team as code owners May 10, 2024 08:44
@Panaetius Panaetius merged commit 6331a00 into release-0.52.x May 10, 2024
10 of 18 checks passed
@Panaetius Panaetius deleted the build/secrets-in-sessions branch May 10, 2024 09:04
lokijuhy pushed a commit that referenced this pull request May 27, 2024
* wip: charts for secrets storage

* add platform init script

* fix init container mount, add helm hook

* fix dockerfile

* comment secret storage for now

* update secrets storage chart

* update deploy action

* docs: add placeholder page for secrets documentation

* wip: charts for secrets storage (#3585)

* wip: charts for secrets storage

* add platform init script

* fix init container mount, add helm hook

* fix dockerfile

* comment secret storage for now

* update secrets storage chart

* update deploy action

* fix secret mount

* fix env var name

---------
Co-authored-by: Ralf Grubenmann <[email protected]>

* add network policy to allow notebooks access to secrets svc

* fix deploy action

* add secrets-storage values

* fix secrets-storage rbac

* add network policy for sidecar to data svc

* Update changelog

* Address some comments

* change secrets generation, remove unneeded secret

* remove leftover reference

---------

Co-authored-by: Mohammad Alisafaee <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
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.

6 participants