Skip to content

Commit 0d2cea7

Browse files
mumoshumumoshu
committed
fix: Make dry-run=server optional
This intends to fix a potential security issue introduced via #458 before cutting the next helm-diff release. Since #458 (unreleased), we had forced helm-diff to use `helm template --dry-run=server` for Helm 3.13 or greater. I think this can create an unintended security hole, where any users, who can run helm-diff via CI or any automation with an arbitrary chart and values, is able to view cluster resources via helm template's `lookup` functions. Previously this was impossible because `helm template` run by `helm diff` had no access to the `lookup` function. To fix this, we need to make `--dry-run=server` optional. And we do so by introducing a new flag `--dry-run=[|client|server]` to helm-diff. See the updated README and the updated helm-diff help message for more details.
1 parent c54f085 commit 0d2cea7

File tree

3 files changed

+129
-16
lines changed

3 files changed

+129
-16
lines changed

‎README.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,16 @@ Examples:
149149
# See https://github.com/databus23/helm-diff/issues/278 for more information.
150150
HELM_DIFF_IGNORE_UNKNOWN_FLAGS=true helm diff upgrade my-release stable/postgres --wait
151151
152+
# helm-diff uses helm-template without the cluster access by default.
153+
# In case you need the cluster access, you use HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN
154+
#
155+
# Set HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true to
156+
# use `helm template --dry-run=server` or
157+
# `helm upgrade --dry-run=server` (in case you also set `HELM_DIFF_USE_UPGRADE_DRY_RUN`)
158+
# See https://github.com/databus23/helm-diff/pull/458
159+
# for more information.
160+
HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true helm diff upgrade my-release datadog/datadog
161+
152162
# Set HELM_DIFF_USE_UPGRADE_DRY_RUN=true to
153163
# use `helm upgrade --dry-run` instead of `helm template` to render manifests from the chart.
154164
# See https://github.com/databus23/helm-diff/issues/253 for more information.

