From 55ebb2708c5c5538555ce21880f2bac4744292f4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 25 Jun 2021 14:13:57 -0400 Subject: [PATCH] remove IsMarked and ContainsMarked calls Make sure sensitivity checks are looking for specific marks rather than any marks at all. --- internal/command/format/diff.go | 15 ++++++++------- internal/command/views/add.go | 22 ++++++++++++---------- internal/command/views/add_test.go | 10 +++++----- internal/command/views/json/diagnostic.go | 5 +++-- internal/lang/funcs/conversion.go | 14 ++++++++------ internal/lang/funcs/conversion_test.go | 13 +++++++++++++ internal/terraform/eval_for_each.go | 3 ++- internal/terraform/node_output.go | 4 +--- 8 files changed, 52 insertions(+), 34 deletions(-) diff --git a/internal/command/format/diff.go b/internal/command/format/diff.go index 79a86f99a..6421f6ee6 100644 --- a/internal/command/format/diff.go +++ b/internal/command/format/diff.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/plans/objchange" "github.com/hashicorp/terraform/internal/states" @@ -733,7 +734,7 @@ func (p *blockBodyDiffPrinter) writeNestedBlockDiffs(name string, blockS *config // If either the old or the new value is marked, // Display a special diff because it is irrelevant // to list all obfuscated attributes as (sensitive) - if old.IsMarked() || new.IsMarked() { + if old.HasMark(marks.Sensitive) || new.HasMark(marks.Sensitive) { p.writeSensitiveNestedBlockDiff(name, old, new, indent, blankBefore, path) return 0 } @@ -1012,7 +1013,7 @@ func (p *blockBodyDiffPrinter) writeNestedBlockDiff(name string, label *string, func (p *blockBodyDiffPrinter) writeValue(val cty.Value, action plans.Action, indent int) { // Could check specifically for the sensitivity marker - if val.IsMarked() { + if val.HasMark(marks.Sensitive) { p.buf.WriteString("(sensitive)") return } @@ -1177,7 +1178,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // However, these specialized implementations can apply only if both // values are known and non-null. if old.IsKnown() && new.IsKnown() && !old.IsNull() && !new.IsNull() && typesEqual { - if old.IsMarked() || new.IsMarked() { + if old.HasMark(marks.Sensitive) || new.HasMark(marks.Sensitive) { p.buf.WriteString("(sensitive)") if p.pathForcesNewResource(path) { p.buf.WriteString(p.color.Color(forcesNewResourceCaption)) @@ -1548,7 +1549,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa switch action { case plans.Create, plans.NoOp: v := new.Index(kV) - if v.IsMarked() { + if v.HasMark(marks.Sensitive) { p.buf.WriteString("(sensitive)") } else { p.writeValue(v, action, indent+4) @@ -1558,7 +1559,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa newV := cty.NullVal(oldV.Type()) p.writeValueDiff(oldV, newV, indent+4, path) default: - if oldV.IsMarked() || newV.IsMarked() { + if oldV.HasMark(marks.Sensitive) || newV.HasMark(marks.Sensitive) { p.buf.WriteString("(sensitive)") } else { p.writeValueDiff(oldV, newV, indent+4, path) @@ -1738,7 +1739,7 @@ func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, inden } } - if new.IsMarked() && !old.IsMarked() { + if new.HasMark(marks.Sensitive) && !old.HasMark(marks.Sensitive) { p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteString(p.color.Color(fmt.Sprintf("# [yellow]Warning:[reset] this %s will be marked as sensitive and will not\n", diffType))) p.buf.WriteString(strings.Repeat(" ", indent)) @@ -1746,7 +1747,7 @@ func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, inden } // Note if changing this attribute will change its sensitivity - if old.IsMarked() && !new.IsMarked() { + if old.HasMark(marks.Sensitive) && !new.HasMark(marks.Sensitive) { p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteString(p.color.Color(fmt.Sprintf("# [yellow]Warning:[reset] this %s will no longer be marked as sensitive\n", diffType))) p.buf.WriteString(strings.Repeat(" ", indent)) diff --git a/internal/command/views/add.go b/internal/command/views/add.go index c4e0bb67d..4b1c96d70 100644 --- a/internal/command/views/add.go +++ b/internal/command/views/add.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -156,9 +157,10 @@ func (v *addHuman) writeConfigAttributesFromExisting(buf *strings.Builder, state } else { val = attrS.EmptyValue() } - if attrS.Sensitive || val.IsMarked() { + if attrS.Sensitive || val.HasMark(marks.Sensitive) { buf.WriteString("null # sensitive") } else { + val, _ = val.Unmark() tok := hclwrite.TokensForValue(val) if _, err := tok.WriteTo(buf); err != nil { return err @@ -322,7 +324,7 @@ func (v *addHuman) writeConfigBlocksFromExisting(buf *strings.Builder, stateVal func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Builder, name string, schema *configschema.Attribute, stateVal cty.Value, indent int) error { 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 nil @@ -347,7 +349,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build buf.WriteString(strings.Repeat(" ", indent)) buf.WriteString(fmt.Sprintf("%s = [", name)) - if schema.Sensitive || stateVal.IsMarked() { + if schema.Sensitive || stateVal.HasMark(marks.Sensitive) { buf.WriteString("] # sensitive\n") return nil } @@ -359,7 +361,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build 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 } @@ -379,7 +381,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build buf.WriteString(strings.Repeat(" ", indent)) buf.WriteString(fmt.Sprintf("%s = {", name)) - if schema.Sensitive || stateVal.IsMarked() { + if schema.Sensitive || stateVal.HasMark(marks.Sensitive) { buf.WriteString(" } # sensitive\n") return nil } @@ -397,7 +399,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build 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 } @@ -426,7 +428,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name 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 nil } @@ -440,7 +442,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name buf.WriteString("}\n") return nil 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 nil @@ -460,7 +462,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name return nil 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 nil } @@ -475,7 +477,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name 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 nil } diff --git a/internal/command/views/add_test.go b/internal/command/views/add_test.go index eae238d9d..1ad0cc747 100644 --- a/internal/command/views/add_test.go +++ b/internal/command/views/add_test.go @@ -343,7 +343,7 @@ func TestAdd_writeConfigBlocksFromExisting(t *testing.T) { v := addHuman{optional: true} val := cty.ObjectVal(map[string]cty.Value{ "root_block_device": cty.ObjectVal(map[string]cty.Value{ - "volume_type": cty.StringVal("foo").Mark("bar"), + "volume_type": cty.StringVal("foo").Mark(marks.Sensitive), }), }) schema := addTestSchema(configschema.NestingSingle) @@ -366,7 +366,7 @@ func TestAdd_writeConfigBlocksFromExisting(t *testing.T) { "root_block_device": cty.ObjectVal(map[string]cty.Value{ "volume_type": cty.StringVal("foo"), }), - }).Mark("bar") + }).Mark(marks.Sensitive) schema := addTestSchema(configschema.NestingSingle) var buf strings.Builder v.writeConfigBlocksFromExisting(&buf, val, schema.BlockTypes, 0) @@ -447,7 +447,7 @@ root_block_device { cty.ObjectVal(map[string]cty.Value{ "volume_type": cty.StringVal("bar"), }), - }).Mark("mark"), + }).Mark(marks.Sensitive), }) schema := addTestSchema(configschema.NestingList) var buf strings.Builder @@ -784,13 +784,13 @@ func TestAdd_WriteConfigNestedTypeAttributeFromExisting(t *testing.T) { "disks": cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ "mount_point": cty.StringVal("/mnt/foo"), - "size": cty.StringVal("50GB").Mark("hi"), + "size": cty.StringVal("50GB").Mark(marks.Sensitive), }), // This is an odd example, where the entire element is marked. cty.ObjectVal(map[string]cty.Value{ "mount_point": cty.StringVal("/mnt/bar"), "size": cty.StringVal("250GB"), - }).Mark("bye"), + }).Mark(marks.Sensitive), }), }) diff --git a/internal/command/views/json/diagnostic.go b/internal/command/views/json/diagnostic.go index b427d443c..85bc394ef 100644 --- a/internal/command/views/json/diagnostic.go +++ b/internal/command/views/json/diagnostic.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hcled" "github.com/hashicorp/hcl/v2/hclparse" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -260,7 +261,7 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost Traversal: traversalStr, } switch { - case val.IsMarked(): + case val.HasMark(marks.Sensitive): // We won't say anything at all about sensitive values, // because we might give away something that was // sensitive about them. @@ -323,7 +324,7 @@ func compactValueStr(val cty.Value) string { // helpful but concise messages in diagnostics. It is not comprehensive // nor intended to be used for other purposes. - if val.IsMarked() { + if val.HasMark(marks.Sensitive) { // We check this in here just to make sure, but note that the caller // of compactValueStr ought to have already checked this and skipped // calling into compactValueStr anyway, so this shouldn't actually diff --git a/internal/lang/funcs/conversion.go b/internal/lang/funcs/conversion.go index b557bb5a9..89dfd00f0 100644 --- a/internal/lang/funcs/conversion.go +++ b/internal/lang/funcs/conversion.go @@ -6,6 +6,7 @@ import ( "strconv" "strings" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" "github.com/zclconf/go-cty/cty/function" @@ -62,28 +63,29 @@ func MakeToFunc(wantTy cty.Type) function.Function { // to be known here but may still be null. ret, err := convert.Convert(args[0], retType) if err != nil { + val, _ := args[0].UnmarkDeep() // Because we used GetConversionUnsafe above, conversion can // still potentially fail in here. For example, if the user // asks to convert the string "a" to bool then we'll // optimistically permit it during type checking but fail here // once we note that the value isn't either "true" or "false". - gotTy := args[0].Type() + gotTy := val.Type() switch { - case args[0].ContainsMarked(): + case marks.Contains(args[0], marks.Sensitive): // Generic message so we won't inadvertently disclose // information about sensitive values. return cty.NilVal, function.NewArgErrorf(0, "cannot convert this sensitive %s to %s", gotTy.FriendlyName(), wantTy.FriendlyNameForConstraint()) case gotTy == cty.String && wantTy == cty.Bool: what := "string" - if !args[0].IsNull() { - what = strconv.Quote(args[0].AsString()) + if !val.IsNull() { + what = strconv.Quote(val.AsString()) } return cty.NilVal, function.NewArgErrorf(0, `cannot convert %s to bool; only the strings "true" or "false" are allowed`, what) case gotTy == cty.String && wantTy == cty.Number: what := "string" - if !args[0].IsNull() { - what = strconv.Quote(args[0].AsString()) + if !val.IsNull() { + what = strconv.Quote(val.AsString()) } return cty.NilVal, function.NewArgErrorf(0, `cannot convert %s to number; given string must be a decimal representation of a number`, what) default: diff --git a/internal/lang/funcs/conversion_test.go b/internal/lang/funcs/conversion_test.go index 97460f5b5..cec2e23c9 100644 --- a/internal/lang/funcs/conversion_test.go +++ b/internal/lang/funcs/conversion_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/zclconf/go-cty/cty" ) @@ -61,6 +62,12 @@ func TestTo(t *testing.T) { cty.StringVal("a").Mark("boop"), cty.Bool, cty.DynamicVal, + `cannot convert "a" to bool; only the strings "true" or "false" are allowed`, + }, + { + cty.StringVal("a").Mark(marks.Sensitive), + cty.Bool, + cty.DynamicVal, `cannot convert this sensitive string to bool`, }, { @@ -73,6 +80,12 @@ func TestTo(t *testing.T) { cty.StringVal("a").Mark("boop"), cty.Number, cty.DynamicVal, + `cannot convert "a" to number; given string must be a decimal representation of a number`, + }, + { + cty.StringVal("a").Mark(marks.Sensitive), + cty.Number, + cty.DynamicVal, `cannot convert this sensitive string to number`, }, { diff --git a/internal/terraform/eval_for_each.go b/internal/terraform/eval_for_each.go index 3d1710ab8..fccf58f91 100644 --- a/internal/terraform/eval_for_each.go +++ b/internal/terraform/eval_for_each.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/internal/lang" + "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -61,7 +62,7 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU // 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() { + if forEachVal.HasMark(marks.Sensitive) { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid for_each argument", diff --git a/internal/terraform/node_output.go b/internal/terraform/node_output.go index e09cab577..93ff30aae 100644 --- a/internal/terraform/node_output.go +++ b/internal/terraform/node_output.go @@ -283,9 +283,7 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) (diags // a sensitive result, to help avoid accidental exposure in the state // of a sensitive value that the user doesn't want to include there. if n.Addr.Module.IsRoot() { - _, m := val.UnmarkDeep() - _, hasSensitive := m[marks.Sensitive] - if !n.Config.Sensitive && hasSensitive { + if !n.Config.Sensitive && marks.Contains(val, marks.Sensitive) { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Output refers to sensitive values",