Refactor implementation

Signed-off-by: Szymon Gibała <szymongib@gmail.com>

go.mod

@@ -37,7 +37,7 @@ require (
 	github.com/xeipuuv/gojsonschema v1.2.0
 	golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad
 	golang.org/x/term v0.0.0-20201117132131-f5c789dd3221
-	gopkg.in/yaml.v2 v2.3.0
+	gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c
 	k8s.io/api v0.20.2
 	k8s.io/apiextensions-apiserver v0.20.2
 	k8s.io/apimachinery v0.20.2

pkg/cli/sanitize/hide_secrets.go

@@ -20,9 +20,11 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/pkg/errors"
+
 	"helm.sh/helm/v3/pkg/release"
 
-	"gopkg.in/yaml.v2"
+	"gopkg.in/yaml.v3"
 )
 
 const (
@@ -31,6 +33,7 @@ const (
 
 // HideManifestSecrets sanitizes release manifest and replaces it.
 // Manifest cannot be applied after this operation.
+// Indentation and extra spaces in Secret's `data` and `stringData` sections can be impacted.
 func HideManifestSecrets(r *release.Release) error {
 	if r == nil {
 		return nil
@@ -55,11 +58,15 @@ func hideSecrets(manifest string) (string, error) {
 		var resourceMap map[string]interface{}
 		err := yaml.Unmarshal([]byte(r), &resourceMap)
 		if err != nil {
-			return "", err
+			return "", errors.Wrapf(err, "failed to unmarshal %q resource", tryToGetName(resourceMap))
 		}
 
 		if isSecret(resourceMap) {
-			r = hideSecretData(r, resourceMap)
+			rs, err := hideSecretData(r)
+			if err != nil {
+				return "", errors.Wrapf(err, "failed to hide %q Secret data", tryToGetName(resourceMap))
+			}
+			r = rs
 		}
 
 		outRes = append(outRes, r)
@@ -68,6 +75,20 @@ func hideSecrets(manifest string) (string, error) {
 	return strings.Join(outRes, "\n---"), nil
 }
 
+func tryToGetName(resourceMap map[string]interface{}) string {
+	metadata, ok := resourceMap["metadata"].(map[string]interface{})
+	if !ok {
+		return ""
+	}
+
+	name, ok := metadata["name"].(string)
+	if !ok {
+		return ""
+	}
+
+	return name
+}
+
 func isSecret(resource map[string]interface{}) bool {
 	kind, ok := resource["kind"].(string)
 	if !ok || kind != "Secret" {
@@ -82,68 +103,105 @@ func isSecret(resource map[string]interface{}) bool {
 	return true
 }
 
-func hideSecretData(raw string, resource map[string]interface{}) string {
+func hideSecretData(raw string) (string, error) {
-	dataRaw, ok := resource["data"].(map[interface{}]interface{})
+	lines := strings.Split(raw, "\n")
-	if !ok || len(dataRaw) == 0 {
+	outLines := make([]string, 0, len(lines))
-		return raw
+
+	// To minimize impact of empty lines and custom indentation
+	// we only marshal `data` and `secretData` sections of the Secrets
+	for i := 0; i < len(lines); i++ {
+		line := lines[i]
+		outLines = append(outLines, line)
+
+		if strings.HasPrefix(line, "data:") || strings.HasPrefix(line, "stringData:") {
+			endLine := findSectionEnd(lines, i+1)
+			sanitizedLines, err := sanitizeDataYaml(lines[i+1 : endLine+1])
+			if err != nil {
+				return "", errors.Wrap(err, "failed to sanitize Secret data")
+			}
+			outLines = append(outLines, sanitizedLines...)
+
+			i = endLine
+		}
 	}
 
-	data := toMapOfStrings(dataRaw)
+	return strings.Join(outLines, "\n"), nil
+}
 
-	lines := strings.Split(raw, "\n")
+func sanitizeDataYaml(yamlLines []string) ([]string, error) {
-	outLines := make([]string, len(lines))
+	yamlData := strings.Join(yamlLines, "\n")
+	data := &yaml.Node{}
+	err := yaml.Unmarshal([]byte(yamlData), data)
+	if err != nil {
+		return nil, err
+	}
+	if len(data.Content) == 0 {
+		return []string{}, nil
+	}
 
-	for i, line := range lines {
+	node := data.Content[0]
-		trimmed := strings.TrimSpace(line)
+	sanitizeNode(node)
 
-		// If line is part of secret.data, sanitize line by replacing the value part
+	// Try to preserve indentation of the data section
-		if key, matches := matchKeyValPair(data, trimmed); matches {
+	indent := "  "
-			sanitizedLine := strings.Replace(line, trimmed, formatHiddenValue(key), 1)
+	if len(node.Content) > 1 {
-			outLines[i] = sanitizedLine
+		lineNum := node.Content[1].Line
-			continue
+		indent = takeWhitespace(yamlLines[lineNum-1])
-		}
+	}
 
-		outLines[i] = line
+	sanitized, err := yaml.Marshal(node)
+	if err != nil {
+		return nil, err
+	}
+	str := strings.TrimSpace(string(sanitized))
+	lines := strings.Split(str, "\n")
+
+	for i := range lines {
+		lines[i] = fmt.Sprintf("%s%s", indent, lines[i])
 	}
 
-	return strings.Join(outLines, "\n")
+	return lines, nil
 }
 
-func toMapOfStrings(rawMap map[interface{}]interface{}) map[string]string {
+func sanitizeNode(node *yaml.Node) {
-	stringsMap := make(map[string]string, len(rawMap))
+	if node.Kind != yaml.MappingNode {
-	for k, v := range rawMap {
+		return
-		key, ok := k.(string)
-		if !ok {
-			continue
-		}
-		val, ok := v.(string)
-		if !ok {
-			continue
-		}
-		stringsMap[key] = val
 	}
-	return stringsMap
+	contents := node.Content
-}
 
-// matchKeyValPair checks if data contains joined key value pair in format
+	for i := 1; i < len(contents); i += 2 {
-// `key: value` equal to specified string.
+		contents[i].Style = 0 // Erase literal style
-// Returns key with which string matched and indicator if it matched any.
+		contents[i].SetString(hiddenSecretValue)
-func matchKeyValPair(data map[string]string, str string) (string, bool) {
+	}
-	for k, v := range data {
+}
-		joined := joinKeyVal(k, v)
 
-		if joined == str {
+func takeWhitespace(line string) string {
-			return k, true
+	buff := make([]byte, 0)
+	for _, c := range line {
+		switch c {
+		case ' ', '\t':
+			buff = append(buff, byte(c))
+			continue
 		}
-	}
 
-	return "", false
+		break
+	}
+	return string(buff)
 }
 
-func joinKeyVal(key, val string) string {
+func findSectionEnd(lines []string, start int) int {
-	return fmt.Sprintf("%s: %s", key, val)
+	i := start
+	for i < len(lines) && isDataLine(lines[i]) {
+		i++
+	}
+	return i - 1
 }
 
-func formatHiddenValue(key string) string {
+func isDataLine(line string) bool {
-	return joinKeyVal(key, hiddenSecretValue)
+	trimmed := strings.TrimSpace(line)
+	if trimmed == "" {
+		return true
+	}
+
+	return len(line) > 1 && (line[0] == byte('\t') || line[0] == byte(' '))
 }

pkg/cli/sanitize/hide_secrets_test.go

@@ -28,43 +28,63 @@ import (
 
 func TestHideManifestSecrets(t *testing.T) {
 
-	t.Run("replace manifest with sanitized one", func(t *testing.T) {
+	for _, testCase := range []struct {
-		manifestRaw, err := ioutil.ReadFile("testdata/manifest-input.yaml")
+		description   string
-		require.NoError(t, err)
+		manifestFile  string
-		expectedManifestRaw, err := ioutil.ReadFile("testdata/manifest-sanitized.yaml")
+		sanitizedFile string
-		require.NoError(t, err)
+	}{
+		{
+			description:   "replace manifest with sanitized one",
+			manifestFile:  "testdata/manifest-input.yaml",
+			sanitizedFile: "testdata/manifest-sanitized.yaml",
+		},
+		{
+			description:   "handle different secrets",
+			manifestFile:  "testdata/different-secrets.yaml",
+			sanitizedFile: "testdata/different-secrets-sanitized.yaml",
+		},
+		{
+			description:   "do not modify manifest when no secret values",
+			manifestFile:  "testdata/manifest-no-secret.yaml",
+			sanitizedFile: "testdata/manifest-no-secret.yaml",
+		},
+	} {
+		t.Run(testCase.description, func(t *testing.T) {
+			manifestRaw, err := ioutil.ReadFile(testCase.manifestFile)
+			require.NoError(t, err)
+			expectedManifestRaw, err := ioutil.ReadFile(testCase.sanitizedFile)
+			require.NoError(t, err)
+
+			rel := &release.Release{
+				Name:     "test",
+				Manifest: string(manifestRaw),
+			}
+
+			err = HideManifestSecrets(rel)
+			require.NoError(t, err)
+
+			assert.Equal(t, string(expectedManifestRaw), rel.Manifest)
+		})
+	}
 
-		rel := &release.Release{
+	t.Run("ignore nil release", func(t *testing.T) {
-			Name:     "test",
+		var rel *release.Release
-			Manifest: string(manifestRaw),
-		}
 
-		err = HideManifestSecrets(rel)
+		err := HideManifestSecrets(rel)
 		require.NoError(t, err)
-
+		assert.Nil(t, rel)
-		assert.Equal(t, string(expectedManifestRaw), rel.Manifest)
 	})
 
-	t.Run("do not modify manifest when no secret values", func(t *testing.T) {
+	t.Run("include secret name in error message", func(t *testing.T) {
-		manifestRaw, err := ioutil.ReadFile("testdata/manifest-no-secret.yaml")
+		manifestRaw, err := ioutil.ReadFile("testdata/invalid-secret.yaml")
 		require.NoError(t, err)
 
 		rel := &release.Release{
 			Name:     "test",
 			Manifest: string(manifestRaw),
 		}
-
 		err = HideManifestSecrets(rel)
-		require.NoError(t, err)
+		require.Error(t, err)
-
+		require.Contains(t, err.Error(), "\"invalid-secret-with-dup-values\"")
-		assert.Equal(t, string(manifestRaw), rel.Manifest)
-	})
-
-	t.Run("ignore nil release", func(t *testing.T) {
-		var rel *release.Release
-
-		err := HideManifestSecrets(rel)
-		require.NoError(t, err)
-		assert.Nil(t, rel)
 	})
 }

pkg/cli/sanitize/testdata/different-secrets-sanitized.yaml

@@ -0,0 +1,77 @@
+---
+# Source: test/templates/secret-no-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+
+---
+# Source: test/templates/secret-empty-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+---
+# Source: test/templates/secret-empty-string-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+stringData:
+---
+# Source: test/templates/secret-empty-data-and-string-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+stringData:
+---
+# Source: test/templates/secret-different-indent.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+      password: '[HIDDEN]'
+stringData:
+  stringPassword: '[HIDDEN]'
+---
+# Source: test/templates/secret-empty-lines.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+      password: '[HIDDEN]'
+      sword: '[HIDDEN]'
+stringData:
+  inline: '[HIDDEN]'
+  stringPassword: '[HIDDEN]'
+---
+# Source: test/templates/secret-with-comments.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+  # Comment
+  password: '[HIDDEN]'
+  # Multi
+  # Line
+  # Comment
+  sword: '[HIDDEN]' # Inline comment
+  # End comment
+stringData:
+  stringPassword: '[HIDDEN]'
+---
+# Source: test/templates/secret-different-order.yaml
+apiVersion: v1
+kind: Secret
+stringData:
+  stringPassword: '[HIDDEN]'
+data:
+  password: '[HIDDEN]'
+metadata:
+  name: secret-sample

pkg/cli/sanitize/testdata/different-secrets.yaml

@@ -0,0 +1,88 @@
+---
+# Source: test/templates/secret-no-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+
+---
+# Source: test/templates/secret-empty-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+---
+# Source: test/templates/secret-empty-string-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+stringData:
+
+---
+# Source: test/templates/secret-empty-data-and-string-data.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+stringData:
+---
+# Source: test/templates/secret-different-indent.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+      password: cGFzc3dvcmQK
+stringData:
+  stringPassword: password
+
+---
+# Source: test/templates/secret-empty-lines.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+
+      password: cGFzc3dvcmQK
+
+
+      sword: cGFzc3dvcmQK
+stringData:
+  inline: |
+    inline: data
+    data: inline
+
+
+
+  stringPassword: password
+---
+# Source: test/templates/secret-with-comments.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: secret-sample
+data:
+  # Comment
+  password: cGFzc3dvcmQK
+  # Multi
+  # Line
+  # Comment
+  sword: cGFzc3dvcmQK # Inline comment
+  # End comment
+stringData:
+  stringPassword: password
+
+---
+# Source: test/templates/secret-different-order.yaml
+apiVersion: v1
+kind: Secret
+stringData:
+  stringPassword: password
+data:
+  password: cGFzc3dvcmQK
+metadata:
+  name: secret-sample

pkg/cli/sanitize/testdata/invalid-secret.yaml

@@ -0,0 +1,11 @@
+---
+# Source: test/templates/secret-different-indent.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: invalid-secret-with-dup-values
+data:
+  password: cGFzc3dvcmQK
+  password: cGFzc3dvcmQyCg==
+stringData:
+  stringPassword: password

pkg/cli/sanitize/testdata/manifest-input.yaml

@@ -13,17 +13,30 @@ metadata:
 data:
   test: YmFyCg==
   password: bXktcGFzc3dvcmQ=
+  complex.key: Y29tcGxleAo=
+  fromFile.json: |
+    ewogICJteS1jb25maWcta2V5IjogIm15IHZhbHVlIiwgCiAgImFnZSI6IDI0LAogICJhcnJheSI6IFsidmFsdWUiLCAidmFsdWUyIl0sCiAgIm9iamVjdCI6IHsKICAgICJrZXkiOiAidmFsdWUiCiAgfQp9Cg==
+stringData:
+  string: super-secret
+  string.complex: complex
+  stringFile.json: |
+    {
+      "my-config-key": "my value",
+      "age": 24,
+      "array": ["value", "value2"],
+      "object": {
+        "key": "value"
+      }
+    }
 ---
 # Source: test/templates/cm.yaml
 apiVersion: v1
 kind: ConfigMap
 metadata:
-  creationTimestamp: 2016-02-18T18:52:05Z
   name: game-config
   namespace: default
-  resourceVersion: "516"
-  uid: b4952dc3-d670-11e5-8cd0-68f728db1985
 data:
+  stringData: string-data
   test: YmFyCg==
   game.properties: |
     enemies=aliens

pkg/cli/sanitize/testdata/manifest-sanitized.yaml

@@ -11,19 +11,23 @@ kind: Secret
 metadata:
   name: secret-sample
 data:
-  test: [HIDDEN]
+  test: '[HIDDEN]'
-  password: [HIDDEN]
+  password: '[HIDDEN]'
+  complex.key: '[HIDDEN]'
+  fromFile.json: '[HIDDEN]'
+stringData:
+  string: '[HIDDEN]'
+  string.complex: '[HIDDEN]'
+  stringFile.json: '[HIDDEN]'
 ---
 # Source: test/templates/cm.yaml
 apiVersion: v1
 kind: ConfigMap
 metadata:
-  creationTimestamp: 2016-02-18T18:52:05Z
   name: game-config
   namespace: default
-  resourceVersion: "516"
-  uid: b4952dc3-d670-11e5-8cd0-68f728db1985
 data:
+  stringData: string-data
   test: YmFyCg==
   game.properties: |
     enemies=aliens