From c1f719345417b8bccdfe001096ed630c7fbedaec Mon Sep 17 00:00:00 2001
From: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Date: Fri, 9 Apr 2021 14:33:49 -0400
Subject: [PATCH] lang/funcs: Make nonsensitive more permissive

Calling the nonsensitive function with values which are not sensitive
will result in an error. This restriction was added with the goal of
preventing confusingly redundant use of this function.

Unfortunately, this breaks when using nonsensitive to reveal the value of
sensitive resource attributes. This is because the validate walk does
not (and cannot) mark attributes as sensitive based on the schema,
because the resource value itself is unknown.

This commit therefore alters this restriction such that it permits
nonsensitive unknown values, and adds a test case to cover this specific
scenario.
---
 lang/funcs/sensitive.go         |  2 +-
 lang/funcs/sensitive_test.go    | 13 ++++---
 terraform/context_plan2_test.go | 62 +++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/lang/funcs/sensitive.go b/lang/funcs/sensitive.go
index b132dc12b..2fc505e73 100644
--- a/lang/funcs/sensitive.go
+++ b/lang/funcs/sensitive.go
@@ -48,7 +48,7 @@ var NonsensitiveFunc = function.New(&function.Spec{
 		return args[0].Type(), nil
 	},
 	Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) {
-		if !args[0].HasMark("sensitive") {
+		if args[0].IsKnown() && !args[0].HasMark("sensitive") {
 			return cty.DynamicVal, function.NewArgErrorf(0, "the given value is not sensitive, so this call is redundant")
 		}
 		v, marks := args[0].Unmark()
diff --git a/lang/funcs/sensitive_test.go b/lang/funcs/sensitive_test.go
index 522346b4c..591f53cf6 100644
--- a/lang/funcs/sensitive_test.go
+++ b/lang/funcs/sensitive_test.go
@@ -133,17 +133,20 @@ func TestNonsensitive(t *testing.T) {
 			cty.NumberIntVal(1),
 			`the given value is not sensitive, so this call is redundant`,
 		},
-		{
-			cty.DynamicVal,
-			`the given value is not sensitive, so this call is redundant`,
-		},
 		{
 			cty.NullVal(cty.String),
 			`the given value is not sensitive, so this call is redundant`,
 		},
+
+		// Unknown values may become sensitive once they are known, so we
+		// permit them to be marked nonsensitive.
+		{
+			cty.DynamicVal,
+			``,
+		},
 		{
 			cty.UnknownVal(cty.String),
-			`the given value is not sensitive, so this call is redundant`,
+			``,
 		},
 	}
 
diff --git a/terraform/context_plan2_test.go b/terraform/context_plan2_test.go
index b2d238495..0444a775c 100644
--- a/terraform/context_plan2_test.go
+++ b/terraform/context_plan2_test.go
@@ -377,3 +377,65 @@ resource "test_object" "a" {
 		}
 	}
 }
+
+func TestContext2Plan_unmarkingSensitiveAttributeForOutput(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_resource" "foo" {
+}
+
+output "result" {
+  value = nonsensitive(test_resource.foo.sensitive_attr)
+}
+`,
+	})
+
+	p := new(MockProvider)
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_resource": {
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:     cty.String,
+						Computed: true,
+					},
+					"sensitive_attr": {
+						Type:      cty.String,
+						Computed:  true,
+						Sensitive: true,
+					},
+				},
+			},
+		},
+	})
+
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		return providers.PlanResourceChangeResponse{
+			PlannedState: cty.UnknownVal(cty.Object(map[string]cty.Type{
+				"id":             cty.String,
+				"sensitive_attr": cty.String,
+			})),
+		}
+	}
+
+	state := states.NewState()
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	for _, res := range plan.Changes.Resources {
+		if res.Action != plans.Create {
+			t.Fatalf("expected create, got: %q %s", res.Addr, res.Action)
+		}
+	}
+}