feat(platform-wallet): keyring_core secret backends — encrypted-file + OS keyring (secrets feature)#3672
Merged
lklimek merged 58 commits intoMay 29, 2026
Conversation
…ppers, error, validation, MemoryStore
Group A (Tasks 1–3) of the secret-storage feature. All gated behind the
opt-in `secrets` Cargo feature (never enabled by `default`).
Task 1 — `secrets::secret`: `SecretString` (trimmed MIT fork of
dash-evo-tool `Secret`, the egui `TextBuffer`/`take()` leak path deleted
by construction — SEC-REQ-3.8.1/3.8.2) + net-new byte-oriented
`SecretBytes`. Redacting `Debug`, no `Display`/`Deref`/`Serialize`,
full-capacity zeroize on drop, best-effort `region` mlock,
`subtle::ConstantTimeEq` on `SecretBytes`. The only `unsafe` is the
forked full-capacity wipe in `Drop`, confined behind a narrow
`#[allow(unsafe_code)]` + `// SAFETY:` proof — `#![deny(unsafe_code)]`
stays crate-wide (SEC-REQ-4.8).
Task 2 — `secrets::error::SecretStoreError`: concrete `thiserror` enum,
no boxed dyn error (SEC-REQ-4.4 / TC-082), no `#[non_exhaustive]`, no
secret/passphrase/plaintext/source in any variant, static `#[error]`
strings. `secrets::validate`: 32-byte `WalletId` newtype +
`^[A-Za-z0-9._-]{1,64}$` label allowlist, reject-not-sanitize
(SEC-REQ-4.3, CWE-22/20).
Task 3 — `secrets::store::SecretStore` trait (`get` returns
`Option<SecretBytes>`, never bare `Vec<u8>` — SEC-REQ-4.1) +
`MemoryStore` test double, gated by `__secrets-test-helpers` so it is
unreachable from production builds (SEC-REQ-2.3.1/2.3.2). `src/lib.rs`
slot activated; `secrets` feature wires only the RustSec-clean pinned
crypto (argon2=0.5.3, chacha20poly1305=0.10.1, zeroize=1.8.2,
subtle=2.6.1, region=3.0.2, getrandom; keyring-core 4.x split). MSRV
1.92 verified to compile the full dep set (`aes-gcm` omitted).
`Send + Sync` / object-safety compile-asserts added.
Satisfies SEC-REQ 3.1, 3.2, 3.3, 3.5, 3.6, 3.8.1, 3.8.2, 4.1, 4.2,
4.3, 4.4, 4.5, 4.6, 4.8, 2.0.3, 2.3.1, 2.3.2.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…a20-Poly1305 vault
Group B Task 4. `secrets::file::{mod,format,crypto}`:
- Argon2id KDF (`argon2 0.5.3`): floors m≥19456 KiB / t≥2 / p=1 enforced
before any derivation; shipped default 64 MiB / t=3; params + 32-byte
CSPRNG salt stored in the versioned header (SEC-REQ-2.2.1/.2/.3/.4).
- XChaCha20-Poly1305 (`chacha20poly1305 0.10.1`): fresh random 24-byte
nonce per `put` (counter forbidden); combined decrypt so no
unverified plaintext is ever materialized (SEC-REQ-2.2.5/.6/.8).
- AAD = canonical length-prefixed `format_version‖wallet_id‖label`,
defeating blob-swap / version-rollback (SEC-REQ-2.2.7).
- Self-describing magic+version header; unknown version refused, fail
closed (SEC-REQ-2.2.9).
- 0600 at creation via O_EXCL + fchmod before any ciphertext byte;
pre-existing loose perms refused; atomic temp→fsync→rename→dir-fsync;
temp holds only ciphertext, removed on failure (SEC-REQ-2.2.10/.11).
- Atomic rekey: fresh salt + fresh per-entry nonces, no `.bak`
(SEC-REQ-2.2.12). Passphrase held in `SecretString`, never persisted,
zeroized on drop; derived key recomputed per op, never retained
(SEC-REQ-2.2.13).
Satisfies SEC-REQ 2.0.1, 2.0.2, 2.0.4, 2.2.1–2.2.13, 4.1.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…ring-core 4.x split)
Group B Task 5. `secrets::keyring::KeyringStore` over the keyring 4.x
split: `keyring-core 1.0.0` API + per-platform store crates
(linux-keyutils / dbus-secret-service / apple-native / windows-native),
all exact-pinned, RustSec-clean, MSRV-1.92-verified.
- Namespacing: service `dash.platform-wallet-storage`, account
`{wallet_id_hex}:{label}` — two wallets cannot collide, a different
app cannot silently read; only the non-secret index appears in
keyring attributes (SEC-REQ-2.1.2, CWE-312).
- Fail-closed: headless / no Secret Service / no D-Bus → typed
`BackendUnavailable`; locked → typed error. Never `unwrap`, never a
silent plaintext / weaker-store fallback (SEC-REQ-2.1.3/.4 / AR-4).
- keyring-core's bare `Vec<u8>` from `get_secret` is wrapped into
`SecretBytes` and the intermediate zeroized immediately
(SEC-REQ-3.1/4.1).
- Per-OS threat-coverage rustdoc on the type (SEC-REQ-2.0.4 / 2.1.3).
Backend selection is an explicit operator decision — no auto-fallback
between KeyringStore and EncryptedFileStore (SEC-REQ-2.1.3 / AR-4).
Satisfies SEC-REQ 2.0.1, 2.0.4, 2.1.1, 2.1.2, 2.1.3, 2.1.4.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…egration tests Group B Task 6. `tests/secrets_guard.rs` (SEC-REQ-4.5.1): positive string-level scan of `src/secrets/` asserting no logging/formatting sink (`tracing::*`/`println!`/`format!`/`panic!`/…) is paired with an `expose_secret()` result — the guard `tests/secrets_scan.rs` deliberately does NOT cover this tree. Green on the clean tree; fails the moment a secret is routed to a sink. `tests/secrets_api.rs`: `get` returns `Option<SecretBytes>` (type binding, never `Vec<u8>` — SEC-REQ-4.1); `dyn SecretStore` object-safety / positive build guard (SEC-REQ-4.5); no boxed dyn error in `src/secrets/` (TC-082 parity, comment-aware); error `Display` is static and secret-free (SEC-REQ-2.0.1/3.3, CWE-209); wrapper `Debug` redacted at the boundary (SEC-REQ-3.3). `MemoryStore` intentionally unreachable from this external test crate (SEC-REQ-2.3.1). Satisfies SEC-REQ 4.5, 4.5.1. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…secrets crypto deps Group B Task 8 (SEC-REQ-4.7). The existing `rustsec/audit-check` already audits the full `Cargo.lock` — which now pins the `secrets`-gated crypto (argon2/chacha20poly1305/zeroize/subtle/region/ keyring-core + per-platform stores), so they are advisory-checked even though `default` does not enable `secrets`. This adds a `cargo-deny check advisories --all-features` job so the feature-conditional dependency graph is exercised explicitly, plus a workspace `deny.toml` (advisory ignore kept in sync with `.cargo/audit.toml`). Locally verified: `cargo audit` exits 0; none of the secrets crypto pins carry any RustSec advisory (confirms Smythe §7 first-hand). The only flagged item, RUSTSEC-2025-0141 (bincode unmaintained), is a pre-existing unrelated wasm-sdk/dpp dependency, not in the secrets path. Satisfies SEC-REQ 4.7. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…d atomic vault write C1 (HIGH, Marvin QA-001): a `put`/`get`/`delete`/`rekey` against an EXISTING vault with a passphrase deriving a DIFFERENT key than the vault was created with previously wrote a mismatched-key entry and returned Ok, producing an unreadable mixed-key vault. The header now carries a passphrase-verification token: an XChaCha20-Poly1305 seal of a fixed constant under the header-Argon2id-derived key, AAD-bound to `(format_version, wallet_id, "\0verify")` (the leading-NUL label is disjoint from every allowlisted entry label, so the token can never alias a real slot). Every operation on an existing vault derives the key from the supplied passphrase and verifies the token FIRST; a mismatch fails the Poly1305 tag (constant-time, no extra compare, no plaintext on failure) and returns `SecretStoreError::WrongPassphrase` before any entry is read, written, or deleted. New vaults write the token at creation; `rekey` verifies the old token and writes a fresh one. `format_version` bumped 1→2; v1/v2 cross-reads fail closed via the existing `VersionUnsupported` path. C6 (LOW, Smythe SEC-RA-001): `write_vault` no longer swallows the directory-fsync result — it is propagated as a typed error so the atomic temp→fsync→rename→dir-fsync chain (SEC-REQ-2.2.11) is fully enforced. C7 (LOW, Marvin QA-004): the temp file now uses a unique name (`pid` + monotonic counter) created with `O_EXCL` and the destination is never pre-removed, so a crash can never leave the vault absent and concurrent writers cannot collide on a fixed temp name. The atomic rename + fsync ordering is unchanged. Tests (red→green, file/mod.rs): wrong-pass `put` to existing vault ⇒ `Err(WrongPassphrase)` + vault still readable with the correct pass + rejected slot never written; wrong-pass `get`/`delete` ⇒ `Err(WrongPassphrase)` + vault unmutated; correct pass round-trips unchanged. The two wrong-pass tests were FAILED before this fix and pass after; format (de)serialize round-trips the token fields. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…ringLocked; correct keyring-core attribution
C3 (MED, Adams PROJ-002 / Marvin QA-003): `map_keyring_err` collapsed
keyring-core's `NoStorageAccess` into `BackendUnavailable`, leaving
`SecretStoreError::KeyringLocked` dead. Per keyring-core 1.0.0 docs,
`NoStorageAccess` covers the locked-collection case ("it might be that
the credential store is locked"), so it now maps to `KeyringLocked`,
enabling the unlock-retry UX (SEC-REQ-2.1.4). Genuinely-absent backends
(`NoDefaultStore` / `PlatformFailure`) stay `BackendUnavailable`.
Added `locked_keyring_maps_to_keyring_locked` asserting the locked,
absent, and not-found mappings.
C5 (LOW, Adams PROJ-003 / Marvin QA-004): the module header said
"keyring-core 4.x split" — inaccurate. Reworded to state the lib is
`keyring-core 1.0.0` plus the per-platform store crates; the `keyring`
4.x crate is the sample CLI and is not a dependency. No dependency
change.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…roizes on drop C4 (MED, Smythe SEC-RA-002 / Adams PROJ-004 / Marvin QA-002): the rustdoc claimed stored values sit in `SecretBytes`, but the map held a bare `Vec<u8>` that never zeroized — code contradicted the doc. Fixed the code (not the doc): the backing map is now `HashMap<(WalletId,String), SecretBytes>`, closing SEC-REQ-2.3.2 so even test memory is wiped on drop. Added `stored_value_is_zeroizing_ wrapper` (type-binding assertion) + a `needs_drop::<SecretBytes>()` compile-time guard. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…rgo.toml comment C5 (LOW, Adams PROJ-003 / Marvin QA-004): the per-platform-store dependency comment said "keyring-core 4.x split". Reworded to state accurately that `keyring-core 1.0.0` is the API and the per-platform crates provide the backends (the `keyring` 4.x crate is the sample CLI and is intentionally not depended on). No dependency change. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…etStore API C2 (MED, Adams PROJ-001): the trait sketch was stale/dangerous — `get -> Option<Vec<u8>>` (the exact CRITICAL leak SEC-REQ-4.1 forbids) and the false "feature flag exists today but flips no code" line. Rewritten to the delivered API: `get -> Result<Option<SecretBytes>, SecretStoreError>`, accurate `put`/`delete` signatures, the real backends (KeyringStore/EncryptedFileStore/MemoryStore with their fail-closed / gating semantics), and the now-true statement that enabling `secrets` activates the module. Present-state only, no history narration; no forbidden token introduced into `src/sqlite/schema/` or `migrations/`. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
Claudius-Maginificent
changed the title
Claudius-Maginificent
changed the title
Claudius-Maginificent
changed the title
This was referenced May 20, 2026
…ult-on Removes the cargo-deny advisories CI job and its `deny.toml` config in favour of the existing `rustsec/audit-check` job. Once `secrets` is in the default feature set, `Cargo.lock` unconditionally pins the RustSec-clean crypto stack (`argon2`/`chacha20poly1305`/`zeroize`/ `subtle`/`region`/`keyring-core` + per-platform store crates) so a single audit run covers them all (SEC-REQ-4.7). `secrets` joins `sqlite`+`cli` as a default feature. Dev-dependency on self adds `default-features = false` so the off-state CI invocation (`--no-default-features --features sqlite,cli`) actually exercises the secrets-disabled graph — otherwise the dev-dep view would silently re-enable defaults for every integration test. New `tests/secrets_off_state.rs` is the runtime D4 guard: gated `#[cfg(not(feature = "secrets"))]`, it builds against the persister surface only and asserts the off-state graph stays consumable. T1+T2 land atomically — cargo-deny removal coincides with secrets going default-on so crypto pins never drop out of audit scope between commits. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…backends
Retires the crate-local `SecretStore` trait + `SecretStoreError` enum
and rebuilds the `secrets` submodule on
`keyring_core::api::{CredentialApi, CredentialStoreApi}` — the upstream
SPI shipped by `keyring-core 1.0.0`. The `EncryptedFileStore`'s
security construction (Argon2id + XChaCha20-Poly1305 + AAD verify
token + 0600 + atomic temp→rename + dir-fsync + zeroize) is preserved
byte-for-byte; only the trait surface changes.
API-shape mapping (Nagatha §1, variant A — the `:` delimiter is rejected
by the label allowlist):
service = "dash.platform-wallet-storage/" + hex(wallet_id)
user = label
Per-task content:
- **T3** `src/secrets/file/error.rs` — new `FileStoreError` enum
(`Decrypt`, `WrongPassphrase`, `KdfFailure`, `VersionUnsupported`,
`MalformedVault`, `InvalidLabel`, `InsecurePermissions`, `Io`).
Static `#[error]` strings only; no secret in any variant.
`src/secrets/file/error_bridge.rs` — `FileStoreFailure` unit-only
marker (Smythe EDIT-3: no `String`/`Vec<u8>`/`Path` fields permitted,
enforced via a compile-time `Copy` assertion) boxed inside
`keyring_core::Error::NoStorageAccess` (WrongPassphrase) or
rendered into `BadStoreFormat`'s static `String` payload. The
`downcast_failure` helper recovers the marker for D1(b).
- **T4** `src/secrets/file/mod.rs` — `EncryptedFileStore` implements
`CredentialStoreApi`; per-`(service, user)` entries implement
`CredentialApi`. The store is held behind an internal `Arc` so
long-lived credentials can outlive the public handle. `delete` honors
upstream's `NoEntry`-if-absent contract (D3). `service` parsing
rejects mismatch with `Invalid("service", _)`; `validated_label` runs
at `build` time AND every `CredentialApi` op (defence in depth,
M-2). All twelve in-module security tests port one-for-one through
the SPI (NoEntry for absence, downcast for typed-error checks).
- **T5** `src/secrets/keyring.rs` — `KeyringStore` wrapper retired in
favour of the bare `default_credential_store() -> Result<Arc<dyn
CredentialStoreApi + Send + Sync>, keyring_core::Error>` constructor.
Headless / unknown OS / D-Bus-less Linux → `NoDefaultStore` per D2
(typed, single SPI error). Never panics, never falls back.
- **T7** `src/secrets/memory.rs` — `MemoryStore` → `MemoryCredentialStore`
implementing `CredentialStoreApi`. Internal map keys on
`(service, user)` strings, values remain `SecretBytes` (SEC-REQ-2.3.2).
Still gated behind `__secrets-test-helpers`.
- **T8** `src/lib.rs` — object-safety + `Send + Sync` assertions now
target `keyring_core::Error` and `dyn CredentialStoreApi + Send +
Sync`. `src/secrets/mod.rs` re-exports the new surface; `pub use
SecretStore` / `SecretStoreError` retired.
- **Tests** — `tests/secrets_api.rs` rewritten against the SPI; the
`Vec<u8> → SecretBytes::new` consumer-seam pattern (Smythe EDIT-1:
no named intermediate `Vec` binding) is the type-shape assertion.
`tests/secrets_guard.rs` extended with the EDIT-2 EDIT-2 guard:
no `{{:?}}`-debug-format paired with `keyring_core::Error` in
`src/secrets/` (since `BadEncoding`/`BadDataFormat` embed raw
`Vec<u8>`). All twelve `EncryptedFileStore` security invariants
pass on the new API.
`tests/secrets_seed_provider_adapter.rs` and the
`seed_provider_adapter.rs` source file are NOT landed on this branch:
the `SeedProvider`/`WalletSecret`/`SeedUnavailable` types they consume
live in `rs-platform-wallet` on PR #3692, not on this base. The
rewritten adapter will land on PR #3692's rebase onto this tip — see
the rework report.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…core SPI
Rewrites SECRETS.md as the present-state spec for the secrets
submodule on the upstream `keyring_core::api` SPI:
- Drops the retired `SecretStore` trait listing.
- Documents the `service = "dash.platform-wallet-storage/" + hex(wid)`,
`user = label` key shape with the allowlist precondition.
- Memory hygiene section codifies Smythe EDIT-1: `SecretBytes::new(...)`
is the consumer-seam wrapper, no named intermediate `Vec` binding.
- Backends section: `EncryptedFileStore` + `default_credential_store()`
+ test-only `MemoryCredentialStore`.
- Cross-SPI error bridge: `FileStoreFailure` unit-only marker (EDIT-3
constraint stated as load-bearing), `downcast_failure` recovery
path, EDIT-2 `{:?}`-format ban on `keyring_core::Error` documented
with its enforcement test.
- Audit hooks section adds `secrets_off_state` (D4) and rephrases
`secrets_guard` to cover both leak sinks.
- Cargo features paragraph notes `secrets` is default-on; cargo-deny
removal is noted via the lockfile-is-audit-coverage rationale.
`src/lib.rs` crate-level doc retouched to point at the new SPI and
backend names (the prior "SecretStore reserved" phrasing retired).
`tests/secrets_scan.rs` exemption comment rephrased to describe the
present state.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…rface `tests/secrets_default_on_compiles.rs` (M-S4) — a build-only assertion that the default feature set (`secrets` in) re-exports every public type/function in the `secrets` submodule. Names: `EncryptedFileStore`, `SecretBytes`, `SecretString`, `WalletId`, `FileStoreError`, `FileStoreFailure`, `SERVICE_PREFIX`, `default_credential_store`, `keyring_core::Error`. Compiling the test target is the assertion; the body never exercises a backend. Pairs with `tests/secrets_off_state.rs` (D4 — runtime proof under `--no-default-features --features sqlite,cli` that the surface compiles out and the persister still links). Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…EDIT-4)
QA-501 (MEDIUM, EDIT-4 forward-compat): `SecretBytes`/`SecretString`
retained `impl PartialEq`/`Eq` despite EDIT-4's binding intent. The
impls delegated to constant-time compares so today's behaviour is
safe, but leaving `==` reachable means future bridge code could
inherit a non-constant-time path or a length-leaking shortcut without
review noticing.
EDIT-4 says: no `==` on secret bytes, no exception. Strip the impls
and let `subtle::ConstantTimeEq::ct_eq` be the only equality path.
- `secret.rs` — removed `impl PartialEq for SecretBytes` /
`impl Eq for SecretBytes` and `impl PartialEq for SecretString` /
`impl Eq for SecretString`. `SecretString` gains an
`impl ConstantTimeEq` so callers keep a constant-time-safe
equivalence path (was previously implicit inside `PartialEq::eq`).
- Public rustdoc on both types names `PartialEq`/`Eq` in the "not
implemented" list and points callers at `ConstantTimeEq::ct_eq`.
- `compile_fail` doc-test on each type asserts that `a == b` does NOT
compile — durable forward-compat guard. If a future change adds
`PartialEq` back, the doc-test starts compiling and the test fails.
- Test callers migrated:
- `secret_string_eq_is_value_based` →
`secret_string_ct_eq_is_value_based`, asserts via
`bool::from(a.ct_eq(&b))`.
- `secret_bytes_constant_time_eq` drops its trailing
`assert_eq!(a, b)` / `assert_ne!(a, c)` lines (the prior
ct_eq-based assertions above them already covered the same
invariant).
Workspace-wide grep confirmed no other `==`/`assert_eq!` callers on
`SecretBytes`/`SecretString` exist.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a default-on secrets subsystem to platform-wallet-storage to persist wallet secret material outside SQLite, using the upstream keyring_core SPI and providing both an encrypted-file vault backend and an OS-keyring backend.
Changes:
- Introduces
platform_wallet_storage::secrets(default feature) withEncryptedFileStore(Argon2id + XChaCha20-Poly1305) anddefault_credential_store()for OS keyrings. - Adds secret-handling wrappers (
SecretBytes,SecretString) plus validation and an error-bridging layer tokeyring_core::Error. - Adds multiple guard tests (
secrets_scan,secrets_guard, API shape checks, and “secrets off” build-mode guard) and updates docs/README/Cargo features accordingly.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-platform-wallet-storage/tests/secrets_scan.rs | Clarifies schema/migrations scan scope vs src/secrets/ exemption. |
| packages/rs-platform-wallet-storage/tests/secrets_off_state.rs | Adds runtime guard ensuring secrets compile out when feature is disabled. |
| packages/rs-platform-wallet-storage/tests/secrets_guard.rs | Adds string-level leak-prevention scans for the secrets module. |
| packages/rs-platform-wallet-storage/tests/secrets_default_on_compiles.rs | Build-only test asserting secrets surface is available in default build. |
| packages/rs-platform-wallet-storage/tests/secrets_api.rs | API/boundary shape tests for secrets SPI usage and error rendering. |
| packages/rs-platform-wallet-storage/src/secrets/validate.rs | Adds WalletId newtype + strict label allowlist validation. |
| packages/rs-platform-wallet-storage/src/secrets/secret.rs | Implements SecretBytes/SecretString zeroizing wrappers and CT equality. |
| packages/rs-platform-wallet-storage/src/secrets/mod.rs | Wires secrets submodules and public re-exports. |
| packages/rs-platform-wallet-storage/src/secrets/memory.rs | Adds in-RAM CredentialStoreApi test double behind __secrets-test-helpers. |
| packages/rs-platform-wallet-storage/src/secrets/keyring.rs | Adds OS-keyring default store constructor with fail-closed behavior. |
| packages/rs-platform-wallet-storage/src/secrets/file/mod.rs | Implements encrypted vault store + CredentialStoreApi/CredentialApi. |
| packages/rs-platform-wallet-storage/src/secrets/file/format.rs | Defines vault format framing and canonical AAD construction. |
| packages/rs-platform-wallet-storage/src/secrets/file/error.rs | Defines file-backend error taxonomy. |
| packages/rs-platform-wallet-storage/src/secrets/file/error_bridge.rs | Bridges file-backend errors into keyring_core::Error + downcast helper. |
| packages/rs-platform-wallet-storage/src/secrets/file/crypto.rs | Implements Argon2id KDF + XChaCha20-Poly1305 seal/open helpers. |
| packages/rs-platform-wallet-storage/src/lib.rs | Exposes secrets module behind feature and adds send/sync/object-safety checks. |
| packages/rs-platform-wallet-storage/SECRETS.md | Updates spec/docs to present-state secrets implementation and audit hooks. |
| packages/rs-platform-wallet-storage/README.md | Updates feature table and build modes to include secrets and helpers. |
| packages/rs-platform-wallet-storage/Cargo.toml | Adds secrets dependencies, platform store deps, features, and dev-dep tweaks. |
| Cargo.lock | Pulls in keyring-core + platform store crates + crypto dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rm-wallet-storage-secrets
Claudius-Maginificent
changed the title
…ead of panicking; doc cleanup `EncryptedFileStore::rekey` panicked via `Arc::get_mut(...).expect(...)` whenever an outstanding `EncryptedFileCredential` (which clones the inner `Arc` in `build()`) was still alive — a caller-reachable runtime state, not a logic bug. Swap the `expect` for a recoverable typed `FileStoreError::Busy`, preserving the fail-loud property (still no silent stale-handle rekey). Wire a parity `FileStoreFailure::Busy` unit variant through the SPI bridge (`into_keyring` -> NoStorageAccess, Display, marker_from_message) keeping the enum unit-variants-only + Copy. Add a focused rekey-busy test plus bridge round-trip coverage. Docs: present-state lede + package description (drop "future SecretStore"), fix `__secrets-test-helpers` to name `MemoryCredentialStore`, add `getrandom` to the SECRETS.md audit-scope enumeration, document the load-bearing FileStoreFailure Display text, and note why SecretBytes keeps `.max(1)` on region::lock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lklimek
requested changes
May 22, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for 6ca1bc1: carried forward STILL VALID prior findings: #1:with-vault-lock-reports-try-write-io-errors-as-busy, #2:build-os-skips-label-allowlist-before-native-keyring; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Latest delta (6ca1bc1) is a small, correct error-construction refactor in rs-platform-wallet-ffi/src/persistence.rs (drops From<String> lift in favor of explicit PersistenceError::backend(...)). No new defects introduced. Both prior findings remain STILL VALID at HEAD: with_vault_lock collapses every try_write io::Error to Busy, and build_os bypasses the documented label allowlist on the OS-keyring arm. Carried forward as suggestions; no blockers.
🟡 2 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/file/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:259-267: with_vault_lock collapses every try_write io::Error into Busy
`fd_lock::RwLock::try_write` returns a plain `io::Error` whose `ErrorKind` distinguishes lock contention (`WouldBlock`) from real I/O faults (`PermissionDenied`, `BadFileDescriptor`, EIO, etc.). The current loop matches `Err(_)` in both the retry and terminal arms, so any non-contention failure is busy-spun until `LOCK_WAIT_BUDGET` expires and then surfaced to callers as `FileStoreError::Busy`. This violates the variant's documented contract ("another process is mid-write, retry") and hides real fs failures (permissions/quota/tampered sidecar) behind a transient retry signal, prompting operators to retry on errors that retry cannot fix. The `FileStoreError::Io(io::Error)` variant already exists via `#[from]`; only `ErrorKind::WouldBlock` should map to `Busy`, everything else should propagate as `Io`.
In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/store.rs:123-130: build_os bypasses the documented label allowlist on the OS-keyring arm
The secrets module documents a reject-not-sanitize label contract (`^[A-Za-z0-9._-]{1,64}$`, enforced in `secrets/validate.rs`) intended to fire before any backend maps a label to a filename or keyring attribute (CWE-22/CWE-20). The File arm enforces it at every entry point (put_bytes/get_bytes/delete_bytes), but `build_os` — the sole entry path for `SecretStore::Os::{set,get,delete}` — hands the raw caller-supplied `label` directly to `store.build(&svc, label, None)`. Whatever validation happens after that is per-backend (macOS Keychain, Windows Credential Manager, libsecret, linux-keyutils all differ), so labels containing whitespace, control characters, path separators, or oversize strings that the File arm rejects may be accepted on some platforms. This breaks the cross-backend invariant that the same `(service, label)` resolves identically and weakens the OS-keyring trust boundary. Enforce `validated_label` in `build_os` so the policy is uniform across arms.
Review details
🤖 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/file/mod.rs`:
- [SUGGESTION] lines 259-267: with_vault_lock collapses every try_write io::Error into Busy
`fd_lock::RwLock::try_write` returns a plain `io::Error` whose `ErrorKind` distinguishes lock contention (`WouldBlock`) from real I/O faults (`PermissionDenied`, `BadFileDescriptor`, EIO, etc.). The current loop matches `Err(_)` in both the retry and terminal arms, so any non-contention failure is busy-spun until `LOCK_WAIT_BUDGET` expires and then surfaced to callers as `FileStoreError::Busy`. This violates the variant's documented contract ("another process is mid-write, retry") and hides real fs failures (permissions/quota/tampered sidecar) behind a transient retry signal, prompting operators to retry on errors that retry cannot fix. The `FileStoreError::Io(io::Error)` variant already exists via `#[from]`; only `ErrorKind::WouldBlock` should map to `Busy`, everything else should propagate as `Io`.
In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] lines 123-130: build_os bypasses the documented label allowlist on the OS-keyring arm
The secrets module documents a reject-not-sanitize label contract (`^[A-Za-z0-9._-]{1,64}$`, enforced in `secrets/validate.rs`) intended to fire before any backend maps a label to a filename or keyring attribute (CWE-22/CWE-20). The File arm enforces it at every entry point (put_bytes/get_bytes/delete_bytes), but `build_os` — the sole entry path for `SecretStore::Os::{set,get,delete}` — hands the raw caller-supplied `label` directly to `store.build(&svc, label, None)`. Whatever validation happens after that is per-backend (macOS Keychain, Windows Credential Manager, libsecret, linux-keyutils all differ), so labels containing whitespace, control characters, path separators, or oversize strings that the File arm rejects may be accepted on some platforms. This breaks the cross-backend invariant that the same `(service, label)` resolves identically and weakens the OS-keyring trust boundary. Enforce `validated_label` in `build_os` so the policy is uniform across arms.
Reviewed commit: 6ca1bc16
…e, lock io::Error fidelity, OS-keyring label allowlist (CMT-001,002,006) CMT-001 (HIGH): EncryptedFileStore now holds a per-vault passphrase map in addition to the open-time default. `rekey(wallet_id, new_pp)` registers the new passphrase ONLY for that wallet's slot, so a multi-wallet store cannot lock itself out by rekeying one vault. The missing-vault rekey arm becomes a true no-op (no map mutation), eliminating the silent store-wide passphrase swap. Two regression tests cover the multi-wallet isolation and the missing-vault no-op shapes. CMT-002 (MEDIUM): `with_vault_lock` now discriminates `WouldBlock` / `Interrupted` (honest lock contention → retry-then-Busy) from every other `io::Error` (EIO/EBADF/EACCES/ENOSPC → surface the real fault). The previous bare `Err(_)` arm busy-spun real faults until the wait budget elapsed then projected to `Busy`, misleading the SPI consumer into retrying a fatal I/O failure. CMT-006 (MEDIUM): `SecretStore::Os` `build_os` shim now validates the label against the crate's reject-not-sanitize allowlist before handing it to the OS backend. Per-backend label policies (macOS Keychain, Windows Credential Manager, Secret Service, linux-keyutils) differ in what they accept, normalize, or reject; enforcing the allowlist at the shim keeps the `(service, label)` invariant uniform across every arm. A mock store that panics on `build()` proves the allowlist rejection fires before the SPI seam.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for b280ca0: carried forward STILL VALID prior findings: none; fixed/outdated prior findings: #1:with-vault-lock-reports-try-write-io-errors-as-busy, #2:build-os-skips-label-allowlist-before-native-keyring; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Both prior findings are FIXED at b280ca0. with_vault_lock (file/mod.rs:305-336) now retries only WouldBlock/Interrupted, maps those to FileStoreError::Busy after the deadline, and propagates all other io::Error kinds via FileStoreError::from(e) so genuine I/O faults are no longer misclassified as transient. build_os (store.rs:130-138) now calls super::validate::validated_label(label).map_err(FileStoreError::from)? before the SPI seam, with a build_os_rejects_invalid_label_before_spi test asserting the rejection via a panicking mock. The latest push delta (per-vault passphrase override, lock io::Error fidelity, OS-keyring label allowlist) is pure-Rust, well-tested, and introduces no new in-scope defects.
Reviewed commit: b280ca0a
…ingle multi-wallet vault file (CMT-001 v2)
Collapse `EncryptedFileStore` from "directory of per-wallet
`{wallet_id}.pwsvault` files all sharing a store-wide passphrase" to
"one file containing all wallets, sealed under one passphrase,
protected by one file lock". Supersedes the per-vault passphrase map
added in b280ca0 (CMT-001 v1): the multi-wallet lockout and
unknown-vault-passphrase-swap bug classes are now impossible by
construction — there is only ever one passphrase to be in the wrong
state.
On-disk shape bumps to format version 3:
- `Vault.wallets: BTreeMap<wallet_id_hex, BTreeMap<label, EntryBody>>`
(nested), replacing the per-wallet file with a single document.
- KDF salt and verify-token are store-wide; `verify_aad` no longer
takes a `wallet_id` argument and binds to a NUL-prefixed sentinel
label so it stays disjoint from every allowlisted entry AAD.
- Entry AAD is unchanged: still
`format_version ‖ wallet_id ‖ label`, so a cross-wallet ciphertext
swap still fails the tag (new test
`blob_swap_across_wallet_is_rejected`).
- No migration path: format v2 files are rejected by the existing
`VersionUnsupported` branch. Backward compatibility was explicitly
not required for this unmerged PR.
Public API changes:
- `EncryptedFileStore::open(path, passphrase)` and
`SecretStore::file(path, passphrase)` now take the vault FILE path
(operator picks the filename); the parent directory is materialized
on first write. No more directory-owning behaviour.
- `rekey(new_passphrase)` rotates the whole store atomically — no
`wallet_id` argument.
- `put_bytes` / `get_bytes` / `delete_bytes` signatures unchanged;
only internals change (read whole vault → mutate
`vault.wallets[service][label]` → write whole vault, under one lock).
Internals:
- Single `passphrase: Mutex<SecretString>` swapped atomically with the
on-disk re-encrypt under `with_vault_lock`.
- One `rmw_mutex` and one `<vault-path>.lock` sidecar guard every
RMW. Sidecar (not direct fd-lock on the vault) so the
`tempfile::NamedTempFile::persist` rename never touches the inode an
open lock fd points at.
- Empty wallet entry maps are removed on delete so the on-disk shape
stays clean.
- `EncryptedFileStoreInner.dir` + per-wallet `vault_path` / per-wallet
lockfile / `passphrases` map / `effective_passphrase` helper / `locks`
map all deleted.
Tests:
- Two CMT-001-v1 regression tests removed (the bug class they guarded
cannot exist in the new model).
- Three replacements:
- `rekey_rotates_whole_store_atomically` — multi-wallet put,
rekey once, both wallets read under the new passphrase, both
fail under the old.
- `rekey_does_not_corrupt_on_disk_temp_failure` — denies write
permission mid-rekey; the original vault is byte-identical
afterward and still readable under the old passphrase.
- `concurrent_writers_serialize_via_lock` — two threads
`put_bytes` different `(wallet_id, label)` pairs, both land.
- Existing tests updated to the new single-file shape
(`test_vault_path()` removed, `test_read_vault()` / `test_write_vault()`
take no path).
- New `blob_swap_across_wallet_is_rejected` guard so the
cross-wallet AAD binding is exercised on the nested-map shape.
Verification: `cargo fmt`, `cargo clippy -- -D warnings`,
`cargo test --all-features` all clean (97 lib tests, all integration
suites green).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…istor' into feat/platform-wallet-storage-secrets
lklimek
reviewed
May 28, 2026
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at a9f12d2. Prior findings status: prior review reported none — carried-forward prior findings: none; fixed/outdated/intentionally deferred prior findings: none. New findings in latest delta: one blocking path-handling bug introduced by the new file-path API (bare relative filenames break first-write on Unix dir-fsync), plus two defense-in-depth suggestions (parent-dir 0700 tightening was dropped without a documented replacement; vault deserialization does not validate map-key shapes the rest of the code enforces). The single-file collapse itself is well-disciplined: cross-wallet AAD binding, whole-store atomic rekey under one lock, sidecar-lock fidelity, and size-cap pre-read are all preserved.
🔴 1 blocking | 🟡 2 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/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:417-437: Bare relative vault filename breaks first write because parent is left as the empty path
For a caller-supplied path with no directory component (e.g. `EncryptedFileStore::open("vault.pwsvault", ...)`), `self.path.parent()` returns `Some(Path::new(""))`, not `None`. The current code therefore keeps `parent` as the empty path, the `!parent.as_os_str().is_empty()` guard skips `fs::create_dir_all`, and the Unix dir-fsync `fs::File::open(parent)?` at line 435 then fails with ENOENT. `with_vault_lock` has the same empty-path guard at line 254-258, so the failure surfaces on the very first write rather than at open time, making a bare relative filename a valid-looking API input that silently never succeeds. Normalize the empty parent to `Path::new(".")` so first-write succeeds in the current directory, matching the documented `open(path)` API.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:121-266: Parent-directory 0700 tightening dropped with no replacement defense or operator-doc handoff
The previous head's CMT-002 hardening tightened the secrets directory to 0700 on open, defending the sidecar `.lock` open (a plain `OpenOptions::new()...open(&lock_path)` that follows symlinks, unlike the vault file's `open_no_follow`). The new path-based API removes that tightening entirely without an `O_NOFOLLOW` open on the lock file or operator-facing documentation that the chosen parent dir must be 0700. If an operator drops the vault under a writable parent dir, a same-UID adversary can pre-place `<vault>.lock` as a symlink and force `set_restrictive_perms` to chmod, plus flock, an unrelated victim file during any RMW. Pick one of: (a) reinstate a small Unix-only `set_restrictive_dir_perms` helper invoked on `open`, (b) open the lock fd with `O_NOFOLLOW` mirroring `open_no_follow`, or (c) state the operator's parent-dir-mode responsibility in the `EncryptedFileStore::open` rustdoc and SECRETS.md — currently the doc only mentions the file's 0600 mode.
In `packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/format.rs:231-255: deserialize does not validate Vault.wallets map-key invariants the rest of the crate enforces
`Vault.wallets: BTreeMap<String, BTreeMap<String, EntryBody>>` accepts any UTF-8 string as outer (wallet-id-hex) or inner (label) key. `deserialize` only enforces AEAD-tag-length floors. Consequences: a vault with malformed outer keys is silently readable/writable across `get`/`put`/`delete` and only surfaces a `MalformedVault` error when `rekey` happens to iterate and run `decode_wallet_id_hex`; malformed inner labels are similarly tolerated since lookups go through `validated_label`-cleaned inputs and just miss (returning `Ok(None)` instead of fail-closed). AEAD tag still binds correctly (the bad key rides through `aad()`), so this is not a confidentiality/integrity issue — but it is a defence-in-depth gap that masks tamper/corruption and is inconsistent with the format layer's existing fail-closed posture (`hex_array`, `deny_unknown_fields`, tag-length floors). Validating outer-key shape (length 64, lowercase, hex) and inner-key allowlist at parse time surfaces tamper immediately and matches the rest of the format layer.
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for 5041aa1: carried forward STILL VALID prior findings: #1:bare-relative-vault-filename-empty-parent-first-write, #2:parent-directory-0700-tightening-dropped, #3:deserialize-misses-vault-wallet-map-key-invariants; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Latest push (a9f12d2..5041aa1) is a pure upstream merge that touches zero files under packages/rs-platform-wallet-storage. All three prior findings remain valid verbatim at current head and are carried forward. No new in-scope defects introduced by the delta.
🔴 1 blocking | 🟡 2 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/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:413-438: Bare relative vault filename still breaks first write — empty parent reaches NamedTempFile and Unix dir-fsync
For a caller-supplied path with no directory component (e.g. `EncryptedFileStore::open("vault.pwsvault", ...)`, explicitly supported per mod.rs:115-120), `self.path.parent()` returns `Some(Path::new(""))`, not `None`. The `unwrap_or_else(|| Path::new("."))` branch at line 417 therefore never fires and `parent` is the empty path. The `!parent.as_os_str().is_empty()` guard at line 420 correctly skips `create_dir_all`, but `tempfile::NamedTempFile::new_in(parent)` at line 423 and especially `fs::File::open(parent)` at line 435 (Unix parent-dir fsync) still receive the empty path. The Unix dir-fsync `fs::File::open("")` returns ENOENT, so `do_write_vault` returns `FileStoreError::Io(ENOENT)` even though the atomic `persist` at line 430 already swapped the new vault into place — the documented bare-filename API appears to fail on the first write while the data is in fact persisted, driving callers to retry and re-derive Argon2id needlessly. Both call sites need the same defaulting `with_vault_lock` uses at lines 254-258. Filter out the empty parent before defaulting so `create_dir_all`, the temp-file creation, and the parent-dir fsync all resolve to `.`.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:250-266: Parent-directory 0700 tightening dropped; sidecar `.lock` opened without O_NOFOLLOW or parent-mode check
The earlier CMT-002 hardening tightened the secrets directory to 0700 on open. The current single-vault path API no longer checks or tightens the parent directory: `open()` (lines 121-129) is a trivial pass-through, `with_vault_lock` only `create_dir_all`s the sidecar's parent (lines 254-258), and `do_write_vault` similarly creates the vault's parent without mode hardening. The vault file itself is `O_NOFOLLOW`+0600 hardened, but the sidecar lock at lines 259-264 is opened with plain `OpenOptions::open(&lock_path).create(true).truncate(false)` — no `O_NOFOLLOW`, no pre-open mode check. On a multi-user host where the operator-chosen path lands under a group/world-writable directory, a co-resident attacker can pre-create the `.lock` path as a symlink to attacker-controlled inode; this process then `chmod(0600)`s the attacker's target via `set_restrictive_perms` and takes the advisory lock on the wrong inode, letting two writers race the RMW that CMT-001 was meant to close. Either restore Unix parent-directory tightening/checking for the parent containing the vault+sidecar, harden the sidecar open path with `OpenOptionsExt::custom_flags(libc::O_NOFOLLOW)` plus a mode check, or surface the trust boundary explicitly in SECRETS.md so operators know the parent ACL is their responsibility.
In `packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/format.rs:231-255: deserialize does not validate Vault.wallets outer wallet-id hex or inner label invariants
`Vault.wallets: BTreeMap<String, BTreeMap<String, EntryBody>>` accepts any UTF-8 string as outer wallet-id key and any string as inner label. `deserialize` (lines 231-255) validates `verify_ct` tag-floor and iterates `vault.wallets.values()` to enforce the per-entry AEAD-tag floor, but never validates the keys themselves — neither that outer keys are 64-char lowercase wallet-id hex (the shape `WalletId::to_hex` produces) nor that labels satisfy `validated_label`. A malformed vault is parsed into an in-memory shape downstream code assumes was produced by validated constructors. While `get`/`put`/`delete` look up by `wallet_id.to_hex()` and silently miss malformed-key entries (self-quarantining for reads), `rekey` at mod.rs:457-458 walks every wallet and calls `decode_wallet_id_hex` per-key — a single bad key denies the whole rotation. Fail closed at the format seam to keep wire-format invariants local to the format layer.
Review details
🤖 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/file/mod.rs`:
- [BLOCKING] lines 413-438: Bare relative vault filename still breaks first write — empty parent reaches NamedTempFile and Unix dir-fsync
For a caller-supplied path with no directory component (e.g. `EncryptedFileStore::open("vault.pwsvault", ...)`, explicitly supported per mod.rs:115-120), `self.path.parent()` returns `Some(Path::new(""))`, not `None`. The `unwrap_or_else(|| Path::new("."))` branch at line 417 therefore never fires and `parent` is the empty path. The `!parent.as_os_str().is_empty()` guard at line 420 correctly skips `create_dir_all`, but `tempfile::NamedTempFile::new_in(parent)` at line 423 and especially `fs::File::open(parent)` at line 435 (Unix parent-dir fsync) still receive the empty path. The Unix dir-fsync `fs::File::open("")` returns ENOENT, so `do_write_vault` returns `FileStoreError::Io(ENOENT)` even though the atomic `persist` at line 430 already swapped the new vault into place — the documented bare-filename API appears to fail on the first write while the data is in fact persisted, driving callers to retry and re-derive Argon2id needlessly. Both call sites need the same defaulting `with_vault_lock` uses at lines 254-258. Filter out the empty parent before defaulting so `create_dir_all`, the temp-file creation, and the parent-dir fsync all resolve to `.`.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 250-266: Parent-directory 0700 tightening dropped; sidecar `.lock` opened without O_NOFOLLOW or parent-mode check
The earlier CMT-002 hardening tightened the secrets directory to 0700 on open. The current single-vault path API no longer checks or tightens the parent directory: `open()` (lines 121-129) is a trivial pass-through, `with_vault_lock` only `create_dir_all`s the sidecar's parent (lines 254-258), and `do_write_vault` similarly creates the vault's parent without mode hardening. The vault file itself is `O_NOFOLLOW`+0600 hardened, but the sidecar lock at lines 259-264 is opened with plain `OpenOptions::open(&lock_path).create(true).truncate(false)` — no `O_NOFOLLOW`, no pre-open mode check. On a multi-user host where the operator-chosen path lands under a group/world-writable directory, a co-resident attacker can pre-create the `.lock` path as a symlink to attacker-controlled inode; this process then `chmod(0600)`s the attacker's target via `set_restrictive_perms` and takes the advisory lock on the wrong inode, letting two writers race the RMW that CMT-001 was meant to close. Either restore Unix parent-directory tightening/checking for the parent containing the vault+sidecar, harden the sidecar open path with `OpenOptionsExt::custom_flags(libc::O_NOFOLLOW)` plus a mode check, or surface the trust boundary explicitly in SECRETS.md so operators know the parent ACL is their responsibility.
In `packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- [SUGGESTION] lines 231-255: deserialize does not validate Vault.wallets outer wallet-id hex or inner label invariants
`Vault.wallets: BTreeMap<String, BTreeMap<String, EntryBody>>` accepts any UTF-8 string as outer wallet-id key and any string as inner label. `deserialize` (lines 231-255) validates `verify_ct` tag-floor and iterates `vault.wallets.values()` to enforce the per-entry AEAD-tag floor, but never validates the keys themselves — neither that outer keys are 64-char lowercase wallet-id hex (the shape `WalletId::to_hex` produces) nor that labels satisfy `validated_label`. A malformed vault is parsed into an in-memory shape downstream code assumes was produced by validated constructors. While `get`/`put`/`delete` look up by `wallet_id.to_hex()` and silently miss malformed-key entries (self-quarantining for reads), `rekey` at mod.rs:457-458 walks every wallet and calls `decode_wallet_id_hex` per-key — a single bad key denies the whole rotation. Fail closed at the format seam to keep wire-format invariants local to the format layer.
Reviewed commit: 5041aa1e
…n, in-memory state, write-back-on-mutate (lklimek review) Supersedes the per-op-lock model from a9f12d2. Addresses three review threads on PR #3672: - "let's go back to version 1, we haven't merged yet, no need to bump version for now" — reverts FORMAT_VERSION 3 → 1. - "let's create the file lock file on vault open, and drop on drop of the vault. We don't want to support concurrency here. Use flock or similar existing crate that is supported both on Windows and Linux/Mac" — single try_write at open(), no retry loop, no wait budget. fd-lock (already in deps) provides the cross-platform primitive (flock on Unix, LockFileEx on Windows). - "constructors — instantiate vault, load to memory; destructor — saves vault from memory, re-sets perms, releases lock; modification operations — saves vault from memory + sync" — resident decrypted Vault on EncryptedFileStoreInner, eager sync on every mutation, drop-time best-effort sync + perm re-assert, lock released when the inner drops. User-facing changes: - `EncryptedFileStore::open(path, passphrase)` now always returns a usable store: a missing vault is created (random salt + fresh verify-token + 0600), an existing vault is loaded + verify-token checked. A wrong passphrase surfaces at open() rather than on the first get(). - The vault file is created on the first open(), not on the first put(). Public callers don't notice: the next put/get/delete is identical. - Reads no longer hit disk (the BTreeMap is resident) and no longer pay the Argon2 cost per call — the derived key is cached on open and zeroized on drop (A3 stays an accepted threat per module docs). - `Busy` renamed → `AlreadyLocked`. Old semantics were "retry-after- contention"; new semantics are "another store handle owns this path, fail fast". Both ride in NoStorageAccess at the SPI seam and remain losslessly typed on the SecretStore path. Implementation notes: - VaultLock wraps fd-lock with a boxed-RwLock + Box::leak pattern in a tiny allow-unsafe submodule (the rest of the crate keeps deny(unsafe_code)). The self-reference is unavoidable: the guard borrows the RwLock and both must live for the store's lifetime. Drop order is explicit (guard first → flock(LOCK_UN); then Box::from_raw → fd close). - Both intra-process and inter-process exclusion fall out of flock's per-open-file-description semantics: two separate open() calls produce distinct fds and the second's flock() returns EWOULDBLOCK, mapped to AlreadyLocked. - rekey rolls back the in-memory swap on disk-write failure so the live handle keeps serving the old data through the old key. - Eager sync over lazy sync (per brief's recommendation): every mutation immediately writes; drop just belt-and-suspenders. Tests delta: dropped `vault_lock_contention_surfaces_busy` (per-op lock gone) and `rekey_with_outstanding_credential_returns_busy_not_panic` (Arc::get_mut Busy path gone); added `open_creates_vault_file_on_first_open`, `open_acquires_exclusive_lock_until_drop`, `mutations_are_visible_after_reopen`, `wrong_passphrase_on_reopen_fails_with_typed_error`, `already_locked_surfaces_typed_lossless` (store.rs). `secrets_api.rs` adapted: wrong-pass now surfaces at open(), not on get(). 95 lib + all integration tests pass. SECRETS.md: future-work section for the planned maintenance CLI (`secrets probe`, `secrets list`) — out of scope for #3672. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…istor' into feat/platform-wallet-storage-secrets # Conflicts: # packages/rs-platform-wallet-ffi/src/persistence.rs
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review against e03c500. Prior #1 (bare-relative filename + Unix parent-dir fsync at do_write_vault_at) is STILL VALID — Path::new("vault.pwsvault").parent() returns Some(""), so the unwrap_or_else(|| ".") fallback never fires and fs::File::open(parent) at line 644 will fail with ENOENT on first write. Prior #2 (lock sidecar opened without O_NOFOLLOW, parent-directory mode untightened) is STILL VALID. Prior #3 (format::deserialize does not validate wallet-id hex outer keys or label inner keys) is STILL VALID. No prior finding is FIXED. New in the resident-vault delta: rekey()'s three separate mutex acquisitions for vault/derived_key/passphrase (lines 355–366) plus credentials that hold an independent Arc<EncryptedFileStoreInner> create a torn-state window where a racing put() seals under the old key and then writes that ciphertext into the just-rotated new-key vault — permanent data loss on the raced entry; the AB/BA deadlock claim from codex-rust-quality is not real (no nested lock holds) but the torn-state race is. A related, narrower bug: put()'s rollback expect("entry just inserted") at line 410 can panic if a concurrent delete() on the same label empties the wallet slot between insert and rollback. Total: 2 blocking + 3 suggestions in scope.
🔴 1 blocking | 🟡 2 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:316-405: rekey() races concurrent credential operations across split mutexes — old-key ciphertext can be persisted into the new-key vault
New issue introduced by the resident-vault refactor. `EncryptedFileStoreInner` now holds resident state across three independent mutexes (`vault`, `derived_key`, `passphrase` at lines 109–123). `rekey()` swaps them in three back-to-back `std::mem::replace` calls at lines 355–366, each acquiring and releasing its own lock. Meanwhile `EncryptedFileCredential` is built with a cloned `Arc<EncryptedFileStoreInner>` at lines 805/874-889, so credentials handed out on other threads can call `put`/`get`/`delete` concurrently with rekey even though `EncryptedFileStore::rekey` takes `&mut self`.
The concrete failure mode is data loss, not just a spurious Corruption: `put()` at lines 391–403 acquires the `derived_key` lock, snapshots a `(nonce, ciphertext)` sealed under that key, releases the key lock, then later acquires the `vault` lock and inserts into the resident vault. If `rekey()` swaps both the vault and the derived key in the gap between those two acquisitions, the racing `put()` writes an old-key sealed entry into the freshly rotated new-key vault, and the subsequent `sync_to_disk()` persists it. Future `get()` on that slot fails with `Corruption` permanently — the original plaintext is unrecoverable.
Symmetric weaker windows exist for `get()` (vault-clone happens before key-lock at lines 447–457, so a rekey landing between can observe new-key body decrypted with old key → false Corruption) and for the rollback path at lines 368–372 (three separate lock acquisitions, same torn-state shape).
Note: codex-rust-quality also flagged an AB/BA deadlock between `put` and `get`; that part is not real because neither method holds two of these locks simultaneously (each scope drops one lock before acquiring the next). The torn-state race, however, is real and is the load-bearing concern. The fix is to collapse `vault`/`derived_key`/`passphrase` under a single `Mutex<ResidentState>` (or hold one outer state mutex continuously across rekey including `sync_to_disk`), so the rotation and every credential op are mutually atomic.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:405-423: put() rollback panics under concurrent delete — `.expect("entry just inserted")` is not concurrency-safe
The vault mutex is released at line 403 between `put`'s insert and the rollback re-acquisition at line 406. A concurrent `delete()` on the same `(wallet_id, label)` can win that gap, observe `entries.is_empty()` after removal at line 484, and remove the wallet slot itself at line 486. When `put`'s rollback then re-acquires the lock and calls `vault.wallets.get_mut(&wallet_id.to_hex()).expect("entry just inserted")` at line 410, the slot is gone and the expect aborts the process — violating the crate's no-unjustified-`expect` policy and abending with secrets still in memory.
This is narrow (requires `sync_to_disk` failure on `put` AND a same-label race), but the fix is mechanical: use `vault.wallets.entry(wallet_hex).or_default()` in the rollback (same shape as the insert path), or hold the vault lock continuously across insert → sync → rollback. The broader concurrency model in finding #2 likely subsumes this once the resident state collapses under a single mutex.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:692-732: Lock sidecar opened without O_NOFOLLOW; parent-directory mode untightened (carried forward — STILL VALID)
Prior review finding #2 against 5041aa1e is unresolved at e03c500e. `VaultLock::acquire` at lines 693–699 opens `<vault>.lock` with plain `OpenOptions::new().read(true).write(true).create(true)...open(lock_path)?` — no `custom_flags(libc::O_NOFOLLOW)`. The vault-data read path is hardened (`read_vault_at` at line 588 uses `open_no_follow`); the sidecar is not, despite the sidecar lock being the entire cross-process exclusion mechanism in the resident-vault model. A local peer who can write in the parent directory can pre-create `<vault>.lock` as a symlink, and the subsequent `set_restrictive_perms(&lock_file)` (which fchmods the fd) will run on the symlink target — a redirect/lock-poisoning primitive against any file the wallet process owns. Symmetrically, the prior parent-directory 0700 tightening was dropped: `open()` at lines 153–157 only `create_dir_all`s and never validates or tightens the parent dir mode, so operators rely purely on umask for the directory that anchors both the lock and the vault. Both items are Unix defence-in-depth; neither breaks the happy path.
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for aab00e6: carried forward STILL VALID prior findings: #1:bare-relative-vault-filename-first-write-unix, #2:resident-vault-rekey-split-mutex-race-old-key-ciphertext, #3:put-rollback-expect-panics-after-concurrent-delete, #4:lock-sidecar-opened-without-no-follow-parent-mode-untightened, #5:deserialize-misses-wallet-key-label-invariant-validation; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Latest push delta is a trivial FFI cleanup in packages/rs-platform-wallet-ffi/src/persistence.rs (drops needless .to_string() on static error literals) and introduces no new findings. All five prior findings against the new encrypted-file secret store remain reproducible at HEAD aab00e6 and are carried forward: two blocking (bare-filename first-write hole; rekey split-mutex race) and three suggestions (put() rollback expect-panic under concurrent delete; lock sidecar opened without O_NOFOLLOW plus untightened parent dir; format::deserialize missing outer wallet-key and inner label validation). Both Claude and Codex (general, security, and rust-quality variants) converge on the same five issues against the current head.
🔴 2 blocking | 🟡 3 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/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:631-648: Bare relative vault filename breaks first write on Unix
`do_write_vault_at` at line 633 uses `path.parent().unwrap_or_else(|| Path::new("."))`, but for a documented bare relative filename like `vault.pwsvault`, `Path::parent()` returns `Some(Path::new(""))`, not `None` — the fallback never fires. The guard at line 634 then correctly skips `create_dir_all`, but `tempfile::NamedTempFile::new_in(parent)` at line 637 and the Unix parent-directory fsync `fs::File::open(parent)` at line 644 still receive the empty path and fail with ENOENT. The same anti-pattern exists at `EncryptedFileStore::open` (lines 153-157), but that path tolerates the empty parent because it never opens the directory directly. Net effect: an operator using a simple relative vault filename in the current working directory cannot initialize the vault — `open()` → `create_new_vault` → `write_vault_at` → `do_write_vault_at` returns an I/O error before any bytes are persisted. Replicates against both `do_write_vault_at` and `open()`'s create branch.
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:316-375: rekey() races concurrent put()/get()/delete() across split mutexes — persists ciphertext that the new key cannot decrypt
`EncryptedFileStoreInner` keeps the resident `vault` (line 123), `derived_key` (line 119), and `passphrase` (line 109) behind three independent `Mutex`es. `rekey()` builds a fresh vault + key, then installs the new state via three separate `std::mem::replace` calls at lines 355, 359, and 363, each acquiring a different mutex, and then calls `sync_to_disk()` at line 368 — which itself only re-locks `vault` (line 312). A concurrent `put()` running on another thread of the same `Arc<EncryptedFileStoreInner>` can interleave inside that swap window: at line 392 `put()` locks `derived_key` and seals under whichever key is currently installed, then at line 400 locks `vault` separately and inserts the resulting `EntryBody`. If the vault swap (line 355) lands before the key swap (line 359), `put()` can seal with the OLD key and insert into the NEW vault; `sync_to_disk()` then persists a vault whose header verify-token validates the new passphrase but contains an entry whose ciphertext is opaque under the new key. After `rekey()` returns `Ok`, that entry is permanently undecryptable (`FileStoreError::Decrypt` → `Corruption`) — silent data loss. The PR description's claim that 'the in-memory vault, derived key, and passphrase advance together under the resident-state mutexes' is an atomicity guarantee the code does not provide. The same window enables analogous tearing modes for `delete()` rollback and `get()`. This is a real availability/data-loss bug because the public API is intentionally exposed through cloneable `Arc`-backed handles and the iOS/Swift integration explicitly uses concurrent stores.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:405-425: put() rollback panics via `.expect("entry just inserted")` after a concurrent delete
`put()` releases the `vault` mutex at the end of the block ending at line 403 and `sync_to_disk()` (line 405) re-locks it only transiently. On disk-write failure the rollback at line 406 reacquires the mutex and calls `vault.wallets.get_mut(&wallet_id.to_hex()).expect("entry just inserted")` at line 407-410. Between the original insert and the rollback, a concurrent `delete()` on the same `(wallet_id, label)` (or any delete that empties the wallet's entry map) can remove the entry and then remove the empty wallet slot at lines 484-486. The expect then panics, poisoning the vault mutex and turning a recoverable I/O failure into a process abort. The rollback should tolerate a missing wallet slot — recreate it via `entry(...).or_default()` when restoring a prior body, and no-op when removing a newly inserted entry whose slot is already gone.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:692-732: Lock sidecar opened without O_NOFOLLOW; vault parent directory mode not tightened
`VaultLock::acquire` (lines 693-699) opens the `<vault>.lock` sidecar with plain `fs::OpenOptions::new().read(true).write(true).create(true).truncate(false).open(lock_path)` and only applies `set_restrictive_perms` after the open succeeds. The vault data read path uses `open_no_follow` with `OpenOptionsExt::custom_flags(libc::O_NOFOLLOW)` (see `read_vault_at`), but the lock sidecar does not, so a same-host attacker with write access to the parent directory can pre-create a symlink at `<vault>.lock` and redirect both the lock open and the subsequent chmod to an inode they choose. Separately, both `EncryptedFileStore::open` (mod.rs:153-157) and `do_write_vault_at` (mod.rs:633-636) call `fs::create_dir_all(parent)` under the default umask and never tighten the resulting directory mode. The PR description claims CMT-002 tightens vault dirs to `0700` on open with looser dirs tightened in place + logged at warn, but no `set_permissions`/`PermissionsExt::from_mode(0o700)` call on the parent dir exists at HEAD — the per-file `0600` hardening can sit inside a group/world-traversable parent.
In `packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/format.rs:231-255: format::deserialize does not validate outer wallet-id hex or inner label invariants
`deserialize()` validates `version` (line 234), the AEAD-tag-length floor on `verify_ct` (line 242), and the same floor on each entry's ciphertext (line 248), but the loop at line 246 iterates only `vault.wallets.values()` and never inspects either (a) the outer wallet-map key — a free-form `String` at the type level, which the write path constrains to 64-char lowercase canonical hex via `WalletId::to_hex()` — or (b) the inner labels, which the write path runs through `validated_label`. Malformed/non-canonical wallet ids and labels containing forbidden bytes (NUL, slashes) therefore persist into the resident vault until much later operations trip `decode_wallet_id_hex` or AAD construction errors. Because lookup uses lowercase `wallet_id.to_hex()` plus `validated_label`, an attacker-controlled vault file can also smuggle hidden namespaces that the public API can never address but that `rekey()` will preserve and re-encrypt. Defense-in-depth fix: enforce the same write-side invariants at the parse boundary so a tampered file fails closed at the same seam as the version/tag-length checks.
Review details
🤖 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/file/mod.rs`:
- [BLOCKING] lines 631-648: Bare relative vault filename breaks first write on Unix
`do_write_vault_at` at line 633 uses `path.parent().unwrap_or_else(|| Path::new("."))`, but for a documented bare relative filename like `vault.pwsvault`, `Path::parent()` returns `Some(Path::new(""))`, not `None` — the fallback never fires. The guard at line 634 then correctly skips `create_dir_all`, but `tempfile::NamedTempFile::new_in(parent)` at line 637 and the Unix parent-directory fsync `fs::File::open(parent)` at line 644 still receive the empty path and fail with ENOENT. The same anti-pattern exists at `EncryptedFileStore::open` (lines 153-157), but that path tolerates the empty parent because it never opens the directory directly. Net effect: an operator using a simple relative vault filename in the current working directory cannot initialize the vault — `open()` → `create_new_vault` → `write_vault_at` → `do_write_vault_at` returns an I/O error before any bytes are persisted. Replicates against both `do_write_vault_at` and `open()`'s create branch.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [BLOCKING] lines 316-375: rekey() races concurrent put()/get()/delete() across split mutexes — persists ciphertext that the new key cannot decrypt
`EncryptedFileStoreInner` keeps the resident `vault` (line 123), `derived_key` (line 119), and `passphrase` (line 109) behind three independent `Mutex`es. `rekey()` builds a fresh vault + key, then installs the new state via three separate `std::mem::replace` calls at lines 355, 359, and 363, each acquiring a different mutex, and then calls `sync_to_disk()` at line 368 — which itself only re-locks `vault` (line 312). A concurrent `put()` running on another thread of the same `Arc<EncryptedFileStoreInner>` can interleave inside that swap window: at line 392 `put()` locks `derived_key` and seals under whichever key is currently installed, then at line 400 locks `vault` separately and inserts the resulting `EntryBody`. If the vault swap (line 355) lands before the key swap (line 359), `put()` can seal with the OLD key and insert into the NEW vault; `sync_to_disk()` then persists a vault whose header verify-token validates the new passphrase but contains an entry whose ciphertext is opaque under the new key. After `rekey()` returns `Ok`, that entry is permanently undecryptable (`FileStoreError::Decrypt` → `Corruption`) — silent data loss. The PR description's claim that 'the in-memory vault, derived key, and passphrase advance together under the resident-state mutexes' is an atomicity guarantee the code does not provide. The same window enables analogous tearing modes for `delete()` rollback and `get()`. This is a real availability/data-loss bug because the public API is intentionally exposed through cloneable `Arc`-backed handles and the iOS/Swift integration explicitly uses concurrent stores.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 405-425: put() rollback panics via `.expect("entry just inserted")` after a concurrent delete
`put()` releases the `vault` mutex at the end of the block ending at line 403 and `sync_to_disk()` (line 405) re-locks it only transiently. On disk-write failure the rollback at line 406 reacquires the mutex and calls `vault.wallets.get_mut(&wallet_id.to_hex()).expect("entry just inserted")` at line 407-410. Between the original insert and the rollback, a concurrent `delete()` on the same `(wallet_id, label)` (or any delete that empties the wallet's entry map) can remove the entry and then remove the empty wallet slot at lines 484-486. The expect then panics, poisoning the vault mutex and turning a recoverable I/O failure into a process abort. The rollback should tolerate a missing wallet slot — recreate it via `entry(...).or_default()` when restoring a prior body, and no-op when removing a newly inserted entry whose slot is already gone.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 692-732: Lock sidecar opened without O_NOFOLLOW; vault parent directory mode not tightened
`VaultLock::acquire` (lines 693-699) opens the `<vault>.lock` sidecar with plain `fs::OpenOptions::new().read(true).write(true).create(true).truncate(false).open(lock_path)` and only applies `set_restrictive_perms` after the open succeeds. The vault data read path uses `open_no_follow` with `OpenOptionsExt::custom_flags(libc::O_NOFOLLOW)` (see `read_vault_at`), but the lock sidecar does not, so a same-host attacker with write access to the parent directory can pre-create a symlink at `<vault>.lock` and redirect both the lock open and the subsequent chmod to an inode they choose. Separately, both `EncryptedFileStore::open` (mod.rs:153-157) and `do_write_vault_at` (mod.rs:633-636) call `fs::create_dir_all(parent)` under the default umask and never tighten the resulting directory mode. The PR description claims CMT-002 tightens vault dirs to `0700` on open with looser dirs tightened in place + logged at warn, but no `set_permissions`/`PermissionsExt::from_mode(0o700)` call on the parent dir exists at HEAD — the per-file `0600` hardening can sit inside a group/world-traversable parent.
In `packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- [SUGGESTION] lines 231-255: format::deserialize does not validate outer wallet-id hex or inner label invariants
`deserialize()` validates `version` (line 234), the AEAD-tag-length floor on `verify_ct` (line 242), and the same floor on each entry's ciphertext (line 248), but the loop at line 246 iterates only `vault.wallets.values()` and never inspects either (a) the outer wallet-map key — a free-form `String` at the type level, which the write path constrains to 64-char lowercase canonical hex via `WalletId::to_hex()` — or (b) the inner labels, which the write path runs through `validated_label`. Malformed/non-canonical wallet ids and labels containing forbidden bytes (NUL, slashes) therefore persist into the resident vault until much later operations trip `decode_wallet_id_hex` or AAD construction errors. Because lookup uses lowercase `wallet_id.to_hex()` plus `validated_label`, an attacker-controlled vault file can also smuggle hidden namespaces that the public API can never address but that `rekey()` will preserve and re-encrypt. Defense-in-depth fix: enforce the same write-side invariants at the parse boundary so a tampered file fails closed at the same seam as the version/tag-length checks.
Reviewed commit: aab00e6b
…istor' into feat/platform-wallet-storage-secrets # Conflicts: # packages/rs-platform-wallet-storage/Cargo.toml
…istor' into feat/platform-wallet-storage-secrets
… of feature-set churn Replace the literal substring assertion against `default = ["sqlite", "cli", "kv"]` with a small parser that pulls the `default = [...]` array out of the `[features]` table and asserts `"kv"` is one of its members. The intent (kv must be on by default) is unchanged; the check no longer breaks every time another default feature is added or reordered — which was already happening at merge time as the `secrets` work landed alongside the sqlite-persistor branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at head 17653ba. The latest delta (aab00e6..17653ba) is the SQLite KV store + base persister merge and does NOT touch packages/rs-platform-wallet-storage/src/secrets/file/. Prior-finding reconciliation: #1 bare-relative-path write failure — STILL VALID (Codex incorrectly marked FIXED; verified at mod.rs:633 — path.parent() returns Some("") for bare filenames, so the Path::new(".") fallback never fires); #2 rekey split-mutex race — STILL VALID; #3 put rollback .expect panic — STILL VALID; #4 lock sidecar O_NOFOLLOW + parent-dir 0700 — STILL VALID; #5 format::deserialize key/label invariants — STILL VALID. No new in-scope defects in the latest delta. All findings below are carried-forward from the prior review.
🔴 1 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:316-375: [carried-forward] rekey() races concurrent put/get/delete across split mutexes — silent data loss and mixed-key corruption (STILL VALID)
Verified at 17653ba8. `EncryptedFileStoreInner` is shared via `Arc`, and `EncryptedFileCredential` methods (`put`/`get`/`delete`) operate on `&self`, so concurrent calls against the same store handle from sibling threads are part of the type's surface. `rekey()` performs the rotation in TWO separate critical sections instead of one:
1. Lines 324–349: takes `vault` + `derived_key` locks, builds `new_vault` re-encrypted under `new_key`, releases both at the close-brace at L349.
2. Lines 355–366: independently re-acquires `vault`, `derived_key`, then `passphrase` and `std::mem::replace`s each one in sequence. Three separate lock acquisitions, no continuous critical section.
Race A (silent write loss): between L349 (locks released) and L355 (vault re-acquired), a concurrent `put()` takes `derived_key` (still OLD), seals at L391-394 under OLD key, takes `vault`, inserts into the resident map, runs `sync_to_disk()` writing the OLD-keyed vault. Rekey then swaps the resident vault wholesale with `new_vault` at L355-358 — the concurrent put's entry is DROPPED from in-memory state and overwritten on disk by the rekey's own `sync_to_disk` at L368. The caller's `put` returned `Ok` but the data is gone.
Race B (mixed-key corruption): between L358 (vault replaced with new_vault, encrypted under new_key) and L362 (derived_key replaced with new_key), the resident vault holds entries sealed under new_key but `derived_key` still holds OLD key. A concurrent `get` in this window calls `crypto::open` with OLD key against a new-key body and surfaces as `FileStoreError::Corruption` — a real-looking integrity error that is actually a transient race. A concurrent `put` in this window seals under OLD key and inserts into new_vault; after rekey completes (verify-token + every other entry under new_key), that one entry permanently fails to decrypt — silent partial data loss with no rollback path because the on-disk vault was successfully written.
The in-process fd-lock does not save this — it excludes other processes, not other threads on the same `Arc<Inner>`. Fix: acquire `vault`, `derived_key`, and `passphrase` together in a single critical section that brackets the entire rekey (decrypt + seal + swap + sync_to_disk + rollback), or fold the three mutexes into one resident-state mutex so the swap is atomic.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:405-425: [carried-forward] put() rollback panics via .expect after a concurrent delete empties the wallet slot (STILL VALID)
Verified at 17653ba8. After the in-memory insert at L399-403, the `vault` mutex is dropped before `sync_to_disk()` runs (it re-takes the lock internally at L312). If a concurrent `delete()` on a sibling credential handle removes that wallet's last entry and empties the entry map, `delete` removes the wallet slot wholesale at L484-487. When `sync_to_disk` then returns Err, the rollback path at L405-422 re-acquires the vault lock and calls `vault.wallets.get_mut(&wallet_id.to_hex()).expect("entry just inserted")` — the slot has been removed, `get_mut` returns `None`, and library code panics. This is a symptom of the same split-lock issue as the rekey race; holding the vault lock continuously across sync_to_disk + rollback would obviate the recovery branch entirely. Minimal fix shown below uses `if let Some(entries)` so the rollback degrades to a benign no-op when concurrent state already removed the wallet.
thepastaclaw
suggested changes
May 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for e005f20: carried forward STILL VALID prior findings: #1:bare-relative-vault-filename-first-write-unix, #2:resident-vault-rekey-split-mutex-race-old-key-ciphertext, #3:put-rollback-expect-panics-after-concurrent-delete, #4:lock-sidecar-opened-without-no-follow-parent-mode-untightened, #5:deserialize-misses-wallet-key-label-invariant-validation; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Latest push delta (17653ba..e005f20) is limited to a CI dependency bump (dorny/paths-filter v3→v4) and a test that no longer hard-codes the literal default-features array — neither touches the secrets/file backend. All five prior findings against secrets/file/mod.rs and secrets/file/format.rs are STILL VALID at the current head and are carried forward: two blocking (bare-relative-filename first-write breakage; rekey split-mutex race that can persist old-key ciphertext under the new header), three suggestion (put rollback expect() panic under concurrent delete; lock sidecar opened without O_NOFOLLOW + parent-dir mode untightened; deserialize missing wallet-id/label invariant validation).
🔴 2 blocking | 🟡 3 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/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:631-648: Bare relative vault filename still breaks first write on Unix
STILL VALID at current head. `do_write_vault_at` derives `let parent = path.parent().unwrap_or_else(|| Path::new("."));` and then guards `create_dir_all` with `if !parent.as_os_str().is_empty()`. For a bare relative filename like `vault.pwsvault`, `Path::parent()` returns `Some(Path::new(""))`, not `None`, so the `"."` fallback never fires; the guard correctly skips `create_dir_all`, but `tempfile::NamedTempFile::new_in(parent)` and the Unix parent-dir fsync `fs::File::open(parent)` at line 644 still receive the empty path and fail with ENOENT/EINVAL. Operators following the natural "pass a filename, run from the wallet's cwd" pattern cannot complete the first write. The same shape repeats at mod.rs:150-157 in `open()` for the lock sidecar parent. Normalize an empty parent to `.` before any path use, not just before `create_dir_all`.
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:316-375: rekey() races concurrent credential operations across split mutexes and can persist undecryptable ciphertext
STILL VALID at current head. `EncryptedFileStoreInner` keeps `vault`, `derived_key`, and `passphrase` behind three independent `Mutex`es. `rekey()` re-encrypts entries into `new_vault` inside one nested-lock block (324-349), releases those locks, then performs three separate `std::mem::replace` swaps each in its own `lock()` (355-366), and finally calls `sync_to_disk()` (368), which re-locks only `vault`. `EncryptedFileStore` shares its `Arc<EncryptedFileStoreInner>` and all mutating methods take `&self`, so concurrent callers are supported. A `put()` thread that acquires `derived_key` (still the old key) between the vault-swap (358) and the key-swap (362), seals an entry with the OLD key, then acquires `vault` (already the NEW vault) and inserts the old-key ciphertext — which then gets persisted by `rekey()`'s `sync_to_disk()` under the new header/verify-token. Subsequent gets fail with `Corruption`/`Decrypt` with no recovery path. `delete()` exhibits a symmetric tearing window. Fix requires holding a single mutation lock across the entire reseal-swap-sync sequence — e.g. one state mutex over `(vault, derived_key, passphrase)`.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:405-423: put() rollback panics on `.expect("entry just inserted")` under concurrent delete
STILL VALID at current head. The vault mutex is released between the insert block (399-403) and the rollback that re-acquires it after `sync_to_disk()` fails (406). A concurrent `delete()` of the same `(wallet_id, label)` can pop the freshly inserted entry and remove the now-empty wallet slot (line 486) in that gap. The rollback's `vault.wallets.get_mut(&wallet_id.to_hex()).expect("entry just inserted")` (408-410) then panics — converting a recoverable disk-write error into a process-level abort with mutex poisoning. Trigger requires a multi-threaded caller plus a disk-write failure (full disk, RO mount, perms loss), which is exactly the scenario the rollback is meant to make safe. Tolerate an already-removed slot and recreate one when restoring a prior value.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:692-732: Lock sidecar opened without O_NOFOLLOW; parent directory mode left at process umask
STILL VALID at current head. `VaultLock::acquire` at mod.rs:694-699 opens `<vault>.lock` with plain `fs::OpenOptions::new().read(true).write(true).create(true).truncate(false).open(lock_path)` and only afterwards applies `set_restrictive_perms`. The vault data read path uses `custom_flags(libc::O_NOFOLLOW)` precisely to prevent a final-component symlink swap from redirecting the fd; the lock sidecar deserves the same protection — a planted `<vault>.lock` symlink in a writable parent can redirect lock creation/opening and the subsequent `set_restrictive_perms` then tightens perms on the attacker-chosen target. Separately, both `EncryptedFileStore::open` (mod.rs:150-157) and `do_write_vault_at` (mod.rs:633-636) materialize parent directories via `fs::create_dir_all(parent)` with no permission tightening or post-check, so the hardened 0600 vault and lock sidecar can live inside a default-umask 0755 parent visible/traversable to other local users.
In `packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/format.rs:231-255: format::deserialize does not validate outer wallet-id hex or inner label invariants
STILL VALID at current head. `deserialize` validates `version` (232-238), the `verify_ct` AEAD-tag floor (242-244), and per-entry `body.ciphertext` floor by iterating only `vault.wallets.values()` (246-252). The outer wallet-id `BTreeMap` keys and the inner label keys are never validated at parse time against the canonical invariants used everywhere else: `decode_wallet_id_hex` (mod.rs:757) requires 64-char lowercase hex decoding to 32 bytes, and `validated_label` (secrets/validate.rs) enforces an allowlist with a length cap. A tampered/corrupt vault with malformed outer keys or labels therefore parses cleanly, gets cached in the resident vault, and only surfaces as `decode_wallet_id_hex` errors during rekey/AAD construction or as silently-skipped lookups in `get`/`delete`. Fail closed at the parse boundary by mirroring the write-side validators.
Review details
🤖 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/file/mod.rs`:
- [BLOCKING] lines 631-648: Bare relative vault filename still breaks first write on Unix
STILL VALID at current head. `do_write_vault_at` derives `let parent = path.parent().unwrap_or_else(|| Path::new("."));` and then guards `create_dir_all` with `if !parent.as_os_str().is_empty()`. For a bare relative filename like `vault.pwsvault`, `Path::parent()` returns `Some(Path::new(""))`, not `None`, so the `"."` fallback never fires; the guard correctly skips `create_dir_all`, but `tempfile::NamedTempFile::new_in(parent)` and the Unix parent-dir fsync `fs::File::open(parent)` at line 644 still receive the empty path and fail with ENOENT/EINVAL. Operators following the natural "pass a filename, run from the wallet's cwd" pattern cannot complete the first write. The same shape repeats at mod.rs:150-157 in `open()` for the lock sidecar parent. Normalize an empty parent to `.` before any path use, not just before `create_dir_all`.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [BLOCKING] lines 316-375: rekey() races concurrent credential operations across split mutexes and can persist undecryptable ciphertext
STILL VALID at current head. `EncryptedFileStoreInner` keeps `vault`, `derived_key`, and `passphrase` behind three independent `Mutex`es. `rekey()` re-encrypts entries into `new_vault` inside one nested-lock block (324-349), releases those locks, then performs three separate `std::mem::replace` swaps each in its own `lock()` (355-366), and finally calls `sync_to_disk()` (368), which re-locks only `vault`. `EncryptedFileStore` shares its `Arc<EncryptedFileStoreInner>` and all mutating methods take `&self`, so concurrent callers are supported. A `put()` thread that acquires `derived_key` (still the old key) between the vault-swap (358) and the key-swap (362), seals an entry with the OLD key, then acquires `vault` (already the NEW vault) and inserts the old-key ciphertext — which then gets persisted by `rekey()`'s `sync_to_disk()` under the new header/verify-token. Subsequent gets fail with `Corruption`/`Decrypt` with no recovery path. `delete()` exhibits a symmetric tearing window. Fix requires holding a single mutation lock across the entire reseal-swap-sync sequence — e.g. one state mutex over `(vault, derived_key, passphrase)`.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 405-423: put() rollback panics on `.expect("entry just inserted")` under concurrent delete
STILL VALID at current head. The vault mutex is released between the insert block (399-403) and the rollback that re-acquires it after `sync_to_disk()` fails (406). A concurrent `delete()` of the same `(wallet_id, label)` can pop the freshly inserted entry and remove the now-empty wallet slot (line 486) in that gap. The rollback's `vault.wallets.get_mut(&wallet_id.to_hex()).expect("entry just inserted")` (408-410) then panics — converting a recoverable disk-write error into a process-level abort with mutex poisoning. Trigger requires a multi-threaded caller plus a disk-write failure (full disk, RO mount, perms loss), which is exactly the scenario the rollback is meant to make safe. Tolerate an already-removed slot and recreate one when restoring a prior value.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 692-732: Lock sidecar opened without O_NOFOLLOW; parent directory mode left at process umask
STILL VALID at current head. `VaultLock::acquire` at mod.rs:694-699 opens `<vault>.lock` with plain `fs::OpenOptions::new().read(true).write(true).create(true).truncate(false).open(lock_path)` and only afterwards applies `set_restrictive_perms`. The vault data read path uses `custom_flags(libc::O_NOFOLLOW)` precisely to prevent a final-component symlink swap from redirecting the fd; the lock sidecar deserves the same protection — a planted `<vault>.lock` symlink in a writable parent can redirect lock creation/opening and the subsequent `set_restrictive_perms` then tightens perms on the attacker-chosen target. Separately, both `EncryptedFileStore::open` (mod.rs:150-157) and `do_write_vault_at` (mod.rs:633-636) materialize parent directories via `fs::create_dir_all(parent)` with no permission tightening or post-check, so the hardened 0600 vault and lock sidecar can live inside a default-umask 0755 parent visible/traversable to other local users.
In `packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- [SUGGESTION] lines 231-255: format::deserialize does not validate outer wallet-id hex or inner label invariants
STILL VALID at current head. `deserialize` validates `version` (232-238), the `verify_ct` AEAD-tag floor (242-244), and per-entry `body.ciphertext` floor by iterating only `vault.wallets.values()` (246-252). The outer wallet-id `BTreeMap` keys and the inner label keys are never validated at parse time against the canonical invariants used everywhere else: `decode_wallet_id_hex` (mod.rs:757) requires 64-char lowercase hex decoding to 32 bytes, and `validated_label` (secrets/validate.rs) enforces an allowlist with a length cap. A tampered/corrupt vault with malformed outer keys or labels therefore parses cleanly, gets cached in the resident vault, and only surfaces as `decode_wallet_id_hex` errors during rekey/AAD construction or as silently-skipped lookups in `get`/`delete`. Fail closed at the parse boundary by mirroring the write-side validators.
Reviewed commit: e005f206
…erral INTENTIONAL The non-Unix `set_restrictive_perms` was an unmarked no-op stub. Add an INTENTIONAL(CMT-008) marker referencing the same follow-up issue as its sibling `check_perms` (#3754), so future reviewers can see the Windows ACL gap is acknowledged-and-tracked rather than overlooked. No behavioral change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…behind a single Mutex<InnerState>
Three issues addressed in one resident-vault refactor:
CMT-001 — rekey/put race ("undecryptable persistence")
Per-field Mutex<Vault> + Mutex<DerivedKey> + Mutex<SecretString> let
a concurrent put() take the OLD derived_key, seal under it, then take
the NEW vault, inserting OLD-key ciphertext into the NEW-key vault.
Replace with one Mutex<InnerState> held for the whole rekey swap and
for every put/get/delete mutation. lock_state() is the sole
acquisition site; sync_to_disk_locked runs while the state lock is
held (cross-process serialization stays on the fd-lock sidecar).
CMT-002 — bare-filename ENOENT on first write
Path::parent() returns Some("") not None for bare relative filenames,
so the unwrap_or_else(|| Path::new(".")) fallback never fired and
NamedTempFile::new_in("") + the Unix parent-dir fsync File::open("")
errored. New normalized_parent() is the single source of truth at
both open() and do_write_vault_at.
CMT-004 — VaultLock symlink redirect + 0755 parent dir
Lock-sidecar open lacked O_NOFOLLOW; create_dir_all used the default
umask. VaultLock::acquire now uses OpenOptionsExt::custom_flags(O_NOFOLLOW)
under cfg(unix); create_parent_dir uses DirBuilderExt::mode(0o700)
recursive. Non-unix paths retain INTENTIONAL(CMT-004) markers tracked
at #3754.
Regression tests:
- rekey_does_not_race_put_into_corruption — 200 put/get iters vs 16
rekeys on shared Arc<Inner>; every get returns the seal'd payload
prefix or a typed error, never garbled bytes.
- open_and_put_with_bare_filename_uses_cwd — bare filename in a
TempDir-cwd succeeds end to end.
- vault_lock_rejects_symlink_at_lock_path (cfg(unix)) — pre-placed
symlink at <vault>.lock causes open to error (ELOOP) instead of
following.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…in format::deserialize (CMT-005)
deserialize previously verified only verify_ct length and per-entry
ciphertext length. The outer wallets BTreeMap keys could be arbitrary
strings and the inner per-entry labels could violate the documented
validate::validated_label allowlist; the parser would accept them and
the failure would surface much later (or never, depending on call
shape).
Walk every (wallet_hex, entries) pair after the ciphertext-length
checks:
- wallet_hex → super::decode_wallet_id_hex (canonical 64-char
lowercase hex contract, returns FileStoreError::MalformedVault)
- each label → super::super::validate::validated_label
(allowlist [A-Za-z0-9._-]{1,64}, mapped to
FileStoreError::InvalidLabel)
Reuses existing error variants (no new taxonomy) and the existing
validation helpers (no logic duplication). decode_wallet_id_hex
visibility widened to pub(super) in the prior commit's refactor.
Regression tests:
- deserialize_rejects_non_hex_wallet_id_key
- deserialize_rejects_short_wallet_id_key
- deserialize_rejects_traversal_label ("../escape" → InvalidLabel)
- deserialize_rejects_oversize_label (65-char label busts 1..=64)
All error at parse time, not at later put/get/delete.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
|
✅ Review complete (commit a0c065b) |
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for e279e25: carried forward STILL VALID prior findings: #3:put-rollback-expect-panics-after-concurrent-delete, #4:lock-sidecar-opened-without-no-follow-parent-mode-untightened; fixed/outdated prior findings: #1:bare-relative-vault-filename-first-write-unix, #2:resident-vault-rekey-split-mutex-race-old-key-ciphertext, #5:deserialize-misses-wallet-key-label-invariant-validation; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Latest delta (e005f20..e279e25) cleanly resolves four of five prior findings outright: the single Mutex collapse fixes the rekey/put split-mutex race and the put-rollback expect panic, normalized_parent fixes the bare-relative-filename Unix write crash, the lock sidecar opens with O_NOFOLLOW + 0700 parent dir on Unix, and format::deserialize now validates outer wallet-id hex and inner labels with regression tests. One prior finding is carried forward as a partial fix: decode_wallet_id_hex still accepts uppercase, asymmetric with parse_service and with WalletId::to_hex's lowercase emission, so a tampered vault can hide ciphertext behind an unaddressable namespace. A second carried suggestion: pre-existing Unix parent directories are documented as intentionally untightened, leaving a defense-in-depth gap if the store is pointed at a group-writable directory. No new latest-delta defects. Windows ACL/reparse-point hardening remains intentionally deferred to #3754.
🟡 2 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/file/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:405-423: put() rollback panics on `.expect("entry just inserted")` under concurrent delete
At the prior head, the vault mutex was released between insert and the rollback that re-acquires it after `sync_to_disk()` fails. A concurrent `delete()` can remove the freshly inserted entry and empty wallet slot, making `.expect("entry just inserted")` panic. Re-validate on current head.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:692-732: Lock sidecar opened without O_NOFOLLOW; parent directory mode left at process umask
At the prior head, `VaultLock::acquire` opened `<vault>.lock` with plain `OpenOptions` and applied restrictive permissions only after open. Parent directories were created without mode hardening. Re-validate whether the lock sidecar now uses no-follow semantics and whether vault parent directories are tightened/verified.
Review details
🤖 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/file/mod.rs`:
- [SUGGESTION] lines 405-423: put() rollback panics on `.expect("entry just inserted")` under concurrent delete
At the prior head, the vault mutex was released between insert and the rollback that re-acquires it after `sync_to_disk()` fails. A concurrent `delete()` can remove the freshly inserted entry and empty wallet slot, making `.expect("entry just inserted")` panic. Re-validate on current head.
In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 692-732: Lock sidecar opened without O_NOFOLLOW; parent directory mode left at process umask
At the prior head, `VaultLock::acquire` opened `<vault>.lock` with plain `OpenOptions` and applied restrictive permissions only after open. Parent directories were created without mode hardening. Re-validate whether the lock sidecar now uses no-follow semantics and whether vault parent directories are tightened/verified.
Reviewed commit: e279e257
Collaborator
|
Correction to my automated review at Current review result is COMMENT with 2 suggestions:
Reconciliation summary: prior findings #1, #2, and #3 are fixed; #4 and #5 are partially fixed with the two suggestions above. No blocking findings in the latest delta. |
lklimek
reviewed
May 29, 2026
…mek) Reviewer judgement: the test adds no behavioural coverage — if `kv` silently drops from the default feature set, the kv integration tests that already exercise the public API would catch it. The standalone manifest-parse assertion was scaffolding around the prior brittle-string-match issue, not a guard worth keeping. Removes `kv_is_in_the_default_feature_set` and its `default_features()` helper (only call site). Sibling `kv_feature_requires_sqlite_in_manifest` left intact (validates a distinct dependency-edge invariant). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…FileStoreInner (lklimek)
The CMT-002 fix introduced a `Mutex<InnerState>` wrapped inside
`EncryptedFileStoreInner`, leaving `path`+`_lock` as immutable
fields outside the mutex. Reviewer noted the two-struct split was
unnecessary indirection: `Arc<Mutex<EncryptedFileStoreInner>>`
directly is cleaner.
Verified `path`+`_lock` are only touched from `sync_to_disk_locked`
(caller already holds the lock), `Drop` (unique ownership), `Debug`
(non-hot), and three test helpers — nothing in a hot path needed
mutex-free access. So everything moves inside the mutex.
Shape after:
- `EncryptedFileStore { inner: Arc<Mutex<EncryptedFileStoreInner>> }`
- `EncryptedFileStoreInner { path, vault, derived_key, passphrase, _lock }`
- `lock_inner(&Arc<Mutex<Inner>>)` is the sole acquisition helper with
poisoned-mutex recovery
- Free helpers `put_locked`/`get_locked`/`delete_locked`/`rekey_locked`
take `&Arc<Mutex<Inner>>`, acquire once, do the full mutation +
`sync_to_disk_locked`
- `EncryptedFileCredential` holds `Arc<Mutex<EncryptedFileStoreInner>>`
and delegates to the free helpers
- `Debug for EncryptedFileStore` acquires the lock to read `path`
(off-hot)
Critical invariant preserved: every entry path acquires the single
mutex exactly once and holds it across the seal/open + on-disk write.
The rekey_does_not_race_put_into_corruption regression still passes
(200 put/get iters concurrent with 16 rekeys, no garbled bytes).
Net: -47 lines after merge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… methods (lklimek)
The free-function indirection (put_locked / get_locked / delete_locked
/ rekey_locked taking &Arc<Mutex<Inner>>) was leftover scaffolding
from the prior InnerState wrapping. After the InnerState collapse it
read as unidiomatic — Rust's mutex-guard idiom is to put the logic on
the inner type as &mut self methods and have the outer type acquire
the lock and delegate.
Shape after:
- EncryptedFileStore::{put_bytes, get_bytes, delete_bytes, rekey} —
thin wrappers that call lock_inner(&self.inner) once and delegate.
- EncryptedFileStoreInner::{put, get, delete, rekey} — the actual
logic; receives &mut self / &self from the MutexGuard deref.
- EncryptedFileCredential now holds EncryptedFileStore (Clone-cheap —
the wrapper struct is a single Arc<Mutex<Inner>> field) and delegates
via store.put_bytes() etc.; no Arc plumbing visible at the
credential layer.
- lock_inner is the sole retained free helper (the one canonical
poisoned-mutex recovery point).
- sync_to_disk_locked renamed to sync_to_disk: the _locked suffix was
redundant — every &mut self call from a MutexGuard is by definition
locked.
- EncryptedFileStore::rekey switched from &mut self to &self: the
Mutex provides exclusion, the borrow checker no longer needs to.
- Debug for EncryptedFileStore now uses try_lock with an
"<unavailable>" fallback — protects against deadlock if Debug ever
fires from a panic path inside the critical section.
Critical invariant preserved: every entry path acquires the lock
exactly once via lock_inner and holds it through seal/open + the
on-disk sync_to_disk + the rollback path.
rekey_does_not_race_put_into_corruption still passes.
Net -11 lines.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lklimek
merged commit 22e496d
into
feat/platform-wallet-sqlite-persistor
3 checks passed
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
Adds the
secretsfeature tors-platform-wallet-storage— aSecretStoreSPI exposing two backends behind thekeyring_coretrait:EncryptedFileStore— passphrase-protected vault file on disk (Argon2id KDF + XChaCha20-Poly1305 AEAD, atomic write viaNamedTempFile::persist, in-process + cross-process locking viafd-lock)Stacks on
feat/platform-wallet-sqlite-persistor(PR #3625, which now includes the storage hardening landed via #3743).Public surface
SecretStoretrait (re-exportskeyring_core::Credential-shaped API for callers)SecretBytes/SecretString—Drop-zeroizing buffers withDisplayredaction andforbid PartialEq(usesubtle::ConstantTimeEqfor comparison); deliberately omitsDeref/AsRefto prevent accidental wideningFileStoreError— typed taxonomy (WrongPassphrase,Corruption,Busy,MalformedVault,VaultTooLarge { found, max },InsecurePermissions, …) with no raw bytes inDisplayOsKeyringErrorKind— security-enforcing payload-stripping projection ofkeyring_core::Error(preventsBadEncoding/BadDataFormatraw bytes from propagating, CWE-209/CWE-532)File vault format
Three core types (post the secrets/file collapse + Vec→BTreeMap change):
Vault { version, kdf: KdfParams, salt: [u8; SALT_LEN], verify_nonce: [u8; NONCE_LEN], verify_ct: Vec<u8>, entries: BTreeMap<String, EntryBody> }EntryBody { nonce: [u8; NONCE_LEN], ciphertext: Vec<u8> }(label is the BTreeMap key — no Vec linear scans)KdfParams { id, m_kib, t, p }(Argon2id; id-field validated inenforce_bounds)Serde-validated via const-generic
hex_arrayadapter on fixed-size byte fields; length mismatches surface asMalformedVaultat parse time. AEAD AAD is the typed(format_version, wallet_id, label)triple — never serialized bytes (C1 invariant).Crate features
secretsSecretStoreSPI +EncryptedFileStore+ per-platform keyring backendssqlitecliOS-keyring backends use target-specific dependencies:
apple-native-keyring-storewithkeychainfeaturewindows-native-keyring-storelinux-keyutils-keyring-store+dbus-secret-service-keyring-storeOff-state CI invocation (
--no-default-features --features sqlite,cli) is exercised bysecrets_off_state.rs— confirms thesecretsmodule compiles out cleanly when disabled.Hardening landed in this PR
Security (from PR review)
Mutex<HashMap<WalletId, Arc<Mutex<()>>>>+ cross-processfd-lock4.0.4 on a sidecar<wallet>.pwsvault.lock. Contention past a 5s timeout surfaces asFileStoreError::Busy. Closes the silent-concurrent-write data-loss window.warn(refusing would break tempfile + umask-022 mkdir used in test fixtures).FileStoreError::VaultTooLarge { found, max }variant.O_NOFOLLOW+ fd-based reads — symlink swap between metadata stat and read can no longer cross-contaminate.libcadded as Unix-only optional dep.Type safety (from PR review)
derive_keysignature now takes&SecretStringdirectly — wrapper propagates one layer deeper,.expose_secret().as_bytes()moves inside the function.inner.getreturnsResult<Option<SecretBytes>, _>— no rawVec<u8>window betweencrypto::openand the SPI seam. Lone.expose_secret().to_vec()at upstream-contract boundary.put/put_bytestake&SecretBytes(vs adding a Deref-like trait —SecretBytesdeliberately omits Deref/AsRef to block accidental widening).Code quality (from PR review)
Vault.entries: Vec<Entry>→BTreeMap<String, EntryBody>. JSON wire shape now object keyed by label; linear scans collapsed to single-key ops.write_vaultis now a 5-line wrapper callingdo_write_vaultmethod +.inspect_err(...)— no closure layer.entry_decrypt_or_corruptionhelper extracted — single source for the Decrypt→Corruption + tracing::warn translation acrossgetandrekey.FileStoreErrortaxonomy, serde+json for the vault format,MemoryCredentialStoreremoved, historical/license-header comments stripped,SecretStoreenum API that never leaks raw bytes.rekey()now decrypts each entry with old_key → re-encrypts under new_key → atomic write_vault → THEN swaps passphrase (was a known-broken order).Vault,Entry,KdfParams) via const-generichex_arrayserde adapter.Deferred (with INTENTIONAL annotation)
windows-acl/winapiGetSecurityInfointegration is out of scope for the secrets-feature landing. Operators on Windows must set ACLs manually until platform-wallet-storage: implement Windows ACL check for vault directory and file #3754 lands.Recent activity
origin/feat/platform-wallet-sqlite-persistor(post-feat(platform-wallet)!: rs-platform-wallet-storage crate (SQLite persister) + trait surface #3743) into this branch — brings in V001/V002 schema collapse, symmetric wallet_id validation, typedPersistenceError, B3 abort-on-mutation, and the rest of the feat(platform-wallet)!: rs-platform-wallet-storage crate (SQLite persister) + trait surface #3743 storage hardening--all-featureson macOS:apple-native-keyring-storeneededfeatures = ["keychain"]Storelives under::keychain::submodule, not at crate rootplatform-wallet-ffi/src/persistence.rs#[cfg(feature = "shielded")]block needed migration off the (removed)From<String> for PersistenceErrorimplTest plan
cargo fmt --all -- --checkcargo clippy -p platform-wallet-storage --all-targets -- -D warnings(default features)cargo clippy -p platform-wallet-storage --all-targets --no-default-features --features sqlite,cli -- -D warnings(off-state)cargo test -p platform-wallet-storage(default features) — 90+ secrets lib tests, 5 secrets_api, 1 secrets_default_on_compiles, 18 sqlite integration suites, all greencargo test -p platform-wallet-storage --no-default-features --features sqlite,cli --test secrets_off_state— 1/1 passBreaking changes
SecretBytesdeliberately omitsDeref/AsRef; callers receive typed wrapper, not&[u8].Related
feat/platform-wallet-sqlite-persistor) — base PR providing the SQLite persister + trait surfacefeat/platform-wallet-rehydration) — stacks on this PR; rehydration / seedless watch-only loadfeat/platform-wallet-consumer-hardening) — sibling of this PR (also stacks on feat(platform-wallet): add platform-wallet-storage crate (sqlite persister) #3625); consumer-side hardening + PROJ-001 FFI ABI break🤖 Generated with Claude Code