From 57dad8d48d2910ab3ff68bc3157f77988a00b044 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 24 Jan 2024 08:52:54 -0500 Subject: [PATCH 1/4] apply schema marks to returned instance values The original sensitivity handling implementation applied the marks from a resource schema only when decoding values for evaluation. This appeared to work in most cases, since the resource value could only be used elsewhere in the configuration by references that would be returned from the evaluator. However, this approach left holes in the handling of the instance values where some marks may have been inserted from its own references to sensitive value, but the instance's own sensitive attributes were left unmarked. This was most noticeable in the state storage, where only sensitive references were listed in `sensitive_attributes`, leaving the actual sensitive attributes unmarked. The same problems would arise in the plan artifacts too, with the change `After` values missing attributes that should be sensitive, and the correction to the sensitive attributes depending on being decoded again during evaluation. Rather than depending on a value to be round-tripped through an encoding/decoding cycle via evaluation in order to be correctly marked, we need to apply the schema marks to all new values returned by a provider. This is added to the `ReadResource`, and `PlanResourceChange` paths to ensure values are consistently marked. We can then remove the re-insertion of the schema marks from the evaluator, because the value it sees should be correctly marked in all cases. Notably we don't need to do this for `Import`, because that value is never stored or referenced, -- an `ImportResource` call always consists of a followup `ReadResource` call, and there is no need to strip and re-apply the same marks. We also don't need to separately apply the schema marks after `ApplyResourceChange`, because they are the same marks we already stored in the plan `After` value (which should always be correct now that we insert them after `PlanResourceChange`). --- internal/terraform/context_apply_test.go | 91 ++++++++++--------- internal/terraform/context_plan2_test.go | 3 +- internal/terraform/evaluate.go | 20 +--- internal/terraform/evaluate_test.go | 30 +++++- internal/terraform/marks.go | 23 +++++ .../node_resource_abstract_instance.go | 22 +++-- 6 files changed, 120 insertions(+), 69 deletions(-) diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index 81d61cebd7..a15a4b5655 100644 --- a/internal/terraform/context_apply_test.go +++ b/internal/terraform/context_apply_test.go @@ -11971,15 +11971,22 @@ resource "test_resource" "foo" { } verifySensitiveValue := func(pvms []cty.PathValueMarks) { - if len(pvms) != 1 { - t.Fatalf("expected 1 sensitive path, got %d", len(pvms)) + if len(pvms) != 2 { + t.Fatalf("expected 2 sensitive paths, got %d", len(pvms)) } - pvm := pvms[0] - if gotPath, wantPath := pvm.Path, cty.GetAttrPath("value"); !gotPath.Equals(wantPath) { - t.Errorf("wrong path\n got: %#v\nwant: %#v", gotPath, wantPath) - } - if gotMarks, wantMarks := pvm.Marks, cty.NewValueMarks(marks.Sensitive); !gotMarks.Equal(wantMarks) { - t.Errorf("wrong marks\n got: %#v\nwant: %#v", gotMarks, wantMarks) + + for _, pvm := range pvms { + switch { + case pvm.Path.Equals(cty.GetAttrPath("value")): + case pvm.Path.Equals(cty.GetAttrPath("sensitive_value")): + default: + t.Errorf("unexpected path mark: %#v", pvm) + return + } + + if want := cty.NewValueMarks(marks.Sensitive); !pvm.Marks.Equal(want) { + t.Errorf("wrong marks\n got: %#v\nwant: %#v", pvm.Marks, want) + } } } @@ -12034,15 +12041,19 @@ resource "test_resource" "baz" { } verifySensitiveValue := func(pvms []cty.PathValueMarks) { - if len(pvms) != 1 { - t.Fatalf("expected 1 sensitive path, got %d", len(pvms)) - } - pvm := pvms[0] - if gotPath, wantPath := pvm.Path, cty.GetAttrPath("value"); !gotPath.Equals(wantPath) { - t.Errorf("wrong path\n got: %#v\nwant: %#v", gotPath, wantPath) - } - if gotMarks, wantMarks := pvm.Marks, cty.NewValueMarks(marks.Sensitive); !gotMarks.Equal(wantMarks) { - t.Errorf("wrong marks\n got: %#v\nwant: %#v", gotMarks, wantMarks) + for _, pvm := range pvms { + switch { + case pvm.Path.Equals(cty.GetAttrPath("value")): + case pvm.Path.Equals(cty.GetAttrPath("sensitive_value")): + case pvm.Path.Equals(cty.GetAttrPath("nesting_single").GetAttr("sensitive_value")): + default: + t.Errorf("unexpected path mark: %#v", pvm) + return + } + + if want := cty.NewValueMarks(marks.Sensitive); !pvm.Marks.Equal(want) { + t.Errorf("wrong marks\n got: %#v\nwant: %#v", pvm.Marks, want) + } } } @@ -12120,17 +12131,22 @@ resource "test_resource" "foo" { fooState := state.ResourceInstance(addr) - if len(fooState.Current.AttrSensitivePaths) != 1 { - t.Fatalf("wrong number of sensitive paths, expected 1, got, %v", len(fooState.Current.AttrSensitivePaths)) - } - got := fooState.Current.AttrSensitivePaths[0] - want := cty.PathValueMarks{ - Path: cty.GetAttrPath("value"), - Marks: cty.NewValueMarks(marks.Sensitive), + if len(fooState.Current.AttrSensitivePaths) != 2 { + t.Fatalf("wrong number of sensitive paths, expected 2, got, %v", len(fooState.Current.AttrSensitivePaths)) } - if !got.Equal(want) { - t.Fatalf("wrong value marks; got:\n%#v\n\nwant:\n%#v\n", got, want) + for _, pvm := range fooState.Current.AttrSensitivePaths { + switch { + case pvm.Path.Equals(cty.GetAttrPath("value")): + case pvm.Path.Equals(cty.GetAttrPath("sensitive_value")): + default: + t.Errorf("unexpected path mark: %#v", pvm) + return + } + + if want := cty.NewValueMarks(marks.Sensitive); !pvm.Marks.Equal(want) { + t.Errorf("wrong marks\n got: %#v\nwant: %#v", pvm.Marks, want) + } } m2 := testModuleInline(t, map[string]string{ @@ -12145,33 +12161,22 @@ resource "test_resource" "foo" { }`, }) - ctx2 := testContext2(t, &ContextOpts{ + ctx = testContext2(t, &ContextOpts{ Providers: map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), }, }) - // NOTE: Prior to our refactoring to make the state an explicit argument - // of Plan, as opposed to hidden state inside Context, this test was - // calling ctx.Apply instead of ctx2.Apply and thus using the previous - // plan instead of this new plan. "Fixing" it to use the new plan seems - // to break the test, so we've preserved that oddity here by saving the - // old plan as oldPlan and essentially discarding the new plan entirely, - // but this seems rather suspicious and we should ideally figure out what - // this test was originally intending to do and make it do that. - oldPlan := plan - _, diags = ctx2.Plan(m2, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) + plan, diags = ctx.Plan(m2, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) assertNoErrors(t, diags) - stateWithoutSensitive, diags := ctx.Apply(oldPlan, m, nil) + stateWithoutSensitive, diags := ctx.Apply(plan, m2, nil) assertNoErrors(t, diags) fooState2 := stateWithoutSensitive.ResourceInstance(addr) - if len(fooState2.Current.AttrSensitivePaths) > 0 { - t.Fatalf( - "wrong number of sensitive paths, expected 0, got, %v\n%s", + if len(fooState2.Current.AttrSensitivePaths) != 1 { + t.Fatalf("wrong number of sensitive paths, expected 1, got, %v\n%#v\n", len(fooState2.Current.AttrSensitivePaths), - spew.Sdump(fooState2.Current.AttrSensitivePaths), - ) + fooState2.Current.AttrSensitivePaths) } } diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 81e6094242..94cafe56f7 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -2932,7 +2932,8 @@ output "output" { } if res.Addr.Resource.Resource.Mode == addrs.DataResourceMode && res.Action != plans.NoOp { - t.Errorf("unexpected %s plan for %s", res.Action, res.Addr) + fmt.Println(res.Addr, res.ActionReason) + t.Errorf("unexpected %s/%s plan for %s", res.Action, res.ActionReason, res.Addr) } } } diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index bbda671e8e..78841cd731 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -744,13 +744,9 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc continue } - // If our provider schema contains sensitive values, mark those as sensitive - afterMarks := change.AfterValMarks - if schema.ContainsSensitive() { - afterMarks = append(afterMarks, schema.ValueMarks(val, nil)...) - } - - instances[key] = val.MarkWithPaths(afterMarks) + // Unlike decoding state, decoding a change does not automatically + // mark values. + instances[key] = val.MarkWithPaths(change.AfterValMarks) continue } @@ -769,16 +765,6 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc val := ios.Value - // If our schema contains sensitive values, mark those as sensitive. - // Since decoding the instance object can also apply sensitivity marks, - // we must remove and combine those before remarking to avoid a double- - // mark error. - if schema.ContainsSensitive() { - var marks []cty.PathValueMarks - val, marks = val.UnmarkDeepWithPaths() - marks = append(marks, schema.ValueMarks(val, nil)...) - val = val.MarkWithPaths(marks) - } instances[key] = val } diff --git a/internal/terraform/evaluate_test.go b/internal/terraform/evaluate_test.go index b1e0283523..bf59c68f4f 100644 --- a/internal/terraform/evaluate_test.go +++ b/internal/terraform/evaluate_test.go @@ -235,6 +235,32 @@ func TestEvaluatorGetResource(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo", "nesting_list": [{"sensitive_value":"abc"}], "nesting_map": {"foo":{"foo":"x"}}, "nesting_set": [{"baz":"abc"}], "nesting_single": {"boop":"abc"}, "nesting_nesting": {"nesting_list":[{"sensitive_value":"abc"}]}, "value":"hello"}`), + AttrSensitivePaths: []cty.PathValueMarks{ + { + Path: cty.GetAttrPath("nesting_list").IndexInt(0).GetAttr("sensitive_value"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_map").IndexString("foo").GetAttr("foo"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_nesting").GetAttr("nesting_list").IndexInt(0).GetAttr("sensitive_value"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_set"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_single").GetAttr("boop"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("value"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + }, }, addrs.AbsProviderConfig{ Provider: addrs.NewDefaultProvider("test"), @@ -433,10 +459,10 @@ func TestEvaluatorGetResource_changes(t *testing.T) { After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("foo"), "to_mark_val": cty.StringVal("pizza").Mark(marks.Sensitive), - "sensitive_value": cty.StringVal("abc"), + "sensitive_value": cty.StringVal("abc").Mark(marks.Sensitive), "sensitive_collection": cty.MapVal(map[string]cty.Value{ "boop": cty.StringVal("beep"), - }), + }).Mark(marks.Sensitive), }), }, } diff --git a/internal/terraform/marks.go b/internal/terraform/marks.go index 7b0d168123..e57c4745fa 100644 --- a/internal/terraform/marks.go +++ b/internal/terraform/marks.go @@ -56,3 +56,26 @@ func marksEqual(a, b []cty.PathValueMarks) bool { return true } + +// Remove duplicate PathValueMarks from the slice. +// When adding marks from a resource schema to a value, most of the time there +// will be duplicates from a prior value already in the list of marks. While +// MarkwithPaths will accept duplicates, we need to be able to easily compare +// the PathValueMarks within this package too. +func dedupePathValueMarks(m []cty.PathValueMarks) []cty.PathValueMarks { + var res []cty.PathValueMarks + // we'll use a GoString format key to differentiate PathValueMarks, since + // the Path portion is not automagically comparable. + seen := make(map[string]bool) + + for _, pvm := range m { + key := fmt.Sprintf("%#v", pvm) + if _, ok := seen[key]; ok { + continue + } + seen[key] = true + res = append(res, pvm) + } + + return res +} diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 960da8599e..2cc42a4f8c 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -620,9 +620,9 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state priorVal := state.Value // Unmarked before sending to provider - var priorPaths []cty.PathValueMarks + var priorMarks []cty.PathValueMarks if priorVal.ContainsMarked() { - priorVal, priorPaths = priorVal.UnmarkDeepWithPaths() + priorVal, priorMarks = priorVal.UnmarkDeepWithPaths() } var resp providers.ReadResourceResponse @@ -714,9 +714,14 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state return ret, diags } - // Mark the value if necessary - if len(priorPaths) > 0 { - ret.Value = ret.Value.MarkWithPaths(priorPaths) + // Mark the value with any prior marks from the state, and the marks from + // the schema. This ensures we capture any marks from the last + // configuration, as well as any marks from the schema which were not in + // the prior state. New marks may appear when the prior state was from an + // import operation, or if the provider added new marks to the schema. + if marks := dedupePathValueMarks(append(priorMarks, schema.ValueMarks(ret.Value, nil)...)); len(marks) > 0 { + //if marks := priorMarks; len(marks) > 0 { + ret.Value = ret.Value.MarkWithPaths(marks) } return ret, diags @@ -993,8 +998,13 @@ func (n *NodeAbstractResourceInstance) plan( } // Add the marks back to the planned new value -- this must happen after ignore changes - // have been processed + // have been processed. We add in the schema marks as well, so ensure that + // provider defined private attributes are marked correctly here. unmarkedPlannedNewVal := plannedNewVal + // Add schema path marks to unmarkedPaths, so that any difference in + // sensitive attributes from the provider are marked correctly too. + unmarkedPaths = dedupePathValueMarks(append(unmarkedPaths, schema.ValueMarks(plannedNewVal, nil)...)) + if len(unmarkedPaths) > 0 { plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) } From 15ead7bce72fe03455cfa1d2ae01e1a113a76cbc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 24 Jan 2024 13:17:58 -0500 Subject: [PATCH 2/4] comment/debuging cleanup --- internal/terraform/context_plan2_test.go | 1 - internal/terraform/node_resource_abstract_instance.go | 9 ++++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 94cafe56f7..1109e9c6d8 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -2932,7 +2932,6 @@ output "output" { } if res.Addr.Resource.Resource.Mode == addrs.DataResourceMode && res.Action != plans.NoOp { - fmt.Println(res.Addr, res.ActionReason) t.Errorf("unexpected %s/%s plan for %s", res.Action, res.ActionReason, res.Addr) } } diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 2cc42a4f8c..696645a91b 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -997,12 +997,11 @@ func (n *NodeAbstractResourceInstance) plan( } } - // Add the marks back to the planned new value -- this must happen after ignore changes - // have been processed. We add in the schema marks as well, so ensure that - // provider defined private attributes are marked correctly here. + // Add the marks back to the planned new value -- this must happen after + // ignore changes have been processed. We add in the schema marks as well, + // to ensure that provider defined private attributes are marked correctly + // here. unmarkedPlannedNewVal := plannedNewVal - // Add schema path marks to unmarkedPaths, so that any difference in - // sensitive attributes from the provider are marked correctly too. unmarkedPaths = dedupePathValueMarks(append(unmarkedPaths, schema.ValueMarks(plannedNewVal, nil)...)) if len(unmarkedPaths) > 0 { From 8655b08b19988898443a16c160d43f63a3ed78b4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 24 Jan 2024 14:17:48 -0500 Subject: [PATCH 3/4] fix sensitive_attributes in test Add the missing sensitive_attributes to the test state, and update the output to reflect the new `"checks"` attribute name. --- .../show-json/conditions/for-refresh.tfstate | 80 ++++++++++------- .../conditions/output-refresh-only.json | 86 ++++++++++++++----- 2 files changed, 113 insertions(+), 53 deletions(-) diff --git a/internal/command/testdata/show-json/conditions/for-refresh.tfstate b/internal/command/testdata/show-json/conditions/for-refresh.tfstate index 1b0b10196f..ec436cef0b 100644 --- a/internal/command/testdata/show-json/conditions/for-refresh.tfstate +++ b/internal/command/testdata/show-json/conditions/for-refresh.tfstate @@ -1,39 +1,55 @@ { - "version": 4, - "terraform_version": "1.2.0-dev", - "serial": 1, - "lineage": "no", - "outputs": {}, - "resources": [ + "version": 4, + "terraform_version": "1.2.0-dev", + "serial": 1, + "lineage": "no", + "outputs": {}, + "resources": [ + { + "mode": "managed", + "type": "test_instance", + "name": "foo", + "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", + "instances": [ { - "mode": "managed", - "type": "test_instance", - "name": "foo", - "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", - "instances": [ - { - "schema_version": 0, - "attributes": { - "ami": "ami-test", - "id": "placeholder" - } - } + "schema_version": 0, + "attributes": { + "ami": "ami-test", + "id": "placeholder" + }, + "sensitive_attributes": [ + [ + { + "type": "get_attr", + "value": "password" + } ] - }, + ] + } + ] + }, + { + "mode": "managed", + "type": "test_instance", + "name": "bar", + "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", + "instances": [ { - "mode": "managed", - "type": "test_instance", - "name": "bar", - "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", - "instances": [ - { - "schema_version": 0, - "attributes": { - "ami": "ami-test", - "id": "placeheld" - } - } + "schema_version": 0, + "attributes": { + "ami": "ami-test", + "id": "placeheld" + }, + "sensitive_attributes": [ + [ + { + "type": "get_attr", + "value": "password" + } ] + ] } - ] + ] + } + ] } diff --git a/internal/command/testdata/show-json/conditions/output-refresh-only.json b/internal/command/testdata/show-json/conditions/output-refresh-only.json index 7b1845f1d7..75452839f3 100644 --- a/internal/command/testdata/show-json/conditions/output-refresh-only.json +++ b/internal/command/testdata/show-json/conditions/output-refresh-only.json @@ -1,6 +1,6 @@ { - "format_version": "1.1", - "terraform_version": "1.2.0-dev", + "format_version": "1.2", + "terraform_version": "1.8.0-dev", "variables": { "ami": { "value": "bad-ami" @@ -33,13 +33,13 @@ }, "prior_state": { "format_version": "1.0", - "terraform_version": "1.1.0", + "terraform_version": "1.8.0", "values": { "outputs": { "foo_id": { "sensitive": false, - "type": "string", - "value": "placeholder" + "value": "placeholder", + "type": "string" } }, "root_module": { @@ -146,26 +146,70 @@ ] } ], - "condition_results": [ + "checks": [ { - "address": "output.foo_id", - "condition_type": "OutputPrecondition", - "result": true, - "unknown": false + "address": { + "kind": "output_value", + "name": "foo_id", + "to_display": "output.foo_id" + }, + "status": "pass", + "instances": [ + { + "address": { + "to_display": "output.foo_id" + }, + "status": "pass" + } + ] }, { - "address": "test_instance.bar", - "condition_type": "ResourcePostcondition", - "result": false, - "unknown": false, - "error_message": "Resource ID is unacceptably short (9 characters)." + "address": { + "kind": "resource", + "mode": "managed", + "name": "bar", + "to_display": "test_instance.bar", + "type": "test_instance" + }, + "status": "fail", + "instances": [ + { + "address": { + "to_display": "test_instance.bar" + }, + "status": "fail", + "problems": [ + { + "message": "Resource ID is unacceptably short (9 characters)." + } + ] + } + ] }, { - "address": "test_instance.foo", - "condition_type": "ResourcePrecondition", - "result": false, - "unknown": false, - "error_message": "Invalid AMI ID: must start with \"ami-\"." + "address": { + "kind": "resource", + "mode": "managed", + "name": "foo", + "to_display": "test_instance.foo", + "type": "test_instance" + }, + "status": "fail", + "instances": [ + { + "address": { + "to_display": "test_instance.foo" + }, + "status": "fail", + "problems": [ + { + "message": "Invalid AMI ID: must start with \"ami-\"." + } + ] + } + ] } - ] + ], + "timestamp": "2024-01-24T18:33:05Z", + "errored": false } From 8994e914b7924d78b12b44e5043136dfbd729a67 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 24 Jan 2024 14:21:29 -0500 Subject: [PATCH 4/4] add sensitive attributes to test json output --- .../command/testdata/show-json-sensitive/output.json | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/command/testdata/show-json-sensitive/output.json b/internal/command/testdata/show-json-sensitive/output.json index 156b12f3e3..db98e521b6 100644 --- a/internal/command/testdata/show-json-sensitive/output.json +++ b/internal/command/testdata/show-json-sensitive/output.json @@ -28,7 +28,8 @@ "password": "secret" }, "sensitive_values": { - "ami": true + "ami": true, + "password": true } }, { @@ -44,7 +45,8 @@ "password": "secret" }, "sensitive_values": { - "ami": true + "ami": true, + "password": true } }, { @@ -60,7 +62,8 @@ "password": "secret" }, "sensitive_values": { - "ami": true + "ami": true, + "password": true } } ]