Skip to content

Commit

Permalink
Set updated status when sanitizing secrets
Browse files Browse the repository at this point in the history
  • Loading branch information
@szh
szh committed Apr 5, 2022
1 parent 0c50a7d commit 0094384
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 28 deletions.
8 changes: 5 additions & 3 deletions pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,24 +157,26 @@ func (p K8sProvider) Provide() (bool, error) {
}

// Retrieve Conjur secrets for all K8s Secrets.
var updated bool
retrievedConjurSecrets, err := p.retrieveConjurSecrets(tr)
if err != nil {
// Delete K8s secret files for Conjur variables that no longer exist or the user no longer has permissions to.
// Delete K8s secrets for Conjur variables that no longer exist or the user no longer has permissions to.
// In the future we'll delete only the secrets that are revoked, but for now we delete all secrets in
// the group because we don't have a way to determine which secrets are revoked.
if (strings.Contains(err.Error(), "403") || strings.Contains(err.Error(), "404")) && p.sanitizeEnabled {
updated = true
rmErr := p.removeDeletedSecrets(tr)
if rmErr != nil {
p.log.recordedError(messages.CSPFK063E)
// Don't return here - continue processing
}
}

return false, p.log.recordedError(messages.CSPFK034E, err.Error())
return updated, p.log.recordedError(messages.CSPFK034E, err.Error())
}

// Update all K8s Secrets with the retrieved Conjur secrets.
updated, err := p.updateRequiredK8sSecrets(retrievedConjurSecrets, tr)
updated, err = p.updateRequiredK8sSecrets(retrievedConjurSecrets, tr)
if err != nil {
return updated, p.log.recordedError(messages.CSPFK023E)
}
Expand Down
34 changes: 16 additions & 18 deletions pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,13 @@ type assertFunc func(*testing.T, testMocks, bool, error, string)
type expectedK8sSecrets map[string]map[string]string
type expectedMissingValues map[string][]string

func assertErrorContains(expErrStr string) assertFunc {
func assertErrorContains(expErrStr string, expectUpdated bool) assertFunc {
return func(t *testing.T, _ testMocks,
updated bool, err error, desc string) {

assert.Error(t, err)
assert.False(t, updated)
assert.Contains(t, err.Error(), expErrStr)
assert.Error(t, err, desc)
assert.Contains(t, err.Error(), expErrStr, desc)
assert.Equal(t, expectUpdated, updated, desc)
}
}

Expand All @@ -100,10 +100,9 @@ func assertSecretsUpdated(expK8sSecrets expectedK8sSecrets,

if expectError {
assert.Error(t, err, desc)
assert.False(t, updated)
} else {
assert.NoError(t, err, desc)
assert.True(t, updated)
assert.True(t, updated, desc)
}

// Check that K8s Secrets contain expected Conjur secret values
Expand Down Expand Up @@ -132,7 +131,6 @@ func assertErrorLogged(msg string, args ...interface{}) assertFunc {
errStr := fmt.Sprintf(msg, args...)
newDesc := desc + ", error logged: " + errStr
assert.True(t, mocks.logger.ErrorWasLogged(errStr), newDesc)
assert.False(t, updated)
}
}

Expand Down Expand Up @@ -300,7 +298,7 @@ func TestProvide(t *testing.T) {
denyK8sRetrieve: true,
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK020E),
assertErrorContains(messages.CSPFK021E),
assertErrorContains(messages.CSPFK021E, false),
},
},
{
Expand All @@ -314,7 +312,7 @@ func TestProvide(t *testing.T) {
denyK8sRetrieve: true,
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK020E),
assertErrorContains(messages.CSPFK021E),
assertErrorContains(messages.CSPFK021E, false),
},
},
{
Expand All @@ -328,7 +326,7 @@ func TestProvide(t *testing.T) {
denyConjurRetrieve: true,
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK034E, "custom error"),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "custom error")),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "custom error"), false),
},
},
{
Expand All @@ -342,7 +340,7 @@ func TestProvide(t *testing.T) {
denyK8sUpdate: true,
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK022E),
assertErrorContains(messages.CSPFK023E),
assertErrorContains(messages.CSPFK023E, false),
},
},
{
Expand All @@ -355,7 +353,7 @@ func TestProvide(t *testing.T) {
requiredSecrets: []string{"non-existent-k8s-secret"},
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK020E),
assertErrorContains(messages.CSPFK021E),
assertErrorContains(messages.CSPFK021E, false),
},
},
{
Expand All @@ -368,7 +366,7 @@ func TestProvide(t *testing.T) {
requiredSecrets: []string{"k8s-secret1"},
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK028E, "k8s-secret1"),
assertErrorContains(messages.CSPFK021E),
assertErrorContains(messages.CSPFK021E, false),
},
},
{
Expand All @@ -381,7 +379,7 @@ func TestProvide(t *testing.T) {
requiredSecrets: []string{"k8s-secret1"},
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK028E, "k8s-secret1"),
assertErrorContains(messages.CSPFK021E),
assertErrorContains(messages.CSPFK021E, false),
},
},
}
Expand Down Expand Up @@ -520,7 +518,7 @@ func TestProvideSanitization(t *testing.T) {
retrieveErrorMsg: "403",
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK034E, "403"),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "403")),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "403"), true),
assertSecretsUpdated(
expectedK8sSecrets{
"k8s-secret1": {"secret1": ""},
Expand All @@ -542,7 +540,7 @@ func TestProvideSanitization(t *testing.T) {
retrieveErrorMsg: "404",
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK034E, "404"),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "404")),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "404"), true),
assertSecretsUpdated(
expectedK8sSecrets{
"k8s-secret1": {"secret1": ""},
Expand All @@ -564,7 +562,7 @@ func TestProvideSanitization(t *testing.T) {
retrieveErrorMsg: "generic error",
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK034E, "generic error"),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "generic error")),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "generic error"), false),
assertSecretsUpdated(
expectedK8sSecrets{
"k8s-secret1": {"secret1": "secret-value1"},
Expand All @@ -586,7 +584,7 @@ func TestProvideSanitization(t *testing.T) {
retrieveErrorMsg: "403",
asserts: []assertFunc{
assertErrorLogged(messages.CSPFK034E, "403"),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "403")),
assertErrorContains(fmt.Sprintf(messages.CSPFK034E, "403"), false),
assertSecretsUpdated(
expectedK8sSecrets{
"k8s-secret1": {"secret1": "secret-value1"},
Expand Down
9 changes: 6 additions & 3 deletions pkg/secrets/pushtofile/provide_conjur_secrets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,14 @@ func provideWithDeps(
// Use the global TracerProvider
tr := trace.NewOtelTracer(otel.Tracer("secrets-provider"))
spanCtx, span := tr.Start(traceContext, "Fetch Conjur Secrets")
var updated bool
secretsByGroup, err := FetchSecretsForGroups(depFuncs.retrieveSecretsFunc, groups, spanCtx)
if err != nil {
// Delete secret files for variables that no longer exist or the user no longer has permissions to
// Delete secret files for variables that no longer exist or the user no longer has permissions to.
// In the future we'll delete only the secrets that are revoked, but for now we delete all secrets in
// the group because we don't have a way to determine which secrets are revoked.
if (strings.Contains(err.Error(), "403") || strings.Contains(err.Error(), "404")) && sanitizeEnabled {
updated = true
for _, group := range groups {
log.Info(messages.CSPFK019I)
rmErr := os.Remove(group.FilePath)
Expand All @@ -95,13 +99,12 @@ func provideWithDeps(

span.RecordErrorAndSetStatus(err)
span.End()
return false, err
return updated, err
}
span.End()

spanCtx, span = tr.Start(traceContext, "Write Secret Files")
defer span.End()
var updated bool
for _, group := range groups {
_, childSpan := tr.Start(spanCtx, "Write Secret Files for group")
defer childSpan.End()
Expand Down
18 changes: 14 additions & 4 deletions pkg/secrets/pushtofile/provide_conjur_secrets_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func TestProvideWithDeps(t *testing.T) {
provider fileProvider
createFileName string
sanitizeEnabled bool
assert func(*testing.T, fileProvider, error, *ClosableBuffer, pushToWriterSpy, openWriteCloserSpy)
assert func(*testing.T, fileProvider, bool, error, *ClosableBuffer, pushToWriterSpy, openWriteCloserSpy)
}{
{
description: "happy path",
Expand All @@ -110,11 +110,13 @@ func TestProvideWithDeps(t *testing.T) {
assert: func(
t *testing.T,
p fileProvider,
updated bool,
err error,
closableBuf *ClosableBuffer,
spyPushToWriter pushToWriterSpy,
spyOpenWriteCloser openWriteCloserSpy,
) {
assert.True(t, updated)
assert.Equal(t, closableBuf, spyPushToWriter.args.writer)
assert.Equal(t, spyOpenWriteCloser.args.path, p.secretGroups[0].FilePath)
assert.Nil(t, err)
Expand All @@ -131,11 +133,13 @@ func TestProvideWithDeps(t *testing.T) {
assert: func(
t *testing.T,
p fileProvider,
updated bool,
err error,
closableBuf *ClosableBuffer,
spyPushToWriter pushToWriterSpy,
spyOpenWriteCloser openWriteCloserSpy,
) {
assert.True(t, updated)
assert.Error(t, err)
// File should be deleted because of 403 error
assert.NoFileExists(t, "path_to_file.yaml")
Expand All @@ -152,11 +156,13 @@ func TestProvideWithDeps(t *testing.T) {
assert: func(
t *testing.T,
p fileProvider,
updated bool,
err error,
closableBuf *ClosableBuffer,
spyPushToWriter pushToWriterSpy,
spyOpenWriteCloser openWriteCloserSpy,
) {
assert.False(t, updated)
assert.Error(t, err)
// File should not be deleted because of generic error
assert.FileExists(t, "path_to_file.yaml")
Expand All @@ -173,11 +179,13 @@ func TestProvideWithDeps(t *testing.T) {
assert: func(
t *testing.T,
p fileProvider,
updated bool,
err error,
closableBuf *ClosableBuffer,
spyPushToWriter pushToWriterSpy,
spyOpenWriteCloser openWriteCloserSpy,
) {
assert.False(t, updated)
assert.Error(t, err)
// File shouldn't be deleted because sanitize is disabled
assert.FileExists(t, "path_to_file.yaml")
Expand All @@ -189,7 +197,9 @@ func TestProvideWithDeps(t *testing.T) {
t.Run(tc.description, func(t *testing.T) {
// Setup mocks
closableBuf := new(ClosableBuffer)
spyPushToWriter := pushToWriterSpy{}
spyPushToWriter := pushToWriterSpy{
targetsUpdated: true,
}
spyOpenWriteCloser := openWriteCloserSpy{
writeCloser: closableBuf,
}
Expand All @@ -200,7 +210,7 @@ func TestProvideWithDeps(t *testing.T) {
defer os.Remove(tc.createFileName)
}

_, err := provideWithDeps(
updated, err := provideWithDeps(
context.Background(),
tc.provider.secretGroups,
tc.sanitizeEnabled,
Expand All @@ -211,7 +221,7 @@ func TestProvideWithDeps(t *testing.T) {
},
)

tc.assert(t, tc.provider, err, closableBuf, spyPushToWriter, spyOpenWriteCloser)
tc.assert(t, tc.provider, updated, err, closableBuf, spyPushToWriter, spyOpenWriteCloser)
})
}
}

0 comments on commit 0094384

Please sign in to comment.