feat(be): sign a session to the II frontend at sign-in - #4268
Conversation
|
❌ 1 possible security or compliance issue detected. Reviewed everything up to 2699f79. The following issues were found:
Evidence:
Security Overview
Detected Code Changes
|
8a56cdc to
afa4f93
Compare
afa4f93 to
1ab21d9
Compare
1ab21d9 to
b0ccd19
Compare
b0ccd19 to
302a89d
Compare
The write path takes the anchor rather than its number so that it owns storing it, but it stored it only where its own change — the session count — had landed. A caller's change to the same anchor was discarded whenever the count did not move. Registering a browser and rotating its key is exactly such a change, and a sign-in from a browser that already holds a session at that account replaces that session rather than adding one, so the count does not move. Every ordinary repeat sign-in therefore threw the rotation away, leaving a browser key that is meant to last one sign-in usable indefinitely — and leaving the browser's last-used stamp, which the registry cap orders on, unmoved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
…s-a-session # Conflicts: # src/internet_identity/src/storage/tests.rs
Three changes that are one change. The write path took `&mut Anchor` and stored it only where its own change to it — the session count — had landed. It now takes the record by value, which is what `write` wanted anyway, and stores it unconditionally: taking it is taking the storing of it. The clone goes with the borrow, and the five callers that never touched the record lose a `mut` that was never true. `create_session` resolves the browser itself instead of being handed the results. The registry cap can give a browser up, and every session that browser held has to go in the same write; passing that consequence in as `dropped_browsers` made it something a caller could forget. Now nothing is passed: the params carry the keys the browser presents, and the id, the entry and whatever the cap gives up are all worked out inside. The unit tests go through real resolution rather than naming browser ids the registry never minted, which is why a browser signing in twice now rotates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
# Conflicts: # src/internet_identity/src/storage.rs # src/internet_identity/src/storage/tests.rs
`prepare_account_session` resolved the browser itself and passed the results — the id, and the browsers the cap gave up — into the write that had to act on them. It now passes the keys the browser presented and lets the write work the rest out, so the registration, the browser the cap gives up, that browser's sessions and the new session are one change nothing has to remember to carry. The ceremony no longer reads or holds the identity record at all. Registering a browser stops being archived, and `Operation::RegisterBrowser` goes with it: knowing whether a browser was new was the last thing the ceremony needed the record for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
`create_session` clamped the idle bound to the life the session was granted, so a `valid_till_ns` at or before now made it zero — and the sweep that prunes dead sessions, which runs in this same call, took the new record straight back out. The write then stored a list without it and this returned `Ok` naming a session no list holds. Refused before anything is read or stored. Not reachable from the ceremony, which clamps the life to at least ten minutes ahead, but this is public and should not answer `Ok` for a session it did not create. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
# Conflicts: # src/internet_identity/src/storage/tests.rs
…ing it A canister sets its salt once, at deployment. Awaiting `ensure_salt_set` on a delegation path was there to set it on the very first call, and the cost of that is an inter-canister boundary in the middle of every delegation ever issued: `time()` is constant only within one execution, so the clock had to be read after the await, and anything computed before it could arrive stale. `prepare_account_delegation`, `session_delegation::prepare_session_delegation` and `mcp::prepare_delegation` drop the await and take `now` from the endpoint that is the instant. All three, and the four endpoints above them, stop being async — there is no interleaving point left on the path. Same reasoning as the account endpoints in this stack, which stopped setting the salt for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
…apping The delegation a browser holds for its session is signed with II as its only target now. It exists to mint app delegations, which is an update call on this canister and nothing else; leaving the restriction to the frontend left it to the party holding the session key, who can decline to add it. `permissions` stays absent for the same reason it is set on app delegations: minting is an update call, so read-only travels on the app delegation instead. `add_delegation_signature` gains the parameter, and both sides take the targets from one helper because the two messages have to match byte for byte. `get_account_session` returned `NoSuchSession` for three different facts. A session that is present with no signature for the asked-for key and expiration is a missing delegation — `NoSuchDelegation`, since the remedy is to ask with the parameters that were signed, not to sign in again — and a seed that will not derive is the salt being unset, which is `InternalCanisterError`. An over-long origin no longer traps. `check_frontend_length` stays as it is for its existing callers, and a second helper answers instead, used where the response already carries a variant for the unreachable edges. Storage stops trusting its callers for the same bound: the one place an origin becomes a stored one refuses it, which covers the sign-in path, the write gate, and whatever is written later. Validations run before anything is stored, cheapest first, so a request with no account to sign in to does not pay for two P-256 verifications on the way to being told so. `witness_session_delegation` becomes `get_session_delegation` and `account_principal` becomes `get_account_principal_for_origin`, both after the names their neighbours already use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
`#[allow(dead_code)]` on a function whose caller has not landed yet is a promise, and it stops being one the moment the caller does land — from then on it silences a function that is genuinely dead. The sign-in ceremony calls `create_session`, `read_session` and the browser-key verifier, so all three lose theirs here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
`account_principal_of` never got the caller its note promised. The sign-in ceremony derives that principal itself, in `sessions.rs`, because storage's version is keyed by an application number while the ceremony has an origin — so the signature was the wrong shape for the one caller it was written for, and the logic was rewritten a layer up instead. Storage keeps `account_principals`, the batch form the index path actually uses. The test that reached for the singular one now reads the principal off the session handle `create_session` wrote, which is the value it was crossing against the index anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
# Conflicts: # src/internet_identity/src/storage/tests.rs
…s-a-session # Conflicts: # src/internet_identity/src/storage.rs
`AccountSessionError::NoSuchSession` and `AppSessionError::NoMatchingSession` said the same thing in two words, beside `NoSuchAccount` and `NoSuchDelegation`. The only difference is how the session was named — by id on one side, by the caller's own principal on the other — which is not worth a second vocabulary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
Design: #4224. Overview: #4230. The verifier only — its caller is the sign-in ceremony in #4268, which also removes the `#[allow(dead_code)]` this carries. A session records which browser created it, so the user can see where they are signed in and end one browser's access everywhere at once. That is only worth anything if the browser named in a sign-in is the browser that made it; otherwise an attacker holding an access method could attribute their session to a browser the user recognises, and the settings list would lie. Ingress messages are public, so a key presented in one sign-in can be read off the wire by anyone. `verify_browser_keys` checks two signatures before a sign-in may name a browser: the **current key** over the session key and the announced successor, and the **successor** over the session key and the current key. Each is made under its own domain separator, `ii-session-browser-key` and `ii-session-browser-successor`, so neither can be replayed in the other's role. Requiring possession of the successor is what closes the wire-reading attack: announcing a key you do not hold is impossible without its private half, so a key read off the wire cannot be claimed when its owner next presents one. **Verifying yields evidence, not a boolean.** It returns a `VerifiedBrowserKeys` whose fields are private and whose only constructor is this function, and `CreateSessionParams` requires one — so `create_session` registering a browser from these keys cannot depend on a caller having remembered to check. That is also why the module sits at the crate root rather than under `sessions`: storage must not depend upward on sessions, and a P-256 verifier that knows nothing of either is something both layers may use. **One encoding per key.** P-256 only, no fallback. A key that does not parse, a signature of the wrong length, and an empty signature are each refused rather than skipped — and so is a compressed SEC1 point, although it is valid DER for the same key. `from_public_key_der` accepts both, so one private key had two spellings and `resolve_browser`, which compares the bytes it is handed, would have seen two browsers. WebCrypto emits only the uncompressed form, so nothing legitimate is turned away and nothing stored needs migrating. Tests: `browser_key::tests` (11), one accepting case and ten refusals — including a successor signature replayed as the current one, a signature over another session key, one paired with a substituted successor, and a compressed key that `from_public_key_der` accepts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Design: #4224. Overview: #4230. The writers have no caller until the ceremony in #4268, which removes the `#[allow(dead_code)]` annotations this adds — each at the PR that first calls the function it sits on. The per-identity cap arrives in #4267. A session record on the account reference: created at, expires at, last refreshed, the browser that made it, and whether the user consented to queries only. **Its identity.** `session_seed = H(salt, "session", account_seed, session_id)`, every field length-prefixed. The id rather than `created_at`, which is not unique within a round. Building on the account's own seed rather than on the identity, application and account numbers is what makes a session survive anything that leaves the account's principal unchanged — naming a default account is exactly that, since it gains a number and a name while still deriving from the identity it was conjured from. Had the numbers been inputs, naming an account would have signed the user out of every app using it. Only `last_refreshed` is mutable, which is why the seed does not take it: a mutable input would change the session's principal every time it was stamped. **Replacement, not reuse.** A ceremony from a browser that already holds a session at this account deletes it and mints a new one, so a copy of the old chain stops working at the user's next sign-in rather than at its expiry. Expired records on the list go in the same write, which is what keeps the design free of any sweep. Read hardest: the seed derivation, since it decides what survives an account being renamed. Tests: `session_creation_tests` (23), including a second browser getting its own session, the same browser's being replaced, expired records pruned on write, an account the identity does not hold refused, and the seed being unchanged by naming a default account. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a-session # Conflicts: # src/internet_identity/internet_identity.did # src/internet_identity/src/browser_key.rs # src/internet_identity/src/storage.rs # src/internet_identity/src/storage/tests.rs
There was a problem hiding this comment.
🟡 Changes recommended
Removing storage read guards allows several existing public APIs to bypass the origin-length limit.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds backend APIs for creating revocable account sessions and retrieving their canister-signed delegations.
Changes:
- Adds session preparation/retrieval APIs with browser-key verification.
- Scopes session delegations to the Internet Identity canister.
- Adds storage validation and integration coverage.
File summaries
| File | Description |
|---|---|
src/internet_identity/tests/integration/sessions.rs |
Adds session ceremony tests. |
src/internet_identity/tests/integration/main.rs |
Registers session tests. |
src/internet_identity/src/storage/tests.rs |
Tests origin-length enforcement. |
src/internet_identity/src/storage.rs |
Adds session wiring and storage validation. |
src/internet_identity/src/sessions.rs |
Implements session APIs. |
src/internet_identity/src/session_delegation.rs |
Updates delegation-signing call. |
src/internet_identity/src/openid.rs |
Updates delegation-signing call. |
src/internet_identity/src/mcp_registration.rs |
Updates delegation-signing call. |
src/internet_identity/src/main.rs |
Exposes session endpoints. |
src/internet_identity/src/email_inbound/smtp.rs |
Updates delegation-signing call. |
src/internet_identity/src/delegation.rs |
Adds target-aware signing. |
src/internet_identity/src/browser_key.rs |
Enables browser-key verifier usage. |
src/internet_identity/src/account_management.rs |
Returns structured origin-length errors. |
src/internet_identity/internet_identity.did |
Defines the public Candid API. |
src/internet_identity_interface/src/internet_identity/types.rs |
Adds shared session types. |
src/frontend/src/lib/generated/internet_identity_types.d.ts |
Updates generated TypeScript types. |
src/frontend/src/lib/generated/internet_identity_idl.js |
Updates generated IDL bindings. |
src/canister_tests/src/framework.rs |
Adds browser-key test helpers. |
src/canister_tests/src/api/internet_identity/api_v2.rs |
Adds test API wrappers. |
Review details
- Files reviewed: 17/19 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Moving the origin bound to the write path left the reads behind. `read_account` and `list_accounts` never reach the choke point that mints an application, and the endpoints above them — `get_default_account`, `get_accounts`, `mcp_get_accounts` — check nothing themselves, so an over-long origin flowed through, absence normalised to the derived default, and the canister handed out a principal for an origin it could never persist. They answer nothing now instead of trapping: an origin that cannot be stored has no accounts under it, which is true rather than defensive and holds for callers not yet written. Two integration tests also stopped short of what they name. The delegation one leaned on `verify_delegation`, which builds the message it checks from whatever the reply carries — drop the targets on both sides and the signature still verifies — so the scope and permissions are now asserted outright. The successor-collision one re-signed only one of the two signatures, so the key proof failed first and `SuccessorAlreadyInUse` was never reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
Design: #4224. Overview: #4230. Depends on #4268 for the ceremony that creates a session. This is the PR the feature turns on: after it, a delegation an app holds lasts five minutes and the session behind it can be revoked. **`app_prepare_delegation` / `app_get_delegation`.** An app holds a session chain and asks for a delegation with it. What it gets is capped at five minutes and the cap is not requestable, so revoking the session ends access within one delegation lifetime. **Finding the session from the call.** The index mapping a session's rooted principal to `{account_principal, session_id}` is written by #4266; this adds the two accessors that read it and the caller that needs them. The call names nothing and attaches nothing: `caller()` is looked up, the account principal resolves through the principal index from #4238, and the record is read from the list. A hit is itself the proof that the caller is that session, since only the holder of its key can arrive as that principal. The account is named by principal rather than by locator on purpose: materialising a default account changes its locator and leaves its principal alone, so naming an account touches one index entry instead of every session on it. **The entry names the session by its id.** A browser keeps its id across sign-ins, so an index entry keyed on the browser and outliving its session would authenticate its holder as whatever that browser created next — a revoked chain coming back to life. `session_id` is allocated per session and never reissued, so an entry can only ever resolve to the one session it was written for. Every path that destroys a session drops its index entry in the same write; the id is what makes a stale entry fail closed regardless. **The `get` re-derives the five-minute ceiling** rather than trusting the `expiration` it is handed, because longer-lived delegations exist over the same account seed. An account's own principal is absent from the session index, so an app delegation cannot mint its own replacement. Tests: `integration/sessions.rs` (11), including a refresh refused once the session has expired, a session replaced by a new ceremony invalidating the previous chain, and the account principal minting nothing itself. Index accessor coverage lands in `account_principal_index_tests` and `session_creation_tests`. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
| StorageError::Browser(_) => AccountSessionError::InvalidBrowserKey, | ||
| // The account was checked above, so anything left is a broken storage invariant | ||
| // rather than a request this caller could have got wrong. | ||
| err => AccountSessionError::InternalCanisterError(err.to_string()), |
There was a problem hiding this comment.
Small nit: might be a good idea to enumerate all the errors so that new variants don't necessarily fall in the catch-all
| } = request; | ||
|
|
||
| check_authorization(identity_number)?; | ||
| frontend_length_within_limit(&origin).map_err(AccountSessionError::InternalCanisterError)?; |
There was a problem hiding this comment.
why is this an internal canister error?
| // Not `NoSuchSession`: the session was just read. A seed that will not derive means | ||
| // the salt is unset, which is a canister that never initialised rather than anything | ||
| // this caller named. | ||
| let seed = session_identity(identity_number, &origin, account_number, &session)?; |
There was a problem hiding this comment.
nit: would it make sense for session_identity to be infallible? or at least to differentiate the missing salt from other errors? thinking there may be other errors added later on in which case a failure here won't necessarily mean "salt not set"
| /// is an update call, so a read-only session that could not make one could not sign in | ||
| /// to an app at all. Read-only travels on the app delegation this credential mints. | ||
| fn session_delegation_targets() -> Vec<Principal> { | ||
| vec![ic_cdk::id()] |
There was a problem hiding this comment.
is this ever not this canister's ID? would it make sense to inline it?
| origin: &FrontendHostname, | ||
| account_number: Option<AccountNumber>, | ||
| ) -> Result<Principal, AccountSessionError> { | ||
| let salt = storage_borrow(|storage| storage.salt().copied()).ok_or_else(|| { |
There was a problem hiding this comment.
nit: I've seen state::salt() used as well, would it make sense to be consistent and either use storage_borrow()... everywhere or alternatively use state::salt() everywhere?
Design: #4224. Overview: #4230. This is the caller #4266 and #4264 were waiting for, so it removes their
#[allow(dead_code)]annotations.prepare_account_session/get_account_session. The first creates a session and signs its delegation to the II frontend's key; the second witnesses it. A separate pair fromprepare_account_delegation, not an option on it: both mint, but one proves a live session and identifies the account by its principal while the other proves an access method and names the anchor outright. Merging them would mean one method with two authorizers and two argument shapes, and would drag the frontend's internal surface into the public API.Everything that can refuse does so before anything is written, cheapest first. The account check leads — an account the identity does not hold is the one failure a caller can provoke, and returning it after the writes would leave a browser registered for a sign-in that never happened — so a request that was never going to succeed does not pay for two P-256 verifications on the way to being told so. Nothing is revealed by that order:
check_authz_and_record_activityabove is the auth guard. The browser proof follows, using the verifier from #4264, andcreate_sessiontakes theVerifiedBrowserKeysit produces rather than two byte strings.The session credential is scoped to Internet Identity. It exists to mint app delegations, which is an update call on this canister and nothing else, so it is signed with
targets = [id()]and can be presented nowhere else. Leaving that to the II frontend would have left it to the party holding the session key, who can decline to add it.permissionsstays absent for the same reason it is set on app delegations: minting is an update call, so a read-only session that could not make one could not sign in to an app at all — read-only travels on the app delegation instead.get_account_sessiontells three failures apart. No stored session for that id isNoSuchSession; a session that is present with no signature for the asked-for key and expiration isNoSuchDelegation, because the remedy is to ask with the parameters that were signed rather than to sign in again; a seed that will not derive is the salt being unset, which isInternalCanisterError. An over-long origin is that too, rather than a rejected message the caller cannot read as a response.The request carries what the browser is, not a label for it.
browser_descriptionholds the tokens the registry stores (#4242), and each token a client writes for itself — an unrecognised brand, an unrecognised system, and the hardware model — is bounded at 64 bytes. The named variants carry no text, so a description of nothing but those is within the limit whatever it says. Refused rather than truncated: a cut-off token would put a value in the record that no parser ever produced.Every later failure traps rather than returning. On the IC, returning an error commits state and only a trap rolls the message back, so once the browser registration is written a failure has to trap or a caller could be told "no" and still have a browser enrolled.
valid_foris the lifetime the user chose at consent, clamped by the canister to between 10 minutes and 30 days. Every ceremony creates, so it always applies: the replacement's expiry is measured from the ceremony that made it, and no session is renewed in place.Tests:
integration/sessions.rs(18) drives the real ceremony — creating and verifying a session, a request for another identity refused, the registry cap dropping the least recently used and ending its sessions, the key proof's rejections at the endpoint, rotation keeping the entry, a retired key returning as a new browser, and two browsers each keeping the description it registered with.