feat: reject update calls when sender_info restricts permissions to queries#10447
Closed
aterga wants to merge 4 commits into
Closed
feat: reject update calls when sender_info restricts permissions to queries#10447aterga wants to merge 4 commits into
aterga wants to merge 4 commits into
Conversation
aterga
commented
Jun 11, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a protocol-level restriction on sender_info for update calls: if a canister-signed sender_info.info decodes as an ICRC-3 map with implicit:permissions = "queries", the replica rejects /call requests during ingress validation (while leaving query/read_state validation behavior unchanged).
Changes:
- Add Candid-decoding of
sender_info.infoinic-validatorto extractimplicit:permissionsand reject update calls when it equals"queries". - Introduce a new validation error
UpdateCallNotPermittedBySenderInfoand plumb it throughic-validator-ingress-message. - Add unit + integration tests and update Cargo/Bazel dependencies needed for Candid decoding in the validator and for encoding test payloads.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rs/validator/src/ingress_validation.rs | Add permission extraction/decision logic and new validation error for update calls. |
| rs/validator/src/ingress_validation/tests.rs | Add unit tests covering permission extraction and fail-open behavior. |
| rs/validator/ingress_message/tests/validate_request.rs | Add end-to-end tests verifying update rejection vs query acceptance with permissions. |
| rs/validator/ingress_message/src/lib.rs | Add new public error variant and Display message in the ingress-message wrapper crate. |
| rs/validator/ingress_message/src/internal/mod.rs | Map the new ic_validator error into the wrapper crate error. |
| rs/validator/ingress_message/Cargo.toml | Add candid dev-dependency for encoding test payloads. |
| rs/validator/ingress_message/BUILD.bazel | Add Bazel test dependency on candid. |
| rs/validator/Cargo.toml | Add candid, serde, and serde_bytes dependencies needed for decoding in validator. |
| rs/validator/BUILD.bazel | Add Bazel deps for candid, serde, and serde_bytes to the validator library. |
| Cargo.lock | Record new dependency edges for candid/serde_bytes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aterga
commented
Jun 11, 2026
| "//rs/crypto/tree_hash", | ||
| "//rs/limits", | ||
| "//rs/types/types", | ||
| "@crate_index//:candid", |
Contributor
Author
There was a problem hiding this comment.
Note to reviewer.
If you dislike adding candid as a dependency here, we can provide a non-candid endpoint in II to serve a different encoding of sender_info - please indicate your preferred encoding to keep this thread actionable.
This was referenced Jun 11, 2026
…ueries When an update call carries a sender_info whose info blob is a Candid-encoded ICRC-3 map (the format used by Internet Identity to certify request attributes, signed under the "ic-sender-info" domain separator) and the map contains the "implicit:permissions" attribute with the text value "queries", the ingress validator now rejects the call with the new UpdateCallNotPermittedBySenderInfo error. Query calls and all other cases (no sender info, unparseable info blob, absent attribute, or any other attribute value) remain permitted. This enables Internet Identity users to sign limited delegations that can only read on their behalf (call query methods) but cannot change state (call update methods), with the restriction enforced by the protocol rather than by each individual canister. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
Import BTreeMap and CandidType instead of qualifying them in the sender_info integration tests, and avoid cloning the permissions string in sender_info_permissions by removing the entry from the owned map. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
Extends the ingress verification system test suite with a test that demonstrates the implicit:permissions sender_info attribute end-to-end: a signer canister certifies an ICRC-3 attributes map restricting the sender to "queries", after which the protocol rejects the sender's update calls during ingress validation while query calls remain permitted (and the receiving canister can read the certified attributes via the msg_caller_info system API). Attribute maps with permissions "updates", or without the permissions attribute, leave update calls permitted. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
Adds e2e tests for the combined design: a delegation with sender_info_required = true forces update calls to carry the certified session attributes, whose implicit:permissions attribute then determines whether update calls are permitted. Covers rejection with queries-only attributes, rejection when omitting the attributes, and acceptance of queries and of update calls with updates-permissions. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
aterga
force-pushed
the
claude/fervent-mccarthy-tmfzpw
branch
from
535eb25 to
0c5ce91
Compare
aterga
changed the base branch from
master
to
claude/fervent-mccarthy-tmfzpw-sender-info-required
Contributor
Author
|
Closing in favor of the more lightweight approach in #10449 (delegations carry the Generated by Claude Code |
pull Bot
pushed a commit
to mikeyhodl/ic
that referenced
this pull request
Jun 24, 2026
…ns (dfinity#10449) # Queries-only permissions on delegations **This is the consolidated read-only-sessions feature** (the previously stacked alternatives dfinity#10447/dfinity#10452, which carried the restriction in certified `sender_info` attributes, were closed in favor of this more lightweight approach). ## What it does Request delegations gain an optional `permissions` field, mirroring the existing `targets` field. It is modeled as a typed enum `DelegationPermissions` whose variants are encoded on the wire (and in the representation-independent hash) as text: * `permissions` **absent** or **`"all"`** (`DelegationPermissions::All`) → the sender can execute all functions — queries, replicated queries, and updates (today's semantics; byte-for-byte identical hash for existing delegations without the field). * **`"queries"`** (`DelegationPermissions::Queries`) → the sender can only execute queries: requests to `/call` endpoints (updates *and* replicated queries) authenticated through such a chain are rejected during ingress validation with the new `RequestValidationError::UpdateCallNotPermittedByDelegation`; query and `read_state` requests remain permitted. * **Any other value** → because the field is a closed enum, an unsupported value fails to deserialize, so the request is **rejected when it is decoded** (before validation). There is no dedicated validator error for it. A restriction anywhere in the chain applies to the whole chain (like `targets`, restrictions only accumulate). The matching interface-spec change (dfinity/developer-docs#292) models the field as the union `Queries | All | Unrestricted`, which this enum mirrors exactly. ## Why this can't be bypassed, and why it's backward compatible * The field is part of the delegation's representation-independent hash, i.e. covered by the delegation signature. A dapp holding the session key **cannot strip the field** — doing so changes the hash and invalidates the signature. * **Old replicas fail closed**: they parse the delegation without the unknown field (serde ignores unknown map keys), recompute the hash without it, and signature verification fails. A restricted delegation is unusable on un-upgraded replicas rather than silently unrestricted. Rollout: replicas first, issuers (Internet Identity) after. * Delegations without the field are completely unaffected, so existing agents, dapps, and issuers need no changes. Agents only need to round-trip the new field once they want to carry restricted delegations; an agent that drops it fails closed. * The enforcement point is `HttpRequestVerifier<SignedIngressContent>` — the same verifier used by the ingress manager for block validation, so a malicious boundary node or replica cannot smuggle a restricted update call into a block. ## Rationale Enables issuers like Internet Identity to sign **read-only delegations**: sessions that can read on the user's behalf (query calls) but cannot change state (update calls), enforced by the protocol regardless of the client's cooperation. II sets this field from a "Read-only mode" checkbox in the authorize flow (II-side changes prepared separately). Historical precedent: the interface spec already extended the delegation map with an optional restriction field once — the `senders` field (added Dec 2021, never implemented by the replica, removed in [interface-spec#246](dfinity/interface-spec#246)). This PR follows the same formal pattern (`verify_delegations` accumulation), but implementation-first. ## Changes * `ic-types`: typed `DelegationPermissions { Queries, All }` enum (serialized as `"queries"`/`"all"`); optional `Delegation.permissions` field included in the signed bytes (`hash_of_map` key `"permissions"`); chainable `with_targets`/`with_permissions` builder methods (replacing the former `new_with_targets`/`new_with_permissions` constructors so a delegation can carry both). * `ic-validator`: `DelegationRestrictions` (targets + queries-only) threaded through delegation-chain validation; update-call rejection via `RequestValidationError::UpdateCallNotPermittedByDelegation`. The `/call` path uses the shared `validate_request_content` (no special-casing) and checks the restriction afterwards. * `ic-validator-ingress-message`: the new error variant mirrored. * `ic-validator-http-request-test-utils`: `delegate_to_with_permissions` chain-builder support. * Tests: signed-bytes golden test for the new field; CBOR encoding + round-trip tests; a decode-rejection test (`delegation_permissions_rejects_unsupported_value`) covering unsupported values including case/whitespace variants; validator-level e2e tests (update-rejected / query-and-read_state-permitted / `"all"`-permitted / restriction-mid-chain / combined targets+permissions); and a system test (`requests_with_delegation_permissions` in `rs/tests/crypto/ingress_verification_test.rs`, tagged `long_test`) exercising the full HTTP path against a real replica. ## Caveats / follow-ups * Calls to query methods submitted via the `/call` path (replicated queries) are rejected for `"queries"` delegations by design — the `"all"` vocabulary makes this explicit. * Interface-spec change in dfinity/developer-docs#292; II-side issuance and agent (`@icp-sdk/core`) round-tripping of the field prepared/pending. `cargo test -p ic-types -p ic-validator -p ic-validator-ingress-message` passes and clippy is clean; downstream consumers (`ic-http-endpoints-public`, `ic-ingress-manager`, `ic-canister-client`, `ic-state-machine-tests`) and both affected system-test binaries compile. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Queries-only permission for
sender_info— combined read-only-session designStacked on #10452 (delegations can require a signed
sender_info): this PR is now rebased on top of that branch, and together they form the complete, protocol-enforced read-only-session design. The base of this PR is set to the #10452 branch so the diff shows only the attribute-checking part; GitHub will retarget it tomasterautomatically when #10452 merges.The combined design
sender_info_required = true. The field is covered by the delegation signature, so the dapp holding the session key cannot strip it; update calls without asender_infoare rejected (SenderInfoRequiredByDelegation).sender_infowhoseinfoblob is a Candid-encoded ICRC-3 map (the format II certifies under theic-sender-infodomain separator) with the attributeimplicit:permissions = "queries", the call is rejected (UpdateCallNotPermittedBySenderInfo). Query calls are unaffected.Together: a read-only session must present its certified attributes on every update call (1), and those attributes forbid update calls (2) — so the restriction holds regardless of client cooperation, while all permission semantics stay in the extensible attribute map rather than the delegation format. The new
read_only_sessionse2e test module inrs/validator/ingress_message/tests/validate_request.rsdemonstrates the full story: update with queries-only attributes → rejected; update omitting attributes → rejected; queries → permitted; update with"updates"attributes → permitted.Design notes (attribute checking, this PR)
HttpRequestVerifier<SignedIngressContent>::validate_request— the same layer that enforces delegationtargets; the ingress manager uses the same verifier for payload validation, so the restriction also holds during block validation.implicit:permissions(single colon), following II's existingimplicit:nonce/implicit:originconvention. The attribute parsing only runs after the canister-signature verification of the blob succeeded, with bounded Candid decoding quotas."queries"restricts. Note the presence requirement of feat: delegations can require calls to carry a signed sender_info #10452 and the delegation field itself are fail-closed./callare also rejected (the validator cannot know method kinds); restricted callers use the query path.Valuetype is mirrored minimally insideic-validatorto avoid pulling ledger packages into the consensus-critical validator.Tests
sender_info, plus the combinedread_only_sessionsmodule described above.requests_with_sender_info_permissionsinrs/tests/crypto/ingress_verification_test.rs) demonstrating attribute enforcement against a real replica across all/callAPI versions.Related
read_only→ attribute issuance / delegation bit) are prepared as patches pending repo access.cargo test -p ic-validator -p ic-validator-ingress-message -p ic-typespasses on the stack, clippy is clean, and the system test binary compiles. Bazel was not available in the development container, so CI should confirm the Bazel build.https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz