Implement detection of changes in K8s Secret content with UT

CHANGELOG.md

@@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   [cyberark/secrets-provider-for-k8s#447](https://github.com/cyberark/secrets-provider-for-k8s/pull/447)
 - Secrets Provider allows for its status to be monitored through the creation of a couple of empty sentinel files: `CONJUR_SECRETS_PROVIDED` and `CONJUR_SECRETS_UPDATED`. The first file is created when SP has completed its first round of providing secrets via secret files / Kubernetes Secrets. It creates/recreates the second file whenever it has updated secret files / Kubernetes Secrets. If desirable, application containers can mount these files via a shared volume.
   [cyberark/secrets-provider-for-k8s#450](https://github.com/cyberark/secrets-provider-for-k8s/pull/450)
+- Kubernetes Secrets are only updated when there are changes when secrets rotation is enabled.
+  [cyberark/secrets-provider-for-k8s#448](https://github.com/cyberark/secrets-provider-for-k8s/pull/448)
 
 ## [1.4.0] - 2022-02-15
 

ROTATION.md

@@ -71,7 +71,13 @@ There are two new annotations introduced and one annotation is updated.
 Prerequisites:
 
 Requires secrets-provider-for-k8s v1.4.0 or later.
+
+<details><summary>For Push to File mode</summary>
 Follow the procedure to set up Secrets Provider for [Push to File](PUSH_TO_FILE.md#set-up-secrets-provider-for-push-to-file)
+</details>
+<details><summary>For Kubernetes Secrets mode</summary>
+Follow the procedure to set up [Kubernetes Secrets](https://docs.cyberark.com/Product-Doc/OnlineHelp/AAM-DAP/Latest/en/Content/Integrations/k8s-ocp/cjr-k8s-secrets-provider-ic.htm?tocpath=Integrations%7COpenShift%252FKubernetes%7CSet%20up%20applications%7CSecrets%20Provider%20for%20Kubernetes%7CInit%20container%7C_____1#SetupSecretsProviderasaninitcontainer)
+</details>
 
 Modify the Kubernetes manifest
 1. Change the Secrets provider container to be a sidecar. If it was configured
@@ -135,7 +141,5 @@ This feature is a **Community** level project that is still under development.
 There could be changes to the documentation so please check back often for updates and additions.
 Future enhancements to this feature will include:
 
-- Support for secrets rotation of Kubernetes secrets
 - atomic writes for multiple Conjur secret values
 - reporting of multiple errored secret variables for bulk Conjur secret retrieval and selective deletion of secret values from files
-- atomic write of secret files

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/cyberark/conjur-opentelemetry-tracer v0.0.1-58
 	github.com/stretchr/testify v1.7.0
 	go.opentelemetry.io/otel v1.3.0
+	gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f
 	gopkg.in/yaml.v2 v2.4.0
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
 	k8s.io/api v0.23.3
@@ -28,8 +29,10 @@ require (
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/googleapis/gnostic v0.5.5 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
+	github.com/kr/text v0.2.0 // indirect
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect
+	github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/sirupsen/logrus v1.8.1 // indirect
 	go.opentelemetry.io/otel/exporters/jaeger v1.3.0 // indirect

pkg/log/messages/info_messages.go

@@ -31,3 +31,4 @@ const CSPFK016I string = "CSPFK016I There are no secrets to be retrieved from Co
 const CSPFK017I string = "CSPFK017I Creating default file name for secret group '%s'"
 const CSPFK018I string = "CSPFK018I No change in secret file, no secret files written"
 const CSPFK019I string = "CSPFK019I Error fetching secrets, deleting secrets file"
+const CSPFK020I string = "CSPFK020I No change in Kubernetes secret, no secrets updated"

pkg/secrets/k8s_secrets_storage/mocks/logger.go

@@ -43,6 +43,11 @@ func (l *Logger) Info(msg string, args ...interface{}) {
 	l.infos = append(l.infos, fmt.Sprintf(msg, args...))
 }
 
+// ClearInfo Clears the info messages
+func (l *Logger) ClearInfo() {
+	l.infos = nil
+}
+
 // Debug logs a debug message.
 func (l *Logger) Debug(msg string, args ...interface{}) {
 	l.debugs = append(l.debugs, fmt.Sprintf(msg, args...))

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go

@@ -1,7 +1,9 @@
 package k8ssecretsstorage
 
 import (
+	"bytes"
 	"context"
+	"fmt"
 
 	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
 	"go.opentelemetry.io/otel"
@@ -13,6 +15,7 @@ import (
 	"github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/conjur"
 	k8sClient "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/utils"
 )
 
 // Secrets that have been retrieved from Conjur may need to be updated in
@@ -30,7 +33,7 @@ type k8sSecretsState struct {
 	// from K8s.
 	originalK8sSecrets map[string]*v1.Secret
 
-	// Maps a Conjur variable ID (policy path) to all of the updateDestination
+	// Maps a Conjur variable ID (policy path) to all the updateDestination
 	// targets which will need to be updated with the corresponding Conjur
 	// secret value after it has been retrieved.
 	updateDestinations map[string][]updateDestination
@@ -76,6 +79,10 @@ type K8sProvider struct {
 	requiredK8sSecrets []string
 	secretsState       k8sSecretsState
 	traceContext       context.Context
+	//prevSecretsChecksums maps a k8s secret name to a sha256 checksum of the
+	// corresponding secret content. This is used to detect changes in
+	// secret content.
+	prevSecretsChecksums map[string]utils.Checksum
 }
 
 // K8sProviderConfig provides config specific to Kubernetes Secrets provider
@@ -132,6 +139,7 @@ func newProvider(
 			updateDestinations: map[string][]updateDestination{},
 		},
 		traceContext: traceContext,
+		prevSecretsChecksums: map[string]utils.Checksum{},
 	}
 }
 
@@ -294,19 +302,35 @@ func (p K8sProvider) updateRequiredK8sSecrets(
 	for k8sSecretName, secretData := range newSecretData {
 		_, childSpan := tracer.Start(spanCtx, "Update K8s Secret")
 		defer childSpan.End()
-
-		err := p.k8s.updateSecret(
-			p.podNamespace,
-			k8sSecretName,
-			p.secretsState.originalK8sSecrets[k8sSecretName],
-			secretData)
+		b := new(bytes.Buffer)
+		_, err := fmt.Fprintf(b, "%v", secretData)
 		if err != nil {
-			// Error messages returned from K8s should be printed only in debug mode
 			p.log.debug(messages.CSPFK005D, err.Error())
 			childSpan.RecordErrorAndSetStatus(err)
 			return updated, p.log.recordedError(messages.CSPFK022E)
 		}
-		updated = true
+
+		// Calculate a sha256 checksum on the content
+		checksum, _ := utils.FileChecksum(b)
+
+		if utils.ContentHasChanged(k8sSecretName, checksum, p.prevSecretsChecksums) {
+			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())
+				childSpan.RecordErrorAndSetStatus(err)
+				return false, p.log.recordedError(messages.CSPFK022E)
+			}
+			p.prevSecretsChecksums[k8sSecretName] = checksum
+			updated = true
+		} else {
+			p.log.info(messages.CSPFK020I)
+			updated = false
+		}
 	}
 
 	return updated, nil

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

@@ -47,6 +47,8 @@ func (m testMocks) setPermissions(denyConjurRetrieve, denyK8sRetrieve,
 	}
 	if denyK8sUpdate {
 		m.kubeClient.CanUpdate = false
+	} else {
+		m.kubeClient.CanUpdate = true
 	}
 }
 
@@ -390,3 +392,95 @@ func TestProvide(t *testing.T) {
 		})
 	}
 }
+
+func TestSecretsContentChanges(t *testing.T) {
+
+	var desc               string
+	var k8sSecrets         k8sStorageMocks.K8sSecrets
+	var requiredSecrets    []string
+	var denyConjurRetrieve bool
+	var denyK8sRetrieve    bool
+	var denyK8sUpdate      bool
+
+	// Initial case, k8s secret should be updated
+	desc = "Only update secrets when there are changes"
+	k8sSecrets = k8sStorageMocks.K8sSecrets{
+		"k8s-secret1": {
+			"conjur-map": {"secret1": "conjur/var/path1"},
+		},
+	}
+	requiredSecrets = []string{"k8s-secret1"}
+	mocks := newTestMocks()
+	mocks.setPermissions(denyConjurRetrieve, denyK8sRetrieve, denyK8sUpdate)
+	for secretName, secretData := range k8sSecrets {
+		mocks.kubeClient.AddSecret(secretName, secretData)
+	}
+	provider := mocks.newProvider(requiredSecrets)
+	update, err := provider.Provide()
+	assert.False(t, mocks.logger.InfoWasLogged(messages.CSPFK020I))
+	assertSecretsUpdated(
+		expectedK8sSecrets{
+			"k8s-secret1": {"secret1": "secret-value1"},
+		},
+		expectedMissingValues{} ) (t, mocks, update, err, desc)
+
+	// Call Provide again, verify it doesn't try to update the secret
+	// as there should be an error if it tried to write the secrets
+	desc = "Verify secrets are not updated when there are no changes"
+	denyK8sUpdate = true
+	mocks.setPermissions(denyConjurRetrieve, denyK8sRetrieve , denyK8sUpdate)
+	update, err = provider.Provide()
+	assert.NoError(t, err)
+	assert.True(t, mocks.logger.InfoWasLogged(messages.CSPFK020I))
+	// verify the same secret still exists
+	assertSecretsUpdated(
+		expectedK8sSecrets{
+			"k8s-secret1": {"secret1": "secret-value1"},
+		},
+		expectedMissingValues{} ) (t, mocks, true, err, desc)
+
+
+	// Change the k8s secret and verify a new secret is written
+	desc = "Verify new secrets are written when there are changes to the Conjur secret"
+	mocks.logger.ClearInfo()
+	secrets,_ := mocks.kubeClient.RetrieveSecret("","k8s-secret1")
+	var newMap = map[string][]byte{
+		"conjur-map": []byte("secret2: conjur/var/path2"),
+	}
+	denyK8sUpdate = false
+	mocks.setPermissions(denyConjurRetrieve, denyK8sRetrieve,denyK8sUpdate)
+	mocks.kubeClient.UpdateSecret("mock namespace", "k8s-secret1", secrets, newMap)
+	update, err = provider.Provide()
+	assert.NoError(t, err)
+	assertSecretsUpdated(
+		expectedK8sSecrets{
+			"k8s-secret1": {"secret2": "secret-value2"},
+		},
+		expectedMissingValues{} ) (t, mocks, update, err, desc)
+	assert.False(t, mocks.logger.InfoWasLogged(messages.CSPFK020I))
+
+	// call again with no changes
+	desc = "Verify again secrets are not updated when there are no changes"
+	update, err = provider.Provide()
+	assert.NoError(t, err)
+	assert.True(t, mocks.logger.InfoWasLogged(messages.CSPFK020I))
+
+	// verify a new k8s secret is written when the Conjur secret changes
+	desc = "Verify new secrets are written when there are changes to the k8s secret"
+	mocks.logger.ClearInfo()
+	var updateConjurSecrets = map[string]string{
+		"conjur/var/path1":        "new-secret-value1",
+		"conjur/var/path2":        "new-secret-value2",
+		"conjur/var/path3":        "new-secret-value3",
+		"conjur/var/path4":        "new-secret-value4",
+		"conjur/var/empty-secret": "",
+	}
+	mocks.conjurClient.AddSecrets(updateConjurSecrets)
+	update, err = provider.Provide()
+	assert.False(t, mocks.logger.InfoWasLogged(messages.CSPFK020I))
+	assertSecretsUpdated(
+		expectedK8sSecrets{
+			"k8s-secret1": {"secret2": "new-secret-value2"},
+		},
+		expectedMissingValues{} ) (t, mocks, update, err, desc)
+}

pkg/secrets/pushtofile/provide_conjur_secrets.go

@@ -17,6 +17,10 @@ type fileProvider struct {
 	secretGroups        []*SecretGroup
 	traceContext        context.Context
 	sanitizeEnabled     bool
+	// prevFileChecksums maps a secret group name to a sha256 checksum of the
+	// corresponding secret file content. This is used to detect changes in
+	// secret file content.
+	//prevFileChecksums map[string]utils.Checksum
 }
 
 type fileProviderDepFuncs struct {
@@ -50,6 +54,7 @@ func NewProvider(
 		secretGroups:        secretGroups,
 		traceContext:        nil,
 		sanitizeEnabled:     sanitizeEnabled,
+		//prevFileChecksums:   map[string]utils.Checksum{},
 	}, nil
 }
 

pkg/secrets/pushtofile/push_to_writer.go

@@ -2,8 +2,8 @@ package pushtofile
 
 import (
 	"bytes"
-	"crypto/sha256"
 	"fmt"
+	"github.com/cyberark/secrets-provider-for-k8s/pkg/utils"
 	"io"
 	"os"
 	"path/filepath"
@@ -36,12 +36,10 @@ type openWriteCloserFunc func(
 	permissions os.FileMode,
 ) (io.WriteCloser, error)
 
-type checksum []byte
-
 // prevFileChecksums maps a secret group name to a sha256 checksum of the
 // corresponding secret file content. This is used to detect changes in
 // secret file content.
-var prevFileChecksums = map[string]checksum{}
+var prevFileChecksums = map[string]utils.Checksum{}
 
 // openFileAsWriteCloser opens a file to write-to with some permissions.
 func openFileAsWriteCloser(path string, permissions os.FileMode) (io.WriteCloser, error) {
@@ -124,10 +122,10 @@ func writeContent(writer io.Writer, fileContent *bytes.Buffer, groupName string)
 	}
 
 	// Calculate a sha256 checksum on the content
-	checksum, _ := fileChecksum(fileContent)
+	checksum, _ := utils.FileChecksum(fileContent)
 
 	// If file contents haven't changed, don't rewrite the file
-	if !contentHasChanged(groupName, checksum) {
+	if !utils.ContentHasChanged(groupName, checksum, prevFileChecksums) {
 		log.Info(messages.CSPFK018I)
 		return false, nil
 	}
@@ -141,22 +139,3 @@ func writeContent(writer io.Writer, fileContent *bytes.Buffer, groupName string)
 	return true, nil
 }
 
-// fileChecksum calculates a checksum on the file content
-func fileChecksum(buf *bytes.Buffer) (checksum, error) {
-	hash := sha256.New()
-	bufCopy := bytes.NewBuffer(buf.Bytes())
-	if _, err := io.Copy(hash, bufCopy); err != nil {
-		return nil, err
-	}
-	checksum := hash.Sum(nil)
-	return checksum, nil
-}
-
-func contentHasChanged(groupName string, fileChecksum checksum) bool {
-	if prevChecksum, exists := prevFileChecksums[groupName]; exists {
-		if bytes.Equal(fileChecksum, prevChecksum) {
-			return false
-		}
-	}
-	return true
-}

pkg/secrets/pushtofile/secret_group_test.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/cyberark/secrets-provider-for-k8s/pkg/utils"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -898,7 +899,7 @@ func TestSecretGroup_PushToFile(t *testing.T) {
 			absoluteFilePath := path.Join(dir, tc.path)
 
 			// Reset the P2F cache so the files will be written even if the values haven't changed
-			prevFileChecksums = map[string]checksum{}
+			prevFileChecksums = map[string]utils.Checksum{}
 
 			// Create a group, and push to file
 			group := SecretGroup{

pkg/utils/checksum.go

@@ -0,0 +1,28 @@
+package utils
+
+import (
+	"bytes"
+	"crypto/sha256"
+	"io"
+)
+
+type Checksum []byte
+
+func FileChecksum(buf *bytes.Buffer) (Checksum, error) {
+	hash := sha256.New()
+	bufCopy := bytes.NewBuffer(buf.Bytes())
+	if _, err := io.Copy(hash, bufCopy); err != nil {
+		return nil, err
+	}
+	checksum := hash.Sum(nil)
+	return checksum, nil
+}
+
+func ContentHasChanged(groupName string, newChecksum Checksum, prevChecksums map[string]Checksum) bool {
+	if prevChecksum, exists := prevChecksums[groupName]; exists {
+		if bytes.Equal(newChecksum, prevChecksum) {
+			return false
+		}
+	}
+	return true
+}