fix(ic-agent): only route canisters by root-attested subnet ranges - #743
Merged
Merged
Conversation
`ic-agent` has two paths that fetch subnet info, and they drew canister ranges from different certificates: * `fetch_subnet_by_canister` takes them from the NNS-root-signed delegation and checks that they contain the canister. * `fetch_subnet_by_id` took them from the certificate's outer tree, which the subnet signs itself, and wrote them into the shared canister index. Only the first source is authoritative for deciding which subnet may answer for a given canister, so the second should not have fed that index. Ranges of the second kind reached `get_subnet_by_canister`, whose `canister_ranges.contains` guard then re-checked against that same self-attested set. The ranges cannot simply be read from the delegation on the by-ID path: for a subnet-scoped `read_state` the delegation attests only `/subnet/<id>/public_key` and `/time`, as this repo's own mainnet fixture (`agent_test/uzr34_node_keys.bin`) shows. So instead, tag cached subnets with the provenance of their ranges: `fetch_subnet_by_id` now inserts keys-only via `insert_subnet_keys_only`, and only root-attested entries can resolve a canister to a subnet. That also covers the interleaved case where a subnet is already in the index from an earlier authoritative fetch -- a later keys-only insert downgrades the entry rather than widening what it can claim, and the next lookup re-fetches through the canister-scoped path. Subnets fetched by ID are still cached by ID for their node keys, which the outer certificate does legitimately attest, and still report their self-declared ranges -- now documented as non-authoritative on that path, mirroring the existing note on `Subnet::key`. Update calls and `read_state` are unchanged; they re-derive ranges from the delegation on every call. Adds regression coverage for the cache boundary, which no existing test exercised -- all of the current range tests are canister-scoped. Both new tests fail against the previous behavior. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lwshang
force-pushed
the
fix/subnet-id-range-cache-poisoning
branch
from
September 15, 2026 13:22
18a8fca to
e028a7c
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The cache provenance guard correctly closes the self-attested routing path and is covered by focused regression tests.
Pull request overview
Restricts canister routing to authoritative, root-attested subnet ranges while retaining subnet-by-ID key caching.
Changes:
- Tracks range provenance in cached subnet entries.
- Prevents subnet-ID fetches from populating the canister index.
- Adds regression tests and provenance documentation.
File summaries
| File | Description |
|---|---|
ic-agent/src/agent/subnet.rs |
Documents range authority semantics. |
ic-agent/src/agent/mod.rs |
Enforces authoritative cache routing. |
ic-agent/src/agent/agent_test.rs |
Tests cache provenance invariants. |
CHANGELOG.md |
Records the security-related fix. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lwshang
marked this pull request as ready for review
September 15, 2026 14:58
|
✅ No security or compliance issues detected. Reviewed everything up to e028a7c. Security Overview
Detected Code Changes
|
adamspofford
approved these changes
Sep 15, 2026
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.
Canister-to-subnet resolution now only uses canister ranges that came from an NNS-root-signed delegation.
Background
Two paths fetch subnet info, and they drew canister ranges from different certificates:
fetch_subnet_by_canister— from the NNS-root-signed delegation, checking that they contain the canister.fetch_subnet_by_id— from the certificate's outer tree, which the subnet signs itself, and wrote them into the shared canister index.Only the first source is authoritative for deciding which subnet may answer for a given canister, so the second should not have fed that index. Ranges of the second kind reached
get_subnet_by_canister, whosecanister_ranges.containsguard then re-checked against that same self-attested set.The ranges can't simply be read from the delegation on the by-ID path. For a subnet-scoped
read_statethe delegation attests only/subnet/<id>/public_keyand/time— confirmed against this repo's own mainnet fixtureagent_test/uzr34_node_keys.bin, where the ranges appear solely in the outer tree. The canister-scoped fixtures do carry them in the delegation, which is why that path can enforce containment.Change
SubnetCacheentries becomeCachedSubnet { subnet, ranges_authoritative }:fetch_subnet_by_idinserts via a newinsert_subnet_keys_only— cached by ID,canister_indexuntouched.get_subnet_by_canisteraccepts only root-attested entries. This also covers the interleaved case where a subnet is already incanister_indexfrom an earlier authoritative fetch: a later keys-only insert downgrades the entry rather than widening what it can claim, and the next lookup re-fetches through the canister-scoped path.Subnets fetched by ID keep their node keys cached — the outer certificate does legitimately attest those — and still report their self-declared ranges via
Subnet::iter_canister_ranges/contains_canister, now documented as non-authoritative on that path, mirroring the existing note onSubnet::key. Update calls andread_stateare unchanged; they re-derive ranges from the delegation on every call.Tests
Two new tests, both of which fail against the previous behavior (verified by temporarily restoring it):
fetch_subnet_by_id_does_not_index_self_declared_ranges— end-to-end against the real mainnet fixture. Asserts the subnet is still cached by ID, that its self-reported range is still visible on the returnedSubnet, and that the canister within it does not resolve out of the cache.subnet_cache_only_routes_root_attested_ranges— pins the cache invariant both ways, including that root-attested ranges still resolve, so the cache isn't quietly disabled.No existing test covered this boundary; all the current range tests are canister-scoped.
cargo test -p ic-agent --lib→ 58 passed, 2 ignored (56 before).cargo clippy --all-targets --workspaceis clean. Theic-utils canister::tests::simplefailure in a full workspace run is pre-existing onmain— it wantsscripts/download_reftest_assets.sh.🤖 Generated with Claude Code