From 428d404d9281b39ffb0eefe5085be6303a35a9bc Mon Sep 17 00:00:00 2001
From: Pam Selle <pam@hashicorp.com>
Date: Thu, 17 Dec 2020 11:27:12 -0500
Subject: [PATCH] Allow for_each arguments containing sensitive values if they
 aren't keys (#27247)

* Add test for existing behavior, when a value contains a marked value

* Allow some marked values as for_each arguments

Rather than disallow values that have any marks
as for_each arguments, this makes the check more
nuanced to disallow cases where the whole value
is marked (a whole map, or any set). This allows
cases where a user may pass a map that has marked
values, but the keys are not sensitive
---
 terraform/eval_for_each.go      | 18 ++++++++++++++----
 terraform/eval_for_each_test.go | 23 +++++++++++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go
index c8ddf6ea7a..d2be0a2c42 100644
--- a/terraform/eval_for_each.go
+++ b/terraform/eval_for_each.go
@@ -22,7 +22,7 @@ func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext) (forEach ma
 	// forEachVal might be unknown, but if it is then there should already
 	// be an error about it in diags, which we'll return below.
 
-	if forEachVal.IsNull() || !forEachVal.IsKnown() || forEachVal.LengthInt() == 0 {
+	if forEachVal.IsNull() || !forEachVal.IsKnown() || markSafeLengthInt(forEachVal) == 0 {
 		// we check length, because an empty set return a nil map
 		return map[string]cty.Value{}, diags
 	}
@@ -58,16 +58,20 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU
 
 	forEachVal, forEachDiags := expr.Value(hclCtx)
 	diags = diags.Append(forEachDiags)
-	if forEachVal.ContainsMarked() {
+
+	// If a whole map is marked, or a set contains marked values (which means the set is then marked)
+	// give an error diagnostic as this value cannot be used in for_each
+	if forEachVal.IsMarked() {
 		diags = diags.Append(&hcl.Diagnostic{
 			Severity:    hcl.DiagError,
 			Summary:     "Invalid for_each argument",
-			Detail:      "Sensitive variables, or values derived from sensitive variables, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.",
+			Detail:      "Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.",
 			Subject:     expr.Range().Ptr(),
 			Expression:  expr,
 			EvalContext: hclCtx,
 		})
 	}
+
 	if diags.HasErrors() {
 		return nullMap, diags
 	}
@@ -109,7 +113,7 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU
 		})
 		return nullMap, diags
 
-	case forEachVal.LengthInt() == 0:
+	case markSafeLengthInt(forEachVal) == 0:
 		// If the map is empty ({}), return an empty map, because cty will
 		// return nil when representing {} AsValueMap. This also covers an empty
 		// set (toset([]))
@@ -168,3 +172,9 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU
 }
 
 const errInvalidForEachUnknownDetail = `The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the for_each depends on.`
+
+// markSafeLengthInt allows calling LengthInt on marked values safely
+func markSafeLengthInt(val cty.Value) int {
+	v, _ := val.UnmarkDeep()
+	return v.LengthInt()
+}
diff --git a/terraform/eval_for_each_test.go b/terraform/eval_for_each_test.go
index cc0c9fdedc..2f71566373 100644
--- a/terraform/eval_for_each_test.go
+++ b/terraform/eval_for_each_test.go
@@ -52,6 +52,16 @@ func TestEvaluateForEachExpression_valid(t *testing.T) {
 				"b": cty.UnknownVal(cty.Bool),
 			},
 		},
+		"map containing sensitive values, but strings are literal": {
+			hcltest.MockExprLiteral(cty.MapVal(map[string]cty.Value{
+				"a": cty.BoolVal(true).Mark("sensitive"),
+				"b": cty.BoolVal(false),
+			})),
+			map[string]cty.Value{
+				"a": cty.BoolVal(true).Mark("sensitive"),
+				"b": cty.BoolVal(false),
+			},
+		},
 	}
 
 	for name, test := range tests {
@@ -110,6 +120,14 @@ func TestEvaluateForEachExpression_errors(t *testing.T) {
 			"Invalid for_each argument",
 			"depends on resource attributes that cannot be determined until apply",
 		},
+		"marked map": {
+			hcltest.MockExprLiteral(cty.MapVal(map[string]cty.Value{
+				"a": cty.BoolVal(true),
+				"b": cty.BoolVal(false),
+			}).Mark("sensitive")),
+			"Invalid for_each argument",
+			"Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.",
+		},
 		"set containing booleans": {
 			hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.BoolVal(true)})),
 			"Invalid for_each set argument",
@@ -130,6 +148,11 @@ func TestEvaluateForEachExpression_errors(t *testing.T) {
 			"Invalid for_each argument",
 			"depends on resource attributes that cannot be determined until apply",
 		},
+		"set containing marked values": {
+			hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.StringVal("beep").Mark("sensitive"), cty.StringVal("boop")})),
+			"Invalid for_each argument",
+			"Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.",
+		},
 	}
 
 	for name, test := range tests {