refactor sensitive marks usage (#2503)

Signed-off-by: ollevche <ollevche@gmail.com>

CHANGELOG.md

@@ -31,7 +31,8 @@ BUG FIXES:
 - Fix the error message when default value of a complex variable is containing a wrong type ([2394](https://github.com/opentofu/opentofu/issues/2394))
 - Fix the way OpenTofu downloads a module that is sourced from a GitHub branch containing slashes in the name. ([2396](https://github.com/opentofu/opentofu/issues/2396))
 - `pg` backend doesn't fail on workspace creation for paralel runs, when the database is shared across multiple projects. ([#2411](https://github.com/opentofu/opentofu/pull/2411))
-- Generating an OpenTofu configuration from an `import` block that is referencing a resource with nested attributes now works correctly, instead of giving an error that the nested computed attribute is required. ([2372](https://github.com/opentofu/opentofu/issues/2372)) 
+- Generating an OpenTofu configuration from an `import` block that is referencing a resource with nested attributes now works correctly, instead of giving an error that the nested computed attribute is required. ([#2372](https://github.com/opentofu/opentofu/issues/2372)) 
+- `base64gunzip` now doesn't expose sensitive values if it fails during the base64 decoding. ([#2503](https://github.com/opentofu/opentofu/pull/2503))
 
 ## Previous Releases
 

internal/genconfig/generate_config.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/opentofu/opentofu/internal/addrs"
 	"github.com/opentofu/opentofu/internal/configs/configschema"
+	"github.com/opentofu/opentofu/internal/lang/marks"
 	"github.com/opentofu/opentofu/internal/tfdiags"
 )
 
@@ -152,7 +153,7 @@ func writeConfigAttributesFromExisting(addr addrs.AbsResourceInstance, buf *stri
 			} else {
 				val = attrS.EmptyValue()
 			}
-			if attrS.Sensitive || val.IsMarked() {
+			if attrS.Sensitive || val.HasMark(marks.Sensitive) {
 				buf.WriteString("null # sensitive")
 			} else {
 				if val.Type() == cty.String {
@@ -319,7 +320,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance,
 
 	switch schema.NestedType.Nesting {
 	case configschema.NestingSingle:
-		if schema.Sensitive || stateVal.IsMarked() {
+		if schema.Sensitive || stateVal.HasMark(marks.Sensitive) {
 			buf.WriteString(strings.Repeat(" ", indent))
 			buf.WriteString(fmt.Sprintf("%s = {} # sensitive\n", name))
 			return diags
@@ -349,7 +350,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance,
 
 	case configschema.NestingList, configschema.NestingSet:
 
-		if schema.Sensitive || stateVal.IsMarked() {
+		if schema.Sensitive || stateVal.HasMark(marks.Sensitive) {
 			buf.WriteString(strings.Repeat(" ", indent))
 			buf.WriteString(fmt.Sprintf("%s = [] # sensitive\n", name))
 			return diags
@@ -369,7 +370,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance,
 			buf.WriteString(strings.Repeat(" ", indent+2))
 
 			// The entire element is marked.
-			if listVals[i].IsMarked() {
+			if listVals[i].HasMark(marks.Sensitive) {
 				buf.WriteString("{}, # sensitive\n")
 				continue
 			}
@@ -384,7 +385,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance,
 		return diags
 
 	case configschema.NestingMap:
-		if schema.Sensitive || stateVal.IsMarked() {
+		if schema.Sensitive || stateVal.HasMark(marks.Sensitive) {
 			buf.WriteString(strings.Repeat(" ", indent))
 			buf.WriteString(fmt.Sprintf("%s = {} # sensitive\n", name))
 			return diags
@@ -412,7 +413,7 @@ func writeConfigNestedTypeAttributeFromExisting(addr addrs.AbsResourceInstance,
 			buf.WriteString(fmt.Sprintf("%s = {", key))
 
 			// This entire value is marked
-			if vals[key].IsMarked() {
+			if vals[key].HasMark(marks.Sensitive) {
 				buf.WriteString("} # sensitive\n")
 				continue
 			}
@@ -444,7 +445,7 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str
 		buf.WriteString(fmt.Sprintf("%s {", name))
 
 		// If the entire value is marked, don't print any nested attributes
-		if stateVal.IsMarked() {
+		if stateVal.HasMark(marks.Sensitive) {
 			buf.WriteString("} # sensitive\n")
 			return diags
 		}
@@ -454,7 +455,7 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str
 		buf.WriteString("}\n")
 		return diags
 	case configschema.NestingList, configschema.NestingSet:
-		if stateVal.IsMarked() {
+		if stateVal.HasMark(marks.Sensitive) {
 			buf.WriteString(strings.Repeat(" ", indent))
 			buf.WriteString(fmt.Sprintf("%s {} # sensitive\n", name))
 			return diags
@@ -470,7 +471,7 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str
 		return diags
 	case configschema.NestingMap:
 		// If the entire value is marked, don't print any nested attributes
-		if stateVal.IsMarked() {
+		if stateVal.HasMark(marks.Sensitive) {
 			buf.WriteString(fmt.Sprintf("%s {} # sensitive\n", name))
 			return diags
 		}
@@ -485,7 +486,7 @@ func writeConfigNestedBlockFromExisting(addr addrs.AbsResourceInstance, buf *str
 			buf.WriteString(strings.Repeat(" ", indent))
 			buf.WriteString(fmt.Sprintf("%s %q {", name, key))
 			// This entire map element is marked
-			if vals[key].IsMarked() {
+			if vals[key].HasMark(marks.Sensitive) {
 				buf.WriteString("} # sensitive\n")
 				return diags
 			}

internal/lang/funcs/collection.go

@@ -288,29 +288,29 @@ var LookupFunc = function.New(&function.Spec{
 		}
 
 		// keep track of marks from the collection and key
-		var markses []cty.ValueMarks
+		var marks []cty.ValueMarks
 
 		// unmark collection, retain marks to reapply later
 		mapVar, mapMarks := args[0].Unmark()
-		markses = append(markses, mapMarks)
+		marks = append(marks, mapMarks)
 
 		// include marks on the key in the result
 		keyVal, keyMarks := args[1].Unmark()
 		if len(keyMarks) > 0 {
-			markses = append(markses, keyMarks)
+			marks = append(marks, keyMarks)
 		}
 		lookupKey := keyVal.AsString()
 
 		if !mapVar.IsKnown() {
-			return cty.UnknownVal(retType).WithMarks(markses...), nil
+			return cty.UnknownVal(retType).WithMarks(marks...), nil
 		}
 
 		if mapVar.Type().IsObjectType() {
 			if mapVar.Type().HasAttribute(lookupKey) {
-				return mapVar.GetAttr(lookupKey).WithMarks(markses...), nil
+				return mapVar.GetAttr(lookupKey).WithMarks(marks...), nil
 			}
 		} else if mapVar.HasIndex(cty.StringVal(lookupKey)) == cty.True {
-			return mapVar.Index(cty.StringVal(lookupKey)).WithMarks(markses...), nil
+			return mapVar.Index(cty.StringVal(lookupKey)).WithMarks(marks...), nil
 		}
 
 		if defaultValueSet {
@@ -318,7 +318,7 @@ var LookupFunc = function.New(&function.Spec{
 			if err != nil {
 				return cty.NilVal, err
 			}
-			return defaultVal.WithMarks(markses...), nil
+			return defaultVal.WithMarks(marks...), nil
 		}
 
 		return cty.UnknownVal(cty.DynamicPseudoType), fmt.Errorf(

internal/lang/funcs/encoding.go

@@ -181,12 +181,13 @@ var Base64GzipFunc = function.New(&function.Spec{
 	},
 })
 
-// Base64GunzipFunc constructs a function that Bae64 decodes a string and decompresses the result with gunzip.
+// Base64GunzipFunc constructs a function that Base64 decodes a string and decompresses the result with gunzip.
 var Base64GunzipFunc = function.New(&function.Spec{
 	Params: []function.Parameter{
 		{
-			Name: "str",
-			Type: cty.String,
+			Name:        "str",
+			Type:        cty.String,
+			AllowMarked: true,
 		},
 	},
 	Type:         function.StaticReturnType(cty.String),
@@ -208,7 +209,7 @@ var Base64GunzipFunc = function.New(&function.Spec{
 			return cty.UnknownVal(cty.String), fmt.Errorf("failed to read gunzip raw data: %w", err)
 		}
 
-		return cty.StringVal(string(gunzip)), nil
+		return cty.StringVal(string(gunzip)).WithMarks(strMarks), nil
 	},
 })
 

internal/lang/funcs/encoding_test.go

@@ -166,12 +166,27 @@ func TestBase64Gunzip(t *testing.T) {
 	tests := []struct {
 		String cty.Value
 		Want   cty.Value
-		Err    bool
+		Err    string
 	}{
 		{
 			cty.StringVal("H4sIAAAAAAAA/ypJLS4BAAAA//8BAAD//wx+f9gEAAAA"),
 			cty.StringVal("test"),
-			false,
+			"",
+		},
+		{
+			cty.StringVal("H4sIAAAAAAAA/ypJLS4BAAAA//8BAAD//wx+f9gEAAAA").Mark(marks.Sensitive),
+			cty.StringVal("test").Mark(marks.Sensitive),
+			"",
+		},
+		{
+			cty.StringVal("hello"),
+			cty.NilVal,
+			`failed to decode base64 data "hello"`,
+		},
+		{
+			cty.StringVal("hello").Mark(marks.Sensitive),
+			cty.NilVal,
+			`failed to decode base64 data (sensitive value)`,
 		},
 	}
 
@@ -179,10 +194,13 @@ func TestBase64Gunzip(t *testing.T) {
 		t.Run(fmt.Sprintf("base64gunzip(%#v)", test.String), func(t *testing.T) {
 			got, err := Base64Gunzip(test.String)
 
-			if test.Err {
+			if test.Err != "" {
 				if err == nil {
 					t.Fatal("succeeded; want error")
 				}
+				if err.Error() != test.Err {
+					t.Fatalf("got unexpected error: %v", err.Error())
+				}
 				return
 			} else if err != nil {
 				t.Fatalf("unexpected error: %s", err)

internal/lang/funcs/redact.go

@@ -12,8 +12,8 @@ import (
 	"github.com/zclconf/go-cty/cty"
 )
 
-func redactIfSensitive(value interface{}, markses ...cty.ValueMarks) string {
-	if marks.Has(cty.DynamicVal.WithMarks(markses...), marks.Sensitive) {
+func redactIfSensitive(value interface{}, valueMarks ...cty.ValueMarks) string {
+	if marks.Has(cty.DynamicVal.WithMarks(valueMarks...), marks.Sensitive) {
 		return "(sensitive value)"
 	}
 	switch v := value.(type) {

internal/lang/funcs/sensitive.go

@@ -30,8 +30,7 @@ var SensitiveFunc = function.New(&function.Spec{
 		return args[0].Type(), nil
 	},
 	Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) {
-		val, _ := args[0].Unmark()
-		return val.Mark(marks.Sensitive), nil
+		return args[0].Mark(marks.Sensitive), nil
 	},
 })
 

internal/lang/funcs/sensitive_test.go

@@ -48,10 +48,8 @@ func TestSensitive(t *testing.T) {
 			``,
 		},
 		{
-			// A value with some non-standard mark gets "fixed" to be marked
-			// with the standard "sensitive" mark. (This situation occurring
-			// would imply an inconsistency/bug elsewhere, so we're just
-			// being robust about it here.)
+			// Any non-sensitive marks must be propagated alongside
+			// with a sensitive one.
 			cty.NumberIntVal(1).Mark("bloop"),
 			``,
 		},
@@ -83,15 +81,11 @@ func TestSensitive(t *testing.T) {
 				t.Errorf("result is not marked sensitive")
 			}
 
+			inputMarks := test.Input.Marks()
+			delete(inputMarks, marks.Sensitive)
+
 			gotRaw, gotMarks := got.Unmark()
-			if len(gotMarks) != 1 {
-				// We're only expecting to have the "sensitive" mark we checked
-				// above. Any others are an error, even if they happen to
-				// appear alongside "sensitive". (We might change this rule
-				// if someday we decide to use marks for some additional
-				// unrelated thing in OpenTofu, but currently we assume that
-				// _all_ marks imply sensitive, and so returning any other
-				// marks would be confusing.)
+			if len(gotMarks) != len(inputMarks)+1 {
 				t.Errorf("extraneous marks %#v", gotMarks)
 			}
 

internal/states/instance_object.go

@@ -12,6 +12,7 @@ import (
 	ctyjson "github.com/zclconf/go-cty/cty/json"
 
 	"github.com/opentofu/opentofu/internal/addrs"
+	"github.com/opentofu/opentofu/internal/lang/marks"
 )
 
 // ResourceInstanceObject is the local representation of a specific remote
@@ -97,7 +98,15 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res
 	// If it contains marks, remove these marks before traversing the
 	// structure with UnknownAsNull, and save the PathValueMarks
 	// so we can save them in state.
-	val, pvm := o.Value.UnmarkDeepWithPaths()
+	val, allPVM := o.Value.UnmarkDeepWithPaths()
+
+	var sensitivePVM = make([]cty.PathValueMarks, 0, len(allPVM))
+
+	for _, pvm := range allPVM {
+		if _, ok := pvm.Marks[marks.Sensitive]; ok {
+			sensitivePVM = append(sensitivePVM, pvm)
+		}
+	}
 
 	// Our state serialization can't represent unknown values, so we convert
 	// them to nulls here. This is lossy, but nobody should be writing unknown
@@ -130,7 +139,7 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res
 	return &ResourceInstanceObjectSrc{
 		SchemaVersion:       schemaVersion,
 		AttrsJSON:           src,
-		AttrSensitivePaths:  pvm,
+		AttrSensitivePaths:  sensitivePVM,
 		Private:             o.Private,
 		Status:              o.Status,
 		Dependencies:        dependencies,

internal/states/instance_object_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/opentofu/opentofu/internal/addrs"
+	"github.com/opentofu/opentofu/internal/lang/marks"
 	"github.com/zclconf/go-cty/cty"
 )
 
@@ -33,22 +34,22 @@ func TestResourceInstanceObject_encode(t *testing.T) {
 
 	// multiple instances may have been assigned the same deps slice
 	objs := []*ResourceInstanceObject{
-		&ResourceInstanceObject{
+		{
 			Value:        value,
 			Status:       ObjectPlanned,
 			Dependencies: depsOne,
 		},
-		&ResourceInstanceObject{
+		{
 			Value:        value,
 			Status:       ObjectPlanned,
 			Dependencies: depsTwo,
 		},
-		&ResourceInstanceObject{
+		{
 			Value:        value,
 			Status:       ObjectPlanned,
 			Dependencies: depsOne,
 		},
-		&ResourceInstanceObject{
+		{
 			Value:        value,
 			Status:       ObjectPlanned,
 			Dependencies: depsOne,
@@ -86,3 +87,56 @@ func TestResourceInstanceObject_encode(t *testing.T) {
 		}
 	}
 }
+
+func TestResourceInstanceObject_encode_sensitivity(t *testing.T) {
+	depsOne := []addrs.ConfigResource{
+		addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "honk"),
+		addrs.RootModule.Child("child").Resource(addrs.ManagedResourceMode, "test", "flub"),
+		addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "boop"),
+	}
+
+	tests := []struct {
+		inputObj           *ResourceInstanceObject
+		wantSensitivePaths bool
+	}{
+		{
+			inputObj: &ResourceInstanceObject{
+				Value: cty.ObjectVal(map[string]cty.Value{
+					"foo": cty.ObjectVal(map[string]cty.Value{
+						"bar": cty.BoolVal(true).Mark(marks.Sensitive),
+					}),
+				}),
+				Status:       ObjectPlanned,
+				Dependencies: depsOne,
+			},
+			wantSensitivePaths: true,
+		},
+		{
+			inputObj: &ResourceInstanceObject{
+				Value: cty.ObjectVal(map[string]cty.Value{
+					"foo": cty.ObjectVal(map[string]cty.Value{
+						"bar": cty.BoolVal(true).Mark("non-sensitive"),
+					}),
+				}),
+				Status:       ObjectPlanned,
+				Dependencies: depsOne,
+			},
+			wantSensitivePaths: false,
+		},
+	}
+
+	for _, test := range tests {
+		encoded, err := test.inputObj.Encode(test.inputObj.Value.Type(), 0)
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+
+		if test.wantSensitivePaths && len(encoded.AttrSensitivePaths) == 0 {
+			t.Fatalf("No AttrSensitivePaths found")
+		}
+
+		if !test.wantSensitivePaths && len(encoded.AttrSensitivePaths) != 0 {
+			t.Fatalf("Got unexpected AttrSensitivePaths: %v", encoded.AttrSensitivePaths)
+		}
+	}
+}

internal/tofu/node_output.go

@@ -616,8 +616,13 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 	// non-root outputs need to keep sensitive marks for evaluation, but are
 	// not serialized.
 	if n.Addr.Module.IsRoot() {
-		val, _ = val.UnmarkDeep()
-		val = cty.UnknownAsNull(val)
+		var valMarks cty.ValueMarks
+
+		val, valMarks = val.UnmarkDeep()
+
+		delete(valMarks, marks.Sensitive)
+
+		val = cty.UnknownAsNull(val).WithMarks(valMarks)
 	}
 
 	state.SetOutputValue(n.Addr, val, n.Config.Sensitive)

internal/tofu/node_output_test.go

@@ -122,6 +122,36 @@ func TestNodeApplyableOutputExecute_sensitiveValueNotOutput(t *testing.T) {
 	}
 }
 
+func TestNodeApplyableOutputExecute_alternativelyMarkedValue(t *testing.T) {
+	ctx := new(MockEvalContext)
+	ctx.StateState = states.NewState().SyncWrapper()
+	ctx.ChecksState = checks.NewState(nil)
+
+	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("alternative-mark"),
+	})
+	ctx.EvaluateExprResult = val
+
+	diags := node.Execute(ctx, walkApply)
+	if diags.HasErrors() {
+		t.Fatalf("Got unexpected error: %v", diags)
+	}
+
+	modOutputAddr, diags := addrs.ParseAbsOutputValueStr("output.map-output")
+	if diags.HasErrors() {
+		t.Fatalf("Invalid mod addr in test: %v", diags)
+	}
+
+	stateVal := ctx.StateState.OutputValue(modOutputAddr)
+
+	if !stateVal.Value.HasMark("alternative-mark") {
+		t.Fatalf("Non-sensitive mark has been erased")
+	}
+}
+
 func TestNodeApplyableOutputExecute_sensitiveValueAndOutput(t *testing.T) {
 	ctx := new(MockEvalContext)
 	ctx.StateState = states.NewState().SyncWrapper()