Add flag for disabling config printing #269
Conversation
By default, ct prints the config on startup. This can be problematic because it may contain senstivie data when helm-repo-extra-args contains passwords. The new flag enables turning off config printing. In addition to that, the config is now printed to stderr. Signed-off-by: Reinhard Nägele <[email protected]>
ct/cmd/root.go
Outdated
| @@ -94,4 +94,5 @@ func addCommonLintAndInstallFlags(flags *pflag.FlagSet) { | |||
| flags.Bool("debug", false, heredoc.Doc(` | |||
| Print CLI calls of external tools to stdout (Note: depending on helm-extra-args | |||
| passed, this may reveal sensitive data)`)) | |||
| flags.Bool("print-config", true, "Prints the configuration to stderr") | |||
There was a problem hiding this comment.
This flag seems a bit not intuitive to me. The default is to print the config (this has a true value). So, you don't pass in the flag to skip it. You have to say it's equal to false. Would it be better to make this --skip-print-config with a value of false? Then use the not of this value for the LoadConfiguration function?
There was a problem hiding this comment.
Good point. Not sure what's better. Ideally, I'd like to change the default to false. I was hesitant to do so. Would that be considered a breaking change?
There was a problem hiding this comment.
Changing a default value is considered a breaking change. The helm client would only do that on a major version change.
There was a problem hiding this comment.
This is fun, because we already have a printConfig arg in LoadConfiguration(), but had no actual config/flag for it 😄
Given the security implications I think we should change chart-testing to not print the config values by default. Also thinking whatever flag name we decide is most intuitive for end uses should include a hard to miss note in the flag help/description about what you said initially @unguiculus. Something like:
caution: setting this may expose sensitive data when helm-repo-extra-args contains passwords
I'm a bit torn on the semver interpretation here. If it's considered a security bug, then changing output wouldn't require MAJOR. In that case, the additional flag to output it if desired would only require MINOR. Thoughts on this?
There was a problem hiding this comment.
I agree with @scottrigby. After all it's only a change in log output, not in functionality.
There was a problem hiding this comment.
Cool. Whatever we decide with semver, not printing this by default is the right thing to do
Signed-off-by: Reinhard Nägele <[email protected]>
Signed-off-by: Reinhard Nägele <[email protected]>
Signed-off-by: Reinhard Nägele <[email protected]>
By default, ct prints the config on startup. This can be problematic
because it may contain senstivie data when helm-repo-extra-args contains
passwords. The new flag enables turning off config printing.
Signed-off-by: Reinhard Nägele [email protected]