Merge commit from fork

map[] in error output exposes secret data in last-applied-annotation
& patch error

Invalid secrets with stringData exposes the secret values in diff. Attempt a
normalization to prevent it.

Refactor stringData to data conversion to eliminate code duplication

Signed-off-by: Siddhesh Ghadi <[email protected]>

Author: svghadi

pkg/diff/diff.go

@@ -7,6 +7,7 @@ package diff
 import (
 	"bytes"
 	"context"
+	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -843,6 +844,32 @@ func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) {
 	if gvk.Group != "" || gvk.Kind != "Secret" {
 		return
 	}
+
+	// move stringData to data section
+	if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil {
+		var data map[string]interface{}
+		data, found, _ = unstructured.NestedMap(un.Object, "data")
+		if !found {
+			data = make(map[string]interface{})
+		}
+
+		// base64 encode string values and add non-string values as is.
+		// This ensures that the apply fails if the secret is invalid.
+		for k, v := range stringData {
+			strVal, ok := v.(string)
+			if ok {
+				data[k] = base64.StdEncoding.EncodeToString([]byte(strVal))
+			} else {
+				data[k] = v
+			}
+		}
+
+		err := unstructured.SetNestedField(un.Object, data, "data")
+		if err == nil {
+			delete(un.Object, "stringData")
+		}
+	}
+
 	o := applyOptions(opts)
 	var secret corev1.Secret
 	err := runtime.DefaultUnstructuredConverter.FromUnstructured(un.Object, &secret)
@@ -856,15 +883,6 @@ func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) {
 			secret.Data[k] = []byte("")
 		}
 	}
-	if len(secret.StringData) > 0 {
-		if secret.Data == nil {
-			secret.Data = make(map[string][]byte)
-		}
-		for k, v := range secret.StringData {
-			secret.Data[k] = []byte(v)
-		}
-		delete(un.Object, "stringData")
-	}
 	newObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&secret)
 	if err != nil {
 		o.log.Error(err, "object unable to convert from secret")

pkg/diff/diff_test.go

@@ -1044,6 +1044,126 @@ func TestHideSecretDataDifferentKeysDifferentValues(t *testing.T) {
 	assert.Equal(t, map[string]interface{}{"key2": replacement2, "key3": replacement1}, secretData(live))
 }
 
+func TestHideStringDataInInvalidSecret(t *testing.T) {
+	liveUn := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name": "test-secret",
+			},
+			"type": "Opaque",
+			"data": map[string]interface{}{
+				"key1": "a2V5MQ==",
+				"key2": "a2V5MQ==",
+			},
+		},
+	}
+	targetUn := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name": "test-secret",
+			},
+			"type": "Opaque",
+			"data": map[string]interface{}{
+				"key1": "a2V5MQ==",
+				"key2": "a2V5Mg==",
+				"key3": false,
+			},
+			"stringData": map[string]interface{}{
+				"key4": "key4",
+				"key5": 5,
+			},
+		},
+	}
+
+	liveUn = remarshal(liveUn, applyOptions(diffOptionsForTest()))
+	targetUn = remarshal(targetUn, applyOptions(diffOptionsForTest()))
+
+	target, live, err := HideSecretData(targetUn, liveUn, nil)
+	require.NoError(t, err)
+
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement2}, secretData(live))
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1, "key3": replacement1, "key4": replacement1, "key5": replacement1}, secretData(target))
+}
+
+// stringData in secrets should be normalized even if it is invalid
+func TestNormalizeSecret(t *testing.T) {
+	var tests = []struct {
+		testname   string
+		data       map[string]interface{}
+		stringData map[string]interface{}
+	}{
+		{
+			testname: "Valid secret",
+			data: map[string]interface{}{
+				"key1": "key1",
+			},
+			stringData: map[string]interface{}{
+				"key2": "a2V5Mg==",
+			},
+		},
+		{
+			testname: "Invalid secret",
+			data: map[string]interface{}{
+				"key1": "key1",
+				"key2": 2,
+			},
+			stringData: map[string]interface{}{
+				"key3": "key3",
+				"key4": nil,
+			},
+		},
+		{
+			testname: "Invalid secret with stringData only",
+			data:     nil,
+			stringData: map[string]interface{}{
+				"key3": "key3",
+				"key4": true,
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.testname, func(t *testing.T) {
+			un := &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Secret",
+					"metadata": map[string]interface{}{
+						"name": "test-secret",
+					},
+					"type":       "Opaque",
+					"data":       tt.data,
+					"stringData": tt.stringData,
+				},
+			}
+			un = remarshal(un, applyOptions(diffOptionsForTest()))
+
+			NormalizeSecret(un)
+
+			_, found, _ := unstructured.NestedMap(un.Object, "stringData")
+			assert.False(t, found)
+
+			data, found, _ := unstructured.NestedMap(un.Object, "data")
+			assert.True(t, found)
+
+			// check all secret keys are found under data in normalized secret
+			for _, obj := range []map[string]interface{}{tt.data, tt.stringData} {
+				if obj == nil {
+					continue
+				}
+				for k := range obj {
+					_, ok := data[k]
+					assert.True(t, ok)
+				}
+			}
+		})
+	}
+}
+
 func TestHideSecretAnnotations(t *testing.T) {
 	tests := []struct {
 		name           string

pkg/utils/kube/kube.go

@@ -205,12 +205,15 @@ var (
 	// See ApplyOpts::Run()
 	// cmdutil.AddSourceToErr(fmt.Sprintf("applying patch:\n%s\nto:\n%v\nfor:", patchBytes, info), info.Source, err)
 	kubectlApplyPatchErrOutRegexp = regexp.MustCompile(`(?s)^error when applying patch:.*\nfor: "\S+": `)
+
+	kubectlErrOutMapRegexp = regexp.MustCompile(`map\[.*\]`)
 )
 
 // cleanKubectlOutput makes the error output of kubectl a little better to read
 func cleanKubectlOutput(s string) string {
 	s = strings.TrimSpace(s)
 	s = kubectlErrOutRegexp.ReplaceAllString(s, "")
+	s = kubectlErrOutMapRegexp.ReplaceAllString(s, "")
 	s = kubectlApplyPatchErrOutRegexp.ReplaceAllString(s, "")
 	s = strings.Replace(s, "; if you choose to ignore these errors, turn validation off with --validate=false", "", -1)
 	return s

pkg/utils/kube/kube_test.go

@@ -74,6 +74,18 @@ Object: &{map["apiVersion":"v1" "kind":"Service" "metadata":map["annotations":ma
 for: "/var/folders/_m/991sn1ds7g39lnbhp6wvqp9d_j5476/T/224503547": Service "my-service" is invalid: spec.clusterIP: Invalid value: "10.96.0.44": field is immutable`
 		assert.Equal(t, cleanKubectlOutput(s), `Service "my-service" is invalid: spec.clusterIP: Invalid value: "10.96.0.44": field is immutable`)
 	}
+	{
+		s := `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/745145319": "" is invalid: patch: Invalid value: "map[data:map[email:aaaaa password:<nil> username:<nil>] metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"email\":\"aaaaa\"},\"kind\":\"Secret\",\"metadata\":{\"annotations\":{},\"labels\":{\"app.kubernetes.io/instance\":\"test\"},\"name\":\"my-secret\",\"namespace\":\"default\"},\"stringData\":{\"id\":1,\"password\":0,\"username\":\"username\"},\"type\":\"Opaque\"}\n]] stringData:map[id:1 password:0 username:username]]": error decoding from json: illegal base64 data at input byte 4`
+		assert.Equal(t, cleanKubectlOutput(s), `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/745145319": "" is invalid: patch: Invalid value: "": error decoding from json: illegal base64 data at input byte 4`)
+	}
+	{
+		s := `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/2250018703": "" is invalid: patch: Invalid value: "map[data:<nil> metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"kind\":\"Secret\",\"metadata\":{\"annotations\":{},\"labels\":{\"app.kubernetes.io/instance\":\"test\"},\"name\":\"my-secret\",\"namespace\":\"default\"},\"stringData\":{\"id\":1,\"password\":0,\"username\":\"username\"},\"type\":\"Opaque\"}\n]]]": cannot convert int64 to string`
+		assert.Equal(t, cleanKubectlOutput(s), `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/2250018703": "" is invalid: patch: Invalid value: "": cannot convert int64 to string`)
+	}
+	{
+		s := `Secret in version "v1" cannot be handled as a Secret: json: cannot unmarshal bool into Go struct field Secret.data of type []uint8`
+		assert.Equal(t, cleanKubectlOutput(s), `Secret in version "v1" cannot be handled as a Secret: json: cannot unmarshal bool into Go struct field Secret.data of type []uint8`)
+	}
 }
 
 func TestInClusterKubeConfig(t *testing.T) {