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 (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 at MAX_PLAINTEXT_LEN so plaintext + overhead ≤ MAX_SECRET_LEN.
• ★ Strict fail-closed read (the keystone): get_secret(service, label, Option<&SecretString>). A Some(pw) read of a scheme-0 / legacy / 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.
• 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.
• 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, and the vault's atomic-write path.

How Has This Been Tested?

• 148 secrets unit + 12 integration tests green; across the feature-flag matrix: default 187, --features secret-serde 188, --features secret-schemars 189 — 0 failures.
• ★ Non-vacuous strip-injection regression (TS-L1-002) on BOTH the file vault AND the OS-keychain arms: it asserts the Some(pw)+scheme-0 read fails closed and that the same blob would decode to the attacker seed under None — proving the strict rule (not malformation) blocks it.
• Crash-safety: file-arm and OS-arm mid-rewrite-failure tests confirm the old value stays intact (no half-rotation).
• Independent QA (5-specialist wave): 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; cargo audit clean on the crypto stack.
• cargo fmt clean; cargo clippy -p platform-wallet-storage --lib --tests zero warnings.

Breaking Changes

• open / rekey now reject a blank passphrase (BlankPassphrase) — the one behavioral break (previously a blank was accepted).
• New public error variants (ExpectedProtectedButUnsealed, NeedsPassword, WrongPassword, BlankPassphrase, UnsupportedEnvelopeVersion).
• get_secret is an additive password-aware read; existing get/set call sites are unchanged (non-breaking wrappers). (Lead commits marked !.)

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: 945e6f4d-489f-4842-9e8b-9542fff210e6

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 1e70ebd)

[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]

🔴 Blocking: 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.

