From 2f83d876ccf3f0106f7115f3341dfa182e7adca2 Mon Sep 17 00:00:00 2001 From: diverdane Date: Wed, 3 Nov 2021 08:38:10 -0400 Subject: [PATCH] Refactor k8s_secrets_storage to eliminate CodeClimate errors This change includes a bunch of refactoring to eliminate the following CodeClimate issues: - Function NewProviderForType has 7 arguments (exceeds 4 allowed). Consider refactoring. Found in pkg/secrets/provide_conjur_secrets.go - Function NewProvider has 5 arguments (exceeds 4 allowed). Consider refactoring. Found in pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go - Method k8sProvider.Provide has 6 return statements (exceeds 4 allowed). Found in pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go Changes include: - Eliminate the following arguments from secrets.NewProviderForType: - k8s.RetrieveK8sSecret - k8s.UpdateK8sSecret Now these dependencies are injected in a non-exported version of this function. - Similar to previous change, eliminated the same arguments in NewProvider in k8s_secrets_storage/provide_conjur_secrets.go. - Converted some functions to receiver methods to eliminate a bunch of arguments for those functions. - Consolidated some functions in k8sProvider.Provide to eliminate some return statements. - Reorganized tests provide_conjur_secrets_test.go to match refactoring changes in the functional code. - Because of the many changes in provide_conjur_secrets_test.go (previous bullet), converted Convey-based tests to table-driven Golang tests. - Fixed the mock AddSecret function. The current implementation only allows one Conjur secret per Kubernetes secret when adding a Kubernetes Secret to the mock database. This function should allow for each Kubernetes Secret to include mappings for multiple Conjur secrets. --- cmd/secrets-provider/main.go | 16 +- .../conjur/mocks/conjur_secrets_retriever.go | 10 +- .../mocks/k8s_secrets_client.go | 86 ++- .../k8s_secrets_storage/mocks/logger.go | 82 ++ .../provide_conjur_secrets.go | 355 +++++---- .../provide_conjur_secrets_test.go | 731 +++++++++--------- pkg/secrets/provide_conjur_secrets.go | 42 +- .../pushtofile/retrieve_secrets_test.go | 4 +- 8 files changed, 758 insertions(+), 568 deletions(-) create mode 100644 pkg/secrets/k8s_secrets_storage/mocks/logger.go diff --git a/cmd/secrets-provider/main.go b/cmd/secrets-provider/main.go index 2d9defbaf..8f97a0c16 100644 --- a/cmd/secrets-provider/main.go +++ b/cmd/secrets-provider/main.go @@ -13,7 +13,6 @@ import ( "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/annotations" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/conjur" - "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/k8s" secretsConfigProvider "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/config" ) @@ -62,15 +61,16 @@ func main() { printErrorAndExit(err.Error()) } + providerConfig := secrets.ProviderConfig{ + StoreType: secretsConfig.StoreType, + PodNamespace: secretsConfig.PodNamespace, + RequiredK8sSecrets: secretsConfig.RequiredK8sSecrets, + SecretFileBasePath: secretsBasePath, + AnnotationsMap: annotationsMap, + } provideSecrets, errs := secrets.NewProviderForType( - k8s.RetrieveK8sSecret, - k8s.UpdateK8sSecret, secretRetriever.Retrieve, - secretsConfig.StoreType, - secretsConfig.PodNamespace, - secretsConfig.RequiredK8sSecrets, - secretsBasePath, - annotationsMap, + providerConfig, ) logErrorsAndConditionalExit(errs, nil, messages.CSPFK053E) diff --git a/pkg/secrets/clients/conjur/mocks/conjur_secrets_retriever.go b/pkg/secrets/clients/conjur/mocks/conjur_secrets_retriever.go index ca4f74fc2..0880c8782 100644 --- a/pkg/secrets/clients/conjur/mocks/conjur_secrets_retriever.go +++ b/pkg/secrets/clients/conjur/mocks/conjur_secrets_retriever.go @@ -11,14 +11,14 @@ import ( validating that those secrets with 'execute' permissions can be fetched. */ -type ConjurMockClient struct { +type ConjurClient struct { CanExecute bool // TODO: CanExecute is really just used to assert on the presence of errors // and should probably just be an optional error. Database map[string]string } -func (c ConjurMockClient) RetrieveSecrets(secretIds []string) (map[string][]byte, error) { +func (c *ConjurClient) RetrieveSecrets(secretIds []string) (map[string][]byte, error) { res := make(map[string][]byte) if !c.CanExecute { @@ -38,20 +38,20 @@ func (c ConjurMockClient) RetrieveSecrets(secretIds []string) (map[string][]byte return res, nil } -func NewConjurMockClient() ConjurMockClient { +func NewConjurClient() *ConjurClient { database := map[string]string{ "conjur_variable1": "conjur_secret1", "conjur_variable2": "conjur_secret2", "conjur_variable_empty_secret": "", } - return ConjurMockClient{ + return &ConjurClient{ CanExecute: true, Database: database, } } -func (c ConjurMockClient) AddSecrets( +func (c *ConjurClient) AddSecrets( secrets map[string]string, ) { for id, secret := range secrets { diff --git a/pkg/secrets/k8s_secrets_storage/mocks/k8s_secrets_client.go b/pkg/secrets/k8s_secrets_storage/mocks/k8s_secrets_client.go index b387a2c91..9da464ce7 100644 --- a/pkg/secrets/k8s_secrets_storage/mocks/k8s_secrets_client.go +++ b/pkg/secrets/k8s_secrets_storage/mocks/k8s_secrets_client.go @@ -7,50 +7,77 @@ import ( v1 "k8s.io/api/core/v1" ) -type KubeSecretsMockClient struct { - // Mocks a K8s database. Maps k8s secret names to mock K8s secrets. - Database map[string]map[string][]byte +// K8sSecrets represents a collection of Kubernetes Secrets to be populated +// into the mock Kubernetes client's database. The logical hierarchy +// represented by this structure is: +// - Each Kubernetes Secret contains a 'Data' field. +// - Each 'Data' field contains one or entries that are key/value pairs. +// - The value in each 'Data' field entry can be a nested set of +// key/value pairs. In particular, for the entry with the key +// 'conjur-info', the value is expected to be a mapping of application +// secret names to the corresponding Conjur variable ID (or policy path) +// that should be used to retrieve the secret value. +type K8sSecrets map[string]k8sSecretData +type k8sSecretData map[string]k8sSecretDataValues +type k8sSecretDataValues map[string]string + +// KubeSecretsClient implements a mock Kubernetes client for testing +// Kubernetes Secrets access by the Secrets Provider. This client provides: +// - A Kubernetes Secret retrieve function +// - A Kubernetes Secret update function +// Kubernetes Secrets are populated for this mock client via the +// AddSecret method. Retrieval and update errors can be simulated +// for testing by setting the 'CanRetrieve' and 'CanUpdate' flags +// (respectively) to false. +type KubeSecretsClient struct { + // Mocks a K8s database. Maps k8s secret names to K8s secrets. + database map[string]map[string][]byte // TODO: CanRetrieve and CanUpdate are really just used to assert on the presence of errors // and should probably just be an optional error. CanRetrieve bool CanUpdate bool } -func NewKubeSecretsMockClient() KubeSecretsMockClient { - client := KubeSecretsMockClient{ - Database: map[string]map[string][]byte{}, +// NewKubeSecretsClient creates an instance of a KubeSecretsClient +func NewKubeSecretsClient() *KubeSecretsClient { + client := KubeSecretsClient{ + database: map[string]map[string][]byte{}, CanRetrieve: true, CanUpdate: true, } - return client + return &client } -func (c KubeSecretsMockClient) AddSecret( +// AddSecret adds a Kubernetes Secret to the mock Kubernetes Secrets client's +// database. +func (c *KubeSecretsClient) AddSecret( secretName string, - key string, - keyConjurPath string, + secretData k8sSecretData, ) { - conjurMap := map[string]string{ - key: keyConjurPath, - } - conjurMapBytes, err := yaml.Marshal(conjurMap) - if err != nil { - panic(err) + // Convert string values to YAML format + yamlizedSecretData := map[string][]byte{} + for key, value := range secretData { + yamlValue, err := yaml.Marshal(value) + if err != nil { + panic(err) + } + yamlizedSecretData[key] = yamlValue } - c.Database[secretName] = map[string][]byte{ - "conjur-map": conjurMapBytes, - } + c.database[secretName] = yamlizedSecretData } -func (c KubeSecretsMockClient) RetrieveSecret(_ string, secretName string) (*v1.Secret, error) { +// RetrieveSecret retrieves a Kubernetes Secret from the mock Kubernetes +// Secrets client's database. +func (c *KubeSecretsClient) RetrieveSecret(_ string, secretName string) (*v1.Secret, error) { + if !c.CanRetrieve { return nil, errors.New("custom error") } // Check if the secret exists in the mock K8s DB - secretData, ok := c.Database[secretName] + secretData, ok := c.database[secretName] if !ok { return nil, errors.New("custom error") } @@ -60,15 +87,28 @@ func (c KubeSecretsMockClient) RetrieveSecret(_ string, secretName string) (*v1. }, nil } -func (c KubeSecretsMockClient) UpdateSecret(_ string, secretName string, originalK8sSecret *v1.Secret, stringDataEntriesMap map[string][]byte) error { +// UpdateSecret updates a Kubernetes Secret in the mock Kubernetes +// Secrets client's database. +func (c *KubeSecretsClient) UpdateSecret( + _ string, secretName string, + originalK8sSecret *v1.Secret, + stringDataEntriesMap map[string][]byte) error { + if !c.CanUpdate { return errors.New("custom error") } - secretToUpdate := c.Database[secretName] + secretToUpdate := c.database[secretName] for key, value := range stringDataEntriesMap { secretToUpdate[key] = value } return nil } + +// InspectSecret provides a way for unit tests to view the 'Data' field +// content of a Kubernetes Secret by reading this content directly from +// the mock client's database. +func (c *KubeSecretsClient) InspectSecret(secretName string) map[string][]byte { + return c.database[secretName] +} diff --git a/pkg/secrets/k8s_secrets_storage/mocks/logger.go b/pkg/secrets/k8s_secrets_storage/mocks/logger.go new file mode 100644 index 000000000..918199b2f --- /dev/null +++ b/pkg/secrets/k8s_secrets_storage/mocks/logger.go @@ -0,0 +1,82 @@ +package mocks + +import ( + "errors" + "fmt" + "strings" +) + +// Logger is used to implement logging functions for testing the +// Kubernetes Secrets storage provider. +type Logger struct { + errors []string + warnings []string + infos []string + debugs []string +} + +// NewLogger returns a shiny, new Logger +func NewLogger() *Logger { + return &Logger{} +} + +// RecordedError logs that an error has occurred and returns a new error +// with the given error message. +func (l *Logger) RecordedError(msg string, args ...interface{}) error { + errStr := fmt.Sprintf(msg, args...) + l.errors = append(l.errors, errStr) + return errors.New(errStr) +} + +// Error logs an error. +func (l *Logger) Error(msg string, args ...interface{}) { + l.errors = append(l.errors, fmt.Sprintf(msg, args...)) +} + +// Warn logs a warning. +func (l *Logger) Warn(msg string, args ...interface{}) { + l.warnings = append(l.warnings, fmt.Sprintf(msg, args...)) +} + +// Info logs an info message. +func (l *Logger) Info(msg string, args ...interface{}) { + l.infos = append(l.infos, fmt.Sprintf(msg, args...)) +} + +// Debug logs a debug message. +func (l *Logger) Debug(msg string, args ...interface{}) { + l.debugs = append(l.debugs, fmt.Sprintf(msg, args...)) +} + +func (l *Logger) messageWasLogged(msg string, loggedMsgs []string) bool { + for _, loggedMsg := range loggedMsgs { + if strings.Contains(loggedMsg, msg) { + return true + } + } + return false +} + +// ErrorWasLogged determines if an error string appears in any +// errors that have been logged. +func (l *Logger) ErrorWasLogged(errStr string) bool { + return l.messageWasLogged(errStr, l.errors) +} + +// WarningWasLogged determines if a warning string appears in any +// warning messages that have been logged. +func (l *Logger) WarningWasLogged(warning string) bool { + return l.messageWasLogged(warning, l.warnings) +} + +// InfoWasLogged determines if a warning string appears in any +// info messages that have been logged. +func (l *Logger) InfoWasLogged(info string) bool { + return l.messageWasLogged(info, l.infos) +} + +// DebugWasLogged determines if a debug string appears in any +// debug messages that have been logged. +func (l *Logger) DebugWasLogged(debug string) bool { + return l.messageWasLogged(debug, l.debugs) +} diff --git a/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go b/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go index 3aa5a4d16..e55069cf6 100644 --- a/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go +++ b/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go @@ -1,200 +1,273 @@ -package k8s_secrets_storage +package k8ssecretsstorage import ( - "fmt" - "strings" - "github.com/cyberark/conjur-authn-k8s-client/pkg/log" "gopkg.in/yaml.v2" v1 "k8s.io/api/core/v1" "github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/conjur" - "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/k8s" + k8sClient "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/k8s" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/config" ) -type K8sSecretsMap struct { - // Maps a k8s Secret name to a data-entry map that holds the new entries that will be added to the k8s secret. - // The data-entry map's key represents an entry name and the value is a Conjur variable ID that holds the value - // of the required k8s secret. After the secret is retrieved from Conjur we replace the variable ID with its - // corresponding secret value. - // The variable ID (which is replaced later with the secret) is held as a byte array so we don't hold the secret as - // clear text string - K8sSecrets map[string]map[string][]byte - - // Maps a k8s Secret name to original K8sSecret fetched from k8s - OriginalK8sSecrets map[string]*v1.Secret - - // Maps a conjur variable id to its place in the k8sSecretsMap. This object helps us to replace - // the variable IDs with their corresponding secret value in the map - PathMap map[string][]string +// Secrets that have been retrieved from Conjur may need to be updated in +// more than one Kubernetes Secrets, and each Kubernetes Secret may refer to +// the application secret with a different name. The updateDestination struct +// represents one destination to which a retrieved Conjur secret value needs +// to be written when Kubernetes Secrets are updated. +type updateDestination struct { + k8sSecretName string + secretName string +} + +type k8sSecretsState struct { + // Maps a K8s Secret name to the original K8s Secret API object fetched + // from K8s. + originalK8sSecrets map[string]*v1.Secret + + // Maps a Conjur variable ID (policy path) to all of the updateDestination + // targets which will need to be updated with the corresponding Conjur + // secret value after it has been retrieved. + updateDestinations map[string][]updateDestination +} + +type k8sAccessDeps struct { + retrieveSecret k8sClient.RetrieveK8sSecretFunc + updateSecret k8sClient.UpdateK8sSecretFunc +} + +type conjurAccessDeps struct { + retrieveSecrets conjur.RetrieveSecretsFunc +} + +type logFunc func(message string, args ...interface{}) +type logFuncWithErr func(message string, args ...interface{}) error +type logDeps struct { + recordedError logFuncWithErr + logError logFunc + warn logFunc + info logFunc + debug logFunc } -type k8sProvider struct { - retrieveK8sSecret k8s.RetrieveK8sSecretFunc - updateK8sSecret k8s.UpdateK8sSecretFunc - retrieveSecretsFunc conjur.RetrieveSecretsFunc - podNamespace string - requiredK8sSecrets []string +type k8sProviderDeps struct { + k8s k8sAccessDeps + conjur conjurAccessDeps + log logDeps +} + +// K8sProvider is the secret provider to be used for K8s Secrets mode. It +// makes secrets available to applications by: +// - Retrieving a list of required K8s Secrets +// - Retrieving all Conjur secrets that are referenced (via variable ID, +// a.k.a. policy path) by those K8s Secrets. +// - Updating the K8s Secrets by replacing each Conjur variable ID +// with the corresponding secret value that was retrieved from Conjur. +type K8sProvider struct { + k8s k8sAccessDeps + conjur conjurAccessDeps + log logDeps + podNamespace string + requiredK8sSecrets []string + secretsState k8sSecretsState } // NewProvider creates a new secret provider for K8s Secrets mode. func NewProvider( - retrievek8sSecret k8s.RetrieveK8sSecretFunc, - updatek8sSecret k8s.UpdateK8sSecretFunc, - retrieveSecretsFunc conjur.RetrieveSecretsFunc, + retrieveConjurSecrets conjur.RetrieveSecretsFunc, requiredK8sSecrets []string, podNamespace string, -) k8sProvider { - return k8sProvider{ - retrieveK8sSecret: retrievek8sSecret, - updateK8sSecret: updatek8sSecret, - retrieveSecretsFunc: retrieveSecretsFunc, - podNamespace: podNamespace, - requiredK8sSecrets: requiredK8sSecrets, - } +) K8sProvider { + return newProvider( + k8sProviderDeps{ + k8s: k8sAccessDeps{ + k8sClient.RetrieveK8sSecret, + k8sClient.UpdateK8sSecret, + }, + conjur: conjurAccessDeps{ + retrieveConjurSecrets, + }, + log: logDeps{ + log.RecordedError, + log.Error, + log.Warn, + log.Info, + log.Debug, + }, + }, + requiredK8sSecrets, + podNamespace) } -// Provide implements a ProviderFunc to retrieve and push secrets to K8s secrets. -func (p k8sProvider) Provide() error { - k8sSecretsMap, err := RetrieveRequiredK8sSecrets(p.retrieveK8sSecret, p.podNamespace, p.requiredK8sSecrets) - - if err != nil { - return log.RecordedError(messages.CSPFK021E) +// newProvider creates a new secret provider for K8s Secrets mode +// using dependencies provided for retrieving and updating Kubernetes +// Secrets objects. +func newProvider( + providerDeps k8sProviderDeps, + requiredK8sSecrets []string, + podNamespace string, +) K8sProvider { + return K8sProvider{ + k8s: providerDeps.k8s, + conjur: providerDeps.conjur, + log: providerDeps.log, + podNamespace: podNamespace, + requiredK8sSecrets: requiredK8sSecrets, + secretsState: k8sSecretsState{ + originalK8sSecrets: map[string]*v1.Secret{}, + updateDestinations: map[string][]updateDestination{}, + }, } +} - variableIDs, err := getVariableIDsToRetrieve(k8sSecretsMap.PathMap) - if err != nil { - return log.RecordedError(messages.CSPFK037E) +// Provide implements a ProviderFunc to retrieve and push secrets to K8s secrets. +func (p K8sProvider) Provide() error { + // Retrieve required K8s Secrets and parse their Data fields. + if err := p.retrieveRequiredK8sSecrets(); err != nil { + return p.log.recordedError(messages.CSPFK021E) } - retrievedConjurSecrets, err := p.retrieveSecretsFunc(variableIDs) + // Retrieve Conjur secrets for all K8s Secrets. + retrievedConjurSecrets, err := p.retrieveConjurSecrets() if err != nil { - return log.RecordedError(messages.CSPFK034E, err.Error()) + return p.log.recordedError(messages.CSPFK034E, err.Error()) } - err = updateK8sSecretsMapWithConjurSecrets(k8sSecretsMap, retrievedConjurSecrets) - if err != nil { - return log.RecordedError(messages.CSPFK027E) + // Update all K8s Secrets with the retrieved Conjur secrets. + if err = p.updateRequiredK8sSecrets(retrievedConjurSecrets); err != nil { + return p.log.recordedError(messages.CSPFK023E) } - err = UpdateRequiredK8sSecrets(p.updateK8sSecret, p.podNamespace, k8sSecretsMap) + p.log.info(messages.CSPFK009I) + return nil +} - if err != nil { - return log.RecordedError(messages.CSPFK023E) +// retrieveRequiredK8sSecrets retrieves all K8s Secrets that need to be +// managed/updated by the Secrets Provider. +func (p K8sProvider) retrieveRequiredK8sSecrets() error { + for _, k8sSecretName := range p.requiredK8sSecrets { + if err := p.retrieveRequiredK8sSecret(k8sSecretName); err != nil { + return err + } } - - log.Info(messages.CSPFK009I) return nil } -func RetrieveRequiredK8sSecrets(retrieveSecretFunc k8s.RetrieveK8sSecretFunc, namespace string, requiredK8sSecrets []string) (*K8sSecretsMap, error) { - k8sSecrets := make(map[string]map[string][]byte) - originalK8sSecrets := make(map[string]*v1.Secret) - pathMap := make(map[string][]string) +// retrieveRequiredK8sSecret retrieves an individual K8s Secrets that needs +// to be managed/updated by the Secrets Provider. +func (p K8sProvider) retrieveRequiredK8sSecret(k8sSecretName string) error { - for _, secretName := range requiredK8sSecrets { + // Retrieve the K8s Secret + k8sSecret, err := p.k8s.retrieveSecret(p.podNamespace, k8sSecretName) + if err != nil { + // Error messages returned from K8s should be printed only in debug mode + p.log.debug(messages.CSPFK004D, err.Error()) + return p.log.recordedError(messages.CSPFK020E) + } - k8sSecret, err := retrieveSecretFunc(namespace, secretName) - if err != nil { - // Error messages returned from K8s should be printed only in debug mode - log.Debug(messages.CSPFK004D, err.Error()) - return nil, log.RecordedError(messages.CSPFK020E) - } + // Record the K8s Secret API object + p.secretsState.originalK8sSecrets[k8sSecretName] = k8sSecret + + // Read the value of the "conjur-map" entry in the K8s Secret's Data + // field, if it exists. If the entry does not exist or has a null + // value, return an error. + conjurMapKey := config.ConjurMapKey + conjurSecretsYAML, entryExists := k8sSecret.Data[conjurMapKey] + if !entryExists { + p.log.debug(messages.CSPFK008D, k8sSecretName, conjurMapKey) + return p.log.recordedError(messages.CSPFK028E, k8sSecretName) + } + if len(conjurSecretsYAML) == 0 { + p.log.debug(messages.CSPFK006D, k8sSecretName, conjurMapKey) + return p.log.recordedError(messages.CSPFK028E, k8sSecretName) + } - // If K8s secret has no "conjur-map" data entry, return an error - if _, ok := k8sSecret.Data[config.ConjurMapKey]; !ok { - // Error messages returned from K8s should be printed only in debug mode - log.Debug(messages.CSPFK008D, secretName, config.ConjurMapKey) - return nil, log.RecordedError(messages.CSPFK028E, secretName) - } + // Parse the YAML-formatted Conjur secrets mapping that has been + // retrieved from this K8s Secret. + p.log.debug(messages.CSPFK009D, conjurMapKey, k8sSecretName) + return p.parseConjurSecretsYAML(conjurSecretsYAML, k8sSecretName) +} - // Parse its "conjur-map" data entry and store its values in the new-data-entries map - // This map holds data entries that will be added to the k8s secret after we retrieve their values from Conjur - newDataEntriesMap := make(map[string][]byte) - conjurMap := make(map[string]string) - for key, value := range k8sSecret.Data { - if key == config.ConjurMapKey { - if len(value) == 0 { - // Error messages returned from K8s should be printed only in debug mode - log.Debug(messages.CSPFK006D, secretName, config.ConjurMapKey) - return nil, log.RecordedError(messages.CSPFK028E, secretName) - } - - log.Debug(messages.CSPFK009D, config.ConjurMapKey, secretName) - err := yaml.Unmarshal(value, &conjurMap) - if err != nil { - // Error messages returned from K8s should be printed only in debug mode - log.Debug(messages.CSPFK007D, secretName, config.ConjurMapKey, err.Error()) - return nil, log.RecordedError(messages.CSPFK028E, secretName) - } else if len(conjurMap) == 0 { - // Error messages returned from K8s should be printed only in debug mode - log.Debug(messages.CSPFK007D, secretName, config.ConjurMapKey, "value is empty") - return nil, log.RecordedError(messages.CSPFK028E, secretName) - } - - for k8sSecretKey, conjurVariableId := range conjurMap { - newDataEntriesMap[k8sSecretKey] = []byte(conjurVariableId) - - // This map will help us later to swap the variable id with the secret value - pathMap[conjurVariableId] = append(pathMap[conjurVariableId], fmt.Sprintf("%s:%s", secretName, k8sSecretKey)) - } - } - } +// Parse the YAML-formatted Conjur secrets mapping that has been retrieved +// from a K8s Secret. This secrets mapping uses application secret names +// as keys and Conjur variable IDs (a.k.a. policy paths) as values. +func (p K8sProvider) parseConjurSecretsYAML(secretsYAML []byte, + k8sSecretName string) error { - k8sSecrets[secretName] = newDataEntriesMap - originalK8sSecrets[secretName] = k8sSecret + conjurMap := map[string]string{} + if err := yaml.Unmarshal(secretsYAML, &conjurMap); err != nil { + p.log.debug(messages.CSPFK007D, k8sSecretName, config.ConjurMapKey, err.Error()) + return p.log.recordedError(messages.CSPFK028E, k8sSecretName) + } + if len(conjurMap) == 0 { + p.log.debug(messages.CSPFK007D, k8sSecretName, config.ConjurMapKey, "value is empty") + return p.log.recordedError(messages.CSPFK028E, k8sSecretName) } - return &K8sSecretsMap{ - K8sSecrets: k8sSecrets, - OriginalK8sSecrets: originalK8sSecrets, - PathMap: pathMap, - }, nil -} - -func UpdateRequiredK8sSecrets(updateSecretFunc k8s.UpdateK8sSecretFunc, namespace string, k8sSecretsMap *K8sSecretsMap) error { - for secretName, dataEntryMap := range k8sSecretsMap.K8sSecrets { - err := updateSecretFunc(namespace, secretName, k8sSecretsMap.OriginalK8sSecrets[secretName], dataEntryMap) - if err != nil { - // Error messages returned from K8s should be printed only in debug mode - log.Debug(messages.CSPFK005D, err.Error()) - return log.RecordedError(messages.CSPFK022E) - } + for secretName, varID := range conjurMap { + dest := updateDestination{k8sSecretName, secretName} + p.secretsState.updateDestinations[varID] = + append(p.secretsState.updateDestinations[varID], dest) } return nil } -func getVariableIDsToRetrieve(pathMap map[string][]string) ([]string, error) { - var variableIDs []string - - if len(pathMap) == 0 { - return nil, log.RecordedError(messages.CSPFK025E) +func (p K8sProvider) retrieveConjurSecrets() (map[string][]byte, error) { + updateDests := p.secretsState.updateDestinations + if len(updateDests) == 0 { + return nil, p.log.recordedError(messages.CSPFK025E) } - for key, _ := range pathMap { + // Gather the set of variable IDs for all secrets that need to be + // retrieved from Conjur. + var variableIDs []string + for key := range updateDests { variableIDs = append(variableIDs, key) } - return variableIDs, nil + retrievedConjurSecrets, err := p.conjur.retrieveSecrets(variableIDs) + if err != nil { + return nil, p.log.recordedError(messages.CSPFK034E, err.Error()) + } + return retrievedConjurSecrets, nil } -func updateK8sSecretsMapWithConjurSecrets(k8sSecretsMap *K8sSecretsMap, conjurSecrets map[string][]byte) error { - var err error - - // Update K8s map by replacing variable IDs with their corresponding secret values - for variableId, secret := range conjurSecrets { - if err != nil { - return log.RecordedError(messages.CSPFK035E) +func (p K8sProvider) updateRequiredK8sSecrets( + conjurSecrets map[string][]byte) error { + + // Create a map of entries to be added to the 'Data' fields of each + // K8s Secret. Each entry will map an application secret name to + // a value retrieved from Conjur. + newSecretData := map[string]map[string][]byte{} + for variableID, secretValue := range conjurSecrets { + dests := p.secretsState.updateDestinations[variableID] + for _, dest := range dests { + k8sSecretName := dest.k8sSecretName + secretName := dest.secretName + // If there are no data entries for this K8s Secret yet, initialize + // its map of data entries. + if newSecretData[k8sSecretName] == nil { + newSecretData[k8sSecretName] = map[string][]byte{} + } + newSecretData[k8sSecretName][secretName] = secretValue } + // Null out the secret value + conjurSecrets[variableID] = []byte{} + } - for _, locationInK8sSecretsMap := range k8sSecretsMap.PathMap[variableId] { - locationInK8sSecretsMap := strings.Split(locationInK8sSecretsMap, ":") - k8sSecretName := locationInK8sSecretsMap[0] - k8sSecretDataEntryKey := locationInK8sSecretsMap[1] - k8sSecretsMap.K8sSecrets[k8sSecretName][k8sSecretDataEntryKey] = secret + // Update K8s Secrets with the retrieved Conjur secrets + for k8sSecretName, secretData := range newSecretData { + err := p.k8s.updateSecret( + p.podNamespace, + k8sSecretName, + p.secretsState.originalK8sSecrets[k8sSecretName], + secretData) + if err != nil { + // Error messages returned from K8s should be printed only in debug mode + p.log.debug(messages.CSPFK005D, err.Error()) + return p.log.recordedError(messages.CSPFK022E) } } diff --git a/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go b/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go index 09d3d4cc8..16836a235 100644 --- a/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go +++ b/pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go @@ -1,395 +1,384 @@ -package k8s_secrets_storage +package k8ssecretsstorage import ( "fmt" - "sort" "testing" - . "github.com/smartystreets/goconvey/convey" + //. "github.com/smartystreets/goconvey/convey" + "github.com/stretchr/testify/assert" "github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages" conjurMocks "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/conjur/mocks" - secretsStorageMocks "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/k8s_secrets_storage/mocks" + k8sStorageMocks "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/k8s_secrets_storage/mocks" ) -func TestProvideConjurSecrets(t *testing.T) { - Convey("getVariableIDsToRetrieve", t, func() { - - Convey("Given a non-empty pathMap", func() { - pathMap := map[string][]string{ - "account/var_path1": {"secret1:key1"}, - "account/var_path2": {"secret1:key2"}, - } - variableIDsExpected := []string{"account/var_path1", "account/var_path2"} - variableIDsActual, err := getVariableIDsToRetrieve(pathMap) - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Returns variable IDs in the pathMap as expected", func() { - // Sort actual and expected, because output order can change - sort.Strings(variableIDsActual) - sort.Strings(variableIDsExpected) - - So(variableIDsActual, ShouldResemble, variableIDsExpected) - }) - }) - - Convey("Given an empty pathMap", func() { - pathMap := map[string][]string{} - - Convey("Raises an error that the map input is empty", func() { - _, err := getVariableIDsToRetrieve(pathMap) - So(err.Error(), ShouldEqual, messages.CSPFK025E) - }) - }) - }) - - Convey("updateK8sSecretsMapWithConjurSecrets", t, func() { - Convey("Given one K8s secret with one Conjur secret", func() { - conjurSecrets := map[string][]byte{ - "allowed/username": []byte("super"), - } - - k8sSecretsMap := map[string]map[string][]byte{ - "mysecret": { - "username": []byte("allowed/username"), - }, - } - - pathMap := map[string][]string{ - "allowed/username": {"mysecret:username"}, - } - - k8sSecretsStruct := K8sSecretsMap{k8sSecretsMap, nil, pathMap} - err := updateK8sSecretsMapWithConjurSecrets(&k8sSecretsStruct, conjurSecrets) - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Replaces secret variable IDs in k8sSecretsMap with their corresponding secret value", func() { - So(k8sSecretsStruct.K8sSecrets["mysecret"]["username"], ShouldResemble, []byte("super")) - }) - }) - - Convey("Given 2 k8s secrets that need the same Conjur secret", func() { - conjurSecrets := map[string][]byte{ - "allowed/username": []byte("super"), - } - dataEntriesMap := map[string][]byte{ - "username": []byte("allowed/username"), - } - k8sSecretsMap := map[string]map[string][]byte{ - "secret": dataEntriesMap, - "another-secret": dataEntriesMap, - } - - pathMap := map[string][]string{ - "allowed/username": {"secret:username", "another-secret:username"}, - } - - k8sSecretsStruct := K8sSecretsMap{k8sSecretsMap, nil, pathMap} - err := updateK8sSecretsMapWithConjurSecrets(&k8sSecretsStruct, conjurSecrets) - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Replaces both Variable IDs in k8sSecretsMap to their corresponding secret values without errors", func() { - secret := []byte("super") - So(k8sSecretsStruct.K8sSecrets["secret"]["username"], ShouldResemble, secret) - So(k8sSecretsStruct.K8sSecrets["another-secret"]["username"], ShouldResemble, secret) - }) - }) - }) - - Convey("RetrieveRequiredK8sSecrets", t, func() { - Convey("Given an existing k8s secret that is mapped to an existing conjur secret", func() { - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.AddSecret("k8s_secret1", "secret_key", "conjur_variable1") - - requiredSecrets := []string{"k8s_secret1"} - - k8sSecretsMap, err := RetrieveRequiredK8sSecrets(kubeMockClient.RetrieveSecret, "someNameSpace", requiredSecrets) - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Creates K8sSecrets map as expected", func() { - expectedK8sSecrets := map[string]map[string][]byte{ - "k8s_secret1": { - "secret_key": []byte("conjur_variable1"), - }, - } - - So(k8sSecretsMap.K8sSecrets, ShouldResemble, expectedK8sSecrets) - }) - - Convey("Creates PathMap map as expected", func() { - expectedPathMap := map[string][]string{ - "conjur_variable1": {"k8s_secret1:secret_key"}, - } - - So(k8sSecretsMap.PathMap, ShouldResemble, expectedPathMap) - }) - }) - - Convey("Given no 'get' permissions on the 'secrets' k8s resource", func() { - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.CanRetrieve = false - kubeMockClient.AddSecret("k8s_secret1", "secret_key", "conjur_variable1") - - requiredSecrets := []string{"k8s_secret1"} - - _, err := RetrieveRequiredK8sSecrets(kubeMockClient.RetrieveSecret, "someNameSpace", requiredSecrets) - - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, messages.CSPFK020E) - }) - }) +var testConjurSecrets = map[string]string{ + "conjur/var/path1": "secret-value1", + "conjur/var/path2": "secret-value2", + "conjur/var/path3": "secret-value3", + "conjur/var/path4": "secret-value4", + "conjur/var/empty-secret": "", +} - Convey("Given a non-existing k8s secret", func() { - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() +type testMocks struct { + conjurClient *conjurMocks.ConjurClient + kubeClient *k8sStorageMocks.KubeSecretsClient + logger *k8sStorageMocks.Logger +} - requiredSecrets := []string{"non_existing"} +func newTestMocks() testMocks { + mocks := testMocks{ + conjurClient: conjurMocks.NewConjurClient(), + kubeClient: k8sStorageMocks.NewKubeSecretsClient(), + logger: k8sStorageMocks.NewLogger(), + } + // Populate Conjur with some test secrets + mocks.conjurClient.AddSecrets(testConjurSecrets) + return mocks +} - _, err := RetrieveRequiredK8sSecrets(kubeMockClient.RetrieveSecret, "someNameSpace", requiredSecrets) +func (m testMocks) setPermissions(denyConjurRetrieve, denyK8sRetrieve, + denyK8sUpdate bool) { + if denyConjurRetrieve { + m.conjurClient.CanExecute = false + } + if denyK8sRetrieve { + m.kubeClient.CanRetrieve = false + } + if denyK8sUpdate { + m.kubeClient.CanUpdate = false + } +} - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, messages.CSPFK020E) - }) - }) +func (m testMocks) newProvider(requiredSecrets []string) K8sProvider { + return newProvider( + k8sProviderDeps{ + k8s: k8sAccessDeps{ + m.kubeClient.RetrieveSecret, + m.kubeClient.UpdateSecret, + }, + conjur: conjurAccessDeps{ + m.conjurClient.RetrieveSecrets, + }, + log: logDeps{ + m.logger.RecordedError, + m.logger.Error, + m.logger.Warn, + m.logger.Info, + m.logger.Debug, + }, + }, + requiredSecrets, + "someNamespace") +} - Convey("Given a k8s secret without 'conjur-map'", func() { - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.Database["no_conjur_map_secret"] = map[string][]byte{ - "not-conjur-map": []byte("some-data"), - } +type assertFunc func(*testing.T, testMocks, error, string) +type expectedK8sSecrets map[string]map[string]string +type expectedMissingValues map[string][]string - requiredSecrets := []string{"no_conjur_map_secret"} - _, err := RetrieveRequiredK8sSecrets(kubeMockClient.RetrieveSecret, "someNameSpace", requiredSecrets) +func assertErrorContains(expErrStr string) assertFunc { + return func(t *testing.T, _ testMocks, + err error, desc string) { - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, fmt.Sprintf(messages.CSPFK028E, "no_conjur_map_secret")) - }) - }) + assert.Error(t, err, desc) + assert.Contains(t, err.Error(), expErrStr, desc) + } +} - Convey("Given a k8s secret with an empty 'conjur-map'", func() { - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.Database["empty_conjur_map_secret"] = map[string][]byte{ - "conjur-map": []byte(""), +func assertSecretsUpdated(expK8sSecrets expectedK8sSecrets, + expMissingValues expectedMissingValues) assertFunc { + return func(t *testing.T, mocks testMocks, err error, desc string) { + assert.NoError(t, err, desc) + // Check that K8s Secrets contain expected Conjur secret values + for k8sSecretName, expSecretData := range expK8sSecrets { + actualSecretData := mocks.kubeClient.InspectSecret(k8sSecretName) + for secretName, expSecretValue := range expSecretData { + newDesc := desc + ", Secret: " + secretName + actualSecretValue := string(actualSecretData[secretName]) + assert.Equal(t, expSecretValue, actualSecretValue, newDesc) } - - requiredSecrets := []string{"empty_conjur_map_secret"} - - _, err := RetrieveRequiredK8sSecrets(kubeMockClient.RetrieveSecret, "someNameSpace", requiredSecrets) - - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, fmt.Sprintf(messages.CSPFK028E, "empty_conjur_map_secret")) - }) - }) - - Convey("Given a k8s secret with an invalid 'conjur-map'", func() { - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.Database["invalid_conjur_map_secret"] = map[string][]byte{ - "conjur-map": []byte("key_with_no_value"), + } + // Check for secret values leaking into the wrong K8s Secrets + for k8sSecretName, expMissingValue := range expMissingValues { + actualSecretData := mocks.kubeClient.InspectSecret(k8sSecretName) + for _, value := range actualSecretData { + actualValue := string(value) + newDesc := desc + ", Leaked secret value: " + actualValue + assert.NotEqual(t, expMissingValue, actualValue, newDesc) } + } + } +} - requiredSecrets := []string{"invalid_conjur_map_secret"} - - _, err := RetrieveRequiredK8sSecrets(kubeMockClient.RetrieveSecret, "someNameSpace", requiredSecrets) - - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, fmt.Sprintf(messages.CSPFK028E, "invalid_conjur_map_secret")) - }) - }) - }) - - Convey("UpdateRequiredK8sSecrets", t, func() { - Convey("Given no 'update' permissions on the 'secrets' k8s resource", func() { - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.CanUpdate = false - kubeMockClient.AddSecret("k8s_secret1", "secret_key1", "conjur_variable1") - requiredSecrets := []string{"k8s_secret1"} - - k8sSecretsMap, err := RetrieveRequiredK8sSecrets(kubeMockClient.RetrieveSecret, "someNameSpace", requiredSecrets) - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - err = UpdateRequiredK8sSecrets(kubeMockClient.UpdateSecret, "someNameSpace", k8sSecretsMap) - - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, messages.CSPFK022E) - }) - }) - }) - - Convey("run", t, func() { - Convey("Given 2 k8s secrets that only one is required by the pod", func() { - conjurMockClient := conjurMocks.NewConjurMockClient() - - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.AddSecret("k8s_secret1", "secret_key1", "conjur_variable1") - kubeMockClient.AddSecret("k8s_secret2", "secret_key2", "conjur_variable2") - requiredSecrets := []string{"k8s_secret1"} - - err := k8sProvider{ - retrieveK8sSecret: kubeMockClient.RetrieveSecret, - updateK8sSecret: kubeMockClient.UpdateSecret, - retrieveSecretsFunc: conjurMockClient.RetrieveSecrets, - podNamespace: "someNamespace", - requiredK8sSecrets: requiredSecrets, - }.Provide() - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Updates K8s secrets with their corresponding Conjur secrets", func() { - verifyK8sSecretValue(kubeMockClient, "k8s_secret1", "secret_key1", "conjur_secret1") - - }) - - Convey("Does not updates other K8s secrets", func() { - actualK8sSecretDataValue := kubeMockClient.Database["k8s_secret2"]["secretkkey1"] - So(actualK8sSecretDataValue, ShouldEqual, nil) - }) - }) - - Convey("Given 2 k8s secrets that are required by the pod - each one has its own Conjur secret", func() { - conjurMockClient := conjurMocks.NewConjurMockClient() - - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.AddSecret("k8s_secret1", "secret_key1", "conjur_variable1") - kubeMockClient.AddSecret("k8s_secret2", "secret_key2", "conjur_variable2") - requiredSecrets := []string{"k8s_secret1", "k8s_secret2"} - - err := k8sProvider{ - retrieveK8sSecret: kubeMockClient.RetrieveSecret, - updateK8sSecret: kubeMockClient.UpdateSecret, - retrieveSecretsFunc: conjurMockClient.RetrieveSecrets, - podNamespace: "someNamespace", - requiredK8sSecrets: requiredSecrets, - }.Provide() - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Updates K8s secrets with their corresponding Conjur secrets", func() { - verifyK8sSecretValue(kubeMockClient, "k8s_secret1", "secret_key1", "conjur_secret1") - verifyK8sSecretValue(kubeMockClient, "k8s_secret2", "secret_key2", "conjur_secret2") - }) - }) - - Convey("Given 2 k8s secrets that are required by the pod - both have the same Conjur secret", func() { - conjurMockClient := conjurMocks.NewConjurMockClient() - - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.AddSecret("k8s_secret1", "secret_key1", "conjur_variable1") - kubeMockClient.AddSecret("k8s_secret2", "secret_key2", "conjur_variable2") - - requiredSecrets := []string{"k8s_secret1", "k8s_secret2"} - - err := k8sProvider{ - retrieveK8sSecret: kubeMockClient.RetrieveSecret, - updateK8sSecret: kubeMockClient.UpdateSecret, - retrieveSecretsFunc: conjurMockClient.RetrieveSecrets, - podNamespace: "someNamespace", - requiredK8sSecrets: requiredSecrets, - }.Provide() - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Updates K8s secrets with their corresponding Conjur secrets", func() { - verifyK8sSecretValue(kubeMockClient, "k8s_secret1", "secret_key1", "conjur_secret1") - verifyK8sSecretValue(kubeMockClient, "k8s_secret2", "secret_key2", "conjur_secret2") - }) - }) - - Convey("Given a k8s secret which is mapped to a non-existing conjur variable", func() { - conjurMockClient := conjurMocks.NewConjurMockClient() - - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.AddSecret("k8s_secret1", "secret_key", "non_existing_conjur_variable") - - requiredSecrets := []string{"k8s_secret1"} - - err := k8sProvider{ - retrieveK8sSecret: kubeMockClient.RetrieveSecret, - updateK8sSecret: kubeMockClient.UpdateSecret, - retrieveSecretsFunc: conjurMockClient.RetrieveSecrets, - podNamespace: "someNamespace", - requiredK8sSecrets: requiredSecrets, - }.Provide() - - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, fmt.Sprintf(messages.CSPFK034E, "no_conjur_secret_error")) - }) - }) - - Convey("Given a k8s secret which is mapped to a conjur secret with an empty secret value", func() { - conjurMockClient := conjurMocks.NewConjurMockClient() - - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.AddSecret("k8s_secret_with_empty_conjur_variable", "secret_key", "conjur_variable_empty_secret") - requiredSecrets := []string{"k8s_secret_with_empty_conjur_variable"} - - err := k8sProvider{ - retrieveK8sSecret: kubeMockClient.RetrieveSecret, - updateK8sSecret: kubeMockClient.UpdateSecret, - retrieveSecretsFunc: conjurMockClient.RetrieveSecrets, - podNamespace: "someNamespace", - requiredK8sSecrets: requiredSecrets, - }.Provide() - - Convey("Finishes without raising an error", func() { - So(err, ShouldEqual, nil) - }) - - Convey("Updates K8s secrets with their corresponding Conjur secrets", func() { - verifyK8sSecretValue(kubeMockClient, "k8s_secret_with_empty_conjur_variable", "secret_key", "") - }) - }) - - Convey("Given no 'execute' permissions on the conjur secret", func() { - conjurMockClient := conjurMocks.NewConjurMockClient() - // no execute privileges on the conjur secret - conjurMockClient.CanExecute = false - - kubeMockClient := secretsStorageMocks.NewKubeSecretsMockClient() - kubeMockClient.AddSecret("k8s_secret_with_no_permission_conjur_variable", "secret_key", "no_execute_permission_conjur_secret") - requiredSecrets := []string{"k8s_secret_with_no_permission_conjur_variable"} - - err := k8sProvider{ - retrieveK8sSecret: kubeMockClient.RetrieveSecret, - updateK8sSecret: kubeMockClient.UpdateSecret, - retrieveSecretsFunc: conjurMockClient.RetrieveSecrets, - podNamespace: "someNamespace", - requiredK8sSecrets: requiredSecrets, - }.Provide() - - Convey("Raises proper error", func() { - So(err.Error(), ShouldEqual, fmt.Sprintf(messages.CSPFK034E, "custom error")) - }) - }) - }) +func assertErrorLogged(msg string, args ...interface{}) assertFunc { + return func(t *testing.T, mocks testMocks, err error, desc string) { + errStr := fmt.Sprintf(msg, args...) + newDesc := desc + ", error logged: " + errStr + assert.True(t, mocks.logger.ErrorWasLogged(errStr), newDesc) + } } -func verifyK8sSecretValue( - client secretsStorageMocks.KubeSecretsMockClient, - secretName string, - key string, - value string, -) { - actualK8sSecretDataValue := client.Database[secretName][key] - expectedK8sSecretDataValue := []byte(value) - So(actualK8sSecretDataValue, ShouldResemble, expectedK8sSecretDataValue) +func TestProvide(t *testing.T) { + testCases := []struct { + desc string + k8sSecrets k8sStorageMocks.K8sSecrets + requiredSecrets []string + denyConjurRetrieve bool + denyK8sRetrieve bool + denyK8sUpdate bool + asserts []assertFunc + }{ + { + desc: "Happy path, existing k8s Secret with existing Conjur secret", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {"secret1": "conjur/var/path1"}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + asserts: []assertFunc{ + assertSecretsUpdated( + expectedK8sSecrets{ + "k8s-secret1": {"secret1": "secret-value1"}, + }, + expectedMissingValues{}, + ), + }, + }, + { + desc: "Happy path, 2 existing k8s Secrets with 2 existing Conjur secrets", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": { + "secret1": "conjur/var/path1", + "secret2": "conjur/var/path2", + }, + }, + "k8s-secret2": { + "conjur-map": { + "secret3": "conjur/var/path3", + "secret4": "conjur/var/path4", + }, + }, + }, + requiredSecrets: []string{"k8s-secret1", "k8s-secret2"}, + asserts: []assertFunc{ + assertSecretsUpdated( + expectedK8sSecrets{ + "k8s-secret1": { + "secret1": "secret-value1", + "secret2": "secret-value2", + }, + "k8s-secret2": { + "secret3": "secret-value3", + "secret4": "secret-value4", + }, + }, + expectedMissingValues{ + "k8s-secret1": {"secret-value3", "secret-value4"}, + "k8s-secret2": {"secret-value1", "secret-value2"}, + }, + ), + }, + }, + { + desc: "Happy path, 2 k8s Secrets use the same Conjur secret with different names", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": { + "secret1": "conjur/var/path1", + "secret2": "conjur/var/path2", + }, + }, + "k8s-secret2": { + "conjur-map": { + "secret3": "conjur/var/path2", + "secret4": "conjur/var/path4", + }, + }, + }, + requiredSecrets: []string{"k8s-secret1", "k8s-secret2"}, + asserts: []assertFunc{ + assertSecretsUpdated( + expectedK8sSecrets{ + "k8s-secret1": { + "secret1": "secret-value1", + "secret2": "secret-value2", + }, + "k8s-secret2": { + "secret3": "secret-value2", + "secret4": "secret-value4", + }, + }, + expectedMissingValues{ + "k8s-secret1": {"secret-value4"}, + "k8s-secret2": {"secret-value1"}, + }, + ), + }, + }, + { + desc: "Happy path, 2 existing k8s Secrets but only 1 managed by SP", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": { + "secret1": "conjur/var/path1", + "secret2": "conjur/var/path2", + }, + }, + "k8s-secret2": { + "conjur-map": { + "secret2": "conjur/var/path2", + "secret3": "conjur/var/path3", + }, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + asserts: []assertFunc{ + assertSecretsUpdated( + expectedK8sSecrets{ + "k8s-secret1": { + "secret1": "secret-value1", + "secret2": "secret-value2", + }, + }, + expectedMissingValues{ + "k8s-secret1": {"secret-value3"}, + "k8s-secret2": {"secret-value1", "secret-value2", "secret-value3"}, + }, + ), + }, + }, + { + desc: "Happy path, k8s Secret maps to Conjur secret with null string value", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {"secret1": "conjur/var/empty-secret"}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + asserts: []assertFunc{ + assertSecretsUpdated( + expectedK8sSecrets{ + "k8s-secret1": {"secret1": ""}, + }, + expectedMissingValues{}, + ), + }, + }, + { + desc: "K8s Secrets maps to a non-existent Conjur secret", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {"secret1": "nonexistent/conjur/var"}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + denyK8sRetrieve: true, + asserts: []assertFunc{ + assertErrorLogged(messages.CSPFK020E), + assertErrorContains(messages.CSPFK021E), + }, + }, + { + desc: "Read access to K8s Secrets is not permitted", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {"secret1": "conjur/var/path1"}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + denyK8sRetrieve: true, + asserts: []assertFunc{ + assertErrorLogged(messages.CSPFK020E), + assertErrorContains(messages.CSPFK021E), + }, + }, + { + desc: "Access to Conjur secrets is not authorized", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {"secret1": "conjur/var/path1"}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + denyConjurRetrieve: true, + asserts: []assertFunc{ + assertErrorLogged(messages.CSPFK034E, "custom error"), + assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "custom error")), + }, + }, + { + desc: "Updates to K8s 'Secrets' are not permitted", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {"secret1": "conjur/var/path1"}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + denyK8sUpdate: true, + asserts: []assertFunc{ + assertErrorLogged(messages.CSPFK022E), + assertErrorContains(messages.CSPFK023E), + }, + }, + { + desc: "K8s secret is required but does not exist", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {"secret1": "conjur/var/path1"}, + }, + }, + requiredSecrets: []string{"non-existent-k8s-secret"}, + asserts: []assertFunc{ + assertErrorLogged(messages.CSPFK020E), + assertErrorContains(messages.CSPFK021E), + }, + }, + { + desc: "K8s secret has no 'conjur-map' entry", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "foobar": {"foo": "bar"}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + asserts: []assertFunc{ + assertErrorLogged(messages.CSPFK028E, "k8s-secret1"), + assertErrorContains(messages.CSPFK021E), + }, + }, + { + desc: "K8s secret has an empty 'conjur-map' entry", + k8sSecrets: k8sStorageMocks.K8sSecrets{ + "k8s-secret1": { + "conjur-map": {}, + }, + }, + requiredSecrets: []string{"k8s-secret1"}, + asserts: []assertFunc{ + assertErrorLogged(messages.CSPFK028E, "k8s-secret1"), + assertErrorContains(messages.CSPFK021E), + }, + }, + } + + for _, tc := range testCases { + // Set up test case + mocks := newTestMocks() + mocks.setPermissions(tc.denyConjurRetrieve, tc.denyK8sRetrieve, + tc.denyK8sUpdate) + for secretName, secretData := range tc.k8sSecrets { + mocks.kubeClient.AddSecret(secretName, secretData) + } + provider := mocks.newProvider(tc.requiredSecrets) + + // Run test case + err := provider.Provide() + + // Confirm results + for _, assert := range tc.asserts { + assert(t, mocks, err, tc.desc) + } + } } diff --git a/pkg/secrets/provide_conjur_secrets.go b/pkg/secrets/provide_conjur_secrets.go index 28cfe5f27..9a723ccac 100644 --- a/pkg/secrets/provide_conjur_secrets.go +++ b/pkg/secrets/provide_conjur_secrets.go @@ -9,42 +9,48 @@ import ( "github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/conjur" - "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/k8s" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/config" - "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/k8s_secrets_storage" + k8sSecretsStorage "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/k8s_secrets_storage" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/pushtofile" "github.com/cyberark/secrets-provider-for-k8s/pkg/utils" ) +// ProviderConfig provides the configuration necessary to create a secrets +// Provider. +type ProviderConfig struct { + // Config common to all providers + StoreType string + + // Config specific to Kubernetes Secrets provider + PodNamespace string + RequiredK8sSecrets []string + + // Config specific to Push to File provider + SecretFileBasePath string + AnnotationsMap map[string]string +} + // ProviderFunc describes a function type responsible for providing secrets to an unspecified target. type ProviderFunc func() error // NewProviderForType returns a ProviderFunc responsible for providing secrets in a given mode. func NewProviderForType( - retrievek8sSecret k8s.RetrieveK8sSecretFunc, - updatek8sSecret k8s.UpdateK8sSecretFunc, secretsRetrieverFunc conjur.RetrieveSecretsFunc, - storeType string, - podNamespace string, - requiredK8sSecrets []string, - secretsBasePath string, - annotations map[string]string, + providerConfig ProviderConfig, ) (ProviderFunc, []error) { - switch storeType { + switch providerConfig.StoreType { case config.K8s: - provider := k8s_secrets_storage.NewProvider( - retrievek8sSecret, - updatek8sSecret, + provider := k8sSecretsStorage.NewProvider( secretsRetrieverFunc, - requiredK8sSecrets, - podNamespace, + providerConfig.RequiredK8sSecrets, + providerConfig.PodNamespace, ) return provider.Provide, nil case config.File: provider, err := pushtofile.NewProvider( secretsRetrieverFunc, - secretsBasePath, - annotations, + providerConfig.SecretFileBasePath, + providerConfig.AnnotationsMap, ) if err != nil { return nil, err @@ -53,7 +59,7 @@ func NewProviderForType( default: return nil, []error{fmt.Errorf( messages.CSPFK054E, - storeType, + providerConfig.StoreType, )} } } diff --git a/pkg/secrets/pushtofile/retrieve_secrets_test.go b/pkg/secrets/pushtofile/retrieve_secrets_test.go index 4ae9d5047..d322b8a07 100644 --- a/pkg/secrets/pushtofile/retrieve_secrets_test.go +++ b/pkg/secrets/pushtofile/retrieve_secrets_test.go @@ -112,7 +112,7 @@ var retrieveSecretsTestCases = []retrieveSecretsTestCase{ } type mockSecretFetcher struct { - conjurMockClient conjurMocks.ConjurMockClient + conjurMockClient *conjurMocks.ConjurClient } func (s mockSecretFetcher) Fetch(secretPaths []string) (map[string][]byte, error) { @@ -121,7 +121,7 @@ func (s mockSecretFetcher) Fetch(secretPaths []string) (map[string][]byte, error func newMockSecretFetcher() mockSecretFetcher { m := mockSecretFetcher{ - conjurMockClient: conjurMocks.NewConjurMockClient(), + conjurMockClient: conjurMocks.NewConjurClient(), } m.conjurMockClient.AddSecrets(