From da4ddd01605cd49b3016db21fd614813c0f3a74c Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Mon, 12 Oct 2020 15:37:44 -0400 Subject: [PATCH] Avoid disclosing values in errors on marked vals AssertObjectCompatible is a special case that will expose Go string values of values unless otherwise stopped. This adds that check. --- plans/objchange/compatible.go | 26 +++++++--------- plans/objchange/compatible_test.go | 50 ++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 5536ad41c..6dc622dfa 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -51,18 +51,17 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu actualV := actual.GetAttr(name) path := append(path, cty.GetAttrStep{Name: name}) - // If our value is marked, unmark it here before - // checking value assertions - unmarkedActualV := actualV - if actualV.ContainsMarked() { - unmarkedActualV, _ = actualV.UnmarkDeep() - } - unmarkedPlannedV := plannedV - if plannedV.ContainsMarked() { - unmarkedPlannedV, _ = actualV.UnmarkDeep() - } + + // Unmark values here before checking value assertions, + // but save the marks so we can see if we should supress + // exposing a value through errors + unmarkedActualV, marksA := actualV.UnmarkDeep() + unmarkedPlannedV, marksP := plannedV.UnmarkDeep() + _, isMarkedActual := marksA["sensitive"] + _, isMarkedPlanned := marksP["sensitive"] + moreErrs := assertValueCompatible(unmarkedPlannedV, unmarkedActualV, path) - if attrS.Sensitive { + if attrS.Sensitive || isMarkedActual || isMarkedPlanned { if len(moreErrs) > 0 { // Use a vague placeholder message instead, to avoid disclosing // sensitive information. @@ -73,9 +72,8 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu } } for name, blockS := range schema.BlockTypes { - // Unmark values before testing compatibility - plannedV, _ := planned.GetAttr(name).UnmarkDeep() - actualV, _ := actual.GetAttr(name).UnmarkDeep() + plannedV, _ := planned.GetAttr(name).Unmark() + actualV, _ := actual.GetAttr(name).Unmark() // As a special case, if there were any blocks whose leaf attributes // are all unknown then we assume (possibly incorrectly) that the diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go index 4c67e610d..1eacdd8b1 100644 --- a/plans/objchange/compatible_test.go +++ b/plans/objchange/compatible_test.go @@ -144,6 +144,56 @@ func TestAssertObjectCompatible(t *testing.T) { `.name: inconsistent values for sensitive attribute`, }, }, + { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "name": { + Type: cty.String, + Required: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "name": cty.StringVal("wotsit").Mark("sensitive"), + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "name": cty.StringVal("thingy"), + }), + []string{ + `.name: inconsistent values for sensitive attribute`, + }, + }, + { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "name": { + Type: cty.String, + Required: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "name": cty.StringVal("wotsit"), + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "name": cty.StringVal("thingy").Mark("sensitive"), + }), + []string{ + `.name: inconsistent values for sensitive attribute`, + }, + }, { &configschema.Block{ Attributes: map[string]*configschema.Attribute{