fix(kotlin-sdk): harden identity-key Keystore recovery and reconcile parity docs#4172
Conversation
The new alias-replacement retrieval regression was red with an InvalidKeyException because retrievePrivateKey attempted OAEP under the replacement key. It is green after gating stable reads on the stored fingerprint. Carry the exact encryption key fingerprint with each KEYS_ALIAS ciphertext so rotation cannot mislabel an old-key blob, and delete permanently invalidated aliases while preserving the typed exception for callers.
The new native-backed canSignWith regression was red because the existence-only check stayed true after KEYS_ALIAS replacement. It is green after capability checks use WalletStorage's fingerprint-aware decryptability boundary.
Exercise the native symbol with a valid BIP-39 vector and require a compact recoverable signature on-device. The direct caller scrubs its JVM-owned mnemonic array in finally; JNI remains responsible only for its Rust-side copies.
Mark completed migration plans as historical, align the leftovers inventory with the current 94/5/0 view totals and all-23 transition coverage, record the completed JNI signer smoke and contact-queue relocation, and correct the masternode-discovery rationale without regenerating the parity summary.
Final multi-agent review of the PR #3999 Keystore/docs follow-ups (6efa83b..ee484e9). The four safety-critical spec claims verified correct (atomic encrypt+fingerprint race fix, invalidation rotation-under- lock, retrieve pre-check, KPIE propagation skipping the biometric retry); build + all instrumented tests green on an arm64 emulator. Fixes for the clear defects the review surfaced: - FfiSmokeTest: drop the final assertArrayEquals(zeros, mnemonicUtf8). It ran after the test's own finally { fill(0) }, so it only proved fill(0) works and re-introduced the exact "JNI scrubs the caller's array" impression the corrected §6 test plan said not to assert. The test still proves the native symbol binds and returns a 65-byte compact recoverable signature, and still scrubs its own array. - keysAliasEncryptionCarriesTheFingerprintOfItsEncryptionKey: strengthen from a no-rotation snapshot (which would have passed against the pre-fix separate-lookup design) into a real guard — rotate KEYS_ALIAS after encryption and assert the blob's captured fingerprint no longer matches the current alias, pinning that the fingerprint is bound to the key used at encrypt time. - KOTLIN_MIGRATION_SPEC.md: fix a broken banner link (docs/sdk/PARITY_SUMMARY.md -> packages/kotlin-sdk/PARITY_SUMMARY.md). - sdk-parity-manifest.json: tighten the network.masternode_discovery reason (443 is also the legitimate mainnet port on the live path; the single-arg overload is not @Deprecated-annotated). Review summary and the deliberately-deferred judgment calls (the unconditional KEYS_ALIAS delete in the invalidation catch; the untested concurrent-rotation atomicity property) are recorded in docs/sdk/CODE_REVIEW_NOTES.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Preserve the public two-argument EncryptedBlob API while carrying the atomic KEYS_ALIAS fingerprint in an internal paired result. Red->green: the stale-generation instrumented test fails under unconditional alias deletion and passes with generation matching; the Java compile guard fails with the widened blob constructor and passes after compatibility restoration.
Mark generation-aware cleanup and EncryptedBlob compatibility as resolved, and reconcile the remaining mnemonic-buffer and partial-view wording.
Document the generation-aware cleanup decision, its red-to-green evidence, and the locked-emulator boundary from the final connected test run.
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✨ 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 |
|
🕓 Ready for review — 7 ahead in queue (commit 6ea27e2) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The generation-guarded invalidation cleanup, atomic fingerprint binding, and fail-closed retrieval behavior are correct. The synchronous FFI capability check can still generate a hardware-backed RSA keypair on a Rust worker thread, and the PR description inaccurately reports the connected test suite as fully green.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— general (failed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— ffi-engineer (failed),claude-sonnet-5— ffi-engineer (failed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt:201: Keep the synchronous capability callback read-only
`NativeSignerBridge.canSignWith` is documented as a fast synchronous callback with no I/O beyond a cache or database hit, and Rust invokes it directly on its current Tokio worker. This call reaches `isCurrentKeysBlob` and `keysAliasFingerprint`; when ciphertext remains but `KEYS_ALIAS` is absent after invalidation cleanup, `keysPublicKey` falls through to `ensureKeysKeyPair`, generating a StrongBox or software RSA-2048 keypair under the process-wide lock. A capability probe can therefore block the Rust worker and mutate Keystore state. Use a non-generating fingerprint lookup that returns null when the certificate is absent, and reserve key generation for encryption or recovery.
…ests The x86_64 emulator job flaked: the 5 Keystore-backed instrumented tests (DashPayUnlockAndSyncTest, WalletManagerRoundTripTest) failed with `InvalidKeyException: Keystore operation failed`. Root cause is timing, not code — on a slow CI boot the unlock step ran a fixed `set-pin` + `dismiss-keyguard` + `sleep 5` that raced the boot (adb errored 4×, emulator console failed to start, boot completed at 36.5s), so the device stayed in a locked state. Keys created with `setUnlockedDeviceRequired(true)` (the MASTER_ALIAS mnemonic AES key those tests decrypt) then throw while locked. Replace the blind sleep with: wait for `sys.boot_completed`, then poll `dumpsys trust` until `deviceLocked=0`, retrying wake/dismiss-keyguard and entering the PIN as a fallback, failing loudly if it never unlocks. This is a CI-infra flake fix; no product code changes, so no test change (verified locally that these 5 tests pass once the emulator is actually unlocked). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous unlock hardening broke the job: android-emulator-runner runs the `script` one line at a time (each line is its own `sh -c` with no shared state), so the multi-line `for` loop failed with `sh: 1: Syntax error: end of file unexpected (expecting "done")` and the `unlocked` variable never carried across lines. Collapse the whole poll-until-unlocked loop onto a single line, and re-check `deviceLocked=0` with a fresh `dumpsys` after it (instead of a cross-line variable) to fail loudly if the device never unlocks. Verified each logic line passes `sh -nc` standalone. Drops the now-redundant boot-wait — the action already waits for boot before running the script. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Confirmed root cause from the failing run's full stack trace: the 5 failing tests die at `KeystoreManager.encrypt` → `WalletStorage.storeMnemonic`, i.e. ENCRYPTING the mnemonic under the MASTER_ALIAS AES key (setUnlockedDeviceRequired(true)) with `InvalidKeyException: Keystore operation failed`. That only happens when the device is LOCKED. The device wasn't failing to unlock initially — the emulator screen was timing off and RE-LOCKING before the wallet tests ran. This branch runs more instrumented tests (32 vs a lighter branch's 27), so its wallet tests cross the screen-off deadline where lighter branches finish first — which is why it failed consistently while siblings passed on the identical script. Fix: set an effectively-infinite screen_off_timeout and `svc power stayon true` so the screen never sleeps and the device stays unlocked for the whole run, then keep the proven set-pin + dismiss-keyguard unlock and add a fail-loud deviceLocked=0 check. Verified locally that stayon holds deviceLocked=0 across the install-gap window. Supersedes the two prior workflow attempts (a multi-line loop that hit the line-by-line-`sh -c` constraint, and a single-line loop that unlocked but didn't prevent re-lock). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # docs/sdk/CODE_REVIEW_NOTES.md
Keep the keystore review/notes artifacts out of the PR, consistent with the spec and code-review notes that are kept local/uncommitted. Content preserved locally and recoverable from git history; no code impact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses PR review finding: canSignWith is a synchronous callback Rust invokes on its Tokio worker, documented as fast and side-effect-free. But it reached isCurrentKeysBlob -> keysAliasFingerprint -> keysPublicKey, which falls through to ensureKeysKeyPair and GENERATES an RSA-2048 keypair under the process-wide lock when KEYS_ALIAS is absent (e.g. right after permanent- invalidation cleanup deletes it). A mere capability probe could therefore block the Rust worker and mutate Keystore state. Add a non-generating keysAliasFingerprintOrNull() (returns null when the certificate is absent) and use it in isCurrentKeysBlob. An absent alias means any stored blob is already unrecoverable, so the probe reports the blob not current WITHOUT regenerating — key generation stays reserved for the encrypt/ recovery path, as the reviewer suggested. Updated the isCurrentKeysBlob / isPrivateKeyDecryptable KDoc that previously (correctly, before this change) warned the lookup could generate an alias. Red->green: capabilityProbeDoesNotRegenerateAMissingKeysAlias stores a key, deletes KEYS_ALIAS, probes, and asserts the alias is still absent afterward. Under the old generating lookup the probe regenerated the cert (assertion fails); with the read-only lookup it stays absent (passes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Introduce KeySecurityPolicy (AUTH_GATED default / DEVICE_BOUND opt-in) and move new identity-key wrapping off the single KEYS_ALIAS onto two dedicated RSA aliases, one per policy — Keystore auth parameters are fixed at key generation, so the policies can never share an alias. The legacy KEYS_ALIAS becomes read-only: it retains whichever superseded key (auth-gated AES-GCM, or the pre-split RSA pair) an upgraded install still holds, reachable only through the decryptLegacy* migration fallbacks. The dashpay#4172 hardening survives the split alias-parameterized: write-time fingerprint capture (encryptForIdentityKeysAlias — one lookup for blob + fingerprint + producing alias), the non-generating fingerprint read, and the KeyPermanentlyInvalidatedException recovery with its generation-checked, per-alias deletion (deleteIdentityKeysAliasIfCurrentGeneration, which refuses the legacy alias by contract). WalletStorage grows the policy-taking constructor and routes identity-key writes/reads through the policy alias. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bing capability split retrievePrivateKey becomes an explicit three-rung ladder that composes the dashpay#4172 fingerprint gate with the legacy-blob recovery paths: 1. Legacy AES-GCM blobs shape-dispatch FIRST — the write-time fingerprint gate never applies to them (they predate it or carry a superseded key's fingerprint); the retained legacy KEYS_ALIAS AES key recovers the value, which is migrated to the policy alias. 2. Empty-IV RSA blobs whose stored fingerprint matches the RECORDED alias's current key (non-generating read, alias-tag routed) decrypt on the fast path. KeyPermanentlyInvalidatedException rethrows after KeystoreManager's generation-checked cleanup — after the deletion the fingerprint can no longer match, so reads and repair re-DERIVE rather than trusting a stale certificate (closes the invalidation brick loop). Other crypto failures fall through (defense in depth). 3. A mismatching or MISSING fingerprint is a routing signal into the former-KEYS_ALIAS-RSA recovery ladder, not an immediate null — pre-split blobs carry the former key's fingerprint and must reach decryptLegacyRsaKeysBlob. The policy alias is never touched here, and a read never provisions a keypair. Capability is split per finding 3: isPrivateKeyDecryptable stays the CHEAP check (presence + fingerprint only — the Rust canSignWith callback thread must never decrypt, prompt, or generate), extended with presence rungs for the legacy schemes; the real-decrypt health check moves to the new probeIdentityKeyRecoverability (DEVICE_BOUND-sibling disproof included, skipped for blobs the sibling legitimately owns by tag), which the example app's WalletKeyHealthSheet now calls. UserNotAuthenticatedException propagates from every rung — a closed auth window is never a wrong key. WalletStorageUpgradeMatrixTest pins every ladder row through the fake Keystore seam, including the new invalidation-cleanup, alias-tag-routing, fast-path-failure, and never-decrypts-on-the-cheap-path rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…as split The inherited dashpay#4172 instrumented tests hardcode the legacy KEYS_ALIAS and the single-alias API; after the alias split the default-policy storage writes under KEYS_ALIAS_AUTH_GATED, so simulating keypair replacement by deleting the legacy alias no longer invalidates anything (the CI failure shumkov diagnosed on storeIfAbsentRederivesWhenTheKeysAliasKeypairWasReplaced). - WalletStorageOwnershipTest: the replaced-keypair, rejected-blob, and no-regeneration probes delete/inspect KEYS_ALIAS_AUTH_GATED; the fingerprint-capture test uses encryptForIdentityKeysAlias (and pins the captured producing alias); the stale-invalidation-cleanup test uses the alias-parameterized deleteIdentityKeysAliasIfCurrentGeneration. - KeystoreSignerInstrumentedTest: canSignWith rejection simulates replacement of the policy alias. The five DashPayUnlockAndSyncTest / WalletManagerRoundTripTest failures from PR dashpay#4183's CI run were NOT test or SDK defects: the branch predated dashpay#4172's workflow hardening (screen_off_timeout / stayon / dismiss-keyguard + the hard deviceLocked=0 guard), so the emulator re-locked mid-run and every MASTER_ALIAS operation threw InvalidKeyException. Rebuilding on the current base inherits the fixed workflow; those tests are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity, and Room v8 shift - sdk-parity-manifest.json (+ regenerated PARITY_SUMMARY.md): new capability entries for the KeySecurityPolicy alias split (Swift not-applicable — iOS Keychain has no per-alias auth-parameter analog), the now-CONVERGENT platform-wallet code-98 typed mapping (was a divergence), the Kotlin-only durable pending-repair surface (Swift gap recorded), and the structured SigningKeyUnavailable discriminator (both hosts, unit-verification pointers on each). shared_symbols gains dash_sdk_sign_async_completion. - KOTLIN_SWIFT_SHARED_PARITY_SPEC.md: rationale for each divergence / convergence, the deprecated MESSAGE_MARKER fallback with its next-minor-release removal horizon, and the migration ladder update (v8 = derivation breadcrumbs; invitations shift to v9). - KOTLIN_MIGRATION_LEFTOVERS.md: marker-fallback removal and the on-device KeyPermanentlyInvalidatedException residual (unit-tier only via the fake seam, same as dashpay#4172). - Swift: testPlatformWalletNotFoundFFIResultMapping pins the convergent 98 mapping the manifest references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g-2 TOCTOU window Round-2 nits (dashpay#4060): - The MESSAGE_MARKER fallback's stated rationale — "old native library + new Kotlin partial builds" — was not viable: the sign-completion JNI arity change (3→4 args) makes that pairing a type-confused native call on every completion, so mixed artifacts are unsupported outright. The fallback's real purpose is the dashpay#4191 merge-order transition (whose marker-based classification predates the typed code 31) plus defense in depth for conversion paths that lose the machine prefix. KDoc/comments in DashSdkError, KeystoreSigner, DashSdkErrorTest, and the parity/leftovers docs now say so; removal horizon unchanged (next minor release). - retrievePrivateKey rung 2 gains a comment naming the accepted fingerprint-read→decrypt TOCTOU window (pre-existing dashpay#4172 parity): a concurrent rotation fails as a wrong-key crypto error into the recovery ladder — never stale plaintext. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduce KeySecurityPolicy (AUTH_GATED default / DEVICE_BOUND opt-in) and move new identity-key wrapping off the single KEYS_ALIAS onto two dedicated RSA aliases, one per policy — Keystore auth parameters are fixed at key generation, so the policies can never share an alias. The legacy KEYS_ALIAS becomes read-only: it retains whichever superseded key (auth-gated AES-GCM, or the pre-split RSA pair) an upgraded install still holds, reachable only through the decryptLegacy* migration fallbacks. The dashpay#4172 hardening survives the split alias-parameterized: write-time fingerprint capture (encryptForIdentityKeysAlias — one lookup for blob + fingerprint + producing alias), the non-generating fingerprint read, and the KeyPermanentlyInvalidatedException recovery with its generation-checked, per-alias deletion (deleteIdentityKeysAliasIfCurrentGeneration, which refuses the legacy alias by contract). WalletStorage grows the policy-taking constructor and routes identity-key writes/reads through the policy alias. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bing capability split retrievePrivateKey becomes an explicit three-rung ladder that composes the dashpay#4172 fingerprint gate with the legacy-blob recovery paths: 1. Legacy AES-GCM blobs shape-dispatch FIRST — the write-time fingerprint gate never applies to them (they predate it or carry a superseded key's fingerprint); the retained legacy KEYS_ALIAS AES key recovers the value, which is migrated to the policy alias. 2. Empty-IV RSA blobs whose stored fingerprint matches the RECORDED alias's current key (non-generating read, alias-tag routed) decrypt on the fast path. KeyPermanentlyInvalidatedException rethrows after KeystoreManager's generation-checked cleanup — after the deletion the fingerprint can no longer match, so reads and repair re-DERIVE rather than trusting a stale certificate (closes the invalidation brick loop). Other crypto failures fall through (defense in depth). 3. A mismatching or MISSING fingerprint is a routing signal into the former-KEYS_ALIAS-RSA recovery ladder, not an immediate null — pre-split blobs carry the former key's fingerprint and must reach decryptLegacyRsaKeysBlob. The policy alias is never touched here, and a read never provisions a keypair. Capability is split per finding 3: isPrivateKeyDecryptable stays the CHEAP check (presence + fingerprint only — the Rust canSignWith callback thread must never decrypt, prompt, or generate), extended with presence rungs for the legacy schemes; the real-decrypt health check moves to the new probeIdentityKeyRecoverability (DEVICE_BOUND-sibling disproof included, skipped for blobs the sibling legitimately owns by tag), which the example app's WalletKeyHealthSheet now calls. UserNotAuthenticatedException propagates from every rung — a closed auth window is never a wrong key. WalletStorageUpgradeMatrixTest pins every ladder row through the fake Keystore seam, including the new invalidation-cleanup, alias-tag-routing, fast-path-failure, and never-decrypts-on-the-cheap-path rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…as split The inherited dashpay#4172 instrumented tests hardcode the legacy KEYS_ALIAS and the single-alias API; after the alias split the default-policy storage writes under KEYS_ALIAS_AUTH_GATED, so simulating keypair replacement by deleting the legacy alias no longer invalidates anything (the CI failure shumkov diagnosed on storeIfAbsentRederivesWhenTheKeysAliasKeypairWasReplaced). - WalletStorageOwnershipTest: the replaced-keypair, rejected-blob, and no-regeneration probes delete/inspect KEYS_ALIAS_AUTH_GATED; the fingerprint-capture test uses encryptForIdentityKeysAlias (and pins the captured producing alias); the stale-invalidation-cleanup test uses the alias-parameterized deleteIdentityKeysAliasIfCurrentGeneration. - KeystoreSignerInstrumentedTest: canSignWith rejection simulates replacement of the policy alias. The five DashPayUnlockAndSyncTest / WalletManagerRoundTripTest failures from PR dashpay#4183's CI run were NOT test or SDK defects: the branch predated dashpay#4172's workflow hardening (screen_off_timeout / stayon / dismiss-keyguard + the hard deviceLocked=0 guard), so the emulator re-locked mid-run and every MASTER_ALIAS operation threw InvalidKeyException. Rebuilding on the current base inherits the fixed workflow; those tests are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity, and Room v8 shift - sdk-parity-manifest.json (+ regenerated PARITY_SUMMARY.md): new capability entries for the KeySecurityPolicy alias split (Swift not-applicable — iOS Keychain has no per-alias auth-parameter analog), the now-CONVERGENT platform-wallet code-98 typed mapping (was a divergence), the Kotlin-only durable pending-repair surface (Swift gap recorded), and the structured SigningKeyUnavailable discriminator (both hosts, unit-verification pointers on each). shared_symbols gains dash_sdk_sign_async_completion. - KOTLIN_SWIFT_SHARED_PARITY_SPEC.md: rationale for each divergence / convergence, the deprecated MESSAGE_MARKER fallback with its next-minor-release removal horizon, and the migration ladder update (v8 = derivation breadcrumbs; invitations shift to v9). - KOTLIN_MIGRATION_LEFTOVERS.md: marker-fallback removal and the on-device KeyPermanentlyInvalidatedException residual (unit-tier only via the fake seam, same as dashpay#4172). - Swift: testPlatformWalletNotFoundFFIResultMapping pins the convergent 98 mapping the manifest references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g-2 TOCTOU window Round-2 nits (dashpay#4060): - The MESSAGE_MARKER fallback's stated rationale — "old native library + new Kotlin partial builds" — was not viable: the sign-completion JNI arity change (3→4 args) makes that pairing a type-confused native call on every completion, so mixed artifacts are unsupported outright. The fallback's real purpose is the dashpay#4191 merge-order transition (whose marker-based classification predates the typed code 31) plus defense in depth for conversion paths that lose the machine prefix. KDoc/comments in DashSdkError, KeystoreSigner, DashSdkErrorTest, and the parity/leftovers docs now say so; removal horizon unchanged (next minor release). - retrievePrivateKey rung 2 gains a comment naming the accepted fingerprint-read→decrypt TOCTOU window (pre-existing dashpay#4172 parity): a concurrent rotation fails as a wrong-key crypto error into the recovery ladder — never stale plaintext. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduce KeySecurityPolicy (AUTH_GATED default / DEVICE_BOUND opt-in) and move new identity-key wrapping off the single KEYS_ALIAS onto two dedicated RSA aliases, one per policy — Keystore auth parameters are fixed at key generation, so the policies can never share an alias. The legacy KEYS_ALIAS becomes read-only: it retains whichever superseded key (auth-gated AES-GCM, or the pre-split RSA pair) an upgraded install still holds, reachable only through the decryptLegacy* migration fallbacks. The dashpay#4172 hardening survives the split alias-parameterized: write-time fingerprint capture (encryptForIdentityKeysAlias — one lookup for blob + fingerprint + producing alias), the non-generating fingerprint read, and the KeyPermanentlyInvalidatedException recovery with its generation-checked, per-alias deletion (deleteIdentityKeysAliasIfCurrentGeneration, which refuses the legacy alias by contract). WalletStorage grows the policy-taking constructor and routes identity-key writes/reads through the policy alias. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bing capability split retrievePrivateKey becomes an explicit three-rung ladder that composes the dashpay#4172 fingerprint gate with the legacy-blob recovery paths: 1. Legacy AES-GCM blobs shape-dispatch FIRST — the write-time fingerprint gate never applies to them (they predate it or carry a superseded key's fingerprint); the retained legacy KEYS_ALIAS AES key recovers the value, which is migrated to the policy alias. 2. Empty-IV RSA blobs whose stored fingerprint matches the RECORDED alias's current key (non-generating read, alias-tag routed) decrypt on the fast path. KeyPermanentlyInvalidatedException rethrows after KeystoreManager's generation-checked cleanup — after the deletion the fingerprint can no longer match, so reads and repair re-DERIVE rather than trusting a stale certificate (closes the invalidation brick loop). Other crypto failures fall through (defense in depth). 3. A mismatching or MISSING fingerprint is a routing signal into the former-KEYS_ALIAS-RSA recovery ladder, not an immediate null — pre-split blobs carry the former key's fingerprint and must reach decryptLegacyRsaKeysBlob. The policy alias is never touched here, and a read never provisions a keypair. Capability is split per finding 3: isPrivateKeyDecryptable stays the CHEAP check (presence + fingerprint only — the Rust canSignWith callback thread must never decrypt, prompt, or generate), extended with presence rungs for the legacy schemes; the real-decrypt health check moves to the new probeIdentityKeyRecoverability (DEVICE_BOUND-sibling disproof included, skipped for blobs the sibling legitimately owns by tag), which the example app's WalletKeyHealthSheet now calls. UserNotAuthenticatedException propagates from every rung — a closed auth window is never a wrong key. WalletStorageUpgradeMatrixTest pins every ladder row through the fake Keystore seam, including the new invalidation-cleanup, alias-tag-routing, fast-path-failure, and never-decrypts-on-the-cheap-path rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…as split The inherited dashpay#4172 instrumented tests hardcode the legacy KEYS_ALIAS and the single-alias API; after the alias split the default-policy storage writes under KEYS_ALIAS_AUTH_GATED, so simulating keypair replacement by deleting the legacy alias no longer invalidates anything (the CI failure shumkov diagnosed on storeIfAbsentRederivesWhenTheKeysAliasKeypairWasReplaced). - WalletStorageOwnershipTest: the replaced-keypair, rejected-blob, and no-regeneration probes delete/inspect KEYS_ALIAS_AUTH_GATED; the fingerprint-capture test uses encryptForIdentityKeysAlias (and pins the captured producing alias); the stale-invalidation-cleanup test uses the alias-parameterized deleteIdentityKeysAliasIfCurrentGeneration. - KeystoreSignerInstrumentedTest: canSignWith rejection simulates replacement of the policy alias. The five DashPayUnlockAndSyncTest / WalletManagerRoundTripTest failures from PR dashpay#4183's CI run were NOT test or SDK defects: the branch predated dashpay#4172's workflow hardening (screen_off_timeout / stayon / dismiss-keyguard + the hard deviceLocked=0 guard), so the emulator re-locked mid-run and every MASTER_ALIAS operation threw InvalidKeyException. Rebuilding on the current base inherits the fixed workflow; those tests are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity, and Room v8 shift - sdk-parity-manifest.json (+ regenerated PARITY_SUMMARY.md): new capability entries for the KeySecurityPolicy alias split (Swift not-applicable — iOS Keychain has no per-alias auth-parameter analog), the now-CONVERGENT platform-wallet code-98 typed mapping (was a divergence), the Kotlin-only durable pending-repair surface (Swift gap recorded), and the structured SigningKeyUnavailable discriminator (both hosts, unit-verification pointers on each). shared_symbols gains dash_sdk_sign_async_completion. - KOTLIN_SWIFT_SHARED_PARITY_SPEC.md: rationale for each divergence / convergence, the deprecated MESSAGE_MARKER fallback with its next-minor-release removal horizon, and the migration ladder update (v8 = derivation breadcrumbs; invitations shift to v9). - KOTLIN_MIGRATION_LEFTOVERS.md: marker-fallback removal and the on-device KeyPermanentlyInvalidatedException residual (unit-tier only via the fake seam, same as dashpay#4172). - Swift: testPlatformWalletNotFoundFFIResultMapping pins the convergent 98 mapping the manifest references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g-2 TOCTOU window Round-2 nits (dashpay#4060): - The MESSAGE_MARKER fallback's stated rationale — "old native library + new Kotlin partial builds" — was not viable: the sign-completion JNI arity change (3→4 args) makes that pairing a type-confused native call on every completion, so mixed artifacts are unsupported outright. The fallback's real purpose is the dashpay#4191 merge-order transition (whose marker-based classification predates the typed code 31) plus defense in depth for conversion paths that lose the machine prefix. KDoc/comments in DashSdkError, KeystoreSigner, DashSdkErrorTest, and the parity/leftovers docs now say so; removal horizon unchanged (next minor release). - retrievePrivateKey rung 2 gains a comment naming the accepted fingerprint-read→decrypt TOCTOU window (pre-existing dashpay#4172 parity): a concurrent rotation fails as a wrong-key crypto error into the recovery ladder — never stale plaintext. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Issue being fixed or feature implemented
Follow-ups to PR #3999 (Kotlin SDK / KotlinExampleApp), stacked on
feat/kotlin-sdk-and-example-app. Closes two findings from that PR's automatedreview plus a documentation-accuracy pass, all scoped to the Android Keystore
identity-key path:
KEYS_ALIASfingerprint detected key replacement butnot the same key being permanently invalidated (e.g. secure lock
disabled / credential reset), which fails differently
(
KeyPermanentlyInvalidatedException, previously uncaught).KeystoreSigner.canSignWithreported a stale, replaced key as usablebecause it checked ciphertext existence, not decryptability.
The DashPay registration-key provisioning finding from the same review is
deliberately out of scope — it is a separate PR/subsystem.
What was done?
A. Permanent-invalidation detection +
KEYS_ALIASrotation.KeystoreManager.decrypt()now catchesKeyPermanentlyInvalidatedExceptiononthe auth-bound RSA private-key
cipher.initand deletes the invalidated alias sothe next use regenerates a fresh keypair. The typed exception propagates
uncaught past
KeystoreSigner.retrieveKeyWithAuth(which only catchesUserNotAuthenticatedException), so it correctly skips the biometric retryinstead of prompt-looping. Deletion is generation-guarded: it only drops the
alias while the currently-present key still matches the invalidated generation's
fingerprint, so a stale decryptor can't orphan a replacement another thread just
regenerated and encrypted under.
Atomic encrypt/fingerprint. Identity-key encryption now binds the persisted
fingerprint to the exact public key object it encrypted with (an internal paired
result), instead of a separate later
keysAliasFingerprint()lookup — closing arace where a concurrent rotation could persist old-key ciphertext labeled with
the new key's fingerprint. The public
EncryptedBlobvalue type / Javaconstructor ABI is preserved (guarded by a Java-interop compile test).
Retrieval pre-check.
WalletStorage.retrievePrivateKeygates on the storedfingerprint before decrypting, so a stable stale blob returns
null(→re-derive) instead of surfacing a provider-specific
BadPadding/KeyStoreerror. This is documented as fail-closed/TOCTOU: a rotation between the check and
doFinalstill throws and is handled as fail-closed.B. Signer capability check.
KeystoreSigner.canSignWith(identity-keybranch) now uses
isPrivateKeyDecryptablerather thanhasPrivateKey, so areplaced-alias blob is reported not-signable.
C. Native-signer smoke test.
FfiSmokeTestgains an on-device test thatloads the
signWithMnemonicAndPathIntoJNI symbol, derives from a BIP-39 vector,and asserts a 65-byte compact recoverable signature. It scrubs its own
JVM-owned mnemonic array in
finallyand deliberately does not assert JNIscrubs it (JNI copies into Rust-owned zeroizing memory and leaves the caller's
array alone, by contract).
D. Parity-doc reconciliation. Reconciled stale/contradictory passages across
the Kotlin-SDK tracking docs (resolved-item banners, corrected PARITY totals
94/5/0,
network.masternode_discoverymanifest reason, historical-baselinebanners, contact-crypto relocation status).
check_sdk_parity_manifest.pystaysgreen (18 capabilities; summary current).
A full multi-agent code review of the diff (correctness/concurrency, Android
Keystore security, test adequacy, doc accuracy) was performed and its findings
folded in; the review/implementation notes are kept local rather than committed.
How Has This Been Tested?
:sdk:assembleDebug :sdk:testDebugUnitTest :app:assembleDebug :app:testDebugUnitTest :sdk:compileDebugAndroidTestKotlin— BUILD SUCCESSFUL.:sdk:connectedDebugAndroidTeston an arm64-v8a emulator (API 35, PINenrolled + device unlocked per the CI harness) — full instrumented suite green,
including the new rotation / capability / native-signer cases. Red→green was
confirmed for the capability and retrieval pre-check tests (they fail against
the pre-fix existence check / direct decrypt and pass after the fix), and the
generation-guarded deletion is pinned by a stale-generation instrumented test.
python3 scripts/check_sdk_parity_manifest.py— OK.x86_64CI runs failed 5 pre-existing wallet tests withInvalidKeyException: Keystore operation failed(DEVICE_LOCKED). That wasan emulator flake — the screen timed out and re-locked mid-run, so the
setUnlockedDeviceRequiredmnemonic key could not be used; it was not a coderegression (the newly added tests passed). Fixed by keeping the CI emulator
awake so it cannot re-lock; the
x86_64suite is now green.Breaking Changes
None. The public
EncryptedBlobtype and its Java constructor are unchanged(guarded by a Java-interop compile test); write-time fingerprint metadata is
carried in an internal type.
Checklist:
🤖 Generated with Claude Code