From 43bf3832d5c8e02ff1fcd00411d2bf2165bc90ba Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 20 Apr 2021 15:49:54 -0700 Subject: [PATCH] core: Loosen output value sensitivity requirement Non-root module outputs no longer strip sensitivity marks from their values, allowing dynamically sensitive values to propagate through the configuration. We also remove the requirement for non-root module outputs to be defined as sensitive if the value is marked as sensitive. This avoids a static/dynamic clash when using shared modules that might unknowingly receive sensitive values via input variables. Co-authored-by: Martin Atkins --- terraform/context_plan2_test.go | 30 ++++++++++++++++++++++++++++++ terraform/context_validate_test.go | 20 ++++++-------------- terraform/evaluate.go | 2 +- terraform/node_output.go | 28 ++++++++++++++++++---------- 4 files changed, 55 insertions(+), 25 deletions(-) diff --git a/terraform/context_plan2_test.go b/terraform/context_plan2_test.go index f95f7f6e3..73bdb283b 100644 --- a/terraform/context_plan2_test.go +++ b/terraform/context_plan2_test.go @@ -2,6 +2,7 @@ package terraform import ( "errors" + "strings" "testing" "github.com/hashicorp/terraform/addrs" @@ -485,3 +486,32 @@ provider "test" { t.Fatal(diags.Err()) } } + +func TestContext2Plan_invalidSensitiveModuleOutput(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "child/main.tf": ` +output "out" { + value = sensitive("xyz") +}`, + "main.tf": ` +module "child" { + source = "./child" +} + +output "root" { + value = module.child.out +}`, + }) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + }) + + _, diags := ctx.Plan() + if !diags.HasErrors() { + t.Fatal("succeeded; want errors") + } + if got, want := diags.Err().Error(), "Output refers to sensitive values"; !strings.Contains(got, want) { + t.Fatalf("wrong error:\ngot: %s\nwant: message containing %q", got, want) + } +} diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 7e2b0a25e..0361e30e4 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -1379,7 +1379,7 @@ resource "aws_instance" "foo" { } } -func TestContext2Validate_invalidSensitiveModuleOutput(t *testing.T) { +func TestContext2Validate_sensitiveRootModuleOutput(t *testing.T) { m := testModuleInline(t, map[string]string{ "child/main.tf": ` variable "foo" { @@ -1395,27 +1395,19 @@ module "child" { source = "./child" } -resource "aws_instance" "foo" { - foo = module.child.out +output "root" { + value = module.child.out + sensitive = true }`, }) - p := testProvider("aws") ctx := testContext2(t, &ContextOpts{ Config: m, - Providers: map[addrs.Provider]providers.Factory{ - addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), - }, }) diags := ctx.Validate() - if !diags.HasErrors() { - t.Fatal("succeeded; want errors") - } - // Should get this error: - // Output refers to sensitive values: Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true. - if got, want := diags.Err().Error(), "Output refers to sensitive values"; !strings.Contains(got, want) { - t.Fatalf("wrong error:\ngot: %s\nwant: message containing %q", got, want) + if diags.HasErrors() { + t.Fatal(diags.Err()) } } diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 92e50614d..2417d2f64 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -459,7 +459,7 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc continue } - instance[cfg.Name] = change.After + instance[cfg.Name] = change.After.MarkWithPaths(changeSrc.AfterValMarks) if change.Sensitive && !change.After.HasMark("sensitive") { instance[cfg.Name] = change.After.Mark("sensitive") diff --git a/terraform/node_output.go b/terraform/node_output.go index c70c76e9c..4db2055c8 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -275,16 +275,24 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) (diags // depends_on expressions here too diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn)) - // Ensure that non-sensitive outputs don't include sensitive values + // For root module outputs in particular, an output value must be + // statically declared as sensitive in order to dynamically return + // a sensitive result, to help avoid accidental exposure in the state + // of a sensitive value that the user doesn't want to include there. _, marks := val.UnmarkDeep() _, hasSensitive := marks["sensitive"] - if !n.Config.Sensitive && hasSensitive { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Output refers to sensitive values", - Detail: "Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true.", - Subject: n.Config.DeclRange.Ptr(), - }) + if n.Addr.Module.IsRoot() { + if !n.Config.Sensitive && hasSensitive { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Output refers to sensitive values", + Detail: `To reduce the risk of accidentally exporting sensitive data that was intended to be only internal, Terraform requires that any root module output containing sensitive data be explicitly marked as sensitive, to confirm your intent. + +If you do intend to export this data, annotate the output value as sensitive by adding the following argument: + sensitive = true`, + Subject: n.Config.DeclRange.Ptr(), + }) + } } } @@ -454,7 +462,7 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C sensitiveChange := sensitiveBefore || n.Config.Sensitive // strip any marks here just to be sure we don't panic on the True comparison - val, _ = val.UnmarkDeep() + unmarkedVal, _ := val.UnmarkDeep() action := plans.Update switch { @@ -468,7 +476,7 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C action = plans.Create case val.IsWhollyKnown() && - val.Equals(before).True() && + unmarkedVal.Equals(before).True() && n.Config.Sensitive == sensitiveBefore: // Sensitivity must also match to be a NoOp. // Theoretically marks may not match here, but sensitivity is the