From 08bb3a20c6a565646d98f68688d268c7bae39573 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 1 Nov 2026 18:23:50 +0000 Subject: [PATCH] Share secret traversal between resolution and validation --- docs/roadmap/equivalence.md | 11 +++ internal/engine/secret_reference.go | 145 +++++++--------------------- internal/engine/secret_walk.go | 67 +++++++++++++ internal/engine/secret_walk_test.go | 137 ++++++++++++++++++++++++++ 4 files changed, 252 insertions(+), 108 deletions(-) create mode 100644 internal/engine/secret_walk.go create mode 100644 internal/engine/secret_walk_test.go diff --git a/docs/roadmap/equivalence.md b/docs/roadmap/equivalence.md index 7c4041b..f50d5f7 100644 --- a/docs/roadmap/equivalence.md +++ b/docs/roadmap/equivalence.md @@ -9,6 +9,17 @@ or a requirement to refactor before release. ## 1. Share typed-secret slot traversal between resolution and mutation auditing +**Status:** Complete. [secret_walk.go](../../internal/engine/secret_walk.go) +now owns traversal for both operations, with explicit writable and read-only map +handling. Resolution and mutation auditing retain separate leaf operations and +lifecycle passes. The [container regression](../../internal/engine/secret_walk_test.go) +checks resolved state, mutation paths, nil containers, and lookup counts through +both passes without deriving expectations from the walker. + +Completion validation: the new container cases passed before and after the +refactor. Focused secret/validation tests, `go test -race ./... -count=1`, and +`./scripts/check.sh` passed. + ### Evidence The tests in [secret_reference_test.go](../../internal/engine/secret_reference_test.go) diff --git a/internal/engine/secret_reference.go b/internal/engine/secret_reference.go index b049b5c..2fa2e46 100644 --- a/internal/engine/secret_reference.go +++ b/internal/engine/secret_reference.go @@ -49,81 +49,45 @@ func resolveSecretReferences(root reflect.Value, schema *schemaDescriptor, input lookup = func(string) (string, bool) { return "", false } } locations := &secretReferenceLocationResolver{input: input} - resolveSecretValue(root, schema.root, schema.secrets, Path{}, false, locations, lookup, &result) + walkSecretSlots(root, schema.root, secretSlotsWritable, func(value reflect.Value, path Path, optional bool) { + resolveSecretReference(value, schema.secrets, path, optional, locations, lookup, &result) + }) result.diagnostics = canonicalDiagnostics(result.diagnostics) return result } -func resolveSecretValue(value reflect.Value, descriptor *typeDescriptor, secrets SecretBinding, path Path, optional bool, locations *secretReferenceLocationResolver, lookup secretLookup, result *secretResolutionResult) { - if descriptor.kind == descriptorPointer { - if value.IsNil() { - return +func resolveSecretReference(value reflect.Value, secrets SecretBinding, path Path, optional bool, locations *secretReferenceLocationResolver, lookup secretLookup, result *secretResolutionResult) { + reference := secrets.Read(value) + name := reference.Name + if name == "" { + code := CodeSecretRequired + message := "required secret reference is absent" + if optional || reference.Specified { + code = CodeSecretInvalidName + message = "secret environment name is empty" } - resolveSecretValue(value.Elem(), descriptor.element, secrets, path, descriptor.element.kind == descriptorSecret, locations, lookup, result) + result.diagnostics = append(result.diagnostics, locations.diagnostic(code, path, message)) return } - - switch descriptor.kind { - case descriptorSecret: - reference := secrets.Read(value) - name := reference.Name - if name == "" { - code := CodeSecretRequired - message := "required secret reference is absent" - if optional || reference.Specified { - code = CodeSecretInvalidName - message = "secret environment name is empty" - } - result.diagnostics = append(result.diagnostics, locations.diagnostic(code, path, message)) - return - } - if !validEnvironmentName(name) { - result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretInvalidName, path, "secret environment name is invalid")) - return - } - found, exists := result.lookups[name] - if !exists { - found.value, found.present = lookup(name) - result.lookups[name] = found - } - if !found.present { - result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretUnset, path, "secret environment variable is not set")) - return - } - if found.value == "" { - result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretEmpty, path, "secret environment variable is empty")) - return - } - value.Set(secrets.Make(SecretState{Name: name, Value: found.value, Specified: true, Resolved: true})) - result.references[path.String()] = name - case descriptorStruct: - for _, member := range descriptor.members { - memberPath := path - if !member.inline { - memberPath = path.Key(member.name) - } - resolveSecretValue(value.Field(member.index), member.descriptor, secrets, memberPath, false, locations, lookup, result) - } - case descriptorMap: - if value.IsNil() { - return - } - keys := value.MapKeys() - slices.SortFunc(keys, func(left, right reflect.Value) int { return cmp.Compare(left.String(), right.String()) }) - for _, key := range keys { - entry := reflect.New(value.Type().Elem()).Elem() - entry.Set(value.MapIndex(key)) - resolveSecretValue(entry, descriptor.element, secrets, path.Key(key.String()), false, locations, lookup, result) - value.SetMapIndex(key, entry) - } - case descriptorSlice, descriptorArray: - if descriptor.kind == descriptorSlice && value.IsNil() { - return - } - for index := 0; index < value.Len(); index++ { - resolveSecretValue(value.Index(index), descriptor.element, secrets, path.Index(index), false, locations, lookup, result) - } + if !validEnvironmentName(name) { + result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretInvalidName, path, "secret environment name is invalid")) + return } + found, exists := result.lookups[name] + if !exists { + found.value, found.present = lookup(name) + result.lookups[name] = found + } + if !found.present { + result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretUnset, path, "secret environment variable is not set")) + return + } + if found.value == "" { + result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretEmpty, path, "secret environment variable is empty")) + return + } + value.Set(secrets.Make(SecretState{Name: name, Value: found.value, Specified: true, Resolved: true})) + result.references[path.String()] = name } func changedSecretStates(root reflect.Value, schema *schemaDescriptor, lookups map[string]secretLookupResult) []Path { @@ -131,50 +95,15 @@ func changedSecretStates(root reflect.Value, schema *schemaDescriptor, lookups m if schema == nil || schema.root == nil { return nil } - collectChangedSecretStates(root, schema.root, schema.secrets, Path{}, lookups, &changed) - slices.SortFunc(changed, func(left, right Path) int { return cmp.Compare(left.String(), right.String()) }) - return changed -} - -func collectChangedSecretStates(value reflect.Value, descriptor *typeDescriptor, secrets SecretBinding, path Path, lookups map[string]secretLookupResult, changed *[]Path) { - if descriptor.kind == descriptorPointer { - if !value.IsNil() { - collectChangedSecretStates(value.Elem(), descriptor.element, secrets, path, lookups, changed) - } - return - } - switch descriptor.kind { - case descriptorSecret: - reference := secrets.Read(value) + walkSecretSlots(root, schema.root, secretSlotsReadOnly, func(value reflect.Value, path Path, _ bool) { + reference := schema.secrets.Read(value) expected, ok := lookups[reference.Name] if ok && (!expected.present || expected.value == "" || !reference.Resolved || reference.Value != expected.value) { - *changed = append(*changed, path.clone()) + changed = append(changed, path.clone()) } - case descriptorStruct: - for _, member := range descriptor.members { - memberPath := path - if !member.inline { - memberPath = path.Key(member.name) - } - collectChangedSecretStates(value.Field(member.index), member.descriptor, secrets, memberPath, lookups, changed) - } - case descriptorMap: - if value.IsNil() { - return - } - keys := value.MapKeys() - slices.SortFunc(keys, func(left, right reflect.Value) int { return cmp.Compare(left.String(), right.String()) }) - for _, key := range keys { - collectChangedSecretStates(value.MapIndex(key), descriptor.element, secrets, path.Key(key.String()), lookups, changed) - } - case descriptorSlice, descriptorArray: - if descriptor.kind == descriptorSlice && value.IsNil() { - return - } - for index := 0; index < value.Len(); index++ { - collectChangedSecretStates(value.Index(index), descriptor.element, secrets, path.Index(index), lookups, changed) - } - } + }) + slices.SortFunc(changed, func(left, right Path) int { return cmp.Compare(left.String(), right.String()) }) + return changed } func cloneSecretReferences(values map[string]string) map[string]string { diff --git a/internal/engine/secret_walk.go b/internal/engine/secret_walk.go new file mode 100644 index 0000000..7bb8cf3 --- /dev/null +++ b/internal/engine/secret_walk.go @@ -0,0 +1,67 @@ +package engine + +import ( + "cmp" + "reflect" + "slices" +) + +type secretSlotAccess uint8 + +const ( + secretSlotsReadOnly secretSlotAccess = iota + secretSlotsWritable +) + +// walkSecretSlots visits schema-bound secret references. Writable traversal +// copies map entries into addressable values and writes them back; read-only +// traversal borrows them. The visitor owns the operation performed at each slot. +func walkSecretSlots(root reflect.Value, descriptor *typeDescriptor, access secretSlotAccess, visit func(reflect.Value, Path, bool)) { + var walk func(reflect.Value, *typeDescriptor, Path, bool) + walk = func(value reflect.Value, descriptor *typeDescriptor, path Path, optional bool) { + if descriptor.kind == descriptorPointer { + if !value.IsNil() { + walk(value.Elem(), descriptor.element, path, descriptor.element.kind == descriptorSecret) + } + return + } + switch descriptor.kind { + case descriptorSecret: + visit(value, path, optional) + case descriptorStruct: + for _, member := range descriptor.members { + memberPath := path + if !member.inline { + memberPath = path.Key(member.name) + } + walk(value.Field(member.index), member.descriptor, memberPath, false) + } + case descriptorMap: + if value.IsNil() { + return + } + keys := value.MapKeys() + slices.SortFunc(keys, func(left, right reflect.Value) int { return cmp.Compare(left.String(), right.String()) }) + for _, key := range keys { + entry := value.MapIndex(key) + if access == secretSlotsWritable { + copy := reflect.New(value.Type().Elem()).Elem() + copy.Set(entry) + entry = copy + } + walk(entry, descriptor.element, path.Key(key.String()), false) + if access == secretSlotsWritable { + value.SetMapIndex(key, entry) + } + } + case descriptorSlice, descriptorArray: + if descriptor.kind == descriptorSlice && value.IsNil() { + return + } + for index := 0; index < value.Len(); index++ { + walk(value.Index(index), descriptor.element, path.Index(index), false) + } + } + } + walk(root, descriptor, Path{}, false) +} diff --git a/internal/engine/secret_walk_test.go b/internal/engine/secret_walk_test.go new file mode 100644 index 0000000..48c4d65 --- /dev/null +++ b/internal/engine/secret_walk_test.go @@ -0,0 +1,137 @@ +package engine + +import ( + "fmt" + "strings" + "testing" +) + +func TestSecretResolutionAndMutationAuditCoverTypedContainers(t *testing.T) { + t.Parallel() + type record struct { + Token Secret `yaml:"token"` + } + type inline struct { + Token Secret `yaml:"inline_token"` + } + type config struct { + Inline *inline `yaml:",inline"` + Pointer *Secret `yaml:"pointer"` + Indirect **Secret `yaml:"indirect"` + Values map[string]Secret `yaml:"values"` + Pointers map[string]*Secret `yaml:"pointers"` + Records map[string]record `yaml:"records"` + PointerRecords map[string]*record `yaml:"pointer_records"` + Slice []record `yaml:"slice"` + Array [1]Secret `yaml:"array"` + Absent *Secret `yaml:"absent"` + NilMap map[string]Secret `yaml:"nil_map"` + NilSlice []Secret `yaml:"nil_slice"` + } + const input = `inline_token: WALK_SECRET +pointer: WALK_SECRET +indirect: WALK_SECRET +values: {"a/b~c": WALK_SECRET} +pointers: {entry: WALK_SECRET, absent: null} +records: {entry: {token: WALK_SECRET}} +pointer_records: {entry: {token: WALK_SECRET}, absent: null} +slice: [{token: WALK_SECRET}] +array: [WALK_SECRET] +` + assertResolved := func(t *testing.T, value *config) { + t.Helper() + if value.Inline == nil || value.Pointer == nil || value.Indirect == nil || *value.Indirect == nil || + value.Pointers["entry"] == nil || value.PointerRecords["entry"] == nil || len(value.Slice) != 1 { + t.Fatal("typed secret containers were not decoded") + } + for _, reference := range []Secret{ + value.Inline.Token, *value.Pointer, **value.Indirect, value.Values["a/b~c"], + *value.Pointers["entry"], value.Records["entry"].Token, value.PointerRecords["entry"].Token, + value.Slice[0].Token, value.Array[0], + } { + if !reference.IsResolved() || reference.EnvName() != "WALK_SECRET" || reference.Value() != "WALK-CREDENTIAL-CANARY" { + t.Fatal("a typed secret did not retain its resolved state") + } + } + if value.Absent != nil || value.NilMap != nil || value.NilSlice != nil || + value.Pointers["absent"] != nil || value.PointerRecords["absent"] != nil { + t.Fatal("nil secret containers changed") + } + } + for _, test := range []struct { + name string + path string + mutate func(*config) + }{ + {name: "read-only validation"}, + {name: "inline pointer", path: "/inline_token", mutate: func(value *config) { + value.Inline.Token = SecretEnv(value.Inline.Token.EnvName()) + }}, + {name: "pointer", path: "/pointer", mutate: func(value *config) { + *value.Pointer = SecretEnv(value.Pointer.EnvName()) + }}, + {name: "pointer chain", path: "/indirect", mutate: func(value *config) { + **value.Indirect = SecretEnv((**value.Indirect).EnvName()) + }}, + {name: "map value", path: "/values/a~1b~0c", mutate: func(value *config) { + value.Values["a/b~c"] = SecretEnv(value.Values["a/b~c"].EnvName()) + }}, + {name: "map pointer", path: "/pointers/entry", mutate: func(value *config) { + *value.Pointers["entry"] = SecretEnv(value.Pointers["entry"].EnvName()) + }}, + {name: "map record", path: "/records/entry/token", mutate: func(value *config) { + entry := value.Records["entry"] + entry.Token = SecretEnv(entry.Token.EnvName()) + value.Records["entry"] = entry + }}, + {name: "map record pointer", path: "/pointer_records/entry/token", mutate: func(value *config) { + entry := value.PointerRecords["entry"] + entry.Token = SecretEnv(entry.Token.EnvName()) + }}, + {name: "slice record", path: "/slice/0/token", mutate: func(value *config) { + value.Slice[0].Token = SecretEnv(value.Slice[0].Token.EnvName()) + }}, + {name: "array", path: "/array/0", mutate: func(value *config) { + value.Array[0] = SecretEnv(value.Array[0].EnvName()) + }}, + } { + t.Run(test.name, func(t *testing.T) { + lookups := 0 + validated := false + result, err := testLoadWithSecretLookup(Bytes("secret-containers.yml", []byte(input)), func(name string) (string, bool) { + lookups++ + if name != "WALK_SECRET" { + t.Fatalf("unexpected secret name %q", name) + } + return "WALK-CREDENTIAL-CANARY", true + }, Options[config]{Definition: Definition[config]{Validate: func(value *config, _ *Validation) { + validated = true + assertResolved(t, value) + if test.mutate != nil { + test.mutate(value) + } + }}}) + if !validated || lookups != 1 { + t.Fatalf("validation ran = %v, lookups = %d", validated, lookups) + } + if test.mutate == nil { + if err != nil { + t.Fatal(err) + } + assertResolved(t, &result.Config) + return + } + if result != nil { + t.Fatal("validator mutation published a result") + } + loadError := requireSecretLoadCode(t, err, CodeValidateMutation) + diagnostics := loadError.Diagnostics() + if len(diagnostics) != 1 || diagnostics[0].Path.String() != test.path { + t.Fatalf("mutation diagnostics = %#v, want one at %s", diagnostics, test.path) + } + if strings.Contains(fmt.Sprintf("%v %#v", err, err), "WALK-CREDENTIAL-CANARY") { + t.Fatal("validator mutation error exposed a credential") + } + }) + } +}