diff --git a/CLAUDE.md b/CLAUDE.md index cbe5242..b2f978a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -251,9 +251,30 @@ The Agent tab must not inherit the admin MCP token. `/api/agent/session` mints a on purpose (per-browser cookie), so a View as / new-thread Agent chat tagged `profile: u-marts-editor` still sends the admin MCP bearer. Chat-access policy then `resolveEffectivePolicy(..., actorIsAdmin=true)` → `none()`. The - provisioner mirrors the target user's token onto every `deepsql.token` the - live process might re-read (`DEEPSQL_TOKEN_FILE` mtime cache in - `mcp/deepsql-phase1-lib.js`). + provisioner writes the target user's token to `$HERMES_HOME/deepsql.token`, + the one shared path the live process may have started from + (`DEEPSQL_TOKEN_FILE` mtime cache in `mcp/deepsql-phase1-lib.js`). + + **Never fan that write out across `profiles/*/deepsql.token`.** It did once, + and made the agent credential globally last-writer-wins: any user opening the + Agent tab overwrote every other user's token, so their agent authenticated as + the newcomer. Verified end to end — `analyst`'s agent read an admin-only + connection (403 on their own session, 200 with the agent token, 133 vault + tables) and the `EDITOR_QUERY_EXECUTED` row named **admin**, not analyst. Two + concurrent users was the whole trigger; no impersonation needed. The + provisioner self-test asserted the fan-out as *correct* (it modelled only the + View-as case, where overwriting is desired), so a green suite guarded the bug — + it now asserts the opposite, that provisioning B leaves A's token intact. + + Because that root file is still shared, the real guard is server-side: + `McpTokenAuthenticationFilter` refuses an MCP token whose owner differs from + the request's `DEEPSQL_MCP_USER_ID` claim (sent as `X-DeepSQL-Client-Agent`), + answering `401 mcp_identity_mismatch`. The claim is only ever used to *refuse*, + never to grant, so forging it cannot widen access. A claim that isn't a real + DeepSQL username is ignored, which is what keeps editor/CLI MCP installs + (`cursor`, `claude-desktop`, any `--caller-agent`) working. `probeMcpAuth` + sends the same header so the boot health check exercises the binding instead + of bypassing it. 6. **The agent image build clones two third-party repos over the public internet, unauthenticated.** `agent/Dockerfile` fetches `NousResearch/hermes-agent` and `nesquena/hermes-webui` at build time. GitHub rate-limits unauthenticated diff --git a/backend/src/main/java/com/dbaagent/controller/AgentBridgeController.java b/backend/src/main/java/com/dbaagent/controller/AgentBridgeController.java index 0c6dd65..00bcabc 100644 --- a/backend/src/main/java/com/dbaagent/controller/AgentBridgeController.java +++ b/backend/src/main/java/com/dbaagent/controller/AgentBridgeController.java @@ -59,7 +59,7 @@ public ResponseEntity> session( Map response = new HashMap<>(); response.put("profile", bootstrap.profile()); response.put("username", username); - boolean mcpAuthOk = agentBridgeService.probeMcpAuth(bootstrap.token()); + boolean mcpAuthOk = agentBridgeService.probeMcpAuth(bootstrap.token(), username); response.put("mcpAuthOk", mcpAuthOk); if (!mcpAuthOk) { response.put("mcpAuthError", "The DeepSQL Agent could not authenticate against this API with its " diff --git a/backend/src/main/java/com/dbaagent/security/McpTokenAuthenticationFilter.java b/backend/src/main/java/com/dbaagent/security/McpTokenAuthenticationFilter.java index 19d5ba0..db1dec2 100644 --- a/backend/src/main/java/com/dbaagent/security/McpTokenAuthenticationFilter.java +++ b/backend/src/main/java/com/dbaagent/security/McpTokenAuthenticationFilter.java @@ -1,5 +1,7 @@ package com.dbaagent.security; +import com.dbaagent.repository.UserRepository; +import com.dbaagent.service.ClientContext; import com.dbaagent.service.McpTokenService; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -8,6 +10,8 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Value; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.userdetails.UserDetails; @@ -27,6 +31,7 @@ public class McpTokenAuthenticationFilter extends OncePerRequestFilter { private final McpTokenService mcpTokenService; private final CustomUserDetailsService userDetailsService; + private final UserRepository userRepository; @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) @@ -49,7 +54,26 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse return; } - mcpTokenService.authenticate(token, request.getRemoteAddr()).ifPresent(authenticatedToken -> { + var authenticated = mcpTokenService.authenticate(token, request.getRemoteAddr()); + if (authenticated.isPresent() && !declaredUserMatches(request, authenticated.get().username())) { + // The agent runtime keeps ONE MCP subprocess for every profile, and + // the provisioner rotates its credential on disk. A token belonging + // to a different user than the one this MCP process was started for + // means the credential was overwritten by someone else's Agent-tab + // open — authenticating it here would run this user's tools as that + // other user (cross-user read + falsified audit attribution). + // Fail closed: the caller must re-provision, not silently proceed. + log.warn("Rejecting MCP token for {} — request declares user {}", + authenticated.get().username(), declaredUser(request)); + response.setStatus(HttpStatus.UNAUTHORIZED.value()); + response.setContentType(MediaType.APPLICATION_JSON_VALUE); + response.getWriter().write("{\"error\":\"mcp_identity_mismatch\",\"message\":" + + "\"This Agent session's credential belongs to a different user. " + + "Reopen the Agent tab to continue.\"}"); + return; + } + + authenticated.ifPresent(authenticatedToken -> { UserDetails userDetails = userDetailsService.loadUserByUsername(authenticatedToken.username()); UsernamePasswordAuthenticationToken authentication = new UsernamePasswordAuthenticationToken( userDetails, @@ -64,4 +88,49 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse chain.doFilter(request, response); } + + /** + * The user this MCP process was provisioned for, as declared by the caller. + * + *

The agent provisioner writes {@code DEEPSQL_MCP_USER_ID=} into + * each profile's MCP server env, and the MCP shim forwards it as + * {@link ClientContext#HEADER_AGENT}. It is a *claim*, not a credential — it + * is only ever used to REFUSE a mismatched token, never to grant access, so + * a forged value cannot widen access beyond what the token already allows. + */ + private static String declaredUser(HttpServletRequest request) { + String declared = request.getHeader(ClientContext.HEADER_AGENT); + return declared == null || declared.isBlank() ? null : declared.trim(); + } + + /** + * True when the request carries no user claim (editor/CLI installs, curl, + * every pre-existing caller) or the claim matches the token's owner. + * + *

Absent claim stays permissive on purpose: {@code DEEPSQL_MCP_USER_ID} + * defaults to non-username values for editor installs ("cursor", + * "claude-code", "mcp-phase1"), and those tokens are not agent-provisioned, + * so there is no shared-credential hazard to guard against. Only a claim + * that looks like a DeepSQL agent profile identity is enforced. + */ + private boolean declaredUserMatches(HttpServletRequest request, String tokenOwner) { + String declared = declaredUser(request); + if (declared == null || tokenOwner == null) { + return true; + } + if (declared.equalsIgnoreCase(tokenOwner)) { + return true; + } + // The claim differs from the token owner. Enforce only when the claim + // names a real DeepSQL user — that is the agent-provisioner case, where + // DEEPSQL_MCP_USER_ID is a username and a mismatch means the shared + // token file was overwritten by another user's Agent-tab open. + // + // Editor/CLI clients put a *tool* name here ("cursor", "claude-desktop", + // "terminal", "mcp-phase1", or any --caller-agent value), which never + // resolves to a user, so those callers are unaffected. Matching against + // the user table rather than a denylist of sentinels keeps free-form + // --caller-agent values working without a list to maintain. + return userRepository.findByUsernameIgnoreCase(declared).isEmpty(); + } } diff --git a/backend/src/main/java/com/dbaagent/service/AgentBridgeService.java b/backend/src/main/java/com/dbaagent/service/AgentBridgeService.java index ec3d68c..28274ca 100644 --- a/backend/src/main/java/com/dbaagent/service/AgentBridgeService.java +++ b/backend/src/main/java/com/dbaagent/service/AgentBridgeService.java @@ -312,16 +312,30 @@ private void callProvisionerRevoke(String username) { * bearer credential against the local backend (loopback — this call never * leaves the box, so no external base-URL config is needed). * + *

Sends the same {@code X-DeepSQL-Client-Agent: } claim the + * provisioned MCP process will send, so the probe passes only if the token + * and the declared identity agree. A probe without that header would call a + * token healthy that the live MCP process cannot actually use. + * + * @param token the credential to verify + * @param username the identity the MCP process will declare for this profile * @return true if the API accepted the token (2xx), false on any * non-2xx/auth failure or network error. */ - public boolean probeMcpAuth(String token) { + public boolean probeMcpAuth(String token, String username) { if (token == null || token.isBlank()) { return false; } try { HttpRequest req = HttpRequest.newBuilder(URI.create(localApiBaseUrl + "/connections")) .header("Authorization", "Bearer " + token) + // Declare the same identity the provisioned MCP process will send + // (DEEPSQL_MCP_USER_ID -> X-DeepSQL-Client-Agent), so the probe + // exercises the identity-binding check in + // McpTokenAuthenticationFilter rather than bypassing it. Probing + // without this header would report a token as healthy even when + // the live MCP process's own calls will be refused. + .header(ClientContext.HEADER_AGENT, username) .timeout(Duration.ofSeconds(5)) .GET() .build(); diff --git a/backend/src/test/java/com/dbaagent/security/McpTokenAuthenticationFilterTest.java b/backend/src/test/java/com/dbaagent/security/McpTokenAuthenticationFilterTest.java index 75c5d47..fffd01e 100644 --- a/backend/src/test/java/com/dbaagent/security/McpTokenAuthenticationFilterTest.java +++ b/backend/src/test/java/com/dbaagent/security/McpTokenAuthenticationFilterTest.java @@ -1,5 +1,6 @@ package com.dbaagent.security; +import com.dbaagent.repository.UserRepository; import com.dbaagent.service.McpTokenService; import jakarta.servlet.ServletException; import org.junit.jupiter.api.AfterEach; @@ -22,6 +23,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -33,6 +36,9 @@ class McpTokenAuthenticationFilterTest { @Mock private CustomUserDetailsService userDetailsService; + @Mock + private UserRepository userRepository; + @InjectMocks private McpTokenAuthenticationFilter filter; @@ -67,4 +73,100 @@ void authenticateUsesResolvedUsernameWithoutTouchingLazyEntity() throws ServletE assertNotNull(SecurityContextHolder.getContext().getAuthentication()); assertEquals("alice", SecurityContextHolder.getContext().getAuthentication().getName()); } + + /** + * The cross-user token leak, at the layer that stops it. + * + *

The agent runtime keeps ONE MCP subprocess and the provisioner rotates + * its credential on disk, so another user's Agent-tab open could leave + * analyst's MCP process holding admin's token. Authenticating that token + * would run analyst's tools as admin — reading connections analyst has no + * grant for, and writing admin into the audit row. The request's own + * DEEPSQL_MCP_USER_ID claim ("analyst") contradicts the token owner + * ("admin"), which is the signal to refuse. + */ + @Test + void rejectsTokenWhoseOwnerDiffersFromTheDeclaredMcpUser() throws ServletException, IOException { + ReflectionTestUtils.setField(filter, "authEnabled", true); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/connections"); + request.addHeader("Authorization", "Bearer dsql_mcp_public.secret"); + // This MCP process was provisioned for analyst... + request.addHeader("X-DeepSQL-Client-Agent", "analyst"); + request.setRemoteAddr("127.0.0.1"); + + MockHttpServletResponse response = new MockHttpServletResponse(); + MockFilterChain chain = new MockFilterChain(); + + when(mcpTokenService.looksLikeMcpToken("dsql_mcp_public.secret")).thenReturn(true); + // ...but the token file was overwritten with admin's credential. + when(mcpTokenService.authenticate("dsql_mcp_public.secret", "127.0.0.1")) + .thenReturn(Optional.of(new McpTokenService.AuthenticatedMcpToken(9L, "admin"))); + when(userRepository.findByUsernameIgnoreCase("analyst")) + .thenReturn(Optional.of(new com.dbaagent.model.User())); + + filter.doFilter(request, response, chain); + + assertNull(SecurityContextHolder.getContext().getAuthentication(), + "a mismatched MCP credential must not authenticate anyone"); + assertEquals(401, response.getStatus()); + assertTrue(response.getContentAsString().contains("mcp_identity_mismatch")); + assertNull(chain.getRequest(), "the request must not reach downstream handlers"); + } + + /** The normal agent case: the claim matches the token owner. */ + @Test + void allowsTokenWhenDeclaredMcpUserMatchesOwner() throws ServletException, IOException { + ReflectionTestUtils.setField(filter, "authEnabled", true); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/connections"); + request.addHeader("Authorization", "Bearer dsql_mcp_public.secret"); + request.addHeader("X-DeepSQL-Client-Agent", "analyst"); + request.setRemoteAddr("127.0.0.1"); + + MockHttpServletResponse response = new MockHttpServletResponse(); + MockFilterChain chain = new MockFilterChain(); + + when(mcpTokenService.looksLikeMcpToken("dsql_mcp_public.secret")).thenReturn(true); + when(mcpTokenService.authenticate("dsql_mcp_public.secret", "127.0.0.1")) + .thenReturn(Optional.of(new McpTokenService.AuthenticatedMcpToken(9L, "analyst"))); + when(userDetailsService.loadUserByUsername("analyst")) + .thenReturn(new User("analyst", "ignored", + List.of(new SimpleGrantedAuthority("ROLE_DEVELOPER")))); + + filter.doFilter(request, response, chain); + + assertEquals("analyst", SecurityContextHolder.getContext().getAuthentication().getName()); + } + + /** + * Editor/CLI installs put a *tool* name in this header ("cursor", + * "claude-desktop", any --caller-agent value). Those tokens are not + * agent-provisioned, so the claim must not be compared against a username — + * otherwise every editor MCP install would 401. + */ + @Test + void allowsEditorClientAgentThatIsNotADeepSqlUsername() throws ServletException, IOException { + ReflectionTestUtils.setField(filter, "authEnabled", true); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/connections"); + request.addHeader("Authorization", "Bearer dsql_mcp_public.secret"); + request.addHeader("X-DeepSQL-Client-Agent", "cursor"); + request.setRemoteAddr("127.0.0.1"); + + MockHttpServletResponse response = new MockHttpServletResponse(); + MockFilterChain chain = new MockFilterChain(); + + when(mcpTokenService.looksLikeMcpToken("dsql_mcp_public.secret")).thenReturn(true); + when(mcpTokenService.authenticate("dsql_mcp_public.secret", "127.0.0.1")) + .thenReturn(Optional.of(new McpTokenService.AuthenticatedMcpToken(11L, "bob"))); + when(userRepository.findByUsernameIgnoreCase("cursor")).thenReturn(Optional.empty()); + when(userDetailsService.loadUserByUsername("bob")) + .thenReturn(new User("bob", "ignored", + List.of(new SimpleGrantedAuthority("ROLE_DEVELOPER")))); + + filter.doFilter(request, response, chain); + + assertEquals("bob", SecurityContextHolder.getContext().getAuthentication().getName()); + } } diff --git a/scripts/local-agent-provisioner.py b/scripts/local-agent-provisioner.py index 3dc59b5..9141204 100755 --- a/scripts/local-agent-provisioner.py +++ b/scripts/local-agent-provisioner.py @@ -76,27 +76,31 @@ def iter_profile_homes(hermes_home: Path): def mirror_token_for_live_mcp(hermes_home: Path, token: str) -> list[Path]: - """Copy the provisioned token onto every file a live MCP process might read. + """Publish the provisioned token to the ONE shared path a live MCP process + may have been started from — without touching any other user's profile. Hermes starts one DeepSQL MCP stdio server from whichever profile first loaded ``mcp_servers``, then keeps that subprocess for the life of the Agent API. ``POST /api/profile/switch`` is explicitly ``process_wide=False`` — it sets a cookie / thread-local for sessions, but does **not** respawn - MCP with the target profile's ``DEEPSQL_AUTH_TOKEN``. - - The MCP client re-reads ``DEEPSQL_TOKEN_FILE`` per request (mtime cache). - Writing only ``profiles/u-/deepsql.token`` therefore leaves the - already-running server on the previous user's credential (in practice the - admin who first opened the Agent tab). View as / a new chat thread then - executes SQL as admin and skips chat-access policy. - - Mirror onto ``$HERMES_HOME/deepsql.token`` and every profile token file so - whichever path the live process was started with picks up the rotation. + MCP with the target profile's ``DEEPSQL_AUTH_TOKEN``. The MCP client + re-reads ``DEEPSQL_TOKEN_FILE`` per request (mtime cache), so the shared + root file is what lets a rotation reach that already-running process. + + This previously also wrote every ``profiles/*/deepsql.token``, which made + the credential globally last-writer-wins: any user opening the Agent tab + overwrote every other user's token, so their agent then authenticated as + the newcomer — a cross-user read whose audit row named the wrong person. + Two concurrent users were enough; no impersonation required. + + The root file alone is still shared, so a mismatched credential can still + reach the live process. That is why the *backend* is the real guard: + ``McpTokenAuthenticationFilter`` refuses a token whose owner differs from + the request's ``DEEPSQL_MCP_USER_ID`` claim, so a stale/foreign token fails + closed with ``mcp_identity_mismatch`` instead of silently reading another + user's data. Never restore the per-profile fan-out. """ - written = [write_token_file(hermes_home, token)] - for home in iter_profile_homes(hermes_home): - written.append(write_token_file(home, token)) - return written + return [write_token_file(hermes_home, token)] def ensure_profile(name: str) -> Path: @@ -284,34 +288,54 @@ def _handle_revoke(self): def _self_test() -> int: - """Prove token mirroring covers the process-global MCP watch path.""" + """Prove one user's Agent-tab open cannot overwrite another user's credential. + + Regression guard for the cross-user token leak: this test previously + asserted the OPPOSITE — that a rotation lands on *every* profile token file + — which encoded the bug as the requirement and would have failed on the fix. + The property that actually matters is isolation: provisioning B leaves A's + credential intact. + """ import tempfile import traceback hermes_home = Path(tempfile.mkdtemp(prefix="deepsql-provisioner-test-")) try: admin = hermes_home / "profiles" / "u-admin" - editor = hermes_home / "profiles" / "u-marts-editor" + analyst = hermes_home / "profiles" / "u-analyst" admin.mkdir(parents=True) - editor.mkdir(parents=True) - write_token_file(admin, "dsql_mcp_admin.old") - write_token_file(editor, "dsql_mcp_editor.old") - - rotated = "dsql_mcp_editor.live" - written = mirror_token_for_live_mcp(hermes_home, rotated) - paths = {p.resolve() for p in written} - expected = { - token_file_for(hermes_home).resolve(), - token_file_for(admin).resolve(), - token_file_for(editor).resolve(), - } - if paths != expected: - raise AssertionError(f"mirrored paths {paths} != {expected}") - for path in expected: - body = path.read_text().strip() - if body != rotated: - raise AssertionError(f"{path} still {body!r}, expected {rotated!r}") - print("ok: live MCP token is mirrored onto every profile token file") + analyst.mkdir(parents=True) + + # Each user opens the Agent tab; each gets their own credential. + write_token_file(analyst, "dsql_mcp_analyst.tok") + mirror_token_for_live_mcp(hermes_home, "dsql_mcp_analyst.tok") + write_token_file(admin, "dsql_mcp_admin.tok") + mirror_token_for_live_mcp(hermes_home, "dsql_mcp_admin.tok") + + # The later open must NOT have rewritten the earlier user's token. + analyst_now = token_file_for(analyst).read_text().strip() + if analyst_now != "dsql_mcp_analyst.tok": + raise AssertionError( + f"cross-user token leak: u-analyst holds {analyst_now!r}, " + "expected 'dsql_mcp_analyst.tok'. A user's Agent-tab open must " + "never overwrite another user's credential." + ) + admin_now = token_file_for(admin).read_text().strip() + if admin_now != "dsql_mcp_admin.tok": + raise AssertionError(f"u-admin holds {admin_now!r}, expected own token") + + # The shared root file (the live-process watch path) still rotates, so + # View as keeps working; the backend rejects it if it names another user. + root_now = token_file_for(hermes_home).read_text().strip() + if root_now != "dsql_mcp_admin.tok": + raise AssertionError(f"root token file {root_now!r} did not rotate") + + # And the fan-out must not come back. + paths = {p.resolve() for p in mirror_token_for_live_mcp(hermes_home, "x")} + if paths != {token_file_for(hermes_home).resolve()}: + raise AssertionError(f"mirror touched more than the root file: {paths}") + + print("ok: per-user tokens stay isolated; only the shared root file rotates") return 0 except Exception: traceback.print_exc() diff --git a/src/components/AgentChat/AgentChatPanel.jsx b/src/components/AgentChat/AgentChatPanel.jsx index f2ec796..39fe406 100644 --- a/src/components/AgentChat/AgentChatPanel.jsx +++ b/src/components/AgentChat/AgentChatPanel.jsx @@ -191,7 +191,25 @@ export default function AgentChatPanel({ connectionId, connectionName, canManage queueMicrotask(() => proposeFromLastTurn()) } }, - onError: () => { setMessages((m) => updateLast(m, (a) => ({ ...a, streaming: false, error: true }))); setSending(false); esRef.current = null }, + // The stream is an EventSource, so we never see a status code here — + // every cause (session expiry at the nginx auth gate, agent restart, + // a credential the backend refused as mcp_identity_mismatch) arrives + // as one opaque event. Re-bootstrap to find out which: it returns the + // real reason, and for a mismatched/rotated credential it also + // re-provisions this user's token so a retry works. + onError: async () => { + setMessages((m) => updateLast(m, (a) => ({ ...a, streaming: false, error: true }))) + setSending(false); esRef.current = null + try { + const { mcpAuthOk, mcpAuthError } = await agentChatAPI.bootstrap(connectionId) + if (mcpAuthOk === false) { + setAuthBlocked(true) + setBootError(mcpAuthError || 'Agent cannot reach DeepSQL (auth). Reconnect / check Agent runtime.') + } + } catch (e) { + setBootError(e?.message || 'The agent connection dropped. Retry to reconnect.') + } + }, }) } catch (e) { setMessages((m) => updateLast(m, (a) => ({ ...a, streaming: false, error: true, content: a.content || `Error: ${e?.message || 'request failed'}` })))