Skip to content

fix(kotlin-sdk): Android host-app integration fixes (key security policy, unmanaged-identity reads, typed signing error)#4060

Open
bfoss765 wants to merge 4 commits into
dashpay:feat/kotlin-sdk-and-example-appfrom
bfoss765:fix/kotlin-sdk-android-integration
Open

fix(kotlin-sdk): Android host-app integration fixes (key security policy, unmanaged-identity reads, typed signing error)#4060
bfoss765 wants to merge 4 commits into
dashpay:feat/kotlin-sdk-and-example-appfrom
bfoss765:fix/kotlin-sdk-android-integration

Conversation

@bfoss765

@bfoss765 bfoss765 commented Jul 9, 2026

Copy link
Copy Markdown

Kotlin-only fixes for the three SDK issues blocking the Dash Android wallet's Platform migration (dashpay/dash-wallet#1507). No native/Rust changes; the prebuilt library is untouched. Defaults are unchanged everywhere.

1. Configurable identity-key security policy + queryable pending-key state (fixes #4053)

Identity private keys were always wrapped under the auth-gated Keystore alias (KeystoreManager.KEYS_ALIAS, setUserAuthenticationRequired(true), ~30 s post-unlock validity) with no configurability — a host app that gates wallet access behind its own auth model (the Dash wallet gates by PIN app-side) fails every sign outside the window, and a failed derive/store during persistence was silently swallowed.

  • New KeySecurityPolicy enum: AUTH_GATED (default — exact historical behavior, same alias, same auth window) and DEVICE_BOUND (a separate non-auth-gated but still hardware-backed / StrongBox-preferring, non-exportable RSA wrapping alias, org.dashfoundation.wallet.keys.devicebound). Distinct aliases because Keystore auth parameters are fixed at key generation; the KDoc documents the pick-once-per-install stability requirement.
  • KeystoreManager(keySecurityPolicy) selects the alias (keysAlias); the RSA helpers are parameterized per alias, and the user-auth gate is only applied when generating the AUTH_GATED alias.
  • WalletStorage(context, keySecurityPolicy) convenience constructor; identity-key encrypt/decrypt routes through keystore.keysAlias.
  • PlatformWalletPersistenceHandler no longer skips a failed identity-key derive/store silently: it logs at ERROR (the key is persisted watch-only and cannot sign) and records a queryable PendingIdentityKey on a StateFlow, cleared when a later persist round stores the key. Exposed as PlatformWalletManager.pendingIdentityKeys, repairable via the existing repairIdentityKey.

2. Dashpay reads return null/empty for unmanaged identities (fixes #4051)

Dashpay.syncState (and payments/contacts/acceptIncomingRequest) threw DashSdkError.PlatformWallet.Generic("requested …ManagedIdentity not found") for identities the wallet does not manage, even though every signature already models absence. The native platform_wallet_get_managed_identity reports the miss via the FFI's blanket Option → result conversion (PlatformWalletFFIResultCode::NotFound, 98) instead of a zero handle, so the Kotlin boundary now translates that specific error to the existing zero-handle path: syncState/payments return null, contacts returns empty lists, acceptIncomingRequest returns false. Other native errors are rethrown untouched.

3. Typed SigningKeyUnavailable error (fixes #4052)

Signing failures caused by a missing stored private key ("no private key stored for ") surfaced as the opaque DashSdkError$PlatformWallet$Generic. The message originates in Kotlin (KeystoreSigner.completeSign), travels through Rust as free text, and returns under the catch-all platform-wallet codes — so the typed mapping happens on the Kotlin boundary where it is reliable: a new DashSdkError.PlatformWallet.SigningKeyUnavailable with a shared MESSAGE_MARKER constant that KeystoreSigner builds its error from (emitter and matcher cannot drift). fromPlatformWalletNative matches the marker on the catch-all codes only; the dedicated retry-semantics codes (16/18/19/20) are never overridden.

Testing

:sdk:testDebugUnitTest — BUILD SUCCESSFUL. New coverage: KeySecurityPolicyTest (alias/policy plumbing, defaults), pending-identity-key tests in PlatformWalletPersistenceHandlerTest (failure recorded, null-identifier recorded, later success clears), ManagedIdentityNotFoundTranslationTest (NotFound → zero handle, other errors rethrown), and DashSdkErrorTest additions (marker mapping on codes 6/99, retry-semantics codes never overridden, NotFound code pin).

Refs dashpay/dash-wallet#1507.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added configurable identity-key security policies: authentication-gated or device-bound storage.
    • Added automatic migration support for legacy encrypted identity keys.
    • Added visibility into identity keys pending repair, with updates when repairs complete.
  • Bug Fixes

    • Improved reporting for unavailable signing keys.
    • Managed identity “not found” cases are handled gracefully during local operations.
    • Improved platform-wallet error classification and retry behavior.
  • Tests

    • Added coverage for security policies, legacy-key migration, pending repairs, and error handling.

bfoss765 and others added 3 commits July 9, 2026 07:25
…le pending-key state

Fixes dashpay#4053.

Identity private keys were always wrapped under the auth-gated Keystore
alias (KEYS_ALIAS, setUserAuthenticationRequired, ~30s post-unlock
validity) with no way for a host app that gates wallet access behind its
own auth model (e.g. an app-side PIN, as the Dash Android wallet does) to
opt out — sign-time decrypts outside the window fail, and there was no
way to even see that a key had been persisted watch-only.

- New KeySecurityPolicy enum: AUTH_GATED (default — behavior unchanged,
  same alias, same auth window) and DEVICE_BOUND (a separate
  non-auth-gated but still hardware-backed / StrongBox-preferring,
  non-exportable RSA wrapping alias). Distinct aliases because Keystore
  auth parameters are fixed at key generation.
- KeystoreManager takes the policy at construction and exposes the
  selected keysAlias; the RSA helpers are parameterized per alias and the
  user-auth gate is applied only to the AUTH_GATED alias.
- WalletStorage gains a policy-taking convenience constructor and routes
  identity-key encrypt/decrypt through the keystore's keysAlias.
- PlatformWalletPersistenceHandler no longer skips a failed identity-key
  derive/store silently: it logs at ERROR (the key is persisted
  watch-only and cannot sign) and records a queryable PendingIdentityKey
  entry on a StateFlow, cleared automatically when a later persist round
  stores the key. PlatformWalletManager exposes the flow as
  pendingIdentityKeys so hosts can surface a repair path
  (repairIdentityKey).

Defaults are unchanged everywhere: constructing KeystoreManager /
WalletStorage without a policy keeps the exact historical AUTH_GATED
behavior.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng keys

Fixes dashpay#4052.

A signing failure caused by a missing stored private key ("no private
key stored for <hex>") surfaced as the opaque
DashSdkError.PlatformWallet.Generic, indistinguishable from any other
wallet failure — hosts could not route the user to key repair.

The error text originates in Kotlin (KeystoreSigner's completeSign
error string), travels through Rust as free text, and returns under the
catch-all platform-wallet codes (ErrorUnknown via the blanket
PlatformWalletError conversion, or ErrorWalletOperation), so the typed
mapping happens on the Kotlin boundary where it is reliable:

- New DashSdkError.PlatformWallet.SigningKeyUnavailable with a
  MESSAGE_MARKER constant; KeystoreSigner now builds its "missing key"
  completion error from the same constant, so the emitter and the
  matcher cannot drift.
- fromPlatformWalletNative recognizes the marker on the catch-all codes
  only — the dedicated retry-semantics codes (16/18/19/20) are never
  overridden.

Also pins PlatformWalletFFIResultCode::NotFound (98) as
PLATFORM_WALLET_NOT_FOUND_CODE for the dashpay#4051 translation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ties

Fixes dashpay#4051.

Dashpay.syncState (and payments/contacts/acceptIncomingRequest) threw
DashSdkError.PlatformWallet.Generic("requested …ManagedIdentity not
found") when called for an identity the wallet does not manage — e.g. a
contact's identity — even though every signature already models the
absence (nullable String, empty Contacts, false).

The native platform_wallet_get_managed_identity reports the miss
through the FFI's blanket Option → result conversion
(PlatformWalletFFIResultCode::NotFound, 98) instead of returning a zero
handle, so the fix translates that specific error to a zero handle on
the Kotlin boundary (translateManagedIdentityNotFoundToZero) and routes
all managed-identity snapshots through it: syncState/payments now
return null, contacts returns empty lists, acceptIncomingRequest
returns false. Any other native error is rethrown untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bfoss765 bfoss765 requested a review from QuantumExplorer as a code owner July 9, 2026 11:26
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The Kotlin SDK adds selectable identity-key security policies, legacy key migration, typed missing-signing-key errors, pending identity-key repair tracking, and managed-identity NotFound translation for Dashpay flows.

Changes

Identity key security

Layer / File(s) Summary
Policy-based keystore aliases
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.kt, .../KeystoreManager.kt
Identity keys use separate auth-gated or device-bound RSA aliases, with policy-specific authentication requirements and legacy AES-GCM aliases retained for migration.
Legacy key recovery and storage integration
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt, .../KeystoreSigner.kt, packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/*
Legacy blobs can be recovered and best-effort migrated to RSA storage; missing private keys emit a stable marker, with policy, alias, blob, and storage behavior covered by tests.

Platform wallet error translation

Layer / File(s) Summary
Typed platform-wallet error mapping
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt, packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
Signing-key markers map to SigningKeyUnavailable, dedicated retry mappings remain unchanged, and the platform-wallet NotFound code maps to Generic.
Managed-identity NotFound handling
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt, packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.kt
Managed-identity NotFound errors become zero handles in affected Dashpay flows; other native errors are rethrown.

Pending identity-key repair

Layer / File(s) Summary
Pending-key persistence state
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt, packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt
Failed derivations create observable PendingIdentityKey entries, while successful re-persistence clears them and records repair metadata.
Repair-state integration
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
PlatformWalletManager exposes pending keys and clears repaired entries after successful private-key derivation and storage.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested labels: Client Only

Suggested reviewers: shumkov, llbartekll, lklimek, thepastaclaw

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and matches the main Kotlin SDK fixes, including key security policy, unmanaged-identity reads, and typed signing errors.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

✅ Review complete (commit d3aa786)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Source: reviewers = claude general opus (failed); claude security-auditor opus (failed); codex general gpt-5.5; codex security-auditor gpt-5.5; verifier = codex gpt-5.5; specialists = security-auditor; policy gate = review_policy.enforce_backport_prereq_policy.

Full-PR verification found two in-scope blocking correctness issues in the Kotlin SDK identity-key changes. No CodeRabbit findings were present, and there are no out-of-scope observations to preserve.

Findings

  1. [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:224-226
    Default key policy deletes the existing identity-key wrapping key.

    The default AUTH_GATED path still uses KEYS_ALIAS, but this code treats an existing AES SecretKey at that alias as a stale/wrong-type entry and deletes it before generating the RSA keypair. Existing installs have DataStore private-key blobs encrypted with that AES key; once the alias is deleted, retrievePrivateKey uses the new RSA private key against the old AES-GCM blob and cannot recover it. The PR documents the default as preserving historical behavior, so upgrade must either use a new RSA alias with AES fallback/migration or re-encrypt existing blobs before deleting the old alias.

  2. [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:284-289
    Successful key repair leaves pendingIdentityKeys stale.

    repairIdentityKey writes the repaired private key directly through identityKeyDeriver.deriveAndStore, but pendingIdentityKeys is owned by PlatformWalletPersistenceHandler and is only cleared by the private clearPendingIdentityKey path during a later successful onPersistIdentityKeyUpsert. A host watching the new pendingIdentityKeys flow will continue to see a repaired key as pending unless another persistence upsert happens for the same key, which is not guaranteed by the repair action. Route repair through the persistence handler or expose a success path that clears the pending entry when deriveAndStore returns a storage identifier.

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/KeystoreManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:224-226: Default key policy deletes the existing identity-key wrapping key
  The default AUTH_GATED path still uses KEYS_ALIAS, but this code treats an existing AES SecretKey at that alias as a stale/wrong-type entry and deletes it before generating the RSA keypair. Existing installs have DataStore private-key blobs encrypted with that AES key; once the alias is deleted, retrievePrivateKey uses the new RSA private key against the old AES-GCM blob and cannot recover it. The PR documents the default as preserving historical behavior, so upgrade must either use a new RSA alias with AES fallback/migration or re-encrypt existing blobs before deleting the old alias.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:284-289: Successful key repair leaves pendingIdentityKeys stale
  repairIdentityKey writes the repaired private key directly through identityKeyDeriver.deriveAndStore, but pendingIdentityKeys is owned by PlatformWalletPersistenceHandler and is only cleared by the private clearPendingIdentityKey path during a later successful onPersistIdentityKeyUpsert. A host watching the new pendingIdentityKeys flow will continue to see a repaired key as pending unless another persistence upsert happens for the same key, which is not guaranteed by the repair action. Route repair through the persistence handler or expose a success path that clears the pending entry when deriveAndStore returns a storage identifier.

Comment on lines +224 to +226
// Absent, or a stale symmetric entry from an earlier build (which
// gated encrypt too and broke unprompted writes) — drop and recreate.
runCatching { keyStore.deleteEntry(KEYS_ALIAS) }
runCatching { keyStore.deleteEntry(alias) }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Default key policy deletes the existing identity-key wrapping key

The default AUTH_GATED path still uses KEYS_ALIAS, but this code treats an existing AES SecretKey at that alias as a stale/wrong-type entry and deletes it before generating the RSA keypair. Existing installs have DataStore private-key blobs encrypted with that AES key; once the alias is deleted, retrievePrivateKey uses the new RSA private key against the old AES-GCM blob and cannot recover it. The PR documents the default as preserving historical behavior, so upgrade must either use a new RSA alias with AES fallback/migration or re-encrypt existing blobs before deleting the old alias.

source: ['codex']

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in d3aa786Default key policy deletes the existing identity-key wrapping key no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

…y; clear pending on repair

Two blocking correctness fixes from review.

1) KeystoreManager: the default AUTH_GATED policy generated the RSA wrapping
   keypair at KEYS_ALIAS — the same alias the pre-RSA scheme used for an
   auth-gated AES-256-GCM key — and ensureKeysKeyPair deleted that AES key
   before creating the RSA pair, stranding existing installs' AES-GCM identity-
   key blobs (data loss). Give AUTH_GATED its own RSA alias
   (KEYS_ALIAS_AUTH_GATED) and keep KEYS_ALIAS as a read-only legacy alias that
   is never deleted or regenerated. WalletStorage.retrievePrivateKey now
   detects a legacy AES-GCM blob, decrypts it with the retained legacy key, and
   transparently re-encrypts (migrates) it under the RSA alias; a fresh install
   has no legacy blobs and always takes the RSA path. isPrivateKeyDecryptable
   reports a legacy blob as recoverable while its key survives, so key-health
   no longer offers a needless re-derive for migratable keys.

2) PlatformWalletManager.repairIdentityKey wrote the re-derived key straight
   through the deriver, bypassing onPersistIdentityKeyUpsert (the only path that
   clears pendingIdentityKeys), so a repaired key lingered as pending. On a
   successful repair it now clears the entry via a new internal
   PlatformWalletPersistenceHandler.markIdentityKeyRepaired hook.

Tests: alias split + blob-type discrimination (RSA vs legacy AES) in
KeySecurityPolicyTest; markIdentityKeyRepaired clears the pending entry in
PlatformWalletPersistenceHandlerTest. (The Keystore crypto round trip itself
can't run on the JVM — AndroidKeyStore has no Robolectric provider — so the
alias/blob decision logic is what's unit-pinned.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Carried-Forward Prior Findings

Both prior blocking findings are FIXED: the legacy AES alias is retained, and successful repairs clear their pending-key entries.

New Findings In Latest Delta

The latest delta introduces one blocking RSA-alias migration regression and one concurrent StateFlow lost-update risk. The cumulative review also found a blocking error-translation defect in the preceding Dashpay commit.

Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); verifier=verifier-codex-fallback-4060-1783736084=gpt-5.6-sol fallback; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).

🔴 2 blocking | 🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/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:143-159: RSA keys stored under the previous alias are stranded
  The PR base already stored empty-IV RSA/OAEP blobs under `KeystoreManager.KEYS_ALIAS`. The latest delta moves AUTH_GATED operations to `KEYS_ALIAS_AUTH_GATED`, but only non-empty-IV AES blobs use the legacy fallback. Existing empty-IV blobs therefore reach line 159 and are decrypted with a newly generated key at the new alias instead of the RSA private key that remains at `KEYS_ALIAS`, making previously stored identity keys unusable after upgrade. `isPrivateKeyDecryptable` also reports these blobs as healthy based only on their shape. Detect whether `KEYS_ALIAS` contains the former RSA keypair, decrypt matching blobs with it, and migrate successful decryptions to the selected policy alias.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt:560-566: Invalid wallet handles are translated into unmanaged identities
  This catches every platform-wallet `NotFound` result and converts it to a zero identity handle. However, `platform_wallet_get_managed_identity` applies the same blanket Option-to-NotFound conversion both when `PLATFORM_WALLET_STORAGE.with_item` cannot find `wallet_handle` and when the nested managed-identity lookup misses. A `Dashpay` instance can retain its captured handle after its owning `ManagedPlatformWallet` is closed, so calls through that stale instance return null, empty collections, or false instead of propagating the native wallet failure. This contradicts the commit's guarantee that non-identity-miss errors remain untouched. Give the native lookup distinct errors for an invalid wallet and a missing managed identity, then translate only the latter.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:2135-2142: Concurrent pending-key updates can overwrite each other
  `recordPendingIdentityKey` and `clearPendingIdentityKey` perform non-atomic read-modify-write operations on `MutableStateFlow.value`. The persistence callback runs synchronously on its Rust caller thread, while the newly added `markIdentityKeyRepaired` can be invoked from an arbitrary host thread through `repairIdentityKey`. If a repair clears key A while a persistence callback records key B, both can read the same map and one write can discard the other. Losing the record leaves a watch-only key without the queryable pending state this PR adds; losing the clear leaves a repaired key stale. Use `MutableStateFlow.update` or another atomic synchronization mechanism for both mutations.

Comment on lines +143 to +159
if (keystore.isLegacyKeysBlob(blob)) {
// Legacy AES-GCM blob: recover with the retained legacy key (may
// throw UserNotAuthenticatedException — the legacy key was
// auth-gated — which the signer handles exactly as the RSA path),
// or null if that key is already gone (unrecoverable).
val plain = keystore.decryptLegacyKeysBlob(blob) ?: return null
// Opportunistically migrate to the RSA scheme. A rewrite failure
// must not lose the value we just recovered, so it stays best-effort
// and the read still returns the plaintext (migration retries next
// read).
runCatching {
val migrated = keystore.encrypt(plain, alias = keystore.keysAlias)
store.edit { it[privateKeyKey(pubkeyHex)] = encode(migrated) }
}
return plain
}
return keystore.decrypt(blob, alias = keystore.keysAlias)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: RSA keys stored under the previous alias are stranded

The PR base already stored empty-IV RSA/OAEP blobs under KeystoreManager.KEYS_ALIAS. The latest delta moves AUTH_GATED operations to KEYS_ALIAS_AUTH_GATED, but only non-empty-IV AES blobs use the legacy fallback. Existing empty-IV blobs therefore reach line 159 and are decrypted with a newly generated key at the new alias instead of the RSA private key that remains at KEYS_ALIAS, making previously stored identity keys unusable after upgrade. isPrivateKeyDecryptable also reports these blobs as healthy based only on their shape. Detect whether KEYS_ALIAS contains the former RSA keypair, decrypt matching blobs with it, and migrate successful decryptions to the selected policy alias.

source: ['codex-general', 'codex-security-auditor']

Comment on lines +560 to +566
internal inline fun translateManagedIdentityNotFoundToZero(getHandle: () -> Long): Long =
try {
getHandle()
} catch (e: DashSDKException) {
val notFound = DashSdkError.PLATFORM_WALLET_CODE_OFFSET +
DashSdkError.PLATFORM_WALLET_NOT_FOUND_CODE
if (e.code == notFound) 0L else throw e

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Invalid wallet handles are translated into unmanaged identities

This catches every platform-wallet NotFound result and converts it to a zero identity handle. However, platform_wallet_get_managed_identity applies the same blanket Option-to-NotFound conversion both when PLATFORM_WALLET_STORAGE.with_item cannot find wallet_handle and when the nested managed-identity lookup misses. A Dashpay instance can retain its captured handle after its owning ManagedPlatformWallet is closed, so calls through that stale instance return null, empty collections, or false instead of propagating the native wallet failure. This contradicts the commit's guarantee that non-identity-miss errors remain untouched. Give the native lookup distinct errors for an invalid wallet and a missing managed identity, then translate only the latter.

source: ['codex-general']

Comment on lines +2135 to +2142
private fun recordPendingIdentityKey(entry: PendingIdentityKey) {
_pendingIdentityKeys.value = _pendingIdentityKeys.value + (entry.publicKeyHex to entry)
}

private fun clearPendingIdentityKey(publicKeyHex: String) {
if (publicKeyHex in _pendingIdentityKeys.value) {
_pendingIdentityKeys.value = _pendingIdentityKeys.value - publicKeyHex
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Concurrent pending-key updates can overwrite each other

recordPendingIdentityKey and clearPendingIdentityKey perform non-atomic read-modify-write operations on MutableStateFlow.value. The persistence callback runs synchronously on its Rust caller thread, while the newly added markIdentityKeyRepaired can be invoked from an arbitrary host thread through repairIdentityKey. If a repair clears key A while a persistence callback records key B, both can read the same map and one write can discard the other. Losing the record leaves a watch-only key without the queryable pending state this PR adds; losing the clear leaves a repaired key stale. Use MutableStateFlow.update or another atomic synchronization mechanism for both mutations.

source: ['sonnet5-security-auditor']

@bfoss765

Copy link
Copy Markdown
Author

Both confirmed and fixed. AUTH_GATED now uses a new RSA alias (KEYS_ALIAS_AUTH_GATED); the legacy KEYS_ALIAS AES key is kept read-only and never deleted, and retrievePrivateKey decrypts legacy AES-GCM blobs and migrates them to the RSA alias (fresh installs take the RSA path only). repairIdentityKey now clears the pending entry via a new markIdentityKeyRepaired hook on the persistence handler. Added tests for the alias split, blob-type discrimination, and pending-clear (the Keystore crypto itself can't run under Robolectric, so the decision logic is what's pinned).

@thepastaclaw

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt (1)

806-873: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Defer pending-state updates until the Room write runs

clearPendingIdentityKey / recordPendingIdentityKey happen before stage(walletId) { ... }, so any rollback or transaction failure leaves pendingIdentityKeys out of sync with the row it describes. That can clear a failed key too early or leave a stale pending entry after a write that never commits. Move the map update into the staged write so it shares the row’s transactional lifetime.

🤖 Prompt for 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.

In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`
around lines 806 - 873, The pending-state mutations in the private-key
persistence flow around deriver.deriveAndStore, clearPendingIdentityKey, and
recordPendingIdentityKey currently occur before the staged Room write. Defer
both updates into the stage(walletId) write callback, applying the clear only
after a successful derive and the pending record for a failed derive within the
same staged operation, so rollback or transaction failure leaves
pendingIdentityKeys consistent with the persisted row.
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt (1)

430-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The KDoc for withManagedIdentity is orphaned.

The existing KDoc (lines 424–429) describes withManagedIdentity but is now separated from it by the new managedIdentityHandleOrZero function and its own KDoc. In Kotlin, only the KDoc immediately before a declaration is associated with it, so withManagedIdentity loses its documentation.

♻️ Proposed fix: move the orphaned KDoc to its correct position
     /**
-     * Open the managed-identity handle for [identityId], run [block],
-     * and destroy the handle before returning (the [contacts] /
-     * [acceptIncomingRequest] discipline). Returns null when the
-     * identity isn't managed by this wallet.
-     */
-    /**
      * Snapshot the managed identity for [identityId], or 0 when the wallet
      * does not manage it. The native side reports an unmanaged identity as
      * a platform-wallet NotFound error rather than a zero handle, so the
      * "not managed" outcome is translated here — every local-read caller
      * treats it as an absence (null / empty / false), never an exception.
      */
     private fun managedIdentityHandleOrZero(identityId: ByteArray): Long =
         translateManagedIdentityNotFoundToZero {
             TokensNative.getManagedIdentity(walletHandle, identityId)
         }
 
+    /**
+     * Open the managed-identity handle for [identityId], run [block],
+     * and destroy the handle before returning (the [contacts] /
+     * [acceptIncomingRequest] discipline). Returns null when the
+     * identity isn't managed by this wallet.
+     */
     private inline fun <T> withManagedIdentity(
         identityId: ByteArray,
         block: (Long) -> T,
     ): T? {
🤖 Prompt for 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.

In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt`
around lines 430 - 446, Move the existing KDoc describing withManagedIdentity so
it is immediately before the withManagedIdentity declaration, after
managedIdentityHandleOrZero and its KDoc. Keep the managedIdentityHandleOrZero
documentation attached to that function and preserve both documentation
contents.
🤖 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.

Outside diff comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- Around line 806-873: The pending-state mutations in the private-key
persistence flow around deriver.deriveAndStore, clearPendingIdentityKey, and
recordPendingIdentityKey currently occur before the staged Room write. Defer
both updates into the stage(walletId) write callback, applying the clear only
after a successful derive and the pending record for a failed derive within the
same staged operation, so rollback or transaction failure leaves
pendingIdentityKeys consistent with the persisted row.

---

Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt`:
- Around line 430-446: Move the existing KDoc describing withManagedIdentity so
it is immediately before the withManagedIdentity declaration, after
managedIdentityHandleOrZero and its KDoc. Keep the managedIdentityHandleOrZero
documentation attached to that function and preserve both documentation
contents.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c43cab53-9e7f-44a1-9a3b-ca57d7cb7970

📥 Commits

Reviewing files that changed from the base of the PR and between d1ec16b and d3aa786.

📒 Files selected for processing (12)
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.kt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants