feat(platform-wallet-storage)!: layered opt-in secret protection (Tier-2 password envelope)#3953
Draft
Claudius-Maginificent wants to merge 10 commits into
Conversation
…opt-in, default-off) Task 1 of the layered secret-protection feature. - `SecretString::is_blank()` — always-on inherent method, trims via the Unicode White_Space property (NBSP blanks, ZWSP does not). The enforcement primitive for the Tier-1 blank-passphrase guard and the Tier-2 blank-object-password reject. - Manual `Deserialize` behind the dedicated default-off `secret-serde` feature: routes the owned `String` through `SecretString::new` so the transient plaintext is zeroized; documents the unavoidable deserializer-input-buffer residual. NO `Serialize` companion. - Manual `JsonSchema` behind default-off `secret-schemars`: renders a plain `string`, no minLength/maxLength/pattern/format/example/default — no length or value policy leak (F-7). Hand-written (no derive), so the lock gains no `schemars_derive`. - `MIN_PASSPHRASE_LEN = 1` (coarse floor; real entropy policy is the consumer's, per GAP-012). - Cargo features: `secret-serde = ["secrets","dep:serde"]`, `secret-schemars = ["secret-serde","dep:schemars"]`; `default` unchanged (excludes both); `secrets` does NOT pull them. The gates are on the IMPLS not the dep, so they are satisfiable default-off even with `secrets` (and serde) on (GAP-002). Tests (TS-SER-001..008): is_blank truth table incl. Unicode boundary; bool-no-borrow signature; compile-time no-Serialize/no-Display assertion; GAP-002 regression (Deserialize ABSENT under `secrets` alone, PRESENT under `secret-serde`); zeroizing-roundtrip Deserialize; schema-shape leak guard. Green under default, `--features secret-serde`, `--features secret-schemars`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
…ction
Task 2 of the layered secret-protection feature.
Add five `SecretStoreError` variants with redacted (secret-free) Display
and `keyring_core::Error` projections:
- `ExpectedProtectedButUnsealed` — L-1 keystone: caller asserted protection
(supplied a password) but the stored value is unprotected → fail closed.
- `NeedsPassword` — protected (scheme-1) read with no password; never
returns ciphertext.
- `WrongPassword` — Tier-2 object-password AEAD tag fail; distinct from
the Tier-1 `WrongPassphrase`.
- `BlankPassphrase` — blank vault passphrase or blank object password.
- `UnsupportedEnvelopeVersion { found: u8 }` — magic present, unknown
version/scheme; fail closed regardless of password (GAP-009).
Projections (resolving GAP-004): the four credential/protection STATE
errors ride `NoStorageAccess(boxed)` — losslessly downcast-recoverable,
mirroring `WrongPassphrase`; `UnsupportedEnvelopeVersion` joins the
secret-free `BadStoreFormat` group, mirroring `VersionUnsupported`.
Tests (TS-ERR-001..003): variants distinct (incl. WrongPassword !=
WrongPassphrase, ExpectedProtectedButUnsealed != Corruption); exact
secret-free Display; recoverable NoStorageAccess downcast for the four
states; BadStoreFormat for the version variant.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
Task 3 of the layered secret-protection feature. New `secrets::envelope`
module — backend-independent, sits above SecretStore over both arms.
Wire format: magic b"PWSEV" + ENVELOPE_VERSION:u8 + scheme:u8
(0 unprotected passthrough / 1 Argon2id+XChaCha20-Poly1305 password);
scheme-1 body = kdf(id,m_kib,t,p LE) + salt[32] + nonce[24] + ct+tag.
Reuses crypto::{derive_key,seal,open,random_bytes,KdfParams,enforce_bounds}
(no bespoke crypto); `crypto`/`format` widened to pub(super) so the
sibling envelope module can share them without duplication.
Guardrails:
- L-2: KDF param ceiling enforced BEFORE derivation on the untrusted
header (enforce_bounds gates pre-alloc).
- L-3: AAD binds domain ‖ magic ‖ version ‖ scheme ‖ kdf ‖ salt ‖
wallet_id ‖ label (length-prefixed), mirroring format::aad/verify_aad —
relocation/header-tamper fail the tag.
- SEC-F006/GAP-006: plaintext capped at MAX_SECRET_LEN−MAX_ENVELOPE_OVERHEAD
(=65408), uniform across schemes, so enveloped bytes always fit the
backend cap. Re-exported as pub `MAX_PLAINTEXT_LEN`.
- GAP-009: magic-present unknown version/scheme → UnsupportedEnvelopeVersion,
fail closed regardless of password.
- Legacy-tolerant read (adopted §4.1 contingency): magic-less + None →
raw bytes (+ one-time warn, re-wrapped on next write); magic-less +
Some(pw) → ExpectedProtectedButUnsealed (L-1 preserved).
The strict fail-closed quadrant lives in `unwrap` (the L-1 keystone is
proven against it and wired into the store in the next task).
Tests (TS-ENV-001..010): scheme-0/1 round-trips, fresh salt+nonce,
WrongPassword, identity-AAD relocation rejection, per-field header tamper,
★ KDF-ceiling-before-derive (no OOM), blank-password reject, plaintext
size cap (v5 boundary), magic/version discrimination, and a 2000-iteration
deterministic byte-fuzz + full truncation sweep that never panics and
never leaks plaintext from a tag-failing branch.
NOTE: a scoped `#![allow(dead_code)]` covers the not-yet-wired primitives;
the next task wires them into SecretStore and removes it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
…eystone) Task 4 of the layered secret-protection feature — THE load-bearing control. Wires the envelope's strict, fail-closed quadrant into the store read path over BOTH arms. - `SecretStore::get_secret(service, label, password: Option<&SecretString>)` reads the opaque backend bytes via a new shared `get_raw` seam, then runs `envelope::unwrap`. The "expected-protected" bit lives SOLELY in the caller's `Some/None` argument — never inferred from the stored blob, and never persisted by the library. `get` is refactored to delegate to `get_raw` (behaviour unchanged). - GAP-005 fixture: `secrets::testing::InMemoryCredentialStore`, a writable in-memory `CredentialStoreApi` mock (gated on cfg(test)/`__test-helpers`) with a `raw_overwrite` attacker primitive, so the Os arm — where the L-1 residual bites hardest (§8.3) — and the File re-seal-under-vault-key strip are both coverable in CI. Tests (TS-L1-001..006), each parameterised over File AND Os: - ★ TS-L1-002 strip-injection (non-vacuous): a protected scheme-1 object is overwritten with a well-formed scheme-0 blob carrying a DIFFERENT seed; get_secret(Some(pw)) ⇒ ExpectedProtectedButUnsealed, the attacker seed is NEVER returned — and the same blob WOULD decode to S_evil under None, proving the refusal is the strict rule, not malformation. - Full quadrant; both DET-bug directions fail closed; expectation never inferred from the scheme byte; upgrade-confusion is DoS-only; in-place 1→0 scheme flip (Some fails closed; None GAP-010 residual pinned). Deviation (documented): magic-less + None → legacy raw bytes (adopted §4.1 contingency), not Corruption as the v4 TS-L1-001 row read; magic-less + Some(pw) still fails closed, so L-1 is intact. `!`: get_secret is additive, but the strict read changes how a password-supplied read of a non-enveloped slot behaves (now refuses). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
Task 5 of the layered secret-protection feature. - `SecretStore::set_secret(service, label, secret, password: Option<&SecretString>)` wraps via the envelope ABOVE the backend (the backend stores only the opaque envelope — ciphertext for a protected object) and writes through a shared `put_raw` seam over BOTH arms. - `set`/`get` are reimplemented as non-breaking `..,None` wrappers (signatures unchanged); `get` now routes through the strict `get_secret`. - `reprotect(service, label, current, new)` — the canonical add/change/ remove flow as one same-slot unwrap→rewrap→overwrite; reads under the `current` expectation (a strip is caught fail-closed before any rewrite), then re-writes under `new`. The atomic put leaves the prior value intact on a crash. - `envelope::wrap` now returns a zeroizing `SecretBytes` (symmetric with `unwrap`): a scheme-0 envelope embeds plaintext, so the wire bytes are mlock'd/wiped by construction rather than living in a bare `Vec`. Tests (TS-PW-001..005, TS-ARM-003, TS-T1-005), parameterised over File and Os: full enrol→change→remove lifecycle; no-recovery (lost password bricks the object, fail closed both ways); set/get `None`-wrapper round-trip + scheme-0 proof; Os-arm round-trip unaffected by the blank guard; and ★ TS-PW-004 [File] crash-safety — a disk-write failure mid-change leaves the OLD protected value intact and readable, no half-rotated state. 137 secrets unit tests + secrets integration tests green; clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
…unprotected + docs Task 6 (final) of the layered secret-protection feature. - `EncryptedFileStore::open` and `rekey` now reject a blank (empty / all-whitespace) passphrase with `BlankPassphrase` via `is_blank` — a blank passphrase derives a key from a public salt only (obfuscation, not confidentiality; closes SEC-A / F-1). INTENDED behavioural break for any caller that relied on `SecretString::empty()`. - `EncryptedFileStore::open_unprotected(path)` / `SecretStore::file_unprotected(path)` — the explicit, named keyless door (AC-2.1 maps here, not to `open`). Used for the empty→real `rekey` migration or hosts where secrets carry their own Tier-2 password. - `reject_weak_passphrase` wires the coarse `MIN_PASSPHRASE_LEN` floor (1 = non-blank); real entropy policy is delegated to the consumer (GAP-012). - SECRETS.md: new "Two-tier secret protection" section — the model, the envelope wire format, which tier defeats which adversary, the strict fail-closed read + the caller-DB anti-downgrade dependency, the value-rollback non-defence, add/change/remove + no-recovery, entropy-policy delegation, greenfield/legacy tolerance, `open_unprotected` caveat, the `MAX_PLAINTEXT_LEN` cap; plus the five new error variants and their projections. Tests (TS-T1-001..004,006): blank rejected at open (no file/lock created) and at rekey (vault unchanged); open_unprotected keyless round-trip + real-pass open → WrongPassphrase; empty→real rekey migration; ★ rekey crash-safety leaves the pre-rekey keyless vault intact. 142 secrets unit tests + integration green; clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
…otate v5 deviations Post-review polish addressing the lead's dedup self-scan + QA annotation requirements. Dedup (prefer-established-packages): the GAP-005 Os-arm fixture was a bespoke `secrets::testing::InMemoryCredentialStore` (182 LOC). keyring-core 1.0.0 already ships `keyring_core::mock::Store` — not feature-gated, returns an `Arc<Store>` usable directly as `SecretStore::Os(..)`, and `build()` returns the SHARED `Arc<Cred>` per `(service, user)` so a raw SPI `set_secret` (the backend-write attacker primitive) persists across the fresh entry `get_secret` builds. Replaced the custom mock with it and removed `testing.rs` and its `pub mod testing` gate. The L-1 strip-injection and full quadrant still pass on the Os arm (now via the upstream mock). QA annotations (v5 design supersedes Marvin's v4 test-spec wherever it overrides): the three adapted tests now name the superseding v5 clause in the body — TS-ENV-008 (v5 §4.6: plaintext cap = MAX_SECRET_LEN − MAX_ENVELOPE_OVERHEAD, not MAX_SECRET_LEN), TS-ENV-010(a) and the TS-L1-001 quadrant row (v5 §4.1 legacy-tolerant: magic-less + None → bytes+warn, not Corruption; + Some(pw) still fails closed so L-1 holds). 142 secrets unit + integration tests green; clippy clean; all test targets compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
…-ID scrub Consolidated QA-wave fix batch, part 1 (docs/comments only). - No-recovery + entropy-delegation warnings added to `set_secret` AND `reprotect` rustdoc (lost object password = permanently unrecoverable; Tier-2 confidentiality rests on caller-supplied password entropy, strength policy is the caller's). - SECRETS.md: new paragraph that an OS-arm envelope tag failure is ambiguous (wrong password OR corrupted keychain item), resolving the `WrongPassword` rustdoc's forward reference; added `UnsupportedEnvelopeVersion` to the itemized BadStoreFormat group; added the `None` + truncated-header → `Corruption` row to the strict-read table; documented `reprotect`'s absent-entry no-op. - `SecretStore` enum doc corrected: reads are `get` / `get_secret` / the read inside `reprotect`, not "only `get`". - `BlankPassphrase` Display now points to `open_unprotected` for the deliberate keyless-vault case. - Softened "atomic on both arms" wording: the File arm is the vault's atomic replace; the Os arm inherits the backend's single-item-replace contract. - Ephemeral-ID scrub: removed session-internal finding/spec/clause IDs (SEC-*, GAP-*, TS-*, L-1/2/3, F-*, AC-*, §x.y) and v4→v5 history narration from all committed comments/rustdoc and SECRETS.md, keeping the technical rationale. Traceability belongs in the PR description, not the source. No production logic changed. clippy --lib --tests clean; 142 secrets unit tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
…t, test gaps Consolidated QA-wave fix batch, part 2 (defense-in-depth + test coverage). - Os-arm read bound: `get_raw` rejects a backend blob larger than `MAX_SECRET_LEN + MAX_ENVELOPE_OVERHEAD` BEFORE it reaches the envelope parse/derive path (`SecretTooLarge`), mirroring the File arm whose stored bytes are already capped by `put_bytes`. The Os backend has no such ceiling; a legitimate envelope never approaches the cap. - Os crash test: a backend failure during the rewrite's write (after the read succeeds) leaves the OLD value intact — no half-rotation. Uses the upstream mock's one-shot error injection, with reprotect's read/write split so the failure lands on the write. Test gaps closed: - `SecretString::new` source-wipe: verifies the `String::zeroize` primitive `new` applies + faithful content copy (the freed-source scan would be use-after-free and the crate forbids `unsafe`). - scheme-1 accepts a plaintext at EXACTLY `MAX_PLAINTEXT_LEN` (the accept boundary) and round-trips; enveloped bytes still fit the backend cap. - value-rollback non-defence pinned: an older valid envelope still decrypts cleanly under the current password (so nobody mistakes the strict read for rollback protection). - the default-on export test references the re-exported `MAX_PLAINTEXT_LEN` and `MIN_PASSPHRASE_LEN`. - a `SecretStore::set`-path variant of the no-plaintext-in-vault test. Skipped (optional, with rationale): extracting the AAD length-prefix idiom into a shared helper — it spans `format.rs` (outside this feature's churn) and the envelope module for a 2-line idiom; the coupling outweighs the win. 147 secrets unit + secrets integration tests green; clippy --lib --tests clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
…urce-wipe
Final QA close-out (two LOWs).
- QA-010: the Os-arm read-size guard in `get_raw` had no test. Add
`os_read_rejects_oversized_blob` — via the mock backend, place a raw blob
one byte over `MAX_SECRET_LEN + MAX_ENVELOPE_OVERHEAD` into an Os slot and
assert both `get_secret` and the legacy `get` return
`SecretTooLarge { found, max }` with `max == MAX_SECRET_LEN +
MAX_ENVELOPE_OVERHEAD`.
- QA-001: the source-wipe test is a proxy (it can't assert `new()` invokes
`source.zeroize()` under `#![deny(unsafe_code)]`), so pin the call site
with a do-not-remove comment at `secret.rs` `source.zeroize();` and reword
the test doc to state what it actually checks (the `String::zeroize`
primitive + faithful copy), dropping the "exact primitive new applies"
overclaim.
148 secrets unit + integration tests green; clippy --lib --tests clean;
fmt clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0157yd3YvWeyckhfQivS9gf7
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
2 participants
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.
Why this PR exists
fix/wallet-core-derived-rehydration).What was done?
A layered, opt-in secret-protection scheme in
packages/rs-platform-wallet-storage:SecretStore::Os) or a passphrase-encrypted file vault (EncryptedFileStore); a real passphrase is required — blank is rejected (BlankPassphrase).open_unprotectedis the explicit keyless door.secrets/envelope.rs):magic ‖ version ‖ scheme ‖ kdf ‖ salt ‖ nonce ‖ ct. AAD binds the full identity + header (relocation defense); the KDF cost ceiling is enforced before derivation (DoS defense); plaintext is capped atMAX_PLAINTEXT_LENsoplaintext + overhead ≤ MAX_SECRET_LEN.get_secret(service, label, Option<&SecretString>). ASome(pw)read of a scheme-0 / legacy / malformed blob returnsExpectedProtectedButUnsealed— it never returns the bytes. The "is this object protected?" expectation lives only in the caller'sSome/None(its trusted DB), and is never inferred from the stored blob. This is what closes the strip/downgrade injection.set_secret(.., password)andreprotect(current, new)(add / change / remove a password, crash-safe same-slot rewrite).set/getare reimplemented as non-breaking.., Nonewrappers.SecretStringgainsDeserialize+JsonSchemabehind dedicated default-OFF featuressecret-serde/secret-schemars(noSerialize; schema leaks no value/min/max/pattern), plus an always-onis_blank().crypto::*primitives,SecretBytes/SecretString, and the vault's atomic-write path.How Has This Been Tested?
--features secret-serde188,--features secret-schemars189 — 0 failures.Some(pw)+scheme-0 read fails closed and that the same blob would decode to the attacker seed underNone— proving the strict rule (not malformation) blocks it.cargo auditclean on the crypto stack.cargo fmtclean;cargo clippy -p platform-wallet-storage --lib --testszero warnings.Breaking Changes
open/rekeynow reject a blank passphrase (BlankPassphrase) — the one behavioral break (previously a blank was accepted).ExpectedProtectedButUnsealed,NeedsPassword,WrongPassword,BlankPassphrase,UnsupportedEnvelopeVersion).get_secretis an additive password-aware read; existingget/setcall sites are unchanged (non-breaking wrappers). (Lead commits marked!.)Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent