Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Add Log Sanitization for Secret Kind in 'Helm Upgrade' Command #12183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

daniel-hutao
Copy link

@daniel-hutao daniel-hutao commented Jul 4, 2023

What this PR does / why we need it:

fix #12176 #11727

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

I have thoroughly tested it locally and have written corresponding unit tests. Please take a review at this pr.

How to Test This Modification Locally

  1. Create a secret.yaml
apiVersion: v1
kind: Secret
metadata:
  name: {{ .Release.Name }}-secret
type: Opaque
data:
  secretKey: {{ .Values.secret.secretKey }}
  1. Set secretKey in values.yaml
secret:
  secretKey: "SGVsbG8gd29ybGQ="  # This is "Hello world" in base64
  # secretKey: "hello"  # This is invalid base64 string
  1. Execute helm install test-release .

  2. Update values.yaml

secret:
  # secretKey: "SGVsbG8gd29ybGQ="  # This is "Hello world" in base64
  secretKey: "hello"  # This is invalid base64 string
  1. Execute helm upgrade test-release . before changing the code
$ helm upgrade test-release .
Error: UPGRADE FAILED: cannot patch "test-release-secret" with kind Secret:  "" is invalid: patch: Invalid value: "{\"apiVersion\":\"v1\",\"data\":{\"secretKey\":\"hello\"},\"kind\":\"Secret\",\"metadata\":{\"annotations\":{\"meta.helm.sh/release-name\":\"test-release\",\"meta.helm.sh/release-namespace\":\"default\"},\"creationTimestamp\":\"2023-07-04T06:51:10Z\",\"labels\":{\"app.kubernetes.io/managed-by\":\"Helm\"},\"managedFields\":[{\"manager\":\"helm\",\"operation\":\"Update\",\"apiVersion\":\"v1\",\"time\":\"2023-07-04T06:51:10Z\",\"fieldsType\":\"FieldsV1\",\"fieldsV1\":{\"f:data\":{\".\":{},\"f:secretKey\":{}},\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:meta.helm.sh/release-name\":{},\"f:meta.helm.sh/release-namespace\":{}},\"f:labels\":{\".\":{},\"f:app.kubernetes.io/managed-by\":{}}},\"f:type\":{}}}],\"name\":\"test-release-secret\",\"namespace\":\"default\",\"resourceVersion\":\"4030333\",\"uid\":\"b2faa643-1711-4bc3-9219-c1f8a32306be\"},\"type\":\"Opaque\"}": illegal base64 data at input byte 4

Pay attention to {\"secretKey\":\"hello\"}

  1. Execute helm upgrade test-release . after changing the Code
$ ./helm upgrade test-release /Users/danielhu/Work/test/test-chart
Error: UPGRADE FAILED: cannot patch "test-release-secret" with kind Secret:  "" is invalid: patch: Invalid value: "{\"apiVersion\":\"v1\",\"data\":{\"secretKey\":\"***\"},\"kind\":\"Secret\",\"metadata\":{\"annotations\":{\"meta.helm.sh/release-name\":\"test-release\",\"meta.helm.sh/release-namespace\":\"default\"},\"creationTimestamp\":\"2023-07-04T13:42:20Z\",\"labels\":{\"app.kubernetes.io/managed-by\":\"Helm\"},\"managedFields\":[{\"manager\":\"helm\",\"operation\":\"Update\",\"apiVersion\":\"v1\",\"time\":\"2023-07-04T13:42:20Z\",\"fieldsType\":\"FieldsV1\",\"fieldsV1\":{\"f:data\":{\".\":{},\"f:secretKey\":{}},\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:meta.helm.sh/release-name\":{},\"f:meta.helm.sh/release-namespace\":{}},\"f:labels\":{\".\":{},\"f:app.kubernetes.io/managed-by\":{}}},\"f:type\":{}}}],\"name\":\"test-release-secret\",\"namespace\":\"default\",\"resourceVersion\":\"4119312\",\"uid\":\"c90737ad-58c8-4c0e-aadf-b0f4dd4c85f8\"},\"type\":\"Opaque\"}": illegal base64 data at input byte 4

Pay attention to {\"secretKey\":\"***\"}

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 4, 2023
pkg/kube/client.go Outdated Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Jul 5, 2023

it is better to add a flag to enable this feature. maybe it's a breaking change currently. @daniel-hutao

@daniel-hutao
Copy link
Author

it is better to add a flag to enable this feature. maybe it's a breaking change currently. @daniel-hutao

I'm not sure if it's necessary to add a flag, as it's just a desensitization of the error log; if it is, then maybe we could add a --log-desensitization flag. But should the flag be added to the root command or to the upgrade sub-command? It seems to make more sense under root. @yxxhero

- Added a new function `desensitizeLog` to sanitize logs by replacing sensitive data in a Secret with "***".
- Modified the `updateResource` function to use `desensitizeLog` when the kind is "Secret".
- Added a test case for the `desensitizeLog` function to ensure it works as expected.

Signed-off-by: Daniel Hu <tao.hu@merico.dev>
@biwwy0
Copy link

biwwy0 commented Nov 14, 2023

Any plans on merging this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm error message can leak sensitive data such as credentials on upgrade error
3 participants