remove IsMarked and ContainsMarked calls

Make sure sensitivity checks are looking for specific marks rather than
any marks at all.
This commit is contained in:
James Bardin 2021-06-25 14:13:57 -04:00
parent cdf7469efd
commit 55ebb2708c
8 changed files with 52 additions and 34 deletions

View File

@ -14,6 +14,7 @@ import (
"github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/configs/configschema" "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"
"github.com/hashicorp/terraform/internal/plans/objchange" "github.com/hashicorp/terraform/internal/plans/objchange"
"github.com/hashicorp/terraform/internal/states" "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, // If either the old or the new value is marked,
// Display a special diff because it is irrelevant // Display a special diff because it is irrelevant
// to list all obfuscated attributes as (sensitive) // 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) p.writeSensitiveNestedBlockDiff(name, old, new, indent, blankBefore, path)
return 0 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) { func (p *blockBodyDiffPrinter) writeValue(val cty.Value, action plans.Action, indent int) {
// Could check specifically for the sensitivity marker // Could check specifically for the sensitivity marker
if val.IsMarked() { if val.HasMark(marks.Sensitive) {
p.buf.WriteString("(sensitive)") p.buf.WriteString("(sensitive)")
return 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 // However, these specialized implementations can apply only if both
// values are known and non-null. // values are known and non-null.
if old.IsKnown() && new.IsKnown() && !old.IsNull() && !new.IsNull() && typesEqual { 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)") p.buf.WriteString("(sensitive)")
if p.pathForcesNewResource(path) { if p.pathForcesNewResource(path) {
p.buf.WriteString(p.color.Color(forcesNewResourceCaption)) p.buf.WriteString(p.color.Color(forcesNewResourceCaption))
@ -1548,7 +1549,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa
switch action { switch action {
case plans.Create, plans.NoOp: case plans.Create, plans.NoOp:
v := new.Index(kV) v := new.Index(kV)
if v.IsMarked() { if v.HasMark(marks.Sensitive) {
p.buf.WriteString("(sensitive)") p.buf.WriteString("(sensitive)")
} else { } else {
p.writeValue(v, action, indent+4) 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()) newV := cty.NullVal(oldV.Type())
p.writeValueDiff(oldV, newV, indent+4, path) p.writeValueDiff(oldV, newV, indent+4, path)
default: default:
if oldV.IsMarked() || newV.IsMarked() { if oldV.HasMark(marks.Sensitive) || newV.HasMark(marks.Sensitive) {
p.buf.WriteString("(sensitive)") p.buf.WriteString("(sensitive)")
} else { } else {
p.writeValueDiff(oldV, newV, indent+4, path) 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(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(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)) 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 // 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(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(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)) p.buf.WriteString(strings.Repeat(" ", indent))

View File

@ -10,6 +10,7 @@ import (
"github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/command/arguments"
"github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/configs/configschema"
"github.com/hashicorp/terraform/internal/lang/marks"
"github.com/hashicorp/terraform/internal/tfdiags" "github.com/hashicorp/terraform/internal/tfdiags"
"github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty"
) )
@ -156,9 +157,10 @@ func (v *addHuman) writeConfigAttributesFromExisting(buf *strings.Builder, state
} else { } else {
val = attrS.EmptyValue() val = attrS.EmptyValue()
} }
if attrS.Sensitive || val.IsMarked() { if attrS.Sensitive || val.HasMark(marks.Sensitive) {
buf.WriteString("null # sensitive") buf.WriteString("null # sensitive")
} else { } else {
val, _ = val.Unmark()
tok := hclwrite.TokensForValue(val) tok := hclwrite.TokensForValue(val)
if _, err := tok.WriteTo(buf); err != nil { if _, err := tok.WriteTo(buf); err != nil {
return err 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 { func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Builder, name string, schema *configschema.Attribute, stateVal cty.Value, indent int) error {
switch schema.NestedType.Nesting { switch schema.NestedType.Nesting {
case configschema.NestingSingle: case configschema.NestingSingle:
if schema.Sensitive || stateVal.IsMarked() { if schema.Sensitive || stateVal.HasMark(marks.Sensitive) {
buf.WriteString(strings.Repeat(" ", indent)) buf.WriteString(strings.Repeat(" ", indent))
buf.WriteString(fmt.Sprintf("%s = {} # sensitive\n", name)) buf.WriteString(fmt.Sprintf("%s = {} # sensitive\n", name))
return nil return nil
@ -347,7 +349,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build
buf.WriteString(strings.Repeat(" ", indent)) buf.WriteString(strings.Repeat(" ", indent))
buf.WriteString(fmt.Sprintf("%s = [", name)) buf.WriteString(fmt.Sprintf("%s = [", name))
if schema.Sensitive || stateVal.IsMarked() { if schema.Sensitive || stateVal.HasMark(marks.Sensitive) {
buf.WriteString("] # sensitive\n") buf.WriteString("] # sensitive\n")
return nil return nil
} }
@ -359,7 +361,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build
buf.WriteString(strings.Repeat(" ", indent+2)) buf.WriteString(strings.Repeat(" ", indent+2))
// The entire element is marked. // The entire element is marked.
if listVals[i].IsMarked() { if listVals[i].HasMark(marks.Sensitive) {
buf.WriteString("{}, # sensitive\n") buf.WriteString("{}, # sensitive\n")
continue continue
} }
@ -379,7 +381,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build
buf.WriteString(strings.Repeat(" ", indent)) buf.WriteString(strings.Repeat(" ", indent))
buf.WriteString(fmt.Sprintf("%s = {", name)) buf.WriteString(fmt.Sprintf("%s = {", name))
if schema.Sensitive || stateVal.IsMarked() { if schema.Sensitive || stateVal.HasMark(marks.Sensitive) {
buf.WriteString(" } # sensitive\n") buf.WriteString(" } # sensitive\n")
return nil return nil
} }
@ -397,7 +399,7 @@ func (v *addHuman) writeConfigNestedTypeAttributeFromExisting(buf *strings.Build
buf.WriteString(fmt.Sprintf("%s = {", key)) buf.WriteString(fmt.Sprintf("%s = {", key))
// This entire value is marked // This entire value is marked
if vals[key].IsMarked() { if vals[key].HasMark(marks.Sensitive) {
buf.WriteString("} # sensitive\n") buf.WriteString("} # sensitive\n")
continue continue
} }
@ -426,7 +428,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name
buf.WriteString(fmt.Sprintf("%s {", name)) buf.WriteString(fmt.Sprintf("%s {", name))
// If the entire value is marked, don't print any nested attributes // If the entire value is marked, don't print any nested attributes
if stateVal.IsMarked() { if stateVal.HasMark(marks.Sensitive) {
buf.WriteString("} # sensitive\n") buf.WriteString("} # sensitive\n")
return nil return nil
} }
@ -440,7 +442,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name
buf.WriteString("}\n") buf.WriteString("}\n")
return nil return nil
case configschema.NestingList, configschema.NestingSet: case configschema.NestingList, configschema.NestingSet:
if stateVal.IsMarked() { if stateVal.HasMark(marks.Sensitive) {
buf.WriteString(strings.Repeat(" ", indent)) buf.WriteString(strings.Repeat(" ", indent))
buf.WriteString(fmt.Sprintf("%s {} # sensitive\n", name)) buf.WriteString(fmt.Sprintf("%s {} # sensitive\n", name))
return nil return nil
@ -460,7 +462,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name
return nil return nil
case configschema.NestingMap: case configschema.NestingMap:
// If the entire value is marked, don't print any nested attributes // 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)) buf.WriteString(fmt.Sprintf("%s {} # sensitive\n", name))
return nil return nil
} }
@ -475,7 +477,7 @@ func (v *addHuman) writeConfigNestedBlockFromExisting(buf *strings.Builder, name
buf.WriteString(strings.Repeat(" ", indent)) buf.WriteString(strings.Repeat(" ", indent))
buf.WriteString(fmt.Sprintf("%s %q {", name, key)) buf.WriteString(fmt.Sprintf("%s %q {", name, key))
// This entire map element is marked // This entire map element is marked
if vals[key].IsMarked() { if vals[key].HasMark(marks.Sensitive) {
buf.WriteString("} # sensitive\n") buf.WriteString("} # sensitive\n")
return nil return nil
} }

View File

@ -343,7 +343,7 @@ func TestAdd_writeConfigBlocksFromExisting(t *testing.T) {
v := addHuman{optional: true} v := addHuman{optional: true}
val := cty.ObjectVal(map[string]cty.Value{ val := cty.ObjectVal(map[string]cty.Value{
"root_block_device": 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) schema := addTestSchema(configschema.NestingSingle)
@ -366,7 +366,7 @@ func TestAdd_writeConfigBlocksFromExisting(t *testing.T) {
"root_block_device": cty.ObjectVal(map[string]cty.Value{ "root_block_device": cty.ObjectVal(map[string]cty.Value{
"volume_type": cty.StringVal("foo"), "volume_type": cty.StringVal("foo"),
}), }),
}).Mark("bar") }).Mark(marks.Sensitive)
schema := addTestSchema(configschema.NestingSingle) schema := addTestSchema(configschema.NestingSingle)
var buf strings.Builder var buf strings.Builder
v.writeConfigBlocksFromExisting(&buf, val, schema.BlockTypes, 0) v.writeConfigBlocksFromExisting(&buf, val, schema.BlockTypes, 0)
@ -447,7 +447,7 @@ root_block_device {
cty.ObjectVal(map[string]cty.Value{ cty.ObjectVal(map[string]cty.Value{
"volume_type": cty.StringVal("bar"), "volume_type": cty.StringVal("bar"),
}), }),
}).Mark("mark"), }).Mark(marks.Sensitive),
}) })
schema := addTestSchema(configschema.NestingList) schema := addTestSchema(configschema.NestingList)
var buf strings.Builder var buf strings.Builder
@ -784,13 +784,13 @@ func TestAdd_WriteConfigNestedTypeAttributeFromExisting(t *testing.T) {
"disks": cty.ListVal([]cty.Value{ "disks": cty.ListVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{ cty.ObjectVal(map[string]cty.Value{
"mount_point": cty.StringVal("/mnt/foo"), "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. // This is an odd example, where the entire element is marked.
cty.ObjectVal(map[string]cty.Value{ cty.ObjectVal(map[string]cty.Value{
"mount_point": cty.StringVal("/mnt/bar"), "mount_point": cty.StringVal("/mnt/bar"),
"size": cty.StringVal("250GB"), "size": cty.StringVal("250GB"),
}).Mark("bye"), }).Mark(marks.Sensitive),
}), }),
}) })

View File

@ -10,6 +10,7 @@ import (
"github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/hcled" "github.com/hashicorp/hcl/v2/hcled"
"github.com/hashicorp/hcl/v2/hclparse" "github.com/hashicorp/hcl/v2/hclparse"
"github.com/hashicorp/terraform/internal/lang/marks"
"github.com/hashicorp/terraform/internal/tfdiags" "github.com/hashicorp/terraform/internal/tfdiags"
"github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty"
) )
@ -260,7 +261,7 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost
Traversal: traversalStr, Traversal: traversalStr,
} }
switch { switch {
case val.IsMarked(): case val.HasMark(marks.Sensitive):
// We won't say anything at all about sensitive values, // We won't say anything at all about sensitive values,
// because we might give away something that was // because we might give away something that was
// sensitive about them. // sensitive about them.
@ -323,7 +324,7 @@ func compactValueStr(val cty.Value) string {
// helpful but concise messages in diagnostics. It is not comprehensive // helpful but concise messages in diagnostics. It is not comprehensive
// nor intended to be used for other purposes. // 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 // We check this in here just to make sure, but note that the caller
// of compactValueStr ought to have already checked this and skipped // of compactValueStr ought to have already checked this and skipped
// calling into compactValueStr anyway, so this shouldn't actually // calling into compactValueStr anyway, so this shouldn't actually

View File

@ -6,6 +6,7 @@ import (
"strconv" "strconv"
"strings" "strings"
"github.com/hashicorp/terraform/internal/lang/marks"
"github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty"
"github.com/zclconf/go-cty/cty/convert" "github.com/zclconf/go-cty/cty/convert"
"github.com/zclconf/go-cty/cty/function" "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. // to be known here but may still be null.
ret, err := convert.Convert(args[0], retType) ret, err := convert.Convert(args[0], retType)
if err != nil { if err != nil {
val, _ := args[0].UnmarkDeep()
// Because we used GetConversionUnsafe above, conversion can // Because we used GetConversionUnsafe above, conversion can
// still potentially fail in here. For example, if the user // still potentially fail in here. For example, if the user
// asks to convert the string "a" to bool then we'll // asks to convert the string "a" to bool then we'll
// optimistically permit it during type checking but fail here // optimistically permit it during type checking but fail here
// once we note that the value isn't either "true" or "false". // once we note that the value isn't either "true" or "false".
gotTy := args[0].Type() gotTy := val.Type()
switch { switch {
case args[0].ContainsMarked(): case marks.Contains(args[0], marks.Sensitive):
// Generic message so we won't inadvertently disclose // Generic message so we won't inadvertently disclose
// information about sensitive values. // information about sensitive values.
return cty.NilVal, function.NewArgErrorf(0, "cannot convert this sensitive %s to %s", gotTy.FriendlyName(), wantTy.FriendlyNameForConstraint()) return cty.NilVal, function.NewArgErrorf(0, "cannot convert this sensitive %s to %s", gotTy.FriendlyName(), wantTy.FriendlyNameForConstraint())
case gotTy == cty.String && wantTy == cty.Bool: case gotTy == cty.String && wantTy == cty.Bool:
what := "string" what := "string"
if !args[0].IsNull() { if !val.IsNull() {
what = strconv.Quote(args[0].AsString()) 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) 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: case gotTy == cty.String && wantTy == cty.Number:
what := "string" what := "string"
if !args[0].IsNull() { if !val.IsNull() {
what = strconv.Quote(args[0].AsString()) 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) return cty.NilVal, function.NewArgErrorf(0, `cannot convert %s to number; given string must be a decimal representation of a number`, what)
default: default:

View File

@ -5,6 +5,7 @@ import (
"testing" "testing"
"github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp"
"github.com/hashicorp/terraform/internal/lang/marks"
"github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty"
) )
@ -61,6 +62,12 @@ func TestTo(t *testing.T) {
cty.StringVal("a").Mark("boop"), cty.StringVal("a").Mark("boop"),
cty.Bool, cty.Bool,
cty.DynamicVal, 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`, `cannot convert this sensitive string to bool`,
}, },
{ {
@ -73,6 +80,12 @@ func TestTo(t *testing.T) {
cty.StringVal("a").Mark("boop"), cty.StringVal("a").Mark("boop"),
cty.Number, cty.Number,
cty.DynamicVal, 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`, `cannot convert this sensitive string to number`,
}, },
{ {

View File

@ -5,6 +5,7 @@ import (
"github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2"
"github.com/hashicorp/terraform/internal/lang" "github.com/hashicorp/terraform/internal/lang"
"github.com/hashicorp/terraform/internal/lang/marks"
"github.com/hashicorp/terraform/internal/tfdiags" "github.com/hashicorp/terraform/internal/tfdiags"
"github.com/zclconf/go-cty/cty" "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) // 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 // 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{ diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError, Severity: hcl.DiagError,
Summary: "Invalid for_each argument", Summary: "Invalid for_each argument",

View File

@ -283,9 +283,7 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) (diags
// a sensitive result, to help avoid accidental exposure in the state // a sensitive result, to help avoid accidental exposure in the state
// of a sensitive value that the user doesn't want to include there. // of a sensitive value that the user doesn't want to include there.
if n.Addr.Module.IsRoot() { if n.Addr.Module.IsRoot() {
_, m := val.UnmarkDeep() if !n.Config.Sensitive && marks.Contains(val, marks.Sensitive) {
_, hasSensitive := m[marks.Sensitive]
if !n.Config.Sensitive && hasSensitive {
diags = diags.Append(&hcl.Diagnostic{ diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError, Severity: hcl.DiagError,
Summary: "Output refers to sensitive values", Summary: "Output refers to sensitive values",