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

External Secret Storage Implementation #85

Merged
merged 71 commits into from
May 26, 2023
Merged

External Secret Storage Implementation #85

merged 71 commits into from
May 26, 2023

Conversation

pradithya
Copy link
Member

@pradithya pradithya commented May 4, 2023

The PR implements the ability for storing secrets in external storage.

The main changes are as follows:

Add Secret Storage Resource

Secret Storage is a resource representing the abstraction for storing secrets. There are 2 types of secret storage implemented: internal and vault. An internal secret storage will store its secret values in the data column of the secrets table. On the other hand, vault secret storage will not populate the column, but instead, store the secret in the corresponding vault instance specified in the VaultConfig.

Allow Users to Choose Secret Storage when Creating Secrets

When creating a secret, users can choose the secret storage to be used. If the secret storage is not specified, it will be stored in the default secret storage that's being configured in the MLP deployment.
Note that secret migration is handled and will be triggered if the secret storage is modified.

Screen.Recording.2023-05-24.at.10.27.35.PM.mov

Secret Migration from Previous MLP API Version

Existing secrets will be migrated to the internal secret storage during the DB migration. To migrate to different secret storage we can send updated secret and specify the new secret storage.

Breaking Changes 🔥

Previous secret APIs implementation encrypts secret data during transfer and when storing it in DB. This behavior is removed and we'll rely on TLS to secure the secret transmission. In order to handle this we'll have to decrypt all the secrets and update the secret value when performing the migration to new secret storage.

Minor Improvements

  • Reuse make it-test-api for both testing in CI and locally.
  • Add make local-env to setup all necessary local dependencies
  • Rename storage package to repository so that it won't get confused with Secret Storage
  • Remove old vault code for retrieving cluster secrets

@pradithya pradithya changed the title Draft: ExternL Secret Storage Implementation Draft: External Secret Storage Implementation May 4, 2023
Pradithya Aria added 2 commits May 4, 2023 17:35
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Left some small comments. Really like the refactoring around the Repository and the test suites. LGTM!

api/client/model_secret_storage.go Show resolved Hide resolved
db-migrations/12_secret_storage.down.sql Outdated Show resolved Hide resolved
docker-compose.yaml Show resolved Hide resolved
api/util/map.go Outdated Show resolved Hide resolved
api/static/swagger.yaml Show resolved Hide resolved
api/api/secrets_api.go Outdated Show resolved Hide resolved
api/pkg/errors/errors.go Show resolved Hide resolved
api/pkg/secretstorage/vault.go Show resolved Hide resolved
api/pkg/secretstorage/vault.go Show resolved Hide resolved
api/repository/project_repository.go Show resolved Hide resolved
api/repository/secret_repository.go Outdated Show resolved Hide resolved
api/repository/secret_storage_repository.go Show resolved Hide resolved
db-migrations/12_secret_storage.up.sql Show resolved Hide resolved
api/service/secret_service.go Outdated Show resolved Hide resolved
@tiopramayudi
Copy link
Contributor

tiopramayudi commented May 24, 2023

Based on the video user can sees existing value in secret. I'm not sure that's a good idea, since in our case the mlp administrator also can see the secret that user defined

@pradithya pradithya temporarily deployed to manual May 24, 2023 03:51 — with GitHub Actions Inactive
@pradithya pradithya temporarily deployed to manual May 24, 2023 14:12 — with GitHub Actions Inactive
@pradithya
Copy link
Member Author

Based on the video user can sees existing value in secret. I'm not sure that's a good idea, since in our case the mlp administrator also can see the secret that user defined

Updated the secret data text box to use EuiFieldPassword instead of EuiTextArea, the caveat is that it's only a single row text box.

pradithya added a commit to caraml-dev/merlin that referenced this pull request May 25, 2023
**What this PR does / why we need it**:
MLP's Secret API removes the application-level encryption when
[returning the secret data](caraml-dev/mlp#85).
Moving forward, we'll rely on TLS to secure the secrets while in
transit.
This PR updates the MLP client used by Merlin and removes the decryption
process when retrieving a secret from MLP.

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required. Enter your extended release note in
the block below.
If the PR requires additional action from users switching to the new
release, include the string "action required".

For more information about release notes, see kubernetes' guide here:
http://git.k8s.io/community/contributors/guide/release-notes.md
-->

```release-note

```

**Checklist**

- [x] Tested in dev environment with MLP version
caraml-dev/mlp#85
- [N.A.] Added unit test, integration, and/or e2e tests
- [N.A.] Updated documentation
- [N.A.] Update Swagger spec if the PR introduce API changes
- [N.A.] Regenerated Golang and Python client if the PR introduce API
changes

---------

Co-authored-by: Pradithya Aria <pradithya.pura@go-jek.com>
@pradithya pradithya temporarily deployed to manual May 25, 2023 07:12 — with GitHub Actions Inactive
@pradithya pradithya merged commit 9a9acac into main May 26, 2023
@pradithya pradithya deleted the external_secret branch May 26, 2023 07:27
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