Skip to content

fix: value-integrity — 5 backends corrupted secrets on read (v0.20.0) - #95

Merged
TechAlchemistX merged 7 commits into
mainfrom
fix/keychain-multiline-hex-decode
Aug 1, 2026
Merged

fix: value-integrity — 5 backends corrupted secrets on read (v0.20.0)#95
TechAlchemistX merged 7 commits into
mainfrom
fix/keychain-multiline-hex-decode

Conversation

@TechAlchemistX

Copy link
Copy Markdown
Owner

Summary

Five of fifteen backends silently corrupted secrets on read. Found from an external bug report against the macOS Keychain backend; that report was the visible tip of a wider, structurally different defect.

set() is unaffected on every backend, so no stored data is damaged and no migration is required. This is a read-path-only class.

The two mechanisms

1. macOS Keychain — undeclared hex encoding. security find-*-password -w hex-encodes its output, with no marker, whenever the value contains any byte outside printable ASCII. Confirmed triggers: newline, tab, 0x7F, and any non-ASCII UTF-8 — so the blast radius is not only PEMs but every accented or emoji-bearing passphrase.

get() now takes security's own declaration of the encoding. Shape decides only whether to ask; find-*-password -g decides what the value is. Encoding is never inferred from shape, because deadbeefcafe0123 is both an ordinary API key and valid hex.

Chose -w-then--g over always--g deliberately: -g prints the password to stderr, and this backend interpolates stderr into operation_failure_message. Making stderr the value channel would put plaintext one bad error path away from a log line.

2. vault / openbao / gcp / azure — trailing-newline truncation. All four ended get() with strip_suffix('\n'), correct only when the CLI appends a newline of its own. vault kv get -field=<f>, bao kv get -field=<f> and gcloud secrets versions access append nothing on a pipe; azure parses the value out of --output json, where no CLI newline exists at all. The strip ate the secret's own last byte.

PEMs, certificates and SSH keys are conventionally stored with a trailing newline and are rejected by openssl / ssh-keygen without it — so this broke exactly the values most likely to be multi-line.

The idiom is per-CLI, not globally wrong

Nine backends were verified clean and deliberately left alone: aws-ssm / aws-secrets (--output text), 1password (op read), conjur, keeper (--format=password), cf-kv (--text), doppler / infisical (--plain), local. Their CLIs do append a newline, so their strip is correct. Deleting the idiom globally would have broken all nine.

cf-kv was diagnosed as broken, fixed, committed — and reverted before shipping. An ad-hoc probe ran wrangler kv key get without the --text flag the backend actually passes. Without it wrangler appends nothing; with it, it appends. The probe "proved" corruption that does not exist, and the fix would have introduced truncation into a correct backend. Section 37 now carries a permanent cf-kv guard.

Why nothing caught this for 19 releases

Every assertion in the live matrix was grep-based, and grep cannot see a lost trailing newline or an undecoded hex payload. Six backends had a multiline unit test — all StrictMock, which returns whatever the test declares, so none could observe real CLI encoding.

Worse, the mock fixtures themselves encoded the wrong model: they fed "value\n", modelling these CLIs as appending a newline they never emit. That is what made the strip look correct under review. This PR corrects the fixtures rather than flipping the assertions, which broke 17 tests — the honest signal.

New permanent gate — smoke section 37

Byte-exact (cmp, not grep) round-trip across 14 of 15 backends, 104 assertions. Four values each: multi-line PEM, non-ASCII UTF-8, legitimate trailing newline, and a hex-shaped secret (deadbeefcafe0123) that fails if anyone reintroduces shape-inferred decoding.

Each value is seeded with the backend's native CLI and read back twice — natively and through secretenv. That split is load-bearing: it separates "this backend cannot hold these bytes" (SKIP, a documented limitation) from "SecretEnv corrupts them" (FAIL, a defect). Without it, Doppler's multi-line rejection and 1Password's write-time newline truncation both read as SecretEnv bugs.

Verification

  • Live smoke: 916/916 PASS, 0 FAIL across all 15 backends (10 SKIPs, all documented limitations).
  • 1136 workspace tests, 0 failures.
  • Stable-channel cargo fmt --check; CI-form clippy --all-targets --workspace.
  • Regression proven both directions: simulating the pre-fix code makes the PEM / UTF-8 / trailing-newline tests fail, while the hex-valued and quotes guards pass on both versions — the correct signature.
  • Version bump touched root Cargo.toml (23 pins) and crates/secretenv-mcp/Cargo.toml's hardcoded core pin; all 24 crates resolve at 0.20.0.
  • release.yml audited — no change needed; it publishes via cargo publish --workspace, so the dep-order incident class is structurally gone.

Known non-defects (recorded, not fixed)

  • Doppler rejects multi-line values at the API.
  • 1Password truncates a trailing newline at write timeop item create stores abc given abc\n. SecretEnv reads back faithfully what 1Password holds.
  • Keeper is not covered by section 37 — seeding needs record-add with a record type and folder, which would mutate the operator's real vault. Both read paths verified instead (JSON path applies no strip; --format=password measured live to append a newline). Recorded as an explicit SKIP so the gap stays visible.

Reviewer note

Section 37 is ~470 lines of new harness code and produced four defects of its own during construction (perl @VAL array interpolation, exact-match arg substitution, an unconditional pre-trim that destroyed the evidence, and Doppler fixture pollution that broke a later run). None reached the product, and all were caught by the two-sided comparison — but a green run proves the backends behave, not that the gate is correct. It's the part of this changeset most worth a careful read.

🤖 Generated with Claude Code

TechAlchemistX and others added 7 commits July 31, 2026 21:05
`security find-*-password -w` silently hex-encodes its output, with no
marker, whenever the value contains a byte outside printable ASCII. The
backend returned that payload verbatim, so any multi-line secret (PEM,
certificate, SSH key) and any non-ASCII UTF-8 value came back corrupted,
silently, on every macOS install.

The blast radius is wider than multi-line values: a passphrase with an
accented character hit the same path.

`get()` now takes `security`'s own declaration of the encoding. Shape
decides only whether to ASK -- `-g` decides what the value IS. This
matters because `deadbeefcafe0123` is both an ordinary API key and valid
hex; inferring the encoding from shape would corrupt it.

- `-w` first, as before; a hex-shaped payload triggers a second read
  with `-g`, which declares the encoding explicitly. Plain values still
  cost exactly one spawn.
- `-g` failure => refuse, rather than return possibly-corrupt bytes.
- Non-UTF-8 item => clean error, not a lossy conversion.

`-g` prints the password to stderr, so that stream is deliberately never
routed into an error message, a log line, or a span field. This is also
why `-w`/stdout stays the value channel for the common path.

`set()` is unaffected -- it has always stored these values correctly, so
no stored data is corrupt and no migration is needed.

