fix(secrets): injector-delivered servers must attach, not exec #120

Merged
michal merged 1 commits from feat/injector-attach-mode into main 2026-08-21 01:05:54 +00:00
2 changed files with 107 additions and 15 deletions
Showing only changes of commit 0a83f71648 - Show all commits

View File

@@ -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/<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 {
/** Session IDs per server for streamable-http protocol */
private sessions = new Map<string, string>();
@@ -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

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