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.

Sign up for GitHub

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

Add startup logic to enable Push-to-File #377

Merged
merged 9 commits into from
Oct 29, 2021
Merged

Add startup logic to enable Push-to-File #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5ba1b31
Relegate AccessToken creation to Conjur package
john-odonnell Oct 21, 2021
112fb53
Create ProviderFunc function type
john-odonnell Oct 21, 2021
4c814a9
Use new ambiguous ProviderFunc in main
john-odonnell Oct 21, 2021
fa6ee2d
Fix K8s Secrets mode
john-odonnell Oct 22, 2021
e4f9f9d
Fix Retry Interval assignment
john-odonnell Oct 22, 2021
3409843
Add RetryableSecretProvider unit tests
john-odonnell Oct 22, 2021
61a55ae
Add unit testing for Push-to-File provider
john-odonnell Oct 22, 2021
f1818dc
PR Review Fixes
john-odonnell Oct 26, 2021
bc9fffa
Fix CodeClimate errors
john-odonnell Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 28 additions & 66 deletions cmd/secrets-provider/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,21 @@ import (
"os"
"time"

"github.com/cenkalti/backoff"
"github.com/cyberark/conjur-authn-k8s-client/pkg/access_token/memory"
"github.com/cyberark/conjur-authn-k8s-client/pkg/authenticator"
authnConfigProvider "github.com/cyberark/conjur-authn-k8s-client/pkg/authenticator/config"
"github.com/cyberark/conjur-authn-k8s-client/pkg/log"

"github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages"
"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"
"github.com/cyberark/secrets-provider-for-k8s/pkg/utils"
)

const (
annotationsFile = "/conjur/podinfo/annotations"
defaultContainerMode = "init"
annotationsFilePath = "/conjur/podinfo/annotations"
secretBasePath = "/conjur/secrets"
)

var annotationsMap map[string]string
Expand All @@ -37,15 +36,13 @@ var envAnnotationsConversion = map[string]string{
}

func main() {
var err error

log.Info(messages.CSPFK008I, secrets.FullVersionName)

// Only attempt to populate from annotations if the annotations file exists
// TODO: Figure out strategy for dealing with explicit annotation file path
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the annotation file path be anything other than /conjur/podinfo ? I don't think it makes sense that the file path would be user configurable...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is actually from #367 by @doodlesbykumbi. GH did a weird auto-rebase on my branch, but I've corrected it since.

I think I would agree with you here, though. The location of the annotations file seems like an internal detail rather than something the user should be concerned with. I feel differently about a configurable secrets file base path, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both of the fixed paths /conjur/podinfo and the /conjur/secrets are from the perspective of the Secrets Provider container only. For the application container, the user can mount the corresponding volumes at any arbitrary mountPaths (though they might not care about the podinfo volume).

We intentionally left out "config knobs" for these SP mountPaths to simplify the user experience and so that we have two less things that can get misconfigured.

With that in mind, we can probably change defaultAnnotationsFile back to annotationsFile, and defaultSecretBasePath to secretBasePath.

Copy link
Contributor

Choose a reason for hiding this comment

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

My motivation for making these values configurable is for

  1. For testing purposes we should be able to specify arbitrary mouth paths.
  2. Potentially supporting dry run capabilities. I acknowledge this as a nice-to-have, and something that might require a separate dedicated discussion.

We don't have to expose these as config knobs to the user, but I think the hardcoded value should only be present in ./main.go.
I opted for defaultAnnotationsFile vs annotationsFile to specifically call out the fact that this is the default, but perhaps that causes some confusion due to the word "default" implying that there are other options. Perhaps a better name would be fixedAnnotationsFilePath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to annotationsFilePath and secretBasePath. When they're made "configurable" in the context of testing/dry run, then we can change it to something more suitable.

// set by user. In that case we can't just ignore that the file is missing.
if _, err := os.Stat(annotationsFile); err == nil {
annotationsMap, err = annotations.NewAnnotationsFromFile(annotationsFile)
// set by user. In that case we can't just ignore that the file is missing.
if _, err := os.Stat(annotationsFilePath); err == nil {
annotationsMap, err = annotations.NewAnnotationsFromFile(annotationsFilePath)
if err != nil {
printErrorAndExit(messages.CSPFK040E)
}
Expand All @@ -54,50 +51,37 @@ func main() {
logErrorsAndConditionalExit(errLogs, infoLogs, messages.CSPFK049E)
}

// Initialize Authenticator configuration
// Initialize Authenticator and Secrets Provider configurations
authnConfig := setupAuthnConfig()
validateContainerMode(authnConfig.ContainerMode)

// Initialize Secrets Provider configuration
secretsConfig := setupSecretsConfig()

provideConjurSecrets, err := secrets.GetProvideConjurSecretFunc(secretsConfig.StoreType)
// Initialize a Conjur Secret Fetcher
secretRetriever, err := conjur.NewConjurSecretRetriever(*authnConfig)
if err != nil {
printErrorAndExit(fmt.Sprintf(messages.CSPFK014E, err.Error()))
printErrorAndExit(err.Error())
}

accessToken, err := memory.NewAccessToken()
if err != nil {
printErrorAndExit(messages.CSPFK001E)
}

authn, err := authenticator.NewWithAccessToken(*authnConfig, accessToken)
if err != nil {
printErrorAndExit(messages.CSPFK009E)
}

limitedBackOff := utils.NewLimitedBackOff(
provideSecrets, errs := secrets.NewProviderForType(
k8s.RetrieveK8sSecret,
k8s.UpdateK8sSecret,
secretRetriever.Retrieve,
secretsConfig.StoreType,
secretsConfig.PodNamespace,
secretsConfig.RequiredK8sSecrets,
annotationsMap,
)
logErrorsAndConditionalExit(errs, nil, messages.CSPFK053E)

provideSecrets = secrets.RetryableSecretProvider(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I like the RetryableSecretProvider(...) wrapper!

time.Duration(secretsConfig.RetryIntervalSec)*time.Second,
secretsConfig.RetryCountLimit)

err = backoff.Retry(func() error {
if limitedBackOff.RetryCount() > 0 {
log.Info(fmt.Sprintf(messages.CSPFK010I, limitedBackOff.RetryCount(), limitedBackOff.RetryLimit))
}

return provideSecretsToTarget(authn, provideConjurSecrets, accessToken, secretsConfig)
}, limitedBackOff)
secretsConfig.RetryCountLimit,
provideSecrets,
)

err = provideSecrets()
if err != nil {
log.Error(messages.CSPFK038E)

// Deleting the retrieved Conjur access token in case we got an error after retrieval.
// if the access token is already deleted the action should not fail
err = accessToken.Delete()
if err != nil {
log.Error(messages.CSPFK003E, err)
}
printErrorAndExit(messages.CSPFK039E)
printErrorAndExit(fmt.Sprintf(messages.CSPFK039E, secretsConfig.StoreType))
}
}

Expand Down Expand Up @@ -140,28 +124,6 @@ func setupSecretsConfig() *secretsConfigProvider.Config {
return secretsConfigProvider.NewConfig(secretsProviderSettings)
}

func provideSecretsToTarget(authn *authenticator.Authenticator, provideConjurSecrets secrets.ProvideConjurSecrets,
accessToken *memory.AccessToken, secretsConfig *secretsConfigProvider.Config) error {
log.Info(fmt.Sprintf(messages.CSPFK001I, authn.Config.Username))
err := authn.Authenticate()
if err != nil {
return log.RecordedError(messages.CSPFK010E)
}

err = provideConjurSecrets(accessToken, secretsConfig)
if err != nil {
return log.RecordedError(messages.CSPFK016E)
}

err = accessToken.Delete()
if err != nil {
return log.RecordedError(messages.CSPFK003E, err.Error())
}

log.Info(messages.CSPFK009I)
return nil
}

func printErrorAndExit(errorMessage string) {
log.Error(errorMessage)
os.Exit(1)
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ github.com/onsi/gomega v1.10.1 h1:o0+MgICZLuZ7xjH7Vx6zS/zcu93/BEp1VwkIW1mEXCE=
github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/prometheus/common v0.32.0 h1:HRmM4uANZDAjdvbsdfOoqI5UDbjz0faKeMs/cGPKKI0=
github.com/prometheus/common v0.32.1 h1:hWIdL3N2HoUx3B8j3YN9mWor0qhY/NlEKZEaXxuIRh4=
github.com/sirupsen/logrus v1.0.5 h1:8c8b5uO0zS4X6RPl/sd1ENwSkIc0/H2PaHxE3udaE8I=
github.com/sirupsen/logrus v1.0.5/go.mod h1:pMByvHTf9Beacp5x1UXfOR9xyW/9antXMhjMPG0dEzc=
github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
Expand Down
5 changes: 4 additions & 1 deletion pkg/log/messages/error_messages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const CSPFK037E string = "CSPFK037E Failed to parse DAP/Conjur variable IDs"

// General
const CSPFK038E string = "CSPFK038E Retransmission backoff exhausted"
const CSPFK039E string = "CSPFK039E Secrets Provider for Kubernetes failed to update Kubernetes Secrets"
const CSPFK039E string = "CSPFK039E Secrets Provider for Kubernetes failed to update secrets in %s mode"

// Annotations
const CSPFK040E string = "CSPFK040E Failed to parse annotations file"
Expand All @@ -74,3 +74,6 @@ const CSPFK049E string = "CSPFK049E Failed to validate Pod annotations"
const CSPFK050E string = "CSPFK050E Failed to unmarshal Push-to-File secrets. Reason: %s"
const CSPFK051E string = "CSPFK051E Unknown file format '%s'"
const CSPFK052E string = "CSPFK052E Failed to retrieve secrets. Reason: %s"

const CSPFK053E string = "CSPFK053E Unable to initialize Secrets Provider: unable to create secret group collection"
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh-oh, I used the same CSPFK053E error code. Looks like we're going to need to arm wrestle for it.

Copy link
Contributor Author

@john-odonnell john-odonnell Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Branching thought off of this...

This file has all error messages numbered sequentially, but it also has them loosely grouped by area of concern, separated by comments, ie. // General, // Push to File, etc.

I've added CSPFK054E, which arises when SP fails for a bad StoreType - which is more of a // General failure. Moving this error to the // General section would require changing the identifiers for every error message declared afterwords. Updating the numbering style might help the error messages to be more extensible and static through future changes.

I think if the messages here are to be grouped by area of concern, then maybe they should be numbered within their area of concern:

const CSPFK-A001E string = "first message concerned with aspect A"
const CSPFK-A002E string = "second message concerned with aspect A"

const CSPFK-B001E string = "first message concerned with aspect B"

This also might help to avoid collisions like this one! (EDIT: not this one exactly, because we're both adding to the same concern group w/ 053E, but similar cases)

const CSPFK054E string = "CSPFK054E Unable to initialize Secrets Provider: unrecognized Store Type '%s'"
1 change: 1 addition & 0 deletions pkg/log/messages/info_messages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ const CSPFK011I string = "CSPFK011I Annotation '%s' valid, but not recognized"
const CSPFK012I string = "CSPFK012I Secrets Provider setting '%s' set by both environment variable '%s' and annotation '%s'"
const CSPFK013I string = "CSPFK013I Gathering settings for Authenticator client configuration..."
const CSPFK014I string = "CSPFK014I Authenticator setting %s provided by %s"
const CSPFK015I string = "CSPFK015I DAP/Conjur Secrets pushed to shared volume successfully"
51 changes: 49 additions & 2 deletions pkg/secrets/clients/conjur/conjur_secrets_retriever.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,63 @@
package conjur

import (
"fmt"
"strings"

"github.com/cyberark/conjur-authn-k8s-client/pkg/access_token/memory"
"github.com/cyberark/conjur-authn-k8s-client/pkg/authenticator"
"github.com/cyberark/conjur-authn-k8s-client/pkg/authenticator/config"
"github.com/cyberark/conjur-authn-k8s-client/pkg/log"

"github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages"
)

type RetrieveConjurSecretsFunc func(accessToken []byte, variableIDs []string) (map[string][]byte, error)
type conjurSecretRetriever struct {
authn *authenticator.Authenticator
}

// RetrieveSecretsFunc defines a function type for retrieving secrets.
type RetrieveSecretsFunc func(variableIDs []string) (map[string][]byte, error)

// NewConjurSecretRetriever creates a new conjurSecretRetriever and Authenticator
// given an authenticator config.
func NewConjurSecretRetriever(authnConfig config.Config) (*conjurSecretRetriever, error) {
accessToken, err := memory.NewAccessToken()
if err != nil {
return nil, fmt.Errorf("%s", messages.CSPFK001E)
}

authn, err := authenticator.NewWithAccessToken(authnConfig, accessToken)
if err != nil {
return nil, fmt.Errorf("%s", messages.CSPFK009E)
}

return &conjurSecretRetriever{
authn: authn,
}, nil
}

// Retrieve implements a RetrieveSecretsFunc for a given conjurSecretRetriever.
// Authenticates the client, and retrieves a given batch of variables from Conjur.
func (retriever conjurSecretRetriever) Retrieve(variableIDs []string) (map[string][]byte, error) {
authn := retriever.authn

err := authn.Authenticate()
if err != nil {
return nil, log.RecordedError(messages.CSPFK010E)
}

accessTokenData, err := authn.AccessToken.Read()
if err != nil {
return nil, log.RecordedError(messages.CSPFK002E)
}
// Always delete the access token. The deletion is idempotent and never fails
defer authn.AccessToken.Delete()

return retrieveConjurSecrets(accessTokenData, variableIDs)
}

func RetrieveConjurSecrets(accessToken []byte, variableIDs []string) (map[string][]byte, error) {
func retrieveConjurSecrets(accessToken []byte, variableIDs []string) (map[string][]byte, error) {
log.Info(messages.CSPFK003I, variableIDs)

conjurClient, err := NewConjurClient(accessToken)
Expand Down
8 changes: 4 additions & 4 deletions pkg/secrets/clients/conjur/mocks/conjur_secrets_retriever.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ type ConjurMockClient 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
Database map[string]string
}

func (c ConjurMockClient) RetrieveSecrets (accessToken []byte, secretIds []string) (map[string][]byte, error) {
func (c ConjurMockClient) RetrieveSecrets(secretIds []string) (map[string][]byte, error) {
res := make(map[string][]byte)

if !c.CanExecute {
Expand All @@ -40,8 +40,8 @@ func (c ConjurMockClient) RetrieveSecrets (accessToken []byte, secretIds []strin

func NewConjurMockClient() ConjurMockClient {
database := map[string]string{
"conjur_variable1": "conjur_secret1",
"conjur_variable2": "conjur_secret2",
"conjur_variable1": "conjur_secret1",
"conjur_variable2": "conjur_secret2",
"conjur_variable_empty_secret": "",
}

Expand Down
53 changes: 29 additions & 24 deletions pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"
"strings"

"github.com/cyberark/conjur-authn-k8s-client/pkg/access_token"
"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
"gopkg.in/yaml.v2"
v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -32,40 +31,45 @@ type K8sSecretsMap struct {
PathMap map[string][]string
}

/*
This method is implemented for implementing the ProvideConjurSecrets interface. All that is done here is to
initialize a K8sSecretsClient and use the internal `run` method.
That method receives different structs as inputs so they can be mocked.
*/
func ProvideConjurSecretsToK8sSecrets(AccessToken access_token.AccessToken, config *config.Config) error {
return run(
k8s.RetrieveK8sSecret,
k8s.UpdateK8sSecret,
config.PodNamespace,
config.RequiredK8sSecrets,
AccessToken,
conjur.RetrieveConjurSecrets,
)
type k8sProvider struct {
retrieveK8sSecret k8s.RetrieveK8sSecretFunc
updateK8sSecret k8s.UpdateK8sSecretFunc
retrieveSecretsFunc conjur.RetrieveSecretsFunc
podNamespace string
requiredK8sSecrets []string
}

func run(retrieveSecretFunc k8s.RetrieveK8sSecretFunc, updateSecretFunc k8s.UpdateK8sSecretFunc, namespace string, requiredK8sSecrets []string, accessToken access_token.AccessToken, retrieveConjurSecretsFunc conjur.RetrieveConjurSecretsFunc) error {
k8sSecretsMap, err := RetrieveRequiredK8sSecrets(retrieveSecretFunc, namespace, requiredK8sSecrets)

if err != nil {
return log.RecordedError(messages.CSPFK021E)
// NewProvider creates a new secret provider for K8s Secrets mode.
func NewProvider(
retrievek8sSecret k8s.RetrieveK8sSecretFunc,
Copy link

Choose a reason for hiding this comment

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

Function NewProvider has 5 arguments (exceeds 4 allowed). Consider refactoring.

updatek8sSecret k8s.UpdateK8sSecretFunc,
retrieveSecretsFunc conjur.RetrieveSecretsFunc,
requiredK8sSecrets []string,
podNamespace string,
) k8sProvider {
return k8sProvider{
retrieveK8sSecret: retrievek8sSecret,
updateK8sSecret: updatek8sSecret,
retrieveSecretsFunc: retrieveSecretsFunc,
podNamespace: podNamespace,
requiredK8sSecrets: requiredK8sSecrets,
}
}

// Provide implements a ProviderFunc to retrieve and push secrets to K8s secrets.
func (p k8sProvider) Provide() error {
Copy link

Choose a reason for hiding this comment

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

Method k8sProvider.Provide has 6 return statements (exceeds 4 allowed).

k8sSecretsMap, err := RetrieveRequiredK8sSecrets(p.retrieveK8sSecret, p.podNamespace, p.requiredK8sSecrets)

accessTokenData, err := accessToken.Read()
if err != nil {
return log.RecordedError(messages.CSPFK002E)
return log.RecordedError(messages.CSPFK021E)
}

variableIDs, err := getVariableIDsToRetrieve(k8sSecretsMap.PathMap)
if err != nil {
return log.RecordedError(messages.CSPFK037E)
}

retrievedConjurSecrets, err := retrieveConjurSecretsFunc(accessTokenData, variableIDs)
retrievedConjurSecrets, err := p.retrieveSecretsFunc(variableIDs)
if err != nil {
return log.RecordedError(messages.CSPFK034E, err.Error())
}
Expand All @@ -75,12 +79,13 @@ func run(retrieveSecretFunc k8s.RetrieveK8sSecretFunc, updateSecretFunc k8s.Upda
return log.RecordedError(messages.CSPFK027E)
}

err = UpdateRequiredK8sSecrets(updateSecretFunc, namespace, k8sSecretsMap)
err = UpdateRequiredK8sSecrets(p.updateK8sSecret, p.podNamespace, k8sSecretsMap)

if err != nil {
return log.RecordedError(messages.CSPFK023E)
}

log.Info(messages.CSPFK009I)
return nil
}

Expand Down
Loading