Suggested change

	fn unwrap_scheme1(
	wallet_id: &WalletId,
	label: &str,
	password: &SecretString,
	body: &[u8],
	) -> Result<SecretBytes, SecretStoreError> {
	if body.len() < MIN_SCHEME1_BODY {
	// The scheme byte says protected, but the body cannot hold a sealed
	// payload — corrupt, not a strip.
	return Err(SecretStoreError::Corruption);
	}
	let kdf = decode_kdf(&body[..KDF_FIELD_LEN]);
	fn unwrap_scheme1(
	wallet_id: &WalletId,
	label: &str,
	password: &SecretString,
	body: &[u8],
	) -> Result<SecretBytes, SecretStoreError> {
	if body.len() < MIN_SCHEME1_BODY {
	// The scheme byte says protected, but the body cannot hold a sealed
	// payload — corrupt, not a strip.
	return Err(SecretStoreError::Corruption);
	}
	// Mirror wrap's invariant: a blank object password is rejected on read
	// as well as enrol, so a backend-write attacker who plants a scheme-1
	// envelope sealed under the blank password cannot inject plaintext to
	// a caller that accidentally forwards Some(empty).
	if password.is_blank() {
	return Err(SecretStoreError::BlankPassphrase);
	}
	let kdf = decode_kdf(&body[..KDF_FIELD_LEN]);

_{source: ['codex']}

[Claudius-Maginificent]

Confirmed against current code. wrap_with_params rejects pw.is_blank() at envelope.rs:151, but unwrap_scheme1 (256-286) goes straight from body.len() < MIN_SCHEME1_BODY to decode_kdf → enforce_bounds → derive_key(password, …). No symmetric password.is_blank() guard exists, so the asymmetry you describe IS present: a backend-write attacker who plants a scheme-1 envelope sealed under the blank password with the correct AAD (wallet_id, label, KdfParams::default_target(), attacker-chosen salt) yields plaintext to a Some(SecretString::empty()) read.

Status: unresolved — actual gap, not addressed in HEAD.

Proposed fix is exactly the one-line if password.is_blank() { return Err(SecretStoreError::BlankPassphrase); } immediately after the MIN_SCHEME1_BODY check, mirroring wrap's invariant. Will land in a follow-up commit with a regression test that wraps under blank, then asserts unwrap returns BlankPassphrase (not plaintext, not WrongPassword).

[thepastaclaw]

Still present at 1e70ebd.

[thepastaclaw]

🟡 Suggestion: 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().

_{source: ['codex']}

[Claudius-Maginificent]

Verified the residual: unwrap_scheme1 calls only kdf.enforce_bounds() (envelope.rs:269) before derive_key. With ARGON2_MAX_M_KIB = 1_048_576 and ARGON2_MAX_T = 16, a backend-write attacker who plants an inflated scheme-1 envelope can drive ~1 GiB / 16-pass Argon2 per protected read on resource-constrained hosts. Public wrap paths all use KdfParams::default_target() (64 MiB / t=3), so a tighter read-time ceiling would not reject any value this crate's own writers produce.

Taking the suggestion. Adding a default_target() ceiling after enforce_bounds() is a strict tightening with no impact on legitimate envelopes and bounds the per-read DoS surface — defense in depth is cheap here. Will land in the same follow-up commit as the blank-pw fix, with a test that pins (a) the new error on inflated headers and (b) kdf_ceiling_enforced_before_derivation still passes for envelopes wrapped at the floor params.

Keeping the wider ARGON2_MAX_* as the absolute ceiling so a later operator-tunable wrap path remains representable; the per-read cap will be KdfParams::default_target() until/unless an operator-tuned wrap surfaces.

[thepastaclaw]

Still present at 1e70ebd.

[lklimek]

why not just define some struct and use bincode for envelope?

[Claudius-Maginificent]

Verified against current code (envelope.rs:122 wrap_with_params). The wire format is hand-rolled magic ‖ version ‖ scheme ‖ kdf ‖ salt ‖ nonce ‖ ciphertext rather than a #[derive(bincode::Encode)] struct on purpose:

1. AAD parity with secrets/file::format::aad — the on-disk vault binds AAD with explicit length-prefixed LE fields (format::aad at file/format.rs:107). Tier-2 must reproduce that exact byte layout in scheme1_aad so a relocated/confused blob fails the tag; a serde/bincode struct introduces an extra layer (field order, varint length encoding, optional framing) that we'd then have to match in AAD construction anyway.
2. Forward compatibility — the header is magic ‖ version ‖ scheme so an unknown version can fail closed at byte index 5 without parsing the rest. A bincode struct would need a sentinel pre-decode anyway, which is what MAGIC already is.
3. Fixed bounded overhead — MAX_ENVELOPE_OVERHEAD = 128 is computed from the fixed-size fields, which feeds MAX_PLAINTEXT_LEN. Bincode-encoded Vec<u8> carries a runtime length prefix; the overhead would no longer be a compile-time constant.

Leaving as-is — unwrap_scheme1 is ~25 lines and the byte layout is the security boundary.

[lklimek]

ensure we don't already have that aad implemented in the secrets/file . If we do, prefer reuse over reimplementing

[Claudius-Maginificent]

Checked secrets/file/format.rs::aad (file/format.rs:107) — it builds format_version(u32 LE) ‖ wallet_id_len(u32 LE) ‖ wallet_id ‖ label_len(u32 LE) ‖ label. Tier-2's scheme1_aad needs to bind a different field set: TIER2_DOMAIN ‖ MAGIC ‖ ENVELOPE_VERSION ‖ SCHEME_PASSWORD ‖ encode_kdf ‖ salt_len ‖ salt ‖ wallet_id_len ‖ wallet_id ‖ label_len ‖ label.

The two AADs share the wallet_id+label tail, but the prefixes are intentionally disjoint:

• The vault entry AAD uses format_version so a rolled-back FORMAT_VERSION fails the tag.
• The Tier-2 AAD uses TIER2_DOMAIN (b"PWSEV-TIER2-AAD-v1") plus the envelope magic/version/scheme so a Tier-2 blob can never be confused with a vault entry AAD-bound value (defense against cross-context tag reuse), and the KDF params + salt go into AAD too so header tamper / KDF downgrade surfaces as WrongPassword.

Factoring out the shared wallet_id_len ‖ wallet_id ‖ label_len ‖ label tail into a bind_object_identity(buf, wallet_id, label) helper IS a clean reuse — it saves the duplicated (len_le, bytes) boilerplate without forcing the two AADs to share a prefix they must not share. Will land in the follow-up commit; rejecting the larger 'call format::aad directly' shape because domain-separation requires the prefix to differ.

[lklimek]

if thie deny(unsafe_code) is only an issue in test, you can annotate the test to explicitly allow unsafe for that one test. Don't allow unsafe on production code.

[Claudius-Maginificent]

Re-read the code carefully — production does NOT allow unsafe. secret.rs carries crate-wide #![deny(unsafe_code)] and there is no #[allow(unsafe_code)] anywhere in SecretString::new or its tests (confirmed via grep -n unsafe packages/rs-platform-wallet-storage/src/secrets/secret.rs).

The // Do not remove comment at secret.rs:62-63 is explaining why source.zeroize() runs unconditionally on the moved-in String source: the test secret_string_new_zeroizes_string_source at line 380 needs to PIN that the source-side wipe happens, but a direct scan of the freed buffer would be use-after-free — which would require unsafe, which the crate forbids. So the test instead asserts the primitive (String::zeroize empties a buffer) plus pins the call-site via this do-not-remove comment. The source.zeroize() IS the workaround that keeps the test sound under deny(unsafe_code); it does not require, nor enable, any unsafe in production.

Status: no code change needed — this is a comment misread. If the comment wording is the concern (it's currently terse, and 'cannot be scanned under deny(unsafe_code)' reads as if the test is somehow constrained), I can reword to (// a direct freed-buffer scan would require unsafe, which this crate forbids; the test at line 380 instead pins the String::zeroize primitive and this call site) to make the rationale legible without re-reading the linked test. Let me know which you prefer.

[lklimek]

better error as not found

[Claudius-Maginificent]

Confirmed: reprotect (store.rs:251-262) currently returns Ok(()) when the object is absent, and that's documented as 'An absent object is a no-op (Ok(()))' on line 238. The matching no-op behavior also exists for delete (line 266) which is idempotent.

Two paths forward:

(a) Change to Err(NotFound) — would surface programmer error at the boundary, matches what you're asking. Trade-off: a benign race where a parallel delete lands between the caller's list/check and reprotect becomes a hard error the caller must absorb, when today it silently no-ops. Currently absence and deletion-in-flight are indistinguishable (and there's no NotFound variant in SecretStoreError; get_raw returns Ok(None) for absence too).

(b) Keep Ok(()), sharpen the doc — the same-slot reprotect intent is 'enforce the protection state on this object'; if the object is gone, the protection state is vacuously enforced. Race-safe, idempotent, symmetric with delete.

I'd lean (b) for race safety + symmetry, but (a) is the stricter API. If you confirm (a), the follow-up will add SecretStoreError::NotFound, change reprotect's absence branch to return it, and audit delete's no-op for consistency (probably keep delete idempotent and only make reprotect strict — the caller of reprotect has typed 'here is a password I want enforced', so an absent target is meaningful in a way it isn't for delete). Which path do you want?

[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.