terraform: Check for sensitive values in outputs

Sensitive values may not be used in outputs which are not also marked
as sensitive. This includes values nested within complex structures.

Note that sensitive values are unmarked before writing to state. This
means that sensitive values used in module outputs will have the
sensitive mark removed. At the moment, we have not implemented
sensitivity propagation from module outputs back to value marks.

This commit also reworks the tests for NodeApplyableOutput to cover
more existing behaviour, as well as this change.

terraform/node_output.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"log"
 
+	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/dag"
@@ -210,6 +211,18 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) error {
 		// depends_on expressions here too
 		diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn))
 
+		// Ensure that non-sensitive outputs don't include sensitive values
+		_, 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(),
+			})
+		}
+
 		state := ctx.State()
 		if state == nil {
 			return nil
@@ -307,7 +320,8 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 		// out here and then we'll save the real unknown value in the planned
 		// changeset below, if we have one on this graph walk.
 		log.Printf("[TRACE] EvalWriteOutput: Saving value for %s in state", n.Addr)
-		stateVal := cty.UnknownAsNull(val)
+		unmarkedVal, _ := val.UnmarkDeep()
+		stateVal := cty.UnknownAsNull(unmarkedVal)
 		state.SetOutputValue(n.Addr, stateVal, n.Config.Sensitive)
 	} else {
 		log.Printf("[TRACE] EvalWriteOutput: Removing %s from state (it is now null)", n.Addr)

terraform/node_output_test.go

@@ -1,61 +1,142 @@
 package terraform
 
 import (
+	"strings"
 	"testing"
 
+	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/states"
 	"github.com/zclconf/go-cty/cty"
 )
 
-func TestNodeApplyableOutputExecute(t *testing.T) {
+func TestNodeApplyableOutputExecute_knownValue(t *testing.T) {
 	ctx := new(MockEvalContext)
 	ctx.StateState = states.NewState().SyncWrapper()
+	ctx.RefreshStateState = states.NewState().SyncWrapper()
 
-	cases := []struct {
-		name string
-		val  cty.Value
-		err  bool
-	}{
-		{
-			// Eval should recognize a single map in a slice, and collapse it
-			// into the map value
-			"single-map",
-			cty.MapVal(map[string]cty.Value{
-				"a": cty.StringVal("b"),
-			}),
-			false,
-		},
-		{
-			// we can't apply a multi-valued map to a variable, so this should error
-			"multi-map",
-			cty.ListVal([]cty.Value{
-				cty.MapVal(map[string]cty.Value{
-					"a": cty.StringVal("b"),
-				}),
-				cty.MapVal(map[string]cty.Value{
-					"c": cty.StringVal("d"),
-				}),
-			}),
-			true,
+	config := &configs.Output{Name: "map-output"}
+	addr := addrs.OutputValue{Name: config.Name}.Absolute(addrs.RootModuleInstance)
+	node := &NodeApplyableOutput{Config: config, Addr: addr}
+	val := cty.MapVal(map[string]cty.Value{
+		"a": cty.StringVal("b"),
+	})
+	ctx.EvaluateExprResult = val
+
+	err := node.Execute(ctx, walkApply)
+	if err != nil {
+		t.Fatalf("unexpected execute error: %s", err)
+	}
+
+	outputVal := ctx.StateState.OutputValue(addr)
+	if got, want := outputVal.Value, val; !got.RawEquals(want) {
+		t.Errorf("wrong output value in state\n got: %#v\nwant: %#v", got, want)
+	}
+
+	if !ctx.RefreshStateCalled {
+		t.Fatal("should have called RefreshState, but didn't")
+	}
+	refreshOutputVal := ctx.RefreshStateState.OutputValue(addr)
+	if got, want := refreshOutputVal.Value, val; !got.RawEquals(want) {
+		t.Fatalf("wrong output value in refresh state\n got: %#v\nwant: %#v", got, want)
+	}
+}
+
+func TestNodeApplyableOutputExecute_noState(t *testing.T) {
+	ctx := new(MockEvalContext)
+
+	config := &configs.Output{Name: "map-output"}
+	addr := addrs.OutputValue{Name: config.Name}.Absolute(addrs.RootModuleInstance)
+	node := &NodeApplyableOutput{Config: config, Addr: addr}
+	val := cty.MapVal(map[string]cty.Value{
+		"a": cty.StringVal("b"),
+	})
+	ctx.EvaluateExprResult = val
+
+	err := node.Execute(ctx, walkApply)
+	if err != nil {
+		t.Fatalf("unexpected execute error: %s", err)
+	}
+}
+
+func TestNodeApplyableOutputExecute_invalidDependsOn(t *testing.T) {
+	ctx := new(MockEvalContext)
+	ctx.StateState = states.NewState().SyncWrapper()
+
+	config := &configs.Output{
+		Name: "map-output",
+		DependsOn: []hcl.Traversal{
+			{
+				hcl.TraverseRoot{Name: "test_instance"},
+				hcl.TraverseAttr{Name: "foo"},
+				hcl.TraverseAttr{Name: "bar"},
+			},
 		},
 	}
+	addr := addrs.OutputValue{Name: config.Name}.Absolute(addrs.RootModuleInstance)
+	node := &NodeApplyableOutput{Config: config, Addr: addr}
+	val := cty.MapVal(map[string]cty.Value{
+		"a": cty.StringVal("b"),
+	})
+	ctx.EvaluateExprResult = val
 
-	for _, tc := range cases {
-		node := &NodeApplyableOutput{
-			Config: &configs.Output{},
-			Addr:   addrs.OutputValue{Name: tc.name}.Absolute(addrs.RootModuleInstance),
-		}
-		ctx.EvaluateExprResult = tc.val
-		t.Run(tc.name, func(t *testing.T) {
-			err := node.Execute(ctx, walkApply)
-			if err != nil && !tc.err {
-				t.Fatal(err)
-			}
-		})
+	err := node.Execute(ctx, walkApply)
+	if err == nil {
+		t.Fatal("expected execute error, but there was none")
+	}
+	if got, want := err.Error(), "Invalid depends_on reference"; !strings.Contains(got, want) {
+		t.Errorf("expected error to include %q, but was: %s", want, got)
+	}
+}
+
+func TestNodeApplyableOutputExecute_sensitiveValueNotOutput(t *testing.T) {
+	ctx := new(MockEvalContext)
+	ctx.StateState = states.NewState().SyncWrapper()
+
+	config := &configs.Output{Name: "map-output"}
+	addr := addrs.OutputValue{Name: config.Name}.Absolute(addrs.RootModuleInstance)
+	node := &NodeApplyableOutput{Config: config, Addr: addr}
+	val := cty.MapVal(map[string]cty.Value{
+		"a": cty.StringVal("b").Mark("sensitive"),
+	})
+	ctx.EvaluateExprResult = val
+
+	err := node.Execute(ctx, walkApply)
+	if err == nil {
+		t.Fatal("expected execute error, but there was none")
+	}
+	if got, want := err.Error(), "Output refers to sensitive values"; !strings.Contains(got, want) {
+		t.Errorf("expected error to include %q, but was: %s", want, got)
 	}
+}
+
+func TestNodeApplyableOutputExecute_sensitiveValueAndOutput(t *testing.T) {
+	ctx := new(MockEvalContext)
+	ctx.StateState = states.NewState().SyncWrapper()
+
+	config := &configs.Output{
+		Name:      "map-output",
+		Sensitive: true,
+	}
+	addr := addrs.OutputValue{Name: config.Name}.Absolute(addrs.RootModuleInstance)
+	node := &NodeApplyableOutput{Config: config, Addr: addr}
+	val := cty.MapVal(map[string]cty.Value{
+		"a": cty.StringVal("b").Mark("sensitive"),
+	})
+	ctx.EvaluateExprResult = val
 
+	err := node.Execute(ctx, walkApply)
+	if err != nil {
+		t.Fatalf("unexpected execute error: %s", err)
+	}
+
+	// Unmarked value should be stored in state
+	outputVal := ctx.StateState.OutputValue(addr)
+	want, _ := val.UnmarkDeep()
+	if got := outputVal.Value; !got.RawEquals(want) {
+		t.Errorf("wrong output value in state\n got: %#v\nwant: %#v", got, want)
+	}
 }
 
 func TestNodeDestroyableOutputExecute(t *testing.T) {