feat(platform-wallet)!: layered opt-in secret protection (Tier-2 password envelope)

[Claudius-Maginificent]

Why this PR exists

• Problem: dash-evo-tool stores wallet secrets — multi-key/seed wallets and single private keys. By default they belong in the OS keychain (or, for the file backend, a passphrase-encrypted vault), but the user had no way to add an extra password to individual critical objects, and the SecretStore had a latent strip/downgrade weakness.
• What breaks without it: an attacker with write access to the secret backend replaces a password-protected secret with a well-formed unprotected blob carrying a chosen seed. If protection is inferred from the stored blob, the password prompt is bypassed and the attacker's seed is returned to the wallet → funds redirection.
• Blocking relationship: stacked on refactor(platform-wallet)!: retire in-band pool snapshot; hardcode core UTXO account_index=0 #3828 (fix/wallet-core-derived-rehydration).

What was done?

A layered, opt-in secret-protection scheme in packages/rs-platform-wallet-storage:

• Tier-1 (baseline, always on): secrets live in the OS keychain (SecretStore::Os) or a passphrase-encrypted file vault (EncryptedFileStore); a real passphrase is required — blank is rejected (BlankPassphrase). open_unprotected is the explicit keyless door.
• Tier-2 (opt-in, per object): a password envelope (Argon2id + XChaCha20-Poly1305) layered above the SecretStore, backend-independent. Applies to seed wallets and single private keys alike.
• Envelope wire format (secrets/wire/): bincode-encoded Envelope { version, payload: Payload } where Payload is Unprotected(Vec<u8>) | Password { kdf, salt, nonce, ciphertext }. The encoder/decoder share WIRE_CONFIG = standard().with_big_endian().with_no_limit() for AAD/encode; the decoder uses a derived DECODE_CONFIG adding with_limit::<MAX_SECRET_LEN + MAX_ENVELOPE_OVERHEAD>() so an attacker-controlled length prefix can't drive multi-GiB Vec::with_capacity before any tag check.
• AAD bind (three separate structs, all bincode-encoded with -v2 domain tags for byte-disjoint cross-context defense): Tier2Aad (domain ‖ envelope_version ‖ scheme_discriminant ‖ kdf ‖ salt ‖ wallet_id ‖ label), EntryAad, VerifyAad. The KDF cost ceiling is enforced before derivation, and per-read also gated against KdfParams::default_target() on both m_kib and t axes so an inflated header bounded by the global ARGON2_MAX_* ceiling can't drive 16× memory or 5.3× CPU per protected read.
• ★ Strict fail-closed read (the keystone): get_secret(service, label, Option<&SecretString>). A Some(pw) read of a scheme-0 / malformed blob returns ExpectedProtectedButUnsealed — it never returns the bytes. The "is this object protected?" expectation lives only in the caller's Some/None (its trusted DB), and is never inferred from the stored blob. This is what closes the strip/downgrade injection.
• Symmetric blank-password guard: unwrap rejects Some(SecretString::empty()) with BlankPassphrase BEFORE any KDF/AEAD work — mirroring the existing wrap-side pw.is_blank() guard. Closes a backend-write attacker plant-under-blank-pw injection path.
• Trailing-byte defense: bincode::decode_from_slice returns (value, consumed); unwrap asserts consumed == blob.len() and fails closed otherwise, so a truncation/extension probe surfaces as Corruption.
• Write API: set_secret(.., password) and reprotect(current, new) (add / change / remove a password, crash-safe same-slot rewrite). set/get are reimplemented as non-breaking .., None wrappers. delete returns Result<bool> (true = removed, false = was absent) and reprotect returns Err(NoEntry) on absent — surfaces upstream keyring-core::Error::NoEntry instead of swallowing it, and closes a downgrade-detection blind spot where a silent reprotect no-op left the caller's trusted protection-status record disagreeing with the backend.
• Serde gating: SecretString gains Deserialize + JsonSchema behind dedicated default-OFF features secret-serde / secret-schemars (no Serialize; schema leaks no value/min/max/pattern), plus an always-on is_blank().
• No bespoke crypto — reuses the existing crypto::* primitives, SecretBytes/SecretString, the vault's atomic-write path. Wire format follows the project's standing bincode = "=2.0.1" + standard().with_big_endian().with_no_limit() convention (used by rs-platform-serialization, rs-dpp, rs-drive-proof-verifier, etc.).

How Has This Been Tested?

