Merge pull request #30659 from hashicorp/alisdair/condition-blocks-sensitive-values

core: Fix crashes when condition block expressions refer to sensitive values

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)
+		}
+	}
+}

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 == "" {

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 == "" {

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())
+				}
+			}
+		})
+	}
+}