‎cmd/helm3.go‎

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
131131
// Let's simulate that in helm-diff.
132132
// See https://medium.com/@kcatstack/understand-helm-upgrade-flags-reset-values-reuse-values-6e58ac8f127e
133133
shouldDefaultReusingValues := isUpgrade && len(d.values) == 0 && len(d.stringValues) == 0 && len(d.valueFiles) == 0 && len(d.fileValues) == 0
134-
if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && !d.dryRun {
134+
if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && d.isRemoteAccessAllowed() {
135135
tmpfile, err := os.CreateTemp("", "existing-values")
136136
if err != nil {
137137
return nil, err
@@ -193,9 +193,28 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
193193
)
194194

195195
if d.useUpgradeDryRun {
196-
if d.dryRun {
197-
return nil, fmt.Errorf("`diff upgrade --dry-run` conflicts with HELM_DIFF_USE_UPGRADE_DRY_RUN_AS_TEMPLATE. Either remove --dry-run to enable cluster access, or unset HELM_DIFF_USE_UPGRADE_DRY_RUN_AS_TEMPLATE to make cluster access unnecessary")
198-
}
196+
//
197+
// # `--dry-run=client` or `--dry-run=server`?
198+
//
199+
// Or what's the relationoship between helm-diff's --dry-run flag,
200+
// HELM_DIFF_UPGRADE_DRY_RUN env var and the helm upgrade --dry-run flag?
201+
//
202+
// If the program reaches here,
203+
// we are sure that the user wants to user the `helm upgrade --dry-run` command
204+
// for generating the manifests to be diffed.
205+
//
206+
// However, which dry-run mode to use is still not clear.
207+
//
208+
// For compatibility with the old and new helm-diff options,
209+
// old and new helm, we assume that the user wants to use the `--dry-run=client` mode
210+
// if helm-diff has been invoked with any of the following flags:
211+
//
212+
// * --dry-run
213+
// * --dry-run=""
214+
// * --dry-run=client
215+
//
216+
// Otherwise, we assume that the user wants to use the `--dry-run=server` mode.
217+
//
199218

200219
if d.isAllowUnreleased() {
201220
// Otherwise you get the following error when this is a diff for a new install
@@ -213,7 +232,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
213232
return extractManifestFromHelmUpgradeDryRunOutput(s, d.noHooks)
214233
}
215234
} else {
216-
if !d.disableValidation && !d.dryRun {
235+
if !d.disableValidation && d.isRemoteAccessAllowed() {
217236
flags = append(flags, "--validate")
218237
}
219238

@@ -229,8 +248,26 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
229248
flags = append(flags, "--kube-version", d.kubeVersion)
230249
}
231250

251+
// To keep the full compatibility with older helm-diff versions,
252+
// we pass --dry-run to `helm template` only if Helm is greater than v3.13.0.
232253
if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService {
233-
flags = append(flags, "--dry-run=server")
254+
// See the fllowing link for more details:
255+
// - https://github.com/databus23/helm-diff/pull/458
256+
// - https://github.com/helm/helm/pull/9426#issuecomment-1501005666
257+
if d.dryRunMode == "server" {
258+
//
259+
// # This is for security reasons!
260+
//
261+
// We give helm-template the additional cluster access for the helm `lookup` function
262+
// only if the user has explicitly requested it by --dry-run=server,
263+
//
264+
// In other words, although helm-diff-upgrade implies limited cluster access by default,
265+
// helm-diff-upgrade without a --dry-run flag does NOT imply
266+
// full cluster-access via helm-template --dry-run=server!
267+
flags = append(flags, "--dry-run=server")
268+
} else {
269+
flags = append(flags, "--dry-run=client")
270+
}
234271
}
235272

236273
subcmd = "template"

‎cmd/upgrade.go‎

Lines changed: 76 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
jsoniterator "github.com/json-iterator/go"
1414
"github.com/pkg/errors"
1515
"github.com/spf13/cobra"
16+
"github.com/spf13/pflag"
1617
"helm.sh/helm/v3/pkg/action"
1718
"helm.sh/helm/v3/pkg/cli"
1819
"helm.sh/helm/v3/pkg/kube"
@@ -37,7 +38,8 @@ type diffCmd struct {
3738
devel bool
3839
disableValidation bool
3940
disableOpenAPIValidation bool
40-
dryRun bool
41+
dryRunMode string
42+
dryRunModeSpecified bool
4143
namespace string // namespace to assume the release to be installed into. Defaults to the current kube config namespace.
4244
valueFiles valueFiles
4345
values []string
@@ -67,6 +69,11 @@ func (d *diffCmd) isAllowUnreleased() bool {
6769
return d.allowUnreleased || d.install
6870
}
6971

72+
func (d *diffCmd) isRemoteAccessAllowed() bool {
73+
clientOnly := (d.dryRunModeSpecified && d.dryRunMode == "") || d.dryRunMode == "true" || d.dryRunMode == "client"
74+
return !clientOnly
75+
}
76+
7077
const globalUsage = `Show a diff explaining what a helm upgrade would change.
7178
7279
This fetches the currently deployed version of a release
@@ -84,9 +91,64 @@ func newChartCommand() *cobra.Command {
8491
}
8592

8693
cmd := &cobra.Command{
87-
Use: "upgrade [flags] [RELEASE] [CHART]",
88-
Short: "Show a diff explaining what a helm upgrade would change.",
89-
Long: globalUsage,
94+
Use: "upgrade [flags] [RELEASE] [CHART]",
95+
Short: "Show a diff explaining what a helm upgrade would change.",
96+
Long: globalUsage,
97+
DisableFlagParsing: true,
98+
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
99+
const (
100+
dryRunUsage = "--dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation." +
101+
" --dry-run=server enables the cluster access with helm-get and the lookup template function."
102+
)
103+
104+
legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError)
105+
legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage)
106+
if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun {
107+
diff.dryRunModeSpecified = true
108+
args = legacyDryRunFlagSet.Args()
109+
} else {
110+
cmd.Flags().StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage)
111+
}
112+
113+
fmt.Fprintf(os.Stderr, "args after legacy dry-run parsing: %v\n", args)
114+
115+
// Here we parse the flags ourselves so that we can support
116+
// both --dry-run and --dry-run=ARG syntaxes.
117+
//
118+
// If you don't do this, then cobra will treat --dry-run as
119+
// a "flag needs an argument: --dry-run" error.
120+
//
121+
// This works becase we have `DisableFlagParsing: true`` above.
122+
// Never turn that off, or you'll get the error again.
123+
if err := cmd.Flags().Parse(args); err != nil {
124+
return err
125+
}
126+
127+
args = cmd.Flags().Args()
128+
129+
if !diff.dryRunModeSpecified {
130+
dryRunModeSpecified := cmd.Flags().Changed("dry-run")
131+
diff.dryRunModeSpecified = dryRunModeSpecified
132+
133+
if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") {
134+
return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode)
135+
}
136+
}
137+
138+
cmd.SetArgs(args)
139+
140+
// We have to do this here because we have to sort out the
141+
// --dry-run flag ambiguity to determine the args to be checked.
142+
//
143+
// In other words, we can't just do:
144+
//
145+
// cmd.Args = func(cmd *cobra.Command, args []string) error {
146+
// return checkArgsLength(len(args), "release name", "chart path")
147+
// }
148+
//
149+
// Because it seems to take precedence over the PersistentPreRunE
150+
return checkArgsLength(len(args), "release name", "chart path")
151+
},
90152
Example: strings.Join([]string{
91153
" helm diff upgrade my-release stable/postgresql --values values.yaml",
92154
"",
@@ -95,6 +157,14 @@ func newChartCommand() *cobra.Command {
95157
" # See https://github.com/databus23/helm-diff/issues/278 for more information.",
96158
" HELM_DIFF_IGNORE_UNKNOWN_FLAGS=true helm diff upgrade my-release stable/postgres --wait",
97159
"",
160+
" # helm-diff disallows the use of the `lookup` function by default.",
161+
" # To enable it, you must set HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true to",
162+
" # use `helm template --dry-run=server` or",
163+
" # `helm upgrade --dry-run=server` (in case you also set `HELM_DIFF_USE_UPGRADE_DRY_RUN`)",
164+
" # See https://github.com/databus23/helm-diff/pull/458",
165+
" # for more information.",
166+
" HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true helm diff upgrade my-release datadog/datadog",
167+
"",
98168
" # Set HELM_DIFF_USE_UPGRADE_DRY_RUN=true to",
99169
" # use `helm upgrade --dry-run` instead of `helm template` to render manifests from the chart.",
100170
" # See https://github.com/databus23/helm-diff/issues/253 for more information.",
@@ -117,9 +187,6 @@ func newChartCommand() *cobra.Command {
117187
"# Read the flag usage below for more information on --context.",
118188
"HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog",
119189
}, "\n"),
120-
Args: func(cmd *cobra.Command, args []string) error {
121-
return checkArgsLength(len(args), "release name", "chart path")
122-
},
123190
RunE: func(cmd *cobra.Command, args []string) error {
124191
// Suppress the command usage on error. See #77 for more info
125192
cmd.SilenceUsage = true
@@ -193,7 +260,6 @@ func newChartCommand() *cobra.Command {
193260
f.BoolVar(&diff.devel, "devel", false, "use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored.")
194261
f.BoolVar(&diff.disableValidation, "disable-validation", false, "disables rendered templates validation against the Kubernetes cluster you are currently pointing to. This is the same validation performed on an install")
195262
f.BoolVar(&diff.disableOpenAPIValidation, "disable-openapi-validation", false, "disables rendered templates validation against the Kubernetes OpenAPI Schema")
196-
f.BoolVar(&diff.dryRun, "dry-run", false, "disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation")
197263
f.StringVar(&diff.postRenderer, "post-renderer", "", "the path to an executable to be used for post rendering. If it exists in $PATH, the binary will be used, otherwise it will try to look for the executable at the given path")
198264
f.StringArrayVar(&diff.postRendererArgs, "post-renderer-args", []string{}, "an argument to the post-renderer (can specify multiple)")
199265
f.BoolVar(&diff.insecureSkipTLSVerify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download")
@@ -213,7 +279,7 @@ func (d *diffCmd) runHelm3() error {
213279

214280
var err error
215281

216-
if !d.dryRun {
282+
if d.isRemoteAccessAllowed() {
217283
releaseManifest, err = getRelease(d.release, d.namespace)
218284
}
219285

@@ -260,7 +326,7 @@ func (d *diffCmd) runHelm3() error {
260326
}
261327

262328
currentSpecs := make(map[string]*manifest.MappingResult)
263-
if !newInstall && !d.dryRun {
329+
if !newInstall && d.isRemoteAccessAllowed() {
264330
if !d.noHooks && !d.threeWayMerge {
265331
hooks, err := getHooks(d.release, d.namespace)
266332
if err != nil {

0 commit comments

Comments
 (0)