fix: Make dry-run=server optional (#499)

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 changing helm-diff's `--dry-run` flag to accept only only booleans but also `client` and `server`. The updated flag usage is `--dry-run[=[|true|false|client|server]]`.

See the updated README and the updated helm-diff help message for more details.

Author: mumoshu

README.md

@@ -149,6 +149,14 @@ Examples:
   # See https://github.com/databus23/helm-diff/issues/278 for more information.
   HELM_DIFF_IGNORE_UNKNOWN_FLAGS=true helm diff upgrade my-release stable/postgres --wait
 
+  # helm-diff disallows the use of the `lookup` function by default.
+  # To enable it, you must set HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true to
+  # use `helm template --dry-run=server` or
+  # `helm upgrade --dry-run=server` (in case you also set `HELM_DIFF_USE_UPGRADE_DRY_RUN`)
+  # See https://github.com/databus23/helm-diff/pull/458
+  # for more information.
+  HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true helm diff upgrade my-release datadog/datadog
+
   # Set HELM_DIFF_USE_UPGRADE_DRY_RUN=true to
   # use `helm upgrade --dry-run` instead of `helm template` to render manifests from the chart.
   # See https://github.com/databus23/helm-diff/issues/253 for more information.

cmd/helm3.go

@@ -14,8 +14,9 @@ import (
 )
 
 var (
-	helmVersionRE                         = regexp.MustCompile(`Version:\s*"([^"]+)"`)
-	minHelmVersion                        = semver.MustParse("v3.1.0-rc.1")
+	helmVersionRE  = regexp.MustCompile(`Version:\s*"([^"]+)"`)
+	minHelmVersion = semver.MustParse("v3.1.0-rc.1")
+	// See https://github.com/helm/helm/pull/9426
 	minHelmVersionWithDryRunLookupSupport = semver.MustParse("v3.13.0")
 )
 
@@ -131,7 +132,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 	// Let's simulate that in helm-diff.
 	// See https://medium.com/@kcatstack/understand-helm-upgrade-flags-reset-values-reuse-values-6e58ac8f127e
 	shouldDefaultReusingValues := isUpgrade && len(d.values) == 0 && len(d.stringValues) == 0 && len(d.jsonValues) == 0 && len(d.valueFiles) == 0 && len(d.fileValues) == 0
-	if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && !d.dryRun {
+	if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && d.clusterAccessAllowed() {
 		tmpfile, err := os.CreateTemp("", "existing-values")
 		if err != nil {
 			return nil, err
@@ -195,17 +196,28 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 		filter func([]byte) []byte
 	)
 
+	// `--dry-run=client` or `--dry-run=server`?
+	//
+	// Or what's the relationoship between helm-diff's --dry-run flag,
+	// HELM_DIFF_UPGRADE_DRY_RUN env var and the helm upgrade --dry-run flag?
+	//
+	// Read on to find out.
 	if d.useUpgradeDryRun {
-		if d.dryRun {
-			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")
-		}
-
 		if d.isAllowUnreleased() {
 			// Otherwise you get the following error when this is a diff for a new install
 			//   Error: UPGRADE FAILED: "$RELEASE_NAME" has no deployed releases
 			flags = append(flags, "--install")
 		}
 
+		// If the program reaches here,
+		// we are sure that the user wants to user the `helm upgrade --dry-run` command
+		// for generating the manifests to be diffed.
+		//
+		// So the question is only whether to use `--dry-run=client` or `--dry-run=server`.
+		//
+		// As HELM_DIFF_UPGRADE_DRY_RUN is there for producing more complete and correct diff results,
+		// we use --dry-run=server if the version of helm supports it.
+		// Otherwise, we use --dry-run=client, as that's the best we can do.
 		if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService {
 			flags = append(flags, "--dry-run=server")
 		} else {
@@ -216,7 +228,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 			return extractManifestFromHelmUpgradeDryRunOutput(s, d.noHooks)
 		}
 	} else {
-		if !d.disableValidation && !d.dryRun {
+		if !d.disableValidation && d.clusterAccessAllowed() {
 			flags = append(flags, "--validate")
 		}
 
@@ -232,8 +244,46 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 			flags = append(flags, "--kube-version", d.kubeVersion)
 		}
 
+		// To keep the full compatibility with older helm-diff versions,
+		// we pass --dry-run to `helm template` only if Helm is greater than v3.13.0.
 		if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService {
-			flags = append(flags, "--dry-run=server")
+			// However, which dry-run mode to use is still not clear.
+			//
+			// For compatibility with the old and new helm-diff options,
+			// old and new helm, we assume that the user wants to use the older `helm template --dry-run=client` mode
+			// if helm-diff has been invoked with any of the following flags:
+			//
+			// * no dry-run flags (to be consistent with helm-template)
+			// * --dry-run
+			// * --dry-run=""
+			// * --dry-run=client
+			//
+			// and the newer `helm template --dry-run=server` mode when invoked with:
+			//
+			// * --dry-run=server
+			//
+			// Any other values should result in errors.
+			//
+			// See the fllowing link for more details:
+			// - https://github.com/databus23/helm-diff/pull/458
+			// - https://github.com/helm/helm/pull/9426#issuecomment-1501005666
+			if d.dryRunMode == "server" {
+				// This is for security reasons!
+				//
+				// We give helm-template the additional cluster access for the helm `lookup` function
+				// only if the user has explicitly requested it by --dry-run=server,
+				//
+				// In other words, although helm-diff-upgrade implies limited cluster access by default,
+				// helm-diff-upgrade without a --dry-run flag does NOT imply
+				// full cluster-access via helm-template --dry-run=server!
+				flags = append(flags, "--dry-run=server")
+			} else {
+				// Since helm-diff 3.9.0 and helm 3.13.0, we pass --dry-run=client to `helm template` by default.
+				// This doesn't make any difference for helm-diff itself,
+				// because helm-template w/o flags is equivalent to helm-template --dry-run=client.
+				// See https://github.com/helm/helm/pull/9426#discussion_r1181397259
+				flags = append(flags, "--dry-run=client")
+			}
 		}
 
 		subcmd = "template"

cmd/upgrade.go

@@ -13,6 +13,7 @@ import (
 	jsoniterator "github.com/json-iterator/go"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/kube"
@@ -37,7 +38,8 @@ type diffCmd struct {
 	devel                    bool
 	disableValidation        bool
 	disableOpenAPIValidation bool
-	dryRun                   bool
+	dryRunMode               string
+	dryRunModeSpecified      bool
 	namespace                string // namespace to assume the release to be installed into. Defaults to the current kube config namespace.
 	valueFiles               valueFiles
 	values                   []string
@@ -68,6 +70,28 @@ func (d *diffCmd) isAllowUnreleased() bool {
 	return d.allowUnreleased || d.install
 }
 
+// clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree.
+//
+// helm-diff basically have 3 modes of operation:
+// 1. no cluster access at all when --dry-run, --dry-run=client, or --dry-run=true is specified.
+// 2. basic cluster access when --dry-run is not specified.
+// 3. extra cluster access when --dry-run=server is specified.
+//
+// clusterAccessAllowed returns true when the mode is either 2 or 3.
+//
+// If false, helm-diff should not access the cluster at all.
+// More concretely:
+// - It shouldn't pass --validate to helm-template because it requires cluster access.
+// - It shouldn't get the current release manifest using helm-get-manifest because it requires cluster access.
+// - It shouldn't get the current release hooks using helm-get-hooks because it requires cluster access.
+// - It shouldn't get the current release values using helm-get-values because it requires cluster access.
+//
+// See also https://github.com/helm/helm/pull/9426#discussion_r1181397259
+func (d *diffCmd) clusterAccessAllowed() bool {
+	clientOnly := (d.dryRunModeSpecified && d.dryRunMode == "") || d.dryRunMode == "true" || d.dryRunMode == "client"
+	return !clientOnly
+}
+
 const globalUsage = `Show a diff explaining what a helm upgrade would change.
 
 This fetches the currently deployed version of a release
@@ -85,9 +109,10 @@ func newChartCommand() *cobra.Command {
 	}
 
 	cmd := &cobra.Command{
-		Use:   "upgrade [flags] [RELEASE] [CHART]",
-		Short: "Show a diff explaining what a helm upgrade would change.",
-		Long:  globalUsage,
+		Use:                "upgrade [flags] [RELEASE] [CHART]",
+		Short:              "Show a diff explaining what a helm upgrade would change.",
+		Long:               globalUsage,
+		DisableFlagParsing: true,
 		Example: strings.Join([]string{
 			"  helm diff upgrade my-release stable/postgresql --values values.yaml",
 			"",
@@ -96,6 +121,14 @@ func newChartCommand() *cobra.Command {
 			"  # See https://github.com/databus23/helm-diff/issues/278 for more information.",
 			"  HELM_DIFF_IGNORE_UNKNOWN_FLAGS=true helm diff upgrade my-release stable/postgres --wait",
 			"",
+			"  # helm-diff disallows the use of the `lookup` function by default.",
+			"  # To enable it, you must set HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true to",
+			"  # use `helm template --dry-run=server` or",
+			"  # `helm upgrade --dry-run=server` (in case you also set `HELM_DIFF_USE_UPGRADE_DRY_RUN`)",
+			"  # See https://github.com/databus23/helm-diff/pull/458",
+			"  # for more information.",
+			"  HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true helm diff upgrade my-release datadog/datadog",
+			"",
 			"  # Set HELM_DIFF_USE_UPGRADE_DRY_RUN=true to",
 			"  # use `helm upgrade --dry-run` instead of `helm template` to render manifests from the chart.",
 			"  # See https://github.com/databus23/helm-diff/issues/253 for more information.",
@@ -118,10 +151,70 @@ func newChartCommand() *cobra.Command {
 			"# Read the flag usage below for more information on --context.",
 			"HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog",
 		}, "\n"),
-		Args: func(cmd *cobra.Command, args []string) error {
-			return checkArgsLength(len(args), "release name", "chart path")
-		},
 		RunE: func(cmd *cobra.Command, args []string) error {
+			// Note that we can't just move this block to PersistentPreRunE,
+			// because cmd.SetArgs(args) does not persist between PersistentPreRunE and RunE.
+			// The choice is between:
+			// 1. Doing this in RunE
+			// 2. Doing this in PersistentPreRunE, saving args somewhere, and calling cmd.SetArgs(args) again in RunE
+			// 2 is more complicated without much benefit, so we choose 1.
+			{
+				const (
+					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." +
+						" --dry-run=server enables the cluster access with helm-get and the lookup template function."
+				)
+
+				legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError)
+				legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage)
+				if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun {
+					diff.dryRunModeSpecified = true
+					args = legacyDryRunFlagSet.Args()
+				} else {
+					cmd.Flags().StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage)
+				}
+
+				fmt.Fprintf(os.Stderr, "args after legacy dry-run parsing: %v\n", args)
+
+				// Here we parse the flags ourselves so that we can support
+				// both --dry-run and --dry-run=ARG syntaxes.
+				//
+				// If you don't do this, then cobra will treat --dry-run as
+				// a "flag needs an argument: --dry-run" error.
+				//
+				// This works becase we have `DisableFlagParsing: true`` above.
+				// Never turn that off, or you'll get the error again.
+				if err := cmd.Flags().Parse(args); err != nil {
+					return err
+				}
+
+				args = cmd.Flags().Args()
+
+				if !diff.dryRunModeSpecified {
+					dryRunModeSpecified := cmd.Flags().Changed("dry-run")
+					diff.dryRunModeSpecified = dryRunModeSpecified
+
+					if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") {
+						return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode)
+					}
+				}
+
+				cmd.SetArgs(args)
+
+				// We have to do this here because we have to sort out the
+				// --dry-run flag ambiguity to determine the args to be checked.
+				//
+				// In other words, we can't just do:
+				//
+				// cmd.Args = func(cmd *cobra.Command, args []string) error {
+				//     return checkArgsLength(len(args), "release name", "chart path")
+				// }
+				//
+				// Because it seems to take precedence over the PersistentPreRunE
+				if err := checkArgsLength(len(args), "release name", "chart path"); err != nil {
+					return err
+				}
+			}
+
 			// Suppress the command usage on error. See #77 for more info
 			cmd.SilenceUsage = true
 
@@ -195,7 +288,6 @@ func newChartCommand() *cobra.Command {
 	f.BoolVar(&diff.devel, "devel", false, "use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored.")
 	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")
 	f.BoolVar(&diff.disableOpenAPIValidation, "disable-openapi-validation", false, "disables rendered templates validation against the Kubernetes OpenAPI Schema")
-	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")
 	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")
 	f.StringArrayVar(&diff.postRendererArgs, "post-renderer-args", []string{}, "an argument to the post-renderer (can specify multiple)")
 	f.BoolVar(&diff.insecureSkipTLSVerify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download")
@@ -215,7 +307,7 @@ func (d *diffCmd) runHelm3() error {
 
 	var err error
 
-	if !d.dryRun {
+	if d.clusterAccessAllowed() {
 		releaseManifest, err = getRelease(d.release, d.namespace)
 	}
 
@@ -262,7 +354,7 @@ func (d *diffCmd) runHelm3() error {
 	}
 
 	currentSpecs := make(map[string]*manifest.MappingResult)
-	if !newInstall && !d.dryRun {
+	if !newInstall && d.clusterAccessAllowed() {
 		if !d.noHooks && !d.threeWayMerge {
 			hooks, err := getHooks(d.release, d.namespace)
 			if err != nil {

cmd/upgrade_test.go

@@ -0,0 +1,58 @@
+package cmd
+
+import "testing"
+
+func TestIsRemoteAccessAllowed(t *testing.T) {
+	cases := []struct {
+		name     string
+		cmd      diffCmd
+		expected bool
+	}{
+		{
+			name:     "no flags",
+			cmd:      diffCmd{},
+			expected: true,
+		},
+		{
+			name: "legacy explicit dry-run flag",
+			cmd: diffCmd{
+				dryRunModeSpecified: true,
+				dryRunMode:          "true",
+			},
+			expected: false,
+		},
+		{
+			name: "legacy empty dry-run flag",
+			cmd: diffCmd{
+				dryRunModeSpecified: true,
+				dryRunMode:          "",
+			},
+			expected: false,
+		},
+		{
+			name: "server-side dry-run flag",
+			cmd: diffCmd{
+				dryRunModeSpecified: true,
+				dryRunMode:          "server",
+			},
+			expected: true,
+		},
+		{
+			name: "client-side dry-run flag",
+			cmd: diffCmd{
+				dryRunModeSpecified: true,
+				dryRunMode:          "client",
+			},
+			expected: false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := tc.cmd.clusterAccessAllowed()
+			if actual != tc.expected {
+				t.Errorf("Expected %v, got %v", tc.expected, actual)
+			}
+		})
+	}
+}