From 7887f52e777c31be230bcac107db756deb8e17a3 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Fri, 28 Aug 2026 22:39:33 -0700 Subject: [PATCH] fix(resolver): restate the hashed marker from the value a reference resolved Reference resolution rebuilds a consumer's envelope by copying every key off the target and then overwriting $value with what it resolved. When that target came from stored state it carries $hashed:true from an earlier at-rest hash, and the copy preserves it whatever the new value is, so the marker stops describing the value beside it. That breaks in both directions, and they pull opposite ways. A live plaintext resolution inherits a stale true. The claim is what makes it permanent: the terminal-state hashing pass returns early on any envelope already marked hashed, correctly refusing to hash a digest twice, so nothing corrects the mislabelled one. A successful apply hides this, because the completion path replaces the desired properties with hashed read-actual values; when the command fails there is no such replacement and the plaintext persists, labelled as a digest. A resolution that is itself a stored digest needs the opposite. The plugin-boundary guard refuses a write on the $hashed marker alone, so simply clearing the marker would let a digest reach a provider as though it were the secret. So carry the incoming value's hashed-ness through setRefValue and restate the marker from it, rather than inheriting whatever the copy happened to hold. The embedded reference path gets the same treatment: it has the identical copy-then-overwrite shape, though no test here reaches it, so its exposure is unproven and the change is made for the invariant rather than an observed failure. --- internal/metastructure/resolver/resolver.go | 27 +++ .../metastructure/resolver/resolver_test.go | 40 ++++ .../apply_forma/failed_command_secret_test.go | 224 ++++++++++++++++++ 3 files changed, 291 insertions(+) create mode 100644 internal/workflow_tests/local/apply_forma/failed_command_secret_test.go diff --git a/internal/metastructure/resolver/resolver.go b/internal/metastructure/resolver/resolver.go index 9091d2b45..131e6c996 100644 --- a/internal/metastructure/resolver/resolver.go +++ b/internal/metastructure/resolver/resolver.go @@ -695,6 +695,13 @@ func (pr *propertyResolver) resolveEmbedRef(properties json.RawMessage, ref pkgm return true }) envMap["$value"] = valueStr + // Same reasoning as resolveReference: restate the marker from the + // resolution rather than inheriting whatever the copy carried. + if ref.ResolvedValue.Hashed { + envMap["$hashed"] = true + } else { + delete(envMap, "$hashed") + } envJSON, err := json.Marshal(envMap) if err != nil { return properties, fmt.Errorf("embed: marshal updated envelope: %w", err) @@ -748,6 +755,18 @@ func (pr *propertyResolver) resolveReference(properties json.RawMessage, ref pkg valueToSet := pr.extractResolvedValue(ref) if valueToSet != nil { refObject["$value"] = valueToSet + // The envelope above was copied wholesale from the target, so any + // $hashed marker on it describes the value it used to hold, not the + // one just written over it. Restate the marker from the resolution + // instead of inheriting it. Leaving a stale true makes the terminal + // hashing pass skip the envelope and persist plaintext labelled as a + // digest; clearing a true that is still accurate would let a digest + // past the plugin-boundary guard and reach the provider as a secret. + if ref.ResolvedValue.Hashed { + refObject["$hashed"] = true + } else { + delete(refObject, "$hashed") + } } if ref.ResolvedValue.Strategy != "" { refObject["$strategy"] = ref.ResolvedValue.Strategy @@ -779,6 +798,12 @@ func (pr *propertyResolver) resolveReference(properties json.RawMessage, ref pkg func (pr *propertyResolver) setRefValue(uri pkgmodel.FormaeURI, value string) error { var actualValue string var inheritedVisibility, inheritedStrategy string + // Whether the value being resolved FROM is itself a stored digest rather + // than recoverable plaintext. It has to travel with the resolved value: + // the plugin-boundary guard refuses a write on the $hashed marker alone, + // so dropping it here would let a digest reach a provider as though it + // were the secret. + var inheritedHashed bool if parsed := gjson.Parse(value); parsed.IsObject() { if parsed.Get("$value").Exists() { @@ -789,6 +814,7 @@ func (pr *propertyResolver) setRefValue(uri pkgmodel.FormaeURI, value string) er inheritedVisibility = parsed.Get("$visibility").String() inheritedStrategy = parsed.Get("$strategy").String() + inheritedHashed = parsed.Get("$hashed").Bool() } else { actualValue = value } @@ -836,6 +862,7 @@ func (pr *propertyResolver) setRefValue(uri pkgmodel.FormaeURI, value string) er // erase the record the next plan compares against. newValue.JSONPath = ref.ResolvedValue.JSONPath newValue.ResolvedFrom = ref.ResolvedValue.ResolvedFrom + newValue.Hashed = inheritedHashed ref.ResolvedValue = newValue } diff --git a/internal/metastructure/resolver/resolver_test.go b/internal/metastructure/resolver/resolver_test.go index fe51dca58..21fde0c54 100644 --- a/internal/metastructure/resolver/resolver_test.go +++ b/internal/metastructure/resolver/resolver_test.go @@ -1697,3 +1697,43 @@ func TestResolvePropertyReferences_PreservesResolvedFrom(t *testing.T) { assert.Equal(t, digest, out.Get("$resolvedFrom").String(), "resolving a reference must not drop its provenance") } + +// Resolution restates the hashed marker from the value it resolved, rather than +// inheriting whatever the target envelope happened to carry. Both directions +// matter and they pull opposite ways: a stale true makes the terminal hashing +// pass skip an envelope and persist plaintext labelled as a digest, while +// clearing a true that is still accurate lets a digest past the plugin-boundary +// guard and reach a provider as though it were the secret. +func TestResolvePropertyReferences_HashedMarkerFollowsTheResolvedValue(t *testing.T) { + const uri = pkgmodel.FormaeURI("formae://2abcDEFghiJKLmnoPQRstuVWxyz#/SecretString") + + // An envelope that was hashed at rest, now being re-resolved. + target := json.RawMessage(`{ + "DbPassword": { + "$ref": "` + string(uri) + `", + "$value": "0000000000000000000000000000000000000000000000000000000000000000", + "$hashed": true, + "$visibility": "Opaque" + } + }`) + + t.Run("live plaintext clears a stale marker", func(t *testing.T) { + out, err := ResolvePropertyReferences(uri, target, "live-plaintext") + require.NoError(t, err) + assert.Equal(t, "live-plaintext", gjson.GetBytes(out, "DbPassword.$value").String()) + assert.False(t, gjson.GetBytes(out, "DbPassword.$hashed").Bool(), + "a live plaintext resolution must not stay marked hashed, or terminal hashing skips it") + assert.NoError(t, guardNoHashedValues(out), + "plaintext must be writable to a provider") + }) + + t.Run("a resolved digest keeps its marker so the write guard still fires", func(t *testing.T) { + digest := `{"$value":"1111111111111111111111111111111111111111111111111111111111111111","$hashed":true,"$visibility":"Opaque"}` + out, err := ResolvePropertyReferences(uri, target, digest) + require.NoError(t, err) + assert.True(t, gjson.GetBytes(out, "DbPassword.$hashed").Bool(), + "resolving from a stored digest must stay marked hashed") + assert.ErrorIs(t, guardNoHashedValues(out), ErrHashedValueNotWritable, + "a digest must never reach a provider as if it were the secret") + }) +} diff --git a/internal/workflow_tests/local/apply_forma/failed_command_secret_test.go b/internal/workflow_tests/local/apply_forma/failed_command_secret_test.go new file mode 100644 index 000000000..b939cb00c --- /dev/null +++ b/internal/workflow_tests/local/apply_forma/failed_command_secret_test.go @@ -0,0 +1,224 @@ +// © 2026 Platform Engineering Labs Inc. +// +// SPDX-License-Identifier: FSL-1.1-ALv2 + +//go:build unit + +package workflow_tests_local + +import ( + "encoding/json" + "fmt" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/platform-engineering-labs/formae/internal/metastructure/config" + "github.com/platform-engineering-labs/formae/internal/metastructure/forma_command" + "github.com/platform-engineering-labs/formae/internal/metastructure/testutil" + "github.com/platform-engineering-labs/formae/internal/metastructure/util" + "github.com/platform-engineering-labs/formae/internal/workflow_tests/test_helpers" + pkgmodel "github.com/platform-engineering-labs/formae/pkg/model" + "github.com/platform-engineering-labs/formae/pkg/plugin" + "github.com/platform-engineering-labs/formae/pkg/plugin/resource" +) + +// failingConsumerOverrides mocks a consumer whose Create succeeds and whose +// Update always fails, so a command that plans a consumer update reaches a +// Failed terminal state after resolution has already substituted the live +// secret value into the consumer's desired state. +func failingConsumerOverrides(updateAttempts *atomic.Int32) *plugin.ResourcePluginOverrides { + return &plugin.ResourcePluginOverrides{ + Create: func(req *resource.CreateRequest) (*resource.CreateResult, error) { + if req.ResourceType != "FakeAWS::S3::Bucket" { + return nil, nil + } + return &resource.CreateResult{ + ProgressResult: &resource.ProgressResult{ + Operation: resource.OperationCreate, + OperationStatus: resource.OperationStatusSuccess, + RequestID: "consumer-create-1", + NativeID: "bucket-native-1", + }, + }, nil + }, + Update: func(req *resource.UpdateRequest) (*resource.UpdateResult, error) { + if req.ResourceType != "FakeAWS::S3::Bucket" { + return nil, nil + } + updateAttempts.Add(1) + return nil, fmt.Errorf("consumer update rejected by the provider") + }, + } +} + +// A command that fails after resolution substituted a secret's live value into +// a consumer's desired state must leave no plaintext at rest. Terminal-state +// hashing runs for every final state, Failed included, so the failed command's +// stored rows must hold digests rather than the value the consumer was about +// to be given. +func TestApplyForma_FailedCommand_LeavesNoConsumerPlaintextAtRest(t *testing.T) { + testutil.RunTestFromProjectRoot(t, func(t *testing.T) { + const secretV1 = "failed-command-secret-v1" + const secretV2 = "failed-command-secret-v2" + + logCapture := test_helpers.SetupTestLogger() + + var updateAttempts atomic.Int32 + overrides := failingConsumerOverrides(&updateAttempts) + + cfg := test_helpers.NewTestMetastructureConfig() + cfg.Agent.Synchronization.Enabled = false + m, def, err := test_helpers.NewTestMetastructureWithConfig(t, overrides, cfg) + defer def() + require.NoError(t, err) + + stack := "test-stack-" + util.NewID() + targets := []pkgmodel.Target{{Label: "test-target", Namespace: "test-namespace"}} + + createForma := &pkgmodel.Forma{ + Stacks: []pkgmodel.Stack{{Label: stack}}, + Resources: []pkgmodel.Resource{secretResource(stack, "my-secret", secretV1), secretConsumer(stack, "my-secret")}, + Targets: targets, + } + _, err = m.ApplyForma(createForma, &config.FormaCommandConfig{Mode: pkgmodel.FormaApplyModeReconcile}, "test-client-id", "", "") + require.NoError(t, err) + waitForApplyComplete(t, m) + + cmds, err := m.Datastore.LoadFormaCommands() + require.NoError(t, err) + createCmd := findCommandByType(cmds, pkgmodel.CommandApply) + require.NotNil(t, createCmd) + require.Equal(t, forma_command.CommandStateSuccess, createCmd.State, + "precondition: the create apply must succeed") + + // Change only the secret's value. The consumer is planned because its + // reference resolves from a moved source, its plugin is handed the new + // value, and the plugin refuses, failing the command. + rotateForma := &pkgmodel.Forma{ + Stacks: []pkgmodel.Stack{{Label: stack}}, + Resources: []pkgmodel.Resource{secretResource(stack, "my-secret", secretV2), secretConsumer(stack, "my-secret")}, + Targets: targets, + } + _, err = m.ApplyForma(rotateForma, &config.FormaCommandConfig{Mode: pkgmodel.FormaApplyModeReconcile}, "test-client-id", "", "") + require.NoError(t, err) + waitForApplyComplete(t, m) + + cmds, err = m.Datastore.LoadFormaCommands() + require.NoError(t, err) + var failedCmd *forma_command.FormaCommand + for _, c := range cmds { + if c.Command == pkgmodel.CommandApply && c.ID != createCmd.ID { + failedCmd = c + } + } + require.NotNil(t, failedCmd, "the second apply command must exist") + require.Greater(t, updateAttempts.Load(), int32(0), + "precondition: the consumer's plugin must have been called with the resolved value") + require.Equal(t, forma_command.CommandStateFailed, failedCmd.State, + "precondition: the consumer's refusal must fail the command") + + // The failed command's stored rows carry no plaintext, from either + // generation of the secret's value. + assertNoPlaintextInResourceUpdates(t, m, failedCmd.ID, secretV2) + assertNoPlaintextInResourceUpdates(t, m, failedCmd.ID, secretV1) + + // Nor does the command blob itself. + blob, err := json.Marshal(failedCmd) + require.NoError(t, err) + assert.NotContains(t, string(blob), secretV2, "the failed command blob leaked the resolved secret") + assert.NotContains(t, string(blob), secretV1, "the failed command blob leaked the prior secret") + + // Nor the resource rows. + resources, err := m.Datastore.LoadResourcesByStack(stack) + require.NoError(t, err) + for i := range resources { + assert.NotContains(t, string(resources[i].Properties), secretV2, + "resources.properties leaked plaintext for %s", resources[i].Label) + assert.NotContains(t, string(resources[i].Properties), secretV1, + "resources.properties leaked prior plaintext for %s", resources[i].Label) + } + + // Nor the logs. + for _, entry := range logCapture.GetEntries() { + assert.NotContains(t, entry, secretV1, "log entry leaked the prior secret") + assert.NotContains(t, entry, secretV2, "log entry leaked the resolved secret") + } + }) +} + +// Destroying a secret that a consumer still references fails the consumer, and +// the failed command must still leave no plaintext at rest: the consumer's +// stored desired state holds a digest, never the value it last resolved. +func TestApplyForma_DestroyedReferencedSecret_LeavesNoConsumerPlaintextAtRest(t *testing.T) { + testutil.RunTestFromProjectRoot(t, func(t *testing.T) { + const secretV1 = "destroyed-secret-v1" + + logCapture := test_helpers.SetupTestLogger() + + var calls atomic.Int32 + var props atomic.Value + overrides := secretConsumerOverrides(&calls, &props) + + cfg := test_helpers.NewTestMetastructureConfig() + cfg.Agent.Synchronization.Enabled = false + m, def, err := test_helpers.NewTestMetastructureWithConfig(t, overrides, cfg) + defer def() + require.NoError(t, err) + + stack := "test-stack-" + util.NewID() + targets := []pkgmodel.Target{{Label: "test-target", Namespace: "test-namespace"}} + + createForma := &pkgmodel.Forma{ + Stacks: []pkgmodel.Stack{{Label: stack}}, + Resources: []pkgmodel.Resource{secretResource(stack, "my-secret", secretV1), secretConsumer(stack, "my-secret")}, + Targets: targets, + } + _, err = m.ApplyForma(createForma, &config.FormaCommandConfig{Mode: pkgmodel.FormaApplyModeReconcile}, "test-client-id", "", "") + require.NoError(t, err) + waitForApplyComplete(t, m) + + cmds, err := m.Datastore.LoadFormaCommands() + require.NoError(t, err) + createCmd := findCommandByType(cmds, pkgmodel.CommandApply) + require.NotNil(t, createCmd) + require.Equal(t, forma_command.CommandStateSuccess, createCmd.State, + "precondition: the create apply must succeed") + + // Reconcile with the secret dropped from the forma. It is destroyed + // while the consumer still references it. + dropForma := &pkgmodel.Forma{ + Stacks: []pkgmodel.Stack{{Label: stack}}, + Resources: []pkgmodel.Resource{secretConsumer(stack, "my-secret")}, + Targets: targets, + } + _, err = m.ApplyForma(dropForma, &config.FormaCommandConfig{Mode: pkgmodel.FormaApplyModeReconcile}, "test-client-id", "", "") + require.NoError(t, err) + waitForApplyComplete(t, m) + + cmds, err = m.Datastore.LoadFormaCommands() + require.NoError(t, err) + var dropCmd *forma_command.FormaCommand + for _, c := range cmds { + if c.Command == pkgmodel.CommandApply && c.ID != createCmd.ID { + dropCmd = c + } + } + require.NotNil(t, dropCmd, "the second apply command must exist") + + assertNoPlaintextInResourceUpdates(t, m, dropCmd.ID, secretV1) + + resources, err := m.Datastore.LoadResourcesByStack(stack) + require.NoError(t, err) + for i := range resources { + assert.NotContains(t, string(resources[i].Properties), secretV1, + "resources.properties leaked plaintext for %s", resources[i].Label) + } + + for _, entry := range logCapture.GetEntries() { + assert.NotContains(t, entry, secretV1, "log entry leaked the secret") + } + }) +}