fix(secrets): list via GET ?list=true — the LIST verb dies at the proxy
Some checks failed
CI/CD / lint (pull_request) Successful in 1m19s
CI/CD / typecheck (pull_request) Successful in 2m44s
CI/CD / test (pull_request) Successful in 1m30s
CI/CD / build (pull_request) Successful in 2m23s
CI/CD / smoke (pull_request) Failing after 3m4s
CI/CD / publish (pull_request) Has been skipped

Found by the readiness probe added in the previous commit, on its first
run against production: `mcpctl status` reported

  Secrets:    bao-k8s* ✗ auth failed: OpenBao list: HTTP 400 Bad Request

That is a real bug, not a probe artifact. `bao-k8s` is configured with
the public ingress URL, and Cilium's ingress Envoy rejects the
non-standard LIST HTTP method outright. Verified live from the mcpd pod:

  LIST   https://bao.ad.itaz.eu/v1/secret/metadata/mcpctl/            -> 400 Bad Request
  GET    https://bao.ad.itaz.eu/v1/secret/metadata/mcpctl/?list=true  -> 403 permission denied (bogus token, i.e. reached bao)
  LIST   http://openbao.openbao.svc:8200/... (ClusterIP, no Envoy)    -> 403 permission denied

So the verb was fine against bao and fatal through the ingress. OpenBao
accepts both forms and documents the GET form for exactly this reason.

This was never noticed because nothing called list() in production —
it backs `mcpctl migrate secrets`, which would have failed with an opaque
HTTP 400 against any ingress-fronted backend.

Also adds the per-server scoping primitives Phase 2 needs, with tests:
buildServerSecretPolicyHcl (no wildcards, read-only, stable under
reordering), buildServerProvisioningPolicyHcl (prefix-confined so mcpd
cannot grant itself more than it holds), ensureKubernetesAuthRole /
delete helpers, the driver-level ensureServerIdentity/removeServerIdentity
capability, and ServerIdentityService.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
This commit is contained in:
Michal
2026-08-20 22:54:42 +01:00
parent eb3e558a44
commit 7b5136491f
8 changed files with 539 additions and 4 deletions

View File

@@ -306,3 +306,83 @@ export async function testWriteReadDelete(
throw new Error(`vault smoke delete ${relPath}: HTTP ${String(delRes.status)} ${await readError(delRes)}`);
}
}
export interface KubernetesAuthRoleConfig {
/** ServiceAccount names permitted to log in as this role. */
boundServiceAccountNames: string[];
/** Namespaces those ServiceAccounts must live in. */
boundServiceAccountNamespaces: string[];
/** Policies attached to tokens issued for this role. */
tokenPolicies: string[];
/** Token TTL in seconds. Default 3600 — pods only need it at startup. */
tokenTtlSeconds?: number;
}
/**
* POST /v1/auth/<authMount>/role/<role>. Idempotent: upserts the role.
*
* Used to give each MCP server pod its own OpenBao identity, bound to its own
* ServiceAccount, so the injector can fetch only that server's secrets. Note
* `bound_service_account_names` is a list but we pass exactly one — a role
* shared between ServiceAccounts would defeat the point.
*/
export async function ensureKubernetesAuthRole(
url: string,
token: string,
authMount: string,
role: string,
cfg: KubernetesAuthRoleConfig,
deps: VaultDeps = {},
): Promise<void> {
const fetchImpl = deps.fetch ?? globalThis.fetch;
const mount = authMount.replace(/^\/|\/$/g, '');
const res = await fetchImpl(`${baseUrl(url)}/v1/auth/${mount}/role/${encodeURIComponent(role)}`, {
method: 'POST',
headers: headers(token, deps.namespace, true),
body: JSON.stringify({
bound_service_account_names: cfg.boundServiceAccountNames,
bound_service_account_namespaces: cfg.boundServiceAccountNamespaces,
token_policies: cfg.tokenPolicies,
token_ttl: cfg.tokenTtlSeconds ?? 3600,
}),
});
if (!res.ok) {
throw new Error(`vault ensure k8s auth role ${role}: HTTP ${String(res.status)} ${await readError(res)}`);
}
}
/** DELETE /v1/auth/<authMount>/role/<role>. Idempotent — 404 is success. */
export async function deleteKubernetesAuthRole(
url: string,
token: string,
authMount: string,
role: string,
deps: VaultDeps = {},
): Promise<void> {
const fetchImpl = deps.fetch ?? globalThis.fetch;
const mount = authMount.replace(/^\/|\/$/g, '');
const res = await fetchImpl(`${baseUrl(url)}/v1/auth/${mount}/role/${encodeURIComponent(role)}`, {
method: 'DELETE',
headers: headers(token, deps.namespace, false),
});
if (!res.ok && res.status !== 404) {
throw new Error(`vault delete k8s auth role ${role}: HTTP ${String(res.status)} ${await readError(res)}`);
}
}
/** DELETE /v1/sys/policies/acl/<name>. Idempotent — 404 is success. */
export async function deletePolicy(
url: string,
token: string,
name: string,
deps: VaultDeps = {},
): Promise<void> {
const fetchImpl = deps.fetch ?? globalThis.fetch;
const res = await fetchImpl(`${baseUrl(url)}/v1/sys/policies/acl/${encodeURIComponent(name)}`, {
method: 'DELETE',
headers: headers(token, deps.namespace, false),
});
if (!res.ok && res.status !== 404) {
throw new Error(`vault delete policy ${name}: HTTP ${String(res.status)} ${await readError(res)}`);
}
}

View File

@@ -33,3 +33,70 @@ export function buildAppMcpdPolicyHcl(cfg: AppMcpdPolicyConfig): string {
'',
].join('\n');
}
/**
* Per-server read policy for the OpenBao Agent Injector.
*
* Each MCP server pod authenticates to OpenBao as its OWN ServiceAccount and
* gets a policy naming only the secrets that server actually declares. That is
* the difference between "the injector moved the credential out of the pod
* spec" and "the injector made things worse": with one shared role, every
* opted-in pod — including third-party images we do not control — could read
* every secret under the prefix. Here, `gitea` can read `gitea-creds` and
* nothing else.
*
* No wildcards. Each secret is named explicitly; adding a secret to a server
* means regenerating and re-writing this policy, which is exactly the audit
* trail we want.
*/
export interface ServerSecretPolicyConfig {
/** KV v2 mount name, e.g. 'secret'. */
mount: string;
/** Path prefix under the mount, e.g. 'mcpctl'. */
pathPrefix: string;
/** Secret names this server may read. Order-insensitive; deduped + sorted. */
secretNames: string[];
}
export function buildServerSecretPolicyHcl(cfg: ServerSecretPolicyConfig): string {
const { mount } = cfg;
const prefix = cfg.pathPrefix.replace(/^\/|\/$/g, '');
// Sort + dedupe so the generated HCL is stable: an unstable policy body would
// rewrite on every reconcile and make real changes invisible in the audit log.
const names = [...new Set(cfg.secretNames)].sort((a, b) => a.localeCompare(b));
const lines: string[] = [];
for (const name of names) {
const path = prefix === '' ? name : `${prefix}/${name}`;
lines.push(`path "${mount}/data/${path}" { capabilities = ["read"] }`);
lines.push(`path "${mount}/metadata/${path}" { capabilities = ["read"] }`);
}
lines.push('');
return lines.join('\n');
}
/**
* Grants mcpd needs in order to provision the per-server identities above:
* write its own scoped policies and the matching Kubernetes auth roles.
*
* Deliberately confined by name prefix. This is a real privilege increase for
* mcpd, but a much narrower one than the obvious alternative of granting it
* Kubernetes `secrets` verbs — that would let it read every Secret in the
* cluster, not just the ones it already owns.
*/
export interface ServerProvisioningPolicyConfig {
/** Kubernetes auth mount, e.g. 'kubernetes-worker0'. */
authMount: string;
/** Shared name prefix for generated policies + roles, e.g. 'mcpctl-server-'. */
namePrefix: string;
}
export function buildServerProvisioningPolicyHcl(cfg: ServerProvisioningPolicyConfig): string {
const authMount = cfg.authMount.replace(/^\/|\/$/g, '');
const prefix = cfg.namePrefix;
return [
`path "sys/policies/acl/${prefix}*" { capabilities = ["create", "read", "update", "delete"] }`,
`path "auth/${authMount}/role/${prefix}*" { capabilities = ["create", "read", "update", "delete"] }`,
`path "auth/${authMount}/role" { capabilities = ["list"] }`,
'',
].join('\n');
}

View File

