From 0a02e7040f42e6e6fd9dbb432d4cd65c3821f1bf Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Thu, 24 Sep 2020 12:40:17 -0400 Subject: [PATCH] Store sensitive attribute paths in state (#26338) * Add creation test and simplify in-place test * Add deletion test * Start adding marking from state Start storing paths that should be marked when pulled out of state. Implements deep copy for attr paths. This commit also includes some comment noise from investigations, and fixing the diff test * Fix apply stripping marks * Expand diff tests * Basic apply test * Update comments on equality checks to clarify current understanding * Add JSON serialization for sensitive paths We need to serialize a slice of cty.Path values to be used to re-mark the sensitive values of a resource instance when loading the state file. Paths consist of a list of steps, each of which may be either getting an attribute value by name, or indexing into a collection by string or number. To serialize these without building a complex parser for a compact string form, we render a nested array of small objects, like so: [ [ { type: "get_attr", value: "foo" }, { type: "index", value: { "type": "number", "value": 2 } } ] ] The above example is equivalent to a path `foo[2]`. * Format diffs with map types Comparisons need unmarked values to operate on, so create unmarked values for those operations. Additionally, change diff to cover map types * Remove debugging printing * Fix bug with marking non-sensitive values When pulling a sensitive value from state, we were previously using those marks to remark the planned new value, but that new value might *not* be sensitive, so let's not do that * Fix apply test Apply was not passing the second state through to the third pass at apply * Consistency in checking for length of paths vs inspecting into value * In apply, don't mark with before paths * AttrPaths test coverage for DeepCopy * Revert format changes Reverts format changes in format/diff for this branch so those changes can be discussed on a separate PR * Refactor name of AttrPaths to AttrSensitivePaths * Rename AttributePaths/attributePaths for naming consistency Co-authored-by: Alisdair McDiarmid --- command/format/diff_test.go | 165 ++++++++++++++++++----- providers/provider.go | 2 +- states/instance_object.go | 6 +- states/instance_object_src.go | 8 ++ states/state_deepcopy.go | 7 + states/state_test.go | 9 +- states/statefile/version4.go | 187 +++++++++++++++++++++++-- states/statefile/version4_test.go | 217 ++++++++++++++++++++++++++++++ terraform/context_apply_test.go | 79 +++++++++++ terraform/eval_apply.go | 18 ++- terraform/eval_diff.go | 36 +++-- terraform/eval_refresh.go | 12 ++ 12 files changed, 686 insertions(+), 60 deletions(-) diff --git a/command/format/diff_test.go b/command/format/diff_test.go index b885791a3..d333315d1 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -3624,24 +3624,13 @@ func TestResourceChange_nestedMap(t *testing.T) { func TestResourceChange_sensitiveVariable(t *testing.T) { testCases := map[string]testCase{ - "in-place update - creation": { - Action: plans.Update, + "creation": { + Action: plans.Create, 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, - })), - }), + Before: cty.NullVal(cty.EmptyObject), 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"), - }), - }), + "ami": cty.StringVal("ami-123"), }), AfterValMarks: []cty.PathValueMarks{ { @@ -3655,29 +3644,141 @@ func TestResourceChange_sensitiveVariable(t *testing.T) { "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 created + + resource "test_instance" "example" { + + ami = (sensitive) + + id = "i-02ae66f368e8518a9" + } +`, + }, + "in-place update - before sensitive": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + }), + BeforeValMarks: []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}, }, }, 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" - } + } +`, + }, + "in-place update - after sensitive, map type": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "tags": cty.MapVal(map[string]cty.Value{ + "name": cty.StringVal("anna"), + }), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "tags": cty.MapVal(map[string]cty.Value{ + "name": cty.StringVal("bob"), + }), + }), + AfterValMarks: []cty.PathValueMarks{ + { + Path: cty.Path{cty.GetAttrStep{Name: "tags"}, cty.IndexStep{Key: cty.StringVal("name")}}, + 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}, + "tags": {Type: cty.Map(cty.String), Optional: true}, + }, + }, + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + id = "i-02ae66f368e8518a9" + ~ tags = (sensitive) + } +`, + }, + "in-place update - both sensitive": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + }), + BeforeValMarks: []cty.PathValueMarks{ + { + Path: cty.Path{cty.GetAttrStep{Name: "ami"}}, + Marks: cty.NewValueMarks("sensitive"), + }}, + 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}, + }, + }, + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + ~ ami = (sensitive) + id = "i-02ae66f368e8518a9" + } +`, + }, + "deletion": { + Action: plans.Delete, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + }), + After: cty.NullVal(cty.EmptyObject), + BeforeValMarks: []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}, + }, + }, + ExpectedOutput: ` # test_instance.example will be destroyed + - resource "test_instance" "example" { + - ami = (sensitive) + - id = "i-02ae66f368e8518a9" -> null } `, }, diff --git a/providers/provider.go b/providers/provider.go index b27e3dbce..2d9db44bb 100644 --- a/providers/provider.go +++ b/providers/provider.go @@ -238,7 +238,7 @@ type PlanResourceChangeResponse struct { // configuration is applied. PlannedState cty.Value - // RequiresReplace is the list of thee attributes that are requiring + // RequiresReplace is the list of the attributes that are requiring // resource replacement. RequiresReplace []cty.Path diff --git a/states/instance_object.go b/states/instance_object.go index 2a9e58327..a9ebf4804 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -98,10 +98,11 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res // and raise an error about that. val := cty.UnknownAsNull(o.Value) - // If it contains marks, dump those now + // If it contains marks, save these in state unmarked := val + var pvm []cty.PathValueMarks if val.ContainsMarked() { - unmarked, _ = val.UnmarkDeep() + unmarked, pvm = val.UnmarkDeepWithPaths() } src, err := ctyjson.Marshal(unmarked, ty) if err != nil { @@ -111,6 +112,7 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res return &ResourceInstanceObjectSrc{ SchemaVersion: schemaVersion, AttrsJSON: src, + AttrSensitivePaths: pvm, Private: o.Private, Status: o.Status, Dependencies: o.Dependencies, diff --git a/states/instance_object_src.go b/states/instance_object_src.go index bf790db04..aeb612eaa 100644 --- a/states/instance_object_src.go +++ b/states/instance_object_src.go @@ -49,6 +49,10 @@ type ResourceInstanceObjectSrc struct { // the recommendations in the AttrsJSON documentation above. AttrsFlat map[string]string + // AttrSensitivePaths is an array of paths to mark as sensitive coming out of + // state, or to save as sensitive paths when saving state + AttrSensitivePaths []cty.PathValueMarks + // These fields all correspond to the fields of the same name on // ResourceInstanceObject. Private []byte @@ -78,6 +82,10 @@ func (os *ResourceInstanceObjectSrc) Decode(ty cty.Type) (*ResourceInstanceObjec } } else { val, err = ctyjson.Unmarshal(os.AttrsJSON, ty) + // Mark the value with paths if applicable + if os.AttrSensitivePaths != nil { + val = val.MarkWithPaths(os.AttrSensitivePaths) + } if err != nil { return nil, err } diff --git a/states/state_deepcopy.go b/states/state_deepcopy.go index f6a3919c3..93e96d756 100644 --- a/states/state_deepcopy.go +++ b/states/state_deepcopy.go @@ -144,6 +144,12 @@ func (obj *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { copy(attrsJSON, obj.AttrsJSON) } + var attrPaths []cty.PathValueMarks + if obj.AttrSensitivePaths != nil { + attrPaths = make([]cty.PathValueMarks, len(obj.AttrSensitivePaths)) + copy(attrPaths, obj.AttrSensitivePaths) + } + var private []byte if obj.Private != nil { private = make([]byte, len(obj.Private)) @@ -164,6 +170,7 @@ func (obj *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { Private: private, AttrsFlat: attrsFlat, AttrsJSON: attrsJSON, + AttrSensitivePaths: attrPaths, Dependencies: dependencies, CreateBeforeDestroy: obj.CreateBeforeDestroy, } diff --git a/states/state_test.go b/states/state_test.go index 9f37ec3a1..6b3eb815e 100644 --- a/states/state_test.go +++ b/states/state_test.go @@ -230,7 +230,14 @@ func TestStateDeepCopy(t *testing.T) { Status: ObjectReady, SchemaVersion: 1, AttrsJSON: []byte(`{"woozles":"confuzles"}`), - Private: []byte("private data"), + // Sensitive path at "woozles" + AttrSensitivePaths: []cty.PathValueMarks{ + { + Path: cty.Path{cty.GetAttrStep{Name: "woozles"}}, + Marks: cty.NewValueMarks("sensitive"), + }, + }, + Private: []byte("private data"), Dependencies: []addrs.ConfigResource{ { Module: addrs.RootModule, diff --git a/states/statefile/version4.go b/states/statefile/version4.go index 450062a5a..8d32eb7ad 100644 --- a/states/statefile/version4.go +++ b/states/statefile/version4.go @@ -7,6 +7,7 @@ import ( "sort" version "github.com/hashicorp/go-version" + "github.com/zclconf/go-cty/cty" ctyjson "github.com/zclconf/go-cty/cty/json" "github.com/hashicorp/terraform/addrs" @@ -151,6 +152,24 @@ func prepareStateV4(sV4 *stateV4) (*File, tfdiags.Diagnostics) { } } + // Sensitive paths + if isV4.AttributeSensitivePaths != nil { + paths, pathsDiags := unmarshalPaths([]byte(isV4.AttributeSensitivePaths)) + diags = diags.Append(pathsDiags) + if pathsDiags.HasErrors() { + continue + } + + var pvm []cty.PathValueMarks + for _, path := range paths { + pvm = append(pvm, cty.PathValueMarks{ + Path: path, + Marks: cty.NewValueMarks("sensitive"), + }) + } + obj.AttrSensitivePaths = pvm + } + { // Status raw := isV4.Status @@ -452,16 +471,27 @@ func appendInstanceObjectStateV4(rs *states.Resource, is *states.ResourceInstanc } } + // Extract paths from path value marks + var paths []cty.Path + for _, vm := range obj.AttrSensitivePaths { + paths = append(paths, vm.Path) + } + + // Marshal paths to JSON + attributeSensitivePaths, pathsDiags := marshalPaths(paths) + diags = diags.Append(pathsDiags) + return append(isV4s, instanceObjectStateV4{ - IndexKey: rawKey, - Deposed: string(deposed), - Status: status, - SchemaVersion: obj.SchemaVersion, - AttributesFlat: obj.AttrsFlat, - AttributesRaw: obj.AttrsJSON, - PrivateRaw: privateRaw, - Dependencies: deps, - CreateBeforeDestroy: obj.CreateBeforeDestroy, + IndexKey: rawKey, + Deposed: string(deposed), + Status: status, + SchemaVersion: obj.SchemaVersion, + AttributesFlat: obj.AttrsFlat, + AttributesRaw: obj.AttrsJSON, + AttributeSensitivePaths: attributeSensitivePaths, + PrivateRaw: privateRaw, + Dependencies: deps, + CreateBeforeDestroy: obj.CreateBeforeDestroy, }), diags } @@ -505,9 +535,10 @@ type instanceObjectStateV4 struct { Status string `json:"status,omitempty"` Deposed string `json:"deposed,omitempty"` - SchemaVersion uint64 `json:"schema_version"` - AttributesRaw json.RawMessage `json:"attributes,omitempty"` - AttributesFlat map[string]string `json:"attributes_flat,omitempty"` + SchemaVersion uint64 `json:"schema_version"` + AttributesRaw json.RawMessage `json:"attributes,omitempty"` + AttributesFlat map[string]string `json:"attributes_flat,omitempty"` + AttributeSensitivePaths json.RawMessage `json:"sensitive_attributes,omitempty,"` PrivateRaw []byte `json:"private,omitempty"` @@ -577,3 +608,135 @@ func (si sortInstancesV4) Less(i, j int) bool { } return false } + +// pathStep is an intermediate representation of a cty.PathStep to facilitate +// consistent JSON serialization. The Value field can either be a cty.Value of +// dynamic type (for index steps), or a string (for get attr steps). +type pathStep struct { + Type string `json:"type"` + Value json.RawMessage `json:"value"` +} + +const ( + indexPathStepType = "index" + getAttrPathStepType = "get_attr" +) + +func unmarshalPaths(buf []byte) ([]cty.Path, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + var jsonPaths [][]pathStep + + err := json.Unmarshal(buf, &jsonPaths) + if err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Error unmarshaling path steps", + err.Error(), + )) + } + + paths := make([]cty.Path, 0, len(jsonPaths)) + +unmarshalOuter: + for _, jsonPath := range jsonPaths { + var path cty.Path + for _, jsonStep := range jsonPath { + switch jsonStep.Type { + case indexPathStepType: + key, err := ctyjson.Unmarshal(jsonStep.Value, cty.DynamicPseudoType) + if err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Error unmarshaling path step", + fmt.Sprintf("Failed to unmarshal index step key: %s", err), + )) + continue unmarshalOuter + } + path = append(path, cty.IndexStep{Key: key}) + case getAttrPathStepType: + var name string + if err := json.Unmarshal(jsonStep.Value, &name); err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Error unmarshaling path step", + fmt.Sprintf("Failed to unmarshal get attr step name: %s", err), + )) + continue unmarshalOuter + } + path = append(path, cty.GetAttrStep{Name: name}) + default: + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Unsupported path step", + fmt.Sprintf("Unsupported path step type %q", jsonStep.Type), + )) + continue unmarshalOuter + } + } + paths = append(paths, path) + } + + return paths, diags +} + +func marshalPaths(paths []cty.Path) ([]byte, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + + // cty.Path is a slice of cty.PathSteps, so our representation of a slice + // of paths is a nested slice of our intermediate pathStep struct + jsonPaths := make([][]pathStep, 0, len(paths)) + +marshalOuter: + for _, path := range paths { + jsonPath := make([]pathStep, 0, len(path)) + for _, step := range path { + var jsonStep pathStep + switch s := step.(type) { + case cty.IndexStep: + key, err := ctyjson.Marshal(s.Key, cty.DynamicPseudoType) + if err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Error marshaling path step", + fmt.Sprintf("Failed to marshal index step key %#v: %s", s.Key, err), + )) + continue marshalOuter + } + jsonStep.Type = indexPathStepType + jsonStep.Value = key + case cty.GetAttrStep: + name, err := json.Marshal(s.Name) + if err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Error marshaling path step", + fmt.Sprintf("Failed to marshal get attr step name %s: %s", s.Name, err), + )) + continue marshalOuter + } + jsonStep.Type = getAttrPathStepType + jsonStep.Value = name + default: + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Unsupported path step", + fmt.Sprintf("Unsupported path step %#v (%t)", step, step), + )) + continue marshalOuter + } + jsonPath = append(jsonPath, jsonStep) + } + jsonPaths = append(jsonPaths, jsonPath) + } + + buf, err := json.Marshal(jsonPaths) + if err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Error marshaling path steps", + fmt.Sprintf("Failed to marshal path steps: %s", err), + )) + } + + return buf, diags +} diff --git a/states/statefile/version4_test.go b/states/statefile/version4_test.go index c59ad5c28..f097a6069 100644 --- a/states/statefile/version4_test.go +++ b/states/statefile/version4_test.go @@ -2,7 +2,11 @@ package statefile import ( "sort" + "strings" "testing" + + "github.com/hashicorp/terraform/tfdiags" + "github.com/zclconf/go-cty/cty" ) // This test verifies that modules are sorted before resources: @@ -39,3 +43,216 @@ func TestVersion4_sort(t *testing.T) { } } } + +func TestVersion4_unmarshalPaths(t *testing.T) { + testCases := map[string]struct { + json string + paths []cty.Path + diags []string + }{ + "no paths": { + json: `[]`, + paths: []cty.Path{}, + }, + "attribute path": { + json: `[ + [ + { + "type": "get_attr", + "value": "password" + } + ] +]`, + paths: []cty.Path{cty.GetAttrPath("password")}, + }, + "attribute and string index": { + json: `[ + [ + { + "type": "get_attr", + "value": "triggers" + }, + { + "type": "index", + "value": { + "value": "secret", + "type": "string" + } + } + ] +]`, + paths: []cty.Path{cty.GetAttrPath("triggers").IndexString("secret")}, + }, + "attribute, number index, attribute": { + json: `[ + [ + { + "type": "get_attr", + "value": "identities" + }, + { + "type": "index", + "value": { + "value": 2, + "type": "number" + } + }, + { + "type": "get_attr", + "value": "private_key" + } + ] +]`, + paths: []cty.Path{cty.GetAttrPath("identities").IndexInt(2).GetAttr("private_key")}, + }, + "multiple paths": { + json: `[ + [ + { + "type": "get_attr", + "value": "alpha" + } + ], + [ + { + "type": "get_attr", + "value": "beta" + } + ], + [ + { + "type": "get_attr", + "value": "gamma" + } + ] +]`, + paths: []cty.Path{cty.GetAttrPath("alpha"), cty.GetAttrPath("beta"), cty.GetAttrPath("gamma")}, + }, + "errors": { + json: `[ + [ + { + "type": "get_attr", + "value": 5 + } + ], + [ + { + "type": "index", + "value": "test" + } + ], + [ + { + "type": "invalid_type", + "value": ["this is invalid too"] + } + ] +]`, + paths: []cty.Path{}, + diags: []string{ + "Failed to unmarshal get attr step name", + "Failed to unmarshal index step key", + "Unsupported path step", + }, + }, + "one invalid path, others valid": { + json: `[ + [ + { + "type": "get_attr", + "value": "alpha" + } + ], + [ + { + "type": "invalid_type", + "value": ["this is invalid too"] + } + ], + [ + { + "type": "get_attr", + "value": "gamma" + } + ] +]`, + paths: []cty.Path{cty.GetAttrPath("alpha"), cty.GetAttrPath("gamma")}, + diags: []string{"Unsupported path step"}, + }, + "invalid structure": { + json: `{}`, + paths: []cty.Path{}, + diags: []string{"Error unmarshaling path steps"}, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + paths, diags := unmarshalPaths([]byte(tc.json)) + + if len(tc.diags) == 0 { + if len(diags) != 0 { + t.Errorf("expected no diags, got: %#v", diags) + } + } else { + if got, want := len(diags), len(tc.diags); got != want { + t.Fatalf("got %d diags, want %d\n%s", got, want, diags.Err()) + } + for i := range tc.diags { + got := tfdiags.Diagnostics{diags[i]}.Err().Error() + if !strings.Contains(got, tc.diags[i]) { + t.Errorf("expected diag %d to contain %q, but was:\n%s", i, tc.diags[i], got) + } + } + } + + if len(paths) != len(tc.paths) { + t.Fatalf("got %d paths, want %d", len(paths), len(tc.paths)) + } + for i, path := range paths { + if !path.Equals(tc.paths[i]) { + t.Errorf("wrong paths\n got: %#v\nwant: %#v", path, tc.paths[i]) + } + } + }) + } +} + +func TestVersion4_marshalPaths(t *testing.T) { + testCases := map[string]struct { + paths []cty.Path + json string + }{ + "no paths": { + paths: []cty.Path{}, + json: `[]`, + }, + "attribute path": { + paths: []cty.Path{cty.GetAttrPath("password")}, + json: `[[{"type":"get_attr","value":"password"}]]`, + }, + "attribute, number index, attribute": { + paths: []cty.Path{cty.GetAttrPath("identities").IndexInt(2).GetAttr("private_key")}, + json: `[[{"type":"get_attr","value":"identities"},{"type":"index","value":{"value":2,"type":"number"}},{"type":"get_attr","value":"private_key"}]]`, + }, + "multiple paths": { + paths: []cty.Path{cty.GetAttrPath("a"), cty.GetAttrPath("b"), cty.GetAttrPath("c")}, + json: `[[{"type":"get_attr","value":"a"}],[{"type":"get_attr","value":"b"}],[{"type":"get_attr","value":"c"}]]`, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + json, diags := marshalPaths(tc.paths) + + if len(diags) != 0 { + t.Fatalf("expected no diags, got: %#v", diags) + } + + if got, want := string(json), tc.json; got != want { + t.Fatalf("wrong JSON output\n got: %s\nwant: %s\n", got, want) + } + }) + } +} diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 182b678b8..255cb91c8 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11916,3 +11916,82 @@ resource "test_resource" "c" { t.Fatalf("apply errors: %s", diags.Err()) } } + +func TestContext2Apply_variableSensitivity(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [sensitive_variables] +} + +variable "sensitive_var" { + default = "foo" + sensitive = true +} + +resource "test_resource" "foo" { + value = var.sensitive_var +}`, + }) + + p := testProvider("test") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags := ctx.Apply() + if diags.HasErrors() { + t.Fatalf("apply errors: %s", diags.Err()) + } + + // Run a second apply with no changes + ctx = testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + State: state, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatalf("apply errors: %s", diags.Err()) + } + + // Now change the variable value + ctx = testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + Variables: InputValues{ + "sensitive_var": &InputValue{ + Value: cty.StringVal("bar"), + }, + }, + State: state, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatalf("apply errors: %s", diags.Err()) + } +} diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 1a2eef0bd..90040c84c 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -105,21 +105,28 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr.Absolute(ctx.Path()), change.Action) - // If our config or After value contain any marked values, + // If our config, Before 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() } + + unmarkedBefore := change.Before + if change.Before.ContainsMarked() { + unmarkedBefore, _ = change.Before.UnmarkDeep() + } + unmarkedAfter := change.After + var afterPaths []cty.PathValueMarks if change.After.ContainsMarked() { - unmarkedAfter, _ = change.After.UnmarkDeep() + unmarkedAfter, afterPaths = change.After.UnmarkDeepWithPaths() } resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ TypeName: n.Addr.Resource.Type, - PriorState: change.Before, + PriorState: unmarkedBefore, Config: unmarkedConfigVal, PlannedState: unmarkedAfter, PlannedPrivate: change.Private, @@ -138,6 +145,11 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { // incomplete. newVal := resp.NewState + // If we have paths to mark, mark those on this new value + if len(afterPaths) > 0 { + newVal = newVal.MarkWithPaths(afterPaths) + } + 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 diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 366e5ddd2..f8fa83a96 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -213,7 +213,14 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { priorVal = cty.NullVal(schema.ImpliedType()) } - proposedNewVal := objchange.ProposedNewObject(schema, priorVal, unmarkedConfigVal) + unmarkedPriorVal := priorVal + if priorVal.ContainsMarked() { + // store the marked values so we can re-mark them later after + // we've sent things over the wire. + unmarkedPriorVal, _ = priorVal.UnmarkDeep() + } + + proposedNewVal := objchange.ProposedNewObject(schema, unmarkedPriorVal, unmarkedConfigVal) // Call pre-diff hook if !n.Stub { @@ -244,7 +251,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // we send back this information, we need to process ignore_changes // so that CustomizeDiff will not act on them var ignoreChangeDiags tfdiags.Diagnostics - proposedNewVal, ignoreChangeDiags = n.processIgnoreChanges(priorVal, proposedNewVal) + proposedNewVal, ignoreChangeDiags = n.processIgnoreChanges(unmarkedPriorVal, proposedNewVal) diags = diags.Append(ignoreChangeDiags) if ignoreChangeDiags.HasErrors() { return nil, diags.Err() @@ -253,7 +260,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { resp := provider.PlanResourceChange(providers.PlanResourceChangeRequest{ TypeName: n.Addr.Resource.Type, Config: unmarkedConfigVal, - PriorState: priorVal, + PriorState: unmarkedPriorVal, ProposedNewState: proposedNewVal, PriorPrivate: priorPrivate, ProviderMeta: metaConfigVal, @@ -274,7 +281,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } // Add the marks back to the planned new value - if configVal.ContainsMarked() { + if len(unmarkedPaths) > 0 { plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) } @@ -352,7 +359,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { continue } - priorChangedVal, priorPathDiags := hcl.ApplyPath(priorVal, path, nil) + priorChangedVal, priorPathDiags := hcl.ApplyPath(unmarkedPriorVal, path, nil) plannedChangedVal, plannedPathDiags := hcl.ApplyPath(plannedNewVal, path, nil) if plannedPathDiags.HasErrors() && priorPathDiags.HasErrors() { // This means the path was invalid in both the prior and new @@ -384,7 +391,10 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { plannedChangedVal = cty.NullVal(priorChangedVal.Type()) } - eqV := plannedChangedVal.Equals(priorChangedVal) + // Unmark for this value for the equality test. If only sensitivity has changed, + // this does not require an Update or Replace + unmarkedPlannedChangedVal, _ := plannedChangedVal.UnmarkDeep() + eqV := unmarkedPlannedChangedVal.Equals(priorChangedVal) if !eqV.IsKnown() || eqV.False() { reqRep.Add(path) } @@ -394,7 +404,10 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } } - eqV := plannedNewVal.Equals(priorVal) + // Unmark for this test for equality. If only sensitivity has changed, + // this does not require an Update or Replace + unmarkedPlannedNewVal, _ := plannedNewVal.UnmarkDeep() + eqV := unmarkedPlannedNewVal.Equals(unmarkedPriorVal) eq := eqV.IsKnown() && eqV.True() var action plans.Action @@ -432,11 +445,11 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { nullPriorVal := cty.NullVal(schema.ImpliedType()) // create a new proposed value from the null state and the config - proposedNewVal = objchange.ProposedNewObject(schema, nullPriorVal, configVal) + proposedNewVal = objchange.ProposedNewObject(schema, nullPriorVal, unmarkedConfigVal) resp = provider.PlanResourceChange(providers.PlanResourceChangeRequest{ TypeName: n.Addr.Resource.Type, - Config: configVal, + Config: unmarkedConfigVal, PriorState: nullPriorVal, ProposedNewState: proposedNewVal, PriorPrivate: plannedPrivate, @@ -453,6 +466,11 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } plannedNewVal = resp.PlannedState plannedPrivate = resp.PlannedPrivate + + if len(unmarkedPaths) > 0 { + plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) + } + for _, err := range plannedNewVal.Type().TestConformance(schema.ImpliedType()) { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, diff --git a/terraform/eval_refresh.go b/terraform/eval_refresh.go index d56291c19..021da2b5d 100644 --- a/terraform/eval_refresh.go +++ b/terraform/eval_refresh.go @@ -80,6 +80,13 @@ func (n *EvalRefresh) Eval(ctx EvalContext) (interface{}, error) { // Refresh! priorVal := state.Value + + // Unmarked before sending to provider + var priorPaths []cty.PathValueMarks + if priorVal.ContainsMarked() { + priorVal, priorPaths = priorVal.UnmarkDeepWithPaths() + } + req := providers.ReadResourceRequest{ TypeName: n.Addr.Resource.Type, PriorState: priorVal, @@ -129,6 +136,11 @@ func (n *EvalRefresh) Eval(ctx EvalContext) (interface{}, error) { return nil, err } + // Mark the value if necessary + if len(priorPaths) > 0 { + newState.Value = newState.Value.MarkWithPaths(priorPaths) + } + if n.Output != nil { *n.Output = newState }