Compare commits

..

1 Commits

Author SHA1 Message Date
Michal
0a83f71648 fix(secrets): injector-delivered servers must attach, not exec
Some checks failed
CI/CD / lint (pull_request) Successful in 1m17s
CI/CD / typecheck (pull_request) Successful in 2m41s
CI/CD / test (pull_request) Successful in 1m28s
CI/CD / build (pull_request) Successful in 2m18s
CI/CD / smoke (pull_request) Failing after 3m3s
CI/CD / publish (pull_request) Has been skipped
With injected delivery the credentials exist ONLY in PID 1's environment:
the container command is a shell that sources /vault/secrets/<name> 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
2026-08-21 02:05:51 +01:00
2 changed files with 107 additions and 15 deletions

View File

@@ -62,6 +62,59 @@ function parseStreamableResponse(body: string): McpProxyResponse {
return JSON.parse(body) as 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/<name> 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 { export class McpProxyService {
/** Session IDs per server for streamable-http protocol */ /** Session IDs per server for streamable-http protocol */
private sessions = new Map<string, string>(); private sessions = new Map<string, string>();
@@ -159,20 +212,15 @@ export class McpProxyService {
// - command set → exec the given command in the container. // - command set → exec the given command in the container.
// - dockerImage only → attach to PID 1 (image entrypoint IS the MCP server). // - dockerImage only → attach to PID 1 (image entrypoint IS the MCP server).
// - nothing → unreachable, reject. // - nothing → unreachable, reject.
const runtime = (server.runtime as string | null) ?? 'node'; const mode = chooseStdioMode({
let mode: StdioMode; name: server.name as string,
if (command && command.length > 0) { id: server.id as string,
mode = { kind: 'exec', command }; secretDelivery: server.secretDelivery as string | null,
} else if (packageName) { command,
mode = { kind: 'exec', command: buildRuntimeSpawnCmd(runtime, packageName) }; packageName,
} else if (dockerImage) { dockerImage,
mode = { kind: 'attach' }; runtime: server.runtime as string | null,
} else { });
throw new InvalidStateError(
`Server '${server.name}' (${server.id}) uses STDIO transport but has no ` +
`packageName, command, or dockerImage. Configure one of these.`,
);
}
// Try persistent connection first // Try persistent connection first
try { try {
@@ -181,7 +229,7 @@ export class McpProxyService {
this.removeClient(instance.containerId); this.removeClient(instance.containerId);
// Fall back to one-shot exec when we have a command to run. // Fall back to one-shot exec when we have a command to run.
if (mode.kind === 'exec') { 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 // Attach mode has no one-shot equivalent, but the failure is usually
// a stale pipe from an in-place container restart — retry once // a stale pipe from an in-place container restart — retry once

View File

@@ -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/<name> 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/);
});
});