From f8427959e58a3fefabc2eb7c3772cf41aa37dec8 Mon Sep 17 00:00:00 2001 From: Michal Date: Thu, 20 Aug 2026 23:13:49 +0100 Subject: [PATCH 1/3] test(secrets): cover per-server identity containment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ServerIdentityService shipped without tests. Containment is the whole point of the design, so it needs assertions rather than trust: a shared role would let third-party MCP images (gitea-mcp, ha-mcp) read every secret under secret/mcpctl/*, which is worse than the pod-spec exposure it replaces. Eight cases, all about what a server must NOT get: the grant covers only the secrets that server declares; a secret referenced twice is one grant, not two; inline env values never widen it; another server's secrets never appear. Plus the ordering invariant (ServiceAccount before the role that binds it — the reverse lets a pod start, fail to log in and crashloop while the role is still being written), and that a backend which cannot scope identities REFUSES rather than silently succeeding, which would leave a pod believing it held access it never got. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki --- .../tests/server-identity-service.test.ts | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 src/mcpd/tests/server-identity-service.test.ts diff --git a/src/mcpd/tests/server-identity-service.test.ts b/src/mcpd/tests/server-identity-service.test.ts new file mode 100644 index 0000000..f72d818 --- /dev/null +++ b/src/mcpd/tests/server-identity-service.test.ts @@ -0,0 +1,117 @@ +/** + * Per-server identity provisioning. The property under test throughout is + * CONTAINMENT: a server's identity must grant exactly its own secrets, because + * the alternative (one shared role) would let third-party MCP images read every + * secret under the prefix — worse than the pod-spec exposure it replaces. + */ +import { describe, it, expect, vi } from 'vitest'; +import type { SecretBackend } from '@prisma/client'; +import { ServerIdentityService, type ServiceAccountPort } from '../src/services/server-identity.service.js'; +import type { SecretBackendService } from '../src/services/secret-backend.service.js'; +import type { SecretBackendDriver } from '../src/services/secret-backends/types.js'; + +const NS = 'mcpctl-servers'; + +function envRef(name: string, secret: string, key: string): unknown { + return { name, valueFrom: { secretRef: { name: secret, key } } }; +} + +function harness(driverOverrides: Partial = {}, backendType = 'openbao') { + const calls = { sa: [] as string[], saRemoved: [] as string[] }; + const ensureServerIdentity = vi.fn(async () => undefined); + const removeServerIdentity = vi.fn(async () => undefined); + const driver = { kind: backendType, ensureServerIdentity, removeServerIdentity, ...driverOverrides } as unknown as SecretBackendDriver; + + const backends = { + getDefault: async (): Promise => ({ id: 'b1', name: 'bao-k8s', type: backendType } as SecretBackend), + driverFor: () => driver, + } as unknown as SecretBackendService; + + const serviceAccounts: ServiceAccountPort = { + namespace: NS, + ensure: async (n) => { calls.sa.push(n); }, + remove: async (n) => { calls.saRemoved.push(n); }, + }; + + return { svc: new ServerIdentityService(backends, serviceAccounts), calls, ensureServerIdentity, removeServerIdentity }; +} + +describe('ServerIdentityService', () => { + it('grants a server exactly the secrets it declares', async () => { + const h = harness(); + await h.svc.ensureFor({ + name: 'gitea', + env: [envRef('GITEA_ACCESS_TOKEN', 'gitea-creds', 'GITEA_ACCESS_TOKEN'), envRef('GITEA_HOST', 'gitea-creds', 'GITEA_HOST')], + } as never); + + expect(h.ensureServerIdentity).toHaveBeenCalledWith({ + name: 'mcpctl-server-gitea', + namespace: NS, + // One secret, not two: the same secret referenced twice is one grant. + secretNames: ['gitea-creds'], + }); + }); + + it('does not leak another server\'s secrets into the grant', async () => { + const h = harness(); + await h.svc.ensureFor({ name: 'gitea', env: [envRef('T', 'gitea-creds', 'K')] } as never); + const granted = h.ensureServerIdentity.mock.calls[0]?.[0] as { secretNames: string[] }; + expect(granted.secretNames).not.toContain('unifi-creds'); + expect(granted.secretNames).not.toContain('anthropic-key'); + }); + + it('ignores inline env values — they are not secrets', async () => { + const h = harness(); + await h.svc.ensureFor({ + name: 's', + env: [{ name: 'PLAIN', value: 'not-a-secret' }, envRef('T', 'real', 'K')], + } as never); + const granted = h.ensureServerIdentity.mock.calls[0]?.[0] as { secretNames: string[] }; + expect(granted.secretNames).toEqual(['real']); + }); + + it('creates the ServiceAccount BEFORE binding the role to it', async () => { + // Reverse order lets the pod start, fail to log in, and crashloop while the + // role is still being written. + const order: string[] = []; + const h = harness(); + const svc = new ServerIdentityService( + { getDefault: async () => ({ id: 'b1', name: 'bao', type: 'openbao' } as SecretBackend), + driverFor: () => ({ kind: 'openbao', ensureServerIdentity: async () => { order.push('role'); } }) } as unknown as SecretBackendService, + { namespace: NS, ensure: async () => { order.push('sa'); }, remove: async () => undefined }, + ); + await svc.ensureFor({ name: 'x', env: [envRef('T', 's', 'K')] } as never); + expect(order).toEqual(['sa', 'role']); + expect(h).toBeDefined(); + }); + + it('produces an empty grant for a server with no secret refs', async () => { + const h = harness(); + await h.svc.ensureFor({ name: 'plain', env: [] } as never); + const granted = h.ensureServerIdentity.mock.calls[0]?.[0] as { secretNames: string[] }; + expect(granted.secretNames).toEqual([]); + }); + + it('refuses on a backend that cannot scope identities, rather than silently succeeding', async () => { + // A plaintext backend has no auth mount to bind a role on. Succeeding here + // would leave a pod believing it had been granted access it never got. + const h = harness({ ensureServerIdentity: undefined }, 'plaintext'); + await expect(h.svc.ensureFor({ name: 's', env: [] } as never)) + .rejects.toThrow(/cannot provision per-server identities/); + }); + + it('revokes the OpenBao identity and the ServiceAccount on removal', async () => { + const h = harness(); + await h.svc.removeFor('gitea'); + expect(h.removeServerIdentity).toHaveBeenCalledWith({ name: 'mcpctl-server-gitea' }); + expect(h.calls.saRemoved).toEqual(['mcpctl-server-gitea']); + }); + + it('still removes the ServiceAccount when OpenBao teardown fails', async () => { + // A half-removed identity grants nothing useful; abandoning the rest would + // strand the k8s side. + const h = harness({ removeServerIdentity: vi.fn(async () => { throw new Error('bao down'); }) as never }); + await expect(h.svc.removeFor('gitea')).resolves.toBeUndefined(); + expect(h.calls.saRemoved).toEqual(['mcpctl-server-gitea']); + }); +}); -- 2.49.1 From 370fd0a03406c1e2fdf7705389d95f5be08911b4 Mon Sep 17 00:00:00 2001 From: Michal Date: Thu, 20 Aug 2026 23:33:13 +0100 Subject: [PATCH 2/3] feat(secrets): opt-in injected secret delivery, scoped per server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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