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(VSecM Safe): #460 Manual Root Key Updates K8s Secret #725

Conversation

gurkanguray
Copy link
Contributor

Manual Root Key Updates K8s Secret

Description

VSECM_MANUAL_ROOT_KEY_UPDATES_K8S_SECRET env variable added for giving an option to updating internal k8s secrets when manual root key provided.

Changes

  • Renamed app/safe/internal/privates.go to app/safe/internal/bootstrap/persist.go since we use this function in the app/safe/internal/server/route/receive.go to persist keys with the manual root key. Therefore, it's not private to bootstrap anymore.
  • Added VSECM_MANUAL_ROOT_KEY_UPDATES_K8S_SECRET to core/safe.go with its unit tests.
  • Added VSECM_MANUAL_ROOT_KEY_UPDATES_K8S_SECRET information details to docs/_pages/0110-configuration.md file

Test Policy Compliance

  • I have added or updated unit tests for my changes.
  • I have included integration tests where applicable.
  • All new and existing tests pass successfully.

Code Quality

  • I have followed the coding standards for this project.
  • I have performed a self-review of my code.
  • My code is well-commented, particularly in areas that may be difficult
    to understand.

Documentation

  • I have made corresponding changes to the documentation (if applicable).
  • I have updated any relevant READMEs or wiki pages.

Checklist

Before you submit this PR, please make sure:

  • You have read the contributing guidelines and
    especially the test policy.
  • You have thoroughly tested your changes.
  • You have followed all the contributing guidelines for this project.
  • You understand and agree that your contributions will be publicly available
    under the project’s license.

By submitting this pull request, you confirm that my contribution is made under
the terms of the project’s license and that you have the authority to grant
these rights.


Thank you for your contribution to VMware Secrets Manager
🐢⚡️!

…g an option to updating internal k8s secrets when manual root key providedVSECM_MANUAL_ROOT_KEY_UPDATES_K8S_SECRET env variable added for giving an option to updating internal k8s secrets when manual root key provided✨ feat(VSecM Safe): vmware-tanzu#460 Manual Root Key Updates K8s Secret

VSECM_MANUAL_ROOT_KEY_UPDATES_K8S_SECRET env variable added for giving an option to updating internal k8s secrets when manual root key provided

Signed-off-by: Guray Gurkan <guraygrkn@gmail.com>
Comment on lines +90 to +94
if env.RootKeyInputModeManual() && env.ManualRootKeyUpdatesKubernetesSecret() {
if err := bootstrap.PersistKeys(agePrivateKey, agePublicKey, aesCipherKey); err != nil {
log.ErrorLn(&cid, "Keys: Problem persisting keys", err.Error())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don’t need the checks in the first if.

The user can change the key (at their own risk) if they want to.

"manual mode" is only for the bootstrapping flow.

Can we replace it with this, please?

	if env.ManualRootKeyUpdatesKubernetesSecret() {
 		if err := bootstrap.PersistKeys(agePrivateKey, agePublicKey, aesCipherKey); err != nil {
 			log.ErrorLn(&cid, "Keys: Problem persisting keys", err.Error())
 		}
 	}

@@ -188,7 +188,7 @@ func CreateCryptoKey(id *string, updatedSecret chan<- bool) {

log.InfoLn(id, "Generated public key, private key, and aes seed")

if err = persistKeys(privateKey, publicKey, aesSeed); err != nil {
if err = PersistKeys(privateKey, publicKey, aesSeed); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment here?

Something like:

// Save the key to VSecM Safe's memory, and also save them to 
// VSecM Safe's root key Kubernetes Secret.
// If the root key input mode is not manual, VSecM Safe will
// use a trusted backing store to retrieve the key in case of
// Pod crash or eviction. As of Mar,17, 2024 the only trusted
// backing store is the VSecM root key Kubernetes Secret; however
// this will change in the future.
if err = PersistKeys(privateKey, publicKey, aesSeed); err != nil {

Copy link
Contributor

Choose a reason for hiding this comment

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

Reasoning: I always forget why we didn’t do a "IsManualRootKeySavedAsK8sSecret()" check here; so it’s reminder to me, and also anyone who thinks the same too :) .

Copy link
Contributor

@v0lkan v0lkan left a comment

Choose a reason for hiding this comment

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

Two relatively straightforward comments and change requests.

Other than that, it looks good.

Thanks for your contribution 🚀 .

@v0lkan
Copy link
Contributor

v0lkan commented Mar 26, 2024

This has been waiting for a while; I’ll merge it and add the necessary changes afterwards maybe.

@v0lkan
Copy link
Contributor

v0lkan commented Mar 27, 2024

What I’ll do.

  1. run tests on main
  2. if they pass, I’ll merge this and run tets.
  3. if they pass, I’ll do a tiny amend and repeat (2)
  4. if either fails, I’ll revert.

@v0lkan v0lkan merged commit 54f637f into vmware-tanzu:main Mar 27, 2024
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.

2 participants