From 0a83f7164894a2e9bf6412053341ad0663ea625f Mon Sep 17 00:00:00 2001 From: Michal Date: Fri, 21 Aug 2026 02:05:51 +0100 Subject: [PATCH] fix(secrets): injector-delivered servers must attach, not exec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With injected delivery the credentials exist ONLY in PID 1's environment: the container command is a shell that sources /vault/secrets/ and execs the real server, so the values live in that process and nowhere else — not in the pod spec, which is the whole point. mcpd picks its STDIO mode from the server's shape: an explicit command or a packageName selects `exec`, a bare dockerImage selects `attach`. `exec` spawns a NEW process inside the container, which never sourced the file and therefore starts with empty credentials. The server comes up, answers tools/list, and fails every authenticated call — precisely the silent empty-token failure this feature exists to prevent. Seen as `Readiness check (list_datasources) failed: process exited 1` on a pod whose PID 1 demonstrably held the token (verified via /proc/1/environ: GRAFANA_URL set, token 46 chars, nothing in the pod spec). So secretDelivery: injector now forces attach regardless of server shape. Safe because wrapCommandForInjector execs rather than forks, so PID 1 IS the server. It also means no bouncer or HTTP shim is needed — mcpd's PersistentStdio already holds one long-lived connection; it was simply pointed at the wrong process. Note this inverts an assumption I had earlier: image-entrypoint servers were already on the attach path and would have worked; it is the package-based ones that were broken. gitea remains the sole exception, and for the unrelated reason that it is distroless and has no shell to source the file. Extracts the decision as a pure `chooseStdioMode()` so it can be tested — it is subtle and fails silently. Six cases pinned, including that env delivery still execs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki --- src/mcpd/src/services/mcp-proxy-service.ts | 78 +++++++++++++++++----- src/mcpd/tests/stdio-mode.test.ts | 44 ++++++++++++ 2 files changed, 107 insertions(+), 15 deletions(-) create mode 100644 src/mcpd/tests/stdio-mode.test.ts diff --git a/src/mcpd/src/services/mcp-proxy-service.ts b/src/mcpd/src/services/mcp-proxy-service.ts index 8e92079..df85367 100644 --- a/src/mcpd/src/services/mcp-proxy-service.ts +++ b/src/mcpd/src/services/mcp-proxy-service.ts @@ -62,6 +62,59 @@ function parseStreamableResponse(body: string): McpProxyResponse { return JSON.parse(body) as McpProxyResponse; } +/** + * Decide how mcpd opens a STDIO session against a running container. + * + * attach → connect to PID 1's stdin/stdout + * exec → spawn a NEW process inside the container + * + * Pure and exported so the choice is testable: it is subtle, and getting it + * wrong fails silently rather than loudly (see the injector case below). + */ +export function chooseStdioMode(server: { + name: string; + id: string; + secretDelivery?: string | null; + command?: string[] | null; + packageName?: string | null; + dockerImage?: string | null; + runtime?: string | null; +}): StdioMode { + // Injected delivery MUST attach, whatever the server type. + // + // The secrets exist only in PID 1's environment: the container command is a + // shell that sources /vault/secrets/ and execs the real server, so the + // values live in that process and nowhere else — not in the pod spec, which + // is the entire point of the feature. + // + // `exec` spawns a NEW process, which never sourced the file and so starts + // with empty credentials. The server comes up, answers tools/list, and fails + // every authenticated call — the exact silent-empty-token failure this + // feature exists to prevent. Observed as + // `Readiness check (list_datasources) failed: process exited 1` on a pod + // whose PID 1 demonstrably held the token. + // + // Safe because wrapCommandForInjector execs rather than forks, so PID 1 IS + // the server process. + if (server.secretDelivery === 'injector') return { kind: 'attach' }; + + if (server.command !== null && server.command !== undefined && server.command.length > 0) { + return { kind: 'exec', command: server.command }; + } + if (server.packageName !== null && server.packageName !== undefined && server.packageName !== '') { + return { kind: 'exec', command: buildRuntimeSpawnCmd(server.runtime ?? 'node', server.packageName) }; + } + // Image entrypoint IS the MCP server. + if (server.dockerImage !== null && server.dockerImage !== undefined && server.dockerImage !== '') { + return { kind: 'attach' }; + } + + throw new InvalidStateError( + `Server '${server.name}' (${server.id}) uses STDIO transport but has no ` + + `packageName, command, or dockerImage. Configure one of these.`, + ); +} + export class McpProxyService { /** Session IDs per server for streamable-http protocol */ private sessions = new Map(); @@ -159,20 +212,15 @@ export class McpProxyService { // - command set → exec the given command in the container. // - dockerImage only → attach to PID 1 (image entrypoint IS the MCP server). // - nothing → unreachable, reject. - const runtime = (server.runtime as string | null) ?? 'node'; - let mode: StdioMode; - if (command && command.length > 0) { - mode = { kind: 'exec', command }; - } else if (packageName) { - mode = { kind: 'exec', command: buildRuntimeSpawnCmd(runtime, packageName) }; - } else if (dockerImage) { - mode = { kind: 'attach' }; - } else { - throw new InvalidStateError( - `Server '${server.name}' (${server.id}) uses STDIO transport but has no ` + - `packageName, command, or dockerImage. Configure one of these.`, - ); - } + const mode = chooseStdioMode({ + name: server.name as string, + id: server.id as string, + secretDelivery: server.secretDelivery as string | null, + command, + packageName, + dockerImage, + runtime: server.runtime as string | null, + }); // Try persistent connection first try { @@ -181,7 +229,7 @@ export class McpProxyService { this.removeClient(instance.containerId); // Fall back to one-shot exec when we have a command to run. if (mode.kind === 'exec') { - return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, runtime); + return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, (server.runtime as string | null) ?? 'node'); } // Attach mode has no one-shot equivalent, but the failure is usually // a stale pipe from an in-place container restart — retry once diff --git a/src/mcpd/tests/stdio-mode.test.ts b/src/mcpd/tests/stdio-mode.test.ts new file mode 100644 index 0000000..a5342d9 --- /dev/null +++ b/src/mcpd/tests/stdio-mode.test.ts @@ -0,0 +1,44 @@ +/** + * How mcpd opens a STDIO session: attach to PID 1, or exec a new process. + * + * Subtle and silent when wrong. With injected secret delivery the credentials + * exist ONLY in PID 1's environment (a shell sourced /vault/secrets/ and + * exec'd the server), so an `exec` starts a process with empty credentials — + * the server comes up, answers tools/list, and fails every authenticated call. + */ +import { describe, it, expect } from 'vitest'; +import { chooseStdioMode } from '../src/services/mcp-proxy-service.js'; + +const base = { name: 's', id: 'id1' }; + +describe('chooseStdioMode', () => { + it('attaches for an injector server even though it has a packageName', () => { + // The regression: packageName would otherwise select exec, and exec loses + // the secrets entirely. + expect(chooseStdioMode({ ...base, secretDelivery: 'injector', packageName: '@leval/mcp-grafana' })) + .toEqual({ kind: 'attach' }); + }); + + it('attaches for an injector server even though it has an explicit command', () => { + expect(chooseStdioMode({ ...base, secretDelivery: 'injector', command: ['node', 'x.js'] })) + .toEqual({ kind: 'attach' }); + }); + + it('still execs a package server on the default env delivery', () => { + const m = chooseStdioMode({ ...base, secretDelivery: 'env', packageName: '@leval/mcp-grafana', runtime: 'node' }); + expect(m.kind).toBe('exec'); + }); + + it('still prefers an explicit command over packageName on env delivery', () => { + expect(chooseStdioMode({ ...base, secretDelivery: 'env', command: ['node', 'x.js'], packageName: 'p' })) + .toEqual({ kind: 'exec', command: ['node', 'x.js'] }); + }); + + it('attaches for an image-entrypoint server, as before', () => { + expect(chooseStdioMode({ ...base, dockerImage: 'gitea/mcp:latest' })).toEqual({ kind: 'attach' }); + }); + + it('rejects a server with no way to start', () => { + expect(() => chooseStdioMode({ ...base })).toThrow(/packageName, command, or dockerImage/); + }); +}); -- 2.49.1