From 6db174e21039e9d243bbc7caeef1b3dfe9a91701 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 11 Mar 2022 11:09:28 -0500 Subject: [PATCH 1/2] core: Fix crash for sensitive values in conditions Precondition and postcondition blocks which evaluated expressions resulting in sensitive values would previously crash. This commit fixes the crashes, and adds an additional diagnostic if the error message expression produces a sensitive value (which we also elide). --- internal/terraform/context_plan2_test.go | 62 ++++++++++++++++++++++++ internal/terraform/eval_conditions.go | 23 ++++++++- 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index e6315c313..3964fac98 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -2859,3 +2859,65 @@ func TestContext2Plan_preconditionErrors(t *testing.T) { }) } } + +func TestContext2Plan_preconditionSensitiveValues(t *testing.T) { + p := testProvider("test") + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +variable "boop" { + sensitive = true + type = string +} + +output "a" { + sensitive = true + value = var.boop + + precondition { + condition = length(var.boop) <= 4 + error_message = "Boop is too long, ${length(var.boop)} > 4" + } +} +`, + }) + + _, diags := ctx.Plan(m, states.NewState(), &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "boop": &InputValue{ + Value: cty.StringVal("bleep"), + SourceType: ValueFromCLIArg, + }, + }, + }) + if !diags.HasErrors() { + t.Fatal("succeeded; want errors") + } + if got, want := len(diags), 2; got != want { + t.Errorf("wrong number of diags, got %d, want %d", got, want) + } + for _, diag := range diags { + desc := diag.Description() + if desc.Summary == "Module output value precondition failed" { + if got, want := desc.Detail, "The error message included a sensitive value, so it will not be displayed."; !strings.Contains(got, want) { + t.Errorf("unexpected detail\ngot: %s\nwant to contain %q", got, want) + } + } else if desc.Summary == "Error message refers to sensitive values" { + if got, want := desc.Detail, "The error expression used to explain this condition refers to sensitive values."; !strings.Contains(got, want) { + t.Errorf("unexpected detail\ngot: %s\nwant to contain %q", got, want) + } + } else { + t.Errorf("unexpected summary\ngot: %s", desc.Summary) + } + } +} diff --git a/internal/terraform/eval_conditions.go b/internal/terraform/eval_conditions.go index 4bf311428..80858eee8 100644 --- a/internal/terraform/eval_conditions.go +++ b/internal/terraform/eval_conditions.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/lang" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -110,6 +111,10 @@ func evalCheckRules(typ checkType, rules []*configs.CheckRule, ctx EvalContext, continue } + // The condition result may be marked if the expression refers to a + // sensitive value. + result, _ = result.Unmark() + if result.True() { continue } @@ -128,7 +133,23 @@ func evalCheckRules(typ checkType, rules []*configs.CheckRule, ctx EvalContext, EvalContext: hclCtx, }) } else { - errorMessage = strings.TrimSpace(errorValue.AsString()) + if marks.Has(errorValue, marks.Sensitive) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: severity, + + Summary: "Error message refers to sensitive values", + Detail: `The error expression used to explain this condition refers to sensitive values. Terraform will not display the resulting message. + +You can correct this by removing references to sensitive values, or by carefully using the nonsensitive() function if the expression will not reveal the sensitive data.`, + + Subject: rule.ErrorMessage.Range().Ptr(), + Expression: rule.ErrorMessage, + EvalContext: hclCtx, + }) + errorMessage = "The error message included a sensitive value, so it will not be displayed." + } else { + errorMessage = strings.TrimSpace(errorValue.AsString()) + } } } if errorMessage == "" { From b5cfc0bb8bc4f9f4ffca495a80d167bc0da02f97 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 11 Mar 2022 11:11:30 -0500 Subject: [PATCH 2/2] core: Fix sensitive variable validation errors Variable validation error message expressions which generated sensitive values would previously crash. This commit updates the logic to align with preconditions and postconditions, eliding sensitive error message values and adding a separate diagnostic explaining why. --- internal/terraform/eval_variable.go | 19 +++- internal/terraform/eval_variable_test.go | 133 +++++++++++++++++++++++ 2 files changed, 151 insertions(+), 1 deletion(-) diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index 629da7051..f6c927e5e 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" @@ -321,7 +322,23 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config EvalContext: hclCtx, }) } else { - errorMessage = strings.TrimSpace(errorValue.AsString()) + if marks.Has(errorValue, marks.Sensitive) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + + Summary: "Error message refers to sensitive values", + Detail: `The error expression used to explain this condition refers to sensitive values. Terraform will not display the resulting message. + +You can correct this by removing references to sensitive values, or by carefully using the nonsensitive() function if the expression will not reveal the sensitive data.`, + + Subject: validation.ErrorMessage.Range().Ptr(), + Expression: validation.ErrorMessage, + EvalContext: hclCtx, + }) + errorMessage = "The error message included a sensitive value, so it will not be displayed." + } else { + errorMessage = strings.TrimSpace(errorValue.AsString()) + } } } if errorMessage == "" { diff --git a/internal/terraform/eval_variable_test.go b/internal/terraform/eval_variable_test.go index c84a969e8..662bf5626 100644 --- a/internal/terraform/eval_variable_test.go +++ b/internal/terraform/eval_variable_test.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/lang" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -715,3 +716,135 @@ func TestEvalVariableValidations_jsonErrorMessageEdgeCase(t *testing.T) { }) } } + +func TestEvalVariableValidations_sensitiveValues(t *testing.T) { + cfgSrc := ` +variable "foo" { + type = string + sensitive = true + default = "boop" + + validation { + condition = length(var.foo) == 4 + error_message = "Foo must be 4 characters, not ${length(var.foo)}" + } +} + +variable "bar" { + type = string + sensitive = true + default = "boop" + + validation { + condition = length(var.bar) == 4 + error_message = "Bar must be 4 characters, not ${nonsensitive(length(var.bar))}." + } +} +` + cfg := testModuleInline(t, map[string]string{ + "main.tf": cfgSrc, + }) + variableConfigs := cfg.Module.Variables + + // Because we loaded our pseudo-module from a temporary file, the + // declaration source ranges will have unpredictable filenames. We'll + // fix that here just to make things easier below. + for _, vc := range variableConfigs { + vc.DeclRange.Filename = "main.tf" + for _, v := range vc.Validations { + v.DeclRange.Filename = "main.tf" + } + } + + tests := []struct { + varName string + given cty.Value + wantErr []string + }{ + // Validations pass on a sensitive variable with an error message which + // would generate a sensitive value + { + varName: "foo", + given: cty.StringVal("boop"), + }, + // Assigning a value which fails the condition generates a sensitive + // error message, which is elided and generates another error + { + varName: "foo", + given: cty.StringVal("bap"), + wantErr: []string{ + "Invalid value for variable", + "The error message included a sensitive value, so it will not be displayed.", + "Error message refers to sensitive values", + }, + }, + // Validations pass on a sensitive variable with a correctly defined + // error message + { + varName: "bar", + given: cty.StringVal("boop"), + }, + // Assigning a value which fails the condition generates a nonsensitive + // error message, which is displayed + { + varName: "bar", + given: cty.StringVal("bap"), + wantErr: []string{ + "Invalid value for variable", + "Bar must be 4 characters, not 3.", + }, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%s %#v", test.varName, test.given), func(t *testing.T) { + varAddr := addrs.InputVariable{Name: test.varName}.Absolute(addrs.RootModuleInstance) + varCfg := variableConfigs[test.varName] + if varCfg == nil { + t.Fatalf("invalid variable name %q", test.varName) + } + + // Build a mock context to allow the function under test to + // retrieve the variable value and evaluate the expressions + ctx := &MockEvalContext{} + + // We need a minimal scope to allow basic functions to be passed to + // the HCL scope + 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) + } + if varCfg.Sensitive { + return test.given.Mark(marks.Sensitive) + } else { + return test.given + } + } + + gotDiags := evalVariableValidations( + varAddr, varCfg, nil, ctx, + ) + + if len(test.wantErr) == 0 { + if len(gotDiags) > 0 { + t.Errorf("no diags expected, got %s", gotDiags.Err().Error()) + } + } else { + wantErrs: + for _, want := range test.wantErr { + for _, diag := range gotDiags { + if diag.Severity() != tfdiags.Error { + continue + } + desc := diag.Description() + if strings.Contains(desc.Summary, want) || strings.Contains(desc.Detail, want) { + continue wantErrs + } + } + t.Errorf("no error diagnostics found containing %q\ngot: %s", want, gotDiags.Err().Error()) + } + } + }) + } +}