• cargo test -p platform-wallet-storage --lib → 215 passed / 0 failed / 0 ignored.
• cargo test -p platform-wallet-storage --all-targets (--lib + 7 integration binaries + 3 doctests) all green.
• cargo check -p platform-wallet -p platform-wallet-ffi --all-targets clean — no caller-side API change despite the wire-format refactor.
• ★ Non-vacuous strip-injection regression: Some(pw)+scheme-0 read fails closed AND the same blob would decode to the attacker seed under None — proving the strict rule (not malformation) blocks it. Symmetric tests on BOTH the file vault AND the OS-keychain arms.
• Golden byte-vector tests pin the exact wire output for scheme-0 and scheme-1 (deterministic salt/nonce via a #[cfg(test)] seal_with_nonce seam) — any future bincode-config drift trips them before it can corrupt stored data.
• Property test (proptest = "1", already in the workspace) prop_single_byte_flip_never_yields_plaintext runs a single-byte flip across every field within its 60-second budget.
• Adversarial fuzz of unwrap — decode_rejects_trailing_garbage, kdf_m_kib_ceiling_fires_before_derive, kdf_t_ceiling_fires_before_derive, scheme-0/1 dispatch quadrants, AAD-bind one-byte flip per field.
• AAD domain-separation tests prove Tier2Aad, EntryAad, VerifyAad are pairwise byte-disjoint.
• Independent QA (5-specialist wave on the layered-protection feature itself): unanimous PASS, 0 findings above LOW; L-1 strip/downgrade, L-2 KDF-DoS, and L-3 relocation confirmed closed within the library's reach.
• Independent QA on the bincode/AAD refactor (security-engineer-smythe + qa-engineer-marvin + project-reviewer-adams parallel panel): 1 HIGH + 4 MED + 13 LOW — all MEDIUM+ items folded into commit c4af266e4e. Final verdict ACCEPT.
• cargo fmt --all -- --check clean; cargo clippy -p platform-wallet-storage --all-targets -- -D warnings zero warnings; cargo audit clean on the crypto stack (RUSTSEC-2025-0141 bincode unmaintained is an INFO-level project-wide notice, no exploit).

Breaking Changes

• open / rekey now reject a blank passphrase (BlankPassphrase).
• SecretStore::delete returns Result<bool, SecretStoreError> instead of Result<(), SecretStoreError>. .delete(...)?; still compiles and silently discards the bool for callers that don't care.
• SecretStore::reprotect now returns Err(SecretStoreError::NoEntry) for an absent target instead of a silent Ok(()) no-op.
• SecretStoreError::NoEntry promoted to a top-level variant (was nested under OsKeyringErrorKind::NoEntry).
• Tier-2 envelope wire format changed (bincode-encoded structs, all three AADs carry -v2 domain tags). Any Tier-2 envelope written by an earlier commit of this PR no longer decodes — acceptable because this PR has not yet shipped.
• New public error variants: ExpectedProtectedButUnsealed, NeedsPassword, WrongPassword, BlankPassphrase, UnsupportedEnvelopeVersion, top-level NoEntry.
• get_secret is an additive password-aware read; existing get/set call sites are unchanged.

Review feedback resolved this round

• Thread feat: enable mainnet for dashmate #2 (BLOCKING) — symmetric is_blank() guard on unwrap (now wire/envelope.rs::unwrap_password_payload).
• Thread docs: improved sidebar and usage in DAPI client #3 (SUGGESTION) — per-read KDF ceiling at default_target() on m_kib AND t.
• Thread chore: update webpack to version 5 in DPP #4 — bind_object_identity helper, subsumed by the new bincode Tier2Aad / EntryAad / VerifyAad structs.
• Thread docs(sdk): provide getTransactionHistory #5 — clarified SecretString::new deny(unsafe_code) comment.
• Thread build(sdk): upgrade to webpack 5 #6 — delete: Result<bool> + reprotect: Err(NoEntry) on absent.
• Smythe S3.1 (MED) — t-axis added to per-read KDF ceiling.
• Smythe S2.3 (LOW) — consumed == blob.len() assert on bincode decode.
• Marvin QA-008 (MED) — DECODE_CONFIG asymmetric-config rustdoc justifies the security-positive deviation from the project's standard with_no_limit().
• Marvin QA-005 / Smythe S1.3 (LOW) — VERIFY_DOMAIN_V2 rustdoc corrected (TIER2 and VERIFY are both 18 bytes; disjointness is by content past the common prefix).
• Adams PROJ-003 (HIGH) — SECRETS.md rewritten for the new wire format.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section
• I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: de718d1b-2560-4fc6-9ca1-d1c4ac81bbbb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/platform-wallet-secret-protection

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Review complete (commit 07579e3)

[thepastaclaw]

Code Review

Tier-2 per-object password envelope is carefully designed: AAD binds identity, KDF bounds enforced pre-derivation, fresh salt/nonce, strict fail-closed read with comprehensive File+Os tests. One blocking asymmetry: blank object passwords are rejected at wrap but not at unwrap, leaving an injection path under backend-write + Some(empty). One hardening suggestion on read-time KDF ceiling.

🔴 1 blocking | 🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/envelope.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/envelope.rs:256-267: Reject blank object passwords on protected reads, mirroring the wrap-side invariant
  `wrap_with_params` rejects `pw.is_blank()` at line 151 before sealing, but `unwrap_scheme1` accepts `Some(blank)` and derives an Argon2 key from the empty string. Under this PR's threat model the backend bytes are attacker-writable; an attacker who plants a fresh scheme-1 envelope encrypted under the blank password — using the correct `(wallet_id, label)` AAD and a key derived from blank+salt — will produce a blob that decrypts successfully under `Some(SecretString::empty())`. Any caller that forwards an empty prompt value as `Some(pw)` (UI bug, defensive double-call, etc.) then receives attacker-controlled plaintext instead of the typed `BlankPassphrase` failure that the wrap path documents as the crate-wide invariant. Since `Option<&SecretString>` carries the protection assertion across this boundary, validate the inner value at the read boundary too. The fix is one line, and both Codex reviewers independently flagged it.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/envelope.rs:267-278: Consider a per-read ceiling at `default_target()` to bound backend-write OOM/CPU attacks
  `unwrap_scheme1` reads KDF params from the attacker-controllable envelope and only gates them against the global `enforce_bounds` ceiling (`ARGON2_MAX_M_KIB = 1_048_576` KiB, `ARGON2_MAX_T = 16`). Because the public `wrap` always uses `KdfParams::default_target()` (64 MiB / t=3), a backend-write attacker can plant a scheme-1 envelope at the bounds maximum and force ~1 GiB allocation plus 16 Argon2 passes per read — substantial on resource-constrained hosts (iOS/embedded) and an availability attack on protected reads. The existing test `kdf_ceiling_enforced_before_derivation` pins ARGON2_MAX as valid, so this is a deliberate design choice rather than a defect; flagging as a suggestion in case the tighter read-time ceiling is desirable defense-in-depth. If the project intends to expose operator-tuned KDF params later, this should remain as-is and the residual should be documented next to the existing KDF-bounds discussion. Fix would be to add `kdf.m_kib > KdfParams::default_target().m_kib || kdf.t > KdfParams::default_target().t => return Err(SecretStoreError::KdfFailure)` immediately after `enforce_bounds()`.

[thepastaclaw]

Code Review

Incremental review fb7953e→1e70ebd0: the latest push is an unrelated upstream merge (rehydration/SDK/UI) that does not touch packages/rs-platform-wallet-storage/src/secrets/envelope.rs. No new in-scope defects were introduced. Both prior findings on the Tier-2 envelope read path are carried forward and verified STILL VALID against current HEAD: (1) unwrap_scheme1 accepts Some(blank) even though wrap_with_params rejects it, leaving an asymmetry exploitable under this PR's backend-write threat model (blocking), and (2) unwrap_scheme1 only enforces the wide global KDF ceiling, allowing a backend-write attacker to force ~1 GiB / t=16 Argon2 work per read (defense-in-depth suggestion).

🔴 1 blocking | 🟡 1 suggestion(s)

No new latest-delta findings were verified. The two findings below are carried forward from the prior fb7953ea review after re-validation against 1e70ebd0. The existing inline threads were updated as still present; this top-level review records the exact-SHA review action.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/envelope.rs:262-269: [Carried forward from prior review] Reject blank object passwords on protected reads, mirroring the wrap-side invariant
  STILL VALID at 1e70ebd0 — the latest push is a merge of unrelated rehydration work and does not touch `secrets/envelope.rs`. Verified against source: `wrap_with_params` rejects `pw.is_blank()` at envelope.rs:151 before sealing, but `unwrap_scheme1` (envelope.rs:256–286) takes `password: &SecretString` and proceeds straight from the body-length check (line 262) to `decode_kdf` / `enforce_bounds` / `crypto::derive_key(password, &salt, kdf)` at line 278 without validating `password.is_blank()`.
  
  Under this PR's stated backend-write threat model, an attacker who can write the stored bytes can plant a fresh scheme-1 envelope encrypted under the blank password — using the correct `(wallet_id, label)` AAD and a key derived from blank+salt — and that blob decrypts successfully when a caller forwards `Some(SecretString::empty())`. Any caller that accidentally forwards an empty prompt value as `Some(pw)` (UI bug, defensive double-call, blank field returned by an unfocused dialog) then receives attacker-controlled plaintext instead of the typed `BlankPassphrase` failure that the wrap path documents as the crate-wide invariant. Because `Option<&SecretString>` is what carries the protection assertion across this boundary, the inner value must also be validated at the read boundary. Six independent reviewers (claude+codex across general/security/rust-quality) converged on this in both the prior and current cycles.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/envelope.rs:267-278: [Carried forward from prior review] Consider a per-read KDF ceiling at `default_target()` to bound backend-write OOM/CPU attacks
  STILL VALID at 1e70ebd0 — envelope.rs unchanged in this delta. `unwrap_scheme1` reads KDF params from the attacker-controllable envelope and only gates them against the global `enforce_bounds` ceiling at line 269 (`ARGON2_MAX_M_KIB = 1_048_576` KiB, `ARGON2_MAX_T = 16`). Because the public `wrap` always emits `KdfParams::default_target()` (64 MiB / t=3), a backend-write attacker can plant a scheme-1 envelope at the bounds maximum and force ~1 GiB allocation plus 16 Argon2 passes per protected read before AEAD tag verification fails — substantial on resource-constrained hosts (iOS/embedded) and an availability attack on every `get_secret(.., Some(pw))` call.
  
  The existing test `kdf_ceiling_enforced_before_derivation` pins ARGON2_MAX as the valid edge, so the current ceiling is a deliberate forward-compatibility choice rather than a defect — flagging as defense-in-depth. If operator-tuned KDF params remain out of scope, a minimal hardening would be `if kdf.m_kib > KdfParams::default_target().m_kib || kdf.t > KdfParams::default_target().t { return Err(SecretStoreError::KdfFailure); }` immediately after `enforce_bounds()`. If higher-cost envelopes are intended later, document the residual next to the existing KDF-bounds discussion instead.

[thepastaclaw]

Code Review

Both prior findings are FIXED at HEAD a29cbf8: prior-1 (blank-password symmetry on read) is closed by the password.is_blank() guard at wire/envelope.rs:270-272 before any KDF/AEAD work, with regression test unwrap_password_payload_rejects_some_blank_password; prior-2 (per-read KDF ceiling) is closed by the default_target() clamp on BOTH m_kib and t at wire/envelope.rs:283-286, with regression tests covering each axis. One new in-scope documentation finding (codex-general): SECRETS.md still describes the old reprotect no-op semantics for absent objects, but the implementation now returns Err(NoEntry) (pinned by reprotect_absent_returns_no_entry). No new blocking defects in the latest delta.

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/SECRETS.md`:
- [SUGGESTION] packages/rs-platform-wallet-storage/SECRETS.md:251-255: Update `reprotect` docs to match `NoEntry` semantics on absent objects
  The SECRETS.md paragraph still says an absent object is a no-op returning `Ok(())`, but the current `SecretStore::reprotect` implementation intentionally returns `Err(SecretStoreError::NoEntry)` when `get_secret` returns `None` (`secrets/store.rs:262-264`), and the test `reprotect_absent_returns_no_entry` (`store.rs:409-419`) pins that behavior. The rustdoc on `reprotect` (`store.rs:239-242`) is already correct. The PR description calls out this change as closing a downgrade-detection blind spot, so leaving the public design doc with the old no-op contract is misleading: a consumer reading SECRETS.md would treat an absent protected object as harmless instead of as a backend/protection-status disagreement that should not be silently dropped. This is in-scope because the semantic shift is introduced by this PR.

[thepastaclaw]

Code Review

Incremental delta a29cbf8→07579e3e is a small cleanup: SECRETS.md reprotect paragraph updated to document Err(SecretStoreError::NoEntry) for absent objects, and the thin secrets/envelope.rs shim removed with secrets/mod.rs and secrets/store.rs rewired to import from wire::envelope directly. Prior finding prior-a29-1 is FIXED at HEAD. The one new agent-flagged blocking finding (codex-general: AAD format break) is a false positive — the entire rs-platform-wallet-storage crate does not exist on origin/master, so there are no existing v1 file vaults to strand; the PR's published 'unshipped' rationale applies to the whole crate, not only the Tier-2 envelope.