From 862ddf73e2474e08bbf30b1332f3381ab82463a0 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 23 Jul 2020 15:56:19 -0400 Subject: [PATCH 01/20] Add a sensitive attribute --- configs/named_values.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/configs/named_values.go b/configs/named_values.go index e0b708a1e..6f867d6c9 100644 --- a/configs/named_values.go +++ b/configs/named_values.go @@ -25,6 +25,7 @@ type Variable struct { Type cty.Type ParsingMode VariableParsingMode Validations []*VariableValidation + Sensitive bool DescriptionSet bool @@ -94,6 +95,11 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno v.ParsingMode = parseMode } + if attr, exists := content.Attributes["sensitive"]; exists { + valDiags := gohcl.DecodeExpression(attr.Expr, nil, &v.Sensitive) + diags = append(diags, valDiags...) + } + if attr, exists := content.Attributes["default"]; exists { val, valDiags := attr.Expr.Value(nil) diags = append(diags, valDiags...) @@ -534,6 +540,9 @@ var variableBlockSchema = &hcl.BodySchema{ { Name: "type", }, + { + Name: "sensitive", + }, }, Blocks: []hcl.BlockHeaderSchema{ { From 84d118e18f709ba4caa3f69ef0c49b065ae9621b Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 7 Aug 2020 11:59:06 -0400 Subject: [PATCH 02/20] Track sensitivity through evaluation Mark sensitivity on a value. However, when the value is encoded to send to the provider to produce a changeset we must remove the marks, so unmark the value and remark it with the saved path afterwards --- command/format/diff.go | 28 +++++++++++++---- plans/changes.go | 9 +++--- plans/changes_src.go | 4 +++ plans/dynamic_value.go | 20 +++++++++++++ terraform/eval_diff.go | 28 +++++++++++++++++ terraform/evaluate.go | 4 +++ .../zclconf/go-cty/cty/json/marshal.go | 3 +- .../zclconf/go-cty/cty/msgpack/marshal.go | 30 +++++++++++++++++++ 8 files changed, 116 insertions(+), 10 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 901c9a081..7fa94fada 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -125,10 +125,21 @@ func ResourceChange( changeV.Change.Before = objchange.NormalizeObjectFromLegacySDK(changeV.Change.Before, schema) changeV.Change.After = objchange.NormalizeObjectFromLegacySDK(changeV.Change.After, schema) - result := p.writeBlockBodyDiff(schema, changeV.Before, changeV.After, 6, path) - if result.bodyWritten { - p.buf.WriteString("\n") - p.buf.WriteString(strings.Repeat(" ", 4)) + // Now that the change is decoded, add back the marks at the defined paths + // change.Markinfo + changeV.Change.After, _ = cty.Transform(changeV.Change.After, func(p cty.Path, v cty.Value) (cty.Value, error) { + if p.Equals(change.ValMarks.Path) { + // TODO The mark is at change.Markinfo.Marks and it would be proper + // to iterate through that set here + return v.Mark("sensitive"), nil + } + return v, nil + }) + + bodyWritten := p.writeBlockBodyDiff(schema, changeV.Before, changeV.After, 6, path) + if bodyWritten { + buf.WriteString("\n") + buf.WriteString(strings.Repeat(" ", 4)) } buf.WriteString("}\n") @@ -586,6 +597,12 @@ 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() { + p.buf.WriteString("(sensitive)") + return + } + if !val.IsKnown() { p.buf.WriteString("(known after apply)") return @@ -1284,7 +1301,8 @@ func ctyGetAttrMaybeNull(val cty.Value, name string) cty.Value { // This allows us to avoid spurious diffs // until we introduce null to the SDK. attrValue := val.GetAttr(name) - if ctyEmptyString(attrValue) { + // If the value is marked, the ctyEmptyString function will fail + if !val.ContainsMarked() && ctyEmptyString(attrValue) { return cty.NullVal(attrType) } diff --git a/plans/changes.go b/plans/changes.go index 9e8f25bae..a8ea099fe 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -341,14 +341,15 @@ func (c *Change) Encode(ty cty.Type) (*ChangeSrc, error) { if err != nil { return nil, err } - afterDV, err := NewDynamicValue(c.After, ty) + afterDV, err, marks := NewDynamicValueMarks(c.After, ty) if err != nil { return nil, err } return &ChangeSrc{ - Action: c.Action, - Before: beforeDV, - After: afterDV, + Action: c.Action, + Before: beforeDV, + After: afterDV, + ValMarks: marks, }, nil } diff --git a/plans/changes_src.go b/plans/changes_src.go index 553a84082..2b7cd9da3 100644 --- a/plans/changes_src.go +++ b/plans/changes_src.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/states" "github.com/zclconf/go-cty/cty" + ctymsgpack "github.com/zclconf/go-cty/cty/msgpack" ) // ResourceInstanceChangeSrc is a not-yet-decoded ResourceInstanceChange. @@ -156,6 +157,9 @@ type ChangeSrc struct { // but have not yet been decoded from the serialized value used for // storage. Before, After DynamicValue + + // Marked Paths + ValMarks *ctymsgpack.MarkInfo } // Decode unmarshals the raw representations of the before and after values diff --git a/plans/dynamic_value.go b/plans/dynamic_value.go index 51fbb24cf..c9080deb0 100644 --- a/plans/dynamic_value.go +++ b/plans/dynamic_value.go @@ -55,6 +55,26 @@ func NewDynamicValue(val cty.Value, ty cty.Type) (DynamicValue, error) { return DynamicValue(buf), nil } +// NewDynamicValueMarks returns a new dynamic value along with a +// associated marking info for the value +func NewDynamicValueMarks(val cty.Value, ty cty.Type) (DynamicValue, error, *ctymsgpack.MarkInfo) { + // If we're given cty.NilVal (the zero value of cty.Value, which is + // distinct from a typed null value created by cty.NullVal) then we'll + // assume the caller is trying to represent the _absense_ of a value, + // and so we'll return a nil DynamicValue. + if val == cty.NilVal { + return DynamicValue(nil), nil, nil + } + + // Currently our internal encoding is msgpack, via ctymsgpack. + buf, err, marks := ctymsgpack.MarshalWithMarks(val, ty) + if err != nil { + return nil, err, marks + } + + return DynamicValue(buf), nil, marks +} + // Decode retrieves the effective value from the receiever by interpreting the // serialized form against the given type constraint. For correct results, // the type constraint must match (or be consistent with) the one that was diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 509bd5bbc..73e37539f 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -141,6 +141,25 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.Err() } + var markedPath cty.Path + // var marks cty.ValueMarks + if configVal.ContainsMarked() { + // store the marked values so we can re-mark them later after + // we've sent things over the wire. Right now this stores + // one path for proof of concept, but we should store multiple + cty.Walk(configVal, func(p cty.Path, v cty.Value) (bool, error) { + if v.IsMarked() { + markedPath = p + return false, nil + // marks = v.Marks() + } + return true, nil + }) + // Unmark the value for sending over the wire + // to providers as marks cannot be serialized + configVal, _ = configVal.UnmarkDeep() + } + metaConfigVal := cty.NullVal(cty.DynamicPseudoType) if n.ProviderMetas != nil { if m, ok := n.ProviderMetas[n.ProviderAddr.Provider]; ok && m != nil { @@ -235,6 +254,15 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } plannedNewVal := resp.PlannedState + + // Add the mark back to the planned new value + plannedNewVal, _ = cty.Transform(plannedNewVal, func(p cty.Path, v cty.Value) (cty.Value, error) { + if p.Equals(markedPath) { + return v.Mark("sensitive"), nil + } + return v, nil + }) + plannedPrivate := resp.PlannedPrivate if plannedNewVal == cty.NilVal { diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 3e77ca9b8..3c11b66ca 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -292,6 +292,10 @@ func (d *evaluationStateData) GetInputVariable(addr addrs.InputVariable, rng tfd val = cty.UnknownVal(wantType) } + if config.Sensitive { + val = val.Mark("sensitive") + } + return val, diags } diff --git a/vendor/github.com/zclconf/go-cty/cty/json/marshal.go b/vendor/github.com/zclconf/go-cty/cty/json/marshal.go index 75e02577b..040b20c27 100644 --- a/vendor/github.com/zclconf/go-cty/cty/json/marshal.go +++ b/vendor/github.com/zclconf/go-cty/cty/json/marshal.go @@ -10,7 +10,8 @@ import ( func marshal(val cty.Value, t cty.Type, path cty.Path, b *bytes.Buffer) error { if val.IsMarked() { - return path.NewErrorf("value has marks, so it cannot be seralized") + // For now, dump the marks when serializing JSON for POC purposes + val, _ = val.UnmarkDeep() } // If we're going to decode as DynamicPseudoType then we need to save diff --git a/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go b/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go index cc351f0f1..950d1c590 100644 --- a/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go +++ b/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go @@ -41,6 +41,36 @@ func Marshal(val cty.Value, ty cty.Type) ([]byte, error) { return buf.Bytes(), nil } +// This type should (likely) be a map of paths +// and marks, so that multiple marks can be found +// in case of a value containing multiple marked values +type MarkInfo struct { + Marks cty.ValueMarks + Path cty.Path +} + +func MarshalWithMarks(val cty.Value, ty cty.Type) ([]byte, error, *MarkInfo) { + markInfo := MarkInfo{} + if val.ContainsMarked() { + // store the marked values so we can re-mark them later after + // we've sent things over the wire + cty.Walk(val, func(p cty.Path, v cty.Value) (bool, error) { + if v.IsMarked() { + markInfo.Path = p + markInfo.Marks = v.Marks() + } + return true, nil + }) + val, _ = val.UnmarkDeep() + } + by, err := Marshal(val, ty) + if err != nil { + return nil, err, &markInfo + } + + return by, nil, &markInfo +} + func marshal(val cty.Value, ty cty.Type, path cty.Path, enc *msgpack.Encoder) error { if val.IsMarked() { return path.NewErrorf("value has marks, so it cannot be seralized") From 896d277a69c2f7f9b52396c65bb5fbf1483ab4da Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Mon, 31 Aug 2020 17:08:10 -0400 Subject: [PATCH 03/20] If the path is empty, we should not be marking the path --- command/format/diff.go | 18 ++++++++++-------- terraform/eval_diff.go | 14 ++++++++------ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 7fa94fada..168d17022 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -127,14 +127,16 @@ func ResourceChange( // Now that the change is decoded, add back the marks at the defined paths // change.Markinfo - changeV.Change.After, _ = cty.Transform(changeV.Change.After, func(p cty.Path, v cty.Value) (cty.Value, error) { - if p.Equals(change.ValMarks.Path) { - // TODO The mark is at change.Markinfo.Marks and it would be proper - // to iterate through that set here - return v.Mark("sensitive"), nil - } - return v, nil - }) + if len(change.ValMarks.Path) != 0 { + changeV.Change.After, _ = cty.Transform(changeV.Change.After, func(p cty.Path, v cty.Value) (cty.Value, error) { + if p.Equals(change.ValMarks.Path) { + // TODO The mark is at change.Markinfo.Marks and it would be proper + // to iterate through that set here + return v.Mark("sensitive"), nil + } + return v, nil + }) + } bodyWritten := p.writeBlockBodyDiff(schema, changeV.Before, changeV.After, 6, path) if bodyWritten { diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 73e37539f..6a8cfb768 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -256,12 +256,14 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { plannedNewVal := resp.PlannedState // Add the mark back to the planned new value - plannedNewVal, _ = cty.Transform(plannedNewVal, func(p cty.Path, v cty.Value) (cty.Value, error) { - if p.Equals(markedPath) { - return v.Mark("sensitive"), nil - } - return v, nil - }) + if len(markedPath) != 0 { + plannedNewVal, _ = cty.Transform(plannedNewVal, func(p cty.Path, v cty.Value) (cty.Value, error) { + if p.Equals(markedPath) { + return v.Mark("sensitive"), nil + } + return v, nil + }) + } plannedPrivate := resp.PlannedPrivate From 6c129a921b082e9945f747f7b5e9508926ed4ecc Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Mon, 31 Aug 2020 17:33:01 -0400 Subject: [PATCH 04/20] Unmark/remark in apply process to allow apply --- plans/objchange/compatible.go | 6 +++++- terraform/eval_apply.go | 36 ++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 4f944775a..80987bbcf 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -51,7 +51,11 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu actualV := actual.GetAttr(name) path := append(path, cty.GetAttrStep{Name: name}) - moreErrs := assertValueCompatible(plannedV, actualV, path) + + // HACK Unmark the values here so we can get past this testing in Apply + unmarked, _ := plannedV.UnmarkDeep() + unmarkedActual, _ := actualV.UnmarkDeep() + moreErrs := assertValueCompatible(unmarked, unmarkedActual, path) if attrS.Sensitive { if len(moreErrs) > 0 { // Use a vague placeholder message instead, to avoid disclosing diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 00af310bb..9a8f42200 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -78,6 +78,26 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { ) } + // Copy paste from eval_diff + var markedPath cty.Path + // var marks cty.ValueMarks + if configVal.ContainsMarked() { + // store the marked values so we can re-mark them later after + // we've sent things over the wire. Right now this stores + // one path for proof of concept, but we should store multiple + cty.Walk(configVal, func(p cty.Path, v cty.Value) (bool, error) { + if v.IsMarked() { + markedPath = p + return false, nil + // marks = v.Marks() + } + return true, nil + }) + // Unmark the value for sending over the wire + // to providers as marks cannot be serialized + configVal, _ = configVal.UnmarkDeep() + } + metaConfigVal := cty.NullVal(cty.DynamicPseudoType) if n.ProviderMetas != nil { log.Printf("[DEBUG] EvalApply: ProviderMeta config value set") @@ -104,11 +124,15 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { } log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr.Absolute(ctx.Path()), change.Action) + + // HACK The after val is also marked so let's fix that + unmarked, _ := change.After.UnmarkDeep() + resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ TypeName: n.Addr.Resource.Type, PriorState: change.Before, Config: configVal, - PlannedState: change.After, + PlannedState: unmarked, PlannedPrivate: change.Private, ProviderMeta: metaConfigVal, }) @@ -125,6 +149,16 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { // incomplete. newVal := resp.NewState + // Add the mark back to the planned new value + if len(markedPath) != 0 { + newVal, _ = cty.Transform(newVal, func(p cty.Path, v cty.Value) (cty.Value, error) { + if p.Equals(markedPath) { + return v.Mark("sensitive"), nil + } + return v, nil + }) + } + if newVal == cty.NilVal { // Providers are supposed to return a partial new value even when errors // occur, but sometimes they don't and so in that case we'll patch that up From 7fef1db20df412a5fe648f8a9b4f9cf0f345176e Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 2 Sep 2020 15:18:26 -0400 Subject: [PATCH 05/20] Add sensitive variable configs test coverage --- configs/testdata/valid-files/variables.tf | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/configs/testdata/valid-files/variables.tf b/configs/testdata/valid-files/variables.tf index 817649307..668761bc9 100644 --- a/configs/testdata/valid-files/variables.tf +++ b/configs/testdata/valid-files/variables.tf @@ -22,3 +22,7 @@ variable "cheeze_pizza" { variable "π" { default = 3.14159265359 } + +variable "sensitive-value" { + sensitive = true +} From bc55b6a28bc681fa59b984d366bba963c5d76836 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 3 Sep 2020 15:42:58 -0400 Subject: [PATCH 06/20] Use UnmarkDeepWithPaths and MarkWithPaths Updates existing code to use the new Value methods for unmarking/marking and removes panics/workarounds in cty marshall methods --- command/format/diff.go | 15 +++---- plans/changes.go | 26 ++++++++--- plans/changes_src.go | 3 +- plans/dynamic_value.go | 20 --------- states/instance_object.go | 12 ++++- terraform/eval_diff.go | 35 +++++---------- .../zclconf/go-cty/cty/json/marshal.go | 3 +- vendor/github.com/zclconf/go-cty/cty/marks.go | 44 +++++++++++++++++++ .../zclconf/go-cty/cty/msgpack/marshal.go | 32 +------------- 9 files changed, 95 insertions(+), 95 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 168d17022..a68431584 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -126,16 +126,11 @@ func ResourceChange( changeV.Change.After = objchange.NormalizeObjectFromLegacySDK(changeV.Change.After, schema) // Now that the change is decoded, add back the marks at the defined paths - // change.Markinfo - if len(change.ValMarks.Path) != 0 { - changeV.Change.After, _ = cty.Transform(changeV.Change.After, func(p cty.Path, v cty.Value) (cty.Value, error) { - if p.Equals(change.ValMarks.Path) { - // TODO The mark is at change.Markinfo.Marks and it would be proper - // to iterate through that set here - return v.Mark("sensitive"), nil - } - return v, nil - }) + if len(change.BeforeValMarks) > 0 { + changeV.Change.Before = changeV.Change.Before.MarkWithPaths(change.BeforeValMarks) + } + if len(change.AfterValMarks) > 0 { + changeV.Change.After = changeV.Change.After.MarkWithPaths(change.AfterValMarks) } bodyWritten := p.writeBlockBodyDiff(schema, changeV.Before, changeV.After, 6, path) diff --git a/plans/changes.go b/plans/changes.go index a8ea099fe..239ceb714 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -337,19 +337,33 @@ type Change struct { // to call the corresponding Encode method of that struct rather than working // directly with its embedded Change. func (c *Change) Encode(ty cty.Type) (*ChangeSrc, error) { - beforeDV, err := NewDynamicValue(c.Before, ty) + // Storing unmarked values so that we can encode unmarked values + // and save the PathValueMarks for re-marking the values later + var beforeVM, afterVM []cty.PathValueMarks + unmarkedBefore := c.Before + unmarkedAfter := c.After + + if c.Before.ContainsMarked() { + unmarkedBefore, beforeVM = c.Before.UnmarkDeepWithPaths() + } + beforeDV, err := NewDynamicValue(unmarkedBefore, ty) if err != nil { return nil, err } - afterDV, err, marks := NewDynamicValueMarks(c.After, ty) + + if c.After.ContainsMarked() { + unmarkedAfter, afterVM = c.After.UnmarkDeepWithPaths() + } + afterDV, err := NewDynamicValue(unmarkedAfter, ty) if err != nil { return nil, err } return &ChangeSrc{ - Action: c.Action, - Before: beforeDV, - After: afterDV, - ValMarks: marks, + Action: c.Action, + Before: beforeDV, + After: afterDV, + BeforeValMarks: beforeVM, + AfterValMarks: afterVM, }, nil } diff --git a/plans/changes_src.go b/plans/changes_src.go index 2b7cd9da3..a5093bb56 100644 --- a/plans/changes_src.go +++ b/plans/changes_src.go @@ -6,7 +6,6 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/states" "github.com/zclconf/go-cty/cty" - ctymsgpack "github.com/zclconf/go-cty/cty/msgpack" ) // ResourceInstanceChangeSrc is a not-yet-decoded ResourceInstanceChange. @@ -159,7 +158,7 @@ type ChangeSrc struct { Before, After DynamicValue // Marked Paths - ValMarks *ctymsgpack.MarkInfo + BeforeValMarks, AfterValMarks []cty.PathValueMarks } // Decode unmarshals the raw representations of the before and after values diff --git a/plans/dynamic_value.go b/plans/dynamic_value.go index c9080deb0..51fbb24cf 100644 --- a/plans/dynamic_value.go +++ b/plans/dynamic_value.go @@ -55,26 +55,6 @@ func NewDynamicValue(val cty.Value, ty cty.Type) (DynamicValue, error) { return DynamicValue(buf), nil } -// NewDynamicValueMarks returns a new dynamic value along with a -// associated marking info for the value -func NewDynamicValueMarks(val cty.Value, ty cty.Type) (DynamicValue, error, *ctymsgpack.MarkInfo) { - // If we're given cty.NilVal (the zero value of cty.Value, which is - // distinct from a typed null value created by cty.NullVal) then we'll - // assume the caller is trying to represent the _absense_ of a value, - // and so we'll return a nil DynamicValue. - if val == cty.NilVal { - return DynamicValue(nil), nil, nil - } - - // Currently our internal encoding is msgpack, via ctymsgpack. - buf, err, marks := ctymsgpack.MarshalWithMarks(val, ty) - if err != nil { - return nil, err, marks - } - - return DynamicValue(buf), nil, marks -} - // Decode retrieves the effective value from the receiever by interpreting the // serialized form against the given type constraint. For correct results, // the type constraint must match (or be consistent with) the one that was diff --git a/states/instance_object.go b/states/instance_object.go index d1b53e292..a4dd671f5 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -19,6 +19,9 @@ type ResourceInstanceObject struct { // Terraform. Value cty.Value + // PathValueMarks is a slice of paths and value marks of the value + PathValueMarks []cty.PathValueMarks + // Private is an opaque value set by the provider when this object was // last created or updated. Terraform Core does not use this value in // any way and it is not exposed anywhere in the user interface, so @@ -98,7 +101,14 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res // and raise an error about that. val := cty.UnknownAsNull(o.Value) - src, err := ctyjson.Marshal(val, ty) + // If it contains marks, dump those now + unmarked := val + if val.ContainsMarked() { + var pvm []cty.PathValueMarks + unmarked, pvm = val.UnmarkDeepWithPaths() + o.PathValueMarks = pvm + } + src, err := ctyjson.Marshal(unmarked, ty) if err != nil { return nil, err } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 6a8cfb768..1408dd01c 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -141,23 +141,17 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.Err() } - var markedPath cty.Path + // Create an unmarked version of our config val, defaulting + // to the configVal so we don't do the work of unmarking unless + // necessary + unmarkedConfigVal := configVal + var unmarkedPaths []cty.PathValueMarks // var marks cty.ValueMarks if configVal.ContainsMarked() { // store the marked values so we can re-mark them later after // we've sent things over the wire. Right now this stores // one path for proof of concept, but we should store multiple - cty.Walk(configVal, func(p cty.Path, v cty.Value) (bool, error) { - if v.IsMarked() { - markedPath = p - return false, nil - // marks = v.Marks() - } - return true, nil - }) - // Unmark the value for sending over the wire - // to providers as marks cannot be serialized - configVal, _ = configVal.UnmarkDeep() + unmarkedConfigVal, unmarkedPaths = configVal.UnmarkDeepWithPaths() } metaConfigVal := cty.NullVal(cty.DynamicPseudoType) @@ -203,7 +197,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { priorVal = cty.NullVal(schema.ImpliedType()) } - proposedNewVal := objchange.ProposedNewObject(schema, priorVal, configVal) + proposedNewVal := objchange.ProposedNewObject(schema, priorVal, unmarkedConfigVal) // Call pre-diff hook if !n.Stub { @@ -222,7 +216,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { validateResp := provider.ValidateResourceTypeConfig( providers.ValidateResourceTypeConfigRequest{ TypeName: n.Addr.Resource.Type, - Config: configVal, + Config: unmarkedConfigVal, }, ) if validateResp.Diagnostics.HasErrors() { @@ -242,7 +236,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { resp := provider.PlanResourceChange(providers.PlanResourceChangeRequest{ TypeName: n.Addr.Resource.Type, - Config: configVal, + Config: unmarkedConfigVal, PriorState: priorVal, ProposedNewState: proposedNewVal, PriorPrivate: priorPrivate, @@ -255,14 +249,9 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { plannedNewVal := resp.PlannedState - // Add the mark back to the planned new value - if len(markedPath) != 0 { - plannedNewVal, _ = cty.Transform(plannedNewVal, func(p cty.Path, v cty.Value) (cty.Value, error) { - if p.Equals(markedPath) { - return v.Mark("sensitive"), nil - } - return v, nil - }) + // Add the marks back to the planned new value + if configVal.ContainsMarked() { + plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) } plannedPrivate := resp.PlannedPrivate diff --git a/vendor/github.com/zclconf/go-cty/cty/json/marshal.go b/vendor/github.com/zclconf/go-cty/cty/json/marshal.go index 040b20c27..7a14ce81a 100644 --- a/vendor/github.com/zclconf/go-cty/cty/json/marshal.go +++ b/vendor/github.com/zclconf/go-cty/cty/json/marshal.go @@ -10,8 +10,7 @@ import ( func marshal(val cty.Value, t cty.Type, path cty.Path, b *bytes.Buffer) error { if val.IsMarked() { - // For now, dump the marks when serializing JSON for POC purposes - val, _ = val.UnmarkDeep() + return path.NewErrorf("value has marks, so it cannot be serialized as JSON") } // If we're going to decode as DynamicPseudoType then we need to save diff --git a/vendor/github.com/zclconf/go-cty/cty/marks.go b/vendor/github.com/zclconf/go-cty/cty/marks.go index 3898e4553..0f6ff6aa2 100644 --- a/vendor/github.com/zclconf/go-cty/cty/marks.go +++ b/vendor/github.com/zclconf/go-cty/cty/marks.go @@ -67,6 +67,23 @@ func (m ValueMarks) GoString() string { return s.String() } +// PathValueMarks is a structure that enables tracking marks +// and the paths where they are located in one type +type PathValueMarks struct { + Path Path + Marks ValueMarks +} + +func (p PathValueMarks) Equal(o PathValueMarks) bool { + if !p.Path.Equals(o.Path) { + return false + } + if !p.Marks.Equal(o.Marks) { + return false + } + return true +} + // IsMarked returns true if and only if the receiving value carries at least // one mark. A marked value cannot be used directly with integration methods // without explicitly unmarking it (and retrieving the markings) first. @@ -174,6 +191,21 @@ func (val Value) Mark(mark interface{}) Value { } } +// MarkWithPaths accepts a slice of PathValueMarks to apply +// marker particular paths +func (val Value) MarkWithPaths(pvm []PathValueMarks) Value { + ret, _ := Transform(val, func(p Path, v Value) (Value, error) { + for _, path := range pvm { + if p.Equals(path.Path) { + return v.WithMarks(path.Marks), nil + + } + } + return v, nil + }) + return ret +} + // Unmark separates the marks of the receiving value from the value itself, // removing a new unmarked value and a map (representing a set) of the marks. // @@ -209,6 +241,18 @@ func (val Value) UnmarkDeep() (Value, ValueMarks) { return ret, marks } +func (val Value) UnmarkDeepWithPaths() (Value, []PathValueMarks) { + var marks []PathValueMarks + ret, _ := Transform(val, func(p Path, v Value) (Value, error) { + unmarkedV, valueMarks := v.Unmark() + if v.IsMarked() { + marks = append(marks, PathValueMarks{p, valueMarks}) + } + return unmarkedV, nil + }) + return ret, marks +} + func (val Value) unmarkForce() Value { unw, _ := val.Unmark() return unw diff --git a/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go b/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go index 950d1c590..2c4da8b50 100644 --- a/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go +++ b/vendor/github.com/zclconf/go-cty/cty/msgpack/marshal.go @@ -41,39 +41,9 @@ func Marshal(val cty.Value, ty cty.Type) ([]byte, error) { return buf.Bytes(), nil } -// This type should (likely) be a map of paths -// and marks, so that multiple marks can be found -// in case of a value containing multiple marked values -type MarkInfo struct { - Marks cty.ValueMarks - Path cty.Path -} - -func MarshalWithMarks(val cty.Value, ty cty.Type) ([]byte, error, *MarkInfo) { - markInfo := MarkInfo{} - if val.ContainsMarked() { - // store the marked values so we can re-mark them later after - // we've sent things over the wire - cty.Walk(val, func(p cty.Path, v cty.Value) (bool, error) { - if v.IsMarked() { - markInfo.Path = p - markInfo.Marks = v.Marks() - } - return true, nil - }) - val, _ = val.UnmarkDeep() - } - by, err := Marshal(val, ty) - if err != nil { - return nil, err, &markInfo - } - - return by, nil, &markInfo -} - func marshal(val cty.Value, ty cty.Type, path cty.Path, enc *msgpack.Encoder) error { if val.IsMarked() { - return path.NewErrorf("value has marks, so it cannot be seralized") + return path.NewErrorf("value has marks, so it cannot be serialized") } // If we're going to decode as DynamicPseudoType then we need to save From e9d9205ce8461f7611cba369a5e8d4fea8c53034 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 4 Sep 2020 12:32:52 -0400 Subject: [PATCH 07/20] Modifications to eval_diff --- terraform/eval_diff.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 1408dd01c..78d152255 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -146,7 +146,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // necessary unmarkedConfigVal := configVal var unmarkedPaths []cty.PathValueMarks - // var marks cty.ValueMarks if configVal.ContainsMarked() { // store the marked values so we can re-mark them later after // we've sent things over the wire. Right now this stores @@ -250,8 +249,9 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { plannedNewVal := resp.PlannedState // Add the marks back to the planned new value + markedPlannedNewVal := plannedNewVal if configVal.ContainsMarked() { - plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) + markedPlannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) } plannedPrivate := resp.PlannedPrivate @@ -499,7 +499,10 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { Change: plans.Change{ Action: action, Before: priorVal, - After: plannedNewVal, + // Pass the marked value through in our change + // to propogate through evaluation. + // Marks will be removed when encoding. + After: markedPlannedNewVal, }, RequiredReplace: reqRep, } From 5b0b1a13a5464da4d53db80c6ddcecd624dbff96 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 4 Sep 2020 12:59:54 -0400 Subject: [PATCH 08/20] Update object compatible check to unmark The hack approach appears consistent, as we can remove marks before calling the value validation --- plans/objchange/compatible.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 80987bbcf..181a044f6 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -51,11 +51,17 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu actualV := actual.GetAttr(name) path := append(path, cty.GetAttrStep{Name: name}) - - // HACK Unmark the values here so we can get past this testing in Apply - unmarked, _ := plannedV.UnmarkDeep() - unmarkedActual, _ := actualV.UnmarkDeep() - moreErrs := assertValueCompatible(unmarked, unmarkedActual, path) + // 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() + } + moreErrs := assertValueCompatible(unmarkedPlannedV, unmarkedActualV, path) if attrS.Sensitive { if len(moreErrs) > 0 { // Use a vague placeholder message instead, to avoid disclosing From 3e8b125e53f9be960b28ca13d7346fe9b3958a30 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 4 Sep 2020 13:00:55 -0400 Subject: [PATCH 09/20] Apply does not need remarking Apply, at this moment, appears that it does not require the remarking strategy, as the plan has already been printed --- terraform/eval_apply.go | 47 ++++++++++++----------------------------- 1 file changed, 13 insertions(+), 34 deletions(-) diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 9a8f42200..1a2eef0bd 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -78,26 +78,6 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { ) } - // Copy paste from eval_diff - var markedPath cty.Path - // var marks cty.ValueMarks - if configVal.ContainsMarked() { - // store the marked values so we can re-mark them later after - // we've sent things over the wire. Right now this stores - // one path for proof of concept, but we should store multiple - cty.Walk(configVal, func(p cty.Path, v cty.Value) (bool, error) { - if v.IsMarked() { - markedPath = p - return false, nil - // marks = v.Marks() - } - return true, nil - }) - // Unmark the value for sending over the wire - // to providers as marks cannot be serialized - configVal, _ = configVal.UnmarkDeep() - } - metaConfigVal := cty.NullVal(cty.DynamicPseudoType) if n.ProviderMetas != nil { log.Printf("[DEBUG] EvalApply: ProviderMeta config value set") @@ -125,14 +105,23 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr.Absolute(ctx.Path()), change.Action) - // HACK The after val is also marked so let's fix that - unmarked, _ := change.After.UnmarkDeep() + // If our config or After value contain any marked values, + // ensure those are stripped out before sending + // this to the provider + unmarkedConfigVal := configVal + if configVal.ContainsMarked() { + unmarkedConfigVal, _ = configVal.UnmarkDeep() + } + unmarkedAfter := change.After + if change.After.ContainsMarked() { + unmarkedAfter, _ = change.After.UnmarkDeep() + } resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ TypeName: n.Addr.Resource.Type, PriorState: change.Before, - Config: configVal, - PlannedState: unmarked, + Config: unmarkedConfigVal, + PlannedState: unmarkedAfter, PlannedPrivate: change.Private, ProviderMeta: metaConfigVal, }) @@ -149,16 +138,6 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { // incomplete. newVal := resp.NewState - // Add the mark back to the planned new value - if len(markedPath) != 0 { - newVal, _ = cty.Transform(newVal, func(p cty.Path, v cty.Value) (cty.Value, error) { - if p.Equals(markedPath) { - return v.Mark("sensitive"), nil - } - return v, nil - }) - } - if newVal == cty.NilVal { // Providers are supposed to return a partial new value even when errors // occur, but sometimes they don't and so in that case we'll patch that up From 61c78fd3b9247362fae7d40f4acdeed0bb6a548f Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 4 Sep 2020 13:18:30 -0400 Subject: [PATCH 10/20] Add case to compactValueStr not to expose sensitive vals in diagnostics --- command/format/diagnostic.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/command/format/diagnostic.go b/command/format/diagnostic.go index 6ceb30894..38626e30b 100644 --- a/command/format/diagnostic.go +++ b/command/format/diagnostic.go @@ -302,6 +302,10 @@ 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.ContainsMarked() { + return "(sensitive value)" + } + ty := val.Type() switch { case val.IsNull(): From b03d5df9dc88fbe9136e111c3e9ef8f391143993 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 4 Sep 2020 13:28:21 -0400 Subject: [PATCH 11/20] Disallow sensitive values as for_each arguments --- terraform/eval_for_each.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go index f2518bcbc..75e6eabf1 100644 --- a/terraform/eval_for_each.go +++ b/terraform/eval_for_each.go @@ -48,6 +48,14 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V forEachVal, forEachDiags := ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil) diags = diags.Append(forEachDiags) + if forEachVal.ContainsMarked() { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid for_each argument", + Detail: "Sensitive variable, 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.", + Subject: expr.Range().Ptr(), + }) + } if diags.HasErrors() { return nullMap, diags } From 712f5a5cc36b1600e9f42c776b1d8db4b3370f78 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 8 Sep 2020 12:49:39 -0400 Subject: [PATCH 12/20] Update plannedNewVal itself Using markedPlannedNewVal caused many test failures with ignoreChanges, and I noted plannedNewVal itself is modified in the eval_diff. plannedNewVal is now marked closer to the change where it needs it. There is also a test fixture update to remove interpolation warnings. --- terraform/eval_diff.go | 16 +++++++--------- terraform/testdata/plan-ignore-changes/main.tf | 4 ++-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 78d152255..ac3f3ca6b 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -247,13 +247,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } plannedNewVal := resp.PlannedState - - // Add the marks back to the planned new value - markedPlannedNewVal := plannedNewVal - if configVal.ContainsMarked() { - markedPlannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) - } - plannedPrivate := resp.PlannedPrivate if plannedNewVal == cty.NilVal { @@ -480,6 +473,11 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } } + // Add the marks back to the planned new value + if configVal.ContainsMarked() { + plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) + } + // Call post-refresh hook if !n.Stub { err := ctx.Hook(func(h Hook) (HookAction, error) { @@ -499,10 +497,10 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { Change: plans.Change{ Action: action, Before: priorVal, - // Pass the marked value through in our change + // Pass the marked planned value through in our change // to propogate through evaluation. // Marks will be removed when encoding. - After: markedPlannedNewVal, + After: plannedNewVal, }, RequiredReplace: reqRep, } diff --git a/terraform/testdata/plan-ignore-changes/main.tf b/terraform/testdata/plan-ignore-changes/main.tf index 056256a1d..ed17c6344 100644 --- a/terraform/testdata/plan-ignore-changes/main.tf +++ b/terraform/testdata/plan-ignore-changes/main.tf @@ -1,9 +1,9 @@ variable "foo" {} resource "aws_instance" "foo" { - ami = "${var.foo}" + ami = var.foo lifecycle { - ignore_changes = ["ami"] + ignore_changes = [ami] } } From 4089b77c2ae17c1036e3055c99062e4b44171bd9 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 8 Sep 2020 15:52:44 -0400 Subject: [PATCH 13/20] Update vendored code --- vendor/github.com/zclconf/go-cty/cty/marks.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/vendor/github.com/zclconf/go-cty/cty/marks.go b/vendor/github.com/zclconf/go-cty/cty/marks.go index 0f6ff6aa2..e3e5ca74f 100644 --- a/vendor/github.com/zclconf/go-cty/cty/marks.go +++ b/vendor/github.com/zclconf/go-cty/cty/marks.go @@ -192,13 +192,13 @@ func (val Value) Mark(mark interface{}) Value { } // MarkWithPaths accepts a slice of PathValueMarks to apply -// marker particular paths +// markers to particular paths and returns the marked +// Value. func (val Value) MarkWithPaths(pvm []PathValueMarks) Value { ret, _ := Transform(val, func(p Path, v Value) (Value, error) { for _, path := range pvm { if p.Equals(path.Path) { return v.WithMarks(path.Marks), nil - } } return v, nil @@ -241,6 +241,10 @@ func (val Value) UnmarkDeep() (Value, ValueMarks) { return ret, marks } +// UnmarkDeepWithPaths is like UnmarkDeep, except it returns a slice +// of PathValueMarks rather than a superset of all marks. This allows +// a caller to know which marks are associated with which paths +// in the Value. func (val Value) UnmarkDeepWithPaths() (Value, []PathValueMarks) { var marks []PathValueMarks ret, _ := Transform(val, func(p Path, v Value) (Value, error) { From 3723594b3d861eaf27cef73b46fc1057533e89f3 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 9 Sep 2020 11:37:48 -0400 Subject: [PATCH 14/20] Point go module at master go-cty --- go.mod | 2 +- go.sum | 2 ++ vendor/modules.txt | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 523e7becc..f288d989f 100644 --- a/go.mod +++ b/go.mod @@ -123,7 +123,7 @@ require ( github.com/xanzy/ssh-agent v0.2.1 github.com/xiang90/probing v0.0.0-20160813154853-07dd2e8dfe18 // indirect github.com/xlab/treeprint v0.0.0-20161029104018-1d6e34225557 - github.com/zclconf/go-cty v1.6.1 + github.com/zclconf/go-cty v1.6.2-0.20200908203537-4ad5e68430d3 github.com/zclconf/go-cty-yaml v1.0.2 go.uber.org/atomic v1.3.2 // indirect go.uber.org/multierr v1.1.0 // indirect diff --git a/go.sum b/go.sum index 1ff07311f..fa1d5b21c 100644 --- a/go.sum +++ b/go.sum @@ -503,6 +503,8 @@ github.com/zclconf/go-cty v1.1.0/go.mod h1:xnAOWiHeOqg2nWS62VtQ7pbOu17FtxJNW8RLE github.com/zclconf/go-cty v1.2.0/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q/N4PQ8= github.com/zclconf/go-cty v1.6.1 h1:wHtZ+LSSQVwUSb+XIJ5E9hgAQxyWATZsAWT+ESJ9dQ0= github.com/zclconf/go-cty v1.6.1/go.mod h1:VDR4+I79ubFBGm1uJac1226K5yANQFHeauxPBoP54+o= +github.com/zclconf/go-cty v1.6.2-0.20200908203537-4ad5e68430d3 h1:iGouBJrrvGf/H4L6a2n7YBCO0FDhq81FEHI4ILDphkw= +github.com/zclconf/go-cty v1.6.2-0.20200908203537-4ad5e68430d3/go.mod h1:VDR4+I79ubFBGm1uJac1226K5yANQFHeauxPBoP54+o= github.com/zclconf/go-cty-yaml v1.0.2 h1:dNyg4QLTrv2IfJpm7Wtxi55ed5gLGOlPrZ6kMd51hY0= github.com/zclconf/go-cty-yaml v1.0.2/go.mod h1:IP3Ylp0wQpYm50IHK8OZWKMu6sPJIUgKa8XhiVHura0= go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU= diff --git a/vendor/modules.txt b/vendor/modules.txt index 7d8599b9c..6d1870e86 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -605,7 +605,7 @@ github.com/xanzy/ssh-agent # github.com/xlab/treeprint v0.0.0-20161029104018-1d6e34225557 ## explicit github.com/xlab/treeprint -# github.com/zclconf/go-cty v1.6.1 +# github.com/zclconf/go-cty v1.6.2-0.20200908203537-4ad5e68430d3 ## explicit github.com/zclconf/go-cty/cty github.com/zclconf/go-cty/cty/convert From 843ed8911b97ac6e85b21e20da0a119be55e7686 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 9 Sep 2020 12:07:07 -0400 Subject: [PATCH 15/20] Don't save PathValueMarks on instance_object --- states/instance_object.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/states/instance_object.go b/states/instance_object.go index a4dd671f5..2a9e58327 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -19,9 +19,6 @@ type ResourceInstanceObject struct { // Terraform. Value cty.Value - // PathValueMarks is a slice of paths and value marks of the value - PathValueMarks []cty.PathValueMarks - // Private is an opaque value set by the provider when this object was // last created or updated. Terraform Core does not use this value in // any way and it is not exposed anywhere in the user interface, so @@ -104,9 +101,7 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res // If it contains marks, dump those now unmarked := val if val.ContainsMarked() { - var pvm []cty.PathValueMarks - unmarked, pvm = val.UnmarkDeepWithPaths() - o.PathValueMarks = pvm + unmarked, _ = val.UnmarkDeep() } src, err := ctyjson.Marshal(unmarked, ty) if err != nil { From 02c1bddfe1c27ef8274b2dcffa16dd8cc2309f83 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 10 Sep 2020 10:08:04 -0400 Subject: [PATCH 16/20] Create experiment for sensitive attribute --- configs/experiments.go | 13 ++++++++++++- configs/testdata/valid-files/variables.tf | 4 ---- .../testdata/warning-files/variables-sensitive.tf | 7 +++++++ experiments/experiment.go | 2 ++ 4 files changed, 21 insertions(+), 5 deletions(-) create mode 100644 configs/testdata/warning-files/variables-sensitive.tf diff --git a/configs/experiments.go b/configs/experiments.go index 8af1e951f..aacca8e61 100644 --- a/configs/experiments.go +++ b/configs/experiments.go @@ -138,6 +138,17 @@ func checkModuleExperiments(m *Module) hcl.Diagnostics { } } */ - + if !m.ActiveExperiments.Has(experiments.SensitiveVariables) { + for _, v := range m.Variables { + if v.Sensitive { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Variable sensitivity is experimental", + Detail: "This feature is currently an opt-in experiment, subject to change in future releases based on feedback.\n\nActivate the feature for this module by adding sensitive_variables to the list of active experiments.", + Subject: v.DeclRange.Ptr(), + }) + } + } + } return diags } diff --git a/configs/testdata/valid-files/variables.tf b/configs/testdata/valid-files/variables.tf index 668761bc9..817649307 100644 --- a/configs/testdata/valid-files/variables.tf +++ b/configs/testdata/valid-files/variables.tf @@ -22,7 +22,3 @@ variable "cheeze_pizza" { variable "π" { default = 3.14159265359 } - -variable "sensitive-value" { - sensitive = true -} diff --git a/configs/testdata/warning-files/variables-sensitive.tf b/configs/testdata/warning-files/variables-sensitive.tf new file mode 100644 index 000000000..5fedc3986 --- /dev/null +++ b/configs/testdata/warning-files/variables-sensitive.tf @@ -0,0 +1,7 @@ +terraform { + experiments = [sensitive_variables] # WARNING: Experimental feature "sensitive_variables" is active +} + +variable "sensitive-value" { + sensitive = true +} \ No newline at end of file diff --git a/experiments/experiment.go b/experiments/experiment.go index cac7d54fc..f4ca707df 100644 --- a/experiments/experiment.go +++ b/experiments/experiment.go @@ -14,12 +14,14 @@ type Experiment string // identifier so that it can be specified in configuration. const ( VariableValidation = Experiment("variable_validation") + SensitiveVariables = Experiment("sensitive_variables") ) func init() { // Each experiment constant defined above must be registered here as either // a current or a concluded experiment. registerConcludedExperiment(VariableValidation, "Custom variable validation can now be used by default, without enabling an experiment.") + registerCurrentExperiment(SensitiveVariables) } // GetCurrent takes an experiment name and returns the experiment value From e4e16ccbd3138f6ab664a459c5dd573b37c186d2 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 10 Sep 2020 11:06:40 -0400 Subject: [PATCH 17/20] Rebase fix --- command/format/diff.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index a68431584..f5f13c1e4 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -133,8 +133,8 @@ func ResourceChange( changeV.Change.After = changeV.Change.After.MarkWithPaths(change.AfterValMarks) } - bodyWritten := p.writeBlockBodyDiff(schema, changeV.Before, changeV.After, 6, path) - if bodyWritten { + result := p.writeBlockBodyDiff(schema, changeV.Before, changeV.After, 6, path) + if result.bodyWritten { buf.WriteString("\n") buf.WriteString(strings.Repeat(" ", 4)) } From 4034cf9f759e5361366e4606cb34a7d4da92638e Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 10 Sep 2020 16:06:37 -0400 Subject: [PATCH 18/20] Add basic plan test coverage This also unearthed that the marking must happen earlier in the eval_diff in order to produce a valid plan (so that the planned marked value matches the marked config value) --- terraform/context.go | 2 +- terraform/context_plan_test.go | 55 +++++++++++++++++++ terraform/eval_diff.go | 10 ++-- .../plan-variable-sensitivity/main.tf | 12 ++++ 4 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 terraform/testdata/plan-variable-sensitivity/main.tf diff --git a/terraform/context.go b/terraform/context.go index cc2e5c7b7..07235776e 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -503,7 +503,6 @@ Note that the -target option is not suitable for routine use, and is provided on func (c *Context) Plan() (*plans.Plan, tfdiags.Diagnostics) { defer c.acquireRun("plan")() c.changes = plans.NewChanges() - var diags tfdiags.Diagnostics if len(c.targets) > 0 { @@ -575,6 +574,7 @@ The -target option is not for routine use, and is provided only for exceptional diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { + fmt.Println("walkerr") return nil, diags } p.Changes = c.changes diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index b81f730af..c981834bc 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -5626,6 +5626,61 @@ resource "aws_instance" "foo" { } } +func TestContext2Plan_variableSensitivity(t *testing.T) { + m := testModule(t, "plan-variable-sensitivity") + + p := testProvider("aws") + p.ValidateResourceTypeConfigFn = func(req providers.ValidateResourceTypeConfigRequest) (resp providers.ValidateResourceTypeConfigResponse) { + foo := req.Config.GetAttr("foo").AsString() + if foo == "bar" { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("foo cannot be bar")) + } + return + } + + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) { + resp.PlannedState = req.ProposedNewState + return + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatalf("unexpected errors: %s", diags.Err()) + } + schema := p.GetSchemaReturn.ResourceTypes["aws_instance"] + ty := schema.ImpliedType() + + if len(plan.Changes.Resources) != 1 { + t.Fatal("expected 1 changes, got", len(plan.Changes.Resources)) + } + + for _, res := range plan.Changes.Resources { + if res.Action != plans.Create { + t.Fatalf("expected resource creation, got %s", res.Action) + } + ric, err := res.Decode(ty) + if err != nil { + t.Fatal(err) + } + + switch i := ric.Addr.String(); i { + case "aws_instance.foo": + checkVals(t, objectVal(t, schema, map[string]cty.Value{ + "foo": cty.StringVal("foo"), + }), ric.After) + default: + t.Fatal("unknown instance:", i) + } + } +} + func checkVals(t *testing.T, expected, got cty.Value) { t.Helper() if !cmp.Equal(expected, got, valueComparer, typeComparer, equateEmpty) { diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index ac3f3ca6b..dcd25297e 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -256,6 +256,11 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { panic(fmt.Sprintf("PlanResourceChange of %s produced nil value", absAddr.String())) } + // Add the marks back to the planned new value + if configVal.ContainsMarked() { + plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) + } + // We allow the planned new value to disagree with configuration _values_ // here, since that allows the provider to do special logic like a // DiffSuppressFunc, but we still require that the provider produces @@ -473,11 +478,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } } - // Add the marks back to the planned new value - if configVal.ContainsMarked() { - plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) - } - // Call post-refresh hook if !n.Stub { err := ctx.Hook(func(h Hook) (HookAction, error) { diff --git a/terraform/testdata/plan-variable-sensitivity/main.tf b/terraform/testdata/plan-variable-sensitivity/main.tf new file mode 100644 index 000000000..e8b02a77c --- /dev/null +++ b/terraform/testdata/plan-variable-sensitivity/main.tf @@ -0,0 +1,12 @@ +terraform { + experiments = [sensitive_variables] +} + +variable "sensitive_var" { + default = "foo" + sensitive = true +} + +resource "aws_instance" "foo" { + foo = var.sensitive_var +} \ No newline at end of file From 8d8389da747eae261565805b9b4f8024372aa46d Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 10 Sep 2020 16:45:31 -0400 Subject: [PATCH 19/20] Add diff test with a sensitive change Adds a diff test for a changed value, and modifies the diff file to cover variable diffs on sensitive values --- command/format/diff.go | 26 +++++++++++-- command/format/diff_test.go | 73 +++++++++++++++++++++++++++++++++++-- 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index f5f13c1e4..ede5620e6 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -301,10 +301,18 @@ func (p *blockBodyDiffPrinter) writeBlockBodyDiff(schema *configschema.Block, ol return result } -func (p *blockBodyDiffPrinter) writeAttrDiff(name string, attrS *configschema.Attribute, old, new cty.Value, nameLen, indent int, path cty.Path) bool { - path = append(path, cty.GetAttrStep{Name: name}) - showJustNew := false +// getPlanActionAndShow returns the action value +// and a boolean for showJustNew. In this function we +// modify the old and new values to remove any possible marks +func getPlanActionAndShow(old cty.Value, new cty.Value) (plans.Action, bool) { var action plans.Action + showJustNew := false + if old.ContainsMarked() { + old, _ = old.UnmarkDeep() + } + if new.ContainsMarked() { + new, _ = new.UnmarkDeep() + } switch { case old.IsNull(): action = plans.Create @@ -317,6 +325,12 @@ func (p *blockBodyDiffPrinter) writeAttrDiff(name string, attrS *configschema.At default: action = plans.Update } + return action, showJustNew +} + +func (p *blockBodyDiffPrinter) writeAttrDiff(name string, attrS *configschema.Attribute, old, new cty.Value, nameLen, indent int, path cty.Path) bool { + path = append(path, cty.GetAttrStep{Name: name}) + action, showJustNew := getPlanActionAndShow(old, new) if action == plans.NoOp && p.concise && !identifyingAttribute(name, attrS) { return true @@ -753,6 +767,12 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa ty := old.Type() typesEqual := ctyTypesEqual(ty, new.Type()) + // If either the old or new value is marked, don't display the value + if old.ContainsMarked() || new.ContainsMarked() { + p.buf.WriteString("(sensitive)") + return + } + // We have some specialized diff implementations for certain complex // values where it's useful to see a visualization of the diff of // the nested elements rather than just showing the entire old and diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 00d0c82c8..b885791a3 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3622,10 +3622,75 @@ func TestResourceChange_nestedMap(t *testing.T) { runTestCases(t, testCases) } +func TestResourceChange_sensitiveVariable(t *testing.T) { + testCases := map[string]testCase{ + "in-place update - creation": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "root_block_device": cty.MapValEmpty(cty.Object(map[string]cty.Type{ + "volume_type": cty.String, + })), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + "root_block_device": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + }), + }), + }), + AfterValMarks: []cty.PathValueMarks{ + { + Path: cty.Path{cty.GetAttrStep{Name: "ami"}}, + Marks: cty.NewValueMarks("sensitive"), + }}, + RequiredReplace: cty.NewPathSet(), + Tainted: false, + Schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Optional: true, Computed: true}, + "ami": {Type: cty.String, Optional: true}, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "root_block_device": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "volume_type": { + Type: cty.String, + Optional: true, + Computed: true, + }, + }, + }, + Nesting: configschema.NestingMap, + }, + }, + }, + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + ~ ami = (sensitive) + id = "i-02ae66f368e8518a9" + + + root_block_device "a" { + + volume_type = "gp2" + } + } +`, + }, + } + runTestCases(t, testCases) +} + type testCase struct { Action plans.Action Mode addrs.ResourceMode Before cty.Value + BeforeValMarks []cty.PathValueMarks + AfterValMarks []cty.PathValueMarks After cty.Value Schema *configschema.Block RequiredReplace cty.PathSet @@ -3679,9 +3744,11 @@ func runTestCases(t *testing.T, testCases map[string]testCase) { Module: addrs.RootModule, }, ChangeSrc: plans.ChangeSrc{ - Action: tc.Action, - Before: before, - After: after, + Action: tc.Action, + Before: before, + After: after, + BeforeValMarks: tc.BeforeValMarks, + AfterValMarks: tc.AfterValMarks, }, RequiredReplace: tc.RequiredReplace, } From 20ee878d0e666b97177995197c1fad8f180bf87e Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Fri, 11 Sep 2020 11:15:44 -0400 Subject: [PATCH 20/20] Updates and improvements to comments --- plans/changes_src.go | 6 +++++- terraform/eval_diff.go | 3 +-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/plans/changes_src.go b/plans/changes_src.go index a5093bb56..683c9f174 100644 --- a/plans/changes_src.go +++ b/plans/changes_src.go @@ -157,7 +157,11 @@ type ChangeSrc struct { // storage. Before, After DynamicValue - // Marked Paths + // BeforeValMarks and AfterValMarks are stored path+mark combinations + // that might be discovered when encoding a change. Marks are removed + // to enable encoding (marked values cannot be marshalled), and so storing + // the path+mark combinations allow us to re-mark the value later + // when, for example, displaying the diff to the UI. BeforeValMarks, AfterValMarks []cty.PathValueMarks } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index dcd25297e..e263a7a09 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -148,8 +148,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { var unmarkedPaths []cty.PathValueMarks if configVal.ContainsMarked() { // store the marked values so we can re-mark them later after - // we've sent things over the wire. Right now this stores - // one path for proof of concept, but we should store multiple + // we've sent things over the wire. unmarkedConfigVal, unmarkedPaths = configVal.UnmarkDeepWithPaths() }