From c89004d223365d634bb9506d9e32725dd893b5d9 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Mon, 26 Apr 2021 16:26:47 -0400 Subject: [PATCH] core: Add sensitive provider attrs to JSON plan When rendering a stored plan file as JSON, we include a data structure representing the sensitivity of the changed resource values. Prior to this commit, this was a direct representation of the sensitivity marks applied to values via mechanisms such as sensitive variables, sensitive outputs, and the `sensitive` function. This commit extends this to include sensitivity based on the provider schema. This is in line with the UI rendering of the plan, which considers these two different types of sensitivity to be equivalent. Co-authored-by: Kristin Laemmert --- command/jsonplan/plan.go | 12 +- command/show_test.go | 145 +++++++++++++ command/testdata/show-json-sensitive/main.tf | 21 ++ .../testdata/show-json-sensitive/output.json | 205 ++++++++++++++++++ configs/configschema/marks.go | 57 +++++ configs/configschema/marks_test.go | 100 +++++++++ terraform/evaluate.go | 51 +---- terraform/evaluate_test.go | 93 -------- 8 files changed, 540 insertions(+), 144 deletions(-) create mode 100644 command/testdata/show-json-sensitive/main.tf create mode 100644 command/testdata/show-json-sensitive/output.json create mode 100644 configs/configschema/marks.go create mode 100644 configs/configschema/marks_test.go diff --git a/command/jsonplan/plan.go b/command/jsonplan/plan.go index 4d8dccd95..8474ceac0 100644 --- a/command/jsonplan/plan.go +++ b/command/jsonplan/plan.go @@ -213,7 +213,11 @@ func (p *plan) marshalResourceChanges(changes *plans.Changes, schemas *terraform if err != nil { return err } - bs := sensitiveAsBool(changeV.Before.MarkWithPaths(rc.BeforeValMarks)) + marks := rc.BeforeValMarks + if schema.ContainsSensitive() { + marks = append(marks, schema.ValueMarks(changeV.Before, nil)...) + } + bs := sensitiveAsBool(changeV.Before.MarkWithPaths(marks)) beforeSensitive, err = ctyjson.Marshal(bs, bs.Type()) if err != nil { return err @@ -238,7 +242,11 @@ func (p *plan) marshalResourceChanges(changes *plans.Changes, schemas *terraform } afterUnknown = unknownAsBool(changeV.After) } - as := sensitiveAsBool(changeV.After.MarkWithPaths(rc.AfterValMarks)) + marks := rc.AfterValMarks + if schema.ContainsSensitive() { + marks = append(marks, schema.ValueMarks(changeV.After, nil)...) + } + as := sensitiveAsBool(changeV.After.MarkWithPaths(marks)) afterSensitive, err = ctyjson.Marshal(as, as.Type()) if err != nil { return err diff --git a/command/show_test.go b/command/show_test.go index e0172ba58..2fcafcc19 100644 --- a/command/show_test.go +++ b/command/show_test.go @@ -343,6 +343,88 @@ func TestShow_json_output(t *testing.T) { } } +func TestShow_json_output_sensitive(t *testing.T) { + td := tempDir(t) + inputDir := "testdata/show-json-sensitive" + testCopyDir(t, inputDir, td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + providerSource, close := newMockProviderSource(t, map[string][]string{"test": {"1.2.3"}}) + defer close() + + p := showFixtureSensitiveProvider() + ui := new(cli.MockUi) + view, _ := testView(t) + m := Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + View: view, + ProviderSource: providerSource, + } + + // init + ic := &InitCommand{ + Meta: m, + } + if code := ic.Run([]string{}); code != 0 { + t.Fatalf("init failed\n%s", ui.ErrorWriter) + } + + // flush init output + ui.OutputWriter.Reset() + + pc := &PlanCommand{ + Meta: m, + } + + args := []string{ + "-out=terraform.plan", + } + + if code := pc.Run(args); code != 0 { + fmt.Println(ui.OutputWriter.String()) + t.Fatalf("wrong exit status %d; want 0\nstderr: %s", code, ui.ErrorWriter.String()) + } + + // flush the plan output from the mock ui + ui.OutputWriter.Reset() + sc := &ShowCommand{ + Meta: m, + } + + args = []string{ + "-json", + "terraform.plan", + } + defer os.Remove("terraform.plan") + + if code := sc.Run(args); code != 0 { + t.Fatalf("wrong exit status %d; want 0\nstderr: %s", code, ui.ErrorWriter.String()) + } + + // compare ui output to wanted output + var got, want plan + + gotString := ui.OutputWriter.String() + json.Unmarshal([]byte(gotString), &got) + + wantFile, err := os.Open("output.json") + if err != nil { + t.Fatalf("err: %s", err) + } + defer wantFile.Close() + byteValue, err := ioutil.ReadAll(wantFile) + if err != nil { + t.Fatalf("err: %s", err) + } + json.Unmarshal([]byte(byteValue), &want) + + if !cmp.Equal(got, want) { + t.Fatalf("wrong result:\n %v\n", cmp.Diff(got, want)) + } +} + // similar test as above, without the plan func TestShow_json_output_state(t *testing.T) { fixtureDir := "testdata/show-json-state" @@ -450,6 +532,32 @@ func showFixtureSchema() *providers.GetProviderSchemaResponse { } } +// showFixtureSensitiveSchema returns a schema suitable for processing the configuration +// in testdata/show. This schema should be assigned to a mock provider +// named "test". It includes a sensitive attribute. +func showFixtureSensitiveSchema() *providers.GetProviderSchemaResponse { + return &providers.GetProviderSchemaResponse{ + Provider: providers.Schema{ + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "region": {Type: cty.String, Optional: true}, + }, + }, + }, + ResourceTypes: map[string]providers.Schema{ + "test_instance": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Optional: true, Computed: true}, + "ami": {Type: cty.String, Optional: true}, + "password": {Type: cty.String, Optional: true, Sensitive: true}, + }, + }, + }, + }, + } +} + // showFixtureProvider returns a mock provider that is configured for basic // operation with the configuration in testdata/show. This mock has // GetSchemaResponse, PlanResourceChangeFn, and ApplyResourceChangeFn populated, @@ -487,6 +595,43 @@ func showFixtureProvider() *terraform.MockProvider { return p } +// showFixtureSensitiveProvider returns a mock provider that is configured for basic +// operation with the configuration in testdata/show. This mock has +// GetSchemaResponse, PlanResourceChangeFn, and ApplyResourceChangeFn populated, +// with the plan/apply steps just passing through the data determined by +// Terraform Core. It also has a sensitive attribute in the provider schema. +func showFixtureSensitiveProvider() *terraform.MockProvider { + p := testProvider() + p.GetProviderSchemaResponse = showFixtureSensitiveSchema() + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + idVal := req.ProposedNewState.GetAttr("id") + if idVal.IsNull() { + idVal = cty.UnknownVal(cty.String) + } + return providers.PlanResourceChangeResponse{ + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "id": idVal, + "ami": req.ProposedNewState.GetAttr("ami"), + "password": req.ProposedNewState.GetAttr("password"), + }), + } + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + idVal := req.PlannedState.GetAttr("id") + if !idVal.IsKnown() { + idVal = cty.StringVal("placeholder") + } + return providers.ApplyResourceChangeResponse{ + NewState: cty.ObjectVal(map[string]cty.Value{ + "id": idVal, + "ami": req.PlannedState.GetAttr("ami"), + "password": req.PlannedState.GetAttr("password"), + }), + } + } + return p +} + // showFixturePlanFile creates a plan file at a temporary location containing a // single change to create or update the test_instance.foo that is included in the "show" // test fixture, returning the location of that plan file. diff --git a/command/testdata/show-json-sensitive/main.tf b/command/testdata/show-json-sensitive/main.tf new file mode 100644 index 000000000..a980e4dc4 --- /dev/null +++ b/command/testdata/show-json-sensitive/main.tf @@ -0,0 +1,21 @@ +provider "test" { + region = "somewhere" +} + +variable "test_var" { + default = "bar" + sensitive = true +} + +resource "test_instance" "test" { + // this variable is sensitive + ami = var.test_var + // the password attribute is sensitive in the showFixtureSensitiveProvider schema. + password = "secret" + count = 3 +} + +output "test" { + value = var.test_var + sensitive = true +} diff --git a/command/testdata/show-json-sensitive/output.json b/command/testdata/show-json-sensitive/output.json new file mode 100644 index 000000000..f625fb316 --- /dev/null +++ b/command/testdata/show-json-sensitive/output.json @@ -0,0 +1,205 @@ +{ + "format_version": "0.1", + "variables": { + "test_var": { + "value": "bar" + } + }, + "planned_values": { + "outputs": { + "test": { + "sensitive": true, + "value": "bar" + } + }, + "root_module": { + "resources": [ + { + "address": "test_instance.test[0]", + "index": 0, + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_name": "registry.terraform.io/hashicorp/test", + "schema_version": 0, + "values": { + "ami": "bar", + "password": "secret" + } + }, + { + "address": "test_instance.test[1]", + "index": 1, + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_name": "registry.terraform.io/hashicorp/test", + "schema_version": 0, + "values": { + "ami": "bar", + "password": "secret" + } + }, + { + "address": "test_instance.test[2]", + "index": 2, + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_name": "registry.terraform.io/hashicorp/test", + "schema_version": 0, + "values": { + "ami": "bar", + "password": "secret" + } + } + ] + } + }, + "prior_state": { + "format_version": "0.1", + "values": { + "outputs": { + "test": { + "sensitive": true, + "value": "bar" + } + }, + "root_module": {} + } + }, + "resource_changes": [ + { + "address": "test_instance.test[0]", + "index": 0, + "mode": "managed", + "type": "test_instance", + "provider_name": "registry.terraform.io/hashicorp/test", + "name": "test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after_unknown": { + "id": true + }, + "after": { + "ami": "bar", + "password": "secret" + }, + "after_sensitive": {"ami": true, "password": true}, + "before_sensitive": false + } + }, + { + "address": "test_instance.test[1]", + "index": 1, + "mode": "managed", + "type": "test_instance", + "provider_name": "registry.terraform.io/hashicorp/test", + "name": "test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after_unknown": { + "id": true + }, + "after": { + "ami": "bar", + "password": "secret" + }, + "after_sensitive": {"ami": true, "password": true}, + "before_sensitive": false + } + }, + { + "address": "test_instance.test[2]", + "index": 2, + "mode": "managed", + "type": "test_instance", + "provider_name": "registry.terraform.io/hashicorp/test", + "name": "test", + "change": { + "actions": [ + "create" + ], + "before": null, + "after_unknown": { + "id": true + }, + "after": { + "ami": "bar", + "password": "secret" + }, + "after_sensitive": {"ami": true, "password": true}, + "before_sensitive": false + } + } + ], + "output_changes": { + "test": { + "actions": [ + "create" + ], + "before": null, + "after": "bar", + "after_unknown": false, + "before_sensitive": true, + "after_sensitive": true + } + }, + "configuration": { + "provider_config": { + "test": { + "name": "test", + "expressions": { + "region": { + "constant_value": "somewhere" + } + } + } + }, + "root_module": { + "outputs": { + "test": { + "expression": { + "references": [ + "var.test_var" + ] + }, + "sensitive": true + } + }, + "resources": [ + { + "address": "test_instance.test", + "mode": "managed", + "type": "test_instance", + "name": "test", + "provider_config_key": "test", + "schema_version": 0, + "expressions": { + "ami": { + "references": [ + "var.test_var" + ] + }, + "password": {"constant_value": "secret"} + }, + "count_expression": { + "constant_value": 3 + } + } + ], + "variables": { + "test_var": { + "default": "bar", + "sensitive": true + } + } + } + } +} diff --git a/configs/configschema/marks.go b/configs/configschema/marks.go new file mode 100644 index 000000000..8b468ec9f --- /dev/null +++ b/configs/configschema/marks.go @@ -0,0 +1,57 @@ +package configschema + +import ( + "fmt" + + "github.com/zclconf/go-cty/cty" +) + +// ValueMarks returns a set of path value marks for a given value and path, +// based on the sensitive flag for each attribute within the schema. Nested +// blocks are descended (if present in the given value). +func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks { + var pvm []cty.PathValueMarks + for name, attrS := range b.Attributes { + if attrS.Sensitive { + // Create a copy of the path, with this step added, to add to our PathValueMarks slice + attrPath := make(cty.Path, len(path), len(path)+1) + copy(attrPath, path) + attrPath = append(path, cty.GetAttrStep{Name: name}) + pvm = append(pvm, cty.PathValueMarks{ + Path: attrPath, + Marks: cty.NewValueMarks("sensitive"), + }) + } + } + + for name, blockS := range b.BlockTypes { + // If our block doesn't contain any sensitive attributes, skip inspecting it + if !blockS.Block.ContainsSensitive() { + continue + } + + blockV := val.GetAttr(name) + if blockV.IsNull() || !blockV.IsKnown() { + continue + } + + // Create a copy of the path, with this step added, to add to our PathValueMarks slice + blockPath := make(cty.Path, len(path), len(path)+1) + copy(blockPath, path) + blockPath = append(path, cty.GetAttrStep{Name: name}) + + switch blockS.Nesting { + case NestingSingle, NestingGroup: + pvm = append(pvm, blockS.Block.ValueMarks(blockV, blockPath)...) + case NestingList, NestingMap, NestingSet: + for it := blockV.ElementIterator(); it.Next(); { + idx, blockEV := it.Element() + morePaths := blockS.Block.ValueMarks(blockEV, append(blockPath, cty.IndexStep{Key: idx})) + pvm = append(pvm, morePaths...) + } + default: + panic(fmt.Sprintf("unsupported nesting mode %s", blockS.Nesting)) + } + } + return pvm +} diff --git a/configs/configschema/marks_test.go b/configs/configschema/marks_test.go new file mode 100644 index 000000000..9a21e4518 --- /dev/null +++ b/configs/configschema/marks_test.go @@ -0,0 +1,100 @@ +package configschema + +import ( + "fmt" + "testing" + + "github.com/zclconf/go-cty/cty" +) + +func TestBlockValueMarks(t *testing.T) { + schema := &Block{ + Attributes: map[string]*Attribute{ + "unsensitive": { + Type: cty.String, + Optional: true, + }, + "sensitive": { + Type: cty.String, + Sensitive: true, + }, + }, + + BlockTypes: map[string]*NestedBlock{ + "list": { + Nesting: NestingList, + Block: Block{ + Attributes: map[string]*Attribute{ + "unsensitive": { + Type: cty.String, + Optional: true, + }, + "sensitive": { + Type: cty.String, + Sensitive: true, + }, + }, + }, + }, + }, + } + + for _, tc := range []struct { + given cty.Value + expect cty.Value + }{ + { + cty.UnknownVal(schema.ImpliedType()), + cty.UnknownVal(schema.ImpliedType()), + }, + { + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.UnknownVal(cty.String), + "unsensitive": cty.UnknownVal(cty.String), + "list": cty.UnknownVal(schema.BlockTypes["list"].ImpliedType()), + }), + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.UnknownVal(cty.String).Mark("sensitive"), + "unsensitive": cty.UnknownVal(cty.String), + "list": cty.UnknownVal(schema.BlockTypes["list"].ImpliedType()), + }), + }, + { + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.NullVal(cty.String), + "unsensitive": cty.UnknownVal(cty.String), + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.UnknownVal(cty.String), + "unsensitive": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.NullVal(cty.String), + "unsensitive": cty.NullVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.NullVal(cty.String).Mark("sensitive"), + "unsensitive": cty.UnknownVal(cty.String), + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.UnknownVal(cty.String).Mark("sensitive"), + "unsensitive": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "sensitive": cty.NullVal(cty.String).Mark("sensitive"), + "unsensitive": cty.NullVal(cty.String), + }), + }), + }), + }, + } { + t.Run(fmt.Sprintf("%#v", tc.given), func(t *testing.T) { + got := tc.given.MarkWithPaths(schema.ValueMarks(tc.given, nil)) + if !got.RawEquals(tc.expect) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", tc.expect, got) + } + }) + } +} diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 2417d2f64..282569ee1 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -760,7 +760,7 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc // If our provider schema contains sensitive values, mark those as sensitive afterMarks := change.AfterValMarks if schema.ContainsSensitive() { - afterMarks = append(afterMarks, getValMarks(schema, val, nil)...) + afterMarks = append(afterMarks, schema.ValueMarks(val, nil)...) } instances[key] = val.MarkWithPaths(afterMarks) @@ -789,7 +789,7 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc if schema.ContainsSensitive() { var marks []cty.PathValueMarks val, marks = val.UnmarkDeepWithPaths() - marks = append(marks, getValMarks(schema, val, nil)...) + marks = append(marks, schema.ValueMarks(val, nil)...) val = val.MarkWithPaths(marks) } instances[key] = val @@ -959,50 +959,3 @@ func moduleDisplayAddr(addr addrs.ModuleInstance) string { return addr.String() } } - -func getValMarks(schema *configschema.Block, val cty.Value, path cty.Path) []cty.PathValueMarks { - var pvm []cty.PathValueMarks - for name, attrS := range schema.Attributes { - if attrS.Sensitive { - // Create a copy of the path, with this step added, to add to our PathValueMarks slice - attrPath := make(cty.Path, len(path), len(path)+1) - copy(attrPath, path) - attrPath = append(path, cty.GetAttrStep{Name: name}) - pvm = append(pvm, cty.PathValueMarks{ - Path: attrPath, - Marks: cty.NewValueMarks("sensitive"), - }) - } - } - - for name, blockS := range schema.BlockTypes { - // If our block doesn't contain any sensitive attributes, skip inspecting it - if !blockS.Block.ContainsSensitive() { - continue - } - - blockV := val.GetAttr(name) - if blockV.IsNull() || !blockV.IsKnown() { - continue - } - - // Create a copy of the path, with this step added, to add to our PathValueMarks slice - blockPath := make(cty.Path, len(path), len(path)+1) - copy(blockPath, path) - blockPath = append(path, cty.GetAttrStep{Name: name}) - - switch blockS.Nesting { - case configschema.NestingSingle, configschema.NestingGroup: - pvm = append(pvm, getValMarks(&blockS.Block, blockV, blockPath)...) - case configschema.NestingList, configschema.NestingMap, configschema.NestingSet: - for it := blockV.ElementIterator(); it.Next(); { - idx, blockEV := it.Element() - morePaths := getValMarks(&blockS.Block, blockEV, append(blockPath, cty.IndexStep{Key: idx})) - pvm = append(pvm, morePaths...) - } - default: - panic(fmt.Sprintf("unsupported nesting mode %s", blockS.Nesting)) - } - } - return pvm -} diff --git a/terraform/evaluate_test.go b/terraform/evaluate_test.go index c25429e93..8c57c8e65 100644 --- a/terraform/evaluate_test.go +++ b/terraform/evaluate_test.go @@ -1,7 +1,6 @@ package terraform import ( - "fmt" "sync" "testing" @@ -562,95 +561,3 @@ func evaluatorForModule(stateSync *states.SyncState, changesSync *plans.ChangesS Changes: changesSync, } } - -func TestGetValMarks(t *testing.T) { - schema := &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "unsensitive": { - Type: cty.String, - Optional: true, - }, - "sensitive": { - Type: cty.String, - Sensitive: true, - }, - }, - - BlockTypes: map[string]*configschema.NestedBlock{ - "list": &configschema.NestedBlock{ - Nesting: configschema.NestingList, - Block: configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "unsensitive": { - Type: cty.String, - Optional: true, - }, - "sensitive": { - Type: cty.String, - Sensitive: true, - }, - }, - }, - }, - }, - } - - for _, tc := range []struct { - given cty.Value - expect cty.Value - }{ - { - cty.UnknownVal(schema.ImpliedType()), - cty.UnknownVal(schema.ImpliedType()), - }, - { - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.UnknownVal(cty.String), - "unsensitive": cty.UnknownVal(cty.String), - "list": cty.UnknownVal(schema.BlockTypes["list"].ImpliedType()), - }), - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.UnknownVal(cty.String).Mark("sensitive"), - "unsensitive": cty.UnknownVal(cty.String), - "list": cty.UnknownVal(schema.BlockTypes["list"].ImpliedType()), - }), - }, - { - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.NullVal(cty.String), - "unsensitive": cty.UnknownVal(cty.String), - "list": cty.ListVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.UnknownVal(cty.String), - "unsensitive": cty.UnknownVal(cty.String), - }), - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.NullVal(cty.String), - "unsensitive": cty.NullVal(cty.String), - }), - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.NullVal(cty.String).Mark("sensitive"), - "unsensitive": cty.UnknownVal(cty.String), - "list": cty.ListVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.UnknownVal(cty.String).Mark("sensitive"), - "unsensitive": cty.UnknownVal(cty.String), - }), - cty.ObjectVal(map[string]cty.Value{ - "sensitive": cty.NullVal(cty.String).Mark("sensitive"), - "unsensitive": cty.NullVal(cty.String), - }), - }), - }), - }, - } { - t.Run(fmt.Sprintf("%#v", tc.given), func(t *testing.T) { - got := tc.given.MarkWithPaths(getValMarks(schema, tc.given, nil)) - if !got.RawEquals(tc.expect) { - t.Fatalf("\nexpected: %#v\ngot: %#v\n", tc.expect, got) - } - }) - } -}