fix: accept delegation chains issued by a non-mainnet auth provider

[marc0olo]

icp identity principal (and account-id, delegation sign) fails outright for an identity
linked against a local Internet Identity:

Caused by:
    1: A canister signature in the delegation chain could not be verified: BLS verification failed

The chain's first link is a canister signature that only BLS-verifies against the issuing replica's
root key. Those commands pass no network root key and have no --network flag to supply one, so
the chain is always checked against mainnet. And DelegatedIdentity::new stops at the first link
it cannot verify, so the links behind it — including the session-key check create_identity
documents as running before anything is written — never ran.

Change

• A resolved network root key is authoritative and the only key consulted; with no network
  resolved, mainnet is the assumption.
• Where that leaves a canister signature unverifiable, every link is still verified as far as it
  can be without a root key — for a canister signature, everything but the certificate's own BLS
  signature. ic-agent verifies canister signatures only as a whole, so those checks are
  reimplemented here.
• Links are classified by the type of the key that signed them, never by the error they produced:
  ic-agent reports corruption through the same variant as a trust-root mismatch.
• The identity cache was keyed by selection alone, so an entry cached for one network was returned
  for another. It is now keyed by (selection, root_key), and canister create / canister settings update take the caller principal from the agent instead of loading the same identity a
  second time without a root key.
• icp identity link web now checks expiry before writing, as the import and load paths already
  do. Pre-existing; fixed here because it lands in the same validator.
• New direct deps ic-certification and serde_bytes, both already in the lockfile via ic-agent.

Stricter than main

A mainnet II identity used against a local network now fails at load rather than at ingress.
Delegation-based identities only — PEM, keyring, HSM and Pbes2 session chains are untouched.

Tests

17 unit tests, including a chain a local Internet Identity actually issued together with that
replica's root key. The synthetic fixtures are encoded by the same types the production code
decodes with, so only real bytes catch a change that shifts encoding and decoding together.

Worth discussing separately

DelegationError::InvalidCanisterSignature collapses a trust-root failure and structural
corruption into one variant, which is why those checks are reimplemented here. Filed as
dfinity/agent-rs#742 and referenced from the code; that landing deletes the reimplementation and
both new dependencies.

Nothing records which network a linked identity belongs to, so local-II and mainnet-II
delegations are indistinguishable on disk and icp identity list cannot show it. Storing a root
key at link time isn't obviously right either: managed networks regenerate theirs on recreate.

[Copilot]

🟡 Changes recommended

Canister-signature corruption is incorrectly treated as a root-key mismatch and accepted.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates delegated identity validation to support chains issued by non-mainnet authentication providers while still validating the remaining chain.

Changes:

• Adds suffix-based delegation verification with network-root fallback.
• Reuses validation during identity loading and linking.
• Adds unit tests for valid, skipped, broken, and mismatched chains.

File summaries

File	Description
crates/icp/src/identity/key.rs	Implements shared delegation-chain verification and tests.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[marc0olo]

Copilot is right, and the fix is now different.

InvalidCanisterSignature has 16 construction sites in ic-agent 0.49.1; only one (verify_cert_bls, "BLS verification failed") is the trust-root check. The other 15 are malformed CBOR, certified-data mismatches, canister-range violations and missing signature-tree leaves — genuine corruption that no root key would fix. Matching on the variant to peel links was wrong, and the deadbeef fixture in the first revision asserted exactly the behaviour that should have been fatal.

Rather than distinguish by error text, the peeling is gone. The chain is now accepted unverified only where no root key could be resolved at all (icp identity principal and friends, which have no --network flag), and only after confirming it was issued to the session key we hold. Where a root key was resolved and the chain does not verify against it, that is fatal.

So corruption is no longer misclassified as a root-key mismatch: on any network-resolving path it is caught, and on the no-root-key path the warning no longer claims a mismatch, only that nothing was available to check against.

A typed UntrustedRootKey variant in ic-agent would let the no-root-key path verify everything but the trust root. That belongs upstream, not here.

[Copilot]

🟡 Changes recommended

Supplied network roots can be bypassed through mainnet validation or cached unchecked identities, and the core fallback lacks direct tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 3
• Review effort level: Balanced

[marc0olo]

All three are valid; fixed.

Mainnet-first ordering. Correct, and it contradicted the rule the PR itself stated. A resolved network root key is now authoritative and the only key consulted — DelegatedIdentity::new_with_root_key with that key, or fatal. The mainnet path runs only when no root key was resolved. This also removed the IC_ROOT_KEY special-casing entirely (the import is now unused), which is a good sign the previous shape was wrong.

Loader cache. Real and reachable: canister create calls get_identity(.., None) at create.rs:358 and then get_agent_for_env (which resolves a root key) four lines later, on the same identity — so the unverified value was returned for the network path. load_identity now returns LoadedIdentity { identity, verified } and Loader only caches when verified. Keying the cache by root key was the alternative, but it would prompt twice for password-protected delegation identities, which have no session-key cache to fall back on.

Test coverage. Also correct — signed_link produces Ed25519 links, so neither test reached the fallback arm. There is now a canister-signature fixture (canister_sig_public_key, OID 1.3.6.1.4.1.56387.1.2) covering acceptance with no root key, rejection when the chain was issued to another session key, and rejection once a root key is resolved. That last one is the regression guard for finding 2.

For the record on round 1: the malformed-CBOR fixture is no longer load-bearing. Nothing peels links any more, so the corruption-versus-root-mismatch distinction only decides whether an error is fatal, and on the no-root-key path both are treated the same and named honestly — nothing was available to check against.

Also confirmed against the live local replica that issued the chain: the canister signature verifies under its root key and fails only under mainnet's.

[Copilot]

🟡 Changes recommended

Cache entries remain root-key agnostic, and unchecked multi-link chains can bypass validation of signatures after the canister-signature link.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/icp/src/identity/key.rs:1806

• This endpoint check does not complete the validation that stopped at the first canister signature. If that link is followed by another delegation with a forged ordinary signature but the expected session public key, the check succeeds and create_identity/link_webauth_identity persist a structurally invalid chain. Validate every suffix signature after the unverifiable canister link (or reject multi-link fallback chains) before returning success.

    ensure!(
        chain_ends_at_session(&from_key, &delegations, &**session)
            .map_err(|message| ValidateDelegationChainError::SessionPrincipal { message })?,

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Balanced

[marc0olo]

All three valid; fixed together, and the root cause of the churn addressed rather than patched.

Unverified links behind the canister signature. Correct — and it is the property the round-1 fix removed. The resolution is to identify unverifiable links structurally rather than by error variant: is_canister_signature_key parses the SPKI and matches OID 1.3.6.1.4.1.56387.1.2. Leading canister-signed links are set aside because their type makes them uncheckable without the issuer's root key; every link after them is verified in full by new_with_root_key, which also enforces the terminal session-key check. A canister signature further down the chain is left as an error. This satisfies round 1 (no peeling driven by error text, no unbounded search) and round 3 (nothing behind the prefix goes unchecked). The suppressed comment on the link-time path is fixed by the same helper.

Cache still root-key agnostic. Correct, and my previous verified flag fixed only half of it — verified means "verified under the key this load used", exactly as you say. The cache key is now (IdentitySelection, Option<Vec<u8>>), which is simply the full input to the load, and the verified plumbing is gone. Separately, the flow that made it reachable is gone too: canister create and settings update took the caller principal from a second root-key-less load of the same identity and now take it from the agent.

Network variant over-applied. Correct. Only InvalidCanisterSignature maps to ValidateDelegationChainNetwork now; a tampered Ed25519/ECDSA link or a session mismatch maps to ValidateDelegationChain. Guarded by a_broken_link_is_not_reported_as_a_network_mismatch.

Test matrix is now 11 cases covering both regressions above rather than the happy path.

Remaining known limit, for the record: with no root key resolved, a corrupt canister signature is still indistinguishable from one issued by an unknown network. Closing that means either matching error text or reimplementing the root-key-independent half of IC certificate verification here. The right fix is a typed variant upstream in ic-agent; not in this PR.

[Copilot]

🟡 Changes recommended

The fallback can persist an expired non-mainnet delegation, and the network-mismatch error is overly definitive.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Balanced

[marc0olo]

Two of three are correct; one is right about the gap but wrong about the cause.

Error message overstates. Correct, and it is the round-1 point applied to wording rather than control flow — I reasoned about that ambiguity and then wrote a message asserting certainty anyway. Now: "could not verify the canister signature ... against the selected network's root key; this identity may have been issued for a different network."

Cache-key regression untested. Correct. cached_identities now also loads the same selection under two distinct root keys, asserts the Arcs differ, and asserts each key keeps its own entry.

Expired chain persisted by the link path. The gap is real, but it is not introduced here — link_webauth_identity has never checked expiry. On main the is_expiring_soon call sites are the PEM session path, load_webauth_identity and create_identity; the link path is absent from that list there too, so an expired mainnet chain persisted just as readily before this PR. Severity is also lower than it reads: the chain is minted by a live login seconds earlier, so this needs clock skew or an unusually short delegation.

Fixed regardless, since it lands in the validator this PR already rewrites, and the import path's own comment argues exactly why it should exist. Added link_webauth_identity_rejects_an_expired_chain, which also asserts nothing was written to disk.

[Copilot]

🟡 Changes recommended

Root-independent corruption in skipped canister-signature links can currently pass validation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced

[marc0olo]

Correct, and taken the thorough way rather than the documentation way.

You are right on both halves: the OID identifies the signing algorithm, not the cause of the failure, and my doc comment claimed this function "verifies everything that does not depend on the root key" when it verified none of the discarded link. Worth being precise about the cost of that: on main a corrupt canister signature did not load, because DelegatedIdentity::new failed. Skipping the link wholesale traded that away, which is a relaxation the PR should not be making.

verify_canister_signature_structure now runs the root-key-independent half of the spec procedure on every discarded link: the signature CBOR decodes, the certificate decodes, lookup(["canister", cid, "certified_data"]) equals the signature tree digest, and lookup(["sig", sha256(seed), sha256(payload)]) is present. Only step 3, BLS verification of the certificate, is skipped. ic-agent offers no way to run those separately — verify_canister_sig is pub(crate) and takes the root key — so they are repeated here, against ic-certification and serde_cbor rather than hand-rolled hashing.

Validation that matters most here is that a genuine signature still passes. Three checks:

• A real local-II chain, unmodified, straight from disk: the structural check returns Ok(()).
• The old deadbeef fixture is now rejected, as are the three new negative cases.
• My previous end-to-end fixture was also rejected — I had re-pointed a real signature at a different delegation, and the new check caught it as a signature over another payload. That fixture was a forgery in exactly the sense this detects, so it has been replaced with a synthetically constructed but structurally sound one.

Test count 12 to 14. New direct dependencies ic-certification and serde_bytes, both already in the lockfile via ic-agent, so nothing new compiles.

The upstream point still stands and I have not tried to work around it here: a typed error separating trust-root failure from structural corruption would make this reimplementation unnecessary.

[Copilot]

🔵 Needs a closer look

It changes identity/delegation verification behavior (including cryptographic validation paths), which is security-sensitive and warrants final human review.

Review details

Suppressed comments (1)

crates/icp/src/identity/key.rs:723

• DelegationError::InvalidCanisterSignature is not only caused by a trust-root mismatch; it can also indicate malformed/tampered canister-signature material (as noted later in this file). The inline comment here currently claims the root key is the only deciding factor, which is inaccurate and risks misleading future maintainers.

            // A canister signature is the only failure the root key decides; anything else is a
            // broken chain, which a different root key would not have saved.
            Err(e @ DelegationError::InvalidCanisterSignature(_)) => {
                Err(e).context(ValidateDelegationChainNetworkSnafu { path: chain_path })
            }

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[marc0olo]

Both halves correct, and they are the same defect: Box::new(inner) moved the session away, so the error arm had nothing left to verify with and could only re-check structure.

inner is now cloned into the call, and the arm additionally runs verify_links_after_canister_signatures before reaching a verdict. That verifies from the last canister-signed link onwards — ordinary signatures plus the terminal check that the chain hands authority to our session key. Nothing before that point is reachable without a root key, since ic-agent verifies a chain from its root outwards, so this is exactly the remainder that ic-agent left unexamined when it stopped. The network verdict is now only returned when every root-key-independent check across the whole chain has passed.

Deliberately verifying from the last canister-signed link rather than past the leading run: the leading-run notion belongs to the no-root-key path, where a mid-chain canister signature is left as an error on purpose. Reusing it here would have re-broken the mid-chain classification fixed in the previous round.

Added a_broken_link_behind_a_canister_signature_is_not_a_network_mismatch, covering a sound canister signature with a sound ordinary link behind it (network mismatch) and with a tampered one (broken chain). Confirmed it fails without the new call and that the mid-chain test from last round still passes. 17 tests in this module, 350 in the crate.

[Copilot]

🔵 Needs a closer look

It changes security-sensitive identity/delegation verification behavior (including canister-signature handling), which warrants final human review despite strong unit test coverage.

Review details

Suppressed comments (1)

crates/icp/src/identity/key.rs:137

• The error message mentions a “malformed certificate delegation”, which reads like a typo/unclear phrasing (it’s not obvious what a “certificate delegation” is). Consider rewording to explicitly say “malformed certificate or delegation” (or similar) so the alternative failure mode is clear.

    #[snafu(display(
        "could not verify the canister signature in the delegation chain loaded from `{path}` \
         against the selected network's root key; this identity was most likely issued for a \
         different network, though a malformed certificate delegation would also fail here"
    ))]

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[marc0olo]

Correct, and easy to confirm on the wire:

real local-II signature:  d9d9f7 a26b63...   <- tag(55799), then the map
fixture (before):         a26b63657274...    <- bare map

So every canister-signature fixture in the suite was decoding an input shape that no auth provider actually produces. The production path handled the tagged form — I checked a genuine chain against it earlier in this PR — but nothing in the committed tests pinned that, so a future change to the CBOR layer could break real signatures with the suite still green. That is exactly the regression the comment describes.

The fixture builder now writes the tag via Serializer::self_describe, and asserts the first three bytes are d9 d9 f7 so it cannot silently drift back. Every canister-signature test therefore exercises tagged decoding, and the end-to-end fixture was regenerated in the same form.

Added a_tagged_canister_signature_decodes, which pins both shapes: the tagged one because it is what the wire carries, and the bare map because the production comment claims serde_cbor sees through the tag and that leniency should not regress unnoticed either. 18 tests in this module, 351 in the crate.

[Copilot]

🔵 Needs a closer look

It introduces new security-sensitive delegation validation logic (certificate/canister-signature handling) that warrants final human review despite good test coverage.

Review details

Suppressed comments (1)

crates/icp/src/identity/key.rs:723

• This comment implies you can distinguish “untrusted root key” vs “damaged signature”, but the follow-up checks don’t (and can’t) validate the certificate’s own BLS signature bytes without a trusted root key. Clarifying that the fallback only distinguishes structural corruption from certificate BLS verification failure will keep the rationale accurate.

            // `InvalidCanisterSignature` covers both a signature this root key does not trust and
            // one that is simply damaged. ic-agent stops at the link it could not verify, so the
            // rest of the chain is still unexamined. Check everything that needs no root key: if
            // all of it holds, the root key is the only thing that turned this chain down.

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[marc0olo]

The conclusion here is not right, but it is pointing at real wording that misled it.

verify_canister_signature_structure never parses certificate.delegation.certificate — inside Certificate that field is raw bytes, decoded only in ic-agent's resolve_cert_key. So when the certificate carries a subnet delegation, five structural failures remain reachable after the structural check passes, none of them BLS:

resolve_cert_key:
  serde_cbor::from_slice(&delegation.certificate)   "invalid delegation certificate CBOR"
  delegation_cert.delegation.is_some()              "nested delegations are not allowed"
  Principal::try_from_slice(&delegation.subnet_id)  "invalid subnet_id in delegation"
  lookup_canister_ranges(..) / ranges.contains(..)  "canister range lookup failed" / not in ranges
  lookup_path([subnet, id, public_key])             "subnet public key not found"

These sit behind the BLS step, so they cannot be reached without a trusted root key, and dropping the hedge would state a certainty the code has not established. Mainnet II certificates always carry a subnet delegation, so this is the live case, not a theoretical one.

That said, the phrase "a malformed certificate delegation" was evidently readable as "a malformed certificate", which the structural check does cover — so the wording was doing the opposite of its job. Both the message and the code comment now say what is actually true:

• message: "...is well formed, but the selected network's root key does not accept it; this identity was most likely issued for a different network — the one part not checked here is the certificate's subnet delegation"
• comment: "what survives is the trust decision itself, plus the handful of structural checks ic-agent makes on the certificate's subnet delegation, which sit behind the BLS step"

The old comment claimed "the root key is the only thing that turned this chain down", which was the overstatement that prompted this. That is the part that was genuinely wrong, and it is fixed.

Eliminating the hedge entirely would mean following the subnet delegation ourselves — parsing the inner certificate and re-deriving canister ranges — which is more reimplementation than a message nuance justifies. The upstream typed-error change remains the real fix.

[Copilot]

🔵 Needs a closer look

It changes core identity/delegation validation behavior (including certificate/hash-tree parsing) in a security-sensitive area and warrants final human review.

Review details

Suppressed comments (1)

crates/icp/src/identity/key.rs:137

• The ValidateDelegationChainNetwork error message currently implies the chain was “most likely issued for a different network”, but after the structural re-checks the remaining causes also include an invalid certificate BLS signature or invalid subnet delegation (both are behind the root-key verification step and aren’t distinguished here). Consider wording this as an ambiguous verification failure rather than primarily a network mismatch.

        "the canister signature in the delegation chain loaded from `{path}` is well formed, but \
         the selected network's root key does not accept it; this identity was most likely issued \
         for a different network — the one part not checked here is the certificate's subnet \
         delegation"

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[marc0olo]

Correct. SubjectPublicKeyInfoRef::decode(&mut SliceReader::new(..)) reads one value and leaves the rest; from_der calls reader.finish() and rejects trailing data. Both functions now go through one strict helper, decode_public_key.

Worth recording why this is safe to tighten, since ic-agent parses the same key leniently and a divergence would be the thing to fear. Well-formed keys decode identically under both, so nothing legitimate changes. They differ only on a key with trailing bytes, and there the strict behaviour is the one we want:

• lenient (before): the link is classified as canister-signed, set aside as unverifiable, structurally checked on the prefix, and the chain loads with a warning — a principal derived from a slice we only partially read.
• strict (now): the link is not classified as canister-signed, so it goes to ordinary verification; ic-agent dispatches on the OID it still finds, the BLS check fails, and the chain is rejected.

So the malformed key is rejected instead of quietly accepted, and no well-formed input moves.

Added a_key_with_trailing_bytes_is_not_a_canister_signature_key, covering the classifier, the parser, and an end-to-end load. Confirmed it fails against the lenient parse. 19 tests in this module, 352 in the crate.

[Copilot]

🔵 Needs a closer look

It changes security-sensitive delegation validation semantics and introduces new certificate/parsing logic where a human review of correctness and edge cases is warranted despite the added tests.

Review details

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes delegation-chain validation behavior in security-sensitive identity code (including new verification logic and dependencies) and warrants final human review despite good test coverage.

Review details

• Files reviewed: 8/9 changed files
• Comments generated: 1
• Review effort level: Lite

[marc0olo]

Deliberate, and declining.

The suggested shape is what main does today, and removing it was part of this change:

// main, build path fallback:
// re-deserialize as ::new just ate the old values (better than an up-front clone since this path should be rare)
let (from_key, signed_delegations) = delegation::to_agent_types(&stored_chain)
    .expect("same conversion already succeeded");

That trades one clone for a second hex-decode pass plus an expect on an error path in identity
loading. The single conversion removes the panic site, which is worth more than the copy.

The fallback needs owned values regardless: new_unchecked takes from_key: Vec<u8> and
chain: Vec<SignedDelegation> by value, so re-converting would be mandatory rather than an
optimisation.

On the cost itself — the chain in testdata/local_ii_chain.json, which is what a real Internet
Identity issues, decodes to about 1.8 KB. The same load reads a PEM from disk, may prompt for a
password, and runs a BLS pairing verification. The clone is not measurable against any of those.

Cloning only when network_root_key.is_none() would avoid the copy on the common path, since that
is the only arm reading the originals, but it puts back a branch the single-match shape just
removed for a saving nobody can observe.