diff --git a/go.mod b/go.mod index 3b396e094..ea2d28e14 100644 --- a/go.mod +++ b/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 diff --git a/pkg/cli/sanitize/hide_secrets.go b/pkg/cli/sanitize/hide_secrets.go index 3876c601a..1be42d663 100644 --- a/pkg/cli/sanitize/hide_secrets.go +++ b/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 { - dataRaw, ok := resource["data"].(map[interface{}]interface{}) - if !ok || len(dataRaw) == 0 { - return raw +func hideSecretData(raw string) (string, error) { + lines := strings.Split(raw, "\n") + outLines := make([]string, 0, len(lines)) + + // 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") - outLines := make([]string, len(lines)) +func sanitizeDataYaml(yamlLines []string) ([]string, error) { + 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 { - trimmed := strings.TrimSpace(line) + node := data.Content[0] + sanitizeNode(node) - // If line is part of secret.data, sanitize line by replacing the value part - if key, matches := matchKeyValPair(data, trimmed); matches { - sanitizedLine := strings.Replace(line, trimmed, formatHiddenValue(key), 1) - outLines[i] = sanitizedLine - continue - } + // Try to preserve indentation of the data section + indent := " " + if len(node.Content) > 1 { + lineNum := node.Content[1].Line + 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 { - stringsMap := make(map[string]string, len(rawMap)) - for k, v := range rawMap { - key, ok := k.(string) - if !ok { - continue - } - val, ok := v.(string) - if !ok { - continue - } - stringsMap[key] = val +func sanitizeNode(node *yaml.Node) { + if node.Kind != yaml.MappingNode { + return } - return stringsMap -} + contents := node.Content -// matchKeyValPair checks if data contains joined key value pair in format -// `key: value` equal to specified string. -// Returns key with which string matched and indicator if it matched any. -func matchKeyValPair(data map[string]string, str string) (string, bool) { - for k, v := range data { - joined := joinKeyVal(k, v) + for i := 1; i < len(contents); i += 2 { + contents[i].Style = 0 // Erase literal style + contents[i].SetString(hiddenSecretValue) + } +} - if joined == str { - return k, true +func takeWhitespace(line string) string { + 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 { - return fmt.Sprintf("%s: %s", key, val) +func findSectionEnd(lines []string, start int) int { + i := start + for i < len(lines) && isDataLine(lines[i]) { + i++ + } + return i - 1 } -func formatHiddenValue(key string) string { - return joinKeyVal(key, hiddenSecretValue) +func isDataLine(line string) bool { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + return true + } + + return len(line) > 1 && (line[0] == byte('\t') || line[0] == byte(' ')) } diff --git a/pkg/cli/sanitize/hide_secrets_test.go b/pkg/cli/sanitize/hide_secrets_test.go index 770233122..4541db0dd 100644 --- a/pkg/cli/sanitize/hide_secrets_test.go +++ b/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) { - manifestRaw, err := ioutil.ReadFile("testdata/manifest-input.yaml") - require.NoError(t, err) - expectedManifestRaw, err := ioutil.ReadFile("testdata/manifest-sanitized.yaml") - require.NoError(t, err) + for _, testCase := range []struct { + description string + manifestFile string + sanitizedFile string + }{ + { + 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{ - Name: "test", - Manifest: string(manifestRaw), - } + t.Run("ignore nil release", func(t *testing.T) { + var rel *release.Release - err = HideManifestSecrets(rel) + err := HideManifestSecrets(rel) require.NoError(t, err) - - assert.Equal(t, string(expectedManifestRaw), rel.Manifest) + assert.Nil(t, rel) }) - t.Run("do not modify manifest when no secret values", func(t *testing.T) { - manifestRaw, err := ioutil.ReadFile("testdata/manifest-no-secret.yaml") + t.Run("include secret name in error message", func(t *testing.T) { + 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) - - 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) + require.Error(t, err) + require.Contains(t, err.Error(), "\"invalid-secret-with-dup-values\"") }) } diff --git a/pkg/cli/sanitize/testdata/different-secrets-sanitized.yaml b/pkg/cli/sanitize/testdata/different-secrets-sanitized.yaml new file mode 100644 index 000000000..df8bdc50f --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/cli/sanitize/testdata/different-secrets.yaml b/pkg/cli/sanitize/testdata/different-secrets.yaml new file mode 100644 index 000000000..d3f5a4e4d --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/cli/sanitize/testdata/invalid-secret.yaml b/pkg/cli/sanitize/testdata/invalid-secret.yaml new file mode 100644 index 000000000..01233a431 --- /dev/null +++ b/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 diff --git a/pkg/cli/sanitize/testdata/manifest-input.yaml b/pkg/cli/sanitize/testdata/manifest-input.yaml index c9773c0ac..566f1bc78 100644 --- a/pkg/cli/sanitize/testdata/manifest-input.yaml +++ b/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 diff --git a/pkg/cli/sanitize/testdata/manifest-sanitized.yaml b/pkg/cli/sanitize/testdata/manifest-sanitized.yaml index acc9a9e20..f095e333f 100644 --- a/pkg/cli/sanitize/testdata/manifest-sanitized.yaml +++ b/pkg/cli/sanitize/testdata/manifest-sanitized.yaml @@ -11,19 +11,23 @@ kind: Secret metadata: name: secret-sample data: - test: [HIDDEN] - password: [HIDDEN] + test: '[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