Conversation
github-actions
Bot
added
scope: dependencies
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ironclaw_chain_signing crate, which implements custodial multi-chain signing for EVM, Solana, and NEAR within the IronClaw substrate. It features two independent enforcement points—grant claim validation and sign-time transaction hash re-checks—alongside a "ship-gate" mechanism that restricts mainnet signing to secure HSM/KMS backends. Feedback focuses on improving code idiomaticity and maintainability by replacing manual hex encoding and decoding logic with built-in functionality from the alloy-primitives and hex libraries.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbe5abfd39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fbe5abf to
27faead
Compare
Replace the crate's hand-rolled hex encode/decode/nibble helpers with the
already-depended-on alloy-primitives hex utilities and Address parsing:
- custodial.rs: bound_evm_address now parses via `Address::parse`; EVM signer
formatted with `{:#x}`; ed25519 pubkey via `hex::decode` + `try_into`; signer
hex via `hex::encode`. Removes hex_decode_20/32, nibble, hex_lower.
- keystore.rs: encode private-key hex via `hex::encode` (kept inside Zeroizing
to preserve key-zeroization), decode via `hex::decode`. Removes hex_encode/
hex_decode/hex_nibble.
No behavior or security-property change: Zeroizing wrapping, exact-bytes
binding, KMS ship-gate, and fail-closed paths are all preserved.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Implement the ironclaw_chain_signing crate for custodial multi-chain sign/broadcast as PR6 of the 10-PR attested-signing stack
Stats: 18 findings (from 22 raw) across 10 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 3
Security (1)
- Low DecryptedSecret has a manual Clone impl that duplicates secret material in memory (
crates/ironclaw_secrets/src/legacy_store.rs:58-99, confidence 50) — anchor:crates/ironclaw_secrets/src/legacy_store.rs:93(no diff position — body only)
DecryptedSecret wraps a SecretString and provides a manual Clone impl that creates a full copy of the underlying secret. Cloning doubles the number of live copies of decrypted secret material in memory.
Fix:Remove the Clone derive/impl from DecryptedSecret. If cloning is needed, implement a consume() method that transfers ownership without copying.
Bugs (1)
- Medium KMS signature binding rejects valid sigs when explicit v is wrong (
crates/ironclaw_chain_signing/src/evm/sign.rs:1603-1617, confidence 75) — anchor:crates/ironclaw_chain_signing/src/evm/sign.rs:1603
In bind_kms_signature, when the KMS returns a 65-byte signature with an explicit v, the code tries recovery with only that v's parity. If recovery fails (e.g. buggy KMS returns wrong v), it returns SignerMismatch instead of falling back to trying both parities.
Fix:After the explicit-v try_v fails, fall through to the both-parities fallback instead of returning SignerMismatch immediately.
Performance (2)
-
Medium LocalKmsSigner keys HashMap grows unbounded with no eviction (
crates/ironclaw_chain_signing/src/kms.rs:102-107, confidence 70) — anchor:crates/ironclaw_chain_signing/src/kms.rs:106
The keys HashMap accepts imports via import_key but has no remove, clear, or capacity-limit mechanism.
Fix:Add a remove_key method or enforce a max capacity at import time; document that imports are expected only at bootstrap.
Also flagged by: security/Medium -
High KmsSigner::sign_digest is sync — cloud backends will block the async executor (
crates/ironclaw_chain_signing/src/kms.rs:70-88, confidence 80) — anchor:crates/ironclaw_chain_signing/src/kms.rs:82
The KmsSigner trait defines sign_digest as a synchronous fn. A production cloud KMS backend will make an HTTP/RPC call inside sign_digest, blocking the tokio worker thread for the full network round-trip.
Fix:Change the trait to async fn sign_digest(...) and update all callers to .await.
Tests (6)
-
Medium CustodialSigner::finalize with non-terminal state not tested (
crates/ironclaw_chain_signing/src/custodial.rs:431-448, confidence 75) — anchor:crates/ironclaw_chain_signing/src/custodial.rs:436
The finalize method rejects non-terminal states with InvalidTransition, but no test exercises this error path.
Fix:tests::custodial_signing::finalize_rejects_non_terminal_state -
Medium bound_evm_address with invalid hex not tested (
crates/ironclaw_chain_signing/src/custodial.rs:471-480, confidence 75) — anchor:crates/ironclaw_chain_signing/src/custodial.rs:471
The bound_evm_address helper returns KeyStore error when the binding's public_address_hex cannot be parsed. No test exercises this malformed-hex error path.
Fix:tests::custodial_signing::bound_evm_address_rejects_invalid_hex -
Medium ed25519_pubkey_from_binding with wrong-length hex not tested (
crates/ironclaw_chain_signing/src/custodial.rs:485-494, confidence 75) — anchor:crates/ironclaw_chain_signing/src/custodial.rs:485
The ed25519_pubkey_from_binding helper returns KeyStore error when the decoded hex is not exactly 32 bytes. No test exercises this.
Fix:tests::custodial_signing::ed25519_pubkey_from_binding_rejects_wrong_length -
Medium ChainFamily::of_transaction and ChainKeyId::family have no direct tests (
crates/ironclaw_chain_signing/src/chain.rs:64-73, confidence 75) — anchor:crates/ironclaw_chain_signing/src/chain.rs:66
The public ChainFamily::of_transaction function and ChainKeyId::family method have no unit tests. These are used in the authorize chain-binding check.
Fix:tests::chain::of_transaction_maps_variants_and_unknown_prefix -
Medium rebuild_signable with unsupported tx type not tested (
crates/ironclaw_chain_signing/src/evm/decode.rs:256-257, confidence 75) — anchor:crates/ironclaw_chain_signing/src/evm/decode.rs:256
rebuild_signable returns a Decode error for unknown tx types. This fail-closed path has no test coverage.
Fix:tests::evm_decode::rebuild_signable_rejects_unsupported_tx_type -
Low SecretsKeyStore::binding returning NotFound not tested (
crates/ironclaw_chain_signing/src/keystore.rs:281-290, confidence 75) — anchor:crates/ironclaw_chain_signing/src/keystore.rs:289
The binding method returns NotFound when no key exists for the scope/chain pair. The binding-specific NotFound path is untested.
Fix:tests::keystore::binding_missing_is_not_found
Conventions (1)
- Medium ChainKeyId uses #[serde(transparent)] instead of canonical try_from newtype template (
crates/ironclaw_chain_signing/src/chain.rs:14-16, confidence 75) — anchor:.claude/rules/types.md:66-110
ChainKeyId is a newly added identity newtype that uses #[serde(transparent)] for serde deserialization, bypassing the canonical newtype template mandated by .claude/rules/types.md.
Fix:Adopt the canonical newtype template from types.md: add validate(&str), switch to #[serde(try_from = "String")], implement TryFrom<String>, AsRef<str>, and From<ChainKeyId> for String.
Local Patterns (2)
-
Medium EVM decode module visibility and re-export pattern diverges from Solana/NEAR siblings (
crates/ironclaw_chain_signing/src/evm/mod.rs:10-23, confidence 100) — anchor:crates/ironclaw_chain_signing/src/solana/mod.rs:14 vs crates/ironclaw_chain_signing/src/evm/mod.rs:10
The EVM vertical declares pub(crate) mod decode and re-exports its functions, while Solana and NEAR both use pub mod decode without re-exports.
Fix:Make EVM decode pub mod decode (matching Solana/NEAR) and remove the pub use decode::{...} re-export, OR add matching re-exports to Solana and NEAR mod.rs files. -
Low Broadcast trait method names differ across chains for the same conceptual operation (
crates/ironclaw_chain_signing/src/evm/broadcast.rs:33-37, confidence 75) — anchor:crates/ironclaw_chain_signing/src/evm/broadcast.rs:36 vs solana/broadcast.rs:27 vs near/broadcast.rs:27
The three broadcaster traits name their submit method differently: send_raw, send_transaction, broadcast_tx.
Fix:Unify to a single method name across all three broadcaster traits, e.g. submit_signed or broadcast.
Maintainability (5)
-
Low authorize_chain has unreachable Err(CustodyDecision::Allow) dead branch (
crates/ironclaw_chain_signing/src/kms.rs:341-352, confidence 50) — anchor:crates/ironclaw_chain_signing/src/kms.rs:348
ShipGate::authorize only ever returns Err(CustodyDecision::Deny).
Fix:Change authorize to return Result<SigningPath, String>. -
Medium Three near-identical sign_* methods could collapse into one generic flow (
crates/ironclaw_chain_signing/src/custodial.rs:236-391, confidence 75) — anchor:crates/ironclaw_chain_signing/src/custodial.rs:236
sign_evm, sign_solana, and sign_near follow the exact same orchestration. Only the digest computation, signing primitive, and outcome formatting differ.
Fix:Define a ChainSignerOps trait with extract_signer(), compute_digest(), sign_hot(), sign_kms(), format_outcome().
Also flagged by: pattern-refactor/Medium
Also flagged by: security/Medium -
Low new() and with_kms() constructors differ only in one Optional field (
crates/ironclaw_chain_signing/src/custodial.rs:99-134, confidence 75) — anchor:crates/ironclaw_chain_signing/src/custodial.rs:99
CustodialSigner::new and with_kms are identical except one sets kms: None.
Fix:Replace both constructors with a single builder or one constructor that takes Option<Arc<dyn KmsSigner>>. -
Medium KeyStoreKey duplicates ScopeKey scope-to-string mapping from ironclaw_secrets (
crates/ironclaw_chain_signing/src/keystore.rs:174-199, confidence 50) — anchor:crates/ironclaw_chain_signing/src/keystore.rs:174
KeyStoreKey reconstructs the same tenant/user/agent/project-to-string mapping.
Fix:Move ScopeKey to ironclaw_host_api or make it pub in ironclaw_secrets. -
Medium Solana and NEAR sign modules are copy-paste with only chain tags differing (
crates/ironclaw_chain_signing/src/solana/sign.rs:40-118, confidence 100) — anchor:crates/ironclaw_chain_signing/src/solana/sign.rs:40
solana/sign.rs and near/sign.rs contain structurally identical functions.
Fix:Extract a shared ed25519_signing module.
Headline (High/perf): make KmsSigner::sign_digest async via #[async_trait] so a production cloud KMS/HSM round-trip never blocks a tokio worker. The in-tree LocalKmsSigner stays CPU-only and returns a ready future; all three custodial sign_* call sites now .await. Ship-gate semantics (mainnet fail-closed without KMS; curve-capability check) and key zeroization are unchanged. Also addressed: - kms: add LocalKmsSigner::remove_key for explicit eviction (+ test); documents bootstrap-only import expectation (unbounded-HashMap finding). - chain: adopt canonical newtype template for ChainKeyId (validate(), #[serde(try_from)], TryFrom<String>, AsRef<str>, From<ChainKeyId> for String, into_inner); fail-closed on empty (conventions finding). - tests: add coverage for ChainFamily::family/of_transaction, ChainKeyId validation, bound_evm_address invalid hex, ed25519_pubkey_from_binding wrong length, rebuild_signable unsupported tx type, and keystore binding NotFound. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addressed henrypark133 review (commit ded8dfd)Worked off the force-pushed Fixed
Already fixed / stale
Deferred (with reasons in inline replies)
Verification
|
8ae1abd to
4045354
Compare
ded8dfd to
19136c8
Compare
| let authorized = self.authorize(req, ChainFamily::Solana).await?; | ||
|
|
||
| let DecodedTransaction::Solana(sol) = &req.decoded else { | ||
| return Err(ChainSigningError::ChainMismatch { |
There was a problem hiding this comment.
Medium — Solana/NEAR sign over sha256(canonical_signing_bytes), not the network wire format; produced signatures are not directly broadcastable, and the approved hash is schema-version-coupled.
The custodial signer for Solana and NEAR signs a 32-byte sha256(canonical_signing_bytes(decoded, schema_version)). This is explicitly acknowledged as a deferred "next slice". The immediate security concern is not the gap itself (the broadcast path is also deferred), but two structural risks that should be addressed before the wire decoder lands:
-
Schema-version coupling of approved hash:
ApprovedTxHashis sealed against(decoded_tx, schema_version). Ifcanonical_signing_byteschanges between schema versions (e.g. a new field is added), a grant approved underSCHEMA_V1cannot be re-verified underSCHEMA_V2. The current codebase has a singleCURRENTversion, but the version is threaded all the way through the signing path, implying future versions are expected. There is no test that signs underSCHEMA_V1and verifies the hash still matches after a hypothetical schema bump. -
Signature domain mismatch: When the Solana/NEAR wire decoder arrives (using
solana-sdk/near-primitivesborsh), the canonical bytes it produces MUST be byte-identical tocanonical_signing_bytesfor the same transaction — otherwise the approved hash from the current schema and the broadcastable signature from the new decoder would disagree. The spec should make this equivalence a hard contract and add a cross-schema round-trip test before that PR lands.
Fix: In this PR, add a #[doc(hidden)] marker or // INVARIANT: comment at canonical_signing_bytes spelling out that any new schema version MUST preserve the property that the wire-format bytes are a deterministic function of the canonical bytes (or the approved hash must be recomputed). Add a future-proofing test: assert_eq!(recompute_approved_hash(&tx, signer, SchemaV1), recompute_approved_hash(&tx, signer, SchemaV1)) as a regression pin.
There was a problem hiding this comment.
Addressed in 5711af0. Added an explicit INVARIANT block on recompute_approved_hash documenting (1) the approved hash is sealed against (decoded_tx, signer, schema_version) so a schema bump fails closed by design, and (2) the future wire decoder must keep canonical bytes a deterministic function of the same pair. Added regression pin recompute_approved_hash_is_stable_for_a_fixed_schema covering same-schema determinism plus the signer-binding (WYSIWYS) property. The canonical encoder itself lives in ironclaw_attestation (PR2 scope); this PR pins the contract at its chain_signing consumption point.
There was a problem hiding this comment.
Still accurate — leaving open, and it is a documented limitation rather than an oversight.
Confirmed in the code: solana/sign.rs signs the 32-byte sha256 commitment of the canonical signing bytes (sign.rs:66-82), not the network wire message. The same holds for NEAR. So the signatures these paths produce are not broadcastable as-is — they attest to IronClaw's canonical commitment, not to a serialized Solana/NEAR transaction.
Why it is this way: ironclaw_attestation deliberately carries no chain SDKs (pinned by the attested_signing_boundaries architecture test), so the SDK-level wire encoders for Solana VersionedMessage/ALT and NEAR borsh Transaction were flagged as a follow-up rather than vendored into the canonical layer. EVM is the complete vertical (rebuild_signable produces a real signable via alloy).
The practical consequence — custodial Solana/NEAR cannot broadcast yet — is contained by the ship gate, and the external-wallet path doesn't have this problem because the wallet serializes and signs natively. Tracked in #6532 as the Solana/NEAR wire-decoder item; leaving open until those land.
Architectural comment —
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Paranoid-architect review — PR6/10 attested-signing: ironclaw_chain_signing
Summary: The two-enforcement-point architecture (grant claim + sign-time hash re-check), WYSIWYS property, ecrecover/ed25519 binding checks, broadcast idempotency, ship-gate, AAD-bound chain-key secrets, and test coverage are all strong and well-structured. This is a high-quality security-critical crate. Two issues need resolution before merge.
Blocking (High)
H1 — LocalKmsSigner::is_secure_custody() = true bypasses the mainnet ship-gate (kms.rs:193): The backend stores keys in process heap memory — the exact attack surface threat #18 guards against. Returning true means any operator who wires LocalKmsSigner (or a derived subclass) with the mainnet opt-in can bypass the KMS requirement. Fix: return false, provide a test-only wrapper that overrides it, and add a prominent doc/compile warning.
Non-Blocking (Medium, Low, Nit)
M2 — Unzeroized hex-decode buffer in SecretsKeyStore::consume() (keystore.rs:331): Raw private-key bytes from hex::decode not wrapped in Zeroizing before moving into ConsumedChainKey::new. Fix: wrap in Zeroizing::new(bytes) before the move, consistent with the bind() path.
M3 — Grant/ledger atomicity gap (custodial.rs:198): authorize() claims the grant one-shot, then returns. Pre-flight failures between claim and ledger.advance(Signing) leave the grant consumed but the ledger at Approved, permanently blocking this gate_ref. Fix: move grants.claim() to immediately before ledger.advance(Signing) after all pre-flight checks pass.
M4 — Schema-version coupling of Solana/NEAR approved hash (custodial.rs): canonical_signing_bytes schema-versioning is threaded through but no contract exists that future schema versions preserve the approved-hash equivalence. Add an invariant comment and regression test pin.
L5 — EIP-7702 v-parity normalization undocumented assumption (evm/sign.rs:167)
Nit — Shared HKDF label across all secret domains (crypto.rs)
See inline comments for details.
H1: LocalKmsSigner::is_secure_custody() no longer hard-codes true — it now reflects a constructor-time flag defaulting to false, so the in-process software backend can NEVER satisfy the mainnet ship-gate in a normal build (threat #18). Tests opt in via the explicit, doc-hidden new_modeling_secure_custody(). Adds a regression test proving the default LocalKmsSigner is refused for mainnet even with the opt-in flag. M2: SecretsKeyStore::consume() wraps the hex-decoded private-key bytes in Zeroizing before ConsumedChainKey::new takes ownership, so the decode buffer is wiped even if into_boxed_slice reallocates (matching the bind() path). M3: Grant/ledger atomicity. The one-shot grant claim moved out of authorize() to the last step before the Approved->Signing ledger transition in each sign_* method, after all chain-specific pre-flight (digest rebuild, address parsing, KMS key_ref resolution) succeeds. A post-authorize pre-flight failure now leaves the grant unclaimed and the gate_ref retryable. Adds a regression test. M4: Documents the schema-version coupling invariant on recompute_approved_hash (approved hash is a deterministic function of (decoded_tx, signer, schema) and a future schema bump fails closed) and pins same-schema determinism + signer binding with a regression test. L5: Documents that the explicit-v parity normalization arm is EIP-155 only and that EIP-7702 (type 4) is rejected upstream by rebuild_signable. Nit: Documents that all secret domains intentionally share the single HKDF label and that AAD + per-secret salt are the cross-domain separators. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addressed henrypark133 paranoid-architect review (tip
|
| Finding | Disposition |
|---|---|
| H1 LocalKmsSigner::is_secure_custody == true bypasses mainnet ship-gate | Fixed. Now a constructor-time flag defaulting to false; new() is insecure, tests opt in via #[doc(hidden)] new_modeling_secure_custody(). Regression test added. |
| M2 Unzeroized hex-decode buffer in consume() | Fixed. Wrapped in Zeroizing<Vec<u8>> before ConsumedChainKey::new, matching the bind() path. |
| M3 Grant/ledger atomicity gap | Fixed (preferred option). Grant claim moved to the last step before ledger.advance(Signing), after all pre-flight (incl. KMS key_ref resolution). Regression test added. |
| M4 Schema-version coupling of approved hash | Addressed. INVARIANT doc on recompute_approved_hash + same-schema determinism / signer-binding regression pin. |
| L5 EIP-7702 v-parity assumption undocumented | Addressed. Comment added: EIP-155-only; type 4 rejected upstream by rebuild_signable. |
| Nit Shared HKDF label | Addressed (accepted as intentional). Clarifying comment: AAD prefix + per-secret salt are the cross-domain separators. |
Preserved invariants: mainnet fail-closed without KMS, KmsSigner curve-capability fail-closed, async KMS sign, ApprovedTxHash WYSIWYS binding via gate-bound signer, Solana broadcast maxRetries:0 idempotency, key zeroization (Zeroizing/SecretBox kept), openssl-free.
Verification (IRONCLAW_DISABLE_OS_KEYCHAIN=1): cargo test -p ironclaw_chain_signing --all-features — 56 unit + 16 integration tests pass; cargo fmt + cargo clippy --all-features --tests clean on both touched crates.
3889b3b to
cddd527
Compare
…dcast (#3965, squashed for cascade)
5711af0 to
29f1cb7
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
…dcast (#3965, squashed for cascade)
cddd527 to
883675c
Compare
29f1cb7 to
685b835
Compare
|
Superseded by #6749, part of consolidating the 20-PR The content of this PR is carried forward in full, re-based on current Closing to keep the queue honest. The review history stays on this PR and remains readable; reopen if the consolidation is rejected. |
…dcast (#3965, squashed for cascade)
…dcast (#3965, squashed for cascade)
2 participants
What this PR is
ironclaw_chain_signing— custodial multi-chain sign/broadcast behind the secrets keystore, with the KMS ship-gate that refuses custodial mainnet signing without secure custody.Cascade changes (API adoption — please review)
This branch forked from
-03before its round-2 security fixes landed, so the port had to adopt the stronger APIs rather than merge around them. None of these weaken an invariant:SigningLedger::create/state/advancetake&LedgerKey { tenant, gate_ref }, not&GateRef. Rethreaded 8 production + 15 test call sites throughSigningContext.tenant(which already carries it). This is the tenant-isolation fix a reviewer caught in round 2 — reverting the API to make the conflict disappear would have silently dropped it.AttestedSigningGrant::seal→::new, and::newis now fallible (timestamp/expiry validation); 10 call sites rewrapped.main;ironclaw_secretsre-export list unioned (chain_key_aad+validate_master_key_material).Verification
56 + 16 crate tests pass; boundary test green — including
chain_signing_crate_carries_chain_sdk_and_secrets, the inverse-purity assertion that this crate is the only one allowed chain SDKs + secrets; clippy clean.