diff --git a/analysis/pr-5696-gap-analysis.md b/analysis/pr-5696-gap-analysis.md index 0fbbd4768e..1ae3c708d9 100644 --- a/analysis/pr-5696-gap-analysis.md +++ b/analysis/pr-5696-gap-analysis.md @@ -1,8 +1,123 @@ -# PR #5696 — Gap Analysis vs. MSRC AG-UI `ThreadId` Session-Hijack Report +# PR #5696 — Gap Analysis vs. the MSRC Session-Scoping Reports This note evaluates how completely the changes in [PR #5696 — ".NET: Support ClaimsIdentity-based scoping of agent sessions"](https://github.com/microsoft/agent-framework/pull/5696) -address the **AG-UI**-specific MSRC report: +address two related MSRC reports against the .NET hosting layer: + +- **Part A** — the *A2A persistence-default* report (the originally-filed + variant): an A2A server, configured per the in-tree quickstart, persists + sessions keyed only by the wire-supplied `contextId`, with no + principal/owner dimension on the `AgentSessionStore` contract. +- **Part B** — the *AG-UI `ThreadId` session-hijack* report: the AG-UI + hosting endpoint trusts the client-supplied `RunAgentInput.ThreadId` as + the sole session-lookup key. + +Both reports are rated **Low / Defense-in-Depth** by MSRC because the +default registered store is a no-op (`NoopAgentSessionStore`); the +cross-user takeover only materializes when an integrator (a) registers a +persistent store and (b) does not scope the conversation-id namespace by +principal. + +> **Design constraint set by the team (applies to both parts):** +> flipping defaults so that the integrator is *required* to register a +> `SessionIsolationKeyProvider` before persistence works is **explicitly +> out of scope**. First-run / single-user / prototyping scenarios — where +> there is no `ClaimsPrincipal`, no auth scheme, and no isolation +> provider — must continue to work "out of the box". Any auto-isolation +> mechanism must therefore degrade to a transparent no-op when no +> principal context is configured. + +--- + +## Part A — A2A persistence-default report + +> Reconstructed from the prior session's notes; the verbatim text was not +> committed to the repo. Findings re-verified against PR #5696 head. + +### The reported issue + +The A2A hosting layer's quickstart pattern — + +```csharp +services.AddAIAgent(...) + .WithSessionStore(new InMemoryAgentSessionStore()) + .AsA2AServer(); +``` + +— produces a server in which any caller who knows (or guesses) another +caller's `contextId` can resume that other caller's persisted thread, +because: + +1. `AgentSessionStore.GetSessionAsync(AIAgent agent, string conversationId, CancellationToken)` + is the only lookup primitive; the contract carries no principal + dimension. +2. `A2AAgentHandler` resolves the keyed `AgentSessionStore` from DI and + passes it directly to `AIHostAgent`, with the wire `contextId` flowing + through unchanged. +3. The persistent stores in-tree (`InMemoryAgentSessionStore`, + sample-grade SQLite/Redis demos) treat `(agent, conversationId)` as a + complete primary key. + +Because the documented A2A quickstart leads developers straight at this +shape, any multi-user A2A deployment that copies the quickstart inherits +the cross-tenant takeover. + +### How PR #5696 changes the A2A path + +The PR introduces an isolation-key abstraction and **auto-wraps on the +A2A code path** when an integrator opts in: + +- `SessionIsolationKeyProvider` returns an opaque key from ambient + context (the shipped impl reads `HttpContext.User`). +- `IsolationKeyScopedAgentSessionStore` is a `DelegatingAgentSessionStore` + that prefixes the inbound `conversationId` with `{escapedKey}::` before + forwarding to the inner store. It carries a `Strict` knob: + - `Strict = false` — when the provider returns no key (e.g. anonymous + request, no auth scheme configured), the call falls through with the + bare `conversationId`. This is the "out-of-box" mode. + - `Strict = true` — a missing key throws / refuses the operation. +- `A2AServerServiceCollectionExtensions.CreateA2AServer` resolves the + keyed store, checks the `GetService()` + service-locator chain, and wraps if needed with + `Strict = isolationKeyProvider != null`. + +Net effect on A2A: + +| Scenario | Before PR | After PR | +|---|---|---| +| No persistent store registered | Ephemeral (`NoopAgentSessionStore`) | Unchanged — still ephemeral | +| Persistent store, **no** `UseClaimsBasedSessionIsolation` | Single-namespace persistence (vulnerable in multi-user) | **Same single-namespace persistence** — wrapper installed but in `Strict = false` no-op pass-through, so the out-of-box first-run scenario is preserved | +| Persistent store **plus** `services.UseClaimsBasedSessionIsolation(...)` | Still vulnerable (no wiring existed) | Auto-scoped per principal claim; missing claim → strict refusal | + +### Item-by-item map of the A2A report's recommended fixes + +| Recommended fix | Status | +|---|---| +| Add a principal/owner dimension to the persistence contract so multi-user safety is *expressible* without per-store rewrites | ✅ Achieved via decorator (`IsolationKeyScopedAgentSessionStore`) rather than a 3-arg `AgentSessionStore` virtual | +| Make the safe path **engage automatically** on the canonical A2A registration shape when an isolation provider is configured | ✅ `CreateA2AServer` auto-wraps the resolved keyed store | +| Preserve out-of-box single-user behaviour (no auth → still works) | ✅ `Strict = false` when no provider is registered; no behavioural change to the no-provider scenario | +| Document the trust model on `AgentSessionStore` / `InMemoryAgentSessionStore` / the A2A quickstart so integrators understand what `(agent, conversationId)` *isn't* | ❌ Not addressed; XML on the legacy surfaces is unchanged | +| Update the A2A end-to-end sample to model the safe pattern with a working auth scheme | ❌ Sample unchanged | + +### Residual A2A gaps + +- **A-G1.** Trust-model XML still missing on `AgentSessionStore`, + `InMemoryAgentSessionStore`, and the A2A registration entry points. + An integrator reading IntelliSense on the canonical pattern is not + prompted toward `UseClaimsBasedSessionIsolation`. +- **A-G2.** No A2A sample exercises the safe configuration end-to-end. +- **A-G3.** Same `conversationId`-mutation contract caveats as Part B + (B-G3 below): inner stores see a rewritten id; logging/telemetry sinks + will leak the isolation key; stores with id-shape constraints must + accept escaped keys of arbitrary length. + +The headline A2A finding (the *handler-level* auto-wrap when a provider +is registered) **is** addressed by this PR, within the agreed +constraint. What remains for A2A is documentation and samples. + +--- + +## Part B — AG-UI `ThreadId` session-hijack report > The Microsoft Agent Framework AG-UI hosting endpoint (.NET) trusts the > client-supplied `RunAgentInput.ThreadId` as the sole key for persisted @@ -18,14 +133,7 @@ materializes when the integrator (a) registers a persistent store and job here is to make the safe path obvious and to document the trust assumption. -> **Design constraint set by the team:** flipping defaults so that the -> integrator is *required* to register a `SessionIsolationKeyProvider` -> before persistence works is **explicitly out of scope** — first-run / -> single-user / prototyping scenarios must continue to work -> "out of the box". Any auto-isolation mechanism must therefore degrade -> to a transparent no-op when no principal context is configured. - -## What the PR delivers (AG-UI-relevant pieces only) +### What the PR delivers (AG-UI-relevant pieces only) - `SessionIsolationKeyProvider` abstraction (`dotnet/src/Microsoft.Agents.AI.Hosting/SessionIsolationKeyProvider.cs`). @@ -50,9 +158,9 @@ assumption. gets a single commented reminder pointing at `UseClaimsBasedSessionIsolation`. -## Item-by-item map of MSRC's recommended fixes to the PR +### Item-by-item map of MSRC's recommended fixes to the PR -### MSRC Fix #1 — Strengthen XML doc on `MapAGUI`, `InMemoryAgentSessionStore`, `AgentSessionStore` to state the trust model when a persistent store is in play +#### MSRC Fix #1 — Strengthen XML doc on `MapAGUI`, `InMemoryAgentSessionStore`, `AgentSessionStore` to state the trust model when a persistent store is in play **Status: ❌ Not addressed on the named surfaces.** @@ -82,7 +190,7 @@ The new types (`IsolationKeyScopedAgentSessionStore`, nothing on the legacy IntelliSense path prompts the integrator to look for them. -### MSRC Fix #2 — Compose principal into the conversation key inside the AG-UI handler when a `ClaimsPrincipal` is available, gated by a versioned opt-in (e.g. `MapAGUIOptions { ScopeSessionsByPrincipal = true }`) +#### MSRC Fix #2 — Compose principal into the conversation key inside the AG-UI handler when a `ClaimsPrincipal` is available, gated by a versioned opt-in (e.g. `MapAGUIOptions { ScopeSessionsByPrincipal = true }`) **Status: ⚠️ Partially addressed; opt-in path exists but does not cover the canonical AG-UI registration shape.** @@ -120,7 +228,7 @@ services.AddKeyedSingleton(agentName, new InMemoryAgentSessio registered, because the keyed store goes straight into `AIHostAgent` without ever passing through `WithSessionStore`'s wrapping logic. That is precisely the precondition pattern the MSRC report calls out. (See -gap **G1** below.) +gap **B-G1** below.) There is also no `MapAGUIOptions` / no MapAGUI-level opt-in flag of the shape MSRC suggested. Discoverability of the safe path lives entirely on @@ -128,7 +236,7 @@ a separate `services.UseClaimsBasedSessionIsolation(...)` extension and on whether the integrator happened to use the right store-registration helper. -### MSRC Fix #3 (optional) — Add a principal-aware overload to `AgentSessionStore` so custom stores can scope by principal without injecting `IHttpContextAccessor` +#### MSRC Fix #3 (optional) — Add a principal-aware overload to `AgentSessionStore` so custom stores can scope by principal without injecting `IHttpContextAccessor` **Status: ✅ Achieved via decorator instead of a virtual overload.** @@ -138,11 +246,11 @@ upstream by `IsolationKeyScopedAgentSessionStore`, and the inner store sees a mutated `conversationId` of the form `{escapedKey}::{conversationId}`. This achieves the *outcome* MSRC asked for (custom stores no longer need `IHttpContextAccessor`) but with two -documented-by-implication trade-offs noted in **G3** below. +documented-by-implication trade-offs noted in **B-G3** below. -## Remaining gaps for the AG-UI threat scenario +### Remaining gaps for the AG-UI threat scenario -### G1. `MapAGUI` does not auto-wrap the keyed store; safe path requires a non-obvious registration shape +#### B-G1. `MapAGUI` does not auto-wrap the keyed store; safe path requires a non-obvious registration shape This is the headline gap relative to the MSRC AG-UI report. The isolation wrapper is auto-installed only on the @@ -173,7 +281,7 @@ chain, wrap it. Use the same `Strict = isolationKeyProvider != null` construction the A2A path uses, so the no-provider case stays a transparent no-op (preserving the out-of-box constraint). -### G2. Trust-model docs missing on the three legacy AG-UI surfaces (MSRC Fix #1) +#### B-G2. Trust-model docs missing on the three legacy AG-UI surfaces (MSRC Fix #1) `MapAGUI`, `InMemoryAgentSessionStore`, and `AgentSessionStore` need the explicit "thread id is a chain-resume identifier, not an authorization @@ -182,7 +290,7 @@ a pointer to `UseClaimsBasedSessionIsolation` / `IsolationKeyScopedAgentSessionStore`. Today nothing on those surfaces flags the risk or points readers at the new safe-path APIs. -### G3. Decorator approach mutates `conversationId` seen by inner stores, undocumented +#### B-G3. Decorator approach mutates `conversationId` seen by inner stores, undocumented `IsolationKeyScopedAgentSessionStore` rewrites the `conversationId` to `{escapedKey}::{conversationId}` before forwarding. Two things follow @@ -198,7 +306,7 @@ A short note on `IsolationKeyScopedAgentSessionStore` (and ideally on `AgentSessionStore` itself — "implementations should treat `conversationId` as opaque") would close this. -### G4. Only one provider implementation ships; `ClaimType` lookup is single-claim only +#### B-G4. Only one provider implementation ships; `ClaimType` lookup is single-claim only The MSRC suggested fix uses `ClaimTypes.NameIdentifier ?? Identity.Name` as the fallback chain. @@ -212,7 +320,7 @@ providers (e.g. `Identity.Name`, an HTTP-header provider, an mTLS subject provider), would close the discoverability gap on the safe path. -### G5. AG-UI sample is a commented reminder, not a working safe configuration +#### B-G5. AG-UI sample is a commented reminder, not a working safe configuration `samples/05-end-to-end/AGUIClientServer/AGUIServer/Program.cs` adds a single commented-out line. The sample's runtime behaviour is unchanged; @@ -221,9 +329,9 @@ sample should compile and run with `UseClaimsBasedSessionIsolation` plus a working auth scheme so the safe pattern is shown end-to-end. The sample also needs to demonstrate registering the store through `WithSessionStore(...)` rather than via raw `AddKeyedSingleton` so that -the decorator chain actually engages — unless **G1** is fixed first. +the decorator chain actually engages — unless **B-G1** is fixed first. -### G6. Asymmetry between A2A and AG-UI auto-wrap behaviour +#### B-G6. Asymmetry between A2A and AG-UI auto-wrap behaviour `A2AServerServiceCollectionExtensions.CreateA2AServer` was edited to auto-wrap any keyed store with `IsolationKeyScopedAgentSessionStore` @@ -240,34 +348,152 @@ called out for the persistence default. (Note: this is **not** a the same when an isolation provider is registered" ask. The out-of-box no-provider behaviour stays unchanged.) -## Summary scorecard (AG-UI threat surface) +### Summary scorecard (AG-UI threat surface) | MSRC ask | Status | |---|---| | #1 Trust-model XML on `MapAGUI`, `InMemoryAgentSessionStore`, `AgentSessionStore` | ❌ Not addressed | | #2 Principal-scoping inside the AG-UI handler (versioned opt-in) | ⚠️ Equivalent opt-in mechanism exists (`UseClaimsBasedSessionIsolation` + decorator), but the AG-UI handler does not engage it for stores registered the canonical way | -| #3 Optional principal-aware store overload | ✅ Achieved via decorator (with caveats — see G3) | +| #3 Optional principal-aware store overload | ✅ Achieved via decorator (with caveats — see B-G3) | | AG-UI sample shows safe pattern | ⚠️ Commented reminder only | | Symmetric behaviour with the A2A hosting layer | ❌ A2A auto-wraps in handler; AG-UI does not | -## Recommended follow-ups (priority order) +--- -1. **Auto-wrap inside `MapAGUI`** mirroring the A2A pattern: if the - resolved keyed store does not expose `IsolationKeyScopedAgentSessionStore` - via `GetService<...>()`, wrap it with - `Strict = isolationKeyProvider != null`. This preserves the - out-of-box no-provider behaviour while making the safe path engage - automatically as soon as the integrator opts in via - `UseClaimsBasedSessionIsolation` (closes **G1** and **G6**). -2. Add the trust-model XML paragraph to `MapAGUI`, - `InMemoryAgentSessionStore`, and `AgentSessionStore`, each pointing - at `UseClaimsBasedSessionIsolation` and - `IsolationKeyScopedAgentSessionStore` (closes **G2**). -3. Document the `conversationId`-mutation contract that the decorator - imposes on inner stores (closes **G3**). -4. Either widen `ClaimsIdentitySessionIsolationKeyProvider` to a - fallback chain (`ClaimType` → `Identity.Name`) or ship one or two - additional providers (header / mTLS subject) so the safe path is - discoverable beyond the single Claims case (closes **G4**). -5. Convert the AG-UI sample from a commented reminder into a working - safe-by-default configuration (closes **G5**). +## Part C — Synthesis across the two reports + +The A2A and AG-UI reports describe the **same root cause** at the +abstraction layer (`AgentSessionStore` only knows +`(agent, conversationId)`; the wire-supplied id is the entire lookup +key) reached through two different hosting surfaces. Read together they +say: + +1. **The contract gap is real, and the PR closes it correctly.** Adding + a principal dimension by *decoration* (`IsolationKeyScopedAgentSessionStore`) + rather than by adding a third parameter to every `AgentSessionStore` + override was the right call: it keeps the contract source-compatible, + it stacks naturally with future scoping dimensions (tenant, session + tag), and it lets a single isolation provider cover every hosting + surface without per-surface plumbing. +2. **"Auto-wrap when the integrator opts in" is the right shape under + the team's design constraint.** The PR does not change defaults: with + no `UseClaimsBasedSessionIsolation` registered, both A2A and the + builder path remain bit-for-bit compatible. The wrapper engages only + when an isolation provider is in the container, and even then defers + to `Strict = false` when the provider returns no key — so the + first-run / single-user / no-auth scenario continues to function. +3. **Coverage is asymmetric across hosting surfaces.** The PR added the + handler-level auto-wrap to the A2A path (`CreateA2AServer`) and to + the agent-builder path (`WithSessionStore`), but **not** to the + AG-UI handler (`MapAGUI`). The result: an integrator who follows + the canonical AG-UI snippet + (`AddKeyedSingleton(name, store)` + + `endpoints.MapAGUI(name, "/ag-ui")`) plus + `UseClaimsBasedSessionIsolation(...)` still gets an unscoped store. + This is the headline residual issue (**B-G1 / B-G6**) and is also + the reason the AG-UI report exists as a separate finding even after + the A2A path is fixed. +4. **Documentation and samples lag the runtime fix on both surfaces.** + `AgentSessionStore`, `InMemoryAgentSessionStore`, the A2A quickstart, + and the AG-UI quickstart all still describe the persistence contract + without flagging the trust assumption that `(agent, conversationId)` + carries no principal dimension and that multi-user hosts must compose + one. The new types are well-documented, but nothing on the legacy + IntelliSense path points readers at them. A second-time reader of + either MSRC report could read the entire surface and miss the safe + path (**A-G1 / B-G2**). +5. **One shared caveat applies to both surfaces.** Because scoping is + done by rewriting `conversationId` to `{escapedKey}::{conversationId}` + before forwarding, any inner store that logs, echoes, audits, or + constrains the shape of `conversationId` is silently affected. This + needs to be a documented contract on `IsolationKeyScopedAgentSessionStore` + (and ideally a one-line "treat `conversationId` as opaque" note on + `AgentSessionStore` itself). It applies identically to A2A + (**A-G3**) and AG-UI (**B-G3**). + +### Combined scorecard + +| MSRC ask (collapsed across both reports) | Status | +|---|---| +| Make principal-scoping *expressible* without rewriting every store | ✅ Decorator (`IsolationKeyScopedAgentSessionStore`) | +| Auto-engage scoping on the **A2A** canonical registration shape when an isolation provider is present | ✅ `CreateA2AServer` wraps | +| Auto-engage scoping on the **AG-UI** canonical registration shape when an isolation provider is present | ❌ `MapAGUI` does not wrap | +| Preserve out-of-box single-user/no-auth behaviour | ✅ `Strict = false` when no provider; `NoopAgentSessionStore` default unchanged | +| Trust-model XML on `AgentSessionStore`, `InMemoryAgentSessionStore`, `MapAGUI`, A2A entry points | ❌ Not addressed | +| Document the `conversationId`-mutation contract the decorator imposes | ❌ Not addressed | +| Ship enough provider variants to be discoverable beyond a single Claims case | ⚠️ Only `ClaimsIdentitySessionIsolationKeyProvider`, single-claim, no `Identity.Name` fallback | +| Working safe-path samples for both A2A and AG-UI | ❌ AG-UI sample is a commented reminder; no A2A sample update | + +--- + +## Part D — Next steps (respecting "out-of-box default must keep working") + +All of the following preserve the design constraint: **with no +isolation provider registered, behaviour must not change** — same +default store (`NoopAgentSessionStore`), same wire contract, same +prototype-friendly first-run experience. None of them require an +integrator to register auth or an isolation provider before persistence +works. + +Listed in priority order, with the closure mapping back to the gap IDs +above. + +1. **Mirror the A2A auto-wrap inside `MapAGUI`.** In the AG-UI endpoint + extension, after resolving the keyed `AgentSessionStore`, walk the + `GetService()` chain; if it is + absent, wrap with `Strict = isolationKeyProvider != null`. The + `Strict = false` branch keeps the "no provider registered → bare + `conversationId` passes through" behaviour the constraint requires. + The `Strict = true` branch engages only when the integrator has + already opted in by calling `UseClaimsBasedSessionIsolation(...)`. + *Closes B-G1 and B-G6 (asymmetry); produces no out-of-box behaviour + change.* + +2. **Add the trust-model XML paragraph to the five legacy surfaces.** + `AgentSessionStore`, `InMemoryAgentSessionStore`, `MapAGUI`, the A2A + `CreateA2AServer` / `AsA2AServer` entry points. The text needs to + say: (a) `conversationId` / `ThreadId` / `contextId` arrives from + the wire and is not an authorization token; (b) `(agent, conversationId)` + has no principal dimension, so persistent stores are single-namespace + by default; (c) multi-user hosts should compose one via + `UseClaimsBasedSessionIsolation` or a custom + `SessionIsolationKeyProvider`. Pure docs change, no behaviour + impact. *Closes A-G1, B-G2.* + +3. **Document the `conversationId`-mutation contract** on + `IsolationKeyScopedAgentSessionStore` and add a one-liner on + `AgentSessionStore` saying inner stores must treat `conversationId` + as opaque (don't parse, don't impose length/charset constraints, + expect logs/telemetry to surface it verbatim). *Closes A-G3, B-G3.* + +4. **Convert the AG-UI sample into a working safe configuration** (and + add an equivalent slice to one A2A sample). The sample should + register an auth scheme, call `UseClaimsBasedSessionIsolation`, and + register the store via the canonical endpoint shape — so once + step 1 lands, copy-paste users land on the safe path automatically. + The unsafe path must still be reachable (constraint), but the + in-tree sample shouldn't model it. *Closes A-G2 follow-up, B-G5.* + +5. **Widen the provider story.** Either extend + `ClaimsIdentitySessionIsolationKeyProvider` to take an ordered + fallback chain (e.g. `ClaimTypes.NameIdentifier` → `Identity.Name`), + or ship one or two additional providers (HTTP-header, + mTLS-subject) so the safe path is discoverable beyond a single + claims-based deployment. Whichever shape is chosen must continue to + return `null` for anonymous requests so the `Strict = false` + pass-through semantics keep holding. *Closes B-G4.* + +### What is explicitly **not** proposed + +- Flipping the default registered store from `NoopAgentSessionStore` to + any persistent default — out of scope per the constraint. +- Requiring `UseClaimsBasedSessionIsolation` (or any + `SessionIsolationKeyProvider`) before persistence works — out of + scope per the constraint. +- Adding a third `scopeId` parameter to `AgentSessionStore` virtuals + (would force every existing custom store to be recompiled and + re-implemented). The decorator approach already covers this case; + the only ask is a documented contract on inner stores. +- Treating an authenticated request without the configured claim as a + hard error in the no-isolation-provider scenario — only applies when + the integrator opted into `Strict = true` by registering a provider.