fix(sdk): keep Keystore's unlocked-device gate from bricking wallets and signing on defective OEM builds - #4643
Open
HashEngineering wants to merge 4 commits into
Open
Conversation
…ocked devices Some OEM builds (HONOR/MagicOS Android 16 in the field; same mechanism as Google Issue Tracker 506989112 on Fairphone) perform unlocks that never satisfy the Keystore's UNLOCKED_DEVICE_REQUIRED gate, so the lock-bound master-alias key stays denied for the whole unlock session while KeyguardManager reports the device unlocked. storeMnemonic's bounded false-locked retry (built for the transient Keystore2 blip) can never outwait that, so wallet creation was unfixably failing on those devices. Add a last rung to the ladder: when the retry schedule exhausts still false-locked, treat the device's UNLOCKED_DEVICE_REQUIRED implementation as defective and store under a new never-lock-bound alias (MASTER_ALIAS_UNBOUND — same hardware-backed non-auth AES-256-GCM, no setUnlockedDeviceRequired ever), recording the defect durably in the same atomic edit. From then on mnemonic writes go straight to the unbound alias, the createWallet preflight stops probing, reads route by the blob's recorded alias (mnemonicalias.<walletIdHex>, the privkeyalias discipline), and pre-existing lock-bound blobs are re-wrapped best-effort on their first successful read. Nothing is ever deleted or re-keyed, genuinely-locked denials keep failing fast, the auth-gated identity aliases are untouched, and healthy devices never provision the new alias — this is the #4060 no-lock-screen downgrade driven by operational evidence instead of a missing lock screen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWalletStorage now handles persistent false-locked Keystore failures for mnemonic and identity-key operations. It records the defect, uses never-lock-bound aliases, routes reads by alias, and rewraps existing blobs when possible. ChangesFalse-locked Keystore handling
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant WalletStorage
participant KeystoreManager
participant DataStore
WalletStorage->>KeystoreManager: encrypt mnemonic with MASTER_ALIAS
KeystoreManager-->>WalletStorage: false-locked denial
WalletStorage->>KeystoreManager: retry with MASTER_ALIAS_UNBOUND
WalletStorage->>DataStore: save blob, alias tag, and defect record
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge risk remains in the reviewed Keystore recovery changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
HashEngineering
changed the title
Collaborator
|
🕓 Queued for automated review — 2nd in line, estimated start in ~10 min (commit 4a3c4e3)
|
thepastaclaw
suggested changes
Sep 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The fallback alias and durable defect marker are implemented coherently, and the retry/degradation paths have substantial coverage. However, the opportunistic re-wrap adds an unsynchronized write to the read path, allowing a deleted or concurrently replaced mnemonic to be restored. The change also modifies a public non-suspending method into a suspending method despite the PR declaring that there are no breaking changes; cancellation during re-wrap additionally leaves plaintext unsanitized.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This is a substantial security-sensitive storage and cryptographic key-management change that alters mnemonic encryption, alias selection, durable migration state, atomic persistence, and retry behavior, where regressions could affect wallet availability or seed protection. - Phase 1 reviewers: not run (skipped for throughput: 17 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🔴 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:481-483: Re-wrap can resurrect a mnemonic after deletion
`retrieveMnemonicUtf8` reads the mnemonic and alias from one DataStore snapshot, decrypts the plaintext, and then performs a separate `store.edit` through `rewrapMnemonicUnbound`. If `deleteMnemonic(walletId)` completes after the snapshot/decrypt but before that edit, the re-wrap writes the old ciphertext and alias back into the store, resurrecting a mnemonic that was just deleted. The same race can overwrite a newer mnemonic written concurrently. The atomicity of the re-wrap edit does not protect the read-to-write interval because the edit is not conditional on the original blob and alias still being present. Serialize re-wraps with mnemonic writes/deletes or perform a compare-and-set edit that only replaces the entry when the original encoded blob and alias still match.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:481-484: Cancellation during re-wrap can leave decrypted mnemonic bytes unsanitized
On the defect path, `plain` is decrypted and passed to `rewrapMnemonicUnbound` before ownership is returned to the caller. That function deliberately rethrows `CancellationException`; if cancellation occurs during the suspending `store.edit`, `retrieveMnemonicUtf8` exits without returning `plain`, so the caller cannot scrub it. The same ownership problem applies to an unexpected throwable from the re-wrap. Clear the plaintext buffer before propagating any re-wrap failure that prevents returning it, while preserving the existing best-effort behavior for ordinary re-wrap errors.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:250: Changing this public method to suspend is a breaking API change
`WalletStorage` is a public class and `ensureMasterKeyNotLockBlocked` is declared as a public method without an `internal` modifier. The base API exposed a regular `fun`; changing it to `suspend fun` requires source callers to be inside a coroutine and changes the generated JVM-facing method shape. The PR's assertion that the change is internal and has no external callers is not sufficient to preserve compatibility for consumers that access the public `WalletStorage` API. Preserve the existing public method with a blocking-free wrapper/alternative, introduce a separate suspending API while retaining the old signature, or explicitly treat and document this as a breaking API change.
Comment on lines
+481
to
+483
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | ||
| rewrapMnemonicUnbound(walletId, plain) | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Re-wrap can resurrect a mnemonic after deletion
retrieveMnemonicUtf8 reads the mnemonic and alias from one DataStore snapshot, decrypts the plaintext, and then performs a separate store.edit through rewrapMnemonicUnbound. If deleteMnemonic(walletId) completes after the snapshot/decrypt but before that edit, the re-wrap writes the old ciphertext and alias back into the store, resurrecting a mnemonic that was just deleted. The same race can overwrite a newer mnemonic written concurrently. The atomicity of the re-wrap edit does not protect the read-to-write interval because the edit is not conditional on the original blob and alias still being present. Serialize re-wraps with mnemonic writes/deletes or perform a compare-and-set edit that only replaces the entry when the original encoded blob and alias still match.
source: ['claude']
Comment on lines
+481
to
+484
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | ||
| rewrapMnemonicUnbound(walletId, plain) | ||
| } | ||
| return plain |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Cancellation during re-wrap can leave decrypted mnemonic bytes unsanitized
On the defect path, plain is decrypted and passed to rewrapMnemonicUnbound before ownership is returned to the caller. That function deliberately rethrows CancellationException; if cancellation occurs during the suspending store.edit, retrieveMnemonicUtf8 exits without returning plain, so the caller cannot scrub it. The same ownership problem applies to an unexpected throwable from the re-wrap. Clear the plaintext buffer before propagating any re-wrap failure that prevents returning it, while preserving the existing best-effort behavior for ordinary re-wrap errors.
Suggested change
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | |
| rewrapMnemonicUnbound(walletId, plain) | |
| } | |
| return plain | |
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | |
| try { | |
| rewrapMnemonicUnbound(walletId, plain) | |
| } catch (t: Throwable) { | |
| plain.fill(0) | |
| throw t | |
| } | |
| } |
source: ['claude']
| * no lock state can deny, so there is nothing to preflight. | ||
| */ | ||
| fun ensureMasterKeyNotLockBlocked(operation: String) { | ||
| suspend fun ensureMasterKeyNotLockBlocked(operation: String) { |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Changing this public method to suspend is a breaking API change
WalletStorage is a public class and ensureMasterKeyNotLockBlocked is declared as a public method without an internal modifier. The base API exposed a regular fun; changing it to suspend fun requires source callers to be inside a coroutine and changes the generated JVM-facing method shape. The PR's assertion that the change is internal and has no external callers is not sufficient to preserve compatibility for consumers that access the public WalletStorage API. Preserve the existing public method with a blocking-free wrapper/alternative, introduce a separate suspending API while retaining the old signature, or explicitly treat and document this as a breaking API change.
source: ['claude']
HashEngineering
added a commit
to dashpay/dash-wallet
that referenced
this pull request
Sep 10, 2026
…O-972)
Two QA reports on 12000007, two different failures, and neither log could
say what actually went wrong.
MO-973 (SM-A536B, shielded): the identity was created, then DPNS name
registration failed three times with
IllegalStateException: username registration did not complete
(retryable): pre-broadcast identity-key validation failure
That reason is a message-match on the FFI's "Invalid identity data", and
the label is the CONTACT-REQUEST reading of it — a missing
ECDSA_SECP256K1 encryption key. It was reported verbatim for a DPNS
registration, which needs no encryption key. The message is genuinely
overloaded: the invitation amount-cap rejection arrives with the same
prefix ("Invalid identity data: invitation amount ... exceeds the cap"),
which InviteCreationFailureTest has been pinning all along. So the label
named a cause nobody had established.
Worse, the engine's own message never reached the log at all:
RestoreIdentityWorker threw via `error(...)`, which builds an
IllegalStateException WITHOUT a cause, so `result.cause` was dropped on
the floor. BaseWorker does log.error(msg, e), so a cause WOULD have
printed as a "Caused by:" chain — there just wasn't one. The real reason
was unrecoverable from the report.
MO-972 (HONOR PTP-N49, transparent): reported as
signing failure (pre-broadcast): Keystore auth window expired
11:49:16 SendCoinsTaskRunner - authenticate with biometric
11:49:17 transparent identity funding rejected pre-broadcast
One second. The window had not expired — the label asserted a timeout
nobody measured, and sent diagnosis the wrong way. The raw error is
"Generic Error: User not authenticated": the Keystore refused to treat a
fresh biometric as satisfying the identity key's auth gate. On the
Samsung the same flow gets PAST signing and fails later, differently, so
this is device-specific — the same OEM Keystore defect family as the
false-locked master alias, on the auth-gated identity alias that
dashpay/platform#4643 explicitly does not cover (#4060's DEVICE_BOUND
policy is the remedy). Nothing to fix in the wallet beyond not lying
about the cause.
So:
- both reasons now carry the engine's message verbatim;
- the auth reason states the refusal and offers expiry as one
POSSIBILITY rather than a fact;
- RestoreIdentityWorker and CreateIdentityService's invite path throw
with `result.cause` attached, so the "Caused by:" chain reaches the
log.
Checked the coupling before changing the strings:
classifyInviteCreationFailure matches over the reason AND the whole cause
chain, so REJECTED/UNREACHABLE verdicts are unchanged — the cause still
carries "Invalid identity data". Its two test literals are updated to
mirror the new production strings, with that coupling pinned so the next
reason-string edit fails loudly instead of silently reclassifying invite
failures.
Tests: 2 added. One asserts two different "Invalid identity data"
failures no longer read alike (missing-encryption-key vs amount-cap) —
the whole point of the change. One asserts the auth reason does not claim
an expiry as fact while staying recognisable. Both mutation-verified by
restoring the old labels. Full :wallet suite green.
This is diagnosis, not a fix: MO-973's actual cause is still unknown and
the next field report is what will name it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HashEngineering
added a commit
to dashpay/dash-wallet
that referenced
this pull request
Sep 10, 2026
…O-972)
Two QA reports on 12000007, two different failures, and neither log could
say what actually went wrong.
MO-973 (SM-A536B, shielded): the identity was created, then DPNS name
registration failed three times with
IllegalStateException: username registration did not complete
(retryable): pre-broadcast identity-key validation failure
That reason is a message-match on the FFI's "Invalid identity data", and
the label is the CONTACT-REQUEST reading of it — a missing
ECDSA_SECP256K1 encryption key. It was reported verbatim for a DPNS
registration, which needs no encryption key. The message is genuinely
overloaded: the invitation amount-cap rejection arrives with the same
prefix ("Invalid identity data: invitation amount ... exceeds the cap"),
which InviteCreationFailureTest has been pinning all along. So the label
named a cause nobody had established.
Worse, the engine's own message never reached the log at all:
RestoreIdentityWorker threw via `error(...)`, which builds an
IllegalStateException WITHOUT a cause, so `result.cause` was dropped on
the floor. BaseWorker does log.error(msg, e), so a cause WOULD have
printed as a "Caused by:" chain — there just wasn't one. The real reason
was unrecoverable from the report.
MO-972 (HONOR PTP-N49, transparent): reported as
signing failure (pre-broadcast): Keystore auth window expired
11:49:16 SendCoinsTaskRunner - authenticate with biometric
11:49:17 transparent identity funding rejected pre-broadcast
One second. The window had not expired — the label asserted a timeout
nobody measured, and sent diagnosis the wrong way. The raw error is
"Generic Error: User not authenticated": the Keystore refused to treat a
fresh biometric as satisfying the identity key's auth gate. On the
Samsung the same flow gets PAST signing and fails later, differently, so
this is device-specific — the same OEM Keystore defect family as the
false-locked master alias, on the auth-gated identity alias that
dashpay/platform#4643 explicitly does not cover (#4060's DEVICE_BOUND
policy is the remedy). Nothing to fix in the wallet beyond not lying
about the cause.
So:
- both reasons now carry the engine's message verbatim;
- the auth reason states the refusal and offers expiry as one
POSSIBILITY rather than a fact;
- RestoreIdentityWorker and CreateIdentityService's invite path throw
with `result.cause` attached, so the "Caused by:" chain reaches the
log.
Checked the coupling before changing the strings:
classifyInviteCreationFailure matches over the reason AND the whole cause
chain, so REJECTED/UNREACHABLE verdicts are unchanged — the cause still
carries "Invalid identity data". Its two test literals are updated to
mirror the new production strings, with that coupling pinned so the next
reason-string edit fails loudly instead of silently reclassifying invite
failures.
Tests: 2 added. One asserts two different "Invalid identity data"
failures no longer read alike (missing-encryption-key vs amount-cap) —
the whole point of the change. One asserts the auth reason does not claim
an expiry as fact while staying recognisable. Both mutation-verified by
restoring the old labels. Full :wallet suite green.
This is diagnosis, not a fix: MO-973's actual cause is still unknown and
the next field report is what will name it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The degradation ladder added in the previous commit can only ever be entered from storeMnemonic, so the defect is discoverable only while WRITING a mnemonic — in practice only at wallet creation. On a wallet that predates the degradation the blob stays under the lock-bound MASTER_ALIAS, no mnemonic is ever written again, and nothing sets MASTER_LOCK_DEFECT_KEY. Since retrieveMnemonicUtf8's opportunistic re-wrap is GATED on that record, the self-heal it exists to provide can never fire on exactly the devices that need it most: the ones already carrying a wallet when the defective OEM gate shows up. Give the read the same bounded ladder as the write. The retry/classify loop both paths now share moves into retryingFalseLockedDenial, so a genuinely-locked denial still fails fast, a transient Keystore2 blip is still retried, and only a denial that outlasts the whole schedule counts as the defect. A denied read cannot heal itself — a refused decrypt never obtained the plaintext to re-encrypt — so it records the device and lets the original typed denial propagate. That record is the missing link: the next read that gets through (the gate jams for stretches of a session, not forever) finally re-wraps the blob onto the never-lock-bound alias, and later writes skip the lock-bound alias outright. Recording is best-effort — a DataStore failure is attached as suppressed rather than replacing the truthful, retryable denial. Two doc corrections found while doing it. isMasterKeyLockBindingDefectObserved claimed the record is "never cleared", but deleteAll() wipes the whole store including it; carving it out is the wrong fix — the test suite depends on deleteAll restoring a clean slate, and re-deriving the record costs one ladder (~2s) on a wiped store's next write — so the claim is narrowed to the targeted mutators and deleteAll's contract is stated. DEVICE_FALSE_LOCKED_RETRY_DELAYS_MS and MASTER_LOCK_DEFECT_KEY were declared `internal` inside a `private companion object`, where internal is inert; they are private and now say so. Four tests, all on the read side the previous commit left untested: fail-fast when genuinely locked, no branding when a retry succeeds, recording when the schedule exhausts, and the end-to-end field shape — a wallet whose defect only a read ever observes still ends up off the defective gate. 432 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…signing
MO-972: DashPay username creation fails outright on a HONOR PTP-N49
(MagicOS, Android 16). Signing an identity state transition dies
pre-broadcast with "Protocol error: Generic Error: User not
authenticated", one second after a successful biometric, twice. The
wallet reported it as "Keystore auth window expired".
There is no auth window. The wallet has run KeySecurityPolicy.DEVICE_BOUND
since dash-wallet 7e7d53485 precisely to be rid of the authentication
gate, and that worked — KEYS_ALIAS_DEVICE_BOUND carries no
setUserAuthenticationRequired. What it does still carry is
setUnlockedDeviceRequired, applied by ensureKeysKeyPair to every alias on
any device with a lock screen. Android reports a denial of THAT gate with
the same UserNotAuthenticatedException it uses for a closed auth window,
and this device's OEM Keystore denies it while KeyguardManager reports
the device unlocked — the defect the previous commits already handle for
the master alias (cf. Google Issue Tracker 506989112, confirmed by Google
on Fairphone 5/6; AOSP ties UNLOCKED_DEVICE_REQUIRED availability to how
the device was unlocked).
The SDK could not tell the two apart because it never tried:
KeystoreManager.decrypt returns early for identity aliases and never
reaches rethrowClassifyingDeviceLockedDenial, whose allowlist was
MASTER_ALIAS alone. So the bare exception arrived at KeystoreSigner,
which read it as a closed auth window, looked for a BiometricGate to
re-prompt with, found none wired, and completed the sign generically.
Classify it where it is unambiguous. The allowlist becomes
{MASTER_ALIAS, KEYS_ALIAS_DEVICE_BOUND} — both lock-bound and NOT
auth-gated, so the exception can only mean the lock gate.
KEYS_ALIAS_AUTH_GATED stays excluded, since it carries both gates and
only the auth one is fixable by prompting; the *_UNBOUND aliases stay
excluded because they carry neither and must not promise a retry no
unlock can satisfy.
Then give identity keys the master alias's degradation ladder, targeting
a new never-lock-bound KEYS_ALIAS_DEVICE_BOUND_UNBOUND. A denied read
retries, records the device, and propagates truthfully; once recorded,
new identity-key writes skip the lock-bound alias, and the first read
that gets through re-wraps the stranded blob through the existing
conditional migration, which now resolves the EFFECTIVE write alias
rather than blindly the policy alias. Dropping lock binding costs nothing
DEVICE_BOUND ever promised — hardware-backed where available,
non-exportable and never auth-gated all survive; only the incidental
"unlocked right now" hardening goes, on a device where that gate is
broken anyway. AUTH_GATED is deliberately NOT given an unbound variant:
its authentication gate is the real control, and no field evidence puts a
defective device on it.
Classifying also fixes two silent mistakes that only appear now the typed
exception exists. KeystoreDeviceLockedException is a
GeneralSecurityException, so retrievePrivateKey's recovery ladder and
tryFormerRsaRecovery would have swallowed a lock denial into "wrong key"
and returned null — a spurious re-derive for an intact key — and
probeOpensBlob would have reported that key strandable to the health
sheet. Both now treat it as what it is: retryable, and recoverable.
Nine tests. Two pin the classifier allowlist prompt-free; seven cover the
storage ladder in a new WalletStorageIdentityKeyLockDefectTest — fail
fast when genuinely locked, no branding when a retry succeeds, recording
when the schedule exhausts, writes moving off the gate, the end-to-end
field shape where only a read ever observes the defect, best-effort
re-wrap failure, and the no-spurious-re-derive guarantee. 441 tests, 0
failures, debug and release.
Device verification is still owed: emulators classify every denial as
genuinely locked, so the defective-OEM branch is unreachable there and
this needs the HONOR PTP-N49 with QA.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… it now covers The class doc still named MASTER_ALIAS as the only thrower and excluded 'the auth-gated identity-key aliases' as a group. Both went stale in the previous commit: KEYS_ALIAS_DEVICE_BOUND now throws it too, and the exclusion is specifically KEYS_ALIAS_AUTH_GATED (both gates, ambiguous) plus the *_UNBOUND aliases (neither gate, so not a lock denial at all). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HashEngineering
changed the title
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
2 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.
Issue being fixed or feature implemented
Some OEM builds deny Keystore operations on keys carrying
setUnlockedDeviceRequiredwhileKeyguardManagerreports the device unlocked. Google confirmed the same defect on Fairphone 5/6 (Issue Tracker 506989112); AOSP tiesUNLOCKED_DEVICE_REQUIREDavailability to how the device was unlocked (class 1/2 biometrics vs class 3/LSKF). We see it on HONOR PTP-N49 (MagicOS, Android 16).The SDK stamps that gate on every alias on any device with a lock screen, so the defect lands twice:
storeMnemonic's master-alias encrypt is denied. The bounded retry was built for a transient Keystore2 blip and cannot outwait a defect that persists for the unlock session, so creation failed unfixably.Protocol error: Generic Error: User not authenticated, one second after a successful biometric, reproducibly. The wallet reported it as "Keystore auth window expired" — but there is no auth window: the wallet has runKeySecurityPolicy.DEVICE_BOUNDsince dash-wallet7e7d53485precisely to be rid of the authentication gate, and that works.KEYS_ALIAS_DEVICE_BOUNDstill carries the lock gate, and Android reports both denials with the identicalUserNotAuthenticatedException. The SDK never disambiguated them, so the failure was unreadable in the field for weeks.What was done?
Tell the two gates apart
KeystoreManager.decryptreturned early for identity aliases and never reachedrethrowClassifyingDeviceLockedDenial, whose allowlist wasMASTER_ALIASalone. The allowlist is nowUNAMBIGUOUS_LOCK_BOUND_ALIASES = {MASTER_ALIAS, KEYS_ALIAS_DEVICE_BOUND}— both lock-bound and not auth-gated, so the exception can only mean the lock gate.KEYS_ALIAS_AUTH_GATEDstays excluded: it carries both gates, and only the auth reading is fixable by prompting. Classifying it would strand theBiometricGateprompt-and-retry contract.*_UNBOUNDaliases stay excluded: neither gate, so a denial there is not a lock denial and must not promise a retry no unlock can satisfy.Degrade off the broken gate, per device, on evidence
Two never-lock-bound aliases, provisioned lazily and only on a device that demonstrated the defect —
MASTER_ALIAS_UNBOUND(AES) andKEYS_ALIAS_DEVICE_BOUND_UNBOUND(RSA). Both bypass the lock-screen ladder ingenerateAesKey/ensureKeysKeyPairunconditionally.The ladder itself is shared by every lock-bound operation (
WalletStorage.retryingFalseLockedDenial): genuinely-locked fails fast, a transient blip is retried, and only a denial outlasting the whole schedule counts as the defect and is recorded durably inMASTER_LOCK_DEFECT_KEY.storeMnemonicdegrades in place (it holds the plaintext) and records atomically with the healed blob. Identity writes route throughencryptIdentityKeyOffDefectiveGate.recordLockBindingDefectFromDeniedRead). That record is load-bearing: it arms the opportunistic re-wrap, which is the only route on a wallet whose blob predates the degradation — nothing writes that secret again, so the write ladder never runs.mnemonicalias.<walletIdHex>,privkeyalias.<pubkeyHex>) route reads, so nothing is ever deleted or re-keyed and healthy devices never provision the new aliases.AUTH_GATEDdeliberately gets no unbound variant: its authentication gate is the real control, and no field evidence puts a defective device on it.Three latent bugs the typed exception exposed
KeystoreDeviceLockedExceptionis aGeneralSecurityException, so once identity denials became typed, existing broad catches would have misread them:retrievePrivateKey's recovery ladder andtryFormerRsaRecoverywould have absorbed a lock denial as "wrong key" →null→ a spurious re-derive of an intact key.probeOpensBlobwould have reported that key strandable to the key-health sheet.All three now treat it as retryable and recoverable.
KeystoreSignerdocuments why the typed exception deliberately bypasses theBiometricGate: the gate tracks device lock state, not authentication recency, so a prompt would burn a user interaction and fail identically.Security note
Dropping lock binding costs nothing
DEVICE_BOUNDever promised — hardware-backed where the device provides it, non-exportable, and never auth-gated all survive. Only the incidental "device unlocked right now" hardening is given up, on a device where that gate is broken anyway, and only after that device proves it.How Has This Been Tested?
441 tests, 0 failures, debug and release variants (428 on
v4.2-dev). Counts read fromsdk/build/test-results/**/TEST-*.xml— Gradle only prints a count on failure.WalletStorageDeviceLockedRetryTest(17 → 21) — the master-alias ladder: createWallet pre-check, retry/fail-fast, degradation and its failure path, buffer scrubbing; new: the read side (fail fast when genuinely locked, no branding when a retry succeeds, recording on exhaustion, and the field shape where only a read ever observes the defect).WalletStorageIdentityKeyLockDefectTest(new, 7) — the identity-key ladder end to end, including writes moving off the gate, best-effort re-wrap failure, and the no-spurious-re-derive guarantee.KeystoreDeviceLockedDenialTest(9 → 11) — pins the classifier allowlist prompt-free:KEYS_ALIAS_DEVICE_BOUNDmaps,KEYS_ALIAS_AUTH_GATEDand both*_UNBOUNDaliases do not.Device verification is still owed. Emulators cannot reproduce this class of defect — AOSP classifies every denial as genuinely locked, so the defective-OEM branch is unreachable. Needs the HONOR PTP-N49 (with QA). Expected outcome: username creation succeeds, or fails with an explicit message naming the alias and the
KeyguardManagerstate — never "auth window expired" again.Breaking Changes
None. New aliases are additive and lazily provisioned only on a device that demonstrates the defect; healthy devices are byte-for-byte unaffected. Nothing is deleted or re-keyed, and pre-existing blobs stay readable under their recorded alias.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests