feat(rs-sdk-ffi): expose dash_sdk_signer_can_sign for signer-delegated key preflight#3924
Conversation
…d key preflight Follow-up to #3922 (review thread r3423222271). `selectSigningKey` picked a document-signing key by ranking candidates by purpose/security and then checking private-material availability against `KeychainManager.shared` (via `KeyManager.withSharedKeychain().findSigningKey`). The six document ops accept a generic `signer: OpaquePointer`, so a signer NOT backed by the shared Keychain — a raw-key `KeyManager.createSigner` signer, a hardware-backed callback signer, or a `KeychainSigner` bound to a different `KeychainManager` — would have its key availability mis-judged. Latent (every current caller passes a shared-Keychain `KeychainSigner`), but it's the coupling #3922 flagged for removal. Remove it by delegating the availability decision to the supplied signer: - rs-sdk-ffi: new `dash_sdk_signer_can_sign(signer, pubkey_bytes, pubkey_len, key_type) -> bool`. Reconstructs a minimal IdentityPublicKey and calls the existing `VTableSigner::can_sign_with`, so it works for both inner variants (Callback forwards through the vtable's `can_sign_with` slot; Native delegates to the wrapped signer). `KeyType::try_from` fails closed on any out-of-range byte (incl. the 0xFF platform-address tag); null signer -> false. - swift-sdk: `selectSigningKey` now takes `signer: OpaquePointer?`, ranks candidates in Swift via the new public `KeyManager.rankSigningCandidates`, and returns the first key the SUPPLIED signer reports it can sign with. The shared-Keychain `findSigningKey` path is kept as the no-signer fallback. Key-selection policy stays in Swift; only the availability check crosses the FFI. No private-key material is pulled across the boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 13 minutes and 30 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
|
✅ Review complete (commit 1e884de) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents converged on no blocking issues. The new dash_sdk_signer_can_sign FFI export is sound (null-checks, fail-closed KeyType::try_from, no ownership transfer, no private material crossing the boundary), and the six Swift call-site rewires correctly delegate availability to the supplied signer. Two nitpicks remain: one about a documented-but-easy-to-misuse signer == nil fallback path in Swift, and one minor doc-contract loosening in the Rust FFI Safety comment.
💬 2 nitpick(s)
| @MainActor | ||
| private func selectSigningKey(from identity: DPPIdentity, operation: String) -> IdentityPublicKey? { | ||
| private func selectSigningKey( | ||
| from identity: DPPIdentity, | ||
| operation: String, | ||
| signer: OpaquePointer? | ||
| ) -> IdentityPublicKey? { | ||
| print("🔑 [\(operation)] Selecting public key for identity \(identity.id.toBase58String())") | ||
|
|
||
| // Contract create/update require a CRITICAL + AUTHENTICATION key; | ||
| // document ops require any AUTHENTICATION key (prefer CRITICAL). | ||
| let isContractOp = operation == "CONTRACT CREATE" || operation == "CONTRACT UPDATE" | ||
| let minimumSecurityLevel: SecurityLevel? = isContractOp ? .critical : nil | ||
|
|
||
| let km = KeyManager.withSharedKeychain() | ||
|
|
||
| // Contract create/update require a CRITICAL + AUTHENTICATION key. | ||
| if operation == "CONTRACT CREATE" || operation == "CONTRACT UPDATE" { | ||
| if let k = km.findSigningKey(for: identity, purpose: .authentication, minimumSecurityLevel: .critical, preferCritical: true) { | ||
| print("📝 [\(operation)] Selected CRITICAL AUTHENTICATION key #\(k.id)") | ||
| // No signer to ask → fall back to the shared-Keychain availability check. | ||
| guard let signer = signer else { | ||
| if let k = km.findSigningKey( | ||
| for: identity, | ||
| purpose: .authentication, | ||
| minimumSecurityLevel: minimumSecurityLevel, | ||
| preferCritical: true | ||
| ) { | ||
| if isContractOp { | ||
| print("📝 [\(operation)] Selected CRITICAL AUTHENTICATION key #\(k.id)") | ||
| } else { | ||
| print("📝 [\(operation)] Selected AUTHENTICATION key #\(k.id) (security \(k.securityLevel.name))") | ||
| } | ||
| return k | ||
| } | ||
| print("❌ [\(operation)] No CRITICAL AUTHENTICATION key with available private material") | ||
| if isContractOp { | ||
| print("❌ [\(operation)] No CRITICAL AUTHENTICATION key with available private material") | ||
| } else { | ||
| print("❌ [\(operation)] No AUTHENTICATION key with available private material") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Document state transitions require an AUTHENTICATION-purpose key. | ||
| // Prefer a CRITICAL auth key, but findSigningKey skips any candidate | ||
| // whose private key is not in the Keychain, so an identity whose only | ||
| // CRITICAL key is a TRANSFER key (or a CRITICAL auth key with no stored | ||
| // private material) correctly falls through to its HIGH auth key. | ||
| if let k = km.findSigningKey(for: identity, purpose: .authentication, minimumSecurityLevel: nil, preferCritical: true) { | ||
| print("📝 [\(operation)] Selected AUTHENTICATION key #\(k.id) (security \(k.securityLevel.name))") | ||
| // Rank candidates by purpose/security in Swift, then delegate the | ||
| // availability decision to the supplied signer. Prefer a CRITICAL auth | ||
| // key, but skip any candidate the signer can't actually sign with, so an | ||
| // identity whose only CRITICAL key is a TRANSFER key (or a CRITICAL auth | ||
| // key with no stored private material) correctly falls through to its | ||
| // HIGH auth key. | ||
| let candidates = km.rankSigningCandidates( | ||
| for: identity, | ||
| purpose: .authentication, | ||
| minimumSecurityLevel: minimumSecurityLevel, | ||
| preferCritical: true | ||
| ) | ||
| for k in candidates where signerCanSign(signer, k) { | ||
| if isContractOp { | ||
| print("📝 [\(operation)] Selected CRITICAL AUTHENTICATION key #\(k.id)") | ||
| } else { | ||
| print("📝 [\(operation)] Selected AUTHENTICATION key #\(k.id) (security \(k.securityLevel.name))") | ||
| } | ||
| return k | ||
| } | ||
| print("❌ [\(operation)] No AUTHENTICATION key with available private material") | ||
|
|
||
| if isContractOp { | ||
| print("❌ [\(operation)] No CRITICAL AUTHENTICATION key with available private material") | ||
| } else { | ||
| print("❌ [\(operation)] No AUTHENTICATION key with available private material") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: signer == nil fallback silently reintroduces the KeychainManager.shared coupling this PR removes
All six in-tree callers of selectSigningKey (the document operations updated in this PR) pass a non-nil signer: OpaquePointer, so the guard let signer = signer else { ... } branch at lines 70–90 is unreachable today. The lines 51–53 docstring already calls out that passing nil falls back to KeyManager.findSigningKey(...) against KeychainManager.withSharedKeychain() — exactly the coupling the rest of the PR removes. Two cleaner options: (a) tighten the parameter to a non-optional OpaquePointer and delete the fallback so any future caller can't accidentally opt back into shared-Keychain semantics, or (b) leave the fallback but rename it so the foot-gun is explicit at the call site. Not a correctness defect for this PR, just a maintenance hazard.
source: ['claude']
| /// # Safety | ||
| /// - `signer` must be a valid `*const SignerHandle` previously returned by | ||
| /// this SDK (e.g. `dash_sdk_signer_create*` / | ||
| /// `dash_sdk_signer_create_from_private_key`) and not yet destroyed. It may | ||
| /// be null, in which case this returns `false`. | ||
| /// - `pubkey_bytes` must point to at least `pubkey_len` readable bytes, OR be | ||
| /// null with `pubkey_len == 0`. The bytes are only read for the duration of | ||
| /// this call and are not retained. |
There was a problem hiding this comment.
💬 Nitpick: Safety contract under-specifies the (non-null pubkey_bytes, pubkey_len == 0) case
The Safety section says pubkey_bytes must "point to at least pubkey_len readable bytes, OR be null with pubkey_len == 0". The Swift caller (signerCanSign) routes through Data.withUnsafeBytes { ... bindMemory(to: UInt8.self).baseAddress }, which for an empty Data may yield either nil or a non-null sentinel depending on Swift runtime. The implementation correctly handles both because std::slice::from_raw_parts(non_null, 0) is well-defined for any non-null aligned pointer, but the doc as written excludes the non-null/zero-length case. Loosening to "pointer is ignored when pubkey_len == 0" would match actual behavior. Pure documentation polish — no behavior change.
source: ['claude']
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3924 +/- ##
============================================
+ Coverage 72.19% 72.78% +0.59%
============================================
Files 21 22 +1
Lines 2946 3054 +108
============================================
+ Hits 2127 2223 +96
- Misses 819 831 +12
🚀 New features to boost your workflow:
|
2 participants
Issue being fixed or feature implemented
Follow-up to #3922 (review thread #3922 (comment)).
selectSigningKey(from:operation:)inStateTransitionExtensions.swiftpicks the public key for a document state transition by ranking candidates by purpose/security, then checking private-material availability againstKeychainManager.shared(viaKeyManager.withSharedKeychain().findSigningKey(...)). The six document SDK ops (documentCreate / Replace / Delete / Transfer / UpdatePrice / Purchase) each accept a genericsigner: OpaquePointer, so a caller passing a signer not backed by the shared Keychain — a raw-keyKeyManager.createSigner(from:)signer, a hardware-backed callback signer, or aKeychainSignerbound to a differentKeychainManager— would have its key availability mis-judged, and the preflight could reject a key that signer can actually sign with.It's a latent design coupling, not an active bug: every current caller passes a
KeychainSignerdefaulting toKeychainManager.shared. But it's exactly the coupling #3922's own doc-comment flagged for removal.What was done?
Removed the coupling by delegating the availability decision to the supplied signer, while keeping key-selection policy (ranking by purpose/security) in Swift.
rs-sdk-ffi(signer.rs) — new export:It reconstructs a minimal
IdentityPublicKeyfrom(pubkey_bytes, key_type)and calls the existingVTableSigner::can_sign_with(previously only invoked internally by Rust during signing). This naturally covers both inner variants:Inner::Callbackforwards(pubkey_bytes, key_type)through the vtable'scan_sign_withslot (same wire encoding assign_async), andInner::Nativedelegates to the wrapped Rust signer.KeyType::try_from(key_type)fails closed on any out-of-range byte (including the0xFFplatform-address tag); a nullsignerreturnsfalse. No private-key material crosses the boundary.swift-sdk:selectSigningKeynow takessigner: OpaquePointer?. It ranks candidates via the new publicKeyManager.rankSigningCandidates(...)and returns the first key the supplied signer reports it can sign with through a thinsignerCanSign(_:_:)marshalling wrapper overdash_sdk_signer_can_sign.findSigningKeypath is retained as the fallback used only when no signer is available (signer == nil).signertoselectSigningKey.KeyManager.rankSigningCandidates(...)is a public, read-only ranking entry point delegating to the existing privaterankKeys(...)(no duplication; stays off the@MainActorKeychain path).The same purpose/security policy as before is preserved: contract create/update → AUTHENTICATION + minimum CRITICAL; document ops → AUTHENTICATION, prefer CRITICAL. Existing log markers are unchanged.
How Has This Been Tested?
cargo fmt --allclean;cargo check -p rs-sdk-ffi(incl.--all-targets) clean.signer.rs(can_sign_ffi_*): true/false vtable verdict round-trip, null signer → false, out-of-range key_type fails closed before the vtable is consulted.cargo test -p rs-sdk-ffi can_sign_ffi→ 4 passed; fullsignersuite → 39 passed.packages/swift-sdk/build_ios.shsucceeded; the newdash_sdk_signer_can_signsymbol is present in the generatedrs-sdk-ffi.h. SwiftExampleApp built toBUILD SUCCEEDED, confirming the Swift wrapper,rankSigningCandidates, and rewiredselectSigningKeycompile against the regenerated header.Breaking Changes
None. This adds a new FFI export and an additional
signer:parameter on aprivateSwift helper (all in-tree call sites updated). Not consensus-breaking.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code