feat(platform-wallet): secp256k1 primitives, identity-update parsing, and scoped signing keys for DashConnect - #4273
Conversation
… and scoped signing keys for DashConnect The iOS DashConnect port (`dash-key:` / `dash-st:` passwordless login, already shipping on Android) needs three capabilities the FFI does not expose, and one FFI validation rule that is stricter than consensus. - `secp256k1_primitives.rs`: verify a compressed point, derive a compressed public key, and compute the raw affine ECDH X — handle-free, over the already-linked `dashcore::secp256k1`. The existing DashPay ECDH takes a derivation path and returns a finished secret, so neither the ephemeral key nor the unhashed X is reachable through it. - `identity_update.rs`: parse-only entry points for a serialized `IdentityUpdateTransition`, so the wallet can verify an app-supplied transition adds exactly the login keys it derived instead of broadcasting foreign bytes blind. No signing, no broadcast. - `KeychainSigner.withAdditionalSigningKeys`: a scoped in-memory key registry the sign trampoline consults first, zeroed on scope exit — proof of possession for keys that are derived on demand and never persisted. - `decode_contract_bounds` no longer requires contract bounds for ENCRYPTION / DECRYPTION. Consensus accepts unbounded keys for every purpose (`validate_identity_public_key_contract_bounds/v1`), and real testnet `dash-st:` transitions carry such a key, so the old guard rejected a key Platform considers valid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds identity-update parsing across the Rust FFI and Swift SDK, standalone secp256k1 primitives, scoped additional signing keys, raw-key buffer signing, private-key zeroization, and support for unbounded contract bounds. ChangesIdentity update parsing
Secp256k1 primitives
Scoped signing keys
Private-key cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedPlatformWallet
participant IdentityUpdateFFI
participant IdentityUpdateTransition
ManagedPlatformWallet->>IdentityUpdateFFI: Parse transition bytes
IdentityUpdateFFI->>IdentityUpdateTransition: Deserialize tagged or tagless encoding
IdentityUpdateTransition-->>IdentityUpdateFFI: Return transition fields
IdentityUpdateFFI-->>ManagedPlatformWallet: Return owned FFI structures
ManagedPlatformWallet->>IdentityUpdateFFI: Free parsed structures
sequenceDiagram
participant FFITrampoline
participant KeychainSigner
participant ScopedSigningKeyRegistry
participant RawKeySigner
FFITrampoline->>KeychainSigner: Request signature
KeychainSigner->>ScopedSigningKeyRegistry: Find active scoped key
alt Scoped key exists
ScopedSigningKeyRegistry-->>KeychainSigner: Return private key
else No scoped key
KeychainSigner->>RawKeySigner: Sign with persisted or derived key
RawKeySigner-->>KeychainSigner: Return signature
end
KeychainSigner-->>FFITrampoline: Return signature
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
🕓 Ready for review — 13 ahead in queue (commit 473b54c) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
3173-3174: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInstall the cleanup
deferbeforetry result.check().Line 3173 can throw before line 3174 registers the free call. Today this leaks nothing, because
platform_wallet_parse_identity_update_transitionresets*outto the default value before it parses and returns early on failure. That makes the Swift side depend on a Rust-side implementation detail. Move thedeferabove the check so cleanup is unconditional.♻️ Proposed reordering
+ defer { platform_wallet_parse_identity_update_transition_free(&out) } try result.check() - defer { platform_wallet_parse_identity_update_transition_free(&out) }🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 3173 - 3174, In the parse identity update transition flow, move the defer that calls platform_wallet_parse_identity_update_transition_free(&out) to immediately before try result.check(), ensuring cleanup is registered before any throwing operation. Keep the existing cleanup call and result validation behavior unchanged.packages/rs-platform-wallet-ffi/src/identity_update.rs (2)
204-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
outmust not already own allocations.Line 213 overwrites
*outwith the default value. If a caller reuses anParsedIdentityUpdateFFIthat still holds pointers from a previous successful parse, those allocations leak. Add the precondition to the doc comment so binding authors free or re-initializeoutbefore each call.🤖 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/rs-platform-wallet-ffi/src/identity_update.rs` around lines 204 - 219, Add a doc comment for platform_wallet_parse_identity_update_transition stating that out must be newly initialized or have all allocations freed before each call; callers must not reuse an instance containing pointers from a previous successful parse because the function overwrites it with ParsedIdentityUpdateFFI::default().
117-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid embedding the full transition Debug in the error message.
{other:?}renders the entire decoded state transition, including signatures and payload bytes. The message is allocated as aCStringand returned to the host, so a large non-identity-update transition produces a very large error string. Print a short discriminant label instead.🤖 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/rs-platform-wallet-ffi/src/identity_update.rs` around lines 117 - 123, Update the non-matching branch of the state_transition match in the identity-update parsing flow to report only a short transition-type discriminant instead of formatting the full other value with Debug. Preserve the existing ErrorInvalidParameter result code and CString error propagation while avoiding signatures and payload bytes in the message.packages/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift (2)
43-65: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winClear the ECDH shared-secret buffer after use.
ecdhSharedXholds the raw ECDH shared-secret X coordinate in the localoutputarray before copying it intoData. The Rust FFI side scrubs equivalent secret material withWipingSecretKey,WipingScalar, andZeroizing. The Swift side does not clearoutputafter it copies the bytes into the returnedData.Overwrite
outputwith zeros before it goes out of scope.🔒 Proposed fix to zero the shared-secret buffer
try result.check() - return Data(output) + let sharedSecret = Data(output) + for index in output.indices { + output[index] = 0 + } + return sharedSecret }🤖 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/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift` around lines 43 - 65, Update ecdhSharedX to overwrite the local output buffer with zeros after constructing the returned Data and before the function exits. Preserve the existing FFI call, result validation, and returned shared X coordinate while ensuring the temporary secret material in output is cleared on the successful path.
1-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Swift-side tests for
isValidCompressedPointandecdhSharedX.Only
compressedPublicKeyhas indirect coverage, throughKeychainSignerAdditionalSigningKeysTests.swift.isValidCompressedPointandecdhSharedXhave no visible Swift tests.Add tests using the same known vectors as
secp256k1_primitives.rs(PRIVATE_KEY_A/PUBLIC_KEY_B/SHARED_X_AB) to catch FFI marshaling regressions across the Swift/Rust boundary.🤖 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/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift` around lines 1 - 66, Add Swift tests for Secp256k1Primitives.isValidCompressedPoint and Secp256k1Primitives.ecdhSharedX, reusing the PRIVATE_KEY_A, PUBLIC_KEY_B, and SHARED_X_AB vectors from secp256k1_primitives.rs. Verify valid and invalid compressed-point inputs, and assert ecdhSharedX returns the expected shared X coordinate while preserving the existing throwing behavior for invalid inputs.packages/rs-platform-wallet-ffi/src/secp256k1_primitives.rs (1)
96-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse a shared secp256k1 context instead of constructing one per call.
platform_wallet_secp256k1_compressed_public_key(Line 105) andplatform_wallet_secp256k1_ecdh_shared_x(Line 126) each build a newSecp256k1::new()context on every invocation. Thesecp256k1crate documents a global static context specifically to avoid repeated context construction on hot paths.Use
secp256k1::SECP256K1(with theglobal-contextfeature) or a lazily-initialized static (e.g.,once_cell::sync::Lazy) shared by both functions.⚡ Proposed fix to share a static context
+use std::sync::OnceLock; + +fn secp_context() -> &'static Secp256k1<dashcore::secp256k1::All> { + static CONTEXT: OnceLock<Secp256k1<dashcore::secp256k1::All>> = OnceLock::new(); + CONTEXT.get_or_init(Secp256k1::new) +} + pub unsafe extern "C" fn platform_wallet_secp256k1_compressed_public_key( seckey: *const u8, seckey_len: usize, out_pubkey: *mut u8, ) -> PlatformWalletFFIResult { check_ptr!(seckey); check_ptr!(out_pubkey); let secret_key = unwrap_result_or_return!(parse_secret_key(seckey, seckey_len)); - let compressed = PublicKey::from_secret_key(&Secp256k1::new(), &secret_key.0).serialize(); + let compressed = PublicKey::from_secret_key(secp_context(), &secret_key.0).serialize(); std::ptr::copy_nonoverlapping(compressed.as_ptr(), out_pubkey, compressed.len()); PlatformWalletFFIResult::ok() }Since this depends on the exact
dashcore-forkedsecp256k1API surface (feature flags for the global context, exact generic parameters), please confirm the recommended pattern against the version pinned in this crate'sCargo.toml.Also applies to: 112-138
🤖 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/rs-platform-wallet-ffi/src/secp256k1_primitives.rs` around lines 96 - 108, Replace the per-call Secp256k1::new() construction in platform_wallet_secp256k1_compressed_public_key and platform_wallet_secp256k1_ecdh_shared_x with one shared context appropriate for the pinned dashcore secp256k1 dependency. Verify the crate’s Cargo.toml features and API, enabling or using secp256k1::SECP256K1 when supported, otherwise introduce a compatible lazily initialized static and reuse it in both functions.packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift (1)
87-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
KeyManagerErrormapping.
AdditionalSigningKeyEntry.sign(Lines 109-117) repeats the exact error-mapping switch used byffiSign(Lines 906-914). Extract one shared function so a future change to the mapping does not need to be applied twice.♻️ Proposed refactor
+ static func performSign( + privateKey: Data, + data: Data, + network: Network + ) -> Result<Data, KeychainSigner.Error> { + do { + return .success(try RawKeySigner.sign(data: data, privateKey: privateKey, network: network)) + } catch KeyManagerError.signerCreationFailed(let message) { + return .failure(.ffiSignerCreationFailed(message: message)) + } catch KeyManagerError.invalidKeyFormat(let message) { + return .failure(.ffiSignerCreationFailed(message: "invalid key format: \(message)")) + } catch KeyManagerError.signingFailed(let message) { + return .failure(.ffiSignFailed(message: message)) + } catch { + return .failure(.ffiSignFailed(message: String(describing: error))) + } + } + final class AdditionalSigningKeyEntry: `@unchecked` Sendable { let publicKey: Data private var privateKeyBytes: [UInt8] @@ func sign(data: Data, network: Network) -> Result<Data, KeychainSigner.Error> { - do { - return .success( - try RawKeySigner.sign( - data: data, - privateKey: Data(privateKeyBytes), - network: network - ) - ) - } catch KeyManagerError.signerCreationFailed(let message) { - return .failure(.ffiSignerCreationFailed(message: message)) - } catch KeyManagerError.invalidKeyFormat(let message) { - return .failure(.ffiSignerCreationFailed(message: "invalid key format: \(message)")) - } catch KeyManagerError.signingFailed(let message) { - return .failure(.ffiSignFailed(message: message)) - } catch { - return .failure(.ffiSignFailed(message: String(describing: error))) - } + KeychainSigner.performSign(privateKey: Data(privateKeyBytes), data: data, network: network) }And in
ffiSign(Lines 899-915):fileprivate func ffiSign( privateKey: Data, data: Data ) -> Result<Data, Error> { KeychainSigner.performSign(privateKey: privateKey, data: data, network: self.network) }🤖 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/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift` around lines 87 - 132, Extract the shared KeyManagerError-to-KeychainSigner.Error mapping from AdditionalSigningKeyEntry.sign and ffiSign into a single helper, then have both signing paths delegate to it. Preserve the existing success behavior and all current error cases/messages while removing the duplicated catch logic.
🤖 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/rs-platform-wallet-ffi/src/identity_update.rs`:
- Around line 129-135: Update encode_contract_bounds to return Result<(u8, [u8;
32], *mut c_char), PlatformWalletFFIResult>; when CString::new fails for
SingleContractDocumentType, return an error instead of kind 1 or a null pointer.
Propagate this Result through project_parsed_identity_update and
platform_wallet_parse_identity_update_transition, preserving the
contract-plus-document-type bound on successful encoding.
- Around line 92-115: Update the framing selection in the identity-update
deserialization flow to try both interpretations when the first byte equals
IDENTITY_UPDATE_VARIANT_TAG: attempt the tagged bytes, then retry with the
prefixed tagless framing if that fails, preserving combined error reporting when
both fail. Remove deserialize_state_transition if it becomes unused, and keep
the existing fallback behavior for non-tagged-leading payloads symmetric.
In `@packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift`:
- Around line 295-376: Update the scoped-key signing flow so lookup and signing
occur within one queue critical section, preventing popAdditionalSigningKeys and
clearAdditionalSigningKeys from zeroing the key during use. Add or update
signWithScopedKeyIfPresent to perform the additionalSigningKey lookup and
entry.sign(data:network:) inside queue.sync, then have signOnDemand use that
helper while preserving the existing fallback behavior. Add a concurrency
regression test if the surrounding test structure supports it.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/identity_update.rs`:
- Around line 204-219: Add a doc comment for
platform_wallet_parse_identity_update_transition stating that out must be newly
initialized or have all allocations freed before each call; callers must not
reuse an instance containing pointers from a previous successful parse because
the function overwrites it with ParsedIdentityUpdateFFI::default().
- Around line 117-123: Update the non-matching branch of the state_transition
match in the identity-update parsing flow to report only a short transition-type
discriminant instead of formatting the full other value with Debug. Preserve the
existing ErrorInvalidParameter result code and CString error propagation while
avoiding signatures and payload bytes in the message.
In `@packages/rs-platform-wallet-ffi/src/secp256k1_primitives.rs`:
- Around line 96-108: Replace the per-call Secp256k1::new() construction in
platform_wallet_secp256k1_compressed_public_key and
platform_wallet_secp256k1_ecdh_shared_x with one shared context appropriate for
the pinned dashcore secp256k1 dependency. Verify the crate’s Cargo.toml features
and API, enabling or using secp256k1::SECP256K1 when supported, otherwise
introduce a compatible lazily initialized static and reuse it in both functions.
In `@packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift`:
- Around line 87-132: Extract the shared KeyManagerError-to-KeychainSigner.Error
mapping from AdditionalSigningKeyEntry.sign and ffiSign into a single helper,
then have both signing paths delegate to it. Preserve the existing success
behavior and all current error cases/messages while removing the duplicated
catch logic.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 3173-3174: In the parse identity update transition flow, move the
defer that calls platform_wallet_parse_identity_update_transition_free(&out) to
immediately before try result.check(), ensuring cleanup is registered before any
throwing operation. Keep the existing cleanup call and result validation
behavior unchanged.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift`:
- Around line 43-65: Update ecdhSharedX to overwrite the local output buffer
with zeros after constructing the returned Data and before the function exits.
Preserve the existing FFI call, result validation, and returned shared X
coordinate while ensuring the temporary secret material in output is cleared on
the successful path.
- Around line 1-66: Add Swift tests for
Secp256k1Primitives.isValidCompressedPoint and Secp256k1Primitives.ecdhSharedX,
reusing the PRIVATE_KEY_A, PUBLIC_KEY_B, and SHARED_X_AB vectors from
secp256k1_primitives.rs. Verify valid and invalid compressed-point inputs, and
assert ecdhSharedX returns the expected shared X coordinate while preserving the
existing throwing behavior for invalid inputs.
🪄 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 Plus
Run ID: db3e23f6-fe58-40de-9db6-051676007b11
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_update.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/secp256k1_primitives.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/KeychainSignerAdditionalSigningKeysTests.swift
…e key race
Two defects in the scoped signing-key registry, one found by CI and one by
review.
`swift test` failed to compile: the package builds in Swift 6 language mode
with `-warnings-as-errors`, where a `@MainActor` caller cannot hand a
non-Sendable closure to a nonisolated `async` function ("sending value of
non-Sendable type '() async -> ()' risks causing data races"). The app did not
catch this because it still builds in Swift 5 mode. Both `withAdditionalSigningKeys`
overloads now take `isolation: isolated (any Actor)? = #isolation`, so `body`
runs in the caller's context and never crosses an isolation boundary; the
parameter is defaulted, so existing call sites are unchanged.
`signOnDemand` also looked the entry up under `queue` but signed with it after
the lock was released, while `popAdditionalSigningKeys` /
`clearAdditionalSigningKeys` zero the same buffer from inside `queue`. A scope
ending on another thread could therefore zero a key mid-signature and yield a
garbage signature instead of a clean failure. `signWithScopedKey` now performs
the lookup and the signature in one critical section.
Also drops the redundant `try` from the two non-throwing test scopes, which
`-warnings-as-errors` promoted to failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t bounds Both from review of the parse entry point, which reads attacker-supplied bytes from a DashConnect `dash-st:` QR. The framing choice was one-directional: a payload whose first byte is the variant tag went straight to the tagged path with no retry, while everything else got both framings. A tagless body whose first byte is 6 by coincidence therefore failed outright. Both orders now fall back to the other framing, and the combined error names which framing produced which failure. `encode_contract_bounds` downgraded `SingleContractDocumentType` to `SingleContract` when the document type name could not become a C string. That reports a broader scope than the transition declares — in the DashConnect approval flow the user would be shown contract-wide access for a key bounded to one document type. It now returns an error that propagates through `project_parsed_identity_update`, which releases the keys it had already projected on the way out; the shared `free_parsed_public_keys` helper is used by both that path and the public free entry point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SwiftExampleApp target compiles the SDK sources without region-based isolation, so returning the generic result out of `body` was rejected there even though SwiftPM's Swift 6 build accepted it: "non-sendable result type 'T' cannot be sent from nonisolated context in call to parameter 'body'". Constrain `T` to `Sendable` on both overloads, as the compiler suggests. Every call site returns Void, so nothing is lost. Verified both ways this time: `swift build --build-tests` (SwiftPM, Swift 6 mode, warnings-as-errors) and `xcodebuild build -scheme SwiftExampleApp`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The identity-update parser, FFI ownership paths, secp256k1 vectors, and contract-bounds widening are otherwise consistent with the stated DashConnect behavior, but the raw ECDH implementation uses a variable-time multiplication API with a secret scalar and must be replaced before merge. The scoped signer also leaves an avoidable private-key copy unwiped, the claimed captured Yappr fixture is not present, and the Swift contract-bounds documentation contradicts the new consensus-compatible behavior.
Source: reviewers codex/general=gpt-5.6-sol(completed), codex/security-auditor=gpt-5.6-sol(completed), codex/ffi-engineer=gpt-5.6-sol(completed); final verifier codex/verifier=gpt-5.6-sol(completed); sonnet=not run (deferred by blocker gate); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol (orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 3 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/rs-platform-wallet-ffi/src/secp256k1_primitives.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/secp256k1_primitives.rs:132-136: ECDH uses variable-time public-tweak multiplication with a secret scalar
`PublicKey::mul_tweak` calls libsecp256k1's `secp256k1_ec_pubkey_tweak_mul`. That path reaches `eckey_pubkey_tweak_mul` and the ordinary WNAF-based `ecmult`, whose branches and table selection depend on the scalar; it is intended for public tweaks. Here the tweak is the private ECDH scalar and the peer controls the input point, so the new DashConnect primitive exposes secret-dependent timing/cache behavior. Use `dashcore::secp256k1::ecdh::shared_secret_point(&public_key, &secret_key.0)`, which invokes `secp256k1_ecdh` and `ecmult_const`; zeroize the returned 64-byte `x || y` buffer after copying its first 32 bytes to `out_shared_x`.
In `packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift:103-107: Scoped private keys leave an unzeroized Data copy after signing
`Data(privateKeyBytes)` creates a separate immutable buffer containing the scoped private scalar. `RawKeySigner.sign` zeroes only the later mutable `[UInt8]` copy it creates, while `popAdditionalSigningKeys` zeroes only the entry's `privateKeyBytes`; neither can scrub this temporary `Data` before Foundation releases it. This avoidable internal copy can therefore remain in allocator memory after the scope exits, weakening the cleanup guarantee for login keys that are intentionally never persisted. Add a signing overload that borrows the entry's mutable byte buffer directly, or explicitly place every temporary key-bearing buffer in wipe-on-drop storage.
In `packages/rs-platform-wallet-ffi/src/identity_update.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_update.rs:471-473: The Yappr framing test does not exercise a captured Yappr payload
The PR states that this test parses a tagless transition captured from the live Yappr testnet app, but the test serializes the local Rust fixture and removes byte zero. That validates the parser against its own framing assumption and cannot detect an interoperability mismatch with Yappr's actual encoder. Check in the captured tagless payload as fixed bytes and assert the projected identity, key fields, unbounded ENCRYPTION key, contract bounds, and disabled IDs.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:176-181: Swift still documents encryption bounds as mandatory
This public API still says Encryption and Decryption keys require contract bounds and that `nil` is only valid for other purposes. The Rust decoder was intentionally widened because consensus accepts unbounded keys for every purpose, including the real DashConnect ENCRYPTION key described by this PR. The contradictory documentation can cause SDK consumers to reject or rewrite a valid transition before calling the now-correct FFI.
| let shared_point = unwrap_result_or_return!(public_key | ||
| .mul_tweak(&secp, &scalar.0) | ||
| .map_err(|_| wallet_operation("ECDH produced the point at infinity"))); | ||
| let uncompressed = shared_point.serialize_uncompressed(); | ||
| std::ptr::copy_nonoverlapping(uncompressed[1..33].as_ptr(), out_shared_x, 32); |
There was a problem hiding this comment.
🔴 Blocking: ECDH uses variable-time public-tweak multiplication with a secret scalar
PublicKey::mul_tweak calls libsecp256k1's secp256k1_ec_pubkey_tweak_mul. That path reaches eckey_pubkey_tweak_mul and the ordinary WNAF-based ecmult, whose branches and table selection depend on the scalar; it is intended for public tweaks. Here the tweak is the private ECDH scalar and the peer controls the input point, so the new DashConnect primitive exposes secret-dependent timing/cache behavior. Use dashcore::secp256k1::ecdh::shared_secret_point(&public_key, &secret_key.0), which invokes secp256k1_ecdh and ecmult_const; zeroize the returned 64-byte x || y buffer after copying its first 32 bytes to out_shared_x.
source: ['codex']
There was a problem hiding this comment.
Addressed in 7d998b8.
platform_wallet_secp256k1_ecdh_shared_x now goes through dashcore::secp256k1::ecdh::shared_secret_point(&public_key, &secret_key.0) — i.e. secp256k1_ecdh / ecmult_const — and holds the returned 64-byte x || y in Zeroizing, copying only the first 32 bytes into out_shared_x.
Agreed on the substance: the tweak here is the wallet's own ephemeral scalar and the point arrives from the peer, so mul_tweak's WNAF ecmult gave the party choosing the input a secret-dependent timing signal. mul_tweak, the Scalar / WipingScalar conversion it required, and the now-unreachable point-at-infinity error path are all gone.
ecdh_shared_x_matches_known_vector is deliberately unchanged and still passes — that is the equivalence proof for the swap.
There was a problem hiding this comment.
Resolved in 7d998b8 — ECDH uses variable-time public-tweak multiplication with a secret scalar 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.
| try RawKeySigner.sign( | ||
| data: data, | ||
| privateKey: Data(privateKeyBytes), | ||
| network: network | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Scoped private keys leave an unzeroized Data copy after signing
Data(privateKeyBytes) creates a separate immutable buffer containing the scoped private scalar. RawKeySigner.sign zeroes only the later mutable [UInt8] copy it creates, while popAdditionalSigningKeys zeroes only the entry's privateKeyBytes; neither can scrub this temporary Data before Foundation releases it. This avoidable internal copy can therefore remain in allocator memory after the scope exits, weakening the cleanup guarantee for login keys that are intentionally never persisted. Add a signing overload that borrows the entry's mutable byte buffer directly, or explicitly place every temporary key-bearing buffer in wipe-on-drop storage.
source: ['codex']
There was a problem hiding this comment.
Addressed in 7d998b8.
RawKeySigner gained a sign(data:privateKeyBuffer:network:) overload taking an UnsafeBufferPointer<UInt8>, and AdditionalSigningKeyEntry.sign calls it inside privateKeyBytes.withUnsafeBufferPointer, so no Data copy of the scalar is created on that path at all.
The Data-taking overload is kept as a thin wrapper over the same body, so no existing call site changed. The defensive keyCopy + memset_s inside stays as well: the FFI signer still never reads directly from caller-owned storage, and that copy is the one that was already being wiped.
There was a problem hiding this comment.
Resolved in 7d998b8 — Scoped private keys leave an unzeroized Data copy after signing 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.
| fn parses_yappr_tagless_identity_update_transition() { | ||
| let tagged = fixture_transition_bytes(); | ||
| let tagless = tagged[1..].to_vec(); |
There was a problem hiding this comment.
🟡 Suggestion: The Yappr framing test does not exercise a captured Yappr payload
The PR states that this test parses a tagless transition captured from the live Yappr testnet app, but the test serializes the local Rust fixture and removes byte zero. That validates the parser against its own framing assumption and cannot detect an interoperability mismatch with Yappr's actual encoder. Check in the captured tagless payload as fixed bytes and assert the projected identity, key fields, unbounded ENCRYPTION key, contract bounds, and disabled IDs.
source: ['codex']
There was a problem hiding this comment.
Addressed in 7d998b8.
parses_yappr_tagless_identity_update_transition now decodes the payload captured from the live testnet app (YAPPR_TAGLESS_IDENTITY_UPDATE_FIXTURE_HEX — the same bytes the iOS side asserts in PlatformDashConnectDataSourceTests) and checks identity id 89fd6ddb…, no disabled ids, and both keys' id / purpose / security level / key type / data: key 6 AUTHENTICATION · HIGH · ECDSA_HASH160, key 7 ENCRYPTION · MEDIUM · ECDSA_SECP256K1, with contract_bounds_kind == 0 on both. That unbounded ENCRYPTION key is exactly the regression guard for the decode_contract_bounds widening in this PR, and there is a comment saying so. Enum variants are compared via try_from, not raw discriminants.
The self-serialized case is kept — it still proves the tagged and prefixed paths agree — but renamed to parses_tagless_framing_by_prepending_the_variant_tag, which is what it actually demonstrates. The inaccurate part was the PR description; the test now lives up to it.
There was a problem hiding this comment.
Resolved in 7d998b8 — The Yappr framing test does not exercise a captured Yappr payload 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.
Round-2 review of #4273. The first is a real weakness, not a style point. **ECDH ran in variable time over the secret scalar.** `PublicKey::mul_tweak` is libsecp256k1's `secp256k1_ec_pubkey_tweak_mul`, which reaches the WNAF `ecmult` whose table selection and branching depend on the scalar — correct for a *public* tweak, wrong here: the tweak is the wallet's private ephemeral key and the peer supplies the point, so the primitive leaked secret-dependent timing to the party choosing its input. Now goes through `ecdh::shared_secret_point` (`secp256k1_ecdh` / `ecmult_const`), with the 64-byte `x || y` result held in `Zeroizing`. `ecdh_shared_x_matches_known_vector` is unchanged and still passes, which is the proof the swap is equivalent. **The scoped signing key left an unzeroed copy.** `Data(privateKeyBytes)` built a buffer nothing could scrub: `RawKeySigner.sign` zeroes only its own `keyCopy`, and `popAdditionalSigningKeys` only the entry's storage. Added a `privateKeyBuffer:` overload that borrows the entry's bytes; the `Data` overload is now a wrapper, so no call site changes. **The "Yappr tagless" test used a self-serialized fixture** — the local fixture with byte 0 removed, which only proved the parser agreed with itself about framing. It now parses the payload captured from the live testnet app (the same bytes the iOS tests assert) and checks both keys' ids, purposes, levels, types, data, and that neither carries contract bounds — the real unbounded ENCRYPTION key that this PR's decoder widening exists for. The synthetic round-trip survives under an honest name. **Swift still documented encryption contract bounds as mandatory,** which contradicts the widened decoder and could lead a consumer to reject a valid transition before ever reaching the FFI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All four findings from the preliminary review are addressed in 7d998b8; the three inline threads have per-finding replies. The fourth was reported outside the diff, so it is answered here: Swift still documents encryption bounds as mandatory — fixed. Verification
CI on this commit is green, Swift SDK build and Rust wallet tests included. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
All four carried-forward prior findings are fixed at the current head, so none remain live. The final combined review confirms one new latest-delta test-coverage finding (the captured Yappr fixture omits read_only assertions) and two cumulative full-PR security suggestions concerning avoidable secret-bearing allocations; no blocking issue remains.
Source: reviewers codex/general=gpt-5.6-sol(completed), codex/security-auditor=gpt-5.6-sol(completed), codex/ffi-engineer=gpt-5.6-sol(completed), claude/general=claude-sonnet-5(completed), claude/security-auditor=claude-sonnet-5(completed), claude/ffi-engineer=claude-sonnet-5(completed); final verifier codex/final-verifier=gpt-5.6-sol(completed, fallback_for_sonnet_verifier=true); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol (orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed)
🟡 3 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift:44-65: ECDH wrapper leaves an extra unwiped shared-secret copy
`platform_wallet_secp256k1_ecdh_shared_x` writes the raw ECDH X coordinate into the mutable `[UInt8]` buffer, and `Data(output)` then copies that session secret into a second allocation for the return value. The original array is released without being scrubbed, leaving an avoidable secret-bearing allocation in allocator memory. Allocate the returned `Data` directly and let Rust write into that buffer so only the intentionally returned copy remains.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swift:81-98: Scoped signing still leaves Rust-side private-key copies unwiped
The new scoped-key path correctly borrows the registry array and wipes Swift’s `keyCopy`, but `dash_sdk_signer_create_from_private_key` then constructs a Rust `SingleKeySigner`. `SingleKeySigner::new_from_slice` copies the scalar into a plain stack `[u8; 32]` and into a `dashcore::PrivateKey`; destroying the handle only drops the `Arc`, while `SingleKeySigner` has no `Drop` implementation and the copyable `SecretKey` is not automatically erased. Signing also calls `secret_bytes()`, creating another ordinary 32-byte array before `dashcore::sign_hash` parses another non-erasing `SecretKey`. Because these DashConnect login keys are deliberately non-persisted and promised to be cleared at scope exit, use zeroizing construction/signing storage and erase the signer-owned key on drop, or replace the temporary-handle path with a one-shot Rust export whose secret values are guarded end-to-end.
In `packages/rs-platform-wallet-ffi/src/identity_update.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_update.rs:503-549: Captured Yappr fixture does not assert the read-only field
The captured fixture verifies each key’s ID, purpose, security level, type, data, and bounds, but never checks `read_only`. That field is projected at `project_parsed_identity_update` and then mapped into Swift’s `IdentityPubkey`; hardcoding or dropping it would leave this interoperability test green. The captured keys both decode as `read_only == false`; assert those values, and extend the synthetic tagged-fixture assertions to verify that its second key remains `read_only == true`.
| public static func ecdhSharedX(privateKey: Data, publicKey: Data) throws -> Data { | ||
| var output = [UInt8](repeating: 0, count: 32) | ||
|
|
||
| let result = privateKey.withUnsafeBytes { privateBuffer -> PlatformWalletFFIResult in | ||
| let privateBytes = privateBuffer.bindMemory(to: UInt8.self) | ||
| return publicKey.withUnsafeBytes { publicBuffer in | ||
| let publicBytes = publicBuffer.bindMemory(to: UInt8.self) | ||
| return output.withUnsafeMutableBufferPointer { outputBuffer in | ||
| platform_wallet_secp256k1_ecdh_shared_x( | ||
| privateBytes.baseAddress, | ||
| UInt(privateBytes.count), | ||
| publicBytes.baseAddress, | ||
| UInt(publicBytes.count), | ||
| outputBuffer.baseAddress | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| try result.check() | ||
| return Data(output) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: ECDH wrapper leaves an extra unwiped shared-secret copy
platform_wallet_secp256k1_ecdh_shared_x writes the raw ECDH X coordinate into the mutable [UInt8] buffer, and Data(output) then copies that session secret into a second allocation for the return value. The original array is released without being scrubbed, leaving an avoidable secret-bearing allocation in allocator memory. Allocate the returned Data directly and let Rust write into that buffer so only the intentionally returned copy remains.
| public static func ecdhSharedX(privateKey: Data, publicKey: Data) throws -> Data { | |
| var output = [UInt8](repeating: 0, count: 32) | |
| let result = privateKey.withUnsafeBytes { privateBuffer -> PlatformWalletFFIResult in | |
| let privateBytes = privateBuffer.bindMemory(to: UInt8.self) | |
| return publicKey.withUnsafeBytes { publicBuffer in | |
| let publicBytes = publicBuffer.bindMemory(to: UInt8.self) | |
| return output.withUnsafeMutableBufferPointer { outputBuffer in | |
| platform_wallet_secp256k1_ecdh_shared_x( | |
| privateBytes.baseAddress, | |
| UInt(privateBytes.count), | |
| publicBytes.baseAddress, | |
| UInt(publicBytes.count), | |
| outputBuffer.baseAddress | |
| ) | |
| } | |
| } | |
| } | |
| try result.check() | |
| return Data(output) | |
| } | |
| public static func ecdhSharedX(privateKey: Data, publicKey: Data) throws -> Data { | |
| var output = Data(count: 32) | |
| let result = output.withUnsafeMutableBytes { outputBuffer -> PlatformWalletFFIResult in | |
| let outputBytes = outputBuffer.bindMemory(to: UInt8.self) | |
| return privateKey.withUnsafeBytes { privateBuffer -> PlatformWalletFFIResult in | |
| let privateBytes = privateBuffer.bindMemory(to: UInt8.self) | |
| return publicKey.withUnsafeBytes { publicBuffer in | |
| let publicBytes = publicBuffer.bindMemory(to: UInt8.self) | |
| return platform_wallet_secp256k1_ecdh_shared_x( | |
| privateBytes.baseAddress, | |
| UInt(privateBytes.count), | |
| publicBytes.baseAddress, | |
| UInt(publicBytes.count), | |
| outputBytes.baseAddress | |
| ) | |
| } | |
| } | |
| } | |
| try result.check() | |
| return output | |
| } |
source: ['codex']
| let keys = unsafe { slice::from_raw_parts(out.add_public_keys, out.add_public_keys_count) }; | ||
|
|
||
| assert_eq!(keys[0].key_id, 6); | ||
| assert_eq!( | ||
| Purpose::try_from(keys[0].purpose).expect("recognized purpose"), | ||
| Purpose::AUTHENTICATION | ||
| ); | ||
| assert_eq!( | ||
| SecurityLevel::try_from(keys[0].security_level).expect("recognized security level"), | ||
| SecurityLevel::HIGH | ||
| ); | ||
| assert_eq!( | ||
| KeyType::try_from(keys[0].key_type).expect("recognized key type"), | ||
| KeyType::ECDSA_HASH160 | ||
| ); | ||
| assert_eq!(keys[0].contract_bounds_kind, 0); | ||
| let key0_data = unsafe { slice::from_raw_parts(keys[0].data_ptr, keys[0].data_len) }; | ||
| assert_eq!( | ||
| key0_data, | ||
| hex::decode("5e24e38a86e720f61757647996957e322686abb7") | ||
| .expect("valid authentication key hex") | ||
| .as_slice() | ||
| ); | ||
|
|
||
| assert_eq!(keys[1].key_id, 7); | ||
| assert_eq!( | ||
| Purpose::try_from(keys[1].purpose).expect("recognized purpose"), | ||
| Purpose::ENCRYPTION | ||
| ); | ||
| assert_eq!( | ||
| SecurityLevel::try_from(keys[1].security_level).expect("recognized security level"), | ||
| SecurityLevel::MEDIUM | ||
| ); | ||
| assert_eq!( | ||
| KeyType::try_from(keys[1].key_type).expect("recognized key type"), | ||
| KeyType::ECDSA_SECP256K1 | ||
| ); | ||
| // The real DashConnect ENCRYPTION key is intentionally unbounded, so | ||
| // this guards the decoder widening that now accepts absent bounds. | ||
| assert_eq!(keys[1].contract_bounds_kind, 0); | ||
| let key1_data = unsafe { slice::from_raw_parts(keys[1].data_ptr, keys[1].data_len) }; | ||
| assert_eq!( | ||
| key1_data, | ||
| hex::decode("035e8cfb0785b54e8902a3dc17bdaad8a5738c6019a18ebc527f79d1c64a27826a") | ||
| .expect("valid encryption key hex") | ||
| .as_slice() | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Captured Yappr fixture does not assert the read-only field
The captured fixture verifies each key’s ID, purpose, security level, type, data, and bounds, but never checks read_only. That field is projected at project_parsed_identity_update and then mapped into Swift’s IdentityPubkey; hardcoding or dropping it would leave this interoperability test green. The captured keys both decode as read_only == false; assert those values, and extend the synthetic tagged-fixture assertions to verify that its second key remains read_only == true.
source: ['codex']
…only Round-3 review of #4273 — three suggestions, no blockers. **The Swift ECDH wrapper copied the shared secret twice.** Rust wrote the raw X coordinate into a scratch `[UInt8]`, then `Data(output)` copied it into the return value and the array was released unscrubbed. `ecdhSharedX` now allocates the returned `Data` up front and lets the FFI write straight into it, so only the copy the caller asked for exists. `compressedPublicKey` is left alone — its output is a public key. **`SingleKeySigner` kept unwiped copies of the scalar.** The FFI entry point already handed it bytes from a `Zeroizing` buffer, but the signer did not maintain that: `new_from_slice` copied into a plain `[u8; 32]`, the struct had no `Drop` (and `secp256k1::SecretKey` is `Copy`, so it does not erase itself), and both `sign` and `verify_key_matches` materialised another bare array per call via `secret_bytes()`. All three now go through `Zeroizing`, and the key is erased on drop — which matters here because DashConnect builds this signer per call for login keys that are deliberately never persisted. `simple-signer` is a shared crate, so its public API is untouched; `rs-sdk-ffi` and `rs-scripts` still compile against it (`cargo check` on both). **Neither identity-update fixture asserted `read_only`.** The field is projected and mapped into Swift's `IdentityPubkey`, so dropping or hardcoding it would have left the suite green. The captured Yappr keys are both asserted as `false`; the synthetic fixture asserts `false` on key 17 and `true` on key 18, proving the field is carried rather than defaulted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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/simple-signer/src/single_key_signer.rs`:
- Around line 151-156: The derived SecretKey temporaries in both ECDSA_SECP256K1
and ECDSA_HASH160 conversion sites remain reachable after public-key comparison.
Make each secret_key mutable, perform the existing public_key_bytes comparison,
then call non_secure_erase() on the SecretKey before returning, preserving the
current validation result and handling both conversion paths.
🪄 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 Plus
Run ID: 60452afa-a8cb-4d70-8f29-cec0d4fdfc88
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/identity_update.rspackages/simple-signer/Cargo.tomlpackages/simple-signer/src/single_key_signer.rspackages/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/Sources/SwiftDashSDK/Security/Secp256k1Primitives.swift
- packages/rs-platform-wallet-ffi/src/identity_update.rs
| let secret_bytes = Zeroizing::new(self.private_key.inner.secret_bytes()); | ||
| let secret_key = | ||
| match dashcore::secp256k1::SecretKey::from_byte_array(&*secret_bytes) { | ||
| Ok(sk) => sk, | ||
| Err(_) => return false, | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file context =="
sed -n '130,185p' packages/simple-signer/src/single_key_signer.rs || true
echo "== dependencies mentioning dashcore/secp256k1 =="
for f in Cargo.toml packages/*/Cargo.toml packages/*/*/Cargo.toml; do
[ -f "$f" ] && sed -n '1,220p' "$f"
done | rg -n "dashcore|secp256k1|bitcoin" -C 2 || true
echo "== find lockfiles and inspect dashcore/secp256k1 entries =="
git ls-files | rg '(^|/)Cargo\.lock$|Cargo\.toml$' | sort
if [ -f Cargo.lock ]; then
rg -n 'name = "secp256k1"|name = "dashcore"' Cargo.lock -A 8 -B 2
fiRepository: dashpay/platform
Length of output: 10262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== current git diff/stat =="
git diff --stat || true
git diff -- packages/simple-signer/src/single_key_signer.rs | sed -n '1,240p' || true
echo "== API/usages of private_key and signing methods =="
rg -n "private_key|secret_key|public_key|non_secure_erase|secret_bytes|from_secret_key|from_byte_array|inner" packages/simple-signer/src/single_key_signer.rs Cargo.lock Cargo.toml packages -g '*.rs' -g 'Cargo.toml' -g 'Cargo.lock'Repository: dashpay/platform
Length of output: 50373
🌐 Web query:
secp256k1 0.30.0 SecretKey non_secure_erase impl Copy documentation
💡 Result:
In the secp256k1 crate version 0.30.0, the SecretKey type implements the Copy trait [1][2]. Regarding the non_secure_erase method: 1. Functionality: The SecretKey type includes a method called non_secure_erase, which attempts to overwrite the memory containing the secret key with a default value [3][4][5]. 2. Security Limitations: The crate documentation explicitly warns that this method provides no guarantees regarding secure erasure [3][6]. Because Rust's compiler is unaware of the "secret" nature of the data, it is permitted to freely copy or move the contents of the memory to other locations, potentially leaving sensitive data in memory even after non_secure_erase is called [4][6][7]. 3. Recommendation: The documentation advises users to consult the zeroize crate for more robust strategies concerning secret erasure in Rust [3][4][6]. The implementation of non_secure_erase is generally handled via a macro that wraps a call to secp256k1_sys::non_secure_erase_impl [5].
Citations:
- 1: https://paritytech.github.io/try-runtime-cli/src/secp256k1/key.rs.html
- 2: https://rust.velas.com/secp256k1/key/struct.SecretKey.html
- 3: https://crates.io/crates/secp256k1/0.30.0
- 4: https://docs.rs/secp256k1/latest/src/secp256k1/key.rs.html
- 5: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/macros.rs
- 6: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/README.md
- 7: https://docs.rs/secp256k1/latest/secp256k1/struct.SecretKey.html
Erase derived SecretKey temporaries after comparing the public key.
secp256k1 0.30.0 makes SecretKey Copy, so the matched scalar remains reachable after PublicKey::from_secret_key derives public_key; Zeroizing only clears secret_bytes. Make the secret_key mutable at both conversion sites, compare public_key_bytes as needed, then call secret_key.non_secure_erase() for both ECDSA_SECP256K1 and ECDSA_HASH160.
🤖 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/simple-signer/src/single_key_signer.rs` around lines 151 - 156, The
derived SecretKey temporaries in both ECDSA_SECP256K1 and ECDSA_HASH160
conversion sites remain reachable after public-key comparison. Make each
secret_key mutable, perform the existing public_key_bytes comparison, then call
non_secure_erase() on the SecretKey before returning, preserving the current
validation result and handling both conversion paths.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4273 +/- ##
=============================================
- Coverage 87.53% 66.84% -20.69%
=============================================
Files 2678 27 -2651
Lines 341047 2835 -338212
=============================================
- Hits 298519 1895 -296624
+ Misses 42528 940 -41588
🚀 New features to boost your workflow:
|
3 participants
Issue being fixed or feature implemented
The iOS wallet is porting DashConnect — the
dash-key:/dash-st:passwordless login flow that already ships on Android. Four gaps inplatform-wallet-ffi/swift-sdkblock it:dashpay.rs: ecdh_shared_secret) — it takes a derivation path and returns a finished secret, so neither the arbitrary ephemeral key nor the raw X is reachable.dash-st:QR carries a serializedIdentityUpdateTransitionbuilt by the dApp. Without deserialization the wallet can only broadcast foreign bytes blind; it needs to verify the transition adds exactly the two login keys it derived for its own identity.decode_contract_boundsrejectedkind == 0(no bounds) for ENCRYPTION / DECRYPTION keys, while consensus accepts unbounded keys for every purpose (validate_identity_public_key_contract_bounds/v1). Real Yappr testnetdash-st:transitions carry an unbounded ENCRYPTION key, so the flow failed on a key Platform considers valid.What was done?
Rust FFI —
packages/rs-platform-wallet-ffisrc/secp256k1_primitives.rs(new) — three handle-free functions over the already-linkeddashcore::secp256k1, exported fromlib.rs:platform_wallet_secp256k1_verify_compressed_pointplatform_wallet_secp256k1_compressed_public_keyplatform_wallet_secp256k1_ecdh_shared_x— raw affine X viamul_tweak, deliberately unhashedsrc/identity_update.rs— parse-only entry pointsplatform_wallet_parse_identity_update_transition/..._free, returningParsedIdentityUpdateFFIandParsedIdentityUpdatePublicKeyFFI. Deserialization only: no signing, no broadcast, no state change.src/identity_registration_with_signer.rs—decode_contract_boundsno longer requires bounds for ENCRYPTION / DECRYPTION.kind == 0now decodes toNonefor every purpose;kind == 1/kind == 2decode and validate exactly as before. Doc comments corrected to describe the consensus rule rather than the old assumption.Swift SDK —
packages/swift-sdkSources/SwiftDashSDK/Security/Secp256k1Primitives.swift(new) —isValidCompressedPoint(_:),compressedPublicKey(privateKey:),ecdhSharedX(privateKey:publicKey:)over the new FFI.Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift—parseIdentityUpdateTransition(_:)wrapper mapping the parsed C structs into Swift values.Sources/SwiftDashSDK/FFI/KeychainSigner.swift—withAdditionalSigningKeys(_:): a scoped in-memory registry of(publicKey, privateKey)pairs that the sign trampoline consults before the platform-address / breadcrumb / persisted-key paths. Entries are popped and their private keys zeroed on scope exit. Mirrors Android'ssigningKeys.How Has This Been Tested?
cargo test -p platform-wallet-ffi— 26 lib + 6 integration tests pass, andcargo fmtis clean.cargo clippy -p platform-wallet-ffi --all-targets -- -D warningsreports nothing in the files this PR touches; its findings are all pre-existing onv4.2-devinsrc/persistence.rsandsrc/core_wallet_types.rs, which are untouched here.src/secp256k1_primitives.rs— 6 tests: known-vector public key and ECDH X, invalid-point rejection, out-of-range scalar, wrong-length inputs.src/identity_update.rs— 4 tests: parses a tagged transition, parses a tagless one captured from the live Yappr testnet app, rejects malformed and truncated bytes.src/identity_registration_with_signer.rs— 4 tests covering the bounds change: an unbounded ENCRYPTION key decodes,kind == 0decodes toNonefor AUTHENTICATION / ENCRYPTION / DECRYPTION, andkind == 1/kind == 2still reject null ids and payloads.SwiftTests/SwiftDashSDKTests/KeychainSignerAdditionalSigningKeysTests.swift(new) — scope push/pop, lookup by public key, and zeroing on exit.Manual, on testnet, from the iOS wallet against the live Yappr instance: scan the
dash-key:QR → publishloginKeyResponse; scan the app's realdash-st:QR → the transition is parsed here, validated against locally derived keys, and its two login keys are added to the identity throughupdateIdentitywith the scoped signer. Signing out on the Yappr website and signing back in with thedash-key:QR alone was also verified end to end.Breaking Changes
None. The new FFI entry points are additive. The only behaviour change is
decode_contract_boundsacceptingkind == 0for ENCRYPTION / DECRYPTION, which previously returnedErrorInvalidParameterfor a key consensus accepts — strictly a widening.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit