feat: reject update calls for delegations with queries-only permissions#10449
Conversation
Adds an optional permissions field to request delegations, mirroring the existing targets field. When a delegation in a request's delegation chain carries permissions = "queries", the ingress validator rejects update calls authenticated through that chain with the new UpdateCallNotPermittedByDelegation error, while query and read_state requests remain permitted. Delegations with permissions = "updates" or without the field are unrestricted, and any other value is rejected as unsupported (fail-closed, since no pre-existing delegations carry the field). The field is covered by the delegation signature (it is part of the representation-independent hash), so a client cannot strip it to lift the restriction: removing the field changes the delegation hash and invalidates the signature. For the same reason, replicas that predate this change fail closed on restricted delegations. This enables issuers like Internet Identity to sign read-only delegations that can read on the user's behalf (perform query calls) but cannot change state (perform update calls), enforced by the protocol regardless of the client's cooperation. It is the strictly-enforced counterpart to the sender_info-based attribute approach (#10447). https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
There was a problem hiding this comment.
Pull request overview
This PR extends IC request delegations with an optional permissions field to support queries-only delegations that are enforced during ingress validation (rejecting /call update requests), while remaining backward compatible for existing delegations and failing closed on unsupported permission values.
Changes:
- Add
Delegation.permissionstoic-types, include it in the delegation’s signed bytes, and expose constructors/accessors. - Thread delegation permission restrictions through
ic-validatordelegation-chain validation and reject update calls when any delegation in the chain haspermissions = "queries". - Add test utilities and end-to-end tests covering acceptance/rejection behavior across request types and invalid permission values.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/validator/tests/ingress_validation.rs | Adds a golden signed-bytes test covering the new permissions field hashing. |
| rs/validator/src/ingress_validation/tests.rs | Updates unit tests to match the new validate_signature return type (restrictions vs targets). |
| rs/validator/src/ingress_validation.rs | Implements delegation permissions parsing, restriction accumulation, and update-call rejection. |
| rs/validator/ingress_message/tests/validate_request.rs | Adds e2e tests verifying update rejection and query/read_state acceptance for queries-only delegations. |
| rs/validator/ingress_message/src/lib.rs | Exposes the new validation/authentication error variants in the standalone validator API. |
| rs/validator/ingress_message/src/internal/mod.rs | Maps new ic_validator error variants into ic_validator_ingress_message equivalents. |
| rs/validator/http_request_test_utils/src/lib.rs | Extends delegation-chain builder utilities to create delegations with permissions. |
| rs/types/types/src/messages/http/tests.rs | Updates CBOR serialization tests to include the new permissions field (null or string). |
| rs/types/types/src/messages/http.rs | Adds Delegation.permissions, constructor/accessor, and includes it in signed-bytes hashing when present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Short-circuit the queries-only delegation rejection before sender_info canister signature verification on the call path, make the error message byte-identical across the validator crates, and strengthen test assertions to also check queries_only. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
The supported values of a delegation's permissions field are now: - "queries": the sender can only execute queries; requests to /call endpoints (updates and replicated queries) are rejected. - "all": the sender can execute all functions (queries, replicated queries, and updates); same as an absent permissions field. Any other value, including the previous "updates", is rejected as unsupported. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
|
✅ No security or compliance issues detected. Reviewed everything up to 937ddbe. Security Overview
Detected Code Changes
|
Adds the test coverage identified as missing: 1. System test (rs/tests/crypto/ingress_verification_test.rs): requests_with_delegation_permissions exercises the full HTTP path against a real replica — "queries" rejects update calls (across all /call API versions) while permitting query and read_state, "all" is accepted, and unsupported values (incl. "updates") are rejected for all request kinds; covers the whole-chain semantics. 2. CBOR wire round-trip (ic-types http tests): confirms the permissions field survives serialize/deserialize and that a delegation encoded without the field deserializes to None (backward compatibility). 3. Combined targets + permissions CBOR encoding case. 4. Lower-level validate_delegations unit tests (ic-validator) asserting queries_only is set for "queries", unset for "all"/absent, and that unsupported values (incl. "updates") yield UnsupportedDelegationPermissions. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
- Replace Delegation::new_with_targets/new_with_permissions with chainable with_targets/with_permissions builder methods, so a delegation can carry both targets and permissions. - Remove the call-path duplication of validate_request_content: the queries-only rejection now happens after the shared validation (the earlier short-circuit-before-sender_info did not actually prevent DoS, since the query/read_state endpoints validate sender_info too). - Clarify docs: an update call or replicated query submitted to /call is rejected by a queries-only delegation. - Tests: add case/whitespace unsupported-permission variants; add an e2e test combining delegate_to_with_targets and delegate_to_with_permissions in one chain; reword the misleading "previously-used" comment. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
Per the settled interface-spec model (dfinity/developer-docs#292, which defines the field as the union `Queries | All | Unrestricted`), replace the `Option<String>` `permissions` field with a typed `Option<DelegationPermissions>` enum whose `Queries`/`All` variants are encoded on the wire (and in the representation-independent hash) as the text `"queries"`/"all"". Consequences: - Unsupported values are now rejected when the request is decoded (the field is a closed enum), so the runtime AuthenticationError::UnsupportedDelegationPermissions check in validate_delegations is gone, along with the error variant in both the validator and ingress-message crates. - The "unsupported value rejected" coverage moves to a CBOR-decoding test in ic-types (delegation_permissions_rejects_unsupported_value), which also covers case/whitespace variants of the supported values. - The validator/e2e/system tests that previously constructed invalid string permissions are removed or switched to the typed enum. https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
pierugo-dfinity
left a comment
There was a problem hiding this comment.
Mostly me ranting about Claude's comments
…ents Trim verbose doc comments on DelegationPermissions and the Delegation builders, drop the broken interface-spec anchor, and clarify the system-test comments per reviewer feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
|
Thanks @pierugo-dfinity — addressed all of these in 0ecc183:
Generated by Claude Code |
Point the doc comment at the post-migration URL with a valid `#authentication` anchor, per reviewer feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQNgPJKgxWohrJ6owwJBzz
# Motivation Bump the PocketIC test harness to v15 — the first version whose bundled replica implements the request-delegation `permissions` extension (dfinity/ic#10449). This is a prerequisite for adding integration tests that exercise **read-only (queries-only) delegation enforcement** end-to-end (follow-up to #4016). Landing the bump on its own keeps that dependency/infra change reviewable in isolation, separate from the feature work. # Changes - **`Cargo.toml`**: `pocket-ic` `"9.0.2"` → `"15"`. This transitively bumps `candid` 0.10.20→0.10.31, `ic-management-canister-types` 0.3.3→0.5.0, and `ic-transport-types` 0.40→0.45 across the workspace (i.e. it touches the II canister's own dependency tree). The canister and the test framework compile with **no code changes**. - **`demos/vc_issuer/Cargo.toml`**: same `pocket-ic` `"9.0.2"` → `"15"` bump (it pins pocket-ic directly *and* path-depends on `canister_tests`, so it must move in lockstep to avoid a two-version collision), plus the `Cargo.lock` refresh. - **`scripts/test-canisters.sh`**: `POCKET_IC_SERVER_VERSION` 9.0.3 → 15.0.0 (same release-asset name; download mechanism unchanged). - **`.github/workflows/canister-tests.yml`**: the two `dfinity/pocketic` action pins 9.0.3 → 15.0.0. - **Test adaptations for v15's cycle-funding behavior** (v15 funds freshly-created canisters with a default balance; no production code is affected): - `archive::should_expose_status`: assert the status exposes a funded balance (`> 0`) instead of a hard-coded `0`. - `should_deploy_archive_with_cycles`: assert the *delta* spent on archive creation (exactly `canister_creation_cycles_cost`) rather than an absolute, base-dependent post-deploy balance. - `icrc3_test_vectors`: also strip the environment-dependent top-level `root_key_hex` and `canister_sig_pk_hex` (the latter embeds the PocketIC-assigned canister id) before comparing. The deterministic `message_hex` / `signed_message_hex` bytes are byte-identical — no II-side change. The E2E Playwright launcher pin (`icp-cli-network-launcher` v12, see the in-file comment about dfinity/ic#10226) is intentionally **left untouched** — that's a separate pocket-ic build used only by the browser suite. # Tests - `cargo check` across `internet_identity` + `canister_tests` (incl. tests): clean, no porting needed. - Full `delegation` integration suite (**37 tests**) passes locally against `pocket-ic-server 15.0.0`. - The three adapted tests above pass locally against `pocket-ic-server 15.0.0`. - CI runs the complete canister-tests suite on v15. > Note for the follow-up enforcement tests: round-tripping an II canister-signature delegation through the replica's ingress validation may interact with dfinity/ic#10226 (strict `subnet_type` cert check). That will be verified when the enforcement-test harness (`make_live` + `ic-agent`) is added. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018gHRjyFYqumMxAMwbPRLuN --- _Generated by [Claude Code](https://claude.ai/code/session_018gHRjyFYqumMxAMwbPRLuN)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Round-trip II-issued delegations through a live PocketIC endpoint (`make_live`) with real signed request envelopes, exercising the replica's actual ingress validation (which the impersonating PocketIC test transport skips). PocketIC v15's replica implements the queries-only `permissions` extension (dfinity/ic#10449), so the enforcement matrix is now testable: - a queries-only delegation authenticates query calls end-to-end and executes as the delegated principal; - a queries-only delegation is rejected AT INGRESS for update calls with the replica's enforcement error ("Update calls are not permitted: a delegation restricts the sender to query calls"); - an unrestricted delegation authenticates both query and update calls (also confirming dfinity/ic#10226's strict `subnet_type` cert check does not reject pocket-ic's own canister-sig delegations); - fail closed: stripping the `permissions` field from a read-only delegation's envelope is rejected as an invalid signature; the restriction cannot be dropped to escalate access. Covered for both mint paths: account delegations (serving /continue and /cli) and MCP per-app delegations. The envelopes are hand-rolled (CBOR + ed25519 + the interface spec's representation-independent hash) because no released ic-agent / ic-transport-types release (checked through 0.47.3) can carry the delegation `permissions` field yet. New dev-dependencies: ed25519-dalek (real session keys) and reqwest (submitting the envelopes).
…ions (#4016) # Motivation Let a user grant **read-only (query-only) access** when authorizing a relying party — a dapp (`/continue`), the CLI (`/cli`), or an MCP server (`/mcp`). A read-only authorization can read on the user's behalf but cannot change state: it relies on the optional `permissions = "queries"` field on request delegations (interface-spec extension drafted in dfinity/ic#10449), which the IC enforces at the **protocol level** by rejecting update calls (`/call` requests) authenticated through such a delegation. Enforcement is not up to any app or canister. # Changes **Delegation signing & types (backend)** - `Delegation` gains an optional `permissions` field (`opt text` in candid; `Option` in the Rust interface). `"queries"` restricts the delegation to query calls; absent means unrestricted (`"all"`), per the spec. - `add_delegation_signature` folds an optional `permissions` value into the representation-independent hash, so the value is covered by the canister signature (byte-identical to the upstream helper when `permissions` is absent). - A new candid `type Permissions = variant { queries; all }` (mirroring the ICP protocol's request-delegation permission values) is the request-side knob. `prepare_account_delegation` / `get_account_delegation` gain a trailing `permissions : opt Permissions`; `queries` yields a delegation carrying `permissions = "queries"`. An **omitted** arg (or `all`) means **unrestricted**, preserving the original argument-count behavior (see "Backwards compatibility"). Internally the wire enum maps to a `DelegationAccess` domain enum at the boundary (`DelegationAccess::from`), so no blind booleans cross a function signature. - Regenerated candid bindings (`internet_identity_idl.js`, `internet_identity_types.d.ts`). **Reusable UI + the three sign-in flows (frontend)** - New `AccessLevelToggle.svelte` (checkbox + tooltip, bindable `accessLevel`, `prompt` prop selecting which level the checkbox opts into), used in all three flows. Per-flow defaults: - `/continue`: **full access** by default ("Read-only mode" opt-in). - `/cli`: **read-only** by default ("Full access" opt-in). - `/mcp`: **read-only** by default ("Full access" opt-in). - A typed `AccessLevel = "read-only" | "full-access"` union threads through `authorizationStore` and the flows; the candid `permissions : opt Permissions` argument (`{ queries }` / `{ all }`) is produced only at the actor-call boundary (`toPermissionsArg`). All first-party callers send an explicit value. **MCP read-only (backend)** - `mcp_set_access` gains `permissions : opt Permissions`, persisted with the grant (as a bool). The MCP server's **standing delegation stays full-access** (required so it can still call the update endpoint `mcp_prepare_account_delegation`); the read-only restriction is applied to the **per-app delegations** the server mints, by reading the persisted flag. - Storage unified: the `Principal → AnchorNumber` reverse index and the read-only flag collapse into one `Principal → StorableMcpAccess { anchor_number, read_only }` map. `caller_access()` returns both in a single lookup, so the MCP server still never passes an anchor number. On a fresh memory index — MCP is preview-only, so the old regions are abandoned and any preview grants are recreated on the next connect (no migration). **Backwards compatibility:** the `permissions` argument is optional and an *omitted* value (or `all`) yields an unrestricted delegation, so existing callers of the pre-feature argument form keep getting update-capable delegations (no silent downgrade), matching the interface spec's default for an absent `permissions` field. Read-only is opt-in via the argument; the per-flow UI defaults above are independent of this (always sent explicitly). **Note on relying parties:** returning a restricted delegation to a dapp requires the agent library to round-trip the `permissions` field, and no released agent stack does yet (checked `ic-agent` / `ic-transport-types` through 0.47.3, and `@icp-sdk/core`). Until then a restricted delegation **fails closed** on the dapp side — its signature doesn't verify without the field, which the ingress tests below prove at the replica. # Tests - **End-to-end ingress enforcement** (`delegation_ingress::`, new): real signed request envelopes submitted to a live PocketIC endpoint (`make_live`), exercising the replica's actual request authentication — which the impersonating PocketIC test transport skips. The envelopes are hand-rolled (CBOR + ed25519 + representation-independent hash) since no released agent can carry `permissions`. Asserted for both mint paths (account delegations, serving `/continue` and `/cli`; MCP per-app delegations): - a queries-only delegation authenticates **query** calls and executes as the delegated principal; - a queries-only delegation is **rejected at ingress for update** calls with the protocol error ("Update calls are not permitted: a delegation restricts the sender to query calls") — before any canister code runs; - an unrestricted delegation authenticates both query and update calls (also confirming dfinity/ic#10226's `subnet_type` cert check doesn't affect pocket-ic v15's canister-sig delegations); - **fail closed**: resubmitting a queries-only delegation with the `permissions` field stripped from the envelope is rejected as an invalid signature — omitting the restriction cannot escalate access. - **Persistence & format (backend units):** `mcp_read_only_grant_persists_across_upgrade` (a read-only grant still mints queries-only after a canister upgrade); `StorableMcpAccess` round-trip + pinned wire-format; a pinned signable-hash vector for the `permissions = "queries"` delegation, plus a check that the absent-permissions message is byte-identical to the upstream helper. - **Delegation mint semantics (impersonating transport):** read-only account delegation carries `permissions = "queries"` and the signature binds to it (an unrestricted lookup yields `NoSuchDelegation`); an omitted arg yields unrestricted; read-only / full-access MCP grants mint queries-only / unrestricted per-app delegations respectively. `verify_delegation` folds both `permissions` and `targets` into the representation-independent hash. - **Frontend units:** `toPermissionsArg` mapping (`read-only → { queries }`, `full-access → { all }`); the `AccessLevelToggle` prompt×checked mapping (all combinations); `authorizationStore` records the access level; the `/cli` and `/mcp` authorize helpers send the correct `permissions` variant and keep the MCP standing delegation full-access. Plus e2e assertions that each flow's checkbox starts in its documented default state. - The `accounts::`, `mcp::`, `session_delegation::`, and `delegation_ingress::` integration suites pass against the built wasm (on `pocket-ic-server 15.0.0`). - Static checks clean: `cargo clippy --all-targets`, `tsc --noEmit` / `svelte-check` (0 errors), `eslint`, `prettier`, and the candid interface check. > Bindings note: the pinned `didc` release used by `npm run generate` is currently unreachable from this environment, so the regenerated `internet_identity_idl.js` / `internet_identity_types.d.ts` were produced by hand and validated (tsc + parsing the idl); a `npm run generate` pass on `main` will confirm they match canonically. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018gHRjyFYqumMxAMwbPRLuN --------- Co-authored-by: Claude <noreply@anthropic.com>
## Summary Extends the IC interface spec with the optional `permissions` field of request delegations, as drafted in the replica implementation in dfinity/ic#10449. - **`https-interface.md` (Authentication):** documents the new `permissions` field of the delegation map. `"queries"` restricts the delegation to query calls and `read_state` requests; requests to `/call` endpoints are not accepted if any delegation in the chain carries this value, and a later delegation cannot lift the restriction. `"all"` is the same as omitting the field. Any other value makes the delegation invalid for requests of any kind (fail-closed). Also adds `permissions` to the string-typed field examples in the representation-independent hashing section, since the field is covered by the delegation signature. - **`abstract-behavior.md` (formal model):** extends `SignedDelegation` with `permissions : Text | Unrestricted`. `verify_delegations` now requires every delegation's `permissions` field to hold a supported value, and `verify_envelope` fails for update calls (content of type `Request`) when any delegation in the chain is restricted to `"queries"`. Because `verify_envelope` can distinguish update calls from read-only requests by the type of the enclosed content, no changes to the nine call sites of `verify_envelope` were needed. - **`changelog.md`:** adds a 0.63.0 entry visibly marked "unreleased"; an HTML comment instructs to assign the final version number and release date when the feature ships. ## Structural decisions - The whole-chain semantics ("queries" anywhere in the chain restricts the entire chain; unsupported values reject all request kinds) mirror `validate_delegations` in `rs/validator/src/ingress_validation.rs` of the draft implementation, including its test for a restriction sitting in the middle of a chain. - The restriction is phrased against `/call` endpoints rather than "update calls" alone, so replicated queries (query methods submitted as update calls) are explicitly covered, matching the implementation which rejects at ingress validation. `npm run build` passes (209 pages). https://claude.ai/code/session_01WBqBka57Q7xYi4btZYfPqT --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: mraszyk <31483726+mraszyk@users.noreply.github.com>
Queries-only permissions on delegations
This is the consolidated read-only-sessions feature (the previously stacked alternatives #10447/#10452, which carried the restriction in certified
sender_infoattributes, were closed in favor of this more lightweight approach).What it does
Request delegations gain an optional
permissionsfield, mirroring the existingtargetsfield. It is modeled as a typed enumDelegationPermissionswhose variants are encoded on the wire (and in the representation-independent hash) as text:permissionsabsent 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/callendpoints (updates and replicated queries) authenticated through such a chain are rejected during ingress validation with the newRequestValidationError::UpdateCallNotPermittedByDelegation; query andread_staterequests remain permitted.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 unionQueries | All | Unrestricted, which this enum mirrors exactly.Why this can't be bypassed, and why it's backward compatible
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
sendersfield (added Dec 2021, never implemented by the replica, removed in interface-spec#246). This PR follows the same formal pattern (verify_delegationsaccumulation), but implementation-first.Changes
ic-types: typedDelegationPermissions { Queries, All }enum (serialized as"queries"/"all"); optionalDelegation.permissionsfield included in the signed bytes (hash_of_mapkey"permissions"); chainablewith_targets/with_permissionsbuilder methods (replacing the formernew_with_targets/new_with_permissionsconstructors so a delegation can carry both).ic-validator:DelegationRestrictions(targets + queries-only) threaded through delegation-chain validation; update-call rejection viaRequestValidationError::UpdateCallNotPermittedByDelegation. The/callpath uses the sharedvalidate_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_permissionschain-builder support.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_permissionsinrs/tests/crypto/ingress_verification_test.rs, taggedlong_test) exercising the full HTTP path against a real replica.Caveats / follow-ups
/callpath (replicated queries) are rejected for"queries"delegations by design — the"all"vocabulary makes this explicit.@icp-sdk/core) round-tripping of the field prepared/pending.cargo test -p ic-types -p ic-validator -p ic-validator-ingress-messagepasses 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