fix: value-integrity — 5 backends corrupted secrets on read (v0.20.0) - #95
Merged
Conversation
`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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 -whex-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 takessecurity's own declaration of the encoding. Shape decides only whether to ask;find-*-password -gdecides what the value is. Encoding is never inferred from shape, becausedeadbeefcafe0123is both an ordinary API key and valid hex.Chose
-w-then--gover always--gdeliberately:-gprints the password to stderr, and this backend interpolates stderr intooperation_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()withstrip_suffix('\n'), correct only when the CLI appends a newline of its own.vault kv get -field=<f>,bao kv get -field=<f>andgcloud secrets versions accessappend 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-keygenwithout 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 getwithout the--textflag 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, andgrepcannot see a lost trailing newline or an undecoded hex payload. Six backends had amultilineunit test — allStrictMock, 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, notgrep) 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
cargo fmt --check; CI-formclippy --all-targets --workspace.Cargo.toml(23 pins) andcrates/secretenv-mcp/Cargo.toml's hardcoded core pin; all 24 crates resolve at 0.20.0.release.ymlaudited — no change needed; it publishes viacargo publish --workspace, so the dep-order incident class is structurally gone.Known non-defects (recorded, not fixed)
op item createstoresabcgivenabc\n. SecretEnv reads back faithfully what 1Password holds.record-addwith a record type and folder, which would mutate the operator's real vault. Both read paths verified instead (JSON path applies no strip;--format=passwordmeasured 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
@VALarray 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