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

util: add vaultDestroyKeys option to destroy Vault kv-v2 secrets #2343

Merged
merged 4 commits into from Aug 6, 2021

Conversation

nixpanic
Copy link
Member

@nixpanic nixpanic commented Aug 2, 2021

Hashicorp Vault does not completely remove the secrets in a kv-v2
backend when the keys are deleted. The metadata of the keys will be
kept, and it is possible to recover the contents of the keys afterwards.

With the new vaultDestroyKeys configuration parameter, this behaviour
can now be selected. By default the parameter will be set to true,
indicating that the keys and contents should completely be destroyed.
Setting it to any other value will make it possible to recover the
deleted keys.

See-also: libopenstorage/secrets#55


Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)
  • /retest all: run this in case the CentOS CI failed to start/report any test
    progress or results

@nixpanic nixpanic added enhancement New feature or request component/rbd Issues related to RBD rebase update the version of an external component component/util Utility functions shared between CephFS and RBD labels Aug 2, 2021
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 2, 2021

@nixpanic CI failure.

@nixpanic nixpanic force-pushed the rbd/encryption/vault/destroy-v2-keys branch from 3d4618c to b614f13 Compare August 2, 2021 13:09
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 3, 2021

/test ci/centos/mini-e2e-helm/k8s-1.19

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 3, 2021

/test ci/centos/mini-e2e-helm/k8s-1.19

X Exiting due to GUEST_PROVISION: Failed to start host: creating host: create: Error creating machine: Error in driver during machine creation: IP not available after waiting: machine minikube didn't return IP after 1 minute

Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM. @nixpanic can we update E2E to verify keys are deleted completely?

@nixpanic nixpanic force-pushed the rbd/encryption/vault/destroy-v2-keys branch from b614f13 to 804d2e4 Compare August 3, 2021 09:33
@nixpanic
Copy link
Member Author

nixpanic commented Aug 3, 2021

LGTM. @nixpanic can we update E2E to verify keys are deleted completely?

Sure, we default to kv-v2 in our testing, and that always supports the metadata for keys. A new commit has been added that enables checking for the metadata after deletion (should have been removed as well).

@nixpanic nixpanic requested a review from Madhu-1 August 3, 2021 09:34
@nixpanic
Copy link
Member Author

nixpanic commented Aug 3, 2021

@Mergifyio rebase

#2349 has been merged, pulling the CentOS images should work again.

@nixpanic nixpanic force-pushed the rbd/encryption/vault/destroy-v2-keys branch from 804d2e4 to 67b9d72 Compare August 3, 2021 12:23
@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2021

Command rebase: success

Branch has been successfully rebased

@nixpanic
Copy link
Member Author

nixpanic commented Aug 3, 2021

/retest ci/centos/mini-e2e-helm/k8s-1.19

Madhu-1
Madhu-1 previously approved these changes Aug 4, 2021
Comment on lines +304 to +312
for k, v := range vc.keyContext {
keyContext[k] = v
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nixpanic do we need this new map generation or add key to existing map?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding it to the existing map will cause failures during non-delete operations. So, creating a copy of the map and adding it there.

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 4, 2021

/test ci/centos/mini-e2e-helm/k8s-1.20

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 4, 2021

/test ci/centos/mini-e2e-helm/k8s-1.20

Aug 3 14:41:07.037: timed out waiting for the rbd-nbd process: rbd-nbd process is not running yet: command terminated with exit code 1

@nixpanic nixpanic requested a review from humblec August 4, 2021 11:16
@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2021

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@nixpanic nixpanic force-pushed the rbd/encryption/vault/destroy-v2-keys branch from 67b9d72 to efd53f5 Compare August 5, 2021 06:22
@mergify mergify bot dismissed Madhu-1’s stale review August 5, 2021 06:23

Pull request has been modified.

@nixpanic nixpanic requested a review from Madhu-1 August 5, 2021 06:23
@nixpanic
Copy link
Member Author

nixpanic commented Aug 5, 2021

Rebasing with resolving merge conflicts was needed. Please review again, @Madhu-1 @humblec

go.mod Show resolved Hide resolved
@nixpanic
Copy link
Member Author

nixpanic commented Aug 6, 2021

@Mergifyio rebase

libopenstorage has added a new feature that makes it possible to destroy
the contents of a key/value in the Hashicorp Vault kv-v2 secrets backend.

See-also: libopenstorage/secrets#55
Signed-off-by: Niels de Vos <ndevos@redhat.com>
Hashicorp Vault does not completely remove the secrets in a kv-v2
backend when the keys are deleted. The metadata of the keys will be
kept, and it is possible to recover the contents of the keys afterwards.

With the new `vaultDestroyKeys` configuration parameter, this behaviour
can now be selected. By default the parameter will be set to `true`,
indicating that the keys and contents should completely be destroyed.
Setting it to any other value will make it possible to recover the
deleted keys.

Signed-off-by: Niels de Vos <ndevos@redhat.com>
Golang-ci complains about the following:

    internal/util/vault_tokens.go:99:20: string `true` has 4 occurrences, but such constant `vaultDefaultDestroyKeys` already exists (goconst)
    	v.VaultCAVerify = "true"
    	                  ^

This occurence of "true" can be replaced by vaultDefaultCAVerify so
address the warning.

Signed-off-by: Niels de Vos <ndevos@redhat.com>
The kmsConfig type in the e2e suite has been enhanced with two functions
that make it possible to validate the destruction of deleted keys.

Signed-off-by: Niels de Vos <ndevos@redhat.com>
@nixpanic nixpanic force-pushed the rbd/encryption/vault/destroy-v2-keys branch from efd53f5 to e2ea5d9 Compare August 6, 2021 10:38
@mergify
Copy link
Contributor

mergify bot commented Aug 6, 2021

Command rebase: success

Branch has been successfully rebased

@mergify mergify bot merged commit bb60173 into ceph:devel Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/rbd Issues related to RBD component/util Utility functions shared between CephFS and RBD enhancement New feature or request rebase update the version of an external component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants