From e07931282facb5ab31ed5ca0cb56af09804c4049 Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:13:04 -0400 Subject: [PATCH] fix: a workspace value has one source; an empty delivered file is broken (r144) Three refusals under the maximum-security posture, from review r144: - f1: WorkspaceValue and WorkspaceValueIfSet now report a pinned value whose live delivery breaks as ErrAuthorityValueChanged wrapping the reason, as Authority.Recheck already did. #88 returned the bare read failure, so a caller that stops or re-seals on that sentinel missed it. - f2: a file carrier naming an empty file is refused with ErrFileCarrier ("delivered file is empty"), like a missing file. Codefly delivers by file only above resources.FileCarrierThreshold, so an empty file is a truncated write or a mount race. It used to read as absent, which let WorkspaceValue and ReadAuthority answer from the secret namespace (r144's "sec1" probe). The check sits in readFileCarrier, so every accessor and LoadEnvironmentVariables refuse it. - f3: a name carried in both the public and the secret namespace is refused with the new ErrWorkspaceValueInBothNamespaces: two sources for one value, like inline plus file. It used to answer from the public one silently (r144's "pub" probe), so a stale public copy left behind after a move to the secret namespace kept answering. WorkspaceConfiguration and WorkspaceSecret, which each name one namespace, still answer. Each test fails with its fix reverted. The README gains a Workspace values section; AGENTS.md, the carrier skill and configuration-documents.md say an empty delivered file is broken, not absent. Co-Authored-By: Claude Opus 5.5 (cherry picked from commit e5f8cefc36f3ad1bfea367a20a4c398311f42de8) --- .../skills/runtime-carrier-accessor/SKILL.md | 6 + AGENTS.md | 6 +- README.md | 21 ++++ codefly.go | 5 +- docs/configuration-documents.md | 4 +- file_carrier.go | 16 ++- file_carrier_test.go | 90 ++++++++++++++ for.go | 115 ++++++++++++------ workspace_configuration_keys_test.go | 5 +- 9 files changed, 220 insertions(+), 48 deletions(-) diff --git a/.claude/skills/runtime-carrier-accessor/SKILL.md b/.claude/skills/runtime-carrier-accessor/SKILL.md index b30212e..33e3ba0 100644 --- a/.claude/skills/runtime-carrier-accessor/SKILL.md +++ b/.claude/skills/runtime-carrier-accessor/SKILL.md @@ -47,6 +47,12 @@ because a composition templating an unset variable ships the name with an empty value; treating that as present would shadow the fallback a caller relies on. New accessors do the same. +**An empty delivered file is not absent; it is broken.** A file carrier exists +only for a value above `resources.FileCarrierThreshold`, so `readFileCarrier` +refuses an empty file with `resources.ErrFileCarrier`, like a missing one. Read +file carriers through `processValue` and you inherit that; never read the file +yourself. + **Two spellings, exact first.** Capability and configuration names historically preserved `-` while newer emitters normalize it to `_`. `For`'s accessors try the canonical key, then the legacy one, so an SDK user is independent of the diff --git a/AGENTS.md b/AGENTS.md index dcf92ad..2d16ce1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,9 +125,9 @@ see the skill below. rebuilds it; a test that sets a carrier and forgets the reload asserts against the previous snapshot and passes for the wrong reason. Tests are `package codefly_test` and use `t.Setenv`. -- **An empty injected value is not a value.** A composition templating an unset - variable ships the name with an empty string; `RuntimeValue` reports `false` - so it cannot shadow the configuration a caller falls back to. Keep that. +- **An empty injected value is absent** (an unset template variable; it must not + shadow a fallback). An empty delivered *file*, and a name in both workspace + namespaces, are refused instead ([README](README.md#workspace-values)). - **The Work Context has exactly one implementation and it is not here.** `core/workcontext` signs, verifies, and answers the *structural* question through `Decode`. Nothing here may sign, check a signature, encode a diff --git a/README.md b/README.md index 50aa8ac..8e58c31 100644 --- a/README.md +++ b/README.md @@ -153,6 +153,27 @@ is the authority for these values, and a value that drifts under a running process is an error rather than a reload — the process has already minted a credential sealed to the old one. `For(ctx).WorkspaceValue` answers from that boot read for a pinned name, so the ordinary accessor is not a way around it. +A pinned value whose live delivery breaks is refused with +`ErrAuthorityValueChanged` wrapping the reason, by `WorkspaceValue`, +`WorkspaceValueIfSet` and `Authority.Recheck` alike. + +## Workspace values + +`WorkspaceValue` and `WorkspaceValueIfSet` read a name that may sit in either +the public or the secret workspace namespace, and it must sit in exactly one: + +- a name carried in **both** namespaces is refused with + `ErrWorkspaceValueInBothNamespaces`, as two sources for one value, the same + way a value delivered both inline and by file is refused; +- a delivery that **cannot be read** (a missing file, an empty file, a value + delivered both inline and by file) is refused with that reason + (`resources.ErrFileCarrier`). The other namespace never answers in its place. + +An **empty delivered file** is a broken delivery, not absence. Codefly +delivers a value by file only when it is too large for the process +environment, so an empty file is a truncated write or a mount race. Every +accessor refuses it, and so does `LoadEnvironmentVariables`. An empty +*inline* value is still absent. ## Tenant-scoped effect receipts diff --git a/codefly.go b/codefly.go index e479e7f..f7c84aa 100644 --- a/codefly.go +++ b/codefly.go @@ -275,8 +275,9 @@ func codeflyEnvironmentVariables() []string { // where an inline value is listed by its own name and a file carrier by the // key it carries. For a live file carrier the non-empty value is its PATH: no // file is opened, so a carrier naming an empty file is listed, and the per-key -// lookup then answers absent. Its content is read only by that lookup, so -// listing a secret namespace neither materialises nor caches a secret value. +// lookup then refuses it as a broken delivery. Its content is read only by that +// lookup, so listing a secret namespace neither materialises nor caches a +// secret value. func deliveredNames() []string { names := map[string]struct{}{} for _, entry := range codeflyEnvironmentVariables() { diff --git a/docs/configuration-documents.md b/docs/configuration-documents.md index 0c86e19..94c9d70 100644 --- a/docs/configuration-documents.md +++ b/docs/configuration-documents.md @@ -18,7 +18,9 @@ For typed decoding, use `DecodeConfigurationDocument(name, &destination)`, instead of rounding large integers through `float64`. Lookups use the injected configuration snapshot, then the process environment, -then the SDK environment snapshot. Empty carriers are absent. Outcomes are +then the SDK environment snapshot. An empty inline carrier is absent. A file +carrier naming an empty or missing file is a broken delivery and reads as +`ErrConfigurationDocumentUnreadable`, never as missing. Outcomes are distinguishable with `errors.Is`: | Outcome | Sentinel | diff --git a/file_carrier.go b/file_carrier.go index 596fe61..3612dfb 100644 --- a/file_carrier.go +++ b/file_carrier.go @@ -27,8 +27,8 @@ var ( // processValue is key's value as the process received it: inline in its // environment, or in the file its carrier names. An inline value and a // carrier for the same key are two sources for one fact and are refused, and -// a carrier that cannot be read is an error, never an absent value: an -// unreadable credential must not read as an unset one. +// a carrier that cannot be read, or whose file is empty, is an error, never an +// absent value: an unreadable credential must not read as an unset one. func processValue(key string) (string, bool, error) { value, inline := os.LookupEnv(key) path, carried := os.LookupEnv(resources.FileCarrierKey(key)) @@ -42,10 +42,17 @@ func processValue(key string) (string, bool, error) { if err != nil { return "", false, err } - return content, content != "", nil + return content, true, nil } // readFileCarrier reads key's file once and keeps it. +// +// An empty file is refused like a missing one. Codefly delivers a value by +// file only when it is too large for the process environment (core's +// resources.FileCarrierThreshold), so a carrier naming an empty file is a +// truncated write or a mount race, never a value the host chose to leave +// unset. Reading it as absent would let a lookup step past a broken delivery +// to another source. func readFileCarrier(key, path string) (string, error) { fileCarrierMu.Lock() defer fileCarrierMu.Unlock() @@ -57,6 +64,9 @@ func readFileCarrier(key, path string) (string, error) { if err != nil { return "", err } + if content == "" { + return "", fmt.Errorf("%w: %s: delivered file is empty", resources.ErrFileCarrier, key) + } fileCarrierValues[cacheKey] = content return content, nil } diff --git a/file_carrier_test.go b/file_carrier_test.go index 3051879..7728e62 100644 --- a/file_carrier_test.go +++ b/file_carrier_test.go @@ -172,6 +172,96 @@ func TestAnUnreadablePublicValueIsNeverAnsweredFromTheSecretNamespace(t *testing err = authority.Recheck(ctx) require.ErrorIs(t, err, codefly.ErrAuthorityValueChanged) require.ErrorIs(t, err, resources.ErrFileCarrier) + // The ordinary accessors report a pinned value's broken delivery exactly as + // Recheck does: ErrAuthorityValueChanged, wrapping the reason. _, err = query.WorkspaceValue("probe-fb-pinned", "BINDING") + require.ErrorIs(t, err, codefly.ErrAuthorityValueChanged) + require.ErrorIs(t, err, resources.ErrFileCarrier) + _, found, err := query.WorkspaceValueIfSet("probe-fb-pinned", "BINDING") + require.ErrorIs(t, err, codefly.ErrAuthorityValueChanged) + require.ErrorIs(t, err, resources.ErrFileCarrier) + require.False(t, found) + require.NotContains(t, err.Error(), "pinnedvalue") +} + +// A file carrier exists only for a value too large for the environment, so a +// carrier naming an empty file is a broken delivery (a truncated write, a mount +// race), refused like a missing file. It never reads as absent, so the secret +// namespace cannot answer in its place (review r144 probed this and got "sec1" +// back with no error), and the process refuses to load it. +func TestAnEmptyDeliveredFileIsABrokenDelivery(t *testing.T) { + ctx := context.Background() + query := codefly.For(ctx) + public := resources.WorkspaceConfigurationPrefix + "__PROBE_EMPTY__K" + path := filepath.Join(t.TempDir(), "empty") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + t.Cleanup(func() { _ = codefly.LoadEnvironmentVariables() }) + t.Setenv(resources.FileCarrierKey(public), path) + t.Setenv(resources.WorkspaceSecretConfigurationPrefix+"__PROBE_EMPTY__K", "sec1") + + _, err := query.WorkspaceConfiguration("probe-empty", "K") require.ErrorIs(t, err, resources.ErrFileCarrier) + require.ErrorContains(t, err, "delivered file is empty") + require.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent) + + value, err := query.WorkspaceValue("probe-empty", "K") + require.ErrorIs(t, err, resources.ErrFileCarrier) + require.Empty(t, value) + _, found, err := query.WorkspaceValueIfSet("probe-empty", "K") + require.ErrorIs(t, err, resources.ErrFileCarrier) + require.False(t, found) + _, err = codefly.ReadAuthority(ctx, codefly.AuthorityValueName{Name: "probe-empty", Key: "K"}) + require.ErrorIs(t, err, codefly.ErrAuthorityValueMissing) + require.ErrorIs(t, err, resources.ErrFileCarrier) + + require.ErrorIs(t, codefly.LoadEnvironmentVariables(), resources.ErrFileCarrier) +} + +// A name carried in both the public and the secret namespace is two sources +// for one value, refused like a value delivered both inline and by file. Review +// r144 probed public "pub" plus secret "sec" and got "pub" back with no error, +// so a value moved to the secret namespace with a stale public copy left behind +// kept answering the stale copy. The per-namespace accessors still answer: each +// asks one namespace by name. +func TestAValueInBothNamespacesIsRefused(t *testing.T) { + ctx := context.Background() + query := codefly.For(ctx) + t.Setenv(resources.WorkspaceConfigurationPrefix+"__PROBE_BOTH__K", "pub") + t.Setenv(resources.WorkspaceSecretConfigurationPrefix+"__PROBE_BOTH__K", "sec") + + value, err := query.WorkspaceValue("probe-both", "K") + require.ErrorIs(t, err, codefly.ErrWorkspaceValueInBothNamespaces) + require.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent) + require.Empty(t, value) + require.NotContains(t, err.Error(), "pub\"") + require.NotContains(t, err.Error(), "sec\"") + require.ErrorContains(t, err, "probe-both/K") + + _, found, err := query.WorkspaceValueIfSet("probe-both", "K") + require.ErrorIs(t, err, codefly.ErrWorkspaceValueInBothNamespaces) + require.False(t, found) + + _, err = codefly.ReadAuthority(ctx, codefly.AuthorityValueName{Name: "probe-both", Key: "K"}) + require.ErrorIs(t, err, codefly.ErrAuthorityValueMissing) + require.ErrorIs(t, err, codefly.ErrWorkspaceValueInBothNamespaces) + + public, err := query.WorkspaceConfiguration("probe-both", "K") + require.NoError(t, err) + require.Equal(t, "pub", public) + secret, err := query.WorkspaceSecret("probe-both", "K") + require.NoError(t, err) + require.Equal(t, "sec", secret) + + // A value pinned from one namespace whose name then appears in the other + // is refused as a changed authority, by Recheck and by the accessor alike. + t.Setenv(resources.WorkspaceSecretConfigurationPrefix+"__PROBE_BOTH_PINNED__K", "sec") + authority, err := codefly.ReadAuthority(ctx, codefly.AuthorityValueName{Name: "probe-both-pinned", Key: "K"}) + require.NoError(t, err) + t.Setenv(resources.WorkspaceConfigurationPrefix+"__PROBE_BOTH_PINNED__K", "sec") + err = authority.Recheck(ctx) + require.ErrorIs(t, err, codefly.ErrAuthorityValueChanged) + require.ErrorIs(t, err, codefly.ErrWorkspaceValueInBothNamespaces) + _, err = query.WorkspaceValue("probe-both-pinned", "K") + require.ErrorIs(t, err, codefly.ErrAuthorityValueChanged) + require.ErrorIs(t, err, codefly.ErrWorkspaceValueInBothNamespaces) } diff --git a/for.go b/for.go index 43c7293..e7afe2c 100644 --- a/for.go +++ b/for.go @@ -403,10 +403,9 @@ var ErrWorkspaceConfigurationAbsent = errors.New("no workspace configuration val // // Listing reads names, never values: no file a carrier names is opened, and // no authority pin is read. A file's content is judged only by the per-key -// accessors, which apply their own rules, so a listed key can still read as -// absent (its file is empty) or fail with a named reason (its file is missing, -// or it is delivered both inline and by file). It is never dropped from the -// list. +// accessors, which apply their own rules, so a listed key can still fail with +// a named reason (its file is missing or empty, or it is delivered both inline +// and by file). It is never dropped from the list. func (q *Query) WorkspaceConfigurationKeys(name string) ([]string, error) { return workspaceConfigurationKeys(resources.WorkspaceConfigurationPrefix, name) } @@ -451,22 +450,25 @@ func workspaceConfigurationKeys(prefix string, name string) ([]string, error) { // then the secret namespace. It is intended for settings whose sensitivity is // deployment-defined while preserving a single SDK-only lookup boundary. // -// Only an absent public value moves the lookup on to the secret namespace. A -// public delivery that cannot be read (a missing file, a value delivered both -// inline and by file) is refused with that reason, and the secret namespace -// is not consulted. +// A value lives in exactly one namespace. A public delivery that cannot be read +// (a missing or empty file, a value delivered both inline and by file) is +// refused with that reason, and the secret namespace does not answer in its +// place. A name carried in both namespaces is refused with +// ErrWorkspaceValueInBothNamespaces: two sources for one value, neither of +// which can be preferred silently. // // A value this process read as authority-bearing (see ReadAuthority) answers -// from what was read at boot, and a live value that has since drifted is -// refused with ErrAuthorityValueChanged rather than returned. Without that, a -// process could seal its credential to the boot value and then authorize -// against a different one through the ordinary accessor. +// from what was read at boot. A live value that has since drifted, been +// withdrawn, or become unreadable is refused with ErrAuthorityValueChanged +// (wrapping the read failure, if there was one) rather than returned. Without +// that, a process could seal its credential to the boot value and then +// authorize against a different one through the ordinary accessor. func (q *Query) WorkspaceValue(name string, key string) (string, error) { addressed := AuthorityValueName{Name: name, Key: key}.canonical() pinned, isPinned := authorityPin(addressed) value, err := q.workspaceValueLive(name, key) if err != nil && !isWorkspaceValueAbsent(err) { - return "", err + return "", pinnedReadFailure(addressed, isPinned, err) } if !isPinned { return value, err @@ -491,15 +493,17 @@ func (q *Query) WorkspaceValue(name string, key string) (string, error) { // drifted, as a refusal rather than as "the operator did not assert it". // Absence is an answer; a failed read is not. // -// So an unreadable file carrier, a value delivered both inline and by file, and -// ErrAuthorityValueChanged all come back as errors here, and only a value no -// namespace carries comes back as (", false, nil"). +// So an unreadable or empty file carrier, a value delivered both inline and by +// file, a name carried in both namespaces, and ErrAuthorityValueChanged all +// come back as errors here, and only a value no namespace carries comes back +// as ("", false, nil). For a pinned name, a read failure is +// ErrAuthorityValueChanged wrapping it, as Authority.Recheck reports it. func (q *Query) WorkspaceValueIfSet(name string, key string) (string, bool, error) { addressed := AuthorityValueName{Name: name, Key: key}.canonical() pinned, isPinned := authorityPin(addressed) value, found, err := q.workspaceValueIfSetLive(name, key) if err != nil { - return "", false, err + return "", false, pinnedReadFailure(addressed, isPinned, err) } if !isPinned { return value, found, nil @@ -513,28 +517,63 @@ func (q *Query) WorkspaceValueIfSet(name string, key string) (string, bool, erro return pinned, true, nil } -// workspaceValueIfSetLive reads the public namespace and then the secret one, -// returning a lookup failure rather than continuing past it. Continuing is what -// makes an unreadable value indistinguishable from an absent one. +// ErrWorkspaceValueInBothNamespaces reports a workspace value carried in both +// the public and the secret namespace under one name. They are two sources for +// one value, like a value delivered both inline and by file, and the SDK +// refuses rather than choose: preferring either would let a stale copy left in +// the other answer, with nothing reported. +var ErrWorkspaceValueInBothNamespaces = errors.New("workspace value is carried in both the public and the secret namespace") + +// workspaceValueIfSetLive reads the public and the secret namespace and +// answers from the one that carries the value. A lookup failure in either is +// returned rather than continued past, because continuing is what makes an +// unreadable value indistinguishable from an absent one, and a value carried +// in both is refused. func (q *Query) workspaceValueIfSetLive(name string, key string) (string, bool, error) { - for _, prefix := range []string{ - resources.WorkspaceConfigurationPrefix, - resources.WorkspaceSecretConfigurationPrefix, - } { - value, err := q.workspaceConfigurationValue(prefix, name, key) - if err != nil { - if isWorkspaceValueAbsent(err) { - continue - } - return "", false, err - } - if value != "" { - return value, true, nil - } + public, publicFound, err := q.workspaceNamespaceValue(resources.WorkspaceConfigurationPrefix, name, key) + if err != nil { + return "", false, err + } + secret, secretFound, err := q.workspaceNamespaceValue(resources.WorkspaceSecretConfigurationPrefix, name, key) + if err != nil { + return "", false, err + } + switch { + case publicFound && secretFound: + return "", false, fmt.Errorf("%w: %s/%s", ErrWorkspaceValueInBothNamespaces, name, key) + case publicFound: + return public, true, nil + case secretFound: + return secret, true, nil } return "", false, nil } +// workspaceNamespaceValue is one namespace's answer with absence as a result +// rather than an error. +func (q *Query) workspaceNamespaceValue(prefix string, name string, key string) (string, bool, error) { + value, err := q.workspaceConfigurationValue(prefix, name, key) + if err != nil { + if isWorkspaceValueAbsent(err) { + return "", false, nil + } + return "", false, err + } + return value, value != "", nil +} + +// pinnedReadFailure is what a failed live read means for name: for a value +// pinned as authority it is ErrAuthorityValueChanged wrapping the failure, as +// Authority.Recheck reports it, so a caller that stops or re-seals on that +// sentinel sees a broken delivery of a pinned value; otherwise it is the +// failure itself. +func pinnedReadFailure(name AuthorityValueName, isPinned bool, err error) error { + if !isPinned { + return err + } + return fmt.Errorf("%w: %s cannot be read: %w", ErrAuthorityValueChanged, name, err) +} + // isWorkspaceValueAbsent reports whether err is the "nothing carries this name" // answer rather than a failure to read something that is there. func isWorkspaceValueAbsent(err error) bool { @@ -542,10 +581,10 @@ func isWorkspaceValueAbsent(err error) bool { } // workspaceValueLive is workspaceValueIfSetLive with absence as an error. A -// public delivery that cannot be read is returned as the failure it is, never -// stepped over to the secret namespace: a value under the same name there is a -// different delivery, and answering with it would let an unreadable public -// value be silently replaced. +// delivery that cannot be read is returned as the failure it is, never +// stepped over to the other namespace: a value under the same name there is a +// different delivery, and answering with it would let an unreadable value be +// silently replaced. func (q *Query) workspaceValueLive(name string, key string) (string, error) { value, found, err := q.workspaceValueIfSetLive(name, key) if err != nil { diff --git a/workspace_configuration_keys_test.go b/workspace_configuration_keys_test.go index 49027eb..728d276 100644 --- a/workspace_configuration_keys_test.go +++ b/workspace_configuration_keys_test.go @@ -188,7 +188,10 @@ func TestWorkspaceConfigurationKeysSeparatesAbsenceFromFailure(t *testing.T) { require.NoError(t, err) assert.Equal(t, []string{"BINDING"}, keys, "listing opens no file, so the carrier's path is what it sees") _, err = codefly.For(ctx).WorkspaceConfiguration("keys-empty-file", "BINDING") - assert.ErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "an empty file reads as absent") + require.Error(t, err) + assert.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "an empty delivered file is a broken delivery, not absence") + assert.ErrorIs(t, err, resources.ErrFileCarrier) + assert.ErrorContains(t, err, "delivered file is empty") }) t.Run("a value delivered both inline and by file", func(t *testing.T) {