Repository navigation
Conversation
Helm registry login passes --password on the argv. execute() joined those args into the error string, which ProcessHelmChart writes onto the virtual kube-system vcluster-deploy ConfigMap. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
blacksoxx
left a comment
blacksoxx
left a comment
There was a problem hiding this comment.
Thanks for catching this, a password in a ConfigMap is an easy one to miss. I checked out the branch: go test ./pkg/helm/ is green, vet and the repo's lint config are clean on the package, and TestRedactHelmArgs cannot pass on main since the function does not exist there.
Traced the exposure path to confirm the description: loginArgs in helm.go:225 is the only place the password enters argv, execute is the only place argv is logged or put into an error (the debug lines for delete and get-values never carry it), and pkg/controllers/deploy/deploy.go:150 copies lastError.Error() into the status message of the deploy ConfigMap. So redacting in execute closes it.
Looks good to me. One optional thought, not a blocker: --password-stdin would remove the value from argv entirely and make this class of leak impossible, but that touches how login invokes helm, so a separate change if the maintainers want it.
What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
What does this pull request do? Which issues does it resolve?
helm registry loginis invoked with--passwordon the argv.execute()joined those args into the error string.ProcessHelmChartthen writeserr.Error()onto the virtualkube-system/vcluster-deployConfigMap, so anyone who can read that ConfigMap (or debug logs) sees the registry password after a failed login.Registry login with
--passwordwas added in #1043 (2023-05-18).This redacts
--password <value>and--password=<value>in the error and debug strings. The argv passed tohelmis unchanged.--password-stdinis left alone.Please provide a short message that should be published in the vcluster release notes
Fixed an issue where a failed Helm registry login could write the registry password into the virtual vcluster-deploy ConfigMap.
What else do we need to know?
Related pattern: kubernetes/kubernetes#71149 sanitizes secrets in
rest.Configstring output.Validation:
go test ./pkg/helm/ -run TestRedactHelmArgsfailed on main (redactHelmArgs() = "... --password s3cret ...")Note
Low Risk
Logging/error-string-only change with no behavior change to Helm invocations; reduces exposure of registry credentials on failure paths.
Overview
Fixes a credential leak when Helm registry login fails:
execute()now formats debug logs and error messages throughredactHelmArgs, which masks--passwordvalues (separate arg or--password=form) while leaving the real argv passed tohelmunchanged.--password-stdinis not treated as a secret value.Adds
TestRedactHelmArgscovering common flag shapes and asserting passwords do not appear in the redacted string.Reviewed by Cursor Bugbot for commit 4791942. Bugbot is set up for automated code reviews on this repo. Configure here.