@@ -0,0 +1,138 @@
/**
* Per-server OpenBao scoping: each MCP server pod gets its own identity and a
* policy naming only its own secrets.
*
* The property under test is containment. A shared role would let any opted-in
* pod — including third-party images we don't control — read every secret under
* the prefix, which is a worse position than leaving values in the pod spec.
*/
import { describe, it, expect, vi } from 'vitest';
import {
buildServerSecretPolicyHcl,
buildServerProvisioningPolicyHcl,
ensureKubernetesAuthRole,
deleteKubernetesAuthRole,
deletePolicy,
} from '../src/vault/index.js';
describe('buildServerSecretPolicyHcl', () => {
const cfg = { mount: 'secret', pathPrefix: 'mcpctl' };
it('grants read on exactly the named secrets and nothing else', () => {
const hcl = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['gitea-creds'] });
expect(hcl).toContain('path "secret/data/mcpctl/gitea-creds"');
expect(hcl).toContain('path "secret/metadata/mcpctl/gitea-creds"');
expect(hcl).toContain('capabilities = ["read"]');
});
it('never emits a wildcard — that is the whole point', () => {
const hcl = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['gitea-creds', 'unifi-creds'] });
expect(hcl).not.toContain('*');
});
it('grants no write capability anywhere', () => {
const hcl = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['a', 'b'] });
for (const verb of ['create', 'update', 'delete', 'list', 'sudo']) {
expect(hcl, `must not grant ${verb}`).not.toContain(verb);
}
});
it('does not reach other servers\' secrets', () => {
const gitea = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['gitea-creds'] });
expect(gitea).not.toContain('unifi-creds');
expect(gitea).not.toContain('anthropic-key');
expect(gitea).not.toContain('litellm-key');
});
it('is stable under reordering and duplication', () => {
// An unstable body would rewrite on every reconcile, drowning real changes
// in the OpenBao audit log.
const a = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['b', 'a', 'b'] });
const b = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['a', 'b'] });
expect(a).toBe(b);
});
it('handles an empty prefix without producing a double slash', () => {
const hcl = buildServerSecretPolicyHcl({ mount: 'secret', pathPrefix: '', secretNames: ['x'] });
expect(hcl).toContain('path "secret/data/x"');
expect(hcl).not.toContain('//');
});
it('emits nothing but a trailing newline for a server with no secrets', () => {
expect(buildServerSecretPolicyHcl({ ...cfg, secretNames: [] })).toBe('');
});
});
describe('buildServerProvisioningPolicyHcl', () => {
const cfg = { authMount: 'kubernetes-worker0', namePrefix: 'mcpctl-server-' };
it('confines mcpd to the generated name prefix', () => {
const hcl = buildServerProvisioningPolicyHcl(cfg);
expect(hcl).toContain('path "sys/policies/acl/mcpctl-server-*"');
expect(hcl).toContain('path "auth/kubernetes-worker0/role/mcpctl-server-*"');
});
it('does not grant blanket policy or auth administration', () => {
const hcl = buildServerProvisioningPolicyHcl(cfg);
expect(hcl).not.toContain('path "sys/policies/acl/*"');
expect(hcl).not.toContain('path "auth/*"');
expect(hcl).not.toContain('path "sys/*"');
});
it('grants no access to secret data at all', () => {
// mcpd reads secrets through its OWN policy; the provisioning grant must
// not widen that surface.
expect(buildServerProvisioningPolicyHcl(cfg)).not.toContain('secret/data');
});
});
describe('ensureKubernetesAuthRole', () => {
it('binds exactly one ServiceAccount in one namespace', async () => {
const fetchFn = vi.fn(async () => new Response(null, { status: 204 }));
await ensureKubernetesAuthRole(
'http://bao.example:8200', 'tok', 'kubernetes-worker0', 'mcpctl-server-gitea',
{
boundServiceAccountNames: ['mcpctl-server-gitea'],
boundServiceAccountNamespaces: ['mcpctl-servers'],
tokenPolicies: ['mcpctl-server-gitea'],
},
{ fetch: fetchFn as unknown as typeof fetch },
);
const [url, init] = fetchFn.mock.calls[0] as [string, RequestInit];
expect(url).toBe('http://bao.example:8200/v1/auth/kubernetes-worker0/role/mcpctl-server-gitea');
const body = JSON.parse(init.body as string) as Record<string, unknown>;
expect(body.bound_service_account_names).toEqual(['mcpctl-server-gitea']);
expect(body.bound_service_account_namespaces).toEqual(['mcpctl-servers']);
expect(body.token_policies).toEqual(['mcpctl-server-gitea']);
expect(body.token_ttl).toBe(3600);
});
it('surfaces the OpenBao error body on failure', async () => {
const fetchFn = vi.fn(async () => new Response(JSON.stringify({ errors: ['permission denied'] }), { status: 403 }));
await expect(ensureKubernetesAuthRole(
'http://bao.example:8200', 'tok', 'kubernetes-worker0', 'r',
{ boundServiceAccountNames: ['a'], boundServiceAccountNamespaces: ['n'], tokenPolicies: ['p'] },
{ fetch: fetchFn as unknown as typeof fetch },
)).rejects.toThrow(/permission denied/);
});
});
describe('cleanup helpers are idempotent', () => {
it('treats a missing role as already deleted', async () => {
const fetchFn = vi.fn(async () => new Response('', { status: 404 }));
await expect(deleteKubernetesAuthRole('http://b', 't', 'kubernetes-worker0', 'gone', { fetch: fetchFn as unknown as typeof fetch }))
.resolves.toBeUndefined();
});
it('treats a missing policy as already deleted', async () => {
const fetchFn = vi.fn(async () => new Response('', { status: 404 }));
await expect(deletePolicy('http://b', 't', 'gone', { fetch: fetchFn as unknown as typeof fetch }))
.resolves.toBeUndefined();
});
it('still raises a real failure', async () => {
const fetchFn = vi.fn(async () => new Response(JSON.stringify({ errors: ['denied'] }), { status: 403 }));
await expect(deletePolicy('http://b', 't', 'p', { fetch: fetchFn as unknown as typeof fetch }))
.rejects.toThrow(/denied/);
});
});