From 8f2c997396c66de70e0a790d45bb51e244625b2c Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 2 Dec 2024 12:26:50 -0800 Subject: [PATCH] tofu: Variable validation diagnostics must mark sensitive values The existing test for input variable validation with sensitive values was building on a false premise that sensitive-flagged variables always emerge from EvalContext.GetVariableValue as marked. In practice that's true only for input variables passed from a parent model where the value was already marked on entry, because the marking caused directly by the "sensitive = true" argument is handled later once the value has been finalized and is being reported from the main expression scope. Since the variable validation codepath writes a not-yet-finalized value for the variable into its own HCL expression scope directly, it needs to apply the sensitive mark itself when the variable was configured as sensitive, which therefore means that the value provided to the "condition" expression is properly marked and any diagnostics generated from it will capture that marked value. Along with fixing the existing test to have a mock GetVariableValue that more realistically models BuiltinEvalContext's behavior, this introduces a new test specifically about the marking of the values in the diagnostic errors to help avoid potentially regressing that specific detail, since diagnostics there can potentially originate from various different parts of the system. Signed-off-by: Martin Atkins --- CHANGELOG.md | 2 + internal/tofu/eval_variable.go | 7 +++ internal/tofu/eval_variable_test.go | 83 +++++++++++++++++++++++++++-- 3 files changed, 87 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04bda774b2..5b9e5f098b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## 1.8.7 (unreleased) +BUG FIXES: +* Error messages related to validation of sensitive input variables will no longer disclose the sensitive value in the UI. ([#2219](https://github.com/opentofu/opentofu/pull/2219)) ## 1.8.6 diff --git a/internal/tofu/eval_variable.go b/internal/tofu/eval_variable.go index 1033033db2..a3f8353fe6 100644 --- a/internal/tofu/eval_variable.go +++ b/internal/tofu/eval_variable.go @@ -235,6 +235,13 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config }) return diags } + if config.Sensitive { + // Normally this marking is handled automatically during construction + // of the HCL eval context the evaluation scope, but this codepath + // augments that scope with the not-yet-finalized input variable + // value, so we need to apply this mark separately. + val = val.Mark(marks.Sensitive) + } for ix, validation := range config.Validations { condFuncs, condDiags := lang.ProviderFunctionsInExpr(addrs.ParseRef, validation.Condition) diags = diags.Append(condDiags) diff --git a/internal/tofu/eval_variable_test.go b/internal/tofu/eval_variable_test.go index 5f7fd87f29..10b07be20d 100644 --- a/internal/tofu/eval_variable_test.go +++ b/internal/tofu/eval_variable_test.go @@ -1332,11 +1332,11 @@ variable "bar" { if got, want := addr.String(), varAddr.String(); got != want { t.Errorf("incorrect argument to GetVariableValue: got %s, want %s", got, want) } - if varCfg.Sensitive { - return test.given.Mark(marks.Sensitive) - } else { - return test.given - } + // NOTE: This intentionally doesn't mark the result as sensitive, + // because BuiltinEvalContext.GetVariableValueFunc doesn't either. + // It's the responsibility of downstream code to detect and handle + // configured sensitivity. + return test.given } ctx.ChecksState = checks.NewState(cfg) ctx.ChecksState.ReportCheckableObjects(varAddr.ConfigCheckable(), addrs.MakeSet[addrs.Checkable](varAddr)) @@ -1371,3 +1371,76 @@ variable "bar" { }) } } + +func TestEvalVariableValidations_sensitiveValueDiagnostics(t *testing.T) { + // This test verifies that values for sensitive variables get captured + // into diagnostic messages with the sensitive mark intact, so that + // the values won't be disclosed in the UI. + // Earlier versions handled this incorrectly: + // https://github.com/opentofu/opentofu/issues/2219 + + cfgSrc := ` + variable "foo" { + type = string + sensitive = true + + validation { + condition = length(var.foo) == 8 # intentionally fails + error_message = "Foo must have 8 characters." + } + } + ` + cfg := testModuleInline(t, map[string]string{ + "main.tf": cfgSrc, + }) + varAddr := addrs.InputVariable{Name: "foo"}.Absolute(addrs.RootModuleInstance) + + ctx := &MockEvalContext{} + ctx.EvaluationScopeScope = &lang.Scope{} + ctx.GetVariableValueFunc = func(addr addrs.AbsInputVariableInstance) cty.Value { + if got, want := addr.String(), varAddr.String(); got != want { + t.Errorf("incorrect argument to GetVariableValue: got %s, want %s", got, want) + } + // NOTE: This intentionally doesn't mark the result as sensitive, + // because BuiltinEvalContext.GetVariableValueFunc doesn't either. + // It's the responsibility of downstream code to detect and handle + // configured sensitivity. + return cty.StringVal("boop") + } + ctx.ChecksState = checks.NewState(cfg) + ctx.ChecksState.ReportCheckableObjects(varAddr.ConfigCheckable(), addrs.MakeSet[addrs.Checkable](varAddr)) + + gotDiags := evalVariableValidations( + varAddr, cfg.Module.Variables["foo"], nil, ctx, + ) + if !gotDiags.HasErrors() { + t.Fatalf("unexpected success; want validation error") + } + + // The generated diagnostic(s) should all capture the evaluation context + // that was used to evaluate the condition, where the variable's value + // should be marked as sensitive so it won't get displayed in clear + // in the UI output. + // (The HasErrors check above guarantees that there's at least one + // diagnostic here for us to iterate over.) + for _, diag := range gotDiags { + if diag.Severity() != tfdiags.Error { + continue + } + fromExpr := diag.FromExpr() + if fromExpr == nil { + t.Fatalf("diagnostic does not have source expression information at all") + } + allVarVals := fromExpr.EvalContext.Variables["var"] + if allVarVals == cty.NilVal || !allVarVals.Type().IsObjectType() { + t.Fatalf("diagnostic did not capture an object value for the top-level symbol 'var'") + } + gotVal := allVarVals.GetAttr("foo") + if gotVal == cty.NilVal { + t.Fatalf("diagnostic did not capture a value for var.foo") + } + if !gotVal.HasMark(marks.Sensitive) { + t.Errorf("var.foo value is not marked as sensitive in diagnostic") + } + } +}