From 20921dbfb8f962aac3a0e25b7ac1f4e104c1b00f Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 23 Sep 2020 16:28:55 -0400 Subject: [PATCH 1/4] Add warning about sensitivity change This commit adds a warning before displaying a sensitive diff, and always obfuscates the old value (even if it was not previously marked as sensitive) --- command/format/diff.go | 65 ++++++++++++++++++++++++++++++------- command/format/diff_test.go | 10 ++++-- 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index ede5620e6..4a14bf119 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -337,6 +337,12 @@ func (p *blockBodyDiffPrinter) writeAttrDiff(name string, attrS *configschema.At } p.buf.WriteString("\n") + + // Write sensitivity warning for non-delete plans + if action != plans.Delete && action != plans.Create { + p.writeSensitivityWarning(old, new, indent) + } + p.buf.WriteString(strings.Repeat(" ", indent)) p.writeActionSymbol(action) @@ -767,12 +773,6 @@ 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 @@ -780,6 +780,15 @@ 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 marked, create unmarked values for comparisons + unmarkedOld := old + unmarkedNew := new + if old.ContainsMarked() { + unmarkedOld, _ = old.UnmarkDeep() + } + if new.ContainsMarked() { + unmarkedNew, _ = new.UnmarkDeep() + } switch { case ty == cty.String: // We have special behavior for both multi-line strings in general @@ -787,6 +796,11 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // to apply, both old and new must be valid JSON. // For single-line strings that don't parse as JSON we just fall // out of this switch block and do the default old -> new rendering. + + if old.ContainsMarked() || new.ContainsMarked() { + p.buf.WriteString("(sensitive)") + return + } oldS := old.AsString() newS := new.AsString() @@ -811,7 +825,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteByte(')') } else { - // if they differ only in insigificant whitespace + // if they differ only in insignificant whitespace // then we'll note that but still expand out the // effective value. if p.pathForcesNewResource(path) { @@ -1104,7 +1118,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa action = plans.Create } else if new.HasIndex(kV).False() { action = plans.Delete - } else if eqV := old.Index(kV).Equals(new.Index(kV)); eqV.IsKnown() && eqV.True() { + } else if eqV := unmarkedOld.Index(kV).Equals(unmarkedNew.Index(kV)); eqV.IsKnown() && eqV.True() { action = plans.NoOp } else { action = plans.Update @@ -1117,6 +1131,10 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa path := append(path, cty.IndexStep{Key: kV}) + oldV := old.Index(kV) + newV := new.Index(kV) + p.writeSensitivityWarning(oldV, newV, indent+2) + p.buf.WriteString(strings.Repeat(" ", indent+2)) p.writeActionSymbol(action) p.writeValue(kV, action, indent+4) @@ -1125,15 +1143,21 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa switch action { case plans.Create, plans.NoOp: v := new.Index(kV) - p.writeValue(v, action, indent+4) + if v.IsMarked() { + p.buf.WriteString("(sensitive)") + } else { + p.writeValue(v, action, indent+4) + } case plans.Delete: oldV := old.Index(kV) newV := cty.NullVal(oldV.Type()) p.writeValueDiff(oldV, newV, indent+4, path) default: - oldV := old.Index(kV) - newV := new.Index(kV) - p.writeValueDiff(oldV, newV, indent+4, path) + if oldV.IsMarked() || newV.IsMarked() { + p.buf.WriteString("(sensitive)") + } else { + p.writeValueDiff(oldV, newV, indent+4, path) + } } p.buf.WriteByte('\n') @@ -1287,6 +1311,23 @@ func (p *blockBodyDiffPrinter) writeActionSymbol(action plans.Action) { } } +func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, indent int) { + if new.IsMarked() && !old.IsMarked() { + p.buf.WriteString(strings.Repeat(" ", indent)) + p.buf.WriteString(p.color.Color("[yellow]# Warning: this attribute value will be marked as sensitive and will\n")) + p.buf.WriteString(strings.Repeat(" ", indent)) + p.buf.WriteString(p.color.Color("[yellow]# not display in UI output after applying this change[reset]\n")) + } + + // Note if changing this attribute will change its sensitivity + if old.IsMarked() && !new.IsMarked() { + p.buf.WriteString(strings.Repeat(" ", indent)) + p.buf.WriteString(p.color.Color("[yellow]# Warning: this attribute value will no longer be marked as sensitive\n")) + p.buf.WriteString(strings.Repeat(" ", indent)) + p.buf.WriteString(p.color.Color("[yellow]# after applying this change[reset]\n")) + } +} + func (p *blockBodyDiffPrinter) pathForcesNewResource(path cty.Path) bool { if !p.action.IsReplace() || p.requiredReplace.Empty() { // "requiredReplace" only applies when the instance is being replaced, diff --git a/command/format/diff_test.go b/command/format/diff_test.go index d333315d1..856dd25d2 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3678,6 +3678,8 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { }, ExpectedOutput: ` # test_instance.example will be updated in-place ~ resource "test_instance" "example" { + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change ~ ami = (sensitive) id = "i-02ae66f368e8518a9" } @@ -3714,7 +3716,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { ExpectedOutput: ` # test_instance.example will be updated in-place ~ resource "test_instance" "example" { id = "i-02ae66f368e8518a9" - ~ tags = (sensitive) + ~ tags = { + # Warning: this attribute value will be marked as sensitive and will + # not display in UI output after applying this change + ~ "name" = (sensitive) + } } `, }, @@ -3777,7 +3783,7 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { }, ExpectedOutput: ` # test_instance.example will be destroyed - resource "test_instance" "example" { - - ami = (sensitive) + - ami = (sensitive) -> null - id = "i-02ae66f368e8518a9" -> null } `, From 531728f6e91d86a52dc4789dfcb70f2237b24023 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 24 Sep 2020 13:27:15 -0400 Subject: [PATCH 2/4] Sensitive diffs for primitive types When showing primitive type diffs, hide possibly sensitive values --- command/format/diff.go | 5 +++++ command/format/diff_test.go | 41 ++++++++++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 4a14bf119..53e7fd68e 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -790,6 +790,11 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa unmarkedNew, _ = new.UnmarkDeep() } switch { + case ty == cty.Bool || ty == cty.Number: + if old.ContainsMarked() || new.ContainsMarked() { + p.buf.WriteString("(sensitive)") + return + } case ty == cty.String: // We have special behavior for both multi-line strings in general // and for strings that can parse as JSON. For the JSON handling diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 856dd25d2..1d0a2fedb 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3652,36 +3652,57 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { } `, }, - "in-place update - before sensitive": { + "in-place update - before sensitive, primitive types": { Action: plans.Update, Mode: addrs.ManagedResourceMode, Before: cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("i-02ae66f368e8518a9"), - "ami": cty.StringVal("ami-BEFORE"), + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "special": cty.BoolVal(true), + "some_number": cty.NumberIntVal(1), }), After: cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("i-02ae66f368e8518a9"), - "ami": cty.StringVal("ami-AFTER"), + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + "special": cty.BoolVal(false), + "some_number": cty.NumberIntVal(2), }), BeforeValMarks: []cty.PathValueMarks{ { Path: cty.Path{cty.GetAttrStep{Name: "ami"}}, Marks: cty.NewValueMarks("sensitive"), - }}, + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "special"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + { + Path: cty.Path{cty.GetAttrStep{Name: "some_number"}}, + 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}, + "id": {Type: cty.String, Optional: true, Computed: true}, + "ami": {Type: cty.String, Optional: true}, + "special": {Type: cty.Bool, Optional: true}, + "some_number": {Type: cty.Number, Optional: true}, }, }, ExpectedOutput: ` # test_instance.example will be updated in-place ~ resource "test_instance" "example" { # Warning: this attribute value will no longer be marked as sensitive # after applying this change - ~ ami = (sensitive) - id = "i-02ae66f368e8518a9" + ~ ami = (sensitive) + id = "i-02ae66f368e8518a9" + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change + ~ some_number = (sensitive) + # Warning: this attribute value will no longer be marked as sensitive + # after applying this change + ~ special = (sensitive) } `, }, From 3c9fad0b0e213fae71cd5daf14c6543bf5d424d9 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 24 Sep 2020 13:49:34 -0400 Subject: [PATCH 3/4] Move plan action check into the sensitivity warning method --- command/format/diff.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 53e7fd68e..5baf7d6f8 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -338,10 +338,7 @@ func (p *blockBodyDiffPrinter) writeAttrDiff(name string, attrS *configschema.At p.buf.WriteString("\n") - // Write sensitivity warning for non-delete plans - if action != plans.Delete && action != plans.Create { - p.writeSensitivityWarning(old, new, indent) - } + p.writeSensitivityWarning(old, new, indent, action) p.buf.WriteString(strings.Repeat(" ", indent)) p.writeActionSymbol(action) @@ -1138,7 +1135,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa oldV := old.Index(kV) newV := new.Index(kV) - p.writeSensitivityWarning(oldV, newV, indent+2) + p.writeSensitivityWarning(oldV, newV, indent+2, action) p.buf.WriteString(strings.Repeat(" ", indent+2)) p.writeActionSymbol(action) @@ -1316,7 +1313,12 @@ func (p *blockBodyDiffPrinter) writeActionSymbol(action plans.Action) { } } -func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, indent int) { +func (p *blockBodyDiffPrinter) writeSensitivityWarning(old, new cty.Value, indent int, action plans.Action) { + // Dont' show this warning for create or delete + if action == plans.Create || action == plans.Delete { + return + } + if new.IsMarked() && !old.IsMarked() { p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteString(p.color.Color("[yellow]# Warning: this attribute value will be marked as sensitive and will\n")) From 5b549224ae67b629582de47e16ddd2fe19a30f4d Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Thu, 24 Sep 2020 16:42:03 -0400 Subject: [PATCH 4/4] Refactor to call ContainsMarked less and use len() instead --- command/format/diff.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 5baf7d6f8..815899058 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -777,18 +777,12 @@ 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 marked, create unmarked values for comparisons - unmarkedOld := old - unmarkedNew := new - if old.ContainsMarked() { - unmarkedOld, _ = old.UnmarkDeep() - } - if new.ContainsMarked() { - unmarkedNew, _ = new.UnmarkDeep() - } + // Create unmarked values for comparisons + unmarkedOld, oldMarks := old.UnmarkDeep() + unmarkedNew, newMarks := new.UnmarkDeep() switch { case ty == cty.Bool || ty == cty.Number: - if old.ContainsMarked() || new.ContainsMarked() { + if len(oldMarks) > 0 || len(newMarks) > 0 { p.buf.WriteString("(sensitive)") return } @@ -799,7 +793,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // For single-line strings that don't parse as JSON we just fall // out of this switch block and do the default old -> new rendering. - if old.ContainsMarked() || new.ContainsMarked() { + if len(oldMarks) > 0 || len(newMarks) > 0 { p.buf.WriteString("(sensitive)") return }