Skip to content
This repository has been archived by the owner on Mar 4, 2024. It is now read-only.

EVEREST-400 encrypt secrets in secrets storage #245

Merged
merged 21 commits into from
Oct 20, 2023

Conversation

recharte
Copy link
Collaborator

@recharte recharte commented Oct 16, 2023

EVEREST-400 Powered by Pull Request Badge

EVEREST-400 encrypt secrets in secrets storage

Problem:
EVEREST-400

We were previously storing secrets in plain-text which obviously a really bad idea.

Solution:
Leverage hashicorp vault's AESGCM barrier module to encrypt the secrets in the postgres internal database.

CHECKLIST

Jira

  • Is the Jira ticket created and referenced properly?

Tests

  • [] Is an Integration test/test case added for the new feature/change?
  • [] Are unit tests added where appropriate?

@recharte recharte force-pushed the EVEREST-400-encrypt-secrets branch 2 times, most recently from b570419 to e4b96a7 Compare October 17, 2023 08:09
@recharte recharte force-pushed the EVEREST-400-encrypt-secrets branch from e4b96a7 to c09ba52 Compare October 17, 2023 08:21
@recharte recharte marked this pull request as ready for review October 17, 2023 09:38
@recharte recharte requested a review from a user October 17, 2023 09:38
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Generally looks good. Left a couple comments for discussion.

cmd/config/config.go Outdated Show resolved Hide resolved
deploy/quickstart-k8s.yaml Outdated Show resolved Hide resolved
model/secrets_storage.go Show resolved Hide resolved
model/secrets_storage.go Outdated Show resolved Hide resolved
api/everest.go Outdated Show resolved Hide resolved
cmd/config/config.go Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
api/everest.go Outdated Show resolved Hide resolved
api/everest.go Outdated Show resolved Hide resolved
deploy/quickstart-k8s.yaml Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
model/secrets_storage.go Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Oct 17, 2023

@recharte One other thing I just realized.
I was expecting we would connect to an external Vault instance - e.g. in case customers already run their own instance.

Do I understand it correctly that instead of running a separate instance we're running Vault as part of Everest backend?

@recharte
Copy link
Collaborator Author

@recharte One other thing I just realized. I was expecting we would connect to an external Vault instance - e.g. in case customers already run their own instance.

Do I understand it correctly that instead of running a separate instance we're running Vault as part of Everest backend?

Neither of those is what we want right now.

What you're describing is phase 2 of the Percona secrets management plan. In phase 2, the plan is to connect to a vault instance which can be external and managed by the user or bundled with a percona product.

What we are implementing right now is phase 1. This leverages the already existing backend DB (postgresql) and encrypts the secrets before persisting them in the DB.
We had many options for handling this encryption before storing but we decided on using the module used by vault to abstract this operation in simple Put, Get, Delete methods. So we are not using vault per se, we are using the same mechanism that vault uses to encrypt secret when using the postgresql physical backend.

cmd/config/config.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
model/database.go Outdated Show resolved Hide resolved
api/everest.go Outdated Show resolved Hide resolved
@recharte recharte requested review from gen1us2k and a user October 17, 2023 19:21
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

lgtm

cmd/config/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@oksana-grishchenko oksana-grishchenko left a comment

Choose a reason for hiding this comment

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

Great job! Pretty tricky way to migrate the secrets storage, thanks for adding the comments. It should work though, haven't found any misalignment. Concerns that appeared during the review are already discussed and resolved in the existing threads, nothing to add here.

If we plan to drop some of the migration logic in the later releases, I would suggest to create an Admin&Maintenance ticket to not forget about it.

@recharte
Copy link
Collaborator Author

Hey folks, I'm keeping this PR open until Thursday, as I'm waiting for a confirmation from legal regarding the BSL license of [email protected].

recharte and others added 2 commits October 19, 2023 17:47
Co-authored-by: Maxim Kondratenko <[email protected]>
Co-authored-by: Maxim Kondratenko <[email protected]>
README.md Outdated Show resolved Hide resolved
Co-authored-by: Maxim Kondratenko <[email protected]>
api/everest.go Show resolved Hide resolved
README.md Show resolved Hide resolved
api/kubernetes.go Show resolved Hide resolved
cmd/config/config.go Show resolved Hide resolved
deploy/quickstart-compose.yml Outdated Show resolved Hide resolved
deploy/quickstart-k8s.yaml Outdated Show resolved Hide resolved
recharte and others added 2 commits October 20, 2023 09:49
Co-authored-by: Maxim Kondratenko <[email protected]>
Co-authored-by: Maxim Kondratenko <[email protected]>
@recharte recharte changed the title EVEREST-400 EVEREST-514 encrypt secrets in secrets storage EVEREST-400 encrypt secrets in secrets storage Oct 20, 2023
@recharte recharte merged commit 3918864 into main Oct 20, 2023
4 checks passed
@recharte recharte deleted the EVEREST-400-encrypt-secrets branch October 20, 2023 14:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants