From f584db41e6d3c99dfc6b8f4bd91dd6bbc10dec9b Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Mon, 2 Aug 2021 11:35:26 +0200 Subject: [PATCH] util: add vaultDestroyKeys option to destroy Vault kv-v2 secrets 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 --- .../kms/vault/csi-kms-connection-details.yaml | 1 + examples/kms/vault/kms-config.yaml | 4 +- internal/util/vault.go | 45 ++++++++++++++++++- internal/util/vault_test.go | 24 ++++++++++ internal/util/vault_tokens.go | 6 ++- internal/util/vault_tokens_test.go | 5 +++ 6 files changed, 82 insertions(+), 3 deletions(-) diff --git a/examples/kms/vault/csi-kms-connection-details.yaml b/examples/kms/vault/csi-kms-connection-details.yaml index 20413f244..ac641c7de 100644 --- a/examples/kms/vault/csi-kms-connection-details.yaml +++ b/examples/kms/vault/csi-kms-connection-details.yaml @@ -29,6 +29,7 @@ data: "KMS_PROVIDER": "vaulttokens", "VAULT_ADDR": "http://vault.default.svc.cluster.local:8200", "VAULT_BACKEND_PATH": "secret", + "VAULT_DESTROY_KEYS": "true", "VAULT_SKIP_VERIFY": "true" } vault-tenant-sa-test: |- diff --git a/examples/kms/vault/kms-config.yaml b/examples/kms/vault/kms-config.yaml index fe1234952..7ecefc1ba 100644 --- a/examples/kms/vault/kms-config.yaml +++ b/examples/kms/vault/kms-config.yaml @@ -10,6 +10,7 @@ data: "vaultAuthPath": "/v1/auth/kubernetes/login", "vaultRole": "csi-kubernetes", "vaultBackend": "kv-v2", + "vaultDestroyKeys": "true", "vaultPassphraseRoot": "/v1/secret", "vaultPassphrasePath": "ceph-csi/", "vaultCAVerify": "false" @@ -29,7 +30,8 @@ data: "vaultCAVerify": "true" }, "an-other-app": { - "tenantTokenName": "storage-encryption-token" + "tenantTokenName": "storage-encryption-token", + "vaultDestroyKeys": "false" } } }, diff --git a/internal/util/vault.go b/internal/util/vault.go index 79e377650..bb095d362 100644 --- a/internal/util/vault.go +++ b/internal/util/vault.go @@ -44,6 +44,7 @@ const ( vaultDefaultNamespace = "" vaultDefaultPassphrasePath = "" vaultDefaultCAVerify = "true" + vaultDefaultDestroyKeys = "true" ) var ( @@ -75,6 +76,15 @@ type vaultConnection struct { secrets loss.Secrets vaultConfig map[string]interface{} keyContext map[string]string + + // vaultDestroyKeys will by default set to `true`, and causes secrets + // to be deleted from Hashicorp Vault to be completely removed. Usually + // secrets in a kv-v2 store will be soft-deleted, and recovering the + // contents is still possible. + // + // This option is only valid during deletion of keys, see + // getDeleteKeyContext() for more details. + vaultDestroyKeys bool } type VaultKMS struct { @@ -153,6 +163,20 @@ func (vc *vaultConnection) initConnection(config map[string]interface{}) error { vaultConfig[vault.VaultBackendPathKey] = vaultBackendPath } + // always set the default to prevent recovering kv-v2 keys + vaultDestroyKeys := vaultDefaultDestroyKeys + err = setConfigString(&vaultDestroyKeys, config, "vaultDestroyKeys") + if errors.Is(err, errConfigOptionInvalid) { + return err + } + if firstInit || !errors.Is(err, errConfigOptionMissing) { + if vaultDestroyKeys == vaultDefaultDestroyKeys { + vc.vaultDestroyKeys = true + } else { + vc.vaultDestroyKeys = false + } + } + vaultTLSServerName := "" // optional err = setConfigString(&vaultTLSServerName, config, "vaultTLSServerName") if errors.Is(err, errConfigOptionInvalid) { @@ -274,6 +298,25 @@ func (vc *vaultConnection) Destroy() { } } +// getDeleteKeyContext creates a new KeyContext that has an optional value set +// to destroy the contents of secrets. This is configurable with the +// `vaultDestroyKeys` configuration parameter. +// +// Setting the option `DestroySecret` option in the KeyContext while creating +// new keys, causes failures, so the option only needs to be set in the +// RemoveDEK() calls. +func (vc *vaultConnection) getDeleteKeyContext() map[string]string { + keyContext := map[string]string{} + for k, v := range vc.keyContext { + keyContext[k] = v + } + if vc.vaultDestroyKeys { + keyContext[loss.DestroySecret] = vaultDefaultDestroyKeys + } + + return keyContext +} + var _ = RegisterKMSProvider(KMSProvider{ UniqueID: kmsTypeVault, Initializer: initVaultKMS, @@ -382,7 +425,7 @@ func (kms *VaultKMS) StoreDEK(key, value string) error { // RemoveDEK deletes passphrase from Vault. func (kms *VaultKMS) RemoveDEK(key string) error { pathKey := filepath.Join(kms.vaultPassphrasePath, key) - err := kms.secrets.DeleteSecret(pathKey, kms.keyContext) + err := kms.secrets.DeleteSecret(pathKey, kms.getDeleteKeyContext()) if err != nil { return fmt.Errorf("delete passphrase at %s request to vault failed: %w", pathKey, err) } diff --git a/internal/util/vault_test.go b/internal/util/vault_test.go index 43395d3bd..93caf707b 100644 --- a/internal/util/vault_test.go +++ b/internal/util/vault_test.go @@ -21,7 +21,9 @@ import ( "os" "testing" + loss "github.com/libopenstorage/secrets" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDetectAuthMountPath(t *testing.T) { @@ -101,6 +103,28 @@ func TestSetConfigString(t *testing.T) { } } +func TestDefaultVaultDestroyKeys(t *testing.T) { + t.Parallel() + + vc := &vaultConnection{} + config := make(map[string]interface{}) + config["vaultAddress"] = "https://vault.test.example.com" + err := vc.initConnection(config) + require.NoError(t, err) + keyContext := vc.getDeleteKeyContext() + destroySecret, ok := keyContext[loss.DestroySecret] + assert.NotEqual(t, destroySecret, "") + assert.True(t, ok) + + // setting vaultDestroyKeys to !true should remove the loss.DestroySecret entry + config["vaultDestroyKeys"] = "false" + err = vc.initConnection(config) + require.NoError(t, err) + keyContext = vc.getDeleteKeyContext() + _, ok = keyContext[loss.DestroySecret] + assert.False(t, ok) +} + func TestVaultKMSRegistered(t *testing.T) { t.Parallel() _, ok := kmsManager.providers[kmsTypeVault] diff --git a/internal/util/vault_tokens.go b/internal/util/vault_tokens.go index 6ee4d1e40..0eabe0feb 100644 --- a/internal/util/vault_tokens.go +++ b/internal/util/vault_tokens.go @@ -59,6 +59,7 @@ type standardVault struct { VaultADDR string `json:"VAULT_ADDR"` VaultBackend string `json:"VAULT_BACKEND"` VaultBackendPath string `json:"VAULT_BACKEND_PATH"` + VaultDestroyKeys string `json:"VAULT_DESTROY_KEYS"` VaultCACert string `json:"VAULT_CACERT"` VaultTLSServerName string `json:"VAULT_TLS_SERVER_NAME"` VaultClientCert string `json:"VAULT_CLIENT_CERT"` @@ -73,6 +74,7 @@ type vaultTokenConf struct { VaultAddress string `json:"vaultAddress"` VaultBackend string `json:"vaultBackend"` VaultBackendPath string `json:"vaultBackendPath"` + VaultDestroyKeys string `json:"vaultDestroyKeys"` VaultCAFromSecret string `json:"vaultCAFromSecret"` VaultTLSServerName string `json:"vaultTLSServerName"` VaultClientCertFromSecret string `json:"vaultClientCertFromSecret"` @@ -87,6 +89,7 @@ func (v *vaultTokenConf) convertStdVaultToCSIConfig(s *standardVault) { v.VaultAddress = s.VaultADDR v.VaultBackend = s.VaultBackend v.VaultBackendPath = s.VaultBackendPath + v.VaultDestroyKeys = s.VaultDestroyKeys v.VaultCAFromSecret = s.VaultCACert v.VaultClientCertFromSecret = s.VaultClientCert v.VaultClientCertKeyFromSecret = s.VaultClientKey @@ -479,7 +482,7 @@ func (vtc *vaultTenantConnection) StoreDEK(key, value string) error { // RemoveDEK deletes passphrase from Vault. func (vtc *vaultTenantConnection) RemoveDEK(key string) error { - err := vtc.secrets.DeleteSecret(key, vtc.keyContext) + err := vtc.secrets.DeleteSecret(key, vtc.getDeleteKeyContext()) if err != nil { return fmt.Errorf("delete passphrase at %s request to vault failed: %w", key, err) } @@ -526,6 +529,7 @@ func isTenantConfigOption(opt string) bool { case "vaultBackendPath": case "vaultAuthNamespace": case "vaultNamespace": + case "vaultDestroyKeys": case "vaultTLSServerName": case "vaultCAFromSecret": case "vaultCAVerify": diff --git a/internal/util/vault_tokens_test.go b/internal/util/vault_tokens_test.go index 5c0fbf77d..cddd8dd0a 100644 --- a/internal/util/vault_tokens_test.go +++ b/internal/util/vault_tokens_test.go @@ -127,6 +127,7 @@ func TestStdVaultToCSIConfig(t *testing.T) { "VAULT_ADDR":"https://vault.example.com", "VAULT_BACKEND":"kv-v2", "VAULT_BACKEND_PATH":"/secret", + "VAULT_DESTROY_KEYS":"true", "VAULT_CACERT":"", "VAULT_TLS_SERVER_NAME":"vault.example.com", "VAULT_CLIENT_CERT":"", @@ -156,6 +157,8 @@ func TestStdVaultToCSIConfig(t *testing.T) { t.Errorf("unexpected value for VaultBackend: %s", v.VaultBackend) case v.VaultBackendPath != "/secret": t.Errorf("unexpected value for VaultBackendPath: %s", v.VaultBackendPath) + case v.VaultDestroyKeys != vaultDefaultDestroyKeys: + t.Errorf("unexpected value for VaultDestroyKeys: %s", v.VaultDestroyKeys) case v.VaultCAFromSecret != "": t.Errorf("unexpected value for VaultCAFromSecret: %s", v.VaultCAFromSecret) case v.VaultClientCertFromSecret != "": @@ -180,6 +183,7 @@ func TestTransformConfig(t *testing.T) { cm["VAULT_ADDR"] = "https://vault.example.com" cm["VAULT_BACKEND"] = "kv-v2" cm["VAULT_BACKEND_PATH"] = "/secret" + cm["VAULT_DESTROY_KEYS"] = "true" cm["VAULT_CACERT"] = "" cm["VAULT_TLS_SERVER_NAME"] = "vault.example.com" cm["VAULT_CLIENT_CERT"] = "" @@ -194,6 +198,7 @@ func TestTransformConfig(t *testing.T) { assert.Equal(t, config["vaultAddress"], cm["VAULT_ADDR"]) assert.Equal(t, config["vaultBackend"], cm["VAULT_BACKEND"]) assert.Equal(t, config["vaultBackendPath"], cm["VAULT_BACKEND_PATH"]) + assert.Equal(t, config["vaultDestroyKeys"], cm["VAULT_DESTROY_KEYS"]) assert.Equal(t, config["vaultCAFromSecret"], cm["VAULT_CACERT"]) assert.Equal(t, config["vaultTLSServerName"], cm["VAULT_TLS_SERVER_NAME"]) assert.Equal(t, config["vaultClientCertFromSecret"], cm["VAULT_CLIENT_CERT"])