Issue #897 - Secret data not redacted in last-applied-configuration (#902)

controller/appcontroller.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
+	"k8s.io/kubernetes/pkg/apis/core"
 
 	statecache "github.com/argoproj/argo-cd/controller/cache"
 	"github.com/argoproj/argo-cd/controller/services"
@@ -182,8 +183,18 @@ func (ctrl *ApplicationController) ManagedResources(ctx context.Context, q *serv
 			Group:     res.Group,
 			Kind:      res.Kind,
 		}
-		live, data := hideSecretData(res.Live, nil)
-		target, _ := hideSecretData(res.Target, data)
+
+		target := res.Target
+		live := res.Live
+		resDiff := res.Diff
+		if res.Kind == kube.SecretKind && res.Group == "" {
+			var err error
+			target, live, err = hideSecretData(res.Target, res.Live)
+			if err != nil {
+				return nil, err
+			}
+			resDiff = *diff.Diff(target, live)
+		}
 
 		if live != nil {
 			data, err := json.Marshal(live)
@@ -204,7 +215,7 @@ func (ctrl *ApplicationController) ManagedResources(ctx context.Context, q *serv
 		} else {
 			item.TargetState = "null"
 		}
-		jsonDiff, err := res.Diff.JSONFormat()
+		jsonDiff, err := resDiff.JSONFormat()
 		if err != nil {
 			return nil, err
 		}
@@ -222,32 +233,78 @@ func toString(val interface{}) string {
 	return fmt.Sprintf("%s", val)
 }
 
-// hideSecretData checks if given object kind is Secret, replaces data keys with stars and returns unchanged data map. The method additionally check if data key if different
-// from corresponding key of optional parameter `otherData` and adds extra star to keep information about difference. So if secret data is out of sync user still can see which
-// fields are different.
-func hideSecretData(state *unstructured.Unstructured, otherData map[string]interface{}) (*unstructured.Unstructured, map[string]interface{}) {
-	obj := state.DeepCopy()
-	if obj != nil && obj.GroupVersionKind().Group == "" && obj.GetKind() == kube.SecretKind {
-		if data, ok, err := unstructured.NestedMap(obj.Object, "data"); err == nil && ok {
-			unchangedData := make(map[string]interface{})
-			for k, v := range data {
-				unchangedData[k] = v
-			}
-			for k := range data {
-				replacement := "********"
-				if otherData != nil {
-					if val, ok := otherData[k]; ok && toString(val) != toString(data[k]) {
-						replacement = replacement + "*"
-					}
-				}
-				data[k] = replacement
-			}
-			_ = unstructured.SetNestedMap(obj.Object, data, "data")
+// hideSecretData replaces secret data values in specified target, live secrets and in last applied configuration of live secret with stars. Also preserves differences between
+// target, live and last applied config values. E.g. if all three are equal the values would be replaced with same number of stars. If all the are different then number of stars
+// in replacement should be different.
+func hideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured) (*unstructured.Unstructured, *unstructured.Unstructured, error) {
+	var orig *unstructured.Unstructured
+	if live != nil {
+		orig = diff.GetLastAppliedConfigAnnotation(live)
+		live = live.DeepCopy()
+	}
+	if target != nil {
+		target = target.DeepCopy()
+	}
 
-			return obj, unchangedData
+	keys := map[string]bool{}
+	for _, obj := range []*unstructured.Unstructured{target, live, orig} {
+		if obj == nil {
+			continue
+		}
+		diff.EncodeSecretStringData(obj)
+		if data, found, err := unstructured.NestedMap(obj.Object, "data"); found && err == nil {
+			for k := range data {
+				keys[k] = true
+			}
 		}
 	}
-	return obj, nil
+
+	for k := range keys {
+		nextReplacement := "*********"
+		valToReplacement := make(map[string]string)
+		for _, obj := range []*unstructured.Unstructured{target, live, orig} {
+			var data map[string]interface{}
+			if obj != nil {
+				var err error
+				data, _, err = unstructured.NestedMap(obj.Object, "data")
+				if err != nil {
+					return nil, nil, err
+				}
+			}
+			if data == nil {
+				data = make(map[string]interface{})
+			}
+			valData, ok := data[k]
+			if !ok {
+				continue
+			}
+			val := toString(valData)
+			replacement, ok := valToReplacement[val]
+			if !ok {
+				replacement = nextReplacement
+				nextReplacement = nextReplacement + "*"
+				valToReplacement[val] = replacement
+			}
+			data[k] = replacement
+			err := unstructured.SetNestedField(obj.Object, data, "data")
+			if err != nil {
+				return nil, nil, err
+			}
+		}
+	}
+	if live != nil && orig != nil {
+		annotations := live.GetAnnotations()
+		if annotations == nil {
+			annotations = make(map[string]string)
+		}
+		lastAppliedData, err := json.Marshal(orig)
+		if err != nil {
+			return nil, nil, err
+		}
+		annotations[core.LastAppliedConfigAnnotation] = string(lastAppliedData)
+		live.SetAnnotations(annotations)
+	}
+	return target, live, nil
 }
 
 // Run starts the Application CRD controller.

controller/appcontroller_test.go

@@ -1,6 +1,7 @@
 package controller
 
 import (
+	"encoding/json"
 	"strings"
 	"testing"
 	"time"
@@ -375,3 +376,75 @@ func TestDontNormalizeApplication(t *testing.T) {
 	ctrl.processAppRefreshQueueItem()
 	assert.False(t, normalized)
 }
+
+func createSecret(data map[string]string) *unstructured.Unstructured {
+	secret := corev1.Secret{TypeMeta: metav1.TypeMeta{Kind: kube.SecretKind}}
+	if data != nil {
+		secret.Data = make(map[string][]byte)
+		for k, v := range data {
+			secret.Data[k] = []byte(v)
+		}
+	}
+
+	return kube.MustToUnstructured(&secret)
+}
+
+func secretData(obj *unstructured.Unstructured) map[string]interface{} {
+	data, _, _ := unstructured.NestedMap(obj.Object, "data")
+	return data
+}
+
+const (
+	replacement1 = "*********"
+	replacement2 = "**********"
+	replacement3 = "***********"
+)
+
+func TestHideSecretDataSameKeysDifferentValues(t *testing.T) {
+	target, live, err := hideSecretData(
+		createSecret(map[string]string{"key1": "test", "key2": "test"}),
+		createSecret(map[string]string{"key1": "test-1", "key2": "test-1"}))
+	assert.Nil(t, err)
+
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1}, secretData(target))
+	assert.Equal(t, map[string]interface{}{"key1": replacement2, "key2": replacement2}, secretData(live))
+}
+
+func TestHideSecretDataSameKeysSameValues(t *testing.T) {
+	target, live, err := hideSecretData(
+		createSecret(map[string]string{"key1": "test", "key2": "test"}),
+		createSecret(map[string]string{"key1": "test", "key2": "test"}))
+	assert.Nil(t, err)
+
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1}, secretData(target))
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1}, secretData(live))
+}
+
+func TestHideSecretDataDifferentKeysDifferentValues(t *testing.T) {
+	target, live, err := hideSecretData(
+		createSecret(map[string]string{"key1": "test", "key2": "test"}),
+		createSecret(map[string]string{"key2": "test-1", "key3": "test-1"}))
+	assert.Nil(t, err)
+
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1}, secretData(target))
+	assert.Equal(t, map[string]interface{}{"key2": replacement2, "key3": replacement1}, secretData(live))
+}
+
+func TestHideSecretDataLastAppliedConfig(t *testing.T) {
+	lastAppliedSecret := createSecret(map[string]string{"key1": "test1"})
+	targetSecret := createSecret(map[string]string{"key1": "test2"})
+	liveSecret := createSecret(map[string]string{"key1": "test3"})
+	lastAppliedStr, err := json.Marshal(lastAppliedSecret)
+	assert.Nil(t, err)
+	liveSecret.SetAnnotations(map[string]string{corev1.LastAppliedConfigAnnotation: string(lastAppliedStr)})
+
+	target, live, err := hideSecretData(targetSecret, liveSecret)
+	assert.Nil(t, err)
+	err = json.Unmarshal([]byte(live.GetAnnotations()[corev1.LastAppliedConfigAnnotation]), &lastAppliedSecret)
+	assert.Nil(t, err)
+
+	assert.Equal(t, map[string]interface{}{"key1": replacement1}, secretData(target))
+	assert.Equal(t, map[string]interface{}{"key1": replacement2}, secretData(live))
+	assert.Equal(t, map[string]interface{}{"key1": replacement3}, secretData(lastAppliedSecret))
+
+}

server/application/application.go

@@ -8,6 +8,8 @@ import (
 	"strings"
 	"time"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc/codes"
@@ -487,6 +489,10 @@ func (s *Server) GetResource(ctx context.Context, q *ApplicationResourceRequest)
 	if err != nil {
 		return nil, err
 	}
+	err = replaceSecretValues(obj)
+	if err != nil {
+		return nil, err
+	}
 	data, err := json.Marshal(obj.Object)
 	if err != nil {
 		return nil, err
@@ -494,6 +500,22 @@ func (s *Server) GetResource(ctx context.Context, q *ApplicationResourceRequest)
 	return &ApplicationResourceResponse{Manifest: string(data)}, nil
 }
 
+func replaceSecretValues(obj *unstructured.Unstructured) error {
+	if obj.GetKind() == kube.SecretKind && obj.GroupVersionKind().Group == "" {
+		data, _, _ := unstructured.NestedMap(obj.Object, "data")
+		if data != nil {
+			for k := range data {
+				data[k] = "**********"
+			}
+			err := unstructured.SetNestedField(obj.Object, data, "data")
+			if err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 func (s *Server) DeleteResource(ctx context.Context, q *ApplicationResourceDeleteRequest) (*ApplicationResponse, error) {
 	resourceRequest := &ApplicationResourceRequest{
 		Name:         q.Name,

test/e2e/functional/secrets/secret.yaml

@@ -0,0 +1,8 @@
+apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+stringData:
+  username: test-username
+data:
+  password: dGVzdC1wYXNzd29yZA==

util/diff/diff.go

@@ -33,13 +33,14 @@ type DiffResultList struct {
 func Diff(config, live *unstructured.Unstructured) *DiffResult {
 	if config != nil {
 		config = stripTypeInformation(config)
-		encodeSecretStringData(config)
+		EncodeSecretStringData(config)
 	}
 	if live != nil {
 		live = stripTypeInformation(live)
 	}
-	orig := getLastAppliedConfigAnnotation(live)
+	orig := GetLastAppliedConfigAnnotation(live)
 	if orig != nil && config != nil {
+		EncodeSecretStringData(orig)
 		dr, err := ThreeWayDiff(orig, config, live)
 		if err == nil {
 			return dr
@@ -182,7 +183,7 @@ func threeWayMergePatch(orig, config, live *unstructured.Unstructured) ([]byte,
 	return strategicpatch.CreateThreeWayMergePatch(origBytes, configBytes, liveBytes, lookupPatchMeta, true)
 }
 
-func getLastAppliedConfigAnnotation(live *unstructured.Unstructured) *unstructured.Unstructured {
+func GetLastAppliedConfigAnnotation(live *unstructured.Unstructured) *unstructured.Unstructured {
 	if live == nil {
 		return nil
 	}
@@ -203,53 +204,6 @@ func getLastAppliedConfigAnnotation(live *unstructured.Unstructured) *unstructur
 	return &obj
 }
 
-// MatchObjectLists takes two possibly disjoint lists of Unstructured objects, and returns two new
-// lists of equal lengths, filled out with nils from missing objects in the opposite list.
-// These lists can then be passed into DiffArray for comparison
-func MatchObjectLists(leftObjs, rightObjs []*unstructured.Unstructured) ([]*unstructured.Unstructured, []*unstructured.Unstructured) {
-	newLeftObjs := make([]*unstructured.Unstructured, 0)
-	newRightObjs := make([]*unstructured.Unstructured, 0)
-
-	for _, left := range leftObjs {
-		if left == nil {
-			continue
-		}
-		newLeftObjs = append(newLeftObjs, left)
-		right := objByKindName(rightObjs, left.GetKind(), left.GetName())
-		newRightObjs = append(newRightObjs, right)
-	}
-
-	for _, right := range rightObjs {
-		if right == nil {
-			continue
-		}
-		left := objByKindName(leftObjs, right.GetKind(), right.GetName())
-		if left != nil {
-			// object exists in both list. this object was already appended to both lists in the
-			// first for/loop
-			continue
-		}
-		// if we get here, we found a right which doesn't exist in the left object list.
-		// append a nil to the left object list
-		newLeftObjs = append(newLeftObjs, nil)
-		newRightObjs = append(newRightObjs, right)
-
-	}
-	return newLeftObjs, newRightObjs
-}
-
-func objByKindName(objs []*unstructured.Unstructured, kind, name string) *unstructured.Unstructured {
-	for _, obj := range objs {
-		if obj == nil {
-			continue
-		}
-		if obj.GetKind() == kind && obj.GetName() == name {
-			return obj
-		}
-	}
-	return nil
-}
-
 // DiffArray performs a diff on a list of unstructured objects. Objects are expected to match
 // environments
 func DiffArray(configArray, liveArray []*unstructured.Unstructured) (*DiffResultList, error) {
@@ -287,7 +241,7 @@ func (d *DiffResult) ASCIIFormat(left *unstructured.Unstructured, formatOpts for
 
 // encodeSecretStringData mutates the supplied object and encodes stringData to data. If the object
 // is not a secret, or is an invalid secret, then returns the same object
-func encodeSecretStringData(un *unstructured.Unstructured) {
+func EncodeSecretStringData(un *unstructured.Unstructured) {
 	if un == nil {
 		return
 	}