Tests: the existing suite is driven by StrictMock, which returns whatever
the test declares and therefore cannot observe the real encoding -- which
is why this survived a green suite. Adds a macOS-only `live` module that
talks to the real `security` binary via an RAII guard (per-PID service
name, deleted on drop, never touches the user's own items). Verified that
the PEM, UTF-8 and trailing-newline tests fail on the pre-fix code while
the hex-valued and quotes/backslashes guards pass on both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four backends ended get() with `strip_suffix('\n')`, which is correct
only when the CLI appends a newline of its own. Where it does not, the
strip ate the secret's own final byte.

  vault    `kv get -field=<f>`             appends nothing on a pipe
  openbao  `kv get -field=<f>`             appends nothing on a pipe
  gcp      `secrets versions access`       writes the payload verbatim
  azure    value parsed from `--output json` -- no CLI newline exists

PEM blocks, certificates and SSH keys are conventionally stored with a
trailing newline and are rejected by openssl / ssh-keygen without it, so
this broke precisely the values most likely to be multi-line. All four
now return the bytes verbatim.

The idiom is NOT wrong everywhere: aws-ssm and aws-secrets (`--output
text`), 1password, conjur, keeper and cf-kv (`--text`) all DO append a
newline, so their strip is correct and is left alone. cf-kv in
particular was briefly "fixed" here on the strength of an ad-hoc probe
that omitted `--text`; with the real argv it appends, and the change was
reverted before it shipped.

`set()` is unaffected throughout, so no stored data is corrupt and no
migration is needed.

Tests: the existing mock fixtures fed "value\n", modelling these CLIs as
appending a newline they never emit -- which is what made the strip look
correct. The fixtures were corrected to match measured behaviour rather
than flipping the assertions, since the fixtures encoded the wrong
model. Adds `get_preserves_legitimate_trailing_newline` to each of the
four; all fail on the pre-fix code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every assertion in the matrix was grep-based and therefore blind to a
lost trailing newline or an undecoded hex payload. That is how a fully
green 810-assertion run coexisted with silent data corruption on five
backends.

Section 37 compares with `cmp`. Four values per backend: multi-line PEM,
non-ASCII UTF-8, a legitimate trailing newline, and a hex-shaped secret
(`deadbeefcafe0123`) that fails if anyone reintroduces shape-inferred
decoding.

Each value is seeded with the backend's native CLI and read back TWICE
-- natively, then through secretenv. That split is load-bearing: it
separates "this backend cannot hold these bytes" (SKIP, a documented
limitation) from "secretenv corrupts them on read" (FAIL, a defect).
Without it, Doppler's multi-line rejection and 1Password's write-time
newline truncation both read as secretenv bugs.

`assert_native_holds` takes an explicit per-backend `appends` flag
rather than accepting "raw or raw-minus-a-newline". The permissive form
cannot distinguish a stored "abc\n" from a stored "abc" printed by a CLI
that appends, and that ambiguity produced a false corruption verdict
against 1Password.

104 assertions across 14 backends. Keeper is an explicit SKIP: seeding
it needs `record-add` with a record type and folder, which would mutate
the operator's real vault, so both read paths were verified by
inspection and a live newline probe instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documents the five-backend value-integrity fix, the behaviour change
(values may now end in a trailing newline where they were previously
truncated), and three known limitations that are NOT secretenv defects:
Doppler rejecting multi-line values, 1Password truncating a trailing
newline at write time, and Keeper's uncovered smoke path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Section 37 seeded ENCODING_* keys into Doppler and Infisical and never
removed them. Doppler branch configs INHERIT from their root config, so
keys left in `secretenv-validation/dev` also surfaced in `dev_registry`,
where `registry list` tried to parse a non-URI value ("abc\n") and
failed -- self-inflicting assertions 312-314 on the following full-matrix
run.

Both are flat key namespaces where a registry source enumerates every
key, so leftovers are not inert the way a scoped path is on vault or
aws-ssm.

Infisical's delete needs `--type shared`: it defaults to `personal` and
a type mismatch deletes nothing while still exiting 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Root workspace.package version + all 23 internal dep pins, plus the
hardcoded secretenv-core pin in secretenv-mcp/Cargo.toml (root-only
replace fails resolution). Cargo.lock regenerated via
`cargo check --workspace`; all 24 crates resolve at 0.20.0.

release.yml audited: no change needed. It publishes via
`cargo publish --workspace`, which derives topological order from the
dep graph, so there is no hand-maintained crate list to drift and no
new crates this cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cargo deny` fails on RUSTSEC-2026-0190, an unsoundness advisory in
anyhow's `Error::downcast_mut()`: adding context via `Error::context`
and later calling `downcast_mut` on the returned `Error` violates borrow
rules and is undefined behaviour.

Pre-existing, not introduced here -- `main` carries the identical
`anyhow = "1"` pin and fails the same check. This branch was simply the
first CI run after the advisory landed in the RustSec database.

No live exposure: the workspace has ZERO `downcast_mut` call sites (only
`downcast_ref` and by-value `downcast`, neither of which is the affected
path). The bump is defence-in-depth rather than an incident response.

Lockfile-only; `anyhow = "1"` already admits 1.0.104. Verified locally:
`cargo deny check` reports advisories/bans/licenses/sources all ok, 1136
workspace tests pass, clippy clean.

Note: `cargo audit` PASSES on the same tree that `cargo deny` rejects --
deny additionally fails on `unsound`/`unmaintained` classes that audit
treats as informational. Running only one of the two is not equivalent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@TechAlchemistX
TechAlchemistX merged commit 2bf030b into main Aug 1, 2026
11 checks passed
@TechAlchemistX
TechAlchemistX deleted the fix/keychain-multiline-hex-decode branch August 1, 2026 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant