From ec35e1cc36aec5ed8c4a760b9d6e59eef385b180 Mon Sep 17 00:00:00 2001 From: Michal Date: Fri, 21 Aug 2026 01:28:21 +0100 Subject: [PATCH] fix(secrets): the injector wrapper must replace the entrypoint, not extend it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught migrating a real server: the pod crashlooped with `.: cannot open /vault/secrets/grafana-creds`, and the reason was in the generated spec: args: ["/bin/sh","-c",". /vault/secrets/grafana-creds; exec \"$0\" \"$@\"", "@leval/mcp-grafana"] mcpd deliberately maps ContainerSpec.command -> k8s `args` so a package server keeps its runner image's ENTRYPOINT (`npx -y`, `uvx`). Putting the sourcing wrapper there meant the pod actually ran `npx -y /bin/sh -c '...'` — npx trying to resolve a package called /bin/sh. The agent had rendered the file correctly; nothing ever sourced it. Adds `ContainerSpec.entrypoint`, which maps to k8s `command` and so REPLACES the image entrypoint, and has wrapCommand fold that entrypoint into the argv it returns (`npx -y` / `uvx` for package servers, the server's own `entrypoint` field for dockerImage servers — already required at validation for exactly this reason). Two tests pin it: a wrapped server emits `command` and no `args`; an unwrapped one still emits `args` and no `command`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki --- src/mcpd/src/services/instance.service.ts | 7 ++- .../src/services/k8s/manifest-generator.ts | 7 ++- src/mcpd/src/services/orchestrator.ts | 11 ++++ .../src/services/server-identity.service.ts | 53 ++++++++++++------- src/mcpd/tests/injector-manifest.test.ts | 20 +++++++ 5 files changed, 76 insertions(+), 22 deletions(-) diff --git a/src/mcpd/src/services/instance.service.ts b/src/mcpd/src/services/instance.service.ts index 14f88ad..4e68821 100644 --- a/src/mcpd/src/services/instance.service.ts +++ b/src/mcpd/src/services/instance.service.ts @@ -564,8 +564,13 @@ export class InstanceService { spec.serviceAccountName = identity; spec.automountServiceAccountToken = true; spec.annotations = this.serverIdentity!.annotationsFor(identity, spec.envFromSecret); + // Replaces the image entrypoint (see wrapCommand); the original argv + // is folded in, so spec.command must not also be emitted as args. const wrapped = this.serverIdentity!.wrapCommand(server, spec.command); - if (wrapped !== undefined) spec.command = wrapped; + if (wrapped !== undefined) { + spec.entrypoint = wrapped; + delete spec.command; + } } catch (idErr) { const msg = idErr instanceof Error ? idErr.message : String(idErr); return this.markInstanceError(instance, `secret identity provisioning failed: ${msg}`); diff --git a/src/mcpd/src/services/k8s/manifest-generator.ts b/src/mcpd/src/services/k8s/manifest-generator.ts index c46ee3f..3d9c0af 100644 --- a/src/mcpd/src/services/k8s/manifest-generator.ts +++ b/src/mcpd/src/services/k8s/manifest-generator.ts @@ -259,7 +259,12 @@ function buildContainerSpec(spec: ContainerSpec) { // In Docker, spec.command maps to Cmd (args to entrypoint). // In k8s, we use `args` to pass arguments to the image's entrypoint, // preserving the runner image's entrypoint (uvx, npx -y, etc.) - if (spec.command && spec.command.length > 0) { + // + // `entrypoint` is the exception: it REPLACES the entrypoint (k8s `command`), + // which injected secret delivery needs so the sourcing shell can be PID 1. + if (spec.entrypoint && spec.entrypoint.length > 0) { + container.command = spec.entrypoint; + } else if (spec.command && spec.command.length > 0) { container.args = spec.command; } diff --git a/src/mcpd/src/services/orchestrator.ts b/src/mcpd/src/services/orchestrator.ts index 69cd343..74bf346 100644 --- a/src/mcpd/src/services/orchestrator.ts +++ b/src/mcpd/src/services/orchestrator.ts @@ -62,6 +62,17 @@ export interface ContainerSpec { serviceAccountName?: string; /** Injected agents need the projected SA token; plain servers do not. */ automountServiceAccountToken?: boolean; + /** + * REPLACES the image's ENTRYPOINT (k8s `command`), unlike `command`, which is + * appended to it as `args`. + * + * Needed only for injected secret delivery: the container has to run a shell + * that sources the rendered file before exec'ing the real process, and that + * shell must BE the entrypoint. Because it replaces the entrypoint, the argv + * here has to include whatever the image's entrypoint would have contributed + * (`npx -y`, `uvx`, ...). + */ + entrypoint?: string[]; /** Host port to bind (null = auto-assign) */ hostPort?: number | null; /** Container port to expose */ diff --git a/src/mcpd/src/services/server-identity.service.ts b/src/mcpd/src/services/server-identity.service.ts index 4d12fe9..50b154b 100644 --- a/src/mcpd/src/services/server-identity.service.ts +++ b/src/mcpd/src/services/server-identity.service.ts @@ -96,26 +96,6 @@ export class ServerIdentityService { }); } - /** - * Rewrite argv so the rendered secrets are sourced before the server runs. - * - * `command` is what mcpd already computed: for package-based servers that is - * the runner image's entrypoint plus the package, which mcpd owns. For a - * dockerImage server it may be absent — the image's own ENTRYPOINT would run, - * and mcpd cannot introspect it, which is why `entrypoint` is required on the - * server row in that case (enforced at validation). - */ - wrapCommand( - server: Pick, - command: string[] | undefined, - ): string[] | undefined { - const secretNames = this.secretNamesFor(server); - if (secretNames.length === 0) return command; - const argv = command ?? (server.entrypoint as string[] | null) ?? undefined; - if (argv === undefined || argv.length === 0) return command; - return wrapCommandForInjector(argv, secretNames); - } - /** Identity name for a server — also the SA, policy and role name. */ identityNameFor(serverName: string): string { return `${IDENTITY_PREFIX}${serverName}`; @@ -135,6 +115,39 @@ export class ServerIdentityService { return [...names].sort((a, b) => a.localeCompare(b)); } + /** + * Build the container ENTRYPOINT that sources the rendered secrets and then + * execs the real server. + * + * This must REPLACE the image's entrypoint, not extend it. mcpd normally puts + * a package server's argv into k8s `args` so the runner image's `npx -y` / + * `uvx` entrypoint still runs; a wrapper placed there would be executed BY + * npx (`npx -y /bin/sh -c ...`) and fail. So the argv returned here folds in + * whatever the image's entrypoint would have contributed. + * + * Returns undefined when there is nothing to wrap, leaving the pod on the + * normal entrypoint+args path. + */ + wrapCommand( + server: Pick, + command: string[] | undefined, + ): string[] | undefined { + const secretNames = this.secretNamesFor(server); + if (secretNames.length === 0) return undefined; + + // mcpd owns the runner images, so their entrypoints are known. A + // dockerImage server's is not introspectable — hence `entrypoint` being + // required on the row at validation time. + const imageEntrypoint = server.packageName + ? server.runtime === 'python' + ? ['uvx'] + : ['npx', '-y'] + : ((server.entrypoint as string[] | null) ?? undefined); + if (imageEntrypoint === undefined || imageEntrypoint.length === 0) return undefined; + + return wrapCommandForInjector([...imageEntrypoint, ...(command ?? [])], secretNames); + } + /** * Converge the identity for one server. Returns the identity name so the * caller can stamp it onto the pod spec. diff --git a/src/mcpd/tests/injector-manifest.test.ts b/src/mcpd/tests/injector-manifest.test.ts index 4b6788c..e8793cd 100644 --- a/src/mcpd/tests/injector-manifest.test.ts +++ b/src/mcpd/tests/injector-manifest.test.ts @@ -136,3 +136,23 @@ describe('shell quoting survives adversarial values', () => { expect(out).toBe(''); }); }); + +describe('the wrapper must REPLACE the image entrypoint, not extend it', () => { + // Regression: mcpd maps ContainerSpec.command -> k8s `args` so the runner + // image's `npx -y` / `uvx` entrypoint still runs. Emitting the wrapper there + // meant the pod actually ran `npx -y /bin/sh -c '...'`, which crashlooped. + it('emits container.command (entrypoint) and no args', () => { + const pod = generatePodSpec({ + name: 'g', image: 'runner', + entrypoint: ['/bin/sh', '-c', '. /vault/secrets/s; exec "$0" "$@"', 'npx', '-y', '@leval/mcp-grafana'], + } as ContainerSpec, 'mcpctl-servers'); + expect(pod.spec.containers[0]?.command?.[0]).toBe('/bin/sh'); + expect(pod.spec.containers[0]?.args).toBeUndefined(); + }); + + it('leaves the normal entrypoint+args path alone when not wrapping', () => { + const pod = generatePodSpec({ name: 'g', image: 'runner', command: ['@leval/mcp-grafana'] } as ContainerSpec, 'mcpctl-servers'); + expect(pod.spec.containers[0]?.command).toBeUndefined(); + expect(pod.spec.containers[0]?.args).toEqual(['@leval/mcp-grafana']); + }); +}); -- 2.49.1