Conversation
github-actions
Bot
added
scope: docs
![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_signing_provider crate, establishing a provider-agnostic SigningProvider trait and core data structures for the attested-signing substrate. The implementation maintains a strict purity invariant, verified by a new architecture test, to ensure the crate remains free of chain-specific or cryptographic dependencies. Review feedback points out a security concern where the Deserialize implementation on VerifiedProof could allow bypassing verification logic and suggests using hex encoding for binary types to improve serialization efficiency and observability.
…roof, canonical trust_model Addresses Codex code review of #3960: - boundary test now an allowlist (async-trait/serde/thiserror only) over the resolved normal-deps, not a bypassable denylist - VerifiedProof drops Deserialize (cannot be rehydrated as a verified token off the wire) and binds provider_id + approved_tx_hash - trust_model() is now a default deriving from ProviderId::trust_model() so an impl cannot declare a contradictory trust model (ship-gate guard) - SigningProviderError #[non_exhaustive] - ApprovedTxHash::from_bytes doc hardened (never from caller input) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Attested-signing substrate — stack mapThis PR is the base of a 14-PR stack implementing the attested-signing substrate (human-in-the-loop gated blockchain signing: WYSIWYS binding, sealed one-shot grants, multi-chain custodial + external-wallet, deterministic post-approval continuation). Reviewing bottom-up; each PR targets its predecessor and collapses onto Merge / dependency order (bottom-up)
Status
Known follow-ups (post-collapse, non-blocking for these PRs)
Suggest merging bottom-up starting here. |
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Introduce a provider-agnostic signing trait crate and plan document to establish the foundation for attested-signing secure-channels.
Stats: 6 findings across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability. Reviewers failed: none. Body-only: 0
🛡️ Security
-
High Public VerifiedProof::new constructor allows forging verified proofs (
crates/ironclaw_signing_provider/src/proof.rs:66-76, confidence 100)
The VerifiedProof struct represents a proof that has been cryptographically validated by a SigningProvider. VerifiedProof::new is fully public, allowing bypass of type-level verification. -
High SigningContext allows untrusted deserialization of tenant and user boundaries (
crates/ironclaw_signing_provider/src/context.rs:113-130, confidence 75)
If the resume API deserializes this context directly from untrusted client payloads without validation, it allows IDOR or cross-tenant signature grant claims. -
Medium Public ApprovedTxHash::from_bytes constructor allows caller-supplied hashes (
crates/ironclaw_signing_provider/src/transaction.rs:83-85, confidence 75)
ApprovedTxHash is intended to bind transactions uniquely and should only be computed server-side, but from_bytes allows direct creation from arbitrary bytes.
🧪 Tests
- Medium SigningProof::payload() accessor not tested for all variants (
crates/ironclaw_signing_provider/src/proof.rs:35-42, confidence 100)
The test 'payload_accessor_returns_inner_bytes_for_every_variant' misses WalletConnectProof and NearRedirectProof.
🏠 Local Patterns
- Low Missing publish = false in Cargo.toml (
crates/ironclaw_signing_provider/Cargo.toml:1-10, confidence 100)
All in-workspace crates in this repository set 'publish = false'. The newly added 'ironclaw_signing_provider' is missing this setting.
📐 Maintainability
- Low Redundant suffix in SigningProof enum variants (
crates/ironclaw_signing_provider/src/proof.rs:21-31, confidence 100)
Variants in SigningProof carry redundant 'Proof' or 'AssertionProof' suffixes.
| @@ -0,0 +1,167 @@ | |||
| //! Identity newtypes and the [`SigningContext`] carried through every signing | |||
There was a problem hiding this comment.
High — SigningContext allows untrusted deserialization of tenant and user boundaries.
SigningContext derives Deserialize and exposes public fields for tenant, user, scope, etc. If the resume API or route handler deserializes this context directly from untrusted client payloads without reconciling and enforcing that the authenticated session's tenant/user matches the context's tenant/user, an attacker can perform an IDOR or cross-tenant signature grant claim attack.
Fix: Validate that the authenticated session user/tenant matches the fields in SigningContext before processing any signing operations.
There was a problem hiding this comment.
Declining in this crate — the concern is real but the enforcement point is downstream, not here. ironclaw_signing_provider is the pure trait crate at the base of the stack; it has no session, no authenticated principal, and no auth store to reconcile against, so it cannot enforce that an authenticated session's tenant/user matches a SigningContext. SigningContext must stay Deserialize because it is the wire payload carried across the resume boundary. The session/context reconciliation (rejecting a resume whose context tenant/user does not match the authenticated session — the IDOR/cross-tenant defense) lands in PR5 where ironclaw_turns gains AttestedResumePort and the authenticated principal is in scope; this is documented in the module header reconciliation note (context.rs:4-19). Adding a half-enforcement here with no principal to check against would give a false sense of safety. Tracking the enforcement as a PR5 requirement.
There was a problem hiding this comment.
Still open — not fixed by the cascade, and I don't want to close it silently.
SigningContext does still derive Deserialize with public fields (context.rs:112). What I can say precisely:
- No untrusted wire path deserializes it in this PR. This crate is the trait/type layer; it has no handlers. In the ported stack the context reaches a verifier only via the server-persisted
AttestedGateBinding(ironclaw_attested_runtime), and the round-2 invariant is that the signer is taken from the gate-boundSigningContext.key_or_account_id, never from the transaction body or a caller payload. - Where your concern actually bites is the gate/resolve ingress (feat(signing): reborn webui attested gate/resolve ingress (attested-signing PR11/12) #3995), which is the next hop in the cascade and is not yet rebased. That is the layer that parses caller input into a resolution, and it is also where the cross-user IDOR (
assert_binding_owner) fix lives.
So: leaving this thread open deliberately, and I'll carry it into #3995 as an explicit check — either the ingress never deserializes a caller-supplied SigningContext (and I'll show the code path), or it needs the tenant/user fields sealed. If you'd prefer the type hardened here regardless (e.g. #[serde(deny_unknown_fields)] plus private fields with checked constructors), say so and I'll do it in this PR.
Addresses henrypark133 CHANGES_REQUESTED review on #3960. High (forgery): VerifiedProof derived Deserialize and its public new() took only the proof, so any caller — or any untrusted wire payload — could mint a "verified" trust token without verification ever running. Drop Deserialize (locked in by a compile_fail doctest) so it can never be rehydrated off the wire, and bind provider_id + approved_tx_hash through new() so a verified proof cannot be re-pointed at a different provider or transaction. Construction stays pub because the real verifiers live in the downstream provider/attestation crates (PR4/PR7-9); the seam is now an un-deserializable token only a verifier can mint. Tests: extend payload_accessor coverage to all four SigningProof variants (WalletConnect + NearRedirect were missing) and assert the serialize-binding / non-deserializable invariant. Conventions: add publish = false to Cargo.toml (matches 55/59 workspace crates). Purity invariant preserved: no new dependencies; the architecture dependency-boundary test stays green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
af3140a to
cd7ca66
Compare
Review feedback addressed (commit cd7ca66)Thanks @henrypark133. Summary of dispositions:
Purity invariant preserved (no new deps; architecture dependency-boundary test green). Verified: |
Stack rebased bottom-up onto the hardened attestation APIThe full attested-signing stack has been rebased + recomposed so it stacks cleanly on the #3961 wire-hardening fix, and reviews have been re-requested on every branch. Context for reviewers: Why a rebase (not just review fixes)#3961 (canonical signing-bytes) merged into the
So New branch tips (each on its rebased parent)Base: #4067 wire-hardening
Reviewer notes
|
cd7ca66 to
18daaff
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a provider-agnostic signing abstraction and chain-specific attestation crate for EVM, Solana, and NEAR. It models transactions, derives rendered and canonical projections, reconstructs wire bytes, computes approved hashes, and adds dependency-boundary and anti-field-smuggling tests. ChangesAttested signing substrate
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Attestation
participant SigningProvider
Caller->>Attestation: approved_tx_hash_for(transaction, signer, schema)
Attestation-->>Caller: ApprovedTxHash and RenderedTx
Caller->>SigningProvider: initiate(context, decoded, rendered, approved_hash)
SigningProvider-->>Caller: InitiationOutcome
Caller->>SigningProvider: verify_resume(context, approved_hash, proof)
SigningProvider-->>Caller: VerifiedProof or SigningProviderError
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/ironclaw_attestation/Cargo.toml`:
- Around line 19-35: Rename the test-only feature in the [features] section from
test-internals to the mandated test-support name, and update the self
dev-dependency to enable test-support. Revise the associated manifest comments
to explicitly state that test-support is the approved sole dev-only seam while
production callers must use approved_tx_hash_for.
In `@crates/ironclaw_attestation/src/approved_tx_hash.rs`:
- Around line 100-101: Remove the #[allow(clippy::too_many_arguments)]
attributes from both occurrences associated with compute_approved_tx_hash_inner
and its companion function, since each function has fewer than Clippy’s
threshold. If either suppression is required for another reason, retain it only
with the mandated immediately preceding arch-exempt comment including the
missing aggregation and plan number.
In `@crates/ironclaw_signing_provider/src/context.rs`:
- Around line 6-19: Resolve shared contract ownership across crates: in
crates/ironclaw_signing_provider/src/context.rs lines 6-19, remove the local
GateRef and identity mirrors and import them from their owning
dependency-neutral crate, or relocate the single definitions there; in
crates/ironclaw_signing_provider/src/transaction.rs lines 13-50, define
decoded/rendered transaction contracts at that same owning boundary and stop
exposing provider-local placeholders. Ensure each shared Rust type has exactly
one canonical definition and update references to use it.
In `@docs/plans/2026-05-23-attested-signing-substrate.md`:
- Around line 33-59: Recast the “Load-Bearing Security Invariants (every PR
honors)” section as planned or target invariants until each control is
implemented. Soften present-tense guarantees for the grant, ledger,
continuation, WebAuthn, audit, and KMS items, and link any already-enforced
guarantee to its concrete implementation or test rather than asserting universal
compliance.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6c1fc565-651d-4767-af97-06f283082cde
📒 Files selected for processing (19)
Cargo.tomlcrates/ironclaw_architecture/tests/attested_signing_boundaries.rscrates/ironclaw_attestation/Cargo.tomlcrates/ironclaw_attestation/src/approved_tx_hash.rscrates/ironclaw_attestation/src/canonical.rscrates/ironclaw_attestation/src/decoded_tx.rscrates/ironclaw_attestation/src/fields.rscrates/ironclaw_attestation/src/lib.rscrates/ironclaw_attestation/src/rendered.rscrates/ironclaw_attestation/src/wire.rscrates/ironclaw_attestation/tests/binding.rscrates/ironclaw_signing_provider/Cargo.tomlcrates/ironclaw_signing_provider/src/context.rscrates/ironclaw_signing_provider/src/error.rscrates/ironclaw_signing_provider/src/lib.rscrates/ironclaw_signing_provider/src/proof.rscrates/ironclaw_signing_provider/src/provider.rscrates/ironclaw_signing_provider/src/transaction.rsdocs/plans/2026-05-23-attested-signing-substrate.md
…ning PR1, squashed for cascade)
18daaff to
062ebcf
Compare
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 `@crates/ironclaw_attestation/src/decoded_tx.rs`:
- Around line 286-299: Extend NearAction::Delegate with the NEP-366 inner
actions payload, using a non-recursive action representation where needed to
avoid recursive type issues. Update fields.rs::project_near_action and
wire.rs::push_near_action to preserve and encode the delegated actions, and
adjust borrow-limited tests to verify that differing inner action lists produce
distinct commitments.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d1258e85-72ce-4d3e-88a2-28a7d6853937
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (19)
Cargo.tomlcrates/ironclaw_architecture/tests/attested_signing_boundaries.rscrates/ironclaw_attestation/Cargo.tomlcrates/ironclaw_attestation/src/approved_tx_hash.rscrates/ironclaw_attestation/src/canonical.rscrates/ironclaw_attestation/src/decoded_tx.rscrates/ironclaw_attestation/src/fields.rscrates/ironclaw_attestation/src/lib.rscrates/ironclaw_attestation/src/rendered.rscrates/ironclaw_attestation/src/wire.rscrates/ironclaw_attestation/tests/binding.rscrates/ironclaw_signing_provider/Cargo.tomlcrates/ironclaw_signing_provider/src/context.rscrates/ironclaw_signing_provider/src/error.rscrates/ironclaw_signing_provider/src/lib.rscrates/ironclaw_signing_provider/src/proof.rscrates/ironclaw_signing_provider/src/provider.rscrates/ironclaw_signing_provider/src/transaction.rsdocs/plans/2026-05-23-attested-signing-substrate.md
| /// Meta-transaction delegate action (NEP-366). Carries the inner action's | ||
| /// fields needed for the signed commitment. | ||
| Delegate { | ||
| /// The account whose key signs the delegate action. | ||
| sender_id: String, | ||
| /// The receiver of the delegated actions. | ||
| receiver_id: String, | ||
| /// Nonce for the delegate action. | ||
| nonce: u64, | ||
| /// Block height past which the delegate action is invalid. | ||
| max_block_height: u64, | ||
| /// Public key authorizing the delegate action. | ||
| public_key: NearPublicKey, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
NEP-366 NEAR DelegateAction borsh fields sender_id receiver_id actions nonce max_block_height public_key
💡 Result:
The DelegateAction structure, introduced by NEP-366 for meta-transactions on the NEAR Protocol, consists of the following fields, which are serialized using Borsh (Binary Object Representation Serializer for Hashing): 1. sender_id (AccountId): The account ID of the signer who initiated the delegated actions [1][2]. 2. receiver_id (AccountId): The account ID where the delegated actions will be executed [1][2]. 3. actions (Vec): A list of actions to be executed. To prevent recursive nesting, this uses the NonDelegateAction type [2][3]. 4. nonce (Nonce): A 64-bit integer used to prevent replay attacks; it must match the expected nonce for the provided public key and is incremented after processing [1][2]. 5. max_block_height (BlockHeight): A 64-bit integer representing the maximum block height at which the DelegateAction remains valid [1][2]. 6. public_key (PublicKey): The public key used to verify the signature of the delegated action [1][2]. In practice, the DelegateAction is typically wrapped in a SignedDelegateAction, which includes the DelegateAction itself and the corresponding signature [1][2]. Additionally, when serializing for the network, a DelegateActionPrefix is often required (per NEP-461) to prevent hash collisions before the encoded DelegateAction bytes [4].
Citations:
- 1: https://github.com/near/NEPs/blob/master/neps/nep-0366.md
- 2: https://near.github.io/nearcore/RuntimeSpec/Actions.html
- 3: https://docs.rs/near-primitives/latest/src/near_primitives/action/delegate.rs.html
- 4: Lumi Architecture Summary: Feedback for schema.ts near/near-api-js#1934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files 'crates/ironclaw_attestation/src/decoded_tx.rs'
echo "== relevant decoded_tx.rs sections =="
sed -n '240,370p' crates/ironclaw_attestation/src/decoded_tx.rs
echo "== project_near_action references =="
rg -n "project_near_action|push_near_action|NearAction::Delegate|ActionsHash|ApprovedTxHash|delegate" crates/ironclaw_attestation/src crates -g '*.rs' -C 3Repository: nearai/ironclaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== decoded_tx.rs imports/types around delegate =="
sed -n '1,180p' crates/ironclaw_attestation/src/decoded_tx.rs
echo "== fields.rs excerpt =="
sed -n '1,260p' crates/ironclaw_attestation/src/fields.rs
echo "== wire.rs delegate excerpt =="
sed -n '130,260p' crates/ironclaw_attestation/src/wire.rs
echo "== exact Delegate/NonDelegate action test names =="
rg -n "delegate|Delegate|NonDelegate|DelegateActionPrefix|DelegateAction|near_distinct_actions_hash_differently|actions_hash" crates/ironclaw_attestation crates/ironclaw_signing_provider -g '*.rs' -C 4Repository: nearai/ironclaw
Length of output: 30649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== binding test delegate section =="
sed -n '539,600p' crates/ironclaw_attestation/tests/binding.rs
echo "== inspect all NearAction::Delegate occurrences in tests =="
rg -n "NearAction::Delegate|Delegate \\{|\\.Delegate" crates/ironclaw_attestation/tests crates/ironclaw_attestation/src -g '*.rs' -C 3
echo "== behavioral probe: current variant projection equality for two distinct inner action sets =="
python3 - <<'PY'
from pathlib import Path
text = Path("crates/ironclaw_attestation/src/fields.rs").read_text()
start = text.index("NearAction::Delegate {")
end = text.index(" }", start)
print(text[start:end+5])
print("Delegate arm contains 'actions' or 'action':", "actions" in text[start:end+5] or "action" in text[start:end+5])
PYRepository: nearai/ironclaw
Length of output: 5313
Model and bind NEP-366 Delegate actions.
NEP-366 signed DelegateAction includes actions; NearAction::Delegate currently stores only the delegate metadata, and both fields.rs::project_near_action and wire.rs::push_near_action ignore any inner actions. Add the delegated action payload, update the borrow-limited tests to distinguish different inner actions, and use a non-recursive action type where appropriate so meta-tx approval cannot collapse distinct delegated action lists into one hash.
🤖 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 `@crates/ironclaw_attestation/src/decoded_tx.rs` around lines 286 - 299, Extend
NearAction::Delegate with the NEP-366 inner actions payload, using a
non-recursive action representation where needed to avoid recursive type issues.
Update fields.rs::project_near_action and wire.rs::push_near_action to preserve
and encode the delegated actions, and adjust borrow-limited tests to verify that
differing inner action lists produce distinct commitments.
|
Superseded by #6748, 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. |
2 participants
What this PR is
ironclaw_signing_provider— the provider-agnosticSigningProvidertrait crate (no chain deps) — plusironclaw_attestation's canonical signing-bytes /ApprovedTxHashcore, and theattested_signing_boundariesarchitecture test that pins crate purity and the openssl-free workspace graph.Cascade changes
main's + additive, not a 3-way merge. The naive merge silently took this branch's stale[workspace.package]and droppedmain'srust-version, breaking workspace resolution. Members =main's current list + only the two crates this PR creates (the branch's own list still namedironclaw_loop_supportandironclaw_reborn, both since deleted frommain).Verification
Workspace resolves; both crates build; 43 crate tests pass;
workspace_graph_is_openssl_freesurvives the grown workspace (this was the main risk for hop 1); clippy clean.