diff --git a/crates/imcp2-core/src/identities.rs b/crates/imcp2-core/src/identities.rs index 2864451..cb71f44 100644 --- a/crates/imcp2-core/src/identities.rs +++ b/crates/imcp2-core/src/identities.rs @@ -127,11 +127,20 @@ pub const MAX_PENDING_CONNECTS: usize = 1_024; const MAX_SESSIONS: usize = 20_000; /// How long a session that never redeemed a grant is kept before the reaper -/// treats the connect as abandoned. Comfortably past `crate::auth`'s 10-minute -/// `CONNECT_TTL`, after which the connect can no longer be redeemed anyway, so -/// this only ever drops sessions that are already dead weight. +/// treats the connect as abandoned. Comfortably past the hosted server's +/// 10-minute `CONNECT_TTL`, after which no new redemption can start, so the +/// redemptions this can cut short are ones that started in time and then ran +/// past the 15 minutes. Those fail with [`SIGN_IN_DROPPED_MSG`] (see +/// [`Identities::redeem_registration_delegation`]) rather than re-create the +/// session with a key II never registered. const PENDING_CONNECT_TTL_NS: u64 = 15 * 60 * 1_000_000_000; +/// What a redemption answers when its connect's session is gone: reaped (see +/// [`PENDING_CONNECT_TTL_NS`]) or evicted at capacity, before or while the +/// `mcp_register_v2` call ran. Shown to the user on the callback page. +pub const SIGN_IN_DROPPED_MSG: &str = "This sign-in was dropped before it could finish: it took \ + too long, or the server was busy. Start the sign-in again."; + /// Per-session ceiling on cached per-app delegations. `store` inserts one per /// distinct `(derivation origin, account)` a session asks for, so a single /// authenticated session could otherwise inflate its own map without limit by @@ -563,12 +572,14 @@ impl Identities { Some(Principal::self_authenticating(&der).to_text()) } - /// Record the grant's expiration from the `mcp_register_v2` reply, at connect - /// redemption. The value is nanoseconds since the epoch. + /// Record a grant expiring at `expiration_ns` (nanoseconds since the epoch) + /// on `session_id`, creating the session if it doesn't exist. Connect + /// redemption does NOT use this: it records through `record_registration`, + /// which refuses rather than re-creates a session reaped mid-call, since a + /// re-created session's fresh key is not the one II registered. pub async fn set_grant_expiration(&self, session_id: &str, expiration_ns: u64) { - // Best effort: the redeem path ensured this session already, so a - // capacity refusal here means it was reaped mid-flight — nothing to - // record, and the client's next call gets the reconnect message. + // Best effort: a capacity refusal leaves nothing to record, and the + // client's next call gets the reconnect message. if let Err(e) = self.ensure_session(session_id).await { tracing::warn!(session_id = %session_id, "could not record the grant expiration: {e}"); return; @@ -582,32 +593,25 @@ impl Identities { // readers/writers of the session map. let opened = { let mut sessions = self.sessions.write().await; - match sessions.get_mut(session_id) { - Some(s) => { - let was_live = s.grant_expiration_ns.is_some_and(|e| e > now); - s.grant_expiration_ns = Some(expiration_ns); - // Redeeming a grant is itself activity, so a freshly-opened - // session counts as active immediately (before its first tool - // call) on the `active_sessions` gauge. - s.last_seen_ns.store(now, Ordering::Relaxed); - !was_live && expiration_ns > now - } - None => false, - } + sessions.get_mut(session_id).is_some_and(|s| record_grant(s, expiration_ns, now)) }; - // "session opened" pairs 1:1 with the reaper's "session closed", so the - // two reconcile in the journal. A re-bind of an already-live grant (a - // redeem retry within the delegation's lifetime) is not a new open. if opened { - tracing::info!( - instance = self.instance.name, - session_id = %session_id, - expiration_ns, - "session opened" - ); + self.log_session_opened(session_id, expiration_ns); } } + /// The "session opened" log. It pairs 1:1 with the reaper's "session + /// closed", so the two reconcile in the journal; logged after the session + /// lock is released, so a slow log sink can't stall the session map. + fn log_session_opened(&self, session_id: &str, expiration_ns: u64) { + tracing::info!( + instance = self.instance.name, + session_id = %session_id, + expiration_ns, + "session opened" + ); + } + /// Record the session's access level from the `mcp_register_v2` reply's /// `permissions` field: `"queries"` = read-only, `"all"` = full access (both /// case-insensitive). Any UNRECOGNIZED value leaves the level `None` (unknown) @@ -615,16 +619,12 @@ impl Identities { /// 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!("mcp_register_v2 reply had unrecognized permissions {other:?}; access level left unknown"); - return; - } + let Some(level) = access_level(permissions) else { + return; }; - // Same best-effort posture as `set_grant_expiration`: recorded on the - // session the redeem path already created, or dropped with a warning. + let level = Some(level); + // Same best-effort posture as `set_grant_expiration` (and, like it, not + // what connect redemption uses). if let Err(e) = self.ensure_session(session_id).await { tracing::warn!(session_id = %session_id, "could not record the access level: {e}"); return; @@ -911,12 +911,12 @@ impl Identities { reg_user_key: Vec, chain: Vec, ) -> Result { - self.ensure_session(session_id).await?; - - // priv(X) to sign the ingress as, and pub(S) to register. + // priv(X) to sign the ingress as, and pub(S) to register. The connect's + // session must still be here: one reaped or evicted since the connect + // began is not re-created, which would only mint a key II never sees. let (reg_seed, reg_der, session_der) = { let sessions = self.sessions.read().await; - let s = sessions.get(session_id).ok_or("no such session")?; + let s = sessions.get(session_id).ok_or(SIGN_IN_DROPPED_MSG)?; let reg_seed = s.reg_key_seed.ok_or("no registration key was minted for this connect")?; let reg_der = s @@ -962,13 +962,67 @@ impl Identities { .map_err(|e| format!("Internet Identity rejected registration: {e}"))?; // Record expiry + access level so the signer's expiry check and the - // read-only guard have what they need. + // read-only guard have what they need — on the session II just + // registered, or not at all (see `record_registration`). let permissions = outcome.permissions.as_text(); - self.set_grant_expiration(session_id, outcome.expiration).await; - self.set_permissions(session_id, permissions).await; + self.record_registration(session_id, &session_der, outcome.expiration, permissions).await?; Ok(RegistrationOutcome { expiration_ns: outcome.expiration, permissions }) } + /// Record a successful `mcp_register_v2` — the grant's expiration and access + /// level — on the session it registered, in one step under the lock. + /// + /// Refused, with nothing recorded, when that session is gone or no longer + /// holds the key `session_der` that II just registered: it was reaped (15 + /// minutes after its connect began, see [`PENDING_CONNECT_TTL_NS`]) or + /// evicted at capacity while the call was in flight. Re-creating it, as + /// [`Self::set_grant_expiration`] would, mints a fresh key II never + /// registered, so the client would get a session that cannot sign anything. + async fn record_registration( + &self, + session_id: &str, + session_der: &[u8], + expiration_ns: u64, + permissions: &str, + ) -> Result<(), String> { + let level = access_level(permissions); + let now = now_ns(); + let recorded = { + let mut sessions = self.sessions.write().await; + match sessions.get_mut(session_id) { + Some(s) if s.pubkey_der == session_der => { + if level.is_some() { + s.read_only = level; + } + Ok(record_grant(s, expiration_ns, now)) + } + Some(_) => Err("re-created with another key"), + None => Err("gone"), + } + }; + match recorded { + Ok(opened) => { + if opened { + self.log_session_opened(session_id, expiration_ns); + } + Ok(()) + } + // Worth an operator's attention: II has already registered this key, + // replacing the user's previous MCP session, so the user is left with + // neither until they sign in again. Logged after the lock is released. + Err(session) => { + tracing::warn!( + instance = self.instance.name, + session_id = %session_id, + session, + "sign-in dropped after Internet Identity registered it: its session was \ + reaped or evicted while mcp_register_v2 ran" + ); + Err(SIGN_IN_DROPPED_MSG.to_owned()) + } + } + } + /// Resolve an optional account `name` at `domain` to its account number /// (`None` = the default account, used when `name` is `None`). Looks the name /// up via [`Self::list_accounts`]; errors if no account (or more than one) at @@ -1212,6 +1266,36 @@ impl Identities { } } +/// Record a grant expiring at `expiration_ns` on `s`, returning whether this +/// opened the session: a genuine not-live -> live transition, so a re-bind of an +/// already-live grant (a redeem retry within the delegation's lifetime) is not a +/// new open. Redeeming a grant is itself activity, so a freshly-opened session +/// counts as active at once (before its first tool call) on the +/// `active_sessions` gauge. +fn record_grant(s: &mut Session, expiration_ns: u64, now: u64) -> bool { + let was_live = s.grant_expiration_ns.is_some_and(|e| e > now); + s.grant_expiration_ns = Some(expiration_ns); + s.last_seen_ns.store(now, Ordering::Relaxed); + !was_live && expiration_ns > now +} + +/// The access level an `mcp_register_v2` reply's `permissions` names: +/// `Some(true)` = read-only (`"queries"`), `Some(false)` = full access (`"all"`), +/// both case-insensitive. Any UNRECOGNIZED value is `None`, with a warning, so +/// the level stays unknown rather than assumed to be full access. +fn access_level(permissions: &str) -> Option { + match permissions.trim().to_ascii_lowercase().as_str() { + "queries" => Some(true), + "all" => Some(false), + other => { + tracing::warn!( + "mcp_register_v2 reply had unrecognized permissions {other:?}; access level left unknown" + ); + None + } + } +} + /// Drop every session that no longer serves anyone, returning the ids of each /// kind so the caller can log them: sessions whose grant has EXPIRED (paired /// with the "session opened" log by a "session closed" one), and ABANDONED @@ -1682,6 +1766,74 @@ mod tests { assert!(ids.require_write("sess2").await.is_ok()); } + // The session key a pending connect's session holds, as `authorize` minted it. + async fn session_der(ids: &Identities, session_id: &str) -> Vec { + ids.sessions.read().await[session_id].pubkey_der.clone() + } + + // A successful registration is recorded on the session it registered: the + // grant's expiration and the access level, in one step. + #[tokio::test] + async fn record_registration_records_on_the_session_it_registered() { + let ids = test_ids(); + ids.ensure_session("sess").await.expect("room for a session"); + let der = session_der(&ids, "sess").await; + let future = now_ns() + 3_600_000_000_000; + + ids.record_registration("sess", &der, future, "queries").await.expect("recorded"); + + assert_eq!(ids.grant_expiration_ns("sess").await, Some(future)); + assert_eq!(ids.is_read_only("sess").await, Some(true)); + } + + // A registration whose session was reaped or evicted while `mcp_register_v2` + // ran is refused, and the session is NOT re-created: a re-created session's + // fresh key is not the one II registered, so it could never sign anything. + #[tokio::test] + async fn record_registration_refuses_a_session_gone_mid_call() { + let ids = test_ids(); + ids.ensure_session("sess").await.expect("room for a session"); + let der = session_der(&ids, "sess").await; + ids.discard_session("sess").await; // reaped while the call ran + + let future = now_ns() + 3_600_000_000_000; + let refused = ids.record_registration("sess", &der, future, "all").await; + + assert_eq!(refused, Err(SIGN_IN_DROPPED_MSG.to_owned())); + assert!(ids.sessions.read().await.is_empty(), "the session is not re-created"); + } + + // Nor is a registration recorded on a session that has since been re-created + // under the same id with another key: the key II registered is gone with the + // old one. + #[tokio::test] + async fn record_registration_refuses_a_session_re_created_with_another_key() { + let ids = test_ids(); + ids.ensure_session("sess").await.expect("room for a session"); + let registered = session_der(&ids, "sess").await; + ids.discard_session("sess").await; + ids.ensure_session("sess").await.expect("room for a session"); + assert_ne!(session_der(&ids, "sess").await, registered); + + let future = now_ns() + 3_600_000_000_000; + let refused = ids.record_registration("sess", ®istered, future, "all").await; + + assert_eq!(refused, Err(SIGN_IN_DROPPED_MSG.to_owned())); + assert_eq!(ids.grant_expiration_ns("sess").await, None, "no grant on the new key"); + } + + // A redemption whose connect's session is already gone is refused up front, + // before any network call, and does not re-create the session. + #[tokio::test] + async fn redeem_refuses_a_connect_whose_session_is_gone() { + let ids = test_ids(); + + let refused = ids.redeem_registration_delegation("gone", vec![], vec![]).await; + + assert_eq!(refused.map(|_| ()), Err(SIGN_IN_DROPPED_MSG.to_owned())); + assert!(ids.sessions.read().await.is_empty(), "the session is not re-created"); + } + // `discard_session` drops a session whatever it holds, a live grant included, // which the reaper and the admission bound would otherwise keep until the // grant expires; an unknown id is a no-op. @@ -2736,7 +2888,9 @@ mod tests { #[tokio::test] async fn redeem_guards_missing_key_and_empty_chain() { let ids = test_ids(); - // No X minted yet: even a plausible user_key can't be redeemed. + // A session with no X minted yet: even a plausible user_key can't be + // redeemed. (No session at all is `redeem_refuses_a_connect_whose_session_is_gone`.) + ids.ensure_session("sess").await.expect("room for a session"); let err = ids .redeem_registration_delegation("sess", vec![1], vec![]) .await diff --git a/src/auth.rs b/src/auth.rs index fafe0f8..8f9ba9d 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -2686,42 +2686,37 @@ const CONNECT_EXPIRED_MSG: &str = "This connect request has expired. Restart fro /// stay claimed until its own claim lets go, or a second registration could /// start before this one's guard clears the mark out from under it. /// -/// Refused for a connect past [`CONNECT_TTL`], even one whose redemption passed -/// the gate in time. `imcp2-core` reaps an unredeemed connect's session 15 -/// minutes in, on the promise that no connect is redeemed past its TTL, and -/// recording the grant on a reaped session re-creates it with a fresh key that -/// II never registered: a code minted then would hand the client a session -/// whose every signed call fails. Until [`EXPIRED_CONNECT_RETENTION`], the sweep -/// had dropped the entry long before that, so this check is what keeps the -/// promise now that expired connects are retained. +/// Not refused for being past [`CONNECT_TTL`]: the TTL bounds when a redemption +/// may START (the gate, and [`claim_redemption`] under the lock), and one that +/// started in time has done its work by now. It consented in time, and II has +/// just registered its session key, replacing the user's previous MCP session. +/// Refusing the code here would leave the user with neither. Nor can it bind +/// the code to a dead session: `imcp2-core` records the registration only on +/// the session holding the key II registered, and fails the redemption, before +/// it gets here, when that session was reaped or evicted mid-call. /// -/// A refusal also discards the session the registration just granted -/// ([`Identities::discard_session`]): no code will ever reach it, and with a -/// grant recorded nothing else would drop it before the grant expired. +/// Refused only when the connect itself is gone (evicted at the cap, or kept +/// past [`EXPIRED_CONNECT_RETENTION`]). That refusal also discards the session +/// the registration just granted ([`Identities::discard_session`]): no code will +/// ever reach it, and with a grant recorded nothing else would drop it before +/// the grant expired. async fn reserve_code( store: &AuthStore, state: &str, fresh: String, ) -> Result<(String, bool), &'static str> { - let refused = { - let mut authz = store.authz.write().await; - match authz.get_mut(state) { - None => "This connect request is no longer available. Restart from your client.", - Some(a) if a.remaining().is_zero() => CONNECT_EXPIRED_MSG, - Some(a) => { - return Ok(match &a.code { - Some(existing) => (existing.clone(), false), - None => { - a.code = Some(fresh.clone()); - (fresh, true) - } - }) + if let Some(a) = store.authz.write().await.get_mut(state) { + return Ok(match &a.code { + Some(existing) => (existing.clone(), false), + None => { + a.code = Some(fresh.clone()); + (fresh, true) } - } - }; + }); + } // Outside the `authz` lock: the session map has its own. store.identities.discard_session(state).await; - Err(refused) + Err("This connect request is no longer available. Restart from your client.") } /// A JSON error the pinned page reads and displays. @@ -2763,9 +2758,9 @@ async fn claim_redemption(store: &AuthStore, state: &str) -> RedeemClaim { if a.redeeming { return RedeemClaim::InProgress; } - // Rechecked under the lock: the gate's snapshot can predate the TTL, and a - // registration started on an expired connect could only be refused at the - // mint, after II had already granted it. + // Rechecked under the lock: the gate's snapshot can predate the TTL, and the + // TTL is what bounds when a redemption may start. One that starts here runs + // to completion, code included (see `reserve_code`). if a.remaining().is_zero() { return RedeemClaim::Expired; } @@ -5140,53 +5135,69 @@ mod tests { assert!(error.contains("unknown or already used"), "{error}"); } - // A redemption that passed the gate in time but finished after the TTL gets - // no code. The connect's session is reaped 15 minutes in, and recording the - // grant re-creates it with a key II never registered, so a code minted then - // would be bound to a session that cannot sign; retaining expired connects - // must not open that up. Within the TTL the code is minted, idempotently. - // - // The refusal also discards the session the registration granted (else it - // would hold its slot until the grant expired), and leaves the connect - // claimed until the attempt's own claim lets go, so no second registration - // can start in between. + // A redemption that started in time gets its code even if it finishes past + // the TTL: the TTL bounds when a redemption may start, and by now II has + // registered the session (replacing the user's previous one), so refusing + // would leave them with neither. A session reaped mid-call never gets this + // far: `imcp2-core` fails the redemption instead of re-creating it. Within + // the TTL the code is minted, idempotently. #[tokio::test(start_paused = true)] - async fn no_code_is_minted_for_a_connect_past_its_ttl() { + async fn a_redemption_started_in_time_gets_its_code_even_past_the_ttl() { let store = test_store(); seed_pending(&store, "sess-in-time", "bind").await; - seed_pending(&store, "sess-late", "bind").await; + seed_pending(&store, "sess-slow", "bind").await; let minted = super::reserve_code(&store, "sess-in-time", "code-1".into()).await; assert_eq!(minted, Ok(("code-1".to_owned(), true))); let again = super::reserve_code(&store, "sess-in-time", "code-2".into()).await; assert_eq!(again, Ok(("code-1".to_owned(), false)), "the first code, idempotently"); - // A late attempt: claimed in time, its registration granted, done after the TTL. - let claim = super::claim_redemption(&store, "sess-late").await; + // A slow attempt: claimed in time, its registration granted, done past the TTL. + let claim = super::claim_redemption(&store, "sess-slow").await; assert!(matches!(claim, super::RedeemClaim::Claimed(_))); - store.identities.set_grant_expiration("sess-late", u64::MAX).await; + store.identities.set_grant_expiration("sess-slow", u64::MAX).await; tokio::time::advance(super::CONNECT_TTL).await; - let late = super::reserve_code(&store, "sess-late", "code-3".into()).await; - assert_eq!(late, Err(super::CONNECT_EXPIRED_MSG)); + let slow = super::reserve_code(&store, "sess-slow", "code-3".into()).await; + assert_eq!(slow, Ok(("code-3".to_owned(), true))); + assert_eq!( + store.identities.grant_expiration_ns("sess-slow").await, + Some(u64::MAX), + "its session keeps the grant the code will reach" + ); + drop(claim); + } + + // A redemption whose connect vanished while it ran (evicted at the cap, or + // forgotten past its retention) gets no code, and the session its + // registration granted is discarded: no code will reach it, and with a grant + // recorded nothing else would drop it before the grant expired. + #[tokio::test] + async fn a_redemption_whose_connect_is_gone_gets_no_code() { + let store = test_store(); + seed_pending(&store, "sess-gone", "bind").await; + let claim = super::claim_redemption(&store, "sess-gone").await; + assert!(matches!(claim, super::RedeemClaim::Claimed(_))); + store.identities.set_grant_expiration("sess-gone", u64::MAX).await; + store.authz.write().await.remove("sess-gone"); + + let refused = super::reserve_code(&store, "sess-gone", "code-1".into()).await; + assert_eq!( - store.identities.grant_expiration_ns("sess-late").await, + refused, + Err("This connect request is no longer available. Restart from your client.") + ); + assert_eq!( + store.identities.grant_expiration_ns("sess-gone").await, None, "the granted session no code will reach is discarded" ); - { - let authz = store.authz.read().await; - assert!(authz["sess-late"].code.is_none()); - assert!(authz["sess-late"].redeeming, "still claimed by the late attempt"); - } drop(claim); - assert!(!store.authz.read().await["sess-late"].redeeming, "released by its own claim"); } // No attempt starts on a connect past its TTL, even for a request that passed // the gate just before it: the claim rechecks the TTL under the lock, so such - // a request answers "expired" instead of starting a registration that could - // only be refused at the mint. + // a request answers "expired" instead of starting a registration past it. #[tokio::test(start_paused = true)] async fn no_attempt_is_claimed_on_a_connect_past_its_ttl() { let store = test_store();