feat(kotlin-sdk)!: keystore rework — policy-alias split, layered key recovery, durable repair, structured signer errors (stacked on #4191)#4183
Open
bfoss765 wants to merge 22 commits into
Conversation
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 21, 2026 12:40
Contributor
|
Warning Review limit reached
Next review available in: 59 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 (10)
📝 WalkthroughWalkthroughAdds policy-based keystore aliases and migration handling, transactional pending identity-key tracking, structured signer errors, typed platform-wallet errors, and managed-identity lookup classification across Kotlin, Rust, and Swift. ChangesKeystore policy and identity-key recovery
Pending identity-key repair state
Signer and platform-wallet error flows
Schema and parity artifacts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Signer
participant KotlinJNI
participant RustFFI
participant PlatformWallet
Signer->>KotlinJNI: completeSign(errorCode, errorMessage)
KotlinJNI->>RustFFI: forward structured signer error
RustFFI->>PlatformWallet: map signing-key-unavailable prefix to code 31
PlatformWallet-->>Signer: return typed platform-wallet error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Collaborator
|
🕓 Ready for review — 15 ahead in queue (commit 8313192) |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- Around line 549-562: Update the migration block around store.edit in
WalletStorage so CancellationException is not swallowed by runCatching; rethrow
cancellation while retaining the existing handling for non-cancellation failures
and successful migration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 987bcd6c-12ec-4a70-8f81-bdad970986df
📒 Files selected for processing (15)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreKeyGenPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageUpgradeMatrixTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.ktpackages/rs-platform-wallet-ffi/src/dashpay.rs
Collaborator
|
This needs reconciliation with #4172 before it can merge — the branch is
Suggestion: split the unmanaged-identity-reads + typed |
Collaborator
|
Additional findings after checking the existing threads and the current #4172 base:
I am not repeating the existing rebase/invalidation-recovery and cancellation comments; those remain valid. The earlier |
…otFound Split out of dashpay#4183 (port of dashpay#4060) per review: the unmanaged-identity read fixes are conflict-free on v4.1-dev and should not be blocked on the Keystore rework. The FFI's blanket Option -> result conversion reports an identity the wallet does not manage as PlatformWalletFFIResultCode::NotFound (98), so the Kotlin callers' zero-handle checks were dead and every local read over an unmanaged identity threw instead of returning absence. - Dashpay: route getManagedIdentity through translateManagedIdentityNotFoundToZero so contacts()/syncState()/ payments()/sendContactRequest() treat "not managed" as null/empty/ false; ErrorInvalidHandle still propagates. - Sweep the remaining dead `== 0L` sites: ManagedPlatformWallet .inMemoryIdentityStates (one unmanaged/just-removed id no longer throws through the whole listing) and IdentityRegistration .contestedDpnsNames (the intended "identity is not managed by this wallet" NotFound now actually surfaces). - platform-wallet-ffi: platform_wallet_get_managed_identity keeps the three outcomes distinct (classify_managed_identity_outcome) so a stale/removed wallet surfaces ErrorInvalidHandle and never masquerades as an unmanaged identity; unit tests pin both arms. - DashSdkError: name the code (PLATFORM_WALLET_NOT_FOUND_CODE = 98); the 98 -> DashSdkError.NotFound mapping is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Split out of dashpay#4183 (port of dashpay#4060) per review: the typed signing error is conflict-free on v4.1-dev and should not be blocked on the Keystore rework. The KeystoreSigner "missing key" completion error travels as free text through Rust and used to come back as an opaque PlatformWallet.Generic (or WalletOperation) failure. It is now built from a shared MESSAGE_MARKER constant and recognized on the Kotlin boundary as the typed DashSdkError.PlatformWallet.SigningKeyUnavailable, so hosts can route users to key repair instead of showing a generic error. The marker is only consulted on the catch-all codes (6 / else), so the dedicated retry-semantics types are never overridden. Known limitation (kept as-is from dashpay#4183 by request): the discriminator is message-text-based (message.contains on the marker); a structured error code across the FFI boundary is follow-up work in the parent PR line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Took your suggestion: the conflict-free slices are split out as #4191 — unmanaged-identity reads (including both |
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>
…und alias, honestly The wallet must work without a screen lock (dashpay#4060), but the AUTH_GATED authentication gate cannot exist without one — KeyMint rejects generate_key. Instead of silently generating a gate-less key under the auth-gated alias (a policy lie), identity-key writes on a lockless device are redirected to KEYS_ALIAS_DEVICE_BOUND, whose gate-less parameters are inherent: - encryptForIdentityKeys resolves the effective write alias (probe first, KeyMint safety-net rejection second) and returns the producing alias with the blob; WalletStorage records it as a per-blob privkeyalias. tag written atomically with the blob + fingerprint, and reads decrypt under the RECORDED alias — keys written while lockless stay readable after a lock screen is enrolled and new writes move to the gated alias. A missing tag means the policy alias (backward compatible with earlier data). - effectiveKeySecurityPolicy() (KeystoreManager + WalletStorage) surfaces the degradation; PlatformWalletManager logs it once at construction. - Strict mode: KeystoreManager(requireAuthGated = true) throws the new KeySecurityPolicyUnavailableException instead of degrading. - MASTER_ALIAS (AES) and the DEVICE_BOUND alias keep the in-place setUnlockedDeviceRequired degradation (no auth gate to lie about); the auth-gated alias is never routed through it. - isNoSecureLockScreenKeyGenFailure is tightened: a nested KeyStoreException classifies only inside the key-gen ProviderException's cause chain AND with a lock-screen message or the observed rejection's numeric code (internal Keystore 4 / KeyMint 10309, read reflectively) — a bare nested Keystore error with an unrelated message no longer triggers the retry. 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>
…key failures Port of the dashpay#4053 pending-keys machinery: when the identity-key persist callback's derive/store fails, the key lands in Room watch-only and every signature with it will fail — previously a single Log.w was the only trace. PlatformWalletPersistenceHandler now records a PendingIdentityKey entry (wallet/identity/keyId, derivation breadcrumbs, reason, timestamp) in a StateFlow keyed by public-key hex, exposed through PlatformWalletManager.pendingIdentityKeys so hosts can surface a repair path. Transactional with the changeset round (finding de3cf44a71fc): while a store round is open, record/clear mutations are staged as pure map deltas in the round's ChangesetBuffer and published in ONE atomic MutableStateFlow.update only after the Room transaction commits; a rolled-back or aborted round discards them with the buffer, leaving the pre-round map untouched. Standalone upserts publish immediately, as does markIdentityKeyRepaired — which repairIdentityKey now calls on success, since the repair stores the scalar through the deriver and bypasses the only persist path that clears pending entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rt (Room 7→8) pendingIdentityKeys was process-lifetime only: the durable public_keys row kept just privateKeyKeychainIdentifier = null, dropping the derivation breadcrumbs a repair needs — kill the app and the repair signal was gone. - Room migration 7→8 adds nullable public_keys.derivationIdentityIndex / derivationKeyIndex. Room is the durability substrate deliberately: the wallet-deletion cascade removes these rows, so pending entries die with their wallet automatically (a DataStore side-table would leak). - onPersistIdentityKeyUpsert records the breadcrumbs whenever Rust supplied derivation indices — success AND failure paths (the breadcrumb is not a failure marker; the null identifier is). - reconstructPendingIdentityKeysFromPersistence rebuilds the map on launch: a breadcrumbed, non-read-only row re-seeds pending when its identifier is null OR the stored blob fails the CHEAP capability check — the second disjunct resurrects repair slots for blobs stranded by a Keystore keypair replacement, not just never-derived keys. One atomic update, live entries never overwritten, reason = "reconstructed from persistence after restart". PlatformWalletManager.loadPersistedWallets runs it before the manager is handed to the host. - DashDatabaseMigrationTest gains the 7→8 case (NULL for pre-existing rows, explicit values accepted); the 4→latest and 1→latest chains extend to 8; schema 8.json exported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
repairIdentityKey previously routed through storeIfAbsent, whose shape+fingerprint usability short-circuit can be satisfied by a blob that does not actually decrypt — the derive was skipped and markIdentityKeyRepaired fired anyway, clearing the pending signal while the key stayed broken (dashpay#4060 finding 6). - WalletStorage.replacePrivateKey: forced store with the same outside-the-lock derive discipline (and tombstone checks) as storeIfAbsent, but on re-entry it UNCONDITIONALLY replaces blob + fingerprint + alias tag in one atomic edit — never the addOwnerIfUsableLocked short-circuit. - PrivateKeyDeriver.deriveAndStore gains force: Boolean = false; IdentityKeyPrivateKeyDeriver routes force → replacePrivateKey. The persistence-callback call site keeps force = false (idempotent upserts must not re-derive on every sync). - PlatformWalletManager.repairIdentityKey: deriveAndStore(force = true), then VERIFY with the real-decrypt probeIdentityKeyRecoverability before clearing pending state — a wrong-key crypto failure fails the repair with a typed PlatformWallet.SigningKeyUnavailable (UserNotAuthenticatedException counts as verified: the key is present and opens after auth, and the just-written fingerprint rules out the wrong-key-behind-locked-gate ambiguity). Only after verification: markIdentityKeyRepaired + the Room rows' privateKeyKeychainIdentifier update, so the durable pending-repair reconstruction does not resurrect the repaired key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t.NotFound PlatformWalletFFIResultCode::NotFound (98) — the blanket Option→result miss (unknown wallet id, unmanaged identity, …) — previously collapsed into the top-level DashSdkError.NotFound shared with rs-sdk-ffi codes 7/8. It now maps to the new DashSdkError.PlatformWallet.NotFound, keeping 98 inside the wallet-error family in exact parity with Swift's PlatformWalletError.notFound, so callers can match a wallet-level absence without sniffing Generic codes. BREAKING: Kotlin hosts catching DashSdkError.NotFound from platform-wallet operations (e.g. rescanSpvFilters with an unknown wallet id) now receive DashSdkError.PlatformWallet.NotFound; rs-sdk-ffi codes 7/8 still map to the top-level NotFound. Dashpay's managed-identity local reads are unaffected — translateManagedIdentityNotFoundToZero intercepts the RAW code (dashpay#4051) before this mapping ever runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. 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>
…closed auth window as recoverable probeIdentityKeyRecoverability treated UserNotAuthenticatedException as "recoverable" unconditionally. UNAE is thrown at cipher.init — before the ciphertext is examined — so after a Keystore loss + regeneration the fresh auth-gated key (whose window is closed in the steady state; it is only open ~30 s after an auth) reported a blob it can never open as healthy: the key-health sheet offered no repair while pendingIdentityKeys simultaneously listed the same key (dashpay#4060 round-2 finding 1). Prompt-free fix: UNAE counts as recoverable ONLY while the blob's stored write-time fingerprint still matches the recorded alias's CURRENT key (non-generating read). A mismatch disproves ownership → not recoverable → repair is offered; a fingerprint-matched blob behind a closed window stays recoverable (genuinely just needs auth). The legacy former-RSA rung keeps the documented residual (no fingerprint surface for the retained legacy key). New matrix rows pin both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ped signer code on the first attempt A KPIE escaping the sign path (retrieveKeyWithAuth — the biometric-gate retry only handles UserNotAuthenticatedException) previously fell into the signAsync catch-all and completed with SIGNER_ERROR_CODE_GENERIC: the host saw an opaque wallet-operation failure on the FIRST post-invalidation sign and only got the typed SigningKeyUnavailable on subsequent attempts (once the fingerprint gate reported the blob non-current). dashpay#4060 round-2 finding 2. - signWithStoredKey catches KPIE explicitly and completes with SIGNER_ERROR_CODE_KEY_UNAVAILABLE (marker text + invalidation reason kept in the message), so the typed code rides the wire immediately. - The signAsync catch-all classifies via the new pure completionErrorCodeFor(t) (KPIE → key-unavailable, everything else generic) as belt-and-braces for KPIE thrown outside the retrieval call. - KeystoreSignerCompletionCodeTest pins the classification (the full signer cannot be constructed on the JVM — its constructor creates a native handle — so the classification is a pure companion function). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… for legacy-alias keys A KeyPermanentlyInvalidatedException on a LEGACY-alias-backed key was invisible everywhere: the legacy Keystore aliases are read-only (no deletion boundary), so hasLegacyKeysKey() stays true forever — canSignWith keeps offering the key, the cheap capability check keeps reporting the blob usable, the restart reconstruction never seeds a repair slot, and every sign fails opaquely (dashpay#4060 round-2 finding 3). Fix without making the cheap check expensive (it is untouched): the sign path's KPIE classification now invokes an onSigningKeyInvalidated hook, wired by PlatformWalletManager to PlatformWalletPersistenceHandler.recordSigningKeyInvalidated, which nulls privateKeyKeychainIdentifier on the key's Room rows — the same durable signal the restart reconstruction reads — and re-runs the reconstruction so pendingIdentityKeys seeds after the FIRST failed sign (reason: "signing key permanently invalidated"), making repair reachable outside the health sheet. Best-effort: bookkeeping failure never eats the typed completion. Harmless for policy-alias keys (their generation-checked deletion already flips the fingerprint gate; this just accelerates the in-process seed). Pre-v8 rows without breadcrumbs still get the identifier null-out and seed once the next persist round back-fills their breadcrumbs. 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>
…former-RSA rung
Round-3 verification finding: tryFormerRsaRecovery collapsed every
GeneralSecurityException to null ('not this key'), which swallowed a
KPIE from an invalidated pre-alias-split key — the signer's classifier
never saw it, so the typed completion and the durable repair seeding
(finding-3 hook) never fired for legacy blobs. KPIE now propagates like
UserNotAuthenticatedException, with a matrix row pinning the rethrow and
the post-regen recovery path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
force-pushed
the
port/v4.1/android-integration
branch
from
July 21, 2026 23:26
00804cd to
ce3490a
Compare
bfoss765
changed the title
bfoss765
changed the title
Collaborator
|
Consolidated re-review of the rework (two independent passes). First the good news: all seven previously-blocking items are genuinely resolved — invalidated-key cleanup survives with generation-checked deletion, the retrieve ladder layers legacy shape-dispatch before the fingerprint gate, However, the rework surfaces new blocking findings:
|
…s and verify the pubkey before persisting Blocker 1 (shumkov): the repair path derived from caller-supplied indices (the example app passed the DPP key id as keyIndex), and its only check was probeIdentityKeyRecoverability — which proves the stored blob DECRYPTS, not that the derived key is the RIGHT one. A wrong index derives a different valid scalar that round-trips fine, clears pending state, and persists an unusable key. Fix: - IdentityKeyPrivateKeyDeriver: on the force (repair) path derive the KEYPAIR and verify the derived public key equals publicKeyData BEFORE any store; a mismatch throws IdentityKeyDerivationMismatchException without persisting. - PlatformWalletPersistenceHandler.repairIdentityKeyDurably: new orchestration that reads the derivation slot from the PERSISTED public_keys breadcrumbs (derivationIdentityIndex/derivationKeyIndex), never a caller index; a row without breadcrumbs fails the repair without clearing pending. Hoisted here (not the JVM-unconstructable manager) so it is unit-testable and shares the authoritative pendingIdentityKeys state. - PlatformWalletManager.repairIdentityKey: drop the identityIndex/keyIndex params and delegate; supply only the wallet-scoped probe. - WalletKeyHealthSheet: stop passing report.identityIndex / key.keyId. Tests (PlatformWalletPersistenceHandlerTest): correct breadcrumbs derive, verify, clear pending, and record the durable identifier (deriver called with the persisted 3/5, not a caller index); mismatched breadcrumbs are rejected (mismatch exception), nothing persisted, blob never probed, pending intact; a row without breadcrumbs fails with SigningKeyUnavailable and never derives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Blocker 3 (shumkov): in the repair path the Room privateKeyKeychainIdentifier update was wrapped in runCatching (swallowed on failure) while the pending state was cleared regardless. A failed durable write then resurrected the repair after restart (the reconstruction reads the persisted rows) while the live session believed it was done — session and restart disagreed. Fix: the durable write in repairIdentityKeyDurably now fails CLOSED — it runs through an injectable persistDurableIdentifier seam (default: the production public_keys write) and its exception propagates; markIdentityKeyRepaired only runs after the write commits. A failed durable write leaves pending intact and the repair retryable, so the session and a restart agree. Test: a repair whose derive + verify succeed but whose durable write throws propagates the failure and leaves the pending entry in place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…horitative signals Blocker 2 (shumkov): isNoSecureLockScreenKeyGenFailure classified any KeyStoreException whose message contained "generate_key" as a lock-screen rejection. But "generate_key" names the failing operation, not its cause — a transient KeyMint generation failure on a device WITH a lock screen carries it too, so a transient failure would silently and PERMANENTLY downgrade an AUTH_GATED key to DEVICE_BOUND instead of retrying. Fix: drop the bare "generate_key" substring match. Classification now rests only on the two authoritative signals — explicit lock-screen text, or the lock-screen rejection numeric code (internal Keystore 4 / KeyMint 10309, which the real android.security.KeyStoreException exposes via getNumericErrorCode()). Tests: the on-device shape now classifies via the numeric code (not the incidental generate_key text); two new negatives — a bare generate_key failure, and a transient generate_key failure under a key-gen ProviderException with a non-lock-screen numeric code — must NOT classify, so AUTH_GATED is retried, not downgraded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…olicy-alias migration Finding 4 (shumkov): migrateToPolicyAlias wrapped its encrypt + DataStore edit in runCatching, which swallows kotlin CancellationException along with genuine rewrite failures. A coroutine cancelled during the migration's suspend points was silently absorbed instead of unwinding — a structured-concurrency violation. Fix: replace runCatching with an explicit try/catch that rethrows CancellationException; only genuine rewrite failures stay best-effort (the recovered value is still returned; migration retries on the next read). Test: a cancellation landing at the migration encrypt (via the fake keystore's onNextPolicyEncrypt hook) now propagates out of retrievePrivateKey rather than being swallowed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fallback Finding 5 (shumkov): the DEVICE_BOUND documentation promised hardware-backed storage while the implementation permits a software AndroidKeyStore fallback (generation prefers StrongBox, falls back to the TEE, and finally to a software-backed AndroidKeyStore key on devices with no secure element — generateWithLockScreenDegradation never fails generation on a missing one). Align the docs: "device-bound" means non-exportable AndroidKeyStore, not a hardware-storage guarantee; backing is hardware-isolated only where the device provides it. AUTH_GATED shares the same backing characteristics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
All five new findings addressed (255 unit tests, 0 failures):
Two honest residuals, both fail-closed (neither can persist a wrong key): the real deriver's FFI pubkey-equality is exercised on-device rather than in a JVM unit test (AndroidKeyStore crypto can't run on the JVM) — it's in our device test plan; and on API 29–32 the numeric error code isn't available, so an exotic "reports-secure-but-rejects" OEM quirk on old Android wouldn't be text-classified, though genuine lockless devices are still caught by the primary |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #4191 (
port/v4.1/unmanaged-identity-reads) — review only thecommits above
ce3a17d71f. Once #4191 merges,git rebase --onto origin/v4.1-dev ce3a17d71fand force-push with--force-with-lease.This replaces the old single-squash
port/v4.1/android-integration(preserved at
backup/port-4060-pre-rework). That squash predated #4172 andsilently rewrote the exact regions #4172 later hardened; this branch is a
rebuild on the current base with each review finding resolved explicitly.
The unmanaged-identity reads and the text-classified SigningKeyUnavailable
from the old squash shipped separately as #4191 and are NOT duplicated here.
Scope — Keystore only, one logical commit per concern:
KeySecurityPolicy(AUTH_GATED default /DEVICE_BOUND opt-in), two dedicated RSA aliases, legacy
KEYS_ALIASread-only; fix(kotlin-sdk): harden identity-key Keystore recovery and reconcile parity docs #4172's fingerprint capture + invalidation recovery survive
alias-parameterized (finding 1).
redirect to the DEVICE_BOUND alias (never a silently gate-less key under
the auth-gated alias);
effectiveKeySecurityPolicy()+requireAuthGatedstrict mode; per-blob alias tag routes reads;tightened KeyMint failure classifier (finding 4).
never applies to legacy blobs, finding 2), fingerprint fast path with
KPIE rethrow-after-generation-checked-cleanup (brick-loop fix, finding 1),
mismatch/missing fp routes into former-RSA recovery, migration forward.
Cheap
canSignWithnever decrypts; the real-decrypt probe moved toprobeIdentityKeyRecoverability(finding 3).pendingIdentityKeysStateFlow,round-transactional staging.
restart reconstruction incl. blobs stranded by keypair replacement
(finding 5).
replacePrivateKey+forcederive path +real-decrypt verification before
markIdentityKeyRepaired(finding 6).PlatformWallet.NotFound(Swift parity; releasenote in the commit).
error_codeon the signcompletion ABI,
ErrorSigningKeyUnavailable = 31(26-30 reserved forfeat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4185/fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) #4184), Kotlin/Swift typed mappings; MESSAGE_MARKER sniff retained as a
deprecated fallback for the fix(kotlin-sdk): unmanaged-identity reads return absence + typed SigningKeyUnavailable (split from #4183) #4191 merge-order transition (mixed
old-native/new-Kotlin artifacts are unsupported — the completion JNI
arity changed) (finding 7). Known residual: the
Rust-internal segment rides a stable machine prefix through
ProtocolError::Generic— a typed rs-dpp variant was deliberatelyavoided (serialization blast radius).
InvalidKeyException failures were the pre-fix(kotlin-sdk): harden identity-key Keystore recovery and reconcile parity docs #4172 emulator-relock flake —
inherited workflow guard covers them).
convergent, pending-repair gap recorded for Swift, code-31 capability),
migration ladder v8 note.
CI expectations: emulator job runs
:sdk:connectedDebugAndroidTest(watchthe
deviceLocked=0guard); swift job rebuilds the xcframework so the5-arg completion ABI and code 31 compile against fresh headers.
Round-3 addendum: verification passes found and fixed three repair-routing defects (health-probe fingerprint disproof for replaced aliases, typed first-attempt invalidation errors, durable repair seeding for legacy and former-RSA keys — commits
6d1fa5be83,f6fe865e56,7ee6e4b7f9,ce3490a3a0). Unit suite: 249 passing; instrumented suite runs on this repo's emulator job.🤖 Generated with Claude Code
Summary by CodeRabbit