From 6bae70b61e164897cc7cded53fd9a292ff22acdd Mon Sep 17 00:00:00 2001 From: Jim Robinson <1643772+jimmyR@users.noreply.github.com> Date: Sat, 7 Mar 2026 14:11:08 +0000 Subject: [PATCH] feat: add HELMFILE_DISABLE_VALS env var to skip vals processing Signed-off-by: Jim Robinson <1643772+jimmyR@users.noreply.github.com> --- docs/index.md | 2 + docs/remote-secrets.md | 62 ++++++++- pkg/envvar/const.go | 2 + pkg/plugins/vals.go | 88 ++++++++++++- pkg/plugins/vals_test.go | 231 +++++++++++++++++++++++++++++++++- pkg/tmpl/expand_secret_ref.go | 9 +- 6 files changed, 380 insertions(+), 14 deletions(-) diff --git a/docs/index.md b/docs/index.md index c553226f..9c728608 100644 --- a/docs/index.md +++ b/docs/index.md @@ -581,6 +581,8 @@ Helmfile uses some OS environment variables to override default behaviour: * `HELMFILE_DISABLE_INSECURE_FEATURES` - disable insecure features, expecting `true` lower case * `HELMFILE_DISABLE_RUNNER_UNIQUE_ID` - disable unique logging ID, expecting any non-empty value +* `HELMFILE_DISABLE_VALS` - disable internal vals processing, `ref+` values pass through unchanged for use with external vals, expecting `true` lower case +* `HELMFILE_DISABLE_VALS_STRICT` - disable vals and error if any `ref+` values are detected, expecting `true` lower case * `HELMFILE_SKIP_INSECURE_TEMPLATE_FUNCTIONS` - disable insecure template functions, expecting `true` lower case * `HELMFILE_USE_HELM_STATUS_TO_CHECK_RELEASE_EXISTENCE` - expecting non-empty value to use `helm status` to check release existence, instead of `helm list` which is the default behaviour * `HELMFILE_EXPERIMENTAL` - enable experimental features, expecting `true` lower case diff --git a/docs/remote-secrets.md b/docs/remote-secrets.md index 0b75b8ef..b07f65c4 100644 --- a/docs/remote-secrets.md +++ b/docs/remote-secrets.md @@ -54,4 +54,64 @@ service: login: svc-login # fetched from vault password: pass -``` \ No newline at end of file +``` + + +## Disabling vals + +You can disable the built-in vals processing using environment variables: + +### Pass-through mode + +Set `HELMFILE_DISABLE_VALS=true` to disable internal vals processing. Any `ref+` values will pass through unchanged, allowing you to validate them with a policy tool such as [conftest](https://www.conftest.dev/) before they are resolved: + +```bash +HELMFILE_DISABLE_VALS=true helmfile template | conftest test - +``` + +### Strict mode + +Set `HELMFILE_DISABLE_VALS_STRICT=true` to disable vals and error if any `ref+` values are detected. This is useful when you want to prevent users from using vals references: + +```bash +HELMFILE_DISABLE_VALS_STRICT=true helmfile sync +# Error: vals is disabled via HELMFILE_DISABLE_VALS_STRICT environment variable +``` + +Note: If both are set, strict mode takes precedence. + +### Validating ref+ expressions with conftest + +You can use `HELMFILE_DISABLE_VALS=true` with [conftest](https://www.conftest.dev/) to validate that all `ref+` expressions conform to your security policy before processing them. + +Example rego policy (`policy/vals_refs.rego`): + +```rego +package main + +allowed_refs := { + "ref+tfstates3://my-terraform-state/networking/eu-west-2/vpc/vpc_id", + "ref+tfstates3://my-terraform-state/networking/eu-west-2/vpc/private_subnet_ids", + "ref+tfstates3://my-terraform-state/platform/eu-west-2/eks/cluster_endpoint", +} + +deny[msg] { + value := input[_] + startswith(value, "ref+tfstates3://") + not allowed_refs[value] + msg := sprintf("ref+ expression references an unapproved tfstates3 URI: %s", [value]) +} + +deny[msg] { + value := input[_] + startswith(value, "ref+") + not startswith(value, "ref+tfstates3://") + msg := sprintf("only tfstates3 ref+ expressions are permitted, got: %s", [value]) +} +``` + +Run against your rendered values: + +```bash +HELMFILE_DISABLE_VALS=true helmfile template | conftest test - +``` diff --git a/pkg/envvar/const.go b/pkg/envvar/const.go index b5df7100..248b2357 100644 --- a/pkg/envvar/const.go +++ b/pkg/envvar/const.go @@ -2,6 +2,8 @@ package envvar const ( DisableInsecureFeatures = "HELMFILE_DISABLE_INSECURE_FEATURES" + DisableVals = "HELMFILE_DISABLE_VALS" // pass-through ref+ for external vals + DisableValsStrict = "HELMFILE_DISABLE_VALS_STRICT" // error on ref+ // use helm status to check if a release exists before installing it UseHelmStatusToCheckReleaseExistence = "HELMFILE_USE_HELM_STATUS_TO_CHECK_RELEASE_EXISTENCE" diff --git a/pkg/plugins/vals.go b/pkg/plugins/vals.go index 8c65ceaf..752065a7 100644 --- a/pkg/plugins/vals.go +++ b/pkg/plugins/vals.go @@ -1,8 +1,10 @@ package plugins import ( + "errors" "io" "os" + "strconv" "strings" "sync" @@ -16,12 +18,94 @@ const ( valsCacheSize = 512 ) -var instance *vals.Runtime +var instance vals.Evaluator var once sync.Once -func ValsInstance() (*vals.Runtime, error) { +var ErrValsDisabled = errors.New("vals is disabled via HELMFILE_DISABLE_VALS_STRICT environment variable") + +// passthroughEvaluator passes values through unchanged (for external vals) +type passthroughEvaluator struct{} + +func (p *passthroughEvaluator) Eval(m map[string]any) (map[string]any, error) { + return normalizeMap(m), nil +} + +// strictEvaluator passes through values but errors if ref+ is detected +type strictEvaluator struct{} + +func (s *strictEvaluator) Eval(m map[string]any) (map[string]any, error) { + if containsRefPlus(m) { + return nil, ErrValsDisabled + } + return normalizeMap(m), nil +} + +// normalizeMap converts []string values to []any to match vals.Eval behavior. +func normalizeMap(m map[string]any) map[string]any { + out := make(map[string]any, len(m)) + for k, v := range m { + if ss, ok := v.([]string); ok { + a := make([]any, len(ss)) + for i, s := range ss { + a[i] = s + } + out[k] = a + } else { + out[k] = v + } + } + return out +} + +func containsRefPlus(v any) bool { + switch val := v.(type) { + case string: + return strings.Contains(val, "ref+") + case map[string]any: + for _, v := range val { + if containsRefPlus(v) { + return true + } + } + case map[any]any: + for _, v := range val { + if containsRefPlus(v) { + return true + } + } + case []any: + for _, v := range val { + if containsRefPlus(v) { + return true + } + } + case []string: + for _, s := range val { + if strings.Contains(s, "ref+") { + return true + } + } + } + return false +} + +func ValsInstance() (vals.Evaluator, error) { var err error once.Do(func() { + // HELMFILE_DISABLE_VALS_STRICT: error on ref+ usage + strict, _ := strconv.ParseBool(os.Getenv(envvar.DisableValsStrict)) + if strict { + instance = &strictEvaluator{} + return + } + + // HELMFILE_DISABLE_VALS: pass-through for external vals + disabled, _ := strconv.ParseBool(os.Getenv(envvar.DisableVals)) + if disabled { + instance = &passthroughEvaluator{} + return + } + // Configure AWS SDK logging via HELMFILE_AWS_SDK_LOG_LEVEL environment variable // Default: "off" to prevent sensitive information (tokens, auth headers) from being exposed // See issue #2270 and vals PR helmfile/vals#893 diff --git a/pkg/plugins/vals_test.go b/pkg/plugins/vals_test.go index 31d71d40..a16afb07 100644 --- a/pkg/plugins/vals_test.go +++ b/pkg/plugins/vals_test.go @@ -4,6 +4,7 @@ import ( "io" "os" "strings" + "sync" "testing" "github.com/helmfile/vals" @@ -11,20 +12,244 @@ import ( "github.com/helmfile/helmfile/pkg/envvar" ) -func TestValsInstance(t *testing.T) { - i, err := ValsInstance() +// resetInstance resets the singleton for testing +func resetInstance() { + instance = nil + once = sync.Once{} +} +func TestValsInstance(t *testing.T) { + resetInstance() + defer resetInstance() + + i, err := ValsInstance() if err != nil { t.Errorf("unexpected error: %v", err) } i2, _ := ValsInstance() - if i != i2 { t.Error("Instances should be equal") } } +func TestDisableVals(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableVals, "true") + defer os.Unsetenv(envvar.DisableVals) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should pass through values unchanged + input := map[string]any{"key": "ref+echo://secret"} + output, err := evaluator.Eval(input) + if err != nil { + t.Fatalf("passthrough should not error: %v", err) + } + + if output["key"] != "ref+echo://secret" { + t.Errorf("expected ref+ to pass through unchanged, got %v", output["key"]) + } +} + +func TestDisableValsStrict(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableValsStrict, "true") + defer os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should error on ref+ + input := map[string]any{"key": "ref+echo://secret"} + _, err = evaluator.Eval(input) + if err == nil { + t.Fatal("strict mode should error on ref+") + } + if err != ErrValsDisabled { + t.Errorf("expected ErrValsDisabled, got %v", err) + } +} + +func TestDisableValsStrictAllowsNonRef(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableValsStrict, "true") + defer os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should pass through non-ref+ values + input := map[string]any{"key": "normal-value"} + output, err := evaluator.Eval(input) + if err != nil { + t.Fatalf("strict mode should allow non-ref+ values: %v", err) + } + if output["key"] != "normal-value" { + t.Errorf("expected value to pass through, got %v", output["key"]) + } +} + +func TestDisableValsStrictNestedRef(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableValsStrict, "true") + defer os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should detect nested ref+ in map[string]any + input := map[string]any{ + "outer": map[string]any{ + "inner": "ref+vault://secret", + }, + } + _, err = evaluator.Eval(input) + if err == nil { + t.Fatal("strict mode should detect nested ref+") + } +} + +func TestDisableValsStrictMapAnyAny(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableValsStrict, "true") + defer os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should detect ref+ inside map[any]any (yaml.v2 nested maps) + input := map[string]any{ + "outer": map[any]any{ + "inner": "ref+vault://secret", + }, + } + _, err = evaluator.Eval(input) + if err == nil { + t.Fatal("strict mode should detect ref+ in map[any]any") + } +} + +func TestDisableValsStrictArrayRef(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableValsStrict, "true") + defer os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should detect ref+ in []any arrays + input := map[string]any{ + "values": []any{"normal", "ref+awssecrets://db/password"}, + } + _, err = evaluator.Eval(input) + if err == nil { + t.Fatal("strict mode should detect ref+ in arrays") + } +} + +func TestDisableValsStrictStringSlice(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableValsStrict, "true") + defer os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should detect ref+ in []string arrays (matches renderValsSecrets usage) + input := map[string]any{ + "values": []string{"normal", "ref+awssecrets://db/password"}, + } + _, err = evaluator.Eval(input) + if err == nil { + t.Fatal("strict mode should detect ref+ in []string arrays") + } +} + +func TestDisableValsPassThroughStringSlice(t *testing.T) { + resetInstance() + defer resetInstance() + + os.Setenv(envvar.DisableVals, "true") + defer os.Unsetenv(envvar.DisableVals) + os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + input := map[string]any{ + "values": []string{"normal", "ref+awssecrets://db/password"}, + } + out, err := evaluator.Eval(input) + if err != nil { + t.Fatalf("pass-through should not error: %v", err) + } + + values, ok := out["values"].([]any) + if !ok { + t.Fatalf("expected out[\"values\"] to be []any, got %T", out["values"]) + } + if values[0] != "normal" || values[1] != "ref+awssecrets://db/password" { + t.Errorf("unexpected values: %v", values) + } +} + +func TestNormalValsProcessing(t *testing.T) { + resetInstance() + defer resetInstance() + + // Ensure both are unset + os.Unsetenv(envvar.DisableVals) + os.Unsetenv(envvar.DisableValsStrict) + + evaluator, err := ValsInstance() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // ref+echo should expand to the value after :// + input := map[string]any{"key": "ref+echo://myvalue"} + output, err := evaluator.Eval(input) + if err != nil { + t.Fatalf("normal vals should process ref+echo: %v", err) + } + + if output["key"] != "myvalue" { + t.Errorf("expected 'myvalue', got %v", output["key"]) + } +} + // TestAWSSDKLogLevelConfiguration tests the AWS SDK log level configuration logic func TestAWSSDKLogLevelConfiguration(t *testing.T) { tests := []struct { diff --git a/pkg/tmpl/expand_secret_ref.go b/pkg/tmpl/expand_secret_ref.go index bf3255b2..224ee9f8 100644 --- a/pkg/tmpl/expand_secret_ref.go +++ b/pkg/tmpl/expand_secret_ref.go @@ -4,8 +4,6 @@ import ( "fmt" "sync" - "github.com/helmfile/vals" - "github.com/helmfile/helmfile/pkg/plugins" ) @@ -42,13 +40,8 @@ func fetchSecretValues(values map[string]any) (map[string]any, error) { var err error // below lines are for tests once.Do(func() { - var valRuntime *vals.Runtime if secretsClient == nil { - valRuntime, err = plugins.ValsInstance() - if err != nil { - return - } - secretsClient = valRuntime + secretsClient, err = plugins.ValsInstance() } }) if secretsClient == nil {