feat(secrets): opt-in injected secret delivery, scoped per server
Completes the path that stops mcpd writing secret VALUES into MCP server
pod specs. With `secretDelivery: injector`, the pod fetches its own
secrets from OpenBao through the agent injector, under a ServiceAccount
and role scoped to just that server's secrets — so the value never enters
etcd, and gitea-mcp cannot read the Grafana token.
Opt-in per server, defaulting to `env`. Every existing server is
bit-for-bit unchanged, and migrating is one reversible decision at a time
rather than a flag day.
The two invariants under most risk, both tested:
- **Opted-out servers produce an identical manifest.** No annotations, no
serviceAccountName, automountServiceAccountToken still false.
- **Opted-in servers still fail LOUDLY on a bad ref.** Once mcpd stops
reading a server's secrets, the check e6cd735 added no longer fires for
it, and a typo'd secretRef would degrade into a vault-agent-init
crashloop that mcpd reports as a generic pod failure — the same class of
bug that had gitea-mcp running for weeks on an empty token while
reporting healthy. `validateServerEnvRefs` resolves every ref and throws
the value away, purely to keep that error. After the value cache it is a
cache hit and costs nothing.
Shell quoting is the other silent-failure trap and is treated as part of
the contract: the agent renders `export NAME='value'` and the container
command sources it, so a value containing a space, `$`, a quote or a
newline would truncate and yield an empty token. `shellSingleQuote` is
tested by executing a real /bin/sh over nine adversarial values including
`'; export PWNED=1; '` — and those tests fail against naive quoting,
confirmed before keeping them.
`sh -c <script> arg0 arg1 …` preserves argv via $0/$@, and `exec` keeps
PID 1 as the real process, which matters because mcpd attaches to PID 1's
stdin/stdout for STDIO servers.
Docker/Podman declare `capabilities.secretRefs: false` and fall back to
inline resolution, so local development is untouched. Deleting a server
revokes its identity, after its pods are gone and best-effort — a role no
pod can authenticate as grants nothing, and failing the delete over it
would strand the row.
Per the CLI rules, `secretDelivery`/`entrypoint` are `create` flags,
round-trip through apply -f, and show in `describe server` — which now
also flags servers still inlining secrets into their pod spec.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
This commit is contained in:
138
src/mcpd/tests/injector-manifest.test.ts
Normal file
138
src/mcpd/tests/injector-manifest.test.ts
Normal file
@@ -0,0 +1,138 @@
|
||||
/**
|
||||
* Injector manifest generation.
|
||||
*
|
||||
* The two things that must not break: a server NOT opted in produces a
|
||||
* byte-identical manifest to before, and a rendered secret value survives the
|
||||
* shell intact whatever it contains. The second is the subtle one — bad quoting
|
||||
* fails silently, yielding an empty token and a server that reports healthy
|
||||
* while every authenticated call fails.
|
||||
*/
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { writeFileSync, mkdtempSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import {
|
||||
generatePodSpec,
|
||||
buildInjectorAnnotations,
|
||||
wrapCommandForInjector,
|
||||
shellSingleQuote,
|
||||
} from '../src/services/k8s/manifest-generator.js';
|
||||
import type { ContainerSpec } from '../src/services/orchestrator.js';
|
||||
|
||||
const BASE: ContainerSpec = { name: 'gitea', image: 'gitea/mcp:latest' } as ContainerSpec;
|
||||
const CFG = { role: 'mcpctl-server-gitea', authPath: 'auth/kubernetes-worker0', mount: 'secret', pathPrefix: 'mcpctl' };
|
||||
|
||||
describe('generatePodSpec — opted-out servers are untouched', () => {
|
||||
it('emits no annotations, no serviceAccountName, automount still false', () => {
|
||||
const pod = generatePodSpec({ ...BASE, env: { TOKEN: 'plain' } } as ContainerSpec, 'mcpctl-servers');
|
||||
expect(pod.metadata.annotations).toBeUndefined();
|
||||
expect(pod.spec.serviceAccountName).toBeUndefined();
|
||||
expect(pod.spec.automountServiceAccountToken).toBe(false);
|
||||
expect(pod.spec.containers[0]?.env).toEqual([{ name: 'TOKEN', value: 'plain' }]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('generatePodSpec — opted-in servers', () => {
|
||||
const spec = {
|
||||
...BASE,
|
||||
annotations: buildInjectorAnnotations(
|
||||
[{ name: 'GITEA_ACCESS_TOKEN', secretName: 'gitea-creds', key: 'GITEA_ACCESS_TOKEN' }],
|
||||
CFG,
|
||||
),
|
||||
serviceAccountName: 'mcpctl-server-gitea',
|
||||
automountServiceAccountToken: true,
|
||||
} as ContainerSpec;
|
||||
|
||||
it('carries no secret VALUE anywhere in the manifest', () => {
|
||||
const pod = generatePodSpec(spec, 'mcpctl-servers');
|
||||
const json = JSON.stringify(pod);
|
||||
expect(json).not.toContain('de4c69ed');
|
||||
expect(pod.spec.containers[0]?.env ?? []).toEqual([]);
|
||||
});
|
||||
|
||||
it('runs as its own ServiceAccount with the SA token projected', () => {
|
||||
const pod = generatePodSpec(spec, 'mcpctl-servers');
|
||||
expect(pod.spec.serviceAccountName).toBe('mcpctl-server-gitea');
|
||||
// The agent cannot log in without it; false here is a silent crashloop.
|
||||
expect(pod.spec.automountServiceAccountToken).toBe(true);
|
||||
});
|
||||
|
||||
it('pins the auth path to the mount that validates THIS cluster', () => {
|
||||
const a = buildInjectorAnnotations([{ name: 'T', secretName: 's', key: 'k' }], CFG);
|
||||
expect(a['vault.hashicorp.com/auth-path']).toBe('auth/kubernetes-worker0');
|
||||
expect(a['vault.hashicorp.com/role']).toBe('mcpctl-server-gitea');
|
||||
// init container only — no sidecar in a 512Mi pod
|
||||
expect(a['vault.hashicorp.com/agent-pre-populate-only']).toBe('true');
|
||||
});
|
||||
|
||||
it('groups env vars by the secret they come from', () => {
|
||||
const a = buildInjectorAnnotations([
|
||||
{ name: 'HOST', secretName: 'gitea-creds', key: 'GITEA_HOST' },
|
||||
{ name: 'TOKEN', secretName: 'gitea-creds', key: 'GITEA_ACCESS_TOKEN' },
|
||||
{ name: 'OTHER', secretName: 'other-creds', key: 'K' },
|
||||
], CFG);
|
||||
expect(a['vault.hashicorp.com/agent-inject-secret-gitea-creds']).toBe('secret/data/mcpctl/gitea-creds');
|
||||
expect(a['vault.hashicorp.com/agent-inject-secret-other-creds']).toBe('secret/data/mcpctl/other-creds');
|
||||
const tpl = a['vault.hashicorp.com/agent-inject-template-gitea-creds'] ?? '';
|
||||
expect(tpl).toContain('export HOST=');
|
||||
expect(tpl).toContain('export TOKEN=');
|
||||
expect(tpl).not.toContain('export OTHER=');
|
||||
});
|
||||
|
||||
it('is stable under input reordering', () => {
|
||||
const refs = [
|
||||
{ name: 'B', secretName: 'z', key: 'k' },
|
||||
{ name: 'A', secretName: 'a', key: 'k' },
|
||||
];
|
||||
expect(buildInjectorAnnotations(refs, CFG)).toEqual(buildInjectorAnnotations([...refs].reverse(), CFG));
|
||||
});
|
||||
});
|
||||
|
||||
describe('wrapCommandForInjector', () => {
|
||||
it('preserves argv exactly via $0/$@ and execs so PID 1 is the real process', () => {
|
||||
// exec matters: mcpd attaches to PID 1 stdin/stdout for STDIO servers.
|
||||
const cmd = wrapCommandForInjector(['node', 'server.js', '--port', '3000'], ['a-creds']);
|
||||
expect(cmd[0]).toBe('/bin/sh');
|
||||
expect(cmd[1]).toBe('-c');
|
||||
expect(cmd[2]).toContain('exec "$0" "$@"');
|
||||
expect(cmd.slice(3)).toEqual(['node', 'server.js', '--port', '3000']);
|
||||
});
|
||||
|
||||
it('sources every referenced secret file, in a stable order', () => {
|
||||
const cmd = wrapCommandForInjector(['x'], ['b-creds', 'a-creds']);
|
||||
expect(cmd[2]).toBe('. /vault/secrets/a-creds; . /vault/secrets/b-creds; exec "$0" "$@"');
|
||||
});
|
||||
});
|
||||
|
||||
describe('shell quoting survives adversarial values', () => {
|
||||
// Executed by a real /bin/sh: asserting on the string alone would prove
|
||||
// nothing about what the shell actually does with it.
|
||||
const nasty = [
|
||||
'plain',
|
||||
'with space',
|
||||
"single'quote",
|
||||
'double"quote',
|
||||
'$USER and `whoami`',
|
||||
'semi;colon && rm -rf /',
|
||||
'new\nline',
|
||||
'back\\slash',
|
||||
"'; export PWNED=1; '",
|
||||
];
|
||||
|
||||
it.each(nasty)('round-trips %j through a sourced file', (value) => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'mcpctl-quote-'));
|
||||
const file = join(dir, 'env');
|
||||
writeFileSync(file, `export SECRET=${shellSingleQuote(value)}\n`);
|
||||
const out = execFileSync('/bin/sh', ['-c', `. ${file}; printf '%s' "$SECRET"`], { encoding: 'utf-8' });
|
||||
expect(out).toBe(value);
|
||||
});
|
||||
|
||||
it('does not let a crafted value execute anything', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'mcpctl-quote-'));
|
||||
const file = join(dir, 'env');
|
||||
writeFileSync(file, `export SECRET=${shellSingleQuote("'; touch /tmp/mcpctl-pwned; '")}\n`);
|
||||
const out = execFileSync('/bin/sh', ['-c', `. ${file}; printf '%s' "$PWNED_MARKER"`], { encoding: 'utf-8' });
|
||||
expect(out).toBe('');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user