Skip to content

feat(signing): ironclaw_attested_runtime — reborn AttestedResumePort + signer continuation + ship-gate (attested-signing PR10/12) - #3994

Closed
zmanian wants to merge 4 commits into
attested-signing-08-near-redirectfrom
attested-signing-10-reborn-runtime
Closed

zmanian wants to merge 4 commits into
attested-signing-08-near-redirectfrom
attested-signing-10-reborn-runtime

Conversation

@zmanian

@zmanian zmanian commented May 24, 2026 •

Copy link
Copy Markdown
Collaborator

Rebased onto current main (attested-signing cascade, 2026-07-23). The stack was 1184 commits behind (merge base 2026-05-24). Tracking issue: #6532.

Inline review threads may have re-anchored or orphaned from the force-push. Reviewers: the "Cascade changes" section states exactly what the rebase altered beyond the original work.

⚠️ This branch was restructured. It was a cumulative merge branch; it is now a linear commit on a rebuilt composed base. See below before diffing.

What this PR is

ironclaw_attested_runtime — the reborn AttestedResumePort implementation, the signer-continuation driver, the attested gate-binding store, and the custodial-mainnet ship gate — plus the composition glue (RebornAttestedComposition) that assembles them. Lives outside src/ per the binary-boundary rule.

Cascade changes (structural — please review)

  1. Composed base rebuilt. This branch merged -06 (chain_signing) and -09 (WalletConnect) onto -08. Those sibling merges captured stale snapshots (each sibling had since gained round-2 fixes). The base was rebuilt by merging the ported siblings onto ported -08, then replaying this PR's single integration commit on top. The boundary test now carries all three assertions (wallet_external purity, chain_signing-carries-SDK, openssl-free).
  2. Production wiring re-expressed against main's store graph. The original wired the port into InMemoryTurnStateStore, which main deleted.
    • production_turn_state_store gained an Option<Arc<dyn AttestedResumePort>> parameter; local-dev builds RuntimeAttestedResumePort over the shared binding store and injects it via the with_attested_resume_port seam added in feat(signing): turns BlockedAttested gate + AttestedResumePort + deterministic resume split (attested-signing PR5/10) #3966. The production/owner store builders pass None (durable backends are a later hop).
    • build_attested_composition assembles the composition (in-memory custodial keystore, ship gate from env so mainnet custodial signing stays refused, empty provider registry) and it is held as RebornServices.attested_signing (Some local-dev / None production) with a RebornRuntime::attested_signing() accessor.
    • Note for reviewers: this PR does not dispatch the continuation driver — every continue_after_resolved call here is in tests. Production dispatch belongs to the gate/resolve ingress PR (feat(signing): reborn webui attested gate/resolve ingress (attested-signing PR11/12) #3995), which is where the composition stored here gets driven.
  3. API adoption: driver ledger calls rethreaded to the tenant-scoped LedgerKey (tenant sourced from binding.context.tenant; recover_unknown gained a tenant parameter); SecretsCrypto::generate() moved to main's rand 0.9 API (SysRng::try_fill_bytes with rng().fill fallback).

Verification

31 ironclaw_attested_runtime tests pass — including the full 21-test threat matrix. Composition e2e test driving the real RebornAttestedComposition passes. Whole workspace builds (incl. all test targets); boundary test green; clippy clean on the composition.

Carried forward

tests/resume_through_store.rs is temporarily set aside — it drives the deleted in-memory store and needs the same row-store retarget as #3966's tests. Its security properties are covered by the threat matrix and #3966's new attested tests; it should be folded back in with #3995, where the ingress/dispatch it assumes actually exists.

@github-actions github-actions Bot added size: XL 500+ changed lines scope: dependencies Dependency updates risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs and removed size: XL 500+ changed lines labels May 24, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 51ee75159d

ℹ️ 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".

Comment thread crates/ironclaw_attested_runtime/src/driver.rs Outdated
Comment thread crates/ironclaw_reborn_composition/src/runtime.rs Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the ironclaw_chain_signing and ironclaw_attested_runtime crates, completing the attested-signing substrate with custodial key management, WalletConnect v2 support, and multi-chain signing capabilities. The implementation features a robust security model using one-shot grants, idempotency ledgers, and HSM/KMS ship-gates. Feedback focuses on enhancing the signer-continuation driver to support job recovery by ignoring existing ledger rows, ensuring the custodial path verifies user authorization proofs, and handling poisoned mutexes explicitly. Further improvements include using zeroizing types for sensitive key material and centralizing duplicated hex utility logic to minimize the maintenance surface and adhere to project rules.

Comment thread crates/ironclaw_attested_runtime/src/driver.rs Outdated
Comment thread crates/ironclaw_attested_runtime/src/driver.rs Outdated
Comment thread crates/ironclaw_attested_runtime/src/binding.rs
Comment thread crates/ironclaw_chain_signing/src/keystore.rs Outdated
Comment thread crates/ironclaw_chain_signing/src/lib.rs
@github-actions github-actions Bot added the size: XL 500+ changed lines label May 24, 2026
zmanian added a commit that referenced this pull request May 25, 2026
Zeroize the transient hex-encoded private key in SecretsKeyStore::bind
after encryption consumes it. The hex copy lived in a plain String that
does not zeroize on drop, leaving key material in process memory longer
than necessary. Scrub it unconditionally (success or error) using the
zeroize re-export already provided by secrecy — no new dependency.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code Review (multi-agent)

Intent: Wire attested-signing substrate into reborn runtime via new ironclaw_attested_runtime crate with resume port, continuation driver, and ship-gate
Stats: 23 findings (from 27 raw) across 12 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 8

Security (7)

  1. High NEAR FunctionCall scope validation relaxed — fail-closed check removed (crates/ironclaw_wallet_external/src/near_redirect/mod.rs:307-340, confidence 100) — anchor: crates/ironclaw_wallet_external/src/near_redirect/mod.rs:323 (no diff position — body only)
    The diff removes the fail-closed check in validate_access_key_scope that rejected FunctionCall access keys when bound.receiver_id was None. An attacker with a FunctionCall key restricted to receiver A could authorize a transaction targeting receiver B.
    Fix: Restore the fail-closed check: when bound.receiver_id is None, reject FunctionCall scopes.

  2. High NEAR redirect verifies signature against callback-supplied key instead of gate-bound key (crates/ironclaw_wallet_external/src/near_redirect/mod.rs:258-280, confidence 100) — anchor: crates/ironclaw_wallet_external/src/near_redirect/mod.rs:270 (no diff position — body only)
    The diff removes the expected_access_key field and changes verify_signature_over_hash to use the callback-supplied key instead of the gate-bound key. An attacker can supply their own keypair and pass verification.
    Fix: Restore the expected_access_key field. Verify the signature against the gate-bound key.

  3. Medium Hex decode can panic on non-ASCII input (DoS) (crates/ironclaw_wallet_external/src/near_redirect/mod.rs:415-428, confidence 75) — anchor: crates/ironclaw_wallet_external/src/near_redirect/mod.rs:417 (no diff position — body only)
    hex_bytes::hex_decode uses u8::from_str_radix on &str slices which panics on non-ASCII at odd byte offsets.
    Fix: Revert to byte-based decoder using as_bytes().chunks_exact(2).

  4. Medium Hex parse in signer.rs can panic on non-ASCII bound_account (DoS) (crates/ironclaw_wallet_external/src/walletconnect/signer.rs:195-231, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/signer.rs:197
    parse_evm_address and parse_ed25519_pubkey use u8::from_str_radix on &str slices which panics on non-ASCII.
    Fix: Use byte-based parsing with as_bytes().chunks_exact(2).

  5. Medium encode_walletconnect_proof silently returns empty bytes on serialization failure (crates/ironclaw_wallet_external/src/walletconnect/mod.rs:271-273, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/mod.rs:271
    encode_walletconnect_proof uses serde_json::to_vec(payload).unwrap_or_default().
    Fix: Return Result<Vec<u8>, SigningProviderError> and propagate the error.

  6. Medium No input size limit on proof payload deserialization (crates/ironclaw_wallet_external/src/walletconnect/proof.rs:52-57, confidence 50) — anchor: crates/ironclaw_wallet_external/src/walletconnect/proof.rs:52
    decode_walletconnect_proof calls serde_json::from_slice on arbitrary input without size limit.
    Fix: Add a size check before deserialization.

  7. Low Hardcoded local-dev master key in source code (crates/ironclaw_reborn_composition/src/runtime.rs:917-930, confidence 50) — anchor: crates/ironclaw_reborn_composition/src/runtime.rs:925
    build_attested_composition hardcodes the local-dev master key.
    Fix: Generate a random key at startup or read from config.

Bugs (3)

  1. Medium SessionBindingStore::record silently drops binding on poisoned lock (crates/ironclaw_wallet_external/src/walletconnect/session.rs:59-63, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/session.rs:60
    If the Mutex is poisoned, record() silently does nothing.
    Fix: Panic or return an error when the lock is poisoned.
    Also flagged by: tests/Medium
    Also flagged by: security/Medium

  2. High External-wallet path advances ledger to Signing before verify_resume, leaving it stuck on failure (crates/ironclaw_attested_runtime/src/driver.rs:344-353, confidence 100) — anchor: crates/ironclaw_attested_runtime/src/driver.rs:344
    The ledger is advanced to Signing BEFORE provider.verify_resume is called. If verify_resume fails, the ledger is stuck at Signing.
    Fix: Move the ledger.advance call to after verify_resume succeeds.
    Also flagged by: tests/High

  3. Medium recover_unknown silently swallows non-InvalidTransition errors in release builds (crates/ironclaw_attested_runtime/src/driver.rs:536-551, confidence 75) — anchor: crates/ironclaw_attested_runtime/src/driver.rs:544
    debug_assert! is a no-op in release, so any other error is silently swallowed.
    Fix: Replace debug_assert! with tracing::warn!.

Performance (3)

  1. Critical hex_decode panics on non-ASCII input — callback DoS (crates/ironclaw_wallet_external/src/walletconnect/mod.rs:293-302, confidence 90) — anchor: crates/ironclaw_wallet_external/src/walletconnect/mod.rs:293
    hex_decode uses &str slicing which panics when the input contains non-ASCII bytes at an odd character boundary. Proof payloads come from external wallets.
    Fix: Replace str-slicing with byte-chunk decoding.

  2. Medium SessionBindingStore::record silently drops errors on poisoned lock (crates/ironclaw_wallet_external/src/walletconnect/session.rs:61-65, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/session.rs:61
    If the Mutex is poisoned, record() silently discards the binding update.
    Fix: Return Result<()> from record().

  3. Medium hex_lower allocates String on every sync resume in contended mutex path (crates/ironclaw_attested_runtime/src/port.rs:113-114, confidence 65) — anchor: crates/ironclaw_attested_runtime/src/port.rs:113
    verify_attested_resume calls hex_lower which allocates a new String inside the turn store resume mutex.
    Fix: Compare bytes directly without hex encoding.
    Also flagged by: maintainability/Low

Tests (5)

  1. Medium recovery_id_from_v invalid v values and parse_evm_address edge cases untested (crates/ironclaw_wallet_external/src/walletconnect/signer.rs:170-184, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/signer.rs:170
    Invalid v values and parse_evm_address edge cases are never tested.
    Fix: tests::walletconnect::signer::recovery_id_from_v_invalid

  2. Medium BindingChainMismatch error path untested at driver level (crates/ironclaw_attested_runtime/src/driver.rs:395-397, confidence 75) — anchor: crates/ironclaw_attested_runtime/src/driver.rs:395
    The driver-level chain mismatch re-check has no test.
    Fix: tests::driver::driver_rejects_binding_chain_mismatch

  3. Medium Contradictory broadcaster behavior paths untested (crates/ironclaw_attested_runtime/src/driver.rs:486-516, confidence 75) — anchor: crates/ironclaw_attested_runtime/src/driver.rs:486
    Two contradictory broadcaster cases have no tests.
    Fix: tests::driver::submitting_broadcaster_returns_not_broadcast_unknown

  4. Medium EIP-2930 rebuild roundtrip and all RebuildError variants untested (crates/ironclaw_attested_runtime/src/driver/rebuild.rs:79-88, confidence 75) — anchor: crates/ironclaw_attested_runtime/src/driver/rebuild.rs:79
    No test for EIP-2930 rebuild or any error path.
    Fix: tests::driver::rebuild::eip2930_rebuild_roundtrips_signature_hash

  5. Medium CustodialMainnetShipGate::from_env() env-var parsing untested (crates/ironclaw_attested_runtime/src/ship_gate.rs:39-48, confidence 75) — anchor: crates/ironclaw_attested_runtime/src/ship_gate.rs:39
    from_env() parses CUSTODIAL_MAINNET_ENABLED. No test verifies the parsing logic.
    Fix: tests::ship_gate::from_env_truthy_values

Local Patterns (3)

  1. Low Orphaned // follow-up: comment interrupts doc-comment block (crates/ironclaw_attested_runtime/src/driver.rs:450-454, confidence 75) — anchor: crates/ironclaw_attested_runtime/src/driver.rs:450
    A regular comment sits between doc-comment paragraphs.
    Fix: Move the follow-up note into the doc-comment block.

  2. Medium New crate missing AGENTS.md that every sibling crate has (crates/ironclaw_attested_runtime/:1-1, confidence 100) — anchor: crates/AGENTS.md (no diff position — body only)
    The new ironclaw_attested_runtime crate has no AGENTS.md.
    Fix: Add crates/ironclaw_attested_runtime/AGENTS.md.

  3. Medium Error enums use manual Display+Error impls instead of thiserror derive (crates/ironclaw_attested_runtime/src/binding.rs:24-69, confidence 75) — anchor: crates/ironclaw_chain_signing/src/error.rs:8-13
    BindingError, ContinuationError, and RebuildError use manual impls while neighboring crates use thiserror.
    Fix: Add thiserror and replace manual impls.

Maintainability (2)

  1. Medium BroadcastDisposition and BroadcastOutcome are structurally identical and map 1:1 (crates/ironclaw_attested_runtime/src/driver.rs:84-118, confidence 75) — anchor: crates/ironclaw_attested_runtime/src/driver.rs:478
    Two types with identical shape and a 1:1 mapping.
    Fix: Unify into a single enum.

  2. Low CustodialSignerLike trait has one method and one impl (crates/ironclaw_attested_runtime/src/driver.rs:555-580, confidence 50) — anchor: crates/ironclaw_attested_runtime/src/driver.rs:568
    The trait exists to avoid naming generic parameters.
    Fix: Either make the driver generic or document the tradeoff.

Comment thread crates/ironclaw_wallet_external/src/walletconnect/mod.rs
Comment thread crates/ironclaw_attested_runtime/src/driver.rs
Comment thread crates/ironclaw_wallet_external/src/walletconnect/signer.rs Outdated
Comment thread crates/ironclaw_wallet_external/src/walletconnect/signer.rs
Comment thread crates/ironclaw_wallet_external/src/walletconnect/mod.rs
Comment thread crates/ironclaw_attested_runtime/src/driver.rs
Comment thread crates/ironclaw_attested_runtime/src/driver/rebuild.rs
Comment thread crates/ironclaw_attested_runtime/src/ship_gate.rs
Comment thread crates/ironclaw_attested_runtime/src/binding.rs
Comment thread crates/ironclaw_attested_runtime/src/port.rs
zmanian added a commit that referenced this pull request May 26, 2026
… hardening)

NEAR redirect (ironclaw_wallet_external):
- High: verify the ed25519 signature against the GATE-BOUND access key, not
  the callback-supplied one. The bound NEAR identity now carries the expected
  access-key pubkey (account_id:hex); a callback that substitutes its own key
  fails closed with SignerMismatch (threat #4), mirroring the Solana provider.
- High: restore fail-closed FunctionCall scope validation. Until the borsh tx
  receiver decode lands, a restricted FunctionCall key cannot be proven to
  cover the bound operation, so it is refused rather than accepted (threat #22).
- Critical/Medium: byte-based hex decoding everywhere (near_redirect, walletconnect
  mod/signer, solana) so non-ASCII callback input can no longer panic (DoS).
- Medium: proof encoders return Result instead of silently emitting empty bytes;
  decoders reject oversized payloads before deserialization.
- Medium: SessionBindingStore recovers a poisoned mutex instead of silently
  dropping the binding.

Attested runtime driver (ironclaw_attested_runtime):
- High: verify-before-advance on the external-wallet path — the ledger only
  advances to Signing after verify_resume succeeds, so a rejected proof leaves
  the row at Approved instead of stranding it at Signing (one-shot deadlock).
- Medium: recover_unknown logs at warn! on a non-benign ledger error instead of
  a release-compiled-out debug_assert.
- Tests: external-wallet verify-fail/success/provider-mismatch, driver-level
  BindingChainMismatch, contradictory broadcaster paths, EIP-2930 rebuild +
  rebuild error paths, and ship_gate from_env parsing.

Local-dev composition (ironclaw_reborn_composition / ironclaw_secrets):
- Low: replace the hardcoded local-dev master key with a random per-process key
  (SecretsCrypto::generate); in-memory stores need no stable key.

Preserves the sealed one-shot grant, ApprovedTxHash binding, broadcast
idempotency, deterministic resume, ship-gate, and openssl-free invariants.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@zmanian

zmanian commented May 26, 2026

Copy link
Copy Markdown
Collaborator Author

Review response — henrypark133 CHANGES_REQUESTED

Pushed 2fd8c946f. All affected-crate checks green: cargo test -p {ironclaw_wallet_external,ironclaw_attested_runtime,ironclaw_secrets,ironclaw_reborn_composition}, cargo fmt --check, cargo clippy --all-features --tests (zero warnings).

Highest-priority (auth-bypass class)

NEAR signature verified against the gate-bound key — GENUINE, fixed. This was real, not stale. The verifier previously checked the ed25519 signature against the callback-supplied payload.public_key while binding only the NEAR account_id. Since a NEAR account holds many access keys, an attacker who knows the (public) account id could supply their own keypair, sign the bound hash with it, echo a valid state (which only bound the account), and pass. The Solana injected provider already binds the signer pubkey (pk_bytes != bound → SignerMismatch, threat #5); NEAR did not. Fixed: the gate-bound identity now carries the expected access-key pubkey (account_id:hex), the proof's public_key must equal it, and the signature is verified against the bound key. New test attacker_supplied_key_is_signer_mismatch fails closed.

NEAR FunctionCall scope fail-closed — fixed. Restored: when the bound operation's receiver is unknown (borsh tx decode not yet available), a restricted FunctionCall key cannot be proven to cover the operation, so it is now refused (ScopeViolation) rather than accepted on the wallet's word. FullAccess remains the supported path until the decode lands.

External-wallet ledger verify-before-advance — fixed. The driver now runs verify_resume before advancing the ledger; a rejected proof leaves the row at Approved (never stranded at Signing, which the one-shot continuation could never re-enter). Test: external_wallet_verify_failure_does_not_strand_ledger_at_signing.

Disposition summary

Finding Disposition
NEAR verifies callback key (High) Fixed — verify against gate-bound key
NEAR FunctionCall scope relaxed (High) Fixed — restored fail-closed
Ext-wallet advances before verify (High) Fixed — verify-before-advance
hex_decode non-ASCII panic ×4 (Critical/Med) Fixed — byte-based decode (near/wc/signer/solana)
encode_*_proof empty-on-fail (Med) Fixed — return Result
proof deser size limit (Med) Fixed — 64 KiB cap
SessionBindingStore poisoned lock (Med) Fixed — recover via into_inner
recover_unknown debug_assert (Med) Fixed — warn! on non-benign error
Hardcoded local-dev master key (Low) Fixed — random per-process key
Orphaned follow-up comment (Low) Fixed — folded into doc block
Missing tests (signer/driver/rebuild/ship_gate) Fixed — added all suggested + more
thiserror migration (Med) Declined — siblings use manual impls; stack-wide follow-up
hex_lower bytes-compare (Med) Declined — crypto-free ironclaw_turns boundary; off hot path
BroadcastDisposition/Outcome unify (Med) Declined — distinct translation seam catches contradictions
New crate AGENTS.md (Med) Declined — 4 direct siblings (attestation/wallet_external/chain_signing/signing_provider) have none; stack-wide follow-up
CustodialSignerLike one-impl trait (Low) Declined — avoids leaking concrete signer generics through the driver
keystore zeroize / binding.rs poison / empty registry Already addressed in prior sweep (a66fbfe / documented fail-closed Option contract / phased-seam) — author replies stand

The load-bearing invariants are preserved: sealed one-shot grant CAS, ApprovedTxHash binding, broadcast idempotency, deterministic resume (no LLM re-entry), ship-gate, TenantId-first isolation, and crypto-free ironclaw_turns.

zmanian added a commit that referenced this pull request May 26, 2026
…<hex> (#3993)

Security-critical: the NEAR signer string is committed into ApprovedTxHash,
so -08 and -10 MUST agree on its format or a proof valid on one branch is
invalid/forgeable on another at the rebase cascade.

Adopt -10's canonical shape (attested-signing-10-reborn-runtime): the expected
ed25519 access-key pubkey is bound INSIDE SigningContext.key_or_account_id as
`account_id:<64-char lowercase-hex 32-byte pubkey>`, parsed by
BoundNearIdentity::parse, and the signature is verified against the BOUND key
(never the callback-supplied one). This is strictly stronger than -08's prior
out-of-band `expected_access_key: Vec<u8>` held on the provider, since the key
is now part of the SigningContext and thus committed into ApprovedTxHash.

near_redirect/{mod,state,verify}.rs are now byte-identical to -10:
- mod.rs: BoundNearIdentity + verify_resume (account+key binding), the
  Result-returning encode_near_redirect_proof, decode size ceiling, and the
  local hex_bytes module — verbatim from -10.
- Removed expected_access_key field + with_expected_access_key constructor
  (the weaker out-of-band path).
- Reverted the -08-only crate::hex_codec extraction (deleted hex_codec.rs;
  restored per-module local hex_bytes/opt_hex_bytes) so the module dedupes
  cleanly against -10, which has no hex_codec.
- Restored NearRedirectState/encode_state/decode_state exports to match -10.

Web ingress (src/channels/web/.../attested.rs):
- verify_near_redirect_proof drops the expected_access_key arg (the key now
  rides in context.key_or_account_id) and constructs via the plain
  NearRedirectSigningProvider::new.
- near_proof_from_input maps the new fallible encode to a 500.
- Tests build the canonical `account_id:<hex>` bound identity; added
  verify_near_redirect_proof_rejects_malformed_bound_identity (caller-level
  fail-closed on a non-canonical, account-only binding).

Tests: tests/near_redirect.rs is byte-identical to -10 (includes
attacker_supplied_key_is_signer_mismatch). New -08-only
tests/near_redirect_binding.rs covers BoundNearIdentity::parse edge cases via
verify_resume (missing ':', empty account, non-hex pubkey, wrong length,
rsplit-on-last-colon), kept separate so the shared file stays identical for
clean dedup.

Verify (IRONCLAW_DISABLE_OS_KEYCHAIN=1): cargo test -p ironclaw_wallet_external
(19 lib + 12 + 5 + 10 integration green), web attested tests green, cargo fmt
--check clean, cargo clippy -p ironclaw_wallet_external --all-features --tests
zero warnings. near_redirect/mod.rs confirmed byte-identical to
attested-signing-10-reborn-runtime.

Cross-ref #3994 (NEAR auth-bypass findings).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@zmanian
zmanian force-pushed the attested-signing-08-near-redirect branch from 2e98c5b to 6da381e Compare May 26, 2026 12:14
zmanian added a commit that referenced this pull request May 26, 2026
…+ signer continuation + ship-gate (attested-signing PR10)

Squashed for stack integration (runtime feat + PR10 review: verify-before-advance,
immutable CAS binding, broadcast-failure recovery + #3994 auth-bypass/DoS hardening).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@zmanian
zmanian force-pushed the attested-signing-10-reborn-runtime branch from 2fd8c94 to b5f0f6f Compare May 26, 2026 12:46
@zmanian
zmanian requested a review from henrypark133 May 26, 2026 14:26
Comment thread crates/ironclaw_secrets/src/crypto.rs
Comment thread crates/ironclaw_attested_runtime/src/port.rs Outdated
/// In-memory [`ResumeGuard`]. A single mutex makes claim atomic.
#[derive(Debug, Default)]
pub struct InMemoryResumeGuard {
claimed: Mutex<HashSet<String>>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Medium — InMemoryResumeGuard and InMemoryAttestedGateBindingStore grow unboundedly; claimed gate_refs and bindings are never evicted. The guard's HashSet<String> accumulates every gate_ref ever resolved (one String per gate, forever). The binding store's HashMap accumulates every binding ever persisted. These are in-memory only and PR12 defers durable backends — but even for a multi-day local-dev session with moderate gate volume this is a process-lifetime leak. The deferred PR12 note covers the durable backend but should explicitly acknowledge that the in-memory stores need a TTL/eviction policy. Suggested: add a // TODO(PR12): these in-memory stores have no eviction; a process-lifetime leak for long-running local-dev sessions. The durable backends in PR12 must include a TTL-based cleanup strategy. comment on both types, and consider a bounded LRU for the guard (old resolved gates are definitionally inert).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Still real — leaving open. Both InMemoryResumeGuard.claimed (Mutex<HashSet<String>>, port.rs:50) and the in-memory binding store grow without bound: a claimed gate_ref is never evicted, so a long-running local runtime accumulates one entry per attested gate forever.

Bounded by what these types are — in-memory dev/test backends; the durable Postgres/libSQL stores land in #3996 and are the production answer. But "the dev backend leaks memory proportional to gates processed" is still a real defect in a runtime that is meant to run for days locally.

The eviction rule needs care rather than a simple cap, which is why I'm not fixing it inline: entries can only be dropped once the corresponding gate is terminal in the ledger, otherwise eviction silently restores replayability — the exact property the guard exists to provide. Tracked in #6532 alongside the durable-store work.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Now fixed for the resume guard, with the safety argument made explicit rather than assumed.

InMemoryResumeGuard is bounded at MAX_CLAIMED_GATES = 8192 with FIFO eviction. The reason eviction is safe here — and the reason I wouldn't have done it blindly — is that the guard is defense in depth, not the primary control: the module doc states the resume guard and the sealed-grant CAS are two independent one-shot controls and either alone fails a replay closed. An evicted gate that is replayed still hits the authoritative sealed-grant CAS in the driver and is refused. That reasoning is now recorded on the constant so a future reader doesn't raise the cap or drop the eviction without understanding what backs it.

Pinned by resume_guard_is_one_shot_and_bounded (one-shot still holds; the set stays bounded past the cap).

The binding store is not bounded — eviction there is only safe once the gate is ledger-terminal, and the store has no ledger access. Leaving that half open; it belongs with the durable stores (#3996).

Comment thread crates/ironclaw_attested_runtime/src/driver.rs
Comment thread crates/ironclaw_attested_runtime/src/driver.rs
@henrypark133

Copy link
Copy Markdown
Collaborator

Architectural note (not a finding) — Gate ownership user_id check (deferred from PR7): RESOLVED by the turns store

The review brief asked to check whether the gate ownership user_id check deferred from PR7 is still unguarded. Having read the full chain:

ironclaw_turns::InMemoryTurnStateStore::resume_turn → prepare_attested_resume → check_resume_preconditions:

if record.scope != request.scope {
    return Err(TurnError::ScopeNotFound);
}
if record.actor != request.actor {
    return Err(TurnError::Unauthorized);
}

TurnActor wraps user_id: UserId. This means:

  • The turn store enforces scope (tenant + thread) AND actor (user_id) equality BEFORE calling the port.
  • A different user cannot supply a ResumeTurnRequest for a gate they did not own — the store rejects it with Unauthorized before the port sees the request.
  • AttestedResumeRequest therefore correctly omits user_id — the turns layer already guaranteed it matches.

Verdict: the deferred gate ownership check is correctly owned by the turns store, not the port. The PR10 port is correctly scoped to crypto verification only. This was architecturally sound and the concern from PR7 is closed.

@henrypark133

Copy link
Copy Markdown
Collaborator

Architectural note — WalletConnect initiate() returns empty directive bytes (live relay wired in PR10/PR11)

WalletConnectSigningProvider::initiate currently returns InitiationOutcome::AwaitingUserAction { directive: Vec::new() }. The comment correctly attributes the live WC v2 session round-trip to PR10/PR11. This is safe for PR9/PR10 scope because:

  1. No production handler currently calls initiate — the WC provider is registered but not yet wired to an HTTP endpoint (that is PR11's /api/chat/gate/resolve job).
  2. The empty directive means a consumer would get no pairing URI, which would surface as a UI-level "nothing to show" rather than a security bypass — the gate stays BlockedAttested.
  3. The verify_resume path (which is what PR10 primarily exercises) is fully implemented and tested.

Recommendation for PR11: the /api/chat/gate/resolve ingress that calls initiate must validate that directive is non-empty before returning it to the client, and should propagate a ProviderUnavailable error if the live relay session cannot be established, rather than silently returning empty bytes.

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

PR10/10 attested-signing terminal PR — Approved with Medium findings

PR10 completes the attested-signing substrate with clean composition-layer glue: ironclaw_attested_runtime (new crate), the production AttestedResumePort, the deterministic AttestedSignerContinuationDriver, and the CustodialMainnetShipGate env-gate. All six lenses reviewed.

Mission isolation / sandbox escape: clean. ironclaw_turns stays crypto-free (dependency direction is attested_runtime → turns, never reverse). The binary-boundary rule is enforced.

Key/grant carry-through: correct. Grant CAS is one-shot, shared store between custodial signer and external-wallet providers, no double-claim path found.

Gate ownership user_id check (PR7 deferred): resolved — turns store enforces record.actor != request.actor (Unauthorized) and record.scope != request.scope before the port is called. Port correctly omits user_id.

Session boundaries: sound. Ledger is one-shot-create per gate_ref; broadcast-before-unknown path documented and tested.

Runtime privilege escalation: the ship-gate (#18) is fail-closed. Custodial mainnet requires both opt-in AND KMS backend; hot-key-only is testnet/dev-only.

Threats #1/#3/#5/#6/#7/#16/#18: all covered by end-to-end tests through the real composition path ("Test Through the Caller" rule honored).

Findings posted inline:

  • Medium (3): unzeroized hex string in SecretsCrypto::generate(); unwrap_or in hex_lower (unreachable but masks intent); InMemoryResumeGuard / binding store unbounded growth (no eviction)
  • Medium (1): BroadcastSubmitted as pre-submit marker — acknowledged in code, deferred Broadcasting state needed in PR12/14
  • Low (1): ProviderRegistry::with_provider silently overwrites duplicate ProviderId
  • Architectural notes (2): gate ownership user_id check confirmed resolved; WalletConnect empty directive recommendation for PR11

No Critical or High findings. Approving.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • staging
  • reborn-integration

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: be0866e9-0b08-4d64-9030-1343eb677338

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

❤️ Share

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

zmanian added 4 commits July 23, 2026 16:09
# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	crates/ironclaw_architecture/tests/attested_signing_boundaries.rs
# Conflicts:
#	Cargo.lock
#	crates/ironclaw_wallet_external/src/lib.rs
@zmanian

zmanian commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #6769, part of consolidating the 20-PR attested-signing-* stack into 8 PRs against current main.

Verified before closing: this branch is an ancestor of the old stack tip, so its content is carried forward in full. The consolidation also re-based everything onto current main — which had moved 79 commits and retired ironclaw_product_workflow, the WebUI composition facade, and the old failure-kind vocabulary — and fixed the CI failures that were red here, including the missing [package.metadata.ironclaw] layer declarations that had been blocking the bottom of the stack.

Closing to keep the queue honest. The review history stays on this PR and remains readable; reopen if the consolidation is rejected.

@zmanian zmanian closed this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: medium Business logic, config, or moderate-risk modules scope: dependencies Dependency updates size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants