From 52112e6ec67f7106fecdca64f79d7acf5ca0516f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 21:45:52 +0000 Subject: [PATCH 1/5] Comply with peer design-review outcomes (H3/H2/H1/M2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement the required changes from the IC MCP server design-review: H3 (account takeover): bind the OAuth browser session across /oauth/authorize -> /oauth/finish with an unguessable HttpOnly; Secure; SameSite=Lax cookie scoped to the instance's /oauth path. finish now requires a matching cookie before minting the code, defeating the session-fixation takeover where an attacker phishes their own II connect link to a victim who trusts the origin. H2 (read-only sessions): parse the completion POST's new `permissions` field ("queries" | "all") to learn the session access level at connect; gate every management (update) tool with require_write, returning an actionable "reconnect with read-only off" message instead of an opaque IC ingress rejection; surface a read-only session in get_principal. The genuine query tool (cycles_balance) stays ungated. Unknown level falls back to the ingress-error path. H1a: normalize target_origin to a bare https:// (strip :443, path, query, fragment, trailing slash) before the gateway remap, so a stray port/path no longer derives a different principal. H1b: stop claiming exact browser-principal parity in tool descriptions, instructions and README; describe derivation as domain-based and add the custom derivation-origin (ii-alternative-origins) fallback hint. M2: drop the RFC 8628 device grant entirely — routes, handlers, structs, AS metadata grant_types_supported, and the register intersection. Doc corrections: replace "synthetic" with "the anchor's current (user-controllable) default", note refresh tokens as a deliberate non-goal, and rewrite the README auth section accordingly. Add tests for target_origin normalization, read-only gating, and the binding-cookie extraction. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- README.md | 162 +++++++++++++------ src/auth.rs | 401 ++++++++++++++++------------------------------ src/identities.rs | 120 ++++++++++++-- src/main.rs | 37 +++-- src/management.rs | 10 +- 5 files changed, 397 insertions(+), 333 deletions(-) diff --git a/README.md b/README.md index 6c89e06..6687156 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ encoding/decoding and signing against the IC via | `get_candid` | `canister_id` | The canister's `candid:service` interface (`.did` text) | | `call_canister` | `canister_id`, `method`, `args` (textual Candid), `is_query`, `domain?`, `account?` | Reply as textual Candid; called anonymously (no `domain`) or as your account at an application domain, derived on demand (`account` names a non-default account there) | | `get_principal` | `domain`, `account?` | The principal you act as at an application domain (derives the delegation on demand, same as `call_canister`), without making a call | -| `list_accounts` | `domain` | The user's Internet Identity accounts at an app — the default ("synthetic") account plus any named ones — with name, account number, and last-used time; name one via `account` in `call_canister`/`get_principal` | +| `list_accounts` | `domain` | The user's Internet Identity accounts at an app — the default account (the anchor's current default at that origin) plus any named ones — with name, account number, and last-used time; name one via `account` in `call_canister`/`get_principal` | | `list_ic_skills` | — | The official [IC skills](https://skills.internetcomputer.org) (Motoko, mops/icp CLIs, cycles, stable memory, security, …), grouped by category | | `get_ic_skill` | `name` | The full `SKILL.md` instructions for one skill (e.g. `motoko`, `icp-cli`, `cycles-management`) | | `cycles_balance` | — | Your cycles-ledger balance (the funds `create_canister`/`top_up_canister` spend), as your standing II principal | @@ -48,12 +48,23 @@ call as your account at that app. For a domain, the server mints a **short-lived account delegation on demand** using the connection's registered Internet Identity session key (see [Domain identities](#domain-identities-on-demand)) — there is no per-app sign-in step. `get_principal` returns that account's principal -without a call. A user may hold several accounts at an app — a default -("synthetic") account everyone gets automatically, plus any they have named — so -`list_accounts(domain)` lists them (via II's `get_accounts`), and -`call_canister`/`get_principal` take an optional `account` (a name from that list) -to act as a specific one; omit it for the default account. All these tools require -a bearer token (see Auth). +without a call. A user may hold several accounts at an app — a default account +everyone gets automatically (the anchor's current, user-controllable default at +that origin), plus any they have named — so `list_accounts(domain)` lists them +(via II's `get_accounts`), and `call_canister`/`get_principal` take an optional +`account` (a name from that list) to act as a specific one; omit it for the +default account. All these tools require a bearer token (see Auth). + +> **Derivation is domain-based — not guaranteed to equal a browser sign-in.** II +> derives the per-app principal from the app's **domain**, which this server +> normalizes to a bare `https://` (with the gateway remap below). That is +> usually the same identity a browser sign-in to the app would use, but **not +> always**: an app can declare a *custom derivation origin* via +> `/.well-known/ii-alternative-origins`, which browsers honour but the `mcp_*` +> methods don't expose — so from the domain alone the server derives a different +> principal, silently. If a principal, account, or balance doesn't match what the +> user sees in their browser at that app, that's the likely cause; the tools tell +> the agent to offer looking up the app's `ii-alternative-origins` and retrying. ### Skills awareness @@ -99,8 +110,12 @@ claude mcp add --transport http ic-poc https://YOUR-HOST/mcp Then run `/mcp` → **ic-poc** → authenticate: the client sends the browser to **Internet Identity**'s `/mcp` handshake; you sign in once, II registers the connection's session key as a time-boxed grant and returns you to the client, and -the tools become available. Redirect-based clients (e.g. Claude.ai) use the -authorization-code flow; clients that poll can use the device grant (RFC 8628). +the tools become available. All clients use the same **authorization-code + PKCE** +flow. + +> On II's consent screen, **leave the read-only option OFF** if you want to create +> or manage canisters — read-only is the default, and it makes every management +> tool inert (see [Read-only sessions](#read-only-sessions) below). ## Run @@ -154,44 +169,82 @@ curl -s "${H[@]}" -d '{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"n `/mcp` is gated by a bearer token. II's `/mcp` handshake has **no redirect back to this server** — the II tab makes two background `fetch()` POSTs to our callback and then finishes on its own — so a classic authorization-code -`redirect_uri` can't be delivered *by II*. We bridge that gap and support **two -flows**, so any MCP client works: +`redirect_uri` can't be delivered *by II*. We bridge that gap with a single +**authorization-code + PKCE** flow, so any OAuth 2.1 client works: + +- `/oauth/authorize` validates the client + redirect and PKCE, sets a + **browser-binding cookie** (see below), and redirects to II's handshake. The + key-request response carries a `finish_url` on our origin, so after + `mcp_register` II navigates the browser to `/oauth/finish`, which checks that + cookie, confirms registration, mints a PKCE-bound code, and 302s to the client's + `redirect_uri?code=…&state=…`. -- **Authorization code (finish redirect)** — for redirect-based clients (e.g. - Claude.ai). `/oauth/authorize` redirects to II's handshake; the key-request - response carries a `finish_url` on our origin, so after `mcp_register` II - navigates the browser to `/oauth/finish`, which confirms registration, mints a - PKCE-bound code, and 302s to the client's `redirect_uri?code=…&state=…`. -- **Device grant (RFC 8628)** — for clients that can poll: `device_code` + - `verification_uri`, and the client polls `/oauth/token` until the grant is live. +The **RFC 8628 device grant was dropped**: no listed MCP client uses it, and it +adds a device-code phishing surface with none of the PKCE binding the rest of the +flow relies on. Endpoints: -- `GET /.well-known/oauth-authorization-server` — AS metadata (advertises both - the `authorization_code` and `device_code` grants) +- `GET /.well-known/oauth-authorization-server` — AS metadata (advertises + `grant_types_supported: ["authorization_code"]`) - `GET /.well-known/oauth-protected-resource` — points clients at the AS - `POST /oauth/register` — dynamic client registration (RFC 7591); `redirect_uris` - are stored (device-only clients may register none) and persisted to - `OAUTH_CLIENTS_FILE`; requested `grant_types` are honoured -- `GET /oauth/authorize` — validates the client + redirect, requires PKCE, then - redirects to II's handshake -- `GET /oauth/finish` — II navigates here after registration; confirms the grant - is live, then 302s to `redirect_uri?code=…&state=…` -- `POST /oauth/device_authorization` — device-grant entry: mints a `device_code`/ - `user_code` + `verification_uri` (RFC 8628 §3.2) -- `GET /oauth/device` — the user opens this; it launches II's `/mcp` handshake - with the connect `state` (= session id) in the URL fragment, `ttl` in **seconds** + are stored and persisted to `OAUTH_CLIENTS_FILE`; requested `grant_types` are + honoured (intersected with `authorization_code`) +- `GET /oauth/authorize` — validates the client + redirect, requires PKCE, sets + the binding cookie, then redirects to II's handshake +- `GET /oauth/finish` — II navigates here after registration; requires the + binding cookie, confirms the grant is live, then 302s to `redirect_uri?code=…&state=…` - `POST /oauth/connect/callback` — serves II's **two cross-origin JSON POSTs**: - a key request `{state}` → `{public_key}` (a fresh session keypair, minted here), - and a completion notification `{state, expiration}` → mark the grant live -- `POST /oauth/token` — exchanges an authorization `code` (PKCE), or polls with a - `device_code` (`authorization_pending`/`slow_down`), for the access token + a key request `{state}` → `{public_key, finish_url}` (a fresh session keypair, + minted here), and a completion notification `{state, expiration, permissions}` → + mark the grant live and record the session's access level +- `POST /oauth/token` — exchanges an authorization `code` (PKCE) for the access token "Grant is live" means II's completion POST arrived **or** a signed `mcp_get_accounts` now succeeds (belt-and-suspenders, since the completion POST is best-effort). Unauthenticated `/mcp` requests get `401` with a `WWW-Authenticate` header pointing at the resource metadata, as the MCP spec expects. +### Browser-session binding (`/oauth/authorize` → `/oauth/finish`) + +The `state` in the II connect link is echoed back to the client in the final +redirect, so it **cannot by itself prove** that the browser finishing at +`/oauth/finish` is the one that started at `/oauth/authorize`. Without a binding +there's a session-fixation takeover: an attacker registers a client (open DCR) +with their own `redirect_uri` + PKCE challenge, calls `/oauth/authorize`, reads +the II connect link from the 302, and phishes it to a victim who already trusts +this origin in II Settings (II's consent screen names only the *origin*, never the +OAuth client, so nothing warns the victim). The victim consents, the server would +mint a code for the *attacker's* pending auth and 302 the victim to the attacker's +`redirect_uri`, and the attacker redeems it with their own PKCE verifier — a token +acting as the victim. (The "arrival ≠ registration" check guards a different +property; it doesn't bind the initiator to the consenting user, and II can't help +— its trust model only ever identifies the origin.) + +The fix: `/oauth/authorize` sets an unguessable, single-use, `HttpOnly` `Secure` +`SameSite=Lax` cookie (scoped to the instance's `…/oauth` path) and `/oauth/finish` +requires it before minting the code — so only the browser that **started** the +flow can complete it. `SameSite=Lax` still rides the top-level cross-site GET II +uses to navigate back to `finish_url`. + +### Read-only sessions + +II's consent screen **defaults to read-only** (opt-out). A user who just clicks +"Allow" gets a session whose per-app delegations are `permissions = "queries"`, +and the IC **rejects update calls made through them at ingress**. That makes the +entire canister-management surface inert — `create`/`install`/`start`/`stop`/ +`uninstall`/`delete`, and even `canister_status`, are update calls. To handle this +without opaque low-level errors: + +- The completion POST now carries `permissions: "queries" | "all"` (§0), so the + server learns the level at connect without minting a probe delegation (falling + back to a missing value = full access). +- Management tools check it up front and, for a read-only session, return an + actionable *"reconnect with read-only off"* message instead of an ingress error. +- `get_principal` reflects a read-only session in its output, so the agent won't + attempt updates it can't make. + **The server is passive during the handshake and holds no key at link time.** On the key-request POST it generates a fresh per-connection Ed25519 keypair and returns only its **public** key (base64url, unpadded, DER). II's *frontend* @@ -202,10 +255,11 @@ calls `mcp_register`.** The issued access token is bound to the session key's principal (`self_authenticating(session_pubkey)`), which is exactly the identity the grant is bound to. -**PKCE (S256)** is required for the authorization-code flow and honoured for the -device grant; auth codes live 120s, device codes 600s, access tokens 1h. Treat any -`Unauthorized` from II as "session over → reconnect": the server surfaces a -reconnect message and does not retry. +**PKCE (S256)** is required for the authorization-code flow; auth codes live 120s, +access tokens 1h (bounded by the II grant — refresh tokens are a deliberate +non-goal, as they'd add no security over the grant and only pay off with +server-side persistence). Treat any `Unauthorized` from II as "session over → +reconnect": the server surfaces a reconnect message and does not retry. Set the public base URL (used in the discovery docs, as the MCP origin, and as the management identity's derivation origin) with `PUBLIC_URL`. The Internet Identity @@ -265,8 +319,15 @@ mcp_get_delegation : - `session_key` is the DER pubkey of a **fresh per-app key**, distinct from the connection's session key; the minted delegation is issued to it. -- `target_origin` is `https://`, with IC gateway domains remapped: - `*.icp0.io` / `*.icp.net` → `*.ic0.app`. +- `target_origin` is the app's **bare** `https://` origin. II derives the + principal from an anchored regex on that bare origin, so the server first strips + any path, query, fragment, trailing slash, or redundant `:443` (a stray one + would derive a *different* principal), then applies the gateway remap: + `*.icp0.io` / `*.icp.net` → `*.ic0.app`. Note this replicates only II's + *domain-based* derivation — a **custom derivation origin** declared via + `/.well-known/ii-alternative-origins` isn't visible through the `mcp_*` methods, + so an app using one derives a different principal here than in a browser (see the + caveat under [Tools](#tools)); fetching that declaration is a future enhancement. - `account_number` names which of the anchor's accounts at `target_origin` to act as; `null` selects the (mutable) default account there. `prepare` resolves it and returns the concrete account in its reply, which is threaded back into @@ -284,10 +345,11 @@ mcp_get_delegation : ### Listing accounts -A user can hold several accounts at one app: a default ("synthetic") account -everyone gets automatically, plus any **named** accounts they created there. Each -account is a **distinct per-origin principal** — the app never sees a global, -cross-app identity. `list_accounts(domain)` returns them by calling II's +A user can hold several accounts at one app: a default account everyone gets +automatically (the anchor's current, user-controllable default at that origin), +plus any **named** accounts they created there. Each account is a **distinct +per-origin principal** — the app never sees a global, cross-app identity. +`list_accounts(domain)` returns them by calling II's ```candid mcp_get_accounts : (target_origin: text) @@ -312,16 +374,18 @@ delegation. Omitting `account` uses the default account. > delegation methods from the earlier `mcp_prepare_account_delegation` / > `mcp_get_account_delegation` and removes `mcp_set_access` / `mcp_access_enabled`. > The live round-trip works once that II build is deployed to the configured -> `II_URL`. Passing `account_number = null` resolves to the anchor's default -> ("synthetic") account. +> `II_URL`. Passing `account_number = null` resolves to the anchor's current +> (user-controllable) default account at the origin — which may be a named account +> the user set as their default there, not necessarily the anchor's base account. ## Roadmap - [x] Candid tools over MCP streamable-HTTP; `discover_canisters`; Candid reference resources. -- [x] OAuth auth (device grant, RFC 8628): the client polls while II's `/mcp` - handshake registers the connection's session key (two JSON callback POSTs, - no delegation chain); PKCE; expiring tokens. +- [x] OAuth 2.1 auth (authorization-code + PKCE): II's `/mcp` handshake registers + the connection's session key (two JSON callback POSTs, no delegation chain), + with `/oauth/authorize`→`/oauth/finish` browser-session binding; expiring + tokens. (The RFC 8628 device grant was dropped.) - [x] On-demand **domain identities**: the registered session key mints per-app account delegations directly via II canister methods (`call_canister`/`get_principal` `domain`); no per-app browser flow. diff --git a/src/auth.rs b/src/auth.rs index 5870c51..9d860f1 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -3,28 +3,29 @@ //! //! II's `/mcp` handshake registers the session key under the user's own auth and, //! if we ask, navigates the browser back to a `finish_url` on our origin (see -//! `docs/mcp-server-guide.md` §2a/§2c / dfinity/internet-identity#4086). We use -//! that hook to serve two flows, so any MCP client works: +//! `docs/mcp-server-guide.md` §2a/§2c / dfinity/internet-identity#4086). We drive +//! one flow — **authorization code + PKCE** — so any OAuth 2.1 client works: //! -//! * **Authorization code (finish redirect)** — for redirect-based clients -//! (e.g. Claude.ai). `/oauth/authorize` redirects to II's handshake; the -//! key-request response carries a `finish_url`, so after `mcp_register` II -//! navigates the browser to `/oauth/finish`, which confirms registration, -//! mints a PKCE-bound code, and 302s to the client's `redirect_uri`. -//! * **Device authorization grant (RFC 8628)** — for clients that can poll the -//! token endpoint: `/oauth/device_authorization` → `device_code` + -//! `verification_uri`; the user opens it (launching the same II handshake, no -//! `finish_url`); the client polls `/oauth/token` until the grant is live. +//! * `/oauth/authorize` sets a browser-binding cookie (H3) and redirects to +//! II's handshake; the key-request response carries a `finish_url`, so after +//! `mcp_register` II navigates the browser to `/oauth/finish`, which requires +//! that cookie, confirms registration, mints a PKCE-bound code, and 302s to +//! the client's `redirect_uri`. +//! +//! The RFC 8628 device grant was dropped (no listed client uses it, and it adds a +//! device-code phishing surface with none of the PKCE binding the rest of the flow +//! relies on). //! //! Connect handshake (Phase 1b): `/oauth/connect/callback` serves the two -//! cross-origin JSON POSTs II makes — a key request `{state}` → `{public_key -//! [, finish_url]}` (a fresh session keypair minted per connection) and a -//! completion notification `{state, expiration}` → mark the grant live. We never -//! receive or verify a delegation chain, and never call `mcp_register` (II's -//! frontend does, under the user's own authentication). +//! cross-origin JSON POSTs II makes — a key request `{state}` → `{public_key, +//! finish_url}` (a fresh session keypair minted per connection) and a completion +//! notification `{state, expiration, permissions}` → mark the grant live and +//! record the session's access level. We never receive or verify a delegation +//! chain, and never call `mcp_register` (II's frontend does, under the user's own +//! authentication). //! //! Implemented: dynamic client registration, PKCE (S256) enforced, short-lived -//! codes/device codes, 1h access tokens, session-key-bound principal. +//! codes, 1h access tokens, session-key-bound principal. use std::{ collections::HashMap, @@ -49,20 +50,19 @@ use uuid::Uuid; use crate::identities::Identities; -/// How long an authorization request (auth-code or device) and its pending II -/// handshake stay valid before the user must restart. +/// How long an authorization request and its pending II handshake stay valid +/// before the user must restart. const CONNECT_TTL: Duration = Duration::from_secs(600); -/// Lifetime of a minted authorization code (auth-code flow). +/// Lifetime of a minted authorization code. const CODE_TTL: Duration = Duration::from_secs(120); -/// Minimum seconds a client should wait between token polls (RFC 8628 `interval`). -const POLL_INTERVAL_SECS: u64 = 5; /// Access-token lifetime (also the II grant's default, 1h). const TOKEN_TTL: Duration = Duration::from_secs(3600); /// `ttl` (seconds) requested for the II grant. Clamped by II to [600, 2592000]. const GRANT_TTL_SECS: u64 = 3600; -/// RFC 8628 device-code grant type. -const DEVICE_GRANT_TYPE: &str = "urn:ietf:params:oauth:grant-type:device_code"; +/// Name of the browser-session cookie that binds `/oauth/authorize` to +/// `/oauth/finish` (H3): only the browser that started the flow can complete it. +const CONNECT_COOKIE: &str = "mcp_connect"; /// Public base URL clients use to reach this server. Override with PUBLIC_URL. pub fn base_url() -> String { @@ -147,18 +147,16 @@ pub struct AuthStore { clients: Arc>>, tokens: Arc>>, /// Auth-code connects in flight, keyed by `session_id` (= the II connect - /// `state`). Serve the poll bridge for redirect-based clients. + /// `state`). authz: Arc>>, /// Minted authorization codes awaiting exchange at `/oauth/token`. codes: Arc>>, - /// Device-authorization grants in flight, keyed by `session_id`. - devices: Arc>>, /// Shared with the MCP tools: the session's backend key / grant expiration /// live here (keyed by `session_id`) for the tools to sign with. identities: Identities, } -/// An auth-code connect awaiting the user's II handshake (poll bridge). +/// An auth-code connect awaiting the user's II handshake. #[derive(Clone, Debug)] struct AuthzPending { client_id: String, @@ -166,10 +164,15 @@ struct AuthzPending { /// The OAuth client's own `state`, echoed back on the final redirect. client_state: String, code_challenge: Option, + /// Unguessable value set as a browser cookie at `/oauth/authorize` and + /// required (matched) at `/oauth/finish` (H3) — so only the browser that + /// STARTED this flow can complete it, closing the session-fixation takeover + /// where an attacker phishes their own connect link to a trusting victim. + cookie: String, created: Instant, /// Grant confirmed live (completion POST or a signed-call fallback). live: bool, - /// The authorization code minted once the grant is live (idempotent polls). + /// The authorization code minted once the grant is live (idempotent finish). code: Option, } @@ -182,21 +185,6 @@ struct CodeGrant { created: Instant, } -/// A device-authorization grant awaiting the user's II handshake. -#[derive(Clone, Debug)] -struct DeviceAuth { - device_code: String, - user_code: String, - session_id: String, - #[allow(dead_code)] - client_id: Option, - scope: Option, - code_challenge: Option, - created: Instant, - live: bool, - last_poll: Option, -} - #[derive(Clone, Debug)] struct TokenInfo { principal: String, @@ -225,7 +213,6 @@ impl AuthStore { tokens: Arc::default(), authz: Arc::default(), codes: Arc::default(), - devices: Arc::default(), identities, } } @@ -317,6 +304,11 @@ pub async fn authorize(State(store): State, Query(q): Query, Query(q): Query Option { + let raw = headers.get(axum::http::header::COOKIE)?.to_str().ok()?; + raw.split(';') + .filter_map(|kv| kv.trim().split_once('=')) + .find(|(k, _)| *k == CONNECT_COOKIE) + .map(|(_, v)| v.to_string()) } #[derive(Debug, Deserialize)] @@ -349,7 +363,11 @@ pub struct FinishQuery { /// Arrival is NOT proof of registration: confirm it (the completion POST flag, or /// a signed `mcp_get_accounts` that returns `Ok`), then mint the authorization /// code and 302 to the client's `redirect_uri` with `code` + the client's `state`. -pub async fn finish(State(store): State, Query(q): Query) -> Response { +pub async fn finish( + State(store): State, + headers: axum::http::HeaderMap, + Query(q): Query, +) -> Response { // Snapshot without holding the lock across the network probe. let snap = { let authz = store.authz.read().await; @@ -362,15 +380,26 @@ pub async fn finish(State(store): State, Query(q): Query a.redirect_uri.clone(), a.client_state.clone(), a.code_challenge.clone(), + a.cookie.clone(), ) }) }; - let Some((expired, live_flag, existing_code, client_id, redirect_uri, client_state, code_challenge)) = snap else { + let Some((expired, live_flag, existing_code, client_id, redirect_uri, client_state, code_challenge, cookie)) = snap + else { return connect_error("unknown or already-used connect request — restart from your client"); }; if expired { return connect_error("connect request expired — restart from your client"); } + // H3: only the browser that STARTED this flow (and holds the binding cookie) + // may complete it. A constant-time compare isn't warranted — the value is a + // fresh 122-bit UUID, unguessable and single-use — but the check itself is + // essential: it defeats the phish-your-own-link session-fixation takeover. + if connect_cookie(&headers).as_deref() != Some(cookie.as_str()) { + return connect_error( + "this sign-in was started in a different browser session — restart the connection from your client", + ); + } // Idempotent: if the code was already minted, just redirect again. if let Some(code) = existing_code { return redirect_302(&build_redirect(&redirect_uri, &code, &client_state)); @@ -449,108 +478,6 @@ fn build_redirect(redirect_uri: &str, code: &str, client_state: &str) -> String r } -// ---- Device authorization: start the II handshake ----------------------- - -#[derive(Debug, Deserialize)] -pub struct DeviceAuthzForm { - #[serde(default)] - client_id: Option, - #[serde(default)] - scope: Option, - #[serde(default)] - code_challenge: Option, - #[serde(default)] - code_challenge_method: Option, -} - -/// POST /oauth/device_authorization (RFC 8628 §3.1–3.2) — mint a device code and -/// point the user at the verification URI that launches II's `/mcp` handshake. -pub async fn device_authorization(State(store): State, Form(req): Form) -> Response { - // We only verify PKCE with S256; reject any other method up front, and - // require a challenge whenever a method is named. - if let Some(method) = &req.code_challenge_method { - if method != "S256" { - return oauth_err(StatusCode::BAD_REQUEST, "invalid_request", "only code_challenge_method=S256 is supported"); - } - if req.code_challenge.is_none() { - return oauth_err(StatusCode::BAD_REQUEST, "invalid_request", "code_challenge required when code_challenge_method is set"); - } - } - - let session_id = format!("sess-{}", Uuid::new_v4()); - let device_code = format!("dc-{}", Uuid::new_v4()); - let user_code = Uuid::new_v4().simple().to_string()[..8].to_uppercase(); - - store.devices.write().await.insert( - session_id.clone(), - DeviceAuth { - device_code: device_code.clone(), - user_code: user_code.clone(), - session_id: session_id.clone(), - client_id: req.client_id.clone(), - scope: req.scope.clone().filter(|s| !s.is_empty()), - code_challenge: req.code_challenge.clone(), - created: Instant::now(), - live: false, - last_poll: None, - }, - ); - - let verification_uri = format!("{}{}/oauth/device", base_url(), store.instance().oauth_prefix); - let verification_uri_complete = format!("{verification_uri}?user_code={user_code}"); - Json(json!({ - "device_code": device_code, - "user_code": user_code, - "verification_uri": verification_uri, - "verification_uri_complete": verification_uri_complete, - "expires_in": CONNECT_TTL.as_secs(), - "interval": POLL_INTERVAL_SECS, - })) - .into_response() -} - -#[derive(Debug, Deserialize)] -pub struct DeviceVerifyQuery { - #[serde(default)] - user_code: Option, -} - -/// GET /oauth/device — the user opens this (via `verification_uri_complete`). -/// With a valid `user_code` it launches II's `/mcp` handshake; otherwise it -/// offers a minimal form to enter the code. -pub async fn device_verify(State(store): State, Query(q): Query) -> Response { - let Some(user_code) = q.user_code.filter(|c| !c.is_empty()) else { - return device_code_form(store.instance().oauth_prefix); - }; - let user_code = user_code.trim().to_uppercase(); - - let session_id = { - let devices = store.devices.read().await; - devices - .values() - .find(|d| d.user_code == user_code && d.created.elapsed() < CONNECT_TTL) - .map(|d| d.session_id.clone()) - }; - match session_id { - Some(sid) => js_redirect(&ii_mcp_url(store.instance(), &sid)), - None => connect_error("unknown or expired code — restart the connection from your client"), - } -} - -/// A minimal manual code-entry page for the bare `verification_uri`. -fn device_code_form(prefix: &str) -> Response { - Html(format!( - "\ -

Connect Internet Identity

\ -
\ -

Enter the code shown by your client:

\ - \ - \ -
" - )) - .into_response() -} - /// Build an instance's II `/mcp` handshake URL for a connection. Everything is in /// the URL fragment (never sent to II's servers): this instance's callback on our /// origin, the single-use `state` (= session id), and the requested grant `ttl` @@ -576,12 +503,18 @@ pub struct ConnectCallback { /// the epoch (u64 ns overflows JSON numbers, so it is a string on the wire). #[serde(default)] expiration: Option, + /// Present on the completion notification (§0): `"queries"` = read-only + /// session, `"all"` = full access. Lets us learn the access level at connect + /// without minting a probe delegation; best-effort like the rest of the POST. + #[serde(default)] + permissions: Option, } /// POST /oauth/connect/callback — II's frontend makes TWO cross-origin JSON /// POSTs here, distinguished by the `expiration` field: -/// (a) key request `{state}` → 200 `{public_key}` (fresh session keypair); -/// (b) completion `{state, expiration}` → mark the grant live; any 2xx. +/// (a) key request `{state}` → 200 `{public_key, finish_url}` (fresh keypair); +/// (b) completion `{state, expiration, permissions}` → mark the grant live and +/// record the access level; any 2xx. /// Never returns a redirect (the response is consumed by `fetch()`), and never /// receives or verifies a delegation chain. pub async fn connect_callback(State(store): State, Json(body): Json) -> Response { @@ -589,28 +522,25 @@ pub async fn connect_callback(State(store): State, Json(body): Json { - let is_authcode = store.connect_known_authz(&body.state).await; - if !is_authcode && !store.connect_known_device(&body.state).await { + if !store.connect_known_authz(&body.state).await { return (StatusCode::FORBIDDEN, Json(json!({ "error": "invalid_state" }))).into_response(); } let public_key = store.identities.session_pubkey_b64(&body.state).await; - let mut resp = json!({ "public_key": public_key }); - if is_authcode { - resp["finish_url"] = json!(format!( - "{}{}/oauth/finish?id={}", - base_url(), - store.instance().oauth_prefix, - urlencoding::encode(&body.state) - )); - } - (StatusCode::OK, Json(resp)).into_response() + let finish_url = format!( + "{}{}/oauth/finish?id={}", + base_url(), + store.instance().oauth_prefix, + urlencoding::encode(&body.state) + ); + (StatusCode::OK, Json(json!({ "public_key": public_key, "finish_url": finish_url }))).into_response() } // (b) Completion notification — best-effort. Mark the grant live if the - // connection is still known; tolerate a missing/expired state (e.g. the + // connection is still known, and record the session's access level + // (`permissions`, §0/H2) so tools can warn before attempting an update + // under a read-only session. Tolerate a missing/expired state (e.g. the // grant was already consumed via the signed-call fallback) with a 2xx, so // a late completion POST doesn't make II treat an otherwise-successful // connect as failed. Never create a session for an unknown state. @@ -621,6 +551,9 @@ pub async fn connect_callback(State(store): State, Json(body): Json store.identities.set_grant_expiration(&body.state, exp_ns).await, Err(_) => tracing::warn!("connect completion had unparseable expiration"), } + if let Some(permissions) = &body.permissions { + store.identities.set_permissions(&body.state, permissions).await; + } } StatusCode::NO_CONTENT.into_response() } @@ -637,28 +570,14 @@ impl AuthStore { .is_some_and(|a| a.created.elapsed() < CONNECT_TTL) } - /// Whether `state` names a known, unexpired device pending connect. - async fn connect_known_device(&self, state: &str) -> bool { - self.devices - .read() - .await - .get(state) - .is_some_and(|d| d.created.elapsed() < CONNECT_TTL) - } - - /// Mark the pending connect live in whichever flow owns `state`; returns - /// whether it was known. + /// Mark the pending connect live; returns whether `state` was known. async fn mark_live(&self, state: &str) -> bool { - let mut known = false; if let Some(a) = self.authz.write().await.get_mut(state) { a.live = true; - known = true; - } - if let Some(d) = self.devices.write().await.get_mut(state) { - d.live = true; - known = true; + true + } else { + false } - known } } @@ -689,7 +608,7 @@ fn connect_error(message: &str) -> Response { .into_response() } -// ---- Token: exchange a code, or poll a device grant --------------------- +// ---- Token: exchange an authorization code ------------------------------ #[derive(Debug, Deserialize)] pub struct TokenForm { @@ -697,20 +616,17 @@ pub struct TokenForm { #[serde(default)] code: String, #[serde(default)] - device_code: String, - #[serde(default)] client_id: String, #[serde(default)] code_verifier: Option, } -/// POST /oauth/token — `authorization_code` (poll-bridge clients) or the RFC 8628 -/// `device_code` grant. +/// POST /oauth/token — the `authorization_code` grant (the only grant we support; +/// the RFC 8628 device grant was dropped). pub async fn token(State(store): State, Form(req): Form) -> Response { match req.grant_type.as_str() { "authorization_code" => token_authorization_code(store, req).await, - g if g == DEVICE_GRANT_TYPE => token_device_code(store, req).await, - _ => oauth_err(StatusCode::BAD_REQUEST, "unsupported_grant_type", "authorization_code or device_code"), + _ => oauth_err(StatusCode::BAD_REQUEST, "unsupported_grant_type", "only authorization_code is supported"), } } @@ -734,65 +650,11 @@ async fn token_authorization_code(store: AuthStore, req: TokenForm) -> Response } } store.authz.write().await.remove(&grant.session_id); - issue_token(&store, &grant.session_id, None).await -} - -async fn token_device_code(store: AuthStore, req: TokenForm) -> Response { - if req.device_code.is_empty() { - return oauth_err(StatusCode::BAD_REQUEST, "invalid_request", "device_code required"); - } - let (session_id, code_challenge, scope) = { - let mut devices = store.devices.write().await; - let Some(d) = devices.values_mut().find(|d| d.device_code == req.device_code) else { - return oauth_err(StatusCode::BAD_REQUEST, "invalid_grant", "unknown or used device_code"); - }; - if d.created.elapsed() >= CONNECT_TTL { - return oauth_err(StatusCode::BAD_REQUEST, "expired_token", "device_code expired — restart"); - } - if !req.client_id.is_empty() { - if let Some(cid) = &d.client_id { - if cid != &req.client_id { - return oauth_err(StatusCode::BAD_REQUEST, "invalid_client", "client_id mismatch"); - } - } - } - if let Some(last) = d.last_poll { - if last.elapsed() < Duration::from_secs(POLL_INTERVAL_SECS) { - return oauth_err(StatusCode::BAD_REQUEST, "slow_down", "poll no faster than the interval"); - } - } - d.last_poll = Some(Instant::now()); - (d.session_id.clone(), d.code_challenge.clone(), d.scope.clone()) - }; - - let live_flag = store - .devices - .read() - .await - .values() - .find(|d| d.device_code == req.device_code) - .map(|d| d.live) - .unwrap_or(false); - if !store.grant_live(&session_id, live_flag).await { - return oauth_err(StatusCode::BAD_REQUEST, "authorization_pending", "waiting for Internet Identity"); - } - - if let Some(challenge) = &code_challenge { - let verifier = match &req.code_verifier { - Some(v) => v, - None => return oauth_err(StatusCode::BAD_REQUEST, "invalid_grant", "code_verifier required"), - }; - if &pkce_s256(verifier) != challenge { - return oauth_err(StatusCode::BAD_REQUEST, "invalid_grant", "PKCE verification failed"); - } - } - - store.devices.write().await.retain(|_, d| d.device_code != req.device_code); - issue_token(&store, &session_id, scope).await + issue_token(&store, &grant.session_id).await } /// Mint + store an access token bound to the session key's principal. -async fn issue_token(store: &AuthStore, session_id: &str, scope: Option) -> Response { +async fn issue_token(store: &AuthStore, session_id: &str) -> Response { let principal = store .identities .session_principal(session_id) @@ -811,15 +673,12 @@ async fn issue_token(store: &AuthStore, session_id: &str, scope: Option) ); tracing::info!(%principal, "issued MCP access token"); - let mut resp = json!({ + Json(json!({ "access_token": access_token, "token_type": "Bearer", "expires_in": TOKEN_TTL.as_secs(), - }); - if let Some(scope) = scope { - resp["scope"] = json!(scope); - } - Json(resp).into_response() + })) + .into_response() } fn pkce_s256(verifier: &str) -> String { @@ -840,7 +699,7 @@ pub struct RegisterRequest { } /// POST /oauth/register (RFC 7591). `redirect_uris` are stored for the auth-code -/// flow; a device-only client may register without any. +/// flow — the only grant we support. pub async fn register(State(store): State, Json(req): Json) -> Response { let client_id = format!("client-{}", Uuid::new_v4()); let snapshot = { @@ -851,8 +710,8 @@ pub async fn register(State(store): State, Json(req): Json = if req.grant_types.is_empty() { supported.iter().map(|s| s.to_string()).collect() } else { @@ -888,11 +747,10 @@ pub async fn authorization_server_metadata(State(store): State) -> Re Json(json!({ "issuer": issuer, "authorization_endpoint": format!("{issuer}/oauth/authorize"), - "device_authorization_endpoint": format!("{issuer}/oauth/device_authorization"), "token_endpoint": format!("{issuer}/oauth/token"), "registration_endpoint": format!("{issuer}/oauth/register"), "response_types_supported": ["code"], - "grant_types_supported": ["authorization_code", DEVICE_GRANT_TYPE], + "grant_types_supported": ["authorization_code"], "code_challenge_methods_supported": ["S256"], "token_endpoint_auth_methods_supported": ["none"], })) @@ -1072,6 +930,25 @@ mod tests { assert!(no_token.contains(&format!("resource_metadata=\"{meta}\""))); } + /// H3: the binding cookie is extracted by name from the `Cookie` header + /// (among other cookies), and its absence yields `None` — `finish` then + /// refuses to complete a flow the browser didn't start. + #[test] + fn connect_cookie_extracts_named_value() { + use axum::http::{header::COOKIE, HeaderMap, HeaderValue}; + let mut h = HeaderMap::new(); + assert_eq!(super::connect_cookie(&h), None); + h.insert( + COOKIE, + HeaderValue::from_static("other=1; mcp_connect=bind-xyz; last=2"), + ); + assert_eq!(super::connect_cookie(&h).as_deref(), Some("bind-xyz")); + // A different cookie name present but not ours -> None. + let mut h2 = HeaderMap::new(); + h2.insert(COOKIE, HeaderValue::from_static("session=abc")); + assert_eq!(super::connect_cookie(&h2), None); + } + #[test] fn build_redirect_encodes_code_and_state() { let r = build_redirect("https://claude.ai/cb", "mcp-code-1", "abc/def"); diff --git a/src/identities.rs b/src/identities.rs index 6350ecf..3e58838 100644 --- a/src/identities.rs +++ b/src/identities.rs @@ -72,6 +72,17 @@ const II_CANISTER_ID_PROD_DEFAULT: &str = "rdmx6-jaaaa-aaaaa-aaadq-cai"; const RECONNECT_MSG: &str = "Your Internet Identity session is over (the grant expired, was revoked, \ or was replaced by a newer connection). Reconnect with Internet Identity to continue — do not retry."; +/// Shown when a tool needs update (write) access but the II session is read-only +/// (`permissions = "queries"` — the default when the user just clicks "Allow" on +/// II's consent screen, which defaults to read-only). The IC rejects update calls +/// made through a read-only session's delegations at ingress, which would +/// otherwise surface as an opaque low-level rejection — so we stop early with an +/// actionable message (H2). +const READ_ONLY_MSG: &str = "This Internet Identity session is read-only, so it can't make changes. \ + Creating, installing, starting, stopping, or deleting canisters — and even reading canister status \ + — are update calls that a read-only session can't make. Reconnect with Internet Identity and turn \ + OFF the read-only option on the consent screen, then try again."; + /// One Internet Identity instance this server can connect users against. The /// default ("beta") instance serves `/mcp` with its OAuth AS at the root of /// `PUBLIC_URL`; the "prod" instance serves `/mcp-prod` with a path-scoped AS @@ -137,13 +148,23 @@ fn now_ns() -> u64 { /// Remap a domain to the `target_origin` II expects for account derivation. /// IC gateway domains (`*.icp0.io`, `*.icp.net`) map to the canonical -/// `*.ic0.app` origin; any other domain is passed through as `https://`. +/// `*.ic0.app` origin; any other domain is passed through as `https://`. +/// +/// II derives the app principal from an anchored regex on a bare origin, so we +/// first reduce the input to its authority (`host[:port]`): drop the scheme, then +/// anything from the first path/query/fragment separator, then a redundant `:443` +/// (the default HTTPS port). Without this, a `target_origin` carrying a path, +/// `:443`, or a trailing slash would fall through un-remapped and derive a +/// DIFFERENT principal (H1). Note this only replicates II's *domain-based* +/// derivation — a dapp declaring a custom derivation origin via +/// `/.well-known/ii-alternative-origins` is NOT handled here (a known limitation). fn target_origin(domain: &str) -> String { let host = domain .trim() .trim_start_matches("https://") .trim_start_matches("http://"); - let host = host.split('/').next().unwrap_or(host); + let host = host.split(['/', '?', '#']).next().unwrap_or(host); + let host = host.strip_suffix(":443").unwrap_or(host); for gateway in [".icp0.io", ".icp.net"] { if let Some(label) = host.strip_suffix(gateway) { return format!("https://{label}.ic0.app"); @@ -167,6 +188,12 @@ struct Session { /// `Unauthorized` from a signed call is the authoritative "session over" /// signal. grant_expiration_ns: Option, + /// The session's access level from the completion POST's `permissions` field + /// (§0/H2): `Some(true)` = read-only (`"queries"`), `Some(false)` = full + /// (`"all"`), `None` = not yet learned (the best-effort POST didn't arrive). + /// Update calls are rejected by the IC at ingress under a read-only session, + /// so tools consult this to fail early with an actionable message. + read_only: Option, /// `(domain, account_number)` -> most recently derived per-app delegation. /// Keyed by account too, since each account at an origin signs as a distinct /// principal (`account_number == None` is that origin's default account). @@ -192,8 +219,8 @@ impl AppDelegation { /// One of the user's Internet Identity accounts at an app origin, as returned by /// [`Identities::list_accounts`] (II's `mcp_get_accounts`). Each account is a -/// distinct per-origin principal; `account_number == None`/`name == None` is the -/// origin's default ("synthetic") account, which every user has automatically. +/// distinct per-origin principal; `account_number == None`/`name == None` selects +/// the anchor's current (user-controllable) default account at that origin. pub struct AccountInfo { /// II account number — `None` for the origin's default account. Pass the /// account's `name` to the acting tools to use a non-default account; the @@ -233,6 +260,7 @@ impl Identities { key_seed, pubkey_der, grant_expiration_ns: None, + read_only: None, app_delegations: HashMap::new(), } }); @@ -274,6 +302,37 @@ impl Identities { } } + /// Record the session's access level from the completion POST's `permissions` + /// field (§0/H2): `"queries"` (case-insensitive) = read-only; anything else + /// (`"all"`) = full access. Same vocabulary as the delegation's `permissions`. + pub async fn set_permissions(&self, session_id: &str, permissions: &str) { + self.ensure_session(session_id).await; + let mut sessions = self.sessions.write().await; + if let Some(s) = sessions.get_mut(session_id) { + s.read_only = Some(permissions.trim().eq_ignore_ascii_case("queries")); + } + } + + /// This session's known access level: `Some(true)` = read-only, `Some(false)` + /// = full, `None` = not yet learned (the best-effort completion POST didn't + /// arrive). Surfaced by `get_principal` so the agent can set expectations. + pub async fn is_read_only(&self, session_id: &str) -> Option { + let sessions = self.sessions.read().await; + sessions.get(session_id).and_then(|s| s.read_only) + } + + /// Guard a tool that needs update (write) access (H2): error early with an + /// actionable "reconnect with read-only off" message if the session is KNOWN + /// to be read-only. An unknown level (`None`) passes — we can't be sure the + /// POST simply didn't arrive, so the call proceeds and the IC's ingress + /// rejection (if any) surfaces as the fallback signal. + pub async fn require_write(&self, session_id: &str) -> Result<(), String> { + if self.is_read_only(session_id).await == Some(true) { + return Err(READ_ONLY_MSG.to_string()); + } + Ok(()) + } + /// Build the plain session-key identity (`BasicIdentity`) the server signs /// II's `mcp_*` calls with. Errors early if the stored grant expiration has /// passed (a missing expiration is not treated as expired — the spec says to @@ -315,9 +374,10 @@ impl Identities { /// List the user's Internet Identity accounts at an app `domain`, via II's /// `mcp_get_accounts(target_origin)` signed as the session key. II recovers /// the anchor from the caller (the registered session-key principal), so no - /// anchor number is needed. Every user has a default ("synthetic") account - /// (`account_number == None`, no name) at any origin, plus any named accounts - /// they created there; each is a distinct per-origin principal. + /// anchor number is needed. Every user has a default account (`account_number + /// == None`, no name) at any origin — the anchor's current, user-controllable + /// default there — plus any named accounts they created; each is a distinct + /// per-origin principal. pub async fn list_accounts( &self, session_id: &str, @@ -352,9 +412,9 @@ impl Identities { } /// Whether the grant is currently usable: a signed `mcp_get_accounts` for the - /// MCP origin succeeds. Used by the device-flow poll as a best-effort "the - /// user has finished connecting" signal, since the completion-notification - /// POST is best-effort and may never arrive. + /// MCP origin succeeds. A best-effort fallback used at `/oauth/finish` to + /// confirm the user has finished connecting, since the completion-notification + /// POST (which also confirms this) is best-effort and may never arrive. pub async fn grant_is_live(&self, session_id: &str) -> bool { self.list_accounts(session_id, &crate::auth::base_url()) .await @@ -372,7 +432,7 @@ impl Identities { name: Option<&str>, ) -> Result, String> { let Some(name) = name else { - return Ok(None); // default ("synthetic") account + return Ok(None); // the anchor's current (user-controllable) default account }; let accounts = self.list_accounts(session_id, domain).await?; let mut matching = accounts.iter().filter(|a| a.name.as_deref() == Some(name)); @@ -661,6 +721,42 @@ mod tests { assert_eq!(target_origin("http://oisy.com"), "https://oisy.com"); } + /// H1a: a `target_origin` carrying `:443`, a path, a trailing slash, a query, + /// or a fragment must normalize to the bare `https://` origin — else II + /// derives a DIFFERENT principal. The gateway remap still applies afterwards. + #[test] + fn target_origin_normalizes_port_path_and_slash() { + assert_eq!(target_origin("https://oisy.com:443"), "https://oisy.com"); + assert_eq!(target_origin("https://oisy.com/"), "https://oisy.com"); + assert_eq!(target_origin("https://oisy.com/app/x"), "https://oisy.com"); + assert_eq!(target_origin("https://oisy.com?a=1"), "https://oisy.com"); + assert_eq!(target_origin("https://oisy.com#frag"), "https://oisy.com"); + assert_eq!(target_origin("https://oisy.com:443/app/x"), "https://oisy.com"); + // The gateway remap still fires once :443 / path is stripped. + assert_eq!(target_origin("foo.icp0.io:443"), "https://foo.ic0.app"); + assert_eq!(target_origin("https://foo.icp0.io/app"), "https://foo.ic0.app"); + // A non-default port is part of the origin and is kept. + assert_eq!(target_origin("https://localhost:8080"), "https://localhost:8080"); + } + + /// H2: `permissions` from the completion POST sets the read-only level, which + /// `require_write` gates on. Unknown (POST not yet arrived) permits the write + /// (we fall back to the IC's ingress rejection); `"queries"` refuses it. + #[tokio::test] + async fn permissions_set_read_only_and_gate_writes() { + let ids = test_ids(); + ids.ensure_session("sess").await; + assert_eq!(ids.is_read_only("sess").await, None); + assert!(ids.require_write("sess").await.is_ok()); + ids.set_permissions("sess", "queries").await; + assert_eq!(ids.is_read_only("sess").await, Some(true)); + assert!(ids.require_write("sess").await.is_err()); + // Case-insensitive, and "all" restores full access. + ids.set_permissions("sess", "ALL").await; + assert_eq!(ids.is_read_only("sess").await, Some(false)); + assert!(ids.require_write("sess").await.is_ok()); + } + // An Identities store over a dummy II instance (tests never hit the network). fn test_ids() -> Identities { Identities::new(IiInstance { @@ -762,7 +858,7 @@ mod tests { let bytes = Encode!(&wire).expect("encode"); let decoded = Decode!(&bytes, McpGetAccountsReply).expect("decode").expect("Ok arm"); assert_eq!(decoded.len(), 2); - // Default (synthetic) account: no number, no name. + // Default account (the anchor's current default): no number, no name. assert_eq!(decoded[0].account_number, None); assert_eq!(decoded[0].name, None); // Named account: number, name, and last_used recovered (origin ignored). diff --git a/src/main.rs b/src/main.rs index f94f04e..f70b8e3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -263,7 +263,7 @@ impl IcTools { } #[tool( - description = "Get the Internet Computer principal you act as at a given application `domain` (e.g. \"oisy.com\"), without making a canister call. The app's account delegation is derived on demand (same as call_canister) from this connection's standing Internet Identity credential, and its principal is returned. By default this resolves the app's default account; pass `account` (an account name from list_accounts) for a specific named account there. Use this when a flow needs the principal itself (e.g. to look up a balance or account) rather than to invoke a method." + description = "Get the Internet Computer principal you act as at a given application `domain` (e.g. \"oisy.com\"), without making a canister call. The app's account delegation is derived on demand (same as call_canister) from this connection's standing Internet Identity credential, and its principal is returned. By default this resolves the app's default account; pass `account` (an account name from list_accounts) for a specific named account there. Use this when a flow needs the principal itself (e.g. to look up a balance or account) rather than to invoke a method. NOTE: the principal is derived from the app's DOMAIN, which is usually — but not always — the identity a browser sign-in to that app would use. Some apps declare a CUSTOM derivation origin (via /.well-known/ii-alternative-origins) that isn't exposed here; if the returned principal (or an account/balance) doesn't match what the user sees in their browser at that app, tell them so and offer to look up the app's ii-alternative-origins (web search / fetch) and retry." )] async fn get_principal( &self, @@ -283,13 +283,25 @@ impl IcTools { Err(e) => return Ok(err(e)), }; match delegated.sender() { - Ok(p) => Ok(ok(p.to_text())), + Ok(p) => { + let mut out = p.to_text(); + // Surface a read-only session (H2) so the LLM won't attempt (and + // have the IC reject at ingress) canister-management updates. + if self.identities.is_read_only(&session_id).await == Some(true) { + out.push_str( + "\n\n(This Internet Identity session is READ-ONLY: reads work, but canister \ + management — create/install/start/stop/delete, and canister_status — needs \ + update access. Ask the user to reconnect with the read-only option turned OFF.)", + ); + } + Ok(ok(out)) + } Err(e) => Ok(err(format!("could not derive principal for '{domain}': {e}"))), } } #[tool( - description = "List the user's Internet Identity accounts at an application `domain` (e.g. \"oisy.com\"). Internet Identity gives the user a distinct principal per app, and within an app they may hold several accounts: a default (\"synthetic\") account everyone gets automatically, plus any named accounts they created. Use this before acting on the user's behalf at an app: if there's only the default account, just proceed (call_canister/get_principal with no `account`); if there are several, pick one with the user (or act on each) by passing its name as `account`. Returns each account's name (the default has none), account number, and last-used time. Requires an authenticated session." + description = "List the user's Internet Identity accounts at an application `domain` (e.g. \"oisy.com\"). Internet Identity gives the user a distinct principal per app (derived from the app's domain), and within an app they may hold several accounts: a default account everyone gets automatically (the anchor's current, user-controllable default at that origin), plus any named accounts they created. Use this before acting on the user's behalf at an app: if there's only the default account, just proceed (call_canister/get_principal with no `account`); if there are several, pick one with the user (or act on each) by passing its name as `account`. Returns each account's name (the default has none), account number, and last-used time. If these accounts don't match what the user sees in their browser at this app, it may use a custom derivation origin not exposed here (offer to look up its ii-alternative-origins and retry). Requires an authenticated session." )] async fn list_accounts( &self, @@ -671,7 +683,7 @@ fn authed_session(ctx: &RequestContext) -> Option, @@ -729,8 +741,17 @@ impl ServerHandler for IcTools { balance or account). A user may hold several accounts at an app (a default one plus \ named ones); `list_accounts(domain)` lists them, and call_canister/get_principal take \ an optional `account` (a name from that list) to act as a specific one — omit it for \ - the default account. The standing credential is obtained when you connect \ - (authenticate via Internet Identity) and lasts ~60 minutes; reconnect when it expires.\n\n\ + the default account. The per-app principal is derived from the app's DOMAIN — usually, \ + but NOT always, the same identity a browser sign-in to that app would use: some apps \ + declare a custom derivation origin (via /.well-known/ii-alternative-origins) not \ + exposed here. If a principal, account, or balance doesn't match what the user sees in \ + their browser at that app, say so and offer to look up the app's ii-alternative-origins \ + (web search / fetch) and retry. The standing credential is obtained when you connect \ + (authenticate via Internet Identity) and lasts ~60 minutes; reconnect when it expires. \ + The session may be READ-ONLY (Internet Identity's consent screen defaults to read-only): \ + reads work, but the canister-management tools below make update calls the network \ + rejects for a read-only session — if one fails that way, ask the user to reconnect with \ + the read-only option turned OFF.\n\n\ To AUTHOR, BUILD and DEPLOY IC code, first consult the official IC skills: \ `list_ic_skills` lists them and `get_ic_skill(name)` loads one. Especially `motoko` \ (language), `mops-cli` (deps/build), `icp-cli` (build & deploy), `cycles-management` \ @@ -854,7 +875,7 @@ fn format_accounts(domain: &str, accounts: &[identities::AccountInfo]) -> String } let mut out = format!("Your accounts at {domain}:\n"); for a in accounts { - // The default ("synthetic") account has no name and no account number. + // The default account (anchor's current default) has no name/number. let label = match &a.name { Some(name) => format!("\"{name}\""), None => "(default account — no name)".to_string(), @@ -1020,8 +1041,6 @@ async fn main() -> anyhow::Result<()> { axum::Router::new() .route("/oauth/authorize", axum::routing::get(auth::authorize)) .route("/oauth/finish", axum::routing::get(auth::finish)) - .route("/oauth/device_authorization", axum::routing::post(auth::device_authorization)) - .route("/oauth/device", axum::routing::get(auth::device_verify)) .route("/oauth/connect/callback", axum::routing::post(auth::connect_callback)) .route("/oauth/token", axum::routing::post(auth::token)) .route("/oauth/register", axum::routing::post(auth::register)) diff --git a/src/management.rs b/src/management.rs index 626882f..008354b 100644 --- a/src/management.rs +++ b/src/management.rs @@ -176,6 +176,7 @@ pub async fn create_canister( session_id: &str, args: CreateCanisterArgs, ) -> Result { + ids.require_write(session_id).await?; let (agent, principal) = management_agent(ids, session_id).await?; let cycles = resolve_cycles(&agent, args.icp.as_deref(), args.cycles).await?; @@ -235,6 +236,7 @@ pub async fn top_up_canister( session_id: &str, args: TopUpArgs, ) -> Result { + ids.require_write(session_id).await?; let target = parse_principal(&args.canister_id)?; let (agent, _) = management_agent(ids, session_id).await?; let cycles = resolve_cycles(&agent, args.icp.as_deref(), args.cycles).await?; @@ -263,6 +265,7 @@ pub async fn install_code( session_id: &str, args: InstallCodeArgs, ) -> Result { + ids.require_write(session_id).await?; let target = parse_principal(&args.canister_id)?; let wasm = decode_wasm(&args)?; if wasm.is_empty() { @@ -304,6 +307,9 @@ pub async fn canister_status( session_id: &str, args: CanisterRefArgs, ) -> Result { + // canister_status is an UPDATE call (controller-gated), so a read-only + // session can't make it — fail early with an actionable message (H2). + ids.require_write(session_id).await?; let target = parse_principal(&args.canister_id)?; let (agent, _) = management_agent(ids, session_id).await?; let arg = Encode!(&CanisterIdRecord { @@ -321,6 +327,7 @@ pub async fn update_canister_settings( session_id: &str, args: UpdateSettingsArgs, ) -> Result { + ids.require_write(session_id).await?; let target = parse_principal(&args.canister_id)?; let mut settings = CanisterSettings::default(); if let Some(cs) = &args.controllers { @@ -428,13 +435,14 @@ async fn update_call( .map_err(|e| format!("{method} failed: {e}")) } -/// Shared body for the no-payload lifecycle methods. +/// Shared body for the no-payload lifecycle methods (all UPDATE calls). async fn lifecycle( ids: &Identities, session_id: &str, canister_id: &str, method: &str, ) -> Result<(), String> { + ids.require_write(session_id).await?; let target = parse_principal(canister_id)?; let (agent, _) = management_agent(ids, session_id).await?; let arg = Encode!(&CanisterIdRecord { From cf8af6363d8294f1a8ed074da77a62398f8547d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 22:19:35 +0000 Subject: [PATCH 2/5] H3: document the cookie as a partial mitigation, not a complete fix An adversarial review found the browser-binding cookie does not fully close the session-fixation takeover it targets: because /oauth/authorize requires no auth, the attacker can be the flow initiator and thus holds the cookie. After phishing only the II link to a victim who consents (registering the session key under the victim's anchor), the attacker completes /oauth/finish themselves with their own cookie and redeems the code as the victim. The cookie only stops the zero-click variant (the victim's browser passively delivering the code); consent happens cross-origin at II keyed by the shared state, so the server can't tie "who receives the code" to "who consented". Keep the cookie as defense-in-depth (it eliminates the zero-click variant) but correct the overstated "closes the takeover" claims in the module docs, the AuthzPending::cookie field, the authorize/finish comments, and the README to describe it accurately as a partial mitigation, with the residual risk and the note that a complete fix needs an II-side control identifying the client (not just the origin). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- README.md | 25 ++++++++++++++++++++----- src/auth.rs | 37 +++++++++++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 6687156..6a26e87 100644 --- a/README.md +++ b/README.md @@ -222,11 +222,26 @@ acting as the victim. (The "arrival ≠ registration" check guards a different property; it doesn't bind the initiator to the consenting user, and II can't help — its trust model only ever identifies the origin.) -The fix: `/oauth/authorize` sets an unguessable, single-use, `HttpOnly` `Secure` -`SameSite=Lax` cookie (scoped to the instance's `…/oauth` path) and `/oauth/finish` -requires it before minting the code — so only the browser that **started** the -flow can complete it. `SameSite=Lax` still rides the top-level cross-site GET II -uses to navigate back to `finish_url`. +The mitigation: `/oauth/authorize` sets an unguessable, single-use, `HttpOnly` +`Secure` `SameSite=Lax` cookie (scoped to the instance's `…/oauth` path) and +`/oauth/finish` requires it before minting the code — so only the browser that +**started** the flow can complete it. `SameSite=Lax` still rides the top-level +cross-site GET II uses to navigate back to `finish_url`. + +> ⚠️ **This is a partial mitigation, not a complete fix — a residual takeover +> path remains** (found by an adversarial review of this change). Because +> `/oauth/authorize` requires no authentication, the *attacker* can be the flow +> initiator: they call authorize themselves and keep **both** the cookie (from +> their own response) and the `state` (from the II link), phish only the II link, +> and after the victim consents they complete `/oauth/finish` **themselves** with +> their cookie and redeem the code as the victim. The cookie only stops the +> *zero-click* variant (the victim's browser passively delivering the code); it +> can't tie "who receives the code" to "who consented," since consent happens +> cross-origin at II keyed by the shared `state`. A complete fix needs an II-side +> control that identifies the requesting **client**, not just the origin (which II +> doesn't currently provide); reducing/vetting DCR shrinks but doesn't close it. +> The cookie is kept as defense-in-depth. This gap is flagged back for the design +> review. ### Read-only sessions diff --git a/src/auth.rs b/src/auth.rs index 9d860f1..2589f0e 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -26,6 +26,23 @@ //! //! Implemented: dynamic client registration, PKCE (S256) enforced, short-lived //! codes, 1h access tokens, session-key-bound principal. +//! +//! ## Known residual risk (H3 is only a partial mitigation) +//! +//! The browser-binding cookie stops the *zero-click* session-fixation variant (a +//! phished victim's browser passively delivering the code to the attacker's +//! `redirect_uri`), but it does NOT fully prevent the account takeover. Because +//! `/oauth/authorize` requires no authentication, the ATTACKER can be the flow +//! initiator (open DCR + their own `redirect_uri`/PKCE): they call authorize, +//! keep both the cookie and the `state` from the II link, phish only the II link +//! to a victim, and after the victim consents (registering the session key under +//! the victim's anchor) they complete `/oauth/finish` themselves with their cookie +//! and redeem the code as the victim. This is structural: consent happens +//! cross-origin at II keyed by the shared `state`, so the server cannot tie "who +//! receives the code" to "who consented" — the cookie only proves initiator == +//! finisher, and the attacker is the initiator. A complete fix needs an II-side +//! control that identifies the requesting client (not just the origin), which II +//! does not currently provide; reducing/vetting DCR shrinks but does not close it. use std::{ collections::HashMap, @@ -166,8 +183,13 @@ struct AuthzPending { code_challenge: Option, /// Unguessable value set as a browser cookie at `/oauth/authorize` and /// required (matched) at `/oauth/finish` (H3) — so only the browser that - /// STARTED this flow can complete it, closing the session-fixation takeover - /// where an attacker phishes their own connect link to a trusting victim. + /// STARTED this flow can complete it. This is a PARTIAL mitigation of the + /// session-fixation takeover: it stops the zero-click variant (a phished + /// victim's browser auto-delivering the code to the attacker's redirect_uri), + /// but NOT a variant where the attacker — being the flow initiator, since + /// `/oauth/authorize` needs no auth — holds this cookie and completes finish + /// themselves after the victim consents. A full fix needs an II-side control + /// that identifies the client, not just the origin (see the module docs). cookie: String, created: Instant, /// Grant confirmed live (completion POST or a signed-call fallback). @@ -306,8 +328,9 @@ pub async fn authorize(State(store): State, Query(q): Query Date: Sat, 4 Jul 2026 22:39:00 +0000 Subject: [PATCH 3/5] H2: leave access level unknown for unrecognized permissions values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set_permissions treated any non-"queries" value as full access (Some(false)). For an unrecognized/future permissions value that wrongly asserts "known writable" instead of leaving the level unknown, which — once the deferred delegation-probe fallback keys on None — would suppress the fallback and re-surface opaque ingress errors. Only accept the known vocabulary ("queries" => read-only, "all" => full); leave the level None (unknown) otherwise, with a warning log. Add a test. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- src/identities.rs | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/identities.rs b/src/identities.rs index 3e58838..05ec7f8 100644 --- a/src/identities.rs +++ b/src/identities.rs @@ -303,13 +303,25 @@ impl Identities { } /// Record the session's access level from the completion POST's `permissions` - /// field (§0/H2): `"queries"` (case-insensitive) = read-only; anything else - /// (`"all"`) = full access. Same vocabulary as the delegation's `permissions`. + /// field (§0/H2), using the same vocabulary as the delegation's `permissions`: + /// `"queries"` = read-only, `"all"` = full access (both case-insensitive). Any + /// UNRECOGNIZED value leaves the level `None` (unknown) rather than assuming + /// full access — so an unexpected/future value falls through to the ingress + /// rejection fallback instead of the server wrongly considering the session + /// writable. pub async fn set_permissions(&self, session_id: &str, permissions: &str) { + let level = match permissions.trim().to_ascii_lowercase().as_str() { + "queries" => Some(true), + "all" => Some(false), + other => { + tracing::warn!("connect completion had unrecognized permissions {other:?}; access level left unknown"); + return; + } + }; self.ensure_session(session_id).await; let mut sessions = self.sessions.write().await; if let Some(s) = sessions.get_mut(session_id) { - s.read_only = Some(permissions.trim().eq_ignore_ascii_case("queries")); + s.read_only = level; } } @@ -755,6 +767,13 @@ mod tests { ids.set_permissions("sess", "ALL").await; assert_eq!(ids.is_read_only("sess").await, Some(false)); assert!(ids.require_write("sess").await.is_ok()); + // An UNRECOGNIZED value must NOT assume full access: it leaves the level + // unknown (None) so the call falls through to the ingress-rejection path, + // rather than the server wrongly reporting the session as writable. + ids.ensure_session("sess2").await; + ids.set_permissions("sess2", "something-new").await; + assert_eq!(ids.is_read_only("sess2").await, None); + assert!(ids.require_write("sess2").await.is_ok()); } // An Identities store over a dummy II instance (tests never hit the network). From 3262a856cce236208a0b65044261277aca732579 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 5 Jul 2026 10:21:23 +0000 Subject: [PATCH 4/5] H3: implement Consent-Bound Completion (close the split-browser injection) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the cookie-only partial mitigation with the reviewed-and-approved Consent-Bound Completion design, which closes the split-browser authorization-code injection for all transports (including loopback). /oauth/finish now mints a code only when the requesting browser proves it is BOTH: 1. the initiator — the `sid` cookie set at /oauth/authorize; and 2. the consenter — a one-time `finish_secret` (256-bit CSPRNG) minted at the key request and disclosed only in that response (embedded in finish_url's query), so only the browser that drove the II handshake holds it; plus proven registration (a signed mcp_get_accounts returning Ok), not a bare completion POST. Because the single-use key request delivers both the finish_secret and the public_key a victim would register, the party that can register as the victim is exactly the party that gets the secret, and the sid cookie pins the finisher to the initiator — so the two proofs co-reside in one browser only in the legitimate same-browser flow. Load-bearing invariants (per the design's P1–P3, P5, P6): - The key request is a single ATOMIC compare-and-set on connect_state: the first claims the PA (stores H(finish_secret)); any later/racing key request 403s. Prevents the TOCTOU double-consume. - finish_secret rides in the query (not a path segment) so path-only access logs don't capture it; /oauth/finish and the propagation-retry page send Referrer-Policy: no-referrer so it can't leak via Referer; stored hashed (SHA-256), constant-time compared. - `registered` is proven by grant_is_live (signed mcp_get_accounts), never by the unauthenticated completion POST (now a latency hint for expiration/permissions only). - The propagation-retry page threads finish_secret across reloads and consumes it only on success. The same-browser variant (a victim driven to a hosted attacker redirect_uri) is out of scope and needs hosted-redirect allow-listing; loopback/native are safe either way. Documented in the module docs + README. Add tests: secret helpers (uniqueness/entropy/constant-time), single-use key request (first mints + claims, replay 403s, secret in query not path), and completion-POST-does-not-materialize (P3). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- README.md | 113 +++++++++------- src/auth.rs | 378 +++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 350 insertions(+), 141 deletions(-) diff --git a/README.md b/README.md index 6a26e87..ccc332c 100644 --- a/README.md +++ b/README.md @@ -174,9 +174,10 @@ callback and then finishes on its own — so a classic authorization-code - `/oauth/authorize` validates the client + redirect and PKCE, sets a **browser-binding cookie** (see below), and redirects to II's handshake. The - key-request response carries a `finish_url` on our origin, so after - `mcp_register` II navigates the browser to `/oauth/finish`, which checks that - cookie, confirms registration, mints a PKCE-bound code, and 302s to the client's + key-request response carries a `finish_url` on our origin (with a one-time + `finish_secret`), so after `mcp_register` II navigates the browser to + `/oauth/finish`, which requires the cookie **and** the secret, confirms + registration, mints a PKCE-bound code, and 302s to the client's `redirect_uri?code=…&state=…`. The **RFC 8628 device grant was dropped**: no listed MCP client uses it, and it @@ -194,54 +195,72 @@ Endpoints: - `GET /oauth/authorize` — validates the client + redirect, requires PKCE, sets the binding cookie, then redirects to II's handshake - `GET /oauth/finish` — II navigates here after registration; requires the - binding cookie, confirms the grant is live, then 302s to `redirect_uri?code=…&state=…` + binding cookie **and** the one-time `finish_secret`, confirms the grant is + registered (proven, see below), then 302s to `redirect_uri?code=…&state=…` - `POST /oauth/connect/callback` — serves II's **two cross-origin JSON POSTs**: - a key request `{state}` → `{public_key, finish_url}` (a fresh session keypair, - minted here), and a completion notification `{state, expiration, permissions}` → - mark the grant live and record the session's access level + a **single-use** key request `{state}` → `{public_key, finish_url}` (a fresh + session keypair + one-time `finish_secret` embedded in `finish_url`; any later + key request for the same `state` gets `403`), and a completion notification + `{state, expiration, permissions}` → record the grant expiry + access level (a + best-effort latency hint only) - `POST /oauth/token` — exchanges an authorization `code` (PKCE) for the access token -"Grant is live" means II's completion POST arrived **or** a signed -`mcp_get_accounts` now succeeds (belt-and-suspenders, since the completion POST is -best-effort). Unauthenticated `/mcp` requests get `401` with a `WWW-Authenticate` -header pointing at the resource metadata, as the MCP spec expects. +"Registered" is proven by a signed `mcp_get_accounts` that returns `Ok` under the +minted session key — **not** by the completion POST, which is unauthenticated and +therefore only a latency hint. Unauthenticated `/mcp` requests get `401` with a +`WWW-Authenticate` header pointing at the resource metadata, as the MCP spec expects. -### Browser-session binding (`/oauth/authorize` → `/oauth/finish`) +### Consent-Bound Completion (`/oauth/authorize` → `/oauth/finish`) The `state` in the II connect link is echoed back to the client in the final redirect, so it **cannot by itself prove** that the browser finishing at -`/oauth/finish` is the one that started at `/oauth/authorize`. Without a binding -there's a session-fixation takeover: an attacker registers a client (open DCR) -with their own `redirect_uri` + PKCE challenge, calls `/oauth/authorize`, reads -the II connect link from the 302, and phishes it to a victim who already trusts -this origin in II Settings (II's consent screen names only the *origin*, never the -OAuth client, so nothing warns the victim). The victim consents, the server would -mint a code for the *attacker's* pending auth and 302 the victim to the attacker's -`redirect_uri`, and the attacker redeems it with their own PKCE verifier — a token -acting as the victim. (The "arrival ≠ registration" check guards a different -property; it doesn't bind the initiator to the consenting user, and II can't help -— its trust model only ever identifies the origin.) - -The mitigation: `/oauth/authorize` sets an unguessable, single-use, `HttpOnly` -`Secure` `SameSite=Lax` cookie (scoped to the instance's `…/oauth` path) and -`/oauth/finish` requires it before minting the code — so only the browser that -**started** the flow can complete it. `SameSite=Lax` still rides the top-level -cross-site GET II uses to navigate back to `finish_url`. - -> ⚠️ **This is a partial mitigation, not a complete fix — a residual takeover -> path remains** (found by an adversarial review of this change). Because -> `/oauth/authorize` requires no authentication, the *attacker* can be the flow -> initiator: they call authorize themselves and keep **both** the cookie (from -> their own response) and the `state` (from the II link), phish only the II link, -> and after the victim consents they complete `/oauth/finish` **themselves** with -> their cookie and redeem the code as the victim. The cookie only stops the -> *zero-click* variant (the victim's browser passively delivering the code); it -> can't tie "who receives the code" to "who consented," since consent happens -> cross-origin at II keyed by the shared `state`. A complete fix needs an II-side -> control that identifies the requesting **client**, not just the origin (which II -> doesn't currently provide); reducing/vetting DCR shrinks but doesn't close it. -> The cookie is kept as defense-in-depth. This gap is flagged back for the design -> review. +`/oauth/finish` is the one that started at `/oauth/authorize` — nor that it is the +one that actually **consented** at II. Without a binding there's a session-fixation +takeover: an attacker registers a client (open DCR) with their own `redirect_uri` + +PKCE challenge, calls `/oauth/authorize`, reads the II connect link from the 302, +and phishes it to a victim who already trusts this origin in II Settings (II's +consent screen names only the *origin*, never the OAuth client, so nothing warns the +victim). The victim consents; the attacker completes the flow and redeems the code +with their own PKCE verifier — a token acting as the victim. (An initiator-only +cookie does *not* close this: the attacker is the initiator, so it holds the cookie.) + +**The fix — Consent-Bound Completion.** `/oauth/finish` mints a code only when the +requesting browser presents **both** proofs, which can co-reside in one browser +only in the legitimate same-browser flow: + +1. **initiator** — an unguessable `HttpOnly; Secure; SameSite=Lax` `sid` cookie + (scoped to the instance's `…/oauth` path) set at `/oauth/authorize`; +2. **consenter** — a one-time `finish_secret` (≥128-bit) minted at the **single-use** + key request and disclosed *only* in that response (embedded in `finish_url`), so + only the browser that drove the II handshake holds it; +3. plus **proven** registration (signed `mcp_get_accounts` = `Ok`), not a bare + completion POST. + +Because the single-use key request delivers *both* the `finish_secret` and the +`public_key` a victim would register, the party that can register as the victim is +exactly the party that gets the secret — and the `sid` cookie pins the finisher to +the initiator. An attacker who initiates then phishes the II link holds `sid` but +never the secret (the victim's key request consumes it); the victim holds the secret +but never `sid`. Neither can finish. This closes the split-browser injection for +**all transports incl. loopback** (a loopback redirect resolves on the consenter's +own machine). `SameSite=Lax` still rides the top-level cross-site GET II uses to +navigate back to `finish_url`. + +Load-bearing invariants (enforced in code): the key request is a single **atomic** +compare-and-set (P1) and II's frontend issues exactly one per connect (a retry +fails as "restart", never a takeover); `finish_secret` rides in the **query** (not a +path segment), is kept out of logs, and `/oauth/finish` sends `Referrer-Policy: +no-referrer` so it can't leak via `Referer` (P2); `registered` reflects a real +on-chain grant (P3). + +> **Companion control (not in this change).** The *same-browser* variant — a victim +> socially engineered into running the whole flow toward an attacker-registered +> **hosted** `redirect_uri` — is not closed by Consent-Bound Completion (the +> victim's browser legitimately holds both proofs). It needs **hosted-redirect +> allow-listing**; loopback/native clients are safe either way (the code resolves on +> the consenter's own machine). So "H3 fully closed" = Consent-Bound Completion + +> hosted-redirect allow-listing — a product decision that trades only against open +> DCR for hosted clients. ### Read-only sessions @@ -399,8 +418,10 @@ delegation. Omitting `account` uses the default account. reference resources. - [x] OAuth 2.1 auth (authorization-code + PKCE): II's `/mcp` handshake registers the connection's session key (two JSON callback POSTs, no delegation chain), - with `/oauth/authorize`→`/oauth/finish` browser-session binding; expiring - tokens. (The RFC 8628 device grant was dropped.) + with **Consent-Bound Completion** binding `/oauth/finish` to both the initiator + (`sid` cookie) and the consenter (one-time `finish_secret`); expiring tokens. + (The RFC 8628 device grant was dropped. Same-browser-variant closure needs + hosted-redirect allow-listing — see Auth.) - [x] On-demand **domain identities**: the registered session key mints per-app account delegations directly via II canister methods (`call_canister`/`get_principal` `domain`); no per-app browser flow. diff --git a/src/auth.rs b/src/auth.rs index 2589f0e..7010f61 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -27,22 +27,37 @@ //! Implemented: dynamic client registration, PKCE (S256) enforced, short-lived //! codes, 1h access tokens, session-key-bound principal. //! -//! ## Known residual risk (H3 is only a partial mitigation) +//! ## H3: Consent-Bound Completion (closes the split-browser code injection) //! -//! The browser-binding cookie stops the *zero-click* session-fixation variant (a -//! phished victim's browser passively delivering the code to the attacker's -//! `redirect_uri`), but it does NOT fully prevent the account takeover. Because -//! `/oauth/authorize` requires no authentication, the ATTACKER can be the flow -//! initiator (open DCR + their own `redirect_uri`/PKCE): they call authorize, -//! keep both the cookie and the `state` from the II link, phish only the II link -//! to a victim, and after the victim consents (registering the session key under -//! the victim's anchor) they complete `/oauth/finish` themselves with their cookie -//! and redeem the code as the victim. This is structural: consent happens -//! cross-origin at II keyed by the shared `state`, so the server cannot tie "who -//! receives the code" to "who consented" — the cookie only proves initiator == -//! finisher, and the attacker is the initiator. A complete fix needs an II-side -//! control that identifies the requesting client (not just the origin), which II -//! does not currently provide; reducing/vetting DCR shrinks but does not close it. +//! `/oauth/finish` mints a code only when the requesting browser proves it is BOTH +//! the **initiator** and the **consenter**: +//! 1. *initiator* — the `sid` cookie ([`CONNECT_COOKIE`]) set at `/oauth/authorize`; +//! 2. *consenter* — a one-time `finish_secret` (≥128-bit) minted at the **single-use +//! key request** and disclosed only in that response (embedded in `finish_url`), +//! so only the browser that drove the II handshake holds it; +//! 3. plus a *proven* registration — a signed `mcp_get_accounts` returning `Ok` +//! (not a bare, unauthenticated completion POST). +//! +//! Because the key request is strictly single-use per `connect_state` (an atomic +//! compare-and-set) and delivers BOTH `finish_secret` and the `public_key` a victim +//! would register, the two proofs can co-reside in one browser only in the +//! legitimate same-browser flow — so an attacker who initiates and then phishes the +//! II link to a victim cannot obtain a code (they hold `sid` but never +//! `finish_secret`; the victim holds `finish_secret` but never `sid`). This closes +//! the split-browser injection for all transports incl. loopback (a loopback +//! redirect resolves on the consenter's own machine). +//! +//! Not closed here (companion control): the *same-browser* variant where a victim +//! is socially engineered into running the whole flow toward an attacker-registered +//! **hosted** `redirect_uri`; that needs hosted-redirect allow-listing (loopback is +//! safe either way). "H3 fully closed" = Consent-Bound Completion + that allow-list. +//! +//! Load-bearing (see the `P?` markers below): the key request is single-use and +//! atomic (P1); `finish_secret` is disclosed only via `finish_url`'s query, never +//! logged, no `Referer` leak (P2); `registered` reflects a real grant, never a bare +//! completion POST (P3). P1 also assumes II's frontend issues exactly one key +//! request per connect and never auto-retries (a retry then fails as "restart", +//! never a takeover — the safe direction). use std::{ collections::HashMap, @@ -86,6 +101,32 @@ pub fn base_url() -> String { std::env::var("PUBLIC_URL").unwrap_or_else(|_| "http://localhost:8000".to_string()) } +/// A fresh consent secret: 256 bits from the OS CSPRNG, URL-safe (base64url, no +/// pad) so it can ride in a query string. Well above the ≥128-bit floor (P5). +fn fresh_secret() -> String { + let mut bytes = [0u8; 32]; + getrandom::fill(&mut bytes).expect("getrandom"); + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes) +} + +/// SHA-256 digest of `s` (used to store `finish_secret` hashed at rest — P5). +fn sha256(s: &str) -> [u8; 32] { + Sha256::digest(s.as_bytes()).into() +} + +/// Constant-time equality for two byte slices (P5) — no early-out on the first +/// differing byte, so a compare doesn't leak how much of a secret matched. +fn ct_eq(a: &[u8], b: &[u8]) -> bool { + if a.len() != b.len() { + return false; + } + let mut diff = 0u8; + for (x, y) in a.iter().zip(b.iter()) { + diff |= x ^ y; + } + diff == 0 +} + /// A registered OAuth client (RFC 7591): the redirect URIs it declared. The /// auth-code flow only redirects a code to one of these (exact match), so the /// server is not an open redirector and needs no hardcoded host allowlist. @@ -181,20 +222,21 @@ struct AuthzPending { /// The OAuth client's own `state`, echoed back on the final redirect. client_state: String, code_challenge: Option, - /// Unguessable value set as a browser cookie at `/oauth/authorize` and - /// required (matched) at `/oauth/finish` (H3) — so only the browser that - /// STARTED this flow can complete it. This is a PARTIAL mitigation of the - /// session-fixation takeover: it stops the zero-click variant (a phished - /// victim's browser auto-delivering the code to the attacker's redirect_uri), - /// but NOT a variant where the attacker — being the flow initiator, since - /// `/oauth/authorize` needs no auth — holds this cookie and completes finish - /// themselves after the victim consents. A full fix needs an II-side control - /// that identifies the client, not just the origin (see the module docs). + /// H3 clause 1 (*initiator* proof): unguessable value set as the `sid` browser + /// cookie at `/oauth/authorize` and matched at `/oauth/finish` — only the + /// browser that STARTED this flow presents it. On its own the cookie is a + /// partial mitigation; combined with `finish_secret_hash` (the *consenter* + /// proof) it closes the split-browser injection (see the module docs). cookie: String, created: Instant, - /// Grant confirmed live (completion POST or a signed-call fallback). - live: bool, - /// The authorization code minted once the grant is live (idempotent finish). + /// H3 clause 2 (*consenter* proof), and the single-use marker for the key + /// request (P1/P3): `None` until the FIRST key request for this connect + /// atomically claims it and stores `H(finish_secret)`; any later key request + /// then 403s. `/oauth/finish` requires a `finish_secret` hashing to this. Its + /// presence also means "the key request happened" — the only way `registered` + /// can legitimately become provable — so no alternate path materializes it. + finish_secret_hash: Option<[u8; 32]>, + /// The authorization code minted once the grant is confirmed (idempotent finish). code: Option, } @@ -271,12 +313,6 @@ impl AuthStore { (info.created.elapsed() < info.ttl).then(|| (info.principal.clone(), info.session_id.clone())) } - /// Whether this connection's grant is live: the completion POST arrived, or - /// (best-effort fallback) a signed `mcp_get_accounts` now succeeds. Takes no - /// lock across the network call. - async fn grant_live(&self, session_id: &str, flag: bool) -> bool { - flag || self.identities.grant_is_live(session_id).await - } } // ---- Authorization code (poll bridge) ---------------------------------- @@ -326,11 +362,11 @@ pub async fn authorize(State(store): State, Query(q): Query, Query(q): Query Option { pub struct FinishQuery { /// The pending-auth id (= session id) carried by `finish_url`. id: String, + /// H3 clause 2: the one-time `finish_secret`, carried in `finish_url`'s QUERY + /// (not a path segment) so path-only access logs don't capture it (P2). + #[serde(default)] + fs: Option, /// Retry counter, so the "finishing" reload is bounded. #[serde(default)] r: u32, } -/// GET /oauth/finish — II navigates the browser here after registering the -/// session key (this is the `finish_url` returned in the key-request response). -/// Arrival is NOT proof of registration: confirm it (the completion POST flag, or -/// a signed `mcp_get_accounts` that returns `Ok`), then mint the authorization -/// code and 302 to the client's `redirect_uri` with `code` + the client's `state`. +/// GET /oauth/finish — II navigates the CONSENTING browser here after registering +/// the session key (this is the `finish_url` returned in the key-request response, +/// carrying the one-time `finish_secret` in its query). Mints a code only if all of +/// H3's predicate holds — (1) the `sid` cookie resolves to this PA, (2) `finish_secret` +/// matches, (3) registration is *proven* by a signed `mcp_get_accounts` that returns +/// `Ok`, (4) not expired / not already completed — then 302s to the client's +/// `redirect_uri` with `code` + the client's `state`. pub async fn finish( State(store): State, headers: axum::http::HeaderMap, @@ -397,7 +439,7 @@ pub async fn finish( authz.get(&q.id).map(|a| { ( a.created.elapsed() >= CONNECT_TTL, - a.live, + a.finish_secret_hash, a.code.clone(), a.client_id.clone(), a.redirect_uri.clone(), @@ -407,32 +449,47 @@ pub async fn finish( ) }) }; - let Some((expired, live_flag, existing_code, client_id, redirect_uri, client_state, code_challenge, cookie)) = snap + let Some((expired, finish_secret_hash, existing_code, client_id, redirect_uri, client_state, code_challenge, cookie)) = + snap else { return connect_error("unknown or already-used connect request — restart from your client"); }; if expired { return connect_error("connect request expired — restart from your client"); } - // H3: only the browser that STARTED this flow (and holds the binding cookie) - // may complete it. A constant-time compare isn't warranted — the value is a - // fresh 122-bit UUID, unguessable and single-use. This blocks the zero-click - // takeover (a phished victim's browser passively delivering the code) but does - // NOT stop an initiator-attacker who holds the cookie and completes finish - // themselves after the victim consents — a PARTIAL mitigation (see module docs). + // H3 clause 1 (initiator): only the browser that STARTED this flow (holding the + // `sid` cookie) may complete it. if connect_cookie(&headers).as_deref() != Some(cookie.as_str()) { return connect_error( "this sign-in was started in a different browser session — restart the connection from your client", ); } + // H3 clause 2 (consenter): the one-time `finish_secret`, disclosed only in the + // single-use key-request response, proves this browser drove the II consent. + // Requiring BOTH proofs forces initiator == consenter, closing the + // split-browser injection. `finish_secret_hash` is `Some` only after the key + // request ran (so this also gates on "the handshake happened"). Constant-time + // compare of the hashes (P5). + let secret_ok = match (q.fs.as_deref(), finish_secret_hash) { + (Some(fs), Some(hash)) => ct_eq(&sha256(fs), &hash), + _ => false, + }; + if !secret_ok { + return connect_error( + "this sign-in can't be completed in this browser — restart the connection from your client", + ); + } + let fs = q.fs.as_deref().unwrap_or_default(); // Idempotent: if the code was already minted, just redirect again. if let Some(code) = existing_code { return redirect_302(&build_redirect(&redirect_uri, &code, &client_state)); } - // Confirm registration (best-effort signed-call fallback, no lock held). If it - // isn't confirmable yet (a race with II's registration/propagation), reload - // shortly — bounded so we don't loop forever. - if !store.grant_live(&q.id, live_flag).await { + // H3 clause 3 (proven registration, P3): confirm the session key is actually + // registered on-chain via a signed `mcp_get_accounts` returning `Ok` — NOT a + // bare completion POST (which any `connect_state`-knower could forge). No lock + // held across the network call. If not yet visible (a race with II's + // registration/propagation), reload shortly — bounded so we don't loop forever. + if !store.identities.grant_is_live(&q.id).await { if q.r >= 8 { return connect_error(&format!( "could not confirm the connection with Internet Identity ({}) — it may not support \ @@ -440,7 +497,7 @@ pub async fn finish( store.instance().ii_url )); } - return finishing_page(store.instance().oauth_prefix, &q.id, q.r + 1); + return finishing_page(store.instance().oauth_prefix, &q.id, fs, q.r + 1); } // Reserve the code under the `authz` lock, then insert into `codes` AFTER @@ -477,21 +534,36 @@ pub async fn finish( /// A tiny self-reloading page shown while we wait for II's registration to become /// confirmable, then it re-hits this instance's `/oauth/finish` (bounded by the -/// retry counter). -fn finishing_page(prefix: &str, id: &str, next_try: u32) -> Response { - let url = js_escape(&format!("{prefix}/oauth/finish?id={}&r={}", urlencoding::encode(id), next_try)); - Html(format!( +/// retry counter). Carries `finish_secret` across the reload (P6) and sets +/// `Referrer-Policy: no-referrer` so the secret-bearing URL never rides a `Referer`. +fn finishing_page(prefix: &str, id: &str, fs: &str, next_try: u32) -> Response { + let url = js_escape(&format!( + "{prefix}/oauth/finish?id={}&fs={}&r={}", + urlencoding::encode(id), + urlencoding::encode(fs), + next_try + )); + let mut resp = Html(format!( "Finishing…\ + \ \

Finishing sign-in…

\ " )) - .into_response() + .into_response(); + resp.headers_mut() + .insert(axum::http::header::REFERRER_POLICY, axum::http::HeaderValue::from_static("no-referrer")); + resp } /// A 302 to an absolute URL (used for the top-level hop back to the OAuth client). +/// Sets `Referrer-Policy: no-referrer` so the `finish_secret` in this request's URL +/// (query) is not leaked to `redirect_uri` via the `Referer` header (P2). fn redirect_302(url: &str) -> Response { - (StatusCode::FOUND, [(axum::http::header::LOCATION, url.to_string())]).into_response() + let mut resp = (StatusCode::FOUND, [(axum::http::header::LOCATION, url.to_string())]).into_response(); + resp.headers_mut() + .insert(axum::http::header::REFERRER_POLICY, axum::http::HeaderValue::from_static("no-referrer")); + resp } fn build_redirect(redirect_uri: &str, code: &str, client_state: &str) -> String { @@ -537,41 +609,56 @@ pub struct ConnectCallback { /// POST /oauth/connect/callback — II's frontend makes TWO cross-origin JSON /// POSTs here, distinguished by the `expiration` field: -/// (a) key request `{state}` → 200 `{public_key, finish_url}` (fresh keypair); -/// (b) completion `{state, expiration, permissions}` → mark the grant live and -/// record the access level; any 2xx. +/// (a) key request `{state}` → 200 `{public_key, finish_url}` (fresh keypair + +/// one-time `finish_secret` embedded in `finish_url`; STRICTLY single-use); +/// (b) completion `{state, expiration, permissions}` → record expiry + access +/// level only (a latency hint; never sets `registered`); any 2xx. /// Never returns a redirect (the response is consumed by `fetch()`), and never /// receives or verifies a delegation chain. pub async fn connect_callback(State(store): State, Json(body): Json) -> Response { match &body.expiration { - // (a) Key request — require a valid, unexpired pending connection (reject - // unknown/replayed/expired state with a non-2xx so II aborts), then - // generate (lazily) this connection's session keypair and return its - // public key for II's frontend to register. Return `finish_url` so II - // navigates the browser back to us to close the OAuth loop. + // (a) Key request — STRICTLY single-use per `connect_state` (P1). Atomically + // (under one lock) require the PA is known/unexpired AND not yet claimed, + // then claim it by storing `H(finish_secret)`; any later or racing key + // request 403s (no keypair, no secret). This atomic compare-and-set is what + // makes secret-disclosure and victim-registration mutually exclusive. None => { - if !store.connect_known_authz(&body.state).await { + let secret = fresh_secret(); + let claimed = { + let mut authz = store.authz.write().await; + match authz.get_mut(&body.state) { + Some(a) if a.created.elapsed() < CONNECT_TTL && a.finish_secret_hash.is_none() => { + a.finish_secret_hash = Some(sha256(&secret)); + true + } + _ => false, + } + }; + if !claimed { return (StatusCode::FORBIDDEN, Json(json!({ "error": "invalid_state" }))).into_response(); } + // Only the single winner mints the session keypair and gets the secret. let public_key = store.identities.session_pubkey_b64(&body.state).await; + // `finish_secret` rides in the QUERY (P2): path-only access logs won't + // capture it, and /oauth/finish sends no `Referer`. It reaches only this + // (the consenting) browser, which II then navigates to `finish_url`. let finish_url = format!( - "{}{}/oauth/finish?id={}", + "{}{}/oauth/finish?id={}&fs={}", base_url(), store.instance().oauth_prefix, - urlencoding::encode(&body.state) + urlencoding::encode(&body.state), + urlencoding::encode(&secret), ); (StatusCode::OK, Json(json!({ "public_key": public_key, "finish_url": finish_url }))).into_response() } - // (b) Completion notification — best-effort. Mark the grant live if the - // connection is still known, and record the session's access level - // (`permissions`, §0/H2) so tools can warn before attempting an update - // under a read-only session. Tolerate a missing/expired state (e.g. the - // grant was already consumed via the signed-call fallback) with a 2xx, so - // a late completion POST doesn't make II treat an otherwise-successful - // connect as failed. Never create a session for an unknown state. + // (b) Completion notification — a best-effort LATENCY HINT only (P3). Record + // the grant expiration and access level (`permissions`, §0/H2) if the PA is + // known, but NEVER set `registered`: this POST is unauthenticated (any + // `connect_state`-knower can send it), so registration is proven separately + // at /oauth/finish by a signed `mcp_get_accounts`. Tolerate a missing/expired + // state with a 2xx so a late POST doesn't fail an otherwise-good connect. Some(exp) => { - let known = store.mark_live(&body.state).await; - if known { + if store.authz_known(&body.state).await { match exp.trim().parse::() { Ok(exp_ns) => store.identities.set_grant_expiration(&body.state, exp_ns).await, Err(_) => tracing::warn!("connect completion had unparseable expiration"), @@ -586,24 +673,15 @@ pub async fn connect_callback(State(store): State, Json(body): Json bool { + /// Whether `state` names a known, unexpired pending connect (read-only; used by + /// the completion POST to decide whether to record its expiry/permissions hint). + async fn authz_known(&self, state: &str) -> bool { self.authz .read() .await .get(state) .is_some_and(|a| a.created.elapsed() < CONNECT_TTL) } - - /// Mark the pending connect live; returns whether `state` was known. - async fn mark_live(&self, state: &str) -> bool { - if let Some(a) = self.authz.write().await.get_mut(state) { - a.live = true; - true - } else { - false - } - } } /// Top-level redirect via a script-initiated navigation (`location.replace`) @@ -982,4 +1060,114 @@ mod tests { let r2 = build_redirect("https://x.test/cb?foo=1", "c", ""); assert_eq!(r2, "https://x.test/cb?foo=1&code=c"); } + + /// H3/P5: `finish_secret` hygiene helpers. `fresh_secret` is high-entropy and + /// unique; `sha256` is deterministic; `ct_eq` matches equal inputs and rejects + /// unequal ones (and differing lengths) without early-out. + #[test] + fn secret_helpers_behave() { + let s1 = super::fresh_secret(); + let s2 = super::fresh_secret(); + assert_ne!(s1, s2, "secrets must be unique"); + assert!(s1.len() >= 43, "256-bit base64url is 43 chars (>=128-bit floor): {}", s1.len()); + // Deterministic hash; constant-time compare of equal vs. unequal. + assert_eq!(super::sha256(&s1), super::sha256(&s1)); + assert!(super::ct_eq(&super::sha256(&s1), &super::sha256(&s1))); + assert!(!super::ct_eq(&super::sha256(&s1), &super::sha256(&s2))); + assert!(!super::ct_eq(b"abc", b"abcd")); + assert!(super::ct_eq(b"", b"")); + } + + // Build an AuthStore over a dummy II instance (these tests never hit the + // network — the key-request path is pure-local crypto/state). + fn test_store() -> super::AuthStore { + use crate::identities::{Identities, IiInstance}; + use candid::Principal; + let ids = Identities::new(IiInstance { + name: "test", + ii_url: "https://ii.test".into(), + ii_canister: Principal::anonymous(), + oauth_prefix: "", + mcp_path: "/mcp", + }); + super::AuthStore::new(ids, super::SharedClients(std::sync::Arc::default())) + } + + async fn seed_pending(store: &super::AuthStore, id: &str, cookie: &str) { + // Insert a pending authorization directly (bypasses the browser redirect). + store.authz.write().await.insert( + id.to_string(), + super::AuthzPending { + client_id: "c".into(), + redirect_uri: "https://app.test/cb".into(), + client_state: String::new(), + code_challenge: Some("cc".into()), + cookie: cookie.into(), + created: std::time::Instant::now(), + finish_secret_hash: None, + code: None, + }, + ); + } + + /// H3/P1: the key request is STRICTLY single-use per `connect_state`. The first + /// mints the keypair + `finish_secret` (embedded in `finish_url`'s query) and + /// claims the PA (`finish_secret_hash` set); a second one 403s — no keypair, no + /// secret. This is the atomic claim that makes secret-disclosure and + /// victim-registration mutually exclusive. + #[tokio::test] + async fn key_request_is_single_use_and_mints_finish_secret() { + use axum::extract::State; + use axum::Json; + let store = test_store(); + seed_pending(&store, "sess-x", "bind-1").await; + + let r1 = super::connect_callback( + State(store.clone()), + Json(super::ConnectCallback { state: "sess-x".into(), expiration: None, permissions: None }), + ) + .await; + assert_eq!(r1.status(), axum::http::StatusCode::OK); + assert!( + store.authz.read().await.get("sess-x").unwrap().finish_secret_hash.is_some(), + "first key request must claim the PA" + ); + let body = axum::body::to_bytes(r1.into_body(), usize::MAX).await.unwrap(); + let v: serde_json::Value = serde_json::from_slice(&body).unwrap(); + let finish_url = v["finish_url"].as_str().unwrap(); + assert!(finish_url.contains("&fs="), "finish_url must carry the secret in the query: {finish_url}"); + assert!(!finish_url.contains("/fs/"), "secret must not be a path segment (P2)"); + assert!(v["public_key"].as_str().is_some()); + + // Second key request for the same state (attacker race / replay) => 403. + let r2 = super::connect_callback( + State(store.clone()), + Json(super::ConnectCallback { state: "sess-x".into(), expiration: None, permissions: None }), + ) + .await; + assert_eq!(r2.status(), axum::http::StatusCode::FORBIDDEN, "single-use: replay must 403"); + } + + /// P3: a completion POST must NOT materialize the connection — with no prior + /// key request it records nothing that could let `/oauth/finish` proceed (it + /// never sets a `finish_secret`, so the consenter proof can't be satisfied). + #[tokio::test] + async fn completion_post_does_not_materialize_finish_secret() { + use axum::extract::State; + use axum::Json; + let store = test_store(); + seed_pending(&store, "sess-y", "bind-2").await; + let r = super::connect_callback( + State(store.clone()), + Json(super::ConnectCallback { + state: "sess-y".into(), + expiration: Some("1000".into()), + permissions: Some("all".into()), + }), + ) + .await; + assert_eq!(r.status(), axum::http::StatusCode::NO_CONTENT); + // No key request ran, so no finish_secret exists => finish can never pass clause 2. + assert!(store.authz.read().await.get("sess-y").unwrap().finish_secret_hash.is_none()); + } } From 664b9a1e8a148678a4f219fc112b722368128d38 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 5 Jul 2026 10:33:06 +0000 Subject: [PATCH 5/5] H3: add single-use observability hook; fix cookie Secure, docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Observability (requested follow-up): count and warn-log key requests that hit a still-valid but already-consumed connect_state — i.e. a *repeat* key request. Under strict single-use (P1) these are ~zero; a sustained rise means II's frontend regressed to re-issuing the key request (which strict single-use turns into failed connects — a benign-but-breaking availability regression, never a takeover), while an isolated hit is a stray replay/probe. Surfaced as `repeat_key_requests` on /version and warn-logged with the instance name, so a silent regression is caught without log scraping. Distinguishes the consumed-repeat case from the ordinary unknown/expired 403 via a KeyClaim enum. Also addresses Copilot review of the Consent-Bound Completion commit: - The `sid` cookie's `Secure` attribute is now conditional on an https PUBLIC_URL. A `Secure` cookie is dropped by browsers over plain HTTP, so the unconditional flag broke the initiator check for local `http://localhost` development; production (https) is unchanged. - log_request's comment now notes `/oauth/finish`'s one-time `?fs=` (finish_secret) is kept out of logs by the never-log-query rule (P2). - README H2 wording corrected: a missing/unrecognized `permissions` value leaves the level unknown (not "full access") and falls through to the IC ingress-rejection fallback, matching the implementation. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- README.md | 10 +++++--- src/auth.rs | 73 +++++++++++++++++++++++++++++++++++++++++++++++------ src/main.rs | 11 +++++--- 3 files changed, 79 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index ccc332c..77120b6 100644 --- a/README.md +++ b/README.md @@ -272,10 +272,12 @@ entire canister-management surface inert — `create`/`install`/`start`/`stop`/ without opaque low-level errors: - The completion POST now carries `permissions: "queries" | "all"` (§0), so the - server learns the level at connect without minting a probe delegation (falling - back to a missing value = full access). -- Management tools check it up front and, for a read-only session, return an - actionable *"reconnect with read-only off"* message instead of an ingress error. + server learns the level at connect without minting a probe delegation. A + missing or unrecognized value leaves the level **unknown** (not assumed + writable): the update is attempted and the IC's ingress rejection is the + fallback signal. +- Management tools check it up front and, for a *known* read-only session, return + an actionable *"reconnect with read-only off"* message instead of an ingress error. - `get_principal` reflects a read-only session in its output, so the agent won't attempt updates it can't make. diff --git a/src/auth.rs b/src/auth.rs index 7010f61..ed96553 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -61,7 +61,10 @@ use std::{ collections::HashMap, - sync::Arc, + sync::{ + atomic::{AtomicU64, Ordering}, + Arc, + }, time::{Duration, Instant}, }; @@ -96,6 +99,21 @@ const GRANT_TTL_SECS: u64 = 3600; /// `/oauth/finish` (H3): only the browser that started the flow can complete it. const CONNECT_COOKIE: &str = "mcp_connect"; +/// Observability for the single-use key request (P1): number of key requests that +/// hit a still-valid but ALREADY-CONSUMED `connect_state` — i.e. a *repeat* key +/// request. Under strict single-use these are expected to be ~zero. An isolated +/// hit is a stray replay / attacker probe (harmless — it 403s); a sustained rise +/// means II's frontend started re-issuing the key request, which strict single-use +/// turns into failed connects (a benign-but-breaking regression to fix upstream — +/// never a takeover). Warn-logged with the instance name and surfaced on `/version` +/// so a silent regression is caught without log scraping. Process-wide. +static REPEAT_KEY_REQUESTS: AtomicU64 = AtomicU64::new(0); + +/// Current value of the repeat-key-request counter (for the `/version` probe). +pub fn repeat_key_requests() -> u64 { + REPEAT_KEY_REQUESTS.load(Ordering::Relaxed) +} + /// Public base URL clients use to reach this server. Override with PUBLIC_URL. pub fn base_url() -> String { std::env::var("PUBLIC_URL").unwrap_or_else(|_| "http://localhost:8000".to_string()) @@ -386,8 +404,12 @@ pub async fn authorize(State(store): State, Query(q): Query, } +/// Outcome of the atomic single-use claim on a key request's `connect_state`. +enum KeyClaim { + /// Won the claim: mint the keypair + `finish_secret`. + Claimed, + /// Known + unexpired but already claimed — a repeat key request (observed). + RepeatConsumed, + /// Unknown or expired — an ordinary rejected replay/stale link. + Reject, +} + /// POST /oauth/connect/callback — II's frontend makes TWO cross-origin JSON /// POSTs here, distinguished by the `expiration` field: /// (a) key request `{state}` → 200 `{public_key, finish_url}` (fresh keypair + @@ -624,18 +656,37 @@ pub async fn connect_callback(State(store): State, Json(body): Json { let secret = fresh_secret(); - let claimed = { + let claim = { let mut authz = store.authz.write().await; match authz.get_mut(&body.state) { + // Known, unexpired, not yet claimed: win the single-use claim. Some(a) if a.created.elapsed() < CONNECT_TTL && a.finish_secret_hash.is_none() => { a.finish_secret_hash = Some(sha256(&secret)); - true + KeyClaim::Claimed } - _ => false, + // Known, unexpired, ALREADY claimed: a repeat key request — the + // signal we watch (II retry regression, or a stray probe). + Some(a) if a.created.elapsed() < CONNECT_TTL => KeyClaim::RepeatConsumed, + // Unknown or expired: an ordinary rejected replay/stale link. + _ => KeyClaim::Reject, } }; - if !claimed { - return (StatusCode::FORBIDDEN, Json(json!({ "error": "invalid_state" }))).into_response(); + match claim { + KeyClaim::Claimed => {} + KeyClaim::RepeatConsumed => { + let total = REPEAT_KEY_REQUESTS.fetch_add(1, Ordering::Relaxed) + 1; + tracing::warn!( + instance = store.instance().name, + total, + "repeat key request on a consumed connect_state (single-use, P1) — \ + a sustained rise means II is re-issuing the key request; an isolated hit \ + is a stray replay/probe" + ); + return (StatusCode::FORBIDDEN, Json(json!({ "error": "invalid_state" }))).into_response(); + } + KeyClaim::Reject => { + return (StatusCode::FORBIDDEN, Json(json!({ "error": "invalid_state" }))).into_response(); + } } // Only the single winner mints the session keypair and gets the secret. let public_key = store.identities.session_pubkey_b64(&body.state).await; @@ -1139,13 +1190,19 @@ mod tests { assert!(!finish_url.contains("/fs/"), "secret must not be a path segment (P2)"); assert!(v["public_key"].as_str().is_some()); - // Second key request for the same state (attacker race / replay) => 403. + // Second key request for the same state (attacker race / replay) => 403, + // and it bumps the repeat-key-request observability counter (P1 health). + let before = super::repeat_key_requests(); let r2 = super::connect_callback( State(store.clone()), Json(super::ConnectCallback { state: "sess-x".into(), expiration: None, permissions: None }), ) .await; assert_eq!(r2.status(), axum::http::StatusCode::FORBIDDEN, "single-use: replay must 403"); + assert!( + super::repeat_key_requests() > before, + "a repeat key request on a consumed connect_state must be counted" + ); } /// P3: a completion POST must NOT materialize the connection — with no prior diff --git a/src/main.rs b/src/main.rs index f70b8e3..eb61c2b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -682,9 +682,10 @@ fn authed_session(ctx: &RequestContext) -> Option, next: axum::middleware::Next, @@ -1125,6 +1126,10 @@ async fn main() -> anyhow::Result<()> { "commit": option_env!("GIT_SHA").unwrap_or("unknown"), "built_at": option_env!("BUILD_TIME").and_then(|s| s.parse::().ok()), "started_at": started_at, + // H3/P1 health: repeat key requests on a consumed connect_state. + // Expected ~0; a sustained rise means II is re-issuing the key + // request (breaks connects under strict single-use), so alert on it. + "repeat_key_requests": auth::repeat_key_requests(), })) }), )