From 378ee6ac56bcf6a7880843173ad6136e8d6abe30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Bataille?= Date: Fri, 18 Feb 2022 11:46:56 +0100 Subject: [PATCH 1/2] Fixes #29156: Failing sensitive variables values are not logged --- internal/terraform/eval_variable.go | 46 ++++++++++------ internal/terraform/eval_variable_test.go | 70 ++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 18 deletions(-) diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index fd57a136f290..52d31ecc82b4 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -90,27 +90,37 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, raw *In val, err := convert.Convert(given, convertTy) if err != nil { log.Printf("[ERROR] prepareFinalInputVariableValue: %s has unsuitable type\n got: %s\n want: %s", addr, given.Type(), convertTy) + var detail string + var subject *hcl.Range if nonFileSource != "" { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for input variable", - Detail: fmt.Sprintf( - "Unsuitable value for %s %s: %s.", - addr, nonFileSource, err, - ), - Subject: cfg.DeclRange.Ptr(), - }) + detail = fmt.Sprintf( + "Unsuitable value for %s %s: %s.", + addr, nonFileSource, err, + ) + subject = cfg.DeclRange.Ptr() } else { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for input variable", - Detail: fmt.Sprintf( - "The given value is not suitable for %s declared at %s: %s.", - addr, cfg.DeclRange.String(), err, - ), - Subject: sourceRange.ToHCL().Ptr(), - }) + detail = fmt.Sprintf( + "The given value is not suitable for %s declared at %s: %s.", + addr, cfg.DeclRange.String(), err, + ) + subject = sourceRange.ToHCL().Ptr() + + // In some workflows, the operator running terraform does not have access to the variables + // themselves. They are for example stored in encrypted files that will be used by the CI toolset + // and not by the operator directly. In such a case, the failing secret value should not be + // displayed to the operator + if cfg.Sensitive { + subject = nil + detail += fmt.Sprintf("\n\n%s is marked as sensitive. Invalid value defined at %s.", addr, sourceRange.ToHCL()) + } } + + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid value for input variable", + Detail: detail, + Subject: subject, + }) // We'll return a placeholder unknown value to avoid producing // redundant downstream errors. return cty.UnknownVal(cfg.Type), diags diff --git a/internal/terraform/eval_variable_test.go b/internal/terraform/eval_variable_test.go index 0ebea982f57b..8ebfe0a539f7 100644 --- a/internal/terraform/eval_variable_test.go +++ b/internal/terraform/eval_variable_test.go @@ -60,6 +60,11 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { type = string default = true } + variable "constrained_string_sensitive_required" { + sensitive = true + nullable = false + type = string + } ` cfg := testModuleInline(t, map[string]string{ "main.tf": cfgSrc, @@ -390,6 +395,14 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { cty.UnknownVal(cty.String), ``, }, + + // sensitive + { + "constrained_string_sensitive_required", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, } for _, test := range tests { @@ -560,4 +573,61 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { }) } }) + + t.Run("SensitiveVariable error message variants, with source variants", func(t *testing.T) { + tests := []struct { + SourceType ValueSourceType + SourceRange tfdiags.SourceRange + WantTypeErr string + HideSubject bool + }{ + { + ValueFromUnknown, + tfdiags.SourceRange{}, + "Invalid value for input variable: Unsuitable value for var.constrained_string_sensitive_required set from outside of the configuration: string required.", + false, + }, + { + ValueFromConfig, + tfdiags.SourceRange{ + Filename: "example.tf", + Start: tfdiags.SourcePos(hcl.InitialPos), + End: tfdiags.SourcePos(hcl.InitialPos), + }, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_sensitive_required declared at main.tf:46,3-51: string required. + +var.constrained_string_sensitive_required is marked as sensitive. Invalid value defined at example.tf:1,1-1.`, + true, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%s %s", test.SourceType, test.SourceRange.StartString()), func(t *testing.T) { + varAddr := addrs.InputVariable{Name: "constrained_string_sensitive_required"}.Absolute(addrs.RootModuleInstance) + varCfg := variableConfigs[varAddr.Variable.Name] + t.Run("type error", func(t *testing.T) { + rawVal := &InputValue{ + Value: cty.EmptyObjectVal, + SourceType: test.SourceType, + SourceRange: test.SourceRange, + } + + _, diags := prepareFinalInputVariableValue( + varAddr, rawVal, varCfg, + ) + if !diags.HasErrors() { + t.Fatalf("unexpected success; want error") + } + + if got, want := diags.Err().Error(), test.WantTypeErr; got != want { + t.Errorf("wrong error\ngot: %s\nwant: %s", got, want) + } + + if test.HideSubject != (diags[0].Source().Subject == nil) { + t.Errorf("Subject (code context) should have been masked\ngot: %v", diags[0].Source().Subject) + } + }) + }) + } + }) } From 6494aa032604259734eb99f94405b7421ccf0345 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 13 May 2022 17:09:38 -0400 Subject: [PATCH 2/2] Include var declaration where possible --- internal/terraform/eval_variable.go | 7 +++++-- internal/terraform/eval_variable_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index 52d31ecc82b4..7de5aa1cd888 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -110,8 +110,11 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, raw *In // and not by the operator directly. In such a case, the failing secret value should not be // displayed to the operator if cfg.Sensitive { - subject = nil - detail += fmt.Sprintf("\n\n%s is marked as sensitive. Invalid value defined at %s.", addr, sourceRange.ToHCL()) + detail = fmt.Sprintf( + "The given value is not suitable for %s, which is sensitive: %s. Invalid value defined at %s.", + addr, err, sourceRange.ToHCL(), + ) + subject = cfg.DeclRange.Ptr() } } diff --git a/internal/terraform/eval_variable_test.go b/internal/terraform/eval_variable_test.go index 8ebfe0a539f7..e8786c9aa844 100644 --- a/internal/terraform/eval_variable_test.go +++ b/internal/terraform/eval_variable_test.go @@ -590,13 +590,11 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { { ValueFromConfig, tfdiags.SourceRange{ - Filename: "example.tf", + Filename: "example.tfvars", Start: tfdiags.SourcePos(hcl.InitialPos), End: tfdiags.SourcePos(hcl.InitialPos), }, - `Invalid value for input variable: The given value is not suitable for var.constrained_string_sensitive_required declared at main.tf:46,3-51: string required. - -var.constrained_string_sensitive_required is marked as sensitive. Invalid value defined at example.tf:1,1-1.`, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_sensitive_required, which is sensitive: string required. Invalid value defined at example.tfvars:1,1-1.`, true, }, } @@ -623,8 +621,10 @@ var.constrained_string_sensitive_required is marked as sensitive. Invalid value t.Errorf("wrong error\ngot: %s\nwant: %s", got, want) } - if test.HideSubject != (diags[0].Source().Subject == nil) { - t.Errorf("Subject (code context) should have been masked\ngot: %v", diags[0].Source().Subject) + if test.HideSubject { + if got, want := diags[0].Source().Subject.StartString(), test.SourceRange.StartString(); got == want { + t.Errorf("Subject start should have been hidden, but was %s", got) + } } }) })