From eea97ca0141b8016480bed296c7bb866695b2fc5 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Fri, 19 Mar 2021 16:21:48 +0100 Subject: [PATCH] util: move GetID() from EncryptionKMS to VolumeEncryption There is no need for each EncryptionKMS to implement the same GetID() function. We have a VolumeEncryption type that is more suitable for keeping track of the KMS-ID that was used to get the configuration of the KMS. This does not change any metadata that is stored anywhere. Signed-off-by: Niels de Vos --- internal/rbd/encryption.go | 2 +- internal/rbd/rbd_journal.go | 4 ++-- internal/util/crypto.go | 21 ++++++++++++++++++--- internal/util/crypto_test.go | 4 ++-- internal/util/kms.go | 14 ++++++++------ internal/util/secretskms.go | 13 ------------- internal/util/secretskms_test.go | 3 --- internal/util/vault.go | 18 +++++------------- internal/util/vault_tokens.go | 4 ++-- internal/util/vault_tokens_test.go | 1 - 10 files changed, 38 insertions(+), 46 deletions(-) diff --git a/internal/rbd/encryption.go b/internal/rbd/encryption.go index 2defec5c1..e9b2622cb 100644 --- a/internal/rbd/encryption.go +++ b/internal/rbd/encryption.go @@ -208,7 +208,7 @@ func (ri *rbdImage) configureEncryption(kmsID string, credentials map[string]str return err } - ri.encryption, err = util.NewVolumeEncryption(kms) + ri.encryption, err = util.NewVolumeEncryption(kmsID, kms) // if the KMS can not store the DEK itself, we'll store it in the // metadata of the RBD image itself diff --git a/internal/rbd/rbd_journal.go b/internal/rbd/rbd_journal.go index 3eafc0ed3..b413890cd 100644 --- a/internal/rbd/rbd_journal.go +++ b/internal/rbd/rbd_journal.go @@ -234,7 +234,7 @@ func (rv *rbdVolume) Exists(ctx context.Context, parentVol *rbdVolume) (bool, er kmsID := "" if rv.isEncrypted() { - kmsID = rv.encryption.KMS.GetID() + kmsID = rv.encryption.GetID() } j, err := volJournal.Connect(rv.Monitors, rv.RadosNamespace, rv.conn.Creds) @@ -411,7 +411,7 @@ func reserveVol(ctx context.Context, rbdVol *rbdVolume, rbdSnap *rbdSnapshot, cr kmsID := "" if rbdVol.isEncrypted() { - kmsID = rbdVol.encryption.KMS.GetID() + kmsID = rbdVol.encryption.GetID() } j, err := volJournal.Connect(rbdVol.Monitors, rbdVol.RadosNamespace, cr) diff --git a/internal/util/crypto.go b/internal/util/crypto.go index 6b87f60b5..7658d5a2c 100644 --- a/internal/util/crypto.go +++ b/internal/util/crypto.go @@ -57,6 +57,8 @@ type VolumeEncryption struct { // dekStore that will be used, this can be the EncryptionKMS or a // different object implementing the DEKStore interface. dekStore DEKStore + + id string } // NewVolumeEncryption creates a new instance of VolumeEncryption and @@ -65,8 +67,18 @@ type VolumeEncryption struct { // Callers that receive a ErrDEKStoreNeeded error, should use // VolumeEncryption.SetDEKStore() to configure an alternative storage for the // DEKs. -func NewVolumeEncryption(kms EncryptionKMS) (*VolumeEncryption, error) { - ve := &VolumeEncryption{KMS: kms} +func NewVolumeEncryption(id string, kms EncryptionKMS) (*VolumeEncryption, error) { + kmsID := id + if kmsID == "" { + // if kmsID is not set, encryption is enabled, and the type is + // SecretsKMS + kmsID = defaultKMSType + } + + ve := &VolumeEncryption{ + id: kmsID, + KMS: kms, + } if kms.requiresDEKStore() == DEKStoreIntegrated { dekStore, ok := kms.(DEKStore) @@ -103,11 +115,14 @@ func (ve *VolumeEncryption) RemoveDEK(volumeID string) error { return ve.dekStore.RemoveDEK(volumeID) } +func (ve *VolumeEncryption) GetID() string { + return ve.id +} + // EncryptionKMS provides external Key Management System for encryption // passphrases storage. type EncryptionKMS interface { Destroy() - GetID() string // requiresDEKStore returns the DEKStoreType that is needed to be // configure for the KMS. Nothing needs to be done when this function diff --git a/internal/util/crypto_test.go b/internal/util/crypto_test.go index 61cce4b81..08a57555e 100644 --- a/internal/util/crypto_test.go +++ b/internal/util/crypto_test.go @@ -58,11 +58,11 @@ func TestKMSWorkflow(t *testing.T) { kms, err := GetKMS("tenant", defaultKMSType, secrets) assert.NoError(t, err) require.NotNil(t, kms) - assert.Equal(t, defaultKMSType, kms.GetID()) - ve, err := NewVolumeEncryption(kms) + ve, err := NewVolumeEncryption("", kms) assert.NoError(t, err) require.NotNil(t, ve) + assert.Equal(t, defaultKMSType, ve.GetID()) volumeID := "volume-id" diff --git a/internal/util/kms.go b/internal/util/kms.go index 5e227702e..174b1f291 100644 --- a/internal/util/kms.go +++ b/internal/util/kms.go @@ -76,7 +76,7 @@ func GetKMS(tenant, kmsID string, secrets map[string]string) (EncryptionKMS, err "section: %s", kmsID) } - return kmsManager.buildKMS(kmsID, tenant, kmsConfig, secrets) + return kmsManager.buildKMS(tenant, kmsConfig, secrets) } // getKMSConfiguration reads the configuration file from the filesystem, or if @@ -181,9 +181,9 @@ func getKMSProvider(config map[string]interface{}) (string, error) { // KMSInitializerArgs get passed to KMSInitializerFunc when a new instance of a // KMSProvider is initialized. type KMSInitializerArgs struct { - ID, Tenant string - Config map[string]interface{} - Secrets map[string]string + Tenant string + Config map[string]interface{} + Secrets map[string]string } // KMSInitializerFunc gets called when the KMSProvider needs to be @@ -225,7 +225,10 @@ func RegisterKMSProvider(provider KMSProvider) bool { return true } -func (kf *kmsProviderList) buildKMS(kmsID, tenant string, config map[string]interface{}, secrets map[string]string) (EncryptionKMS, error) { +// buildKMS creates a new KMSProvider instance, based on the configuration that +// was passed. This uses getKMSProvider() internally to identify the +// KMSProvider to instantiate. +func (kf *kmsProviderList) buildKMS(tenant string, config map[string]interface{}, secrets map[string]string) (EncryptionKMS, error) { providerName, err := getKMSProvider(config) if err != nil { return nil, err @@ -238,7 +241,6 @@ func (kf *kmsProviderList) buildKMS(kmsID, tenant string, config map[string]inte } return provider.Initializer(KMSInitializerArgs{ - ID: kmsID, Tenant: tenant, Config: config, Secrets: secrets, diff --git a/internal/util/secretskms.go b/internal/util/secretskms.go index 94ac4c375..bfb8c981c 100644 --- a/internal/util/secretskms.go +++ b/internal/util/secretskms.go @@ -58,11 +58,6 @@ func initSecretsKMS(secrets map[string]string) (EncryptionKMS, error) { return SecretsKMS{passphrase: passphraseValue}, nil } -// GetID is returning ID representing default KMS `default`. -func (kms SecretsKMS) GetID() string { - return defaultKMSType -} - // Destroy frees all used resources. func (kms SecretsKMS) Destroy() { // nothing to do @@ -89,8 +84,6 @@ func (kms SecretsKMS) RemoveDEK(key string) error { // Data-Encryption-Key (DEK) in the metadata of the volume. type SecretsMetadataKMS struct { SecretsKMS - - encryptionKMSID string } var _ = RegisterKMSProvider(KMSProvider{ @@ -114,16 +107,10 @@ func initSecretsMetadataKMS(args KMSInitializerArgs) (EncryptionKMS, error) { smKMS := SecretsMetadataKMS{} smKMS.SecretsKMS = sKMS - smKMS.encryptionKMSID = args.ID return smKMS, nil } -// GetID is returning ID representing the SecretsMetadataKMS. -func (kms SecretsMetadataKMS) GetID() string { - return kms.encryptionKMSID -} - // Destroy frees all used resources. func (kms SecretsMetadataKMS) Destroy() { kms.SecretsKMS.Destroy() diff --git a/internal/util/secretskms_test.go b/internal/util/secretskms_test.go index 9da0c0d7b..9493ff13f 100644 --- a/internal/util/secretskms_test.go +++ b/internal/util/secretskms_test.go @@ -42,7 +42,6 @@ func TestGenerateCipher(t *testing.T) { func TestInitSecretsMetadataKMS(t *testing.T) { args := KMSInitializerArgs{ - ID: "secrets-metadata-unit-test", Tenant: "tenant", Config: nil, Secrets: map[string]string{}, @@ -59,7 +58,6 @@ func TestInitSecretsMetadataKMS(t *testing.T) { kms, err = initSecretsMetadataKMS(args) assert.NoError(t, err) require.NotNil(t, kms) - assert.Equal(t, "secrets-metadata-unit-test", kms.GetID()) assert.Equal(t, DEKStoreMetadata, kms.requiresDEKStore()) } @@ -68,7 +66,6 @@ func TestWorkflowSecretsMetadataKMS(t *testing.T) { encryptionPassphraseKey: "my-passphrase-from-kubernetes", } args := KMSInitializerArgs{ - ID: "secrets-metadata-unit-test", Tenant: "tenant", Config: nil, Secrets: secrets, diff --git a/internal/util/vault.go b/internal/util/vault.go index 5977fb07a..3d8f1f79c 100644 --- a/internal/util/vault.go +++ b/internal/util/vault.go @@ -71,10 +71,9 @@ Example JSON structure in the KMS config is, */ type vaultConnection struct { - EncryptionKMSID string - secrets loss.Secrets - vaultConfig map[string]interface{} - keyContext map[string]string + secrets loss.Secrets + vaultConfig map[string]interface{} + keyContext map[string]string } type VaultKMS struct { @@ -114,12 +113,10 @@ func setConfigString(option *string, config map[string]interface{}, key string) // vc.connectVault(). // // nolint:gocyclo // iterating through many config options, not complex at all. -func (vc *vaultConnection) initConnection(kmsID string, config map[string]interface{}) error { +func (vc *vaultConnection) initConnection(config map[string]interface{}) error { vaultConfig := make(map[string]interface{}) keyContext := make(map[string]string) - vc.EncryptionKMSID = kmsID - firstInit := (vc.vaultConfig == nil) vaultAddress := "" // required @@ -268,7 +265,7 @@ var _ = RegisterKMSProvider(KMSProvider{ // InitVaultKMS returns an interface to HashiCorp Vault KMS. func initVaultKMS(args KMSInitializerArgs) (EncryptionKMS, error) { kms := &VaultKMS{} - err := kms.initConnection(args.ID, args.Config) + err := kms.initConnection(args.Config) if err != nil { return nil, fmt.Errorf("failed to initialize Vault connection: %w", err) } @@ -328,11 +325,6 @@ func initVaultKMS(args KMSInitializerArgs) (EncryptionKMS, error) { return kms, nil } -// GetID is returning correlation ID to KMS configuration. -func (vc *vaultConnection) GetID() string { - return vc.EncryptionKMSID -} - // FetchDEK returns passphrase from Vault. The passphrase is stored in a // data.data.passphrase structure. func (kms *VaultKMS) FetchDEK(key string) (string, error) { diff --git a/internal/util/vault_tokens.go b/internal/util/vault_tokens.go index f9c93d6a4..cb9e4481c 100644 --- a/internal/util/vault_tokens.go +++ b/internal/util/vault_tokens.go @@ -199,7 +199,7 @@ func initVaultTokensKMS(args KMSInitializerArgs) (EncryptionKMS, error) { } kms := &VaultTokensKMS{} - err = kms.initConnection(args.ID, args.Config) + err = kms.initConnection(args.Config) if err != nil { return nil, fmt.Errorf("failed to initialize Vault connection: %w", err) } @@ -263,7 +263,7 @@ func initVaultTokensKMS(args KMSInitializerArgs) (EncryptionKMS, error) { // secrets. This method can be called multiple times, i.e. to override // configuration options from tenants. func (kms *VaultTokensKMS) parseConfig(config map[string]interface{}) error { - err := kms.initConnection(kms.EncryptionKMSID, config) + err := kms.initConnection(config) if err != nil { return err } diff --git a/internal/util/vault_tokens_test.go b/internal/util/vault_tokens_test.go index af2d425b3..89f498fe8 100644 --- a/internal/util/vault_tokens_test.go +++ b/internal/util/vault_tokens_test.go @@ -78,7 +78,6 @@ func TestInitVaultTokensKMS(t *testing.T) { } args := KMSInitializerArgs{ - ID: "vault-tokens-config", Tenant: "bob", Config: make(map[string]interface{}), Secrets: nil,