diff --git a/docs/secret-backends.md b/docs/secret-backends.md index d53d358..78df3bf 100644 --- a/docs/secret-backends.md +++ b/docs/secret-backends.md @@ -118,8 +118,110 @@ That's the whole point of keeping plaintext around — it's the trust root: token itself. DB access is now equivalent to OpenBao token access (a single key), not equivalent to all API keys in the system. -Follow-up work (not shipped yet) replaces static token auth with Kubernetes -ServiceAccount auth so no bootstrap token is needed at all. +#### Kubernetes ServiceAccount auth (no bootstrap token) + +`auth: kubernetes` removes the chicken-and-egg entirely: mcpd exchanges its +projected ServiceAccount JWT for an OpenBao token at +`auth//role/`, so there is no static credential in the database +at all. The token is cached for its lease and re-minted lazily with a 60s grace +window. + +```yaml +kind: secretbackend +name: bao-k8s +type: openbao +isDefault: true +config: + url: https://bao.example + auth: kubernetes + role: mcpctl + authMount: kubernetes-worker0 # defaults to `kubernetes` +``` + +Note that the daily **rotator does not apply** to these backends — there is no +stored token to rotate. That has a consequence for monitoring, see below. + +## Reliability + +Remote backends are network dependencies on the critical path of nearly +everything: server env resolution, LLM api keys, chat, git providers, code +repos, webhooks. Three mechanisms keep an outage from cascading. + +### Request hardening + +Every call carries a timeout (default 5s) and retries `5xx`/`429`/network +failures with full-jitter exponential backoff (3 attempts). A **sealed** OpenBao +answers `503`, so this covers unseal windows and failovers. + +The `403` path is separate and deliberately single-shot: the driver purges its +cached token, re-authenticates and retries **once**. That is a credential +refresh, not a backend-unavailable condition — looping on it would hide a +genuinely revoked grant. + +### Value cache with stale-while-error + +Resolved values are cached per backend (default TTL 5 minutes, LRU-bounded). +Past the TTL the backend is always consulted; if it fails *as a transport +failure*, the last known-good value is served instead of throwing. + +| Failure | Behaviour | +|---|---| +| Backend unreachable / timeout / exhausted 5xx | Serve last known-good, mark degraded, log `BACKEND_UNREACHABLE` once | +| Secret deleted (404) | **Evict and throw.** Never served stale — that would resurrect a revoked credential | +| 403 after a token refresh | Throw. Revoked grants must stay loud | +| Nothing cached yet | Throw | + +The stale window is unbounded on purpose: a cap would mean a long outage +eventually takes mcpd down anyway. + +`plaintext` backends are not cached — their `read()` is an identity function +over the row the caller already supplied. + +**Cold cache is the known gap.** If mcpd restarts *while* the backend is +unreachable, nothing has a last-known-good value and secret-bearing servers fail +to start. That is deliberate: booting a server with an empty credential is worse +(gitea-mcp once ran for weeks with an empty `GITEA_ACCESS_TOKEN`, answering +`tools/list` and reporting healthy while every authenticated call failed). mcpd +mitigates it by warming the cache at boot — one read per referenced secret — so +an outage that starts *after* startup is fully absorbed. + +### Health: `live` vs `ready` + +```bash +curl $MCPD/api/v1/secretbackends//health +``` + +```json +{ "live": true, "liveDetail": "active", + "ready": false, "readyDetail": "OpenBao list: HTTP 403 permission denied", + "cache": { "entries": 9, "servingStale": 0 }, + "rotation": { "rotatable": false, "lastRotationError": null } } +``` + +- **`live`** — unauthenticated `sys/health`. Distinguishes *down* from *sealed* + from *standby*. +- **`ready`** — a real read with our credentials. + +The two are separate because `live && !ready` is a distinct, important state: a +re-initialised OpenBao hands back valid-looking tokens that grant nothing. +Collapsing them into one boolean is what let that go unnoticed for four days. + +`mcpctl status` renders the probe directly: + +``` +Secrets: bao-k8s* ✓ reachable, default ✓ reachable +Secrets: bao-k8s* ⚠ degraded — serving 7 cached secret(s) +Secrets: bao-k8s* ✗ unreachable: sealed +Secrets: bao-k8s* ✗ auth failed: HTTP 403 permission denied +Secrets: bao-k8s* ? unknown +``` + +> **Historical note.** This verdict used to come solely from +> `tokenMeta.lastRotationError`, which only the rotator writes — and the rotator +> skips `auth: kubernetes` backends. The Secrets line was therefore *incapable* +> of going red for a k8s-auth backend, and reported OpenBao healthy while it was +> unreachable. `?` (probe failed) renders yellow, never green: not knowing is not +> health. ## Migration — `mcpctl migrate secrets` diff --git a/src/cli/src/commands/status.ts b/src/cli/src/commands/status.ts index a523d9d..c2ce0b3 100644 --- a/src/cli/src/commands/status.ts +++ b/src/cli/src/commands/status.ts @@ -50,6 +50,7 @@ interface ServerLlm { * of the last credential-rotation failure (e.g. a dead OpenBao token). */ interface SecretBackendInfo { + id: string; name: string; type: string; isDefault?: boolean; @@ -60,6 +61,22 @@ interface SecretBackendInfo { } | null; } +/** + * Live probe result from GET /api/v1/secretbackends/:id/health. + * + * `live` and `ready` are deliberately separate: a backend that is reachable but + * whose credentials no longer grant anything is the failure mode that hid an + * OpenBao re-init for four days. `null` means the probe itself failed, which we + * report as unknown rather than pretending it means healthy. + */ +interface SecretBackendHealth { + live: boolean; + liveDetail?: string; + ready: boolean; + readyDetail?: string; + cache?: { entries: number; servingStale: number; oldestStaleSince?: number | null } | null; +} + /** * Result of a live "say hi" probe against a server LLM. `ok` says we got a * 200 + non-empty content back; `say` is the trimmed first 16 chars of the @@ -99,6 +116,7 @@ export interface StatusCommandDeps { probeServerLlm: (mcpdUrl: string, name: string, token: string | null) => Promise; /** Fetch SecretBackends from mcpd to surface backend health. Null on error. */ fetchSecretBackends: (mcpdUrl: string, token: string | null) => Promise; + probeSecretBackend: (mcpdUrl: string, id: string, token: string | null) => Promise; isTTY: boolean; } @@ -275,6 +293,38 @@ function defaultFetchSecretBackends(mcpdUrl: string, token: string | null): Prom }); } +/** + * Live-probe one SecretBackend. Resolves to null on any unhappy path — same + * never-throw discipline as the other probes here, and `null` renders as + * "unknown", never as healthy. + */ +function defaultProbeSecretBackend(mcpdUrl: string, id: string, token: string | null): Promise { + return new Promise((resolve) => { + let req: http.ClientRequest; + const headers: Record = { Accept: 'application/json' }; + if (token !== null) headers['Authorization'] = `Bearer ${token}`; + try { + req = httpDriverFor(mcpdUrl).get(`${mcpdUrl}/api/v1/secretbackends/${id}/health`, { timeout: 5000, headers }, (res) => { + if (res.statusCode !== 200) { resolve(null); res.resume(); return; } + const chunks: Buffer[] = []; + res.on('data', (chunk: Buffer) => chunks.push(chunk)); + res.on('end', () => { + try { + resolve(JSON.parse(Buffer.concat(chunks).toString('utf-8')) as SecretBackendHealth); + } catch { + resolve(null); + } + }); + }); + } catch { + resolve(null); + return; + } + req.on('error', () => resolve(null)); + req.on('timeout', () => { req.destroy(); resolve(null); }); + }); +} + /** * POST a tiny "say hi" prompt to /api/v1/llms//infer and decide if * the LLM actually serves inference. Returns ok=true when the response is @@ -386,6 +436,7 @@ const defaultDeps: StatusCommandDeps = { fetchProviders: defaultFetchProviders, fetchServerLlms: defaultFetchServerLlms, fetchSecretBackends: defaultFetchSecretBackends, + probeSecretBackend: defaultProbeSecretBackend, probeServerLlm: defaultProbeServerLlm, isTTY: process.stdout.isTTY ?? false, }; @@ -448,7 +499,7 @@ function formatProviderStatus(name: string, info: ProvidersInfo, ansi: boolean): } export function createStatusCommand(deps?: Partial): Command { - const { configDeps, credentialsDeps, log, write, checkHealth, checkLlm, fetchModels, fetchProviders, fetchServerLlms, probeServerLlm, fetchSecretBackends, isTTY } = { ...defaultDeps, ...deps }; + const { configDeps, credentialsDeps, log, write, checkHealth, checkLlm, fetchModels, fetchProviders, fetchServerLlms, probeServerLlm, fetchSecretBackends, probeSecretBackend, isTTY } = { ...defaultDeps, ...deps }; return new Command('status') .description('Show mcpctl status and connectivity') @@ -482,6 +533,30 @@ export function createStatusCommand(deps?: Partial): Command }))) : null; + // Same live probe the table view uses. `healthy` is derived from the + // probe, NOT from tokenMeta.lastRotationError — that field is only ever + // written for token-auth backends, so scripts consuming it were told + // every kubernetes-auth backend was healthy unconditionally. + const secretBackendsWithHealth = secretBackends !== null + ? await Promise.all(secretBackends.map(async (b) => { + const health = await probeSecretBackend(config.mcpdUrl, b.id, token); + return { + name: b.name, + type: b.type, + healthy: health !== null && health.live && health.ready, + live: health?.live ?? null, + ready: health?.ready ?? null, + servingStale: health?.cache?.servingStale ?? 0, + error: health === null + ? 'health probe failed' + : !health.live ? (health.liveDetail ?? 'unreachable') + : !health.ready ? (health.readyDetail ?? 'auth failed') + : null, + rotationError: b.tokenMeta?.lastRotationError ?? null, + }; + })) + : null; + const llm = llmLabel ? llmStatus === 'ok' ? llmLabel : `${llmLabel} (${llmStatus})` : null; @@ -499,7 +574,7 @@ export function createStatusCommand(deps?: Partial): Command llmStatus, ...(providersInfo ? { providers: providersInfo } : {}), ...(serverLlmsWithHealth !== null ? { serverLlms: serverLlmsWithHealth } : {}), - ...(secretBackends !== null ? { secretBackends: secretBackends.map((b) => ({ name: b.name, type: b.type, healthy: !b.tokenMeta?.lastRotationError, error: b.tokenMeta?.lastRotationError ?? null })) } : {}), + ...(secretBackends !== null ? { secretBackends: secretBackendsWithHealth } : {}), }; log(opts.output === 'json' ? formatJson(status) : formatYaml(status)); @@ -530,7 +605,7 @@ export function createStatusCommand(deps?: Partial): Command if (!llmLabel) { log(`LLM: not configured (run 'mcpctl config setup')`); - await renderSecretBackendsSection(secretBackendsPromise, isTTY); + await renderSecretBackendsSection(secretBackendsPromise, isTTY, config.mcpdUrl, token); await renderServerLlmsSection(serverLlmsPromise, config.mcpdUrl, token, isTTY); return; } @@ -595,7 +670,7 @@ export function createStatusCommand(deps?: Partial): Command } } - await renderSecretBackendsSection(secretBackendsPromise, isTTY); + await renderSecretBackendsSection(secretBackendsPromise, isTTY, config.mcpdUrl, token); await renderServerLlmsSection(serverLlmsPromise, config.mcpdUrl, token, isTTY); }); @@ -609,21 +684,50 @@ export function createStatusCommand(deps?: Partial): Command async function renderSecretBackendsSection( backendsPromise: Promise, ansi: boolean, + mcpdUrl: string, + token: string | null, ): Promise { const backends = await backendsPromise; if (backends === null || backends.length === 0) return; - const parts = backends.map((b) => { - const err = b.tokenMeta?.lastRotationError; - const tag = b.isDefault ? `${b.name}*` : b.name; - if (err) { - const short = err.split('\n')[0]?.slice(0, 80) ?? 'error'; - return ansi ? `${tag} ${RED}✗ ${short}${RESET}` : `${tag} ✗ ${short}`; - } - return ansi ? `${tag} ${GREEN}✓${RESET}` : `${tag} ✓`; - }); + const healths = await Promise.all(backends.map((b) => probeSecretBackend(mcpdUrl, b.id, token))); + const parts = backends.map((b, i) => renderOneBackend(b, healths[i] ?? null, ansi)); log(`Secrets: ${parts.join(', ')}`); } + /** + * Render one backend's status line. + * + * This used to be `tokenMeta.lastRotationError ? red : green`, which was a + * hard-coded green tick for every `auth: kubernetes` backend — the rotator + * only writes that field for token-auth backends, so it was never set and + * `mcpctl status` reported OpenBao healthy even when it was unreachable. + * Rotation state is now one clause among several, not the only signal. + */ + function renderOneBackend(b: SecretBackendInfo, health: SecretBackendHealth | null, ansi: boolean): string { + const tag = b.isDefault === true ? `${b.name}*` : b.name; + const paint = (colour: string, text: string): string => (ansi ? `${colour}${text}${RESET}` : text); + const rotationErr = b.tokenMeta?.lastRotationError ?? ''; + const rotationClause = rotationErr === '' + ? '' + : ` (rotation: ${rotationErr.split('\n')[0]?.slice(0, 60) ?? 'error'})`; + + if (health === null) { + // The probe itself failed. Unknown is not healthy — say so. + return `${tag} ${paint(YELLOW, '? unknown')}${rotationClause}`; + } + if (!health.live) { + return `${tag} ${paint(RED, `✗ unreachable: ${health.liveDetail ?? 'no detail'}`)}${rotationClause}`; + } + if (!health.ready) { + return `${tag} ${paint(RED, `✗ auth failed: ${(health.readyDetail ?? 'no detail').slice(0, 60)}`)}${rotationClause}`; + } + const stale = health.cache?.servingStale ?? 0; + if (stale > 0) { + return `${tag} ${paint(YELLOW, `⚠ degraded — serving ${String(stale)} cached secret(s)`)}${rotationClause}`; + } + return `${tag} ${paint(GREEN, '✓ reachable')}${rotationClause}`; + } + /** * Print a "Server LLMs:" section listing mcpd-managed Llm rows by tier * with a per-LLM "say hi" liveness probe. Distinct from the mcplocal-side diff --git a/src/cli/tests/commands/status.test.ts b/src/cli/tests/commands/status.test.ts index d627b55..b6b5ee9 100644 --- a/src/cli/tests/commands/status.test.ts +++ b/src/cli/tests/commands/status.test.ts @@ -30,6 +30,7 @@ function baseDeps(overrides?: Partial): Partial null, probeServerLlm: async () => ({ ok: true, ms: 12, say: 'hi' }), fetchSecretBackends: async () => null, + probeSecretBackend: async () => ({ live: true, ready: true, cache: { entries: 0, servingStale: 0 } }), isTTY: false, ...overrides, }; @@ -46,33 +47,73 @@ afterEach(() => { }); describe('status command', () => { + const BAO = { id: 'b1', name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: null } }; + it('shows a healthy secret backend in the Secrets line', async () => { const cmd = createStatusCommand(baseDeps({ - fetchSecretBackends: async () => [ - { name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: null } }, - { name: 'default', type: 'plaintext' }, - ], + fetchSecretBackends: async () => [BAO, { id: 'b2', name: 'default', type: 'plaintext' }], })); await cmd.parseAsync([], { from: 'user' }); const out = output.join('\n'); expect(out).toContain('Secrets:'); - expect(out).toContain('bao* ✓'); - expect(out).toContain('default ✓'); + expect(out).toContain('bao* ✓ reachable'); + expect(out).toContain('default ✓ reachable'); }); it('flags a dead secret-backend token in the Secrets line', async () => { const cmd = createStatusCommand(baseDeps({ fetchSecretBackends: async () => [ - { name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: 'BACKEND_TOKEN_DEAD: rejected the stored token\nmore detail' } }, + { ...BAO, tokenMeta: { lastRotationError: 'BACKEND_TOKEN_DEAD: rejected the stored token\nmore detail' } }, ], })); await cmd.parseAsync([], { from: 'user' }); const out = output.join('\n'); - expect(out).toContain('bao* ✗'); expect(out).toContain('BACKEND_TOKEN_DEAD'); expect(out).not.toContain('more detail'); // only first line, truncated }); + it('reports an unreachable backend even when rotation never errored', async () => { + // THE bug: a kubernetes-auth backend never writes tokenMeta.lastRotationError, + // so this line used to render a green tick with OpenBao completely down. + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => ({ live: false, liveDetail: 'sealed', ready: false }), + })); + await cmd.parseAsync([], { from: 'user' }); + const out = output.join('\n'); + expect(out).toContain('bao* ✗ unreachable: sealed'); + expect(out).not.toContain('✓'); + }); + + it('distinguishes reachable-but-unusable from unreachable', async () => { + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => ({ live: true, ready: false, readyDetail: 'HTTP 403 permission denied' }), + })); + await cmd.parseAsync([], { from: 'user' }); + expect(output.join('\n')).toContain('bao* ✗ auth failed: HTTP 403 permission denied'); + }); + + it('reports degraded while serving cached secrets', async () => { + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => ({ live: true, ready: true, cache: { entries: 9, servingStale: 7 } }), + })); + await cmd.parseAsync([], { from: 'user' }); + expect(output.join('\n')).toContain('bao* ⚠ degraded — serving 7 cached secret(s)'); + }); + + it('reports unknown — never healthy — when the probe itself fails', async () => { + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => null, + })); + await cmd.parseAsync([], { from: 'user' }); + const out = output.join('\n'); + expect(out).toContain('bao* ? unknown'); + expect(out).not.toContain('✓'); + }); + it('omits the Secrets line when mcpd returns no backends', async () => { const cmd = createStatusCommand(baseDeps({ fetchSecretBackends: async () => null })); await cmd.parseAsync([], { from: 'user' }); diff --git a/src/mcpd/src/bootstrap/warm-secret-cache.ts b/src/mcpd/src/bootstrap/warm-secret-cache.ts new file mode 100644 index 0000000..665f984 --- /dev/null +++ b/src/mcpd/src/bootstrap/warm-secret-cache.ts @@ -0,0 +1,67 @@ +/** + * One-shot: resolve every secret that a running server depends on, so the + * value cache holds a last-known-good copy before anything needs it. + * + * The caching driver absorbs a backend outage by serving the last value it saw + * — but only for secrets it has actually seen. Without this, a cold mcpd (fresh + * deploy, pod reschedule, crash-restart) has an empty cache, and if the backend + * is unreachable at that moment every secret-bearing server fails to start. + * + * This is the honest mitigation, and it is deliberately partial: if the backend + * is ALSO down at boot, this changes nothing and instances fail loudly, which + * is correct. The alternatives — persisting last-known-good to Postgres or to + * disk — are just "plaintext secrets at rest" wearing a hat, which is the thing + * we are trying to move away from. + * + * Best-effort by construction: a failure here must never block startup, and the + * warm is per-secret so one bad reference doesn't abandon the rest. + */ +import type { PrismaClient } from '@prisma/client'; +import type { SecretService } from '../services/secret.service.js'; +import type { ServerEnvEntry } from '../validation/mcp-server.schema.js'; + +export interface WarmLog { + info: (msg: string) => void; + warn: (msg: string) => void; +} + +export async function warmSecretCache( + prisma: PrismaClient, + secrets: SecretService, + log: WarmLog, +): Promise<{ warmed: number; failed: number }> { + const servers = await prisma.mcpServer.findMany({ + where: { replicas: { gt: 0 } }, + select: { name: true, env: true }, + }); + + // Distinct (secret, key) pairs — several servers commonly share one secret, + // and there is no point paying for the same read more than once. + const refs = new Map(); + for (const server of servers) { + for (const entry of (server.env ?? []) as ServerEnvEntry[]) { + const ref = entry.valueFrom?.secretRef; + if (ref === undefined) continue; + refs.set(`${ref.name}/${ref.key}`, { name: ref.name, key: ref.key }); + } + } + if (refs.size === 0) return { warmed: 0, failed: 0 }; + + let warmed = 0; + let failed = 0; + for (const ref of refs.values()) { + try { + // Value deliberately discarded — we only want it in the cache. + await secrets.resolve(ref.name, ref.key); + warmed++; + } catch { + // Expected when the backend is down, or when a server references a + // secret that no longer exists. Neither should block startup, and both + // surface loudly at instance-start time anyway. + failed++; + } + } + + log.info(`secret cache warm: ${String(warmed)} resolved, ${String(failed)} unavailable`); + return { warmed, failed }; +} diff --git a/src/mcpd/src/main.ts b/src/mcpd/src/main.ts index 02a92a5..16964bd 100644 --- a/src/mcpd/src/main.ts +++ b/src/mcpd/src/main.ts @@ -25,11 +25,13 @@ import { SecretBackendService } from './services/secret-backend.service.js'; import { SecretMigrateService } from './services/secret-migrate.service.js'; import { bootstrapSecretBackends } from './bootstrap/secret-backends.js'; import { backfillSecretKeyNames } from './bootstrap/secret-key-names.js'; +import { warmSecretCache } from './bootstrap/warm-secret-cache.js'; import { registerSecretBackendRoutes } from './routes/secret-backends.js'; import { registerSecretMigrateRoutes } from './routes/secret-migrate.js'; import { SecretBackendRotator } from './services/secret-backend-rotator.service.js'; import { SecretBackendRotatorLoop } from './services/secret-backend-rotator-loop.js'; import { registerSecretBackendRotateRoutes } from './routes/secret-backend-rotate.js'; +import { registerSecretBackendHealthRoutes } from './routes/secret-backend-health.js'; import { LlmRepository } from './repositories/llm.repository.js'; import { LlmService } from './services/llm.service.js'; import { InferenceTaskRepository } from './repositories/inference-task.repository.js'; @@ -474,16 +476,32 @@ async function main(): Promise { }, }, secretRefResolver: secretResolverBridge, + }, { + // Cache-transition events go through pino so BACKEND_UNREACHABLE / + // BACKEND_RECOVERED land in ErrorLogBuffer and `mcpctl errors`. + log: { + warn: (obj: Record, msg: string): void => { app.log.warn(obj, msg); }, + info: (obj: Record, msg: string): void => { app.log.info(obj, msg); }, + }, }); const secretService = new SecretService(secretRepo, secretBackendService); const secretMigrateService = new SecretMigrateService(secretRepo, secretBackendService); const secretBackendRotator = new SecretBackendRotator({ backends: secretBackendService, secrets: secretService, + log: { + error: (obj: Record, msg: string): void => { app.log.error(obj, msg); }, + warn: (msg: string): void => { app.log.warn(msg); }, + }, }); const secretBackendRotatorLoop = new SecretBackendRotatorLoop({ backends: secretBackendService, rotator: secretBackendRotator, + log: { + info: (msg: string): void => { app.log.info(`[rotator] ${msg}`); }, + warn: (msg: string): void => { app.log.warn(`[rotator] ${msg}`); }, + error: (obj: Record, msg: string): void => { app.log.error(obj, msg); }, + }, }); const llmAdapters = new LlmAdapterRegistry(); // LlmService takes the adapter registry so create()/update() can run an @@ -673,6 +691,7 @@ async function main(): Promise { registerSecretRoutes(app, secretService); registerSecretBackendRoutes(app, secretBackendService); registerSecretBackendRotateRoutes(app, secretBackendRotator); + registerSecretBackendHealthRoutes(app, secretBackendService); registerSecretMigrateRoutes(app, secretMigrateService); registerLlmRoutes(app, llmService); registerAgentRoutes(app, agentService); @@ -960,6 +979,18 @@ async function main(): Promise { app.log.error({ err }, 'secret keyNames backfill failed'); }); + // One-shot: pre-populate the secret value cache so a later backend outage is + // absorbed rather than cascading into instance ERROR loops. Best-effort — if + // the backend is already down at boot this is a no-op and instances fail + // honestly. See bootstrap/warm-secret-cache.ts. + warmSecretCache( + prisma, + secretService, + { info: (m: string): void => { app.log.info(m); }, warn: (m: string): void => { app.log.warn(m); } }, + ).catch((err: unknown) => { + app.log.warn({ err }, 'secret cache warm failed (non-fatal)'); + }); + // Graceful shutdown setupGracefulShutdown(app, { disconnectDb: async () => { diff --git a/src/mcpd/src/routes/secret-backend-health.ts b/src/mcpd/src/routes/secret-backend-health.ts new file mode 100644 index 0000000..8a0e457 --- /dev/null +++ b/src/mcpd/src/routes/secret-backend-health.ts @@ -0,0 +1,76 @@ +/** + * GET /api/v1/secretbackends/:id/health — a live probe of a secret backend. + * + * Exists because the only health signal we had was `tokenMeta.lastRotationError`, + * and the rotator writes that field ONLY for `auth: 'token'` backends + * (`SecretBackendRotator.isRotatable()`). A `kubernetes`-auth backend therefore + * never wrote it and rendered a hard-coded green tick in `mcpctl status` — even + * with OpenBao completely unreachable. + * + * Two signals, deliberately separate, mirroring the liveness/readiness split + * that instance health probes already use: + * + * live — is the backend reachable at all? (unauthenticated) + * ready — can we actually read through it? (uses our credentials) + * + * A backend that is `live` but not `ready` is the exact shape of a re-initialised + * OpenBao that left us holding valid-looking tokens granting nothing. Collapsing + * the two into one boolean is what hid that for four days. + * + * RBAC: no special mapping needed — `mapUrlToPermission` falls through to the + * generic `secretbackends` resource, so a GET requires `view:secretbackends`. + */ +import type { FastifyInstance } from 'fastify'; +import type { SecretBackendService } from '../services/secret-backend.service.js'; +import { NotFoundError } from '../services/mcp-server.service.js'; + +interface TokenMetaShape { + lastRotationAt?: string; + lastRotationError?: string | null; + rotatable?: boolean; +} + +export function registerSecretBackendHealthRoutes( + app: FastifyInstance, + backends: SecretBackendService, +): void { + app.get<{ Params: { id: string } }>('/api/v1/secretbackends/:id/health', async (request, reply) => { + try { + const backend = await backends.getById(request.params.id); + const driver = backends.driverFor(backend); + + const live = await driver.healthCheck?.() ?? { ok: true, detail: 'no probe' }; + // Only probe readiness if the backend answered at all — otherwise the + // auth check just re-reports the same outage with a confusing message. + const ready = live.ok + ? await driver.authCheck?.() ?? { ok: true, detail: 'no probe' } + : { ok: false, detail: 'not probed (backend unreachable)' }; + + const meta = (backend.tokenMeta ?? {}) as TokenMetaShape; + const cache = backends.cacheStatsFor(backend); + + return { + backend: backend.name, + type: backend.type, + live: live.ok, + liveDetail: live.detail, + ready: ready.ok, + readyDetail: ready.detail, + // Present only for cached (remote) backends; plaintext has no cache. + cache: cache ?? null, + rotation: { + rotatable: meta.rotatable ?? false, + lastRotationAt: meta.lastRotationAt ?? null, + lastRotationError: meta.lastRotationError ?? null, + }, + }; + } catch (err) { + if (err instanceof NotFoundError) { + reply.code(404); + return { error: err.message }; + } + reply.code(502); + return { error: err instanceof Error ? err.message : String(err) }; + } + }); +} diff --git a/src/mcpd/src/services/secret-backend-rotator-loop.ts b/src/mcpd/src/services/secret-backend-rotator-loop.ts index 2fae8ce..0cbef82 100644 --- a/src/mcpd/src/services/secret-backend-rotator-loop.ts +++ b/src/mcpd/src/services/secret-backend-rotator-loop.ts @@ -26,7 +26,11 @@ export interface SecretBackendRotatorLoopDeps { /** Override in tests. */ setTimeout?: (cb: () => void, ms: number) => NodeJS.Timeout; clearTimeout?: (t: NodeJS.Timeout) => void; - log?: { info: (msg: string) => void; warn: (msg: string) => void }; + log?: { + info: (msg: string) => void; + warn: (msg: string) => void; + error: (obj: Record, msg: string) => void; + }; } const DEFAULT_INTERVAL_MS = 24 * 3600 * 1000; @@ -36,7 +40,7 @@ export class SecretBackendRotatorLoop { private readonly timers = new Map(); private readonly setT: (cb: () => void, ms: number) => NodeJS.Timeout; private readonly clearT: (t: NodeJS.Timeout) => void; - private readonly log: { info: (msg: string) => void; warn: (msg: string) => void }; + private readonly log: NonNullable; private stopped = false; constructor(private readonly deps: SecretBackendRotatorLoopDeps) { @@ -44,9 +48,11 @@ export class SecretBackendRotatorLoop { this.clearT = deps.clearTimeout ?? ((t) => global.clearTimeout(t)); this.log = deps.log ?? { // eslint-disable-next-line no-console - info: (m) => console.log(`[rotator] ${m}`), + info: (m: string): void => { console.log(`[rotator] ${m}`); }, // eslint-disable-next-line no-console - warn: (m) => console.warn(`[rotator] ${m}`), + warn: (m: string): void => { console.warn(`[rotator] ${m}`); }, + // eslint-disable-next-line no-console + error: (obj: Record, m: string): void => { console.error(JSON.stringify({ level: 'fatal', ...obj, message: m })); }, }; } @@ -70,13 +76,10 @@ export class SecretBackendRotatorLoop { this.deps.rotator.healthCheck(b.id) .then((res) => { if (!res.ok) { - // eslint-disable-next-line no-console - console.error(JSON.stringify({ - level: 'fatal', - kind: 'BACKEND_TOKEN_DEAD', - backend: b.name, - message: res.message ?? 'unknown', - })); + this.log.error( + { kind: 'BACKEND_TOKEN_DEAD', backend: b.name }, + res.message ?? 'unknown', + ); this.log.warn(`backend '${b.name}' health check failed: ${res.message ?? 'unknown'}`); } }) diff --git a/src/mcpd/src/services/secret-backend-rotator.service.ts b/src/mcpd/src/services/secret-backend-rotator.service.ts index 0a8ed11..27a8989 100644 --- a/src/mcpd/src/services/secret-backend-rotator.service.ts +++ b/src/mcpd/src/services/secret-backend-rotator.service.ts @@ -53,18 +53,37 @@ export interface TokenMeta { rotatable?: boolean; } +/** + * Structured logger. Must be a real pino-shaped logger in production: the + * `BACKEND_TOKEN_DEAD` fatals below used to go out via bare `console.error`, + * which bypasses the pino multistream feeding `ErrorLogBuffer` — so the one + * failure `mcpctl errors` exists to surface was the one it never saw. + */ +export interface RotatorLog { + error(obj: Record, msg: string): void; + warn(msg: string): void; +} + export interface SecretBackendRotatorDeps { backends: SecretBackendService; secrets: SecretService; fetch?: typeof globalThis.fetch; now?: () => Date; + log?: RotatorLog; } export class SecretBackendRotator { private readonly now: () => Date; + private readonly log: RotatorLog; constructor(private readonly deps: SecretBackendRotatorDeps) { this.now = deps.now ?? (() => new Date()); + this.log = deps.log ?? { + // eslint-disable-next-line no-console + error: (obj: Record, msg: string): void => { console.error(JSON.stringify({ level: 'fatal', ...obj, message: msg })); }, + // eslint-disable-next-line no-console + warn: (msg: string): void => { console.warn(msg); }, + }; } /** True iff this backend is a wizard-provisioned token-auth openbao with rotation enabled. */ @@ -144,15 +163,16 @@ export class SecretBackendRotator { : err; const wrappedMsg = wrapped instanceof Error ? wrapped.message : String(wrapped); await this.recordError(backendId, meta, wrappedMsg); - // Loud, structured log so the operator sees it in `kubectl logs deploy/mcpd`. - // eslint-disable-next-line no-console - console.error(JSON.stringify({ - level: 'fatal', - kind: tokenDead ? 'BACKEND_TOKEN_DEAD' : 'BACKEND_ROTATION_FAILED', - backend: backend.name, - url: cfg.url, - message: wrappedMsg, - })); + // Loud and structured, through pino so it also lands in ErrorLogBuffer + // and therefore in `mcpctl errors` — not just in `kubectl logs`. + this.log.error( + { + kind: tokenDead ? 'BACKEND_TOKEN_DEAD' : 'BACKEND_ROTATION_FAILED', + backend: backend.name, + url: cfg.url, + }, + wrappedMsg, + ); throw wrapped; } @@ -164,7 +184,7 @@ export class SecretBackendRotator { // Log but don't fail the rotation — the new token is already live. const msg = err instanceof Error ? err.message : String(err); // eslint-disable-next-line no-console - console.warn(`rotation: revoke old accessor '${oldAccessor}' on backend '${backend.name}' failed (continuing): ${msg}`); + this.log.warn(`rotation: revoke old accessor '${oldAccessor}' on backend '${backend.name}' failed (continuing): ${msg}`); } } @@ -249,7 +269,7 @@ export class SecretBackendRotator { } catch (inner) { // Don't mask the original error — just log the DB failure. // eslint-disable-next-line no-console - console.warn(`rotation: failed to persist lastRotationError (${message}): ${inner instanceof Error ? inner.message : String(inner)}`); + this.log.warn(`rotation: failed to persist lastRotationError (${message}): ${inner instanceof Error ? inner.message : String(inner)}`); } } } diff --git a/src/mcpd/src/services/secret-backend.service.ts b/src/mcpd/src/services/secret-backend.service.ts index fcf5d97..c1578a1 100644 --- a/src/mcpd/src/services/secret-backend.service.ts +++ b/src/mcpd/src/services/secret-backend.service.ts @@ -2,6 +2,7 @@ import type { SecretBackend } from '@prisma/client'; import type { ISecretBackendRepository } from '../repositories/secret-backend.repository.js'; import type { SecretBackendDriver } from './secret-backends/types.js'; import { createDriver, type DriverFactoryDeps } from './secret-backends/factory.js'; +import { CachingSecretBackendDriver, type CachingDriverOptions, type CacheStats } from './secret-backends/caching.js'; import { NotFoundError, ConflictError } from './mcp-server.service.js'; export class SecretBackendInUseError extends Error { @@ -17,6 +18,7 @@ export class SecretBackendService { constructor( private readonly repo: ISecretBackendRepository, private readonly driverDeps: DriverFactoryDeps, + private readonly cacheOpts: CachingDriverOptions = {}, ) {} async list(): Promise { @@ -87,12 +89,34 @@ export class SecretBackendService { this.driverCache.delete(id); } - /** Get the driver for a given backend id, creating + caching on first call. */ + /** + * Get the driver for a given backend id, creating + caching on first call. + * + * Remote backends are wrapped in `CachingSecretBackendDriver` so a backend + * outage degrades to "serving last known-good" instead of failing every + * caller. `plaintext` is deliberately NOT wrapped: its `read()` is an + * identity function over the DB row passed in by the caller, so a value cache + * there would serve pre-update data with nothing to invalidate it. + * + * Config changes invalidate for free — `update()` and `delete()` drop this + * map, and the value cache lives inside the driver instance, which is right: + * if `url`/`mount`/`pathPrefix` change, the cached names now mean something + * different. + */ driverFor(backend: SecretBackend): SecretBackendDriver { const cached = this.driverCache.get(backend.id); if (cached) return cached; - const driver = createDriver(backend, this.driverDeps); + const base = createDriver(backend, this.driverDeps); + const driver = backend.type === 'plaintext' + ? base + : new CachingSecretBackendDriver(base, { ...this.cacheOpts, backendName: backend.name }); this.driverCache.set(backend.id, driver); return driver; } + + /** Cache state for a backend, for the health endpoint. Never exposes values. */ + cacheStatsFor(backend: SecretBackend): CacheStats | undefined { + const driver = this.driverFor(backend); + return driver instanceof CachingSecretBackendDriver ? driver.stats() : undefined; + } } diff --git a/src/mcpd/src/services/secret-backends/caching.ts b/src/mcpd/src/services/secret-backends/caching.ts new file mode 100644 index 0000000..22b4276 --- /dev/null +++ b/src/mcpd/src/services/secret-backends/caching.ts @@ -0,0 +1,198 @@ +/** + * Caching + stale-while-error decorator for any `SecretBackendDriver`. + * + * ## Why this exists + * + * `SecretService.resolveData()` calls `driver.read()` on *every* use, and every + * consumer funnels through it: server env resolution, LLM api keys, chat, git + * providers, code repos, webhooks. With a remote backend that means one network + * round-trip per secret per call, and — worse — any OpenBao blip propagates + * straight through. An instance that restarts during a blip fails env + * resolution, gets marked ERROR, and enters a 30s×5-then-5min backoff + * (`instance.service.ts`), so a few seconds of backend unavailability turns + * into minutes of degraded service. + * + * ## Semantics + * + * - **Fresh** (age < ttlMs): served from memory, no network. + * - **Stale-while-error**: past the TTL we always try the backend first. If it + * answers, we refresh. If it fails *as a transport failure* + * (`SecretBackendUnavailableError`), we serve the last known-good value + * instead of throwing. This is the part that actually stops the ERROR storm. + * - **`SecretNotFoundError` evicts and rethrows.** Never served stale — that + * would resurrect a deliberately deleted or revoked credential, which is + * strictly worse than an outage. + * - **Any other error rethrows, without stale.** A 403 that survives a token + * refresh means our grants were revoked; papering over it with cached data is + * exactly how an upstream OpenBao re-init once went unnoticed for four days. + * - **No negative caching.** A miss must re-check; retry/backoff already lives + * in the driver. + * + * The stale window is deliberately unbounded. A cap would mean a long outage + * eventually takes mcpd down anyway, which defeats the purpose, and the + * revoked-credential case is already handled definitively by `SecretNotFound`. + * + * ## What this does NOT fix + * + * A cold cache during an outage. If mcpd restarts while the backend is + * unreachable, nothing has a last-known-good value and secret-bearing servers + * fail to start — honestly, with a loud error. That is the correct behaviour: + * booting a server with an empty credential is the failure mode that had + * gitea-mcp reporting healthy while every authed call failed. The mitigation is + * to warm this cache at boot, not to invent a value. + * + * Values live in heap in cleartext for the TTL, so the map is bounded (LRU) and + * values are never logged. + */ +import type { SecretBackendDriver, SecretData, ExternalRef } from './types.js'; +import { SecretNotFoundError, SecretBackendUnavailableError } from './types.js'; + +export interface CachingDriverLog { + warn(obj: Record, msg: string): void; + info(obj: Record, msg: string): void; +} + +export interface CachingDriverOptions { + /** How long a value is served without consulting the backend. */ + ttlMs?: number; + /** LRU bound — these are plaintext credentials held in memory. */ + maxEntries?: number; + /** Backend name, for log context only. */ + backendName?: string; + now?: () => number; + log?: CachingDriverLog; +} + +interface CacheEntry { + data: SecretData; + fetchedAt: number; + /** Set when we last served this past its TTL because the backend was down. */ + staleSince: number | undefined; +} + +export const DEFAULT_CACHE_TTL_MS = 300_000; +export const DEFAULT_CACHE_MAX_ENTRIES = 500; + +const NOOP_LOG: CachingDriverLog = { warn: () => undefined, info: () => undefined }; + +export interface CacheStats { + entries: number; + servingStale: number; + oldestStaleSince: number | undefined; +} + +export class CachingSecretBackendDriver implements SecretBackendDriver { + readonly kind: string; + + private readonly entries = new Map(); + private readonly ttlMs: number; + private readonly maxEntries: number; + private readonly backendName: string; + private readonly nowFn: () => number; + private readonly log: CachingDriverLog; + + constructor(private readonly inner: SecretBackendDriver, opts: CachingDriverOptions = {}) { + this.kind = `cached:${inner.kind}`; + this.ttlMs = opts.ttlMs ?? DEFAULT_CACHE_TTL_MS; + this.maxEntries = opts.maxEntries ?? DEFAULT_CACHE_MAX_ENTRIES; + this.backendName = opts.backendName ?? inner.kind; + this.nowFn = opts.now ?? ((): number => Date.now()); + this.log = opts.log ?? NOOP_LOG; + } + + async read(input: { name: string; externalRef: ExternalRef; data: SecretData }): Promise { + const now = this.nowFn(); + const cached = this.entries.get(input.name); + + if (cached !== undefined && now - cached.fetchedAt < this.ttlMs) { + this.touch(input.name, cached); + return cached.data; + } + + try { + const data = await this.inner.read(input); + if (cached?.staleSince !== undefined) { + // Edge-triggered: only on the transition back to healthy. + this.log.info( + { kind: 'BACKEND_RECOVERED', backend: this.backendName, secret: input.name, + staleForMs: now - cached.staleSince }, + `secret backend '${this.backendName}' recovered; '${input.name}' is live again`, + ); + } + this.store(input.name, { data, fetchedAt: now, staleSince: undefined }); + return data; + } catch (err) { + if (err instanceof SecretNotFoundError) { + // Definitive. Drop the stale copy so we can never hand it out later. + this.entries.delete(input.name); + throw err; + } + if (!(err instanceof SecretBackendUnavailableError) || cached === undefined) { + throw err; + } + if (cached.staleSince === undefined) { + cached.staleSince = now; + this.log.warn( + { kind: 'BACKEND_UNREACHABLE', backend: this.backendName, secret: input.name, + ageMs: now - cached.fetchedAt, reason: err.message }, + `secret backend '${this.backendName}' unreachable; serving cached '${input.name}'`, + ); + } + this.touch(input.name, cached); + return cached.data; + } + } + + async write(input: { name: string; data: SecretData }): Promise<{ externalRef: ExternalRef; storedData: SecretData }> { + const result = await this.inner.write(input); + // Cache what a subsequent read() would return — the values just written — + // not `storedData`, which remote drivers deliberately leave empty. + this.store(input.name, { data: input.data, fetchedAt: this.nowFn(), staleSince: undefined }); + return result; + } + + async delete(input: { name: string; externalRef: ExternalRef }): Promise { + await this.inner.delete(input); + this.entries.delete(input.name); + } + + async list(): Promise> { + return this.inner.list(); + } + + async healthCheck(): Promise<{ ok: boolean; detail?: string }> { + return this.inner.healthCheck?.() ?? { ok: true, detail: 'no probe' }; + } + + async authCheck(): Promise<{ ok: boolean; detail?: string }> { + return this.inner.authCheck?.() ?? { ok: true, detail: 'no probe' }; + } + + /** Cache state for the backend health endpoint. Never exposes values. */ + stats(): CacheStats { + let servingStale = 0; + let oldestStaleSince: number | undefined; + for (const e of this.entries.values()) { + if (e.staleSince === undefined) continue; + servingStale++; + if (oldestStaleSince === undefined || e.staleSince < oldestStaleSince) oldestStaleSince = e.staleSince; + } + return { entries: this.entries.size, servingStale, oldestStaleSince }; + } + + /** Move an entry to the MRU end of the insertion-ordered Map. */ + private touch(name: string, entry: CacheEntry): void { + this.entries.delete(name); + this.entries.set(name, entry); + } + + private store(name: string, entry: CacheEntry): void { + this.entries.delete(name); + this.entries.set(name, entry); + while (this.entries.size > this.maxEntries) { + const oldest = this.entries.keys().next(); + if (oldest.done === true) break; + this.entries.delete(oldest.value); + } + } +} diff --git a/src/mcpd/src/services/secret-backends/openbao.ts b/src/mcpd/src/services/secret-backends/openbao.ts index 93e902c..34ddedc 100644 --- a/src/mcpd/src/services/secret-backends/openbao.ts +++ b/src/mcpd/src/services/secret-backends/openbao.ts @@ -28,6 +28,7 @@ */ import { readFile } from 'node:fs/promises'; import type { SecretBackendDriver, SecretData, ExternalRef, SecretRefResolver } from './types.js'; +import { SecretNotFoundError, SecretBackendUnavailableError } from './types.js'; /** Best-effort read of a response body for error messages. Empty on parse failure. */ async function bodyText(res: Response): Promise { @@ -77,10 +78,23 @@ export interface OpenBaoDriverDeps { readServiceAccountToken?: (path: string) => Promise; /** Clock for cache TTL — overridable in tests. */ now?: () => number; + /** Per-request timeout. Without one, an unreachable OpenBao hangs every caller. */ + timeoutMs?: number; + /** Total attempts for retryable failures (network / 5xx / 429). 1 disables retry. */ + maxAttempts?: number; + /** Base for exponential backoff between retries; full jitter is applied. */ + backoffBaseMs?: number; + /** Test seam — real sleeps would make the retry tests take seconds. */ + sleep?: (ms: number) => Promise; } const SA_TOKEN_DEFAULT_PATH = '/var/run/secrets/kubernetes.io/serviceaccount/token'; const TOKEN_RENEW_GRACE_MS = 60_000; +const DEFAULT_TIMEOUT_MS = 5_000; +const DEFAULT_MAX_ATTEMPTS = 3; +const DEFAULT_BACKOFF_BASE_MS = 200; +/** Statuses worth retrying: the backend is up but cannot answer right now. */ +const RETRYABLE_STATUS = new Set([429, 500, 502, 503, 504]); export class OpenBaoDriver implements SecretBackendDriver { readonly kind = 'openbao'; @@ -98,6 +112,10 @@ export class OpenBaoDriver implements SecretBackendDriver { private readonly resolver: SecretRefResolver | undefined; private readonly readSaToken: (path: string) => Promise; private readonly nowFn: () => number; + private readonly timeoutMs: number; + private readonly maxAttempts: number; + private readonly backoffBaseMs: number; + private readonly sleep: (ms: number) => Promise; // Cached vault token + when (epoch ms) it should be considered expired and refetched. private cachedToken: string | undefined; @@ -131,13 +149,19 @@ export class OpenBaoDriver implements SecretBackendDriver { if (deps.secretRefResolver !== undefined) this.resolver = deps.secretRefResolver; this.readSaToken = deps.readServiceAccountToken ?? ((path) => readFile(path, 'utf-8').then((s) => s.trim())); this.nowFn = deps.now ?? (() => Date.now()); + this.timeoutMs = deps.timeoutMs ?? DEFAULT_TIMEOUT_MS; + this.maxAttempts = deps.maxAttempts ?? DEFAULT_MAX_ATTEMPTS; + this.backoffBaseMs = deps.backoffBaseMs ?? DEFAULT_BACKOFF_BASE_MS; + this.sleep = deps.sleep ?? ((ms: number): Promise => new Promise((r) => { setTimeout(r, ms); })); } async read(input: { name: string; externalRef: ExternalRef; data: SecretData }): Promise { const path = this.pathFor(input.name); const res = await this.request('GET', `/v1/${this.mount}/data/${path}`); if (res.status === 404) { - throw new Error(`OpenBao: secret '${input.name}' not found at ${path}`); + // Definitive answer, not a transport failure — the caching decorator + // must evict rather than serve a stale value here. + throw new SecretNotFoundError(`OpenBao: secret '${input.name}' not found at ${path}`); } if (!res.ok) throw new Error(`OpenBao read ${path}: HTTP ${res.status} ${await bodyText(res)}`); const body = await res.json() as { data?: { data?: SecretData } }; @@ -174,10 +198,50 @@ export class OpenBaoDriver implements SecretBackendDriver { })); } + /** + * LIVENESS. Deliberately unauthenticated: `sys/health` needs no token, and + * routing it through `request()` (as this used to) took a login first — so an + * expired role reported as "OpenBao is down", and every probe cost a login. + * + * OpenBao encodes its state in the status code, so map it rather than + * collapsing everything to ok/not-ok. + */ async healthCheck(): Promise<{ ok: boolean; detail?: string }> { try { - const res = await this.request('GET', '/v1/sys/health'); - return { ok: res.ok, detail: `HTTP ${res.status}` }; + const headers: Record = {}; + if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace; + const res = await this.fetchImpl(`${this.url}/v1/sys/health`, { + method: 'GET', + headers, + signal: AbortSignal.timeout(this.timeoutMs), + }); + switch (res.status) { + case 200: return { ok: true, detail: 'active' }; + case 429: return { ok: true, detail: 'standby' }; + case 472: case 473: return { ok: true, detail: 'replication secondary' }; + case 501: return { ok: false, detail: 'not initialized' }; + case 503: return { ok: false, detail: 'sealed' }; + default: return { ok: res.ok, detail: `HTTP ${String(res.status)}` }; + } + } catch (err) { + return { ok: false, detail: err instanceof Error ? err.message : String(err) }; + } + } + + /** + * READINESS. Exercises the capability we actually depend on — read/list under + * `//` — using the credentials we hold. + * + * `list()` rather than `auth/token/lookup-self` on purpose: lookup-self only + * proves the token exists, not that its policy still grants anything. The + * four-day outage in e51b924 was exactly a live token whose grants had been + * dropped by an upstream re-init. The existing read policy already permits + * this call, so it needs no bao-side change. + */ + async authCheck(): Promise<{ ok: boolean; detail?: string }> { + try { + await this.list(); + return { ok: true, detail: `readable at ${this.mount}/${this.pathPrefix}` }; } catch (err) { return { ok: false, detail: err instanceof Error ? err.message : String(err) }; } @@ -206,11 +270,28 @@ export class OpenBaoDriver implements SecretBackendDriver { const loginUrl = `${this.url}/v1/auth/${this.k8sAuthMount}/login`; const headers: Record = { 'Content-Type': 'application/json' }; if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace; - const res = await this.fetchImpl(loginUrl, { - method: 'POST', - headers, - body: JSON.stringify({ role: this.k8sRole, jwt }), - }); + // Bounded like every other call: a hung login is indistinguishable from a + // hung read to the caller, and this one used to have no timeout at all. + let res: Response; + try { + res = await this.fetchImpl(loginUrl, { + method: 'POST', + headers, + body: JSON.stringify({ role: this.k8sRole, jwt }), + signal: AbortSignal.timeout(this.timeoutMs), + }); + } catch (err) { + throw new SecretBackendUnavailableError( + `OpenBao kubernetes login (role=${this.k8sRole!}): ${err instanceof Error ? err.message : String(err)}`, + { cause: err }, + ); + } + if (RETRYABLE_STATUS.has(res.status)) { + throw new SecretBackendUnavailableError( + `OpenBao kubernetes login (role=${this.k8sRole!}): HTTP ${String(res.status)}`, + { lastStatus: res.status }, + ); + } if (!res.ok) { const text = await res.text().catch(() => ''); throw new Error(`OpenBao kubernetes login (role=${this.k8sRole!}): HTTP ${String(res.status)} ${text}`); @@ -229,30 +310,77 @@ export class OpenBaoDriver implements SecretBackendDriver { return clientToken; } - private async request(method: string, path: string, body?: unknown): Promise { - const token = await this.getToken(); + /** Build a fresh RequestInit — headers must not be shared across attempts. */ + private buildInit(method: string, token: string, body?: unknown): RequestInit { const headers: Record = { 'X-Vault-Token': token }; if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace; if (body !== undefined) headers['Content-Type'] = 'application/json'; - - const init: RequestInit = { method, headers }; + const init: RequestInit = { method, headers, signal: AbortSignal.timeout(this.timeoutMs) }; if (body !== undefined) init.body = JSON.stringify(body); + return init; + } - const res = await this.fetchImpl(`${this.url}${path}`, init); + /** Full-jitter exponential backoff, so concurrent callers don't resonate. */ + private backoffFor(attempt: number): number { + return Math.random() * this.backoffBaseMs * Math.pow(2, attempt - 1); + } - // If the cached token expired between cache-check and request (k8s clock - // skew, server-side revocation, etc.), purge cache and retry once. - if (res.status === 403 && this.cachedToken !== undefined) { - this.cachedToken = undefined; - this.cachedTokenExpiresAt = 0; - const fresh = await this.getToken(); - const retryHeaders: Record = { 'X-Vault-Token': fresh }; - if (this.namespace !== undefined) retryHeaders['X-Vault-Namespace'] = this.namespace; - if (body !== undefined) retryHeaders['Content-Type'] = 'application/json'; - const retryInit: RequestInit = { method, headers: retryHeaders }; - if (body !== undefined) retryInit.body = JSON.stringify(body); - return this.fetchImpl(`${this.url}${path}`, retryInit); + private async request(method: string, path: string, body?: unknown): Promise { + const url = `${this.url}${path}`; + let lastStatus: number | undefined; + let lastErr: unknown; + + for (let attempt = 1; attempt <= this.maxAttempts; attempt++) { + let res: Response; + try { + const token = await this.getToken(); + res = await this.fetchImpl(url, this.buildInit(method, token, body)); + } catch (err) { + // Network failure, DNS failure, or our own AbortSignal firing. + lastErr = err; + if (attempt < this.maxAttempts) { + await this.sleep(this.backoffFor(attempt)); + continue; + } + throw new SecretBackendUnavailableError( + `OpenBao ${method} ${path}: ${err instanceof Error ? err.message : String(err)} (after ${String(attempt)} attempt(s))`, + { cause: err }, + ); + } + + // If the cached token expired between cache-check and request (k8s clock + // skew, server-side revocation, etc.), purge cache and retry once. This + // is deliberately OUTSIDE the retry budget: it is a credential refresh, + // not a backend-unavailable condition, and it must stay single-shot so a + // genuinely revoked grant fails loudly instead of looping. + if (res.status === 403 && this.cachedToken !== undefined) { + this.cachedToken = undefined; + this.cachedTokenExpiresAt = 0; + const fresh = await this.getToken(); + return this.fetchImpl(url, this.buildInit(method, fresh, body)); + } + + // The backend is up but cannot answer right now — 503 is also what a + // sealed OpenBao returns, which used to be an immediate hard failure. + if (RETRYABLE_STATUS.has(res.status) && attempt < this.maxAttempts) { + lastStatus = res.status; + await this.sleep(this.backoffFor(attempt)); + continue; + } + if (RETRYABLE_STATUS.has(res.status)) { + throw new SecretBackendUnavailableError( + `OpenBao ${method} ${path}: HTTP ${String(res.status)} after ${String(attempt)} attempt(s)`, + { lastStatus: res.status }, + ); + } + + return res; } - return res; + + /* c8 ignore next 5 -- unreachable: every loop exit above returns or throws */ + throw new SecretBackendUnavailableError( + `OpenBao ${method} ${path}: exhausted ${String(this.maxAttempts)} attempt(s)`, + lastErr !== undefined ? { cause: lastErr, ...(lastStatus !== undefined ? { lastStatus } : {}) } : (lastStatus !== undefined ? { lastStatus } : {}), + ); } } diff --git a/src/mcpd/src/services/secret-backends/types.ts b/src/mcpd/src/services/secret-backends/types.ts index bab41d5..8d91ce9 100644 --- a/src/mcpd/src/services/secret-backends/types.ts +++ b/src/mcpd/src/services/secret-backends/types.ts @@ -46,8 +46,24 @@ export interface SecretBackendDriver { /** List everything the backend knows about. Used for migration + drift detection. */ list(): Promise>; - /** Optional: health probe. Used by `mcpctl describe secretbackend`. */ + /** + * Optional LIVENESS probe: is the backend reachable at all? + * + * Must NOT require authentication — the whole point is to separate "the + * backend is down/sealed" from "our credentials stopped working". Compare + * `authCheck()`, which is the readiness half. + */ healthCheck?(): Promise<{ ok: boolean; detail?: string }>; + + /** + * Optional READINESS probe: can we actually read through this backend with + * the credentials we hold? + * + * A backend that answers `healthCheck()` but fails here is the exact shape of + * the incident where a re-initialised OpenBao left mcpd holding valid-looking + * tokens that granted nothing. Reporting one signal for both hides it. + */ + authCheck?(): Promise<{ ok: boolean; detail?: string }>; } /** Stored config for a SecretBackend row; dispatched on `type`. */ @@ -66,3 +82,40 @@ export interface BackendRow { export interface SecretRefResolver { resolve(secretName: string, key: string): Promise; } + +/** + * The backend gave a definitive answer: this secret (or key) does not exist. + * + * Callers may treat this as final. The caching decorator EVICTS on this and + * never serves a stale value for it — serving stale here would resurrect a + * deliberately deleted or revoked credential, which is strictly worse than an + * outage. + */ +export class SecretNotFoundError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'SecretNotFoundError'; + } +} + +/** + * The backend could not be reached or did not answer: DNS/TCP failure, request + * timeout, or an exhausted retry budget against 5xx/429. + * + * This is the ONLY error the caching decorator will serve a stale value for. + * The distinction has to be typed rather than string-matched: a mis-classified + * "not found" would resurrect deleted secrets, and a mis-classified auth + * failure would silently paper over a backend whose grants were revoked — the + * failure mode that let an OpenBao re-init break every secret write for four + * days (commit e51b924). + */ +export class SecretBackendUnavailableError extends Error { + /** HTTP status of the last attempt, when the failure was an HTTP response. */ + readonly lastStatus: number | undefined; + + constructor(message: string, options?: { cause?: unknown; lastStatus?: number }) { + super(message, options?.cause !== undefined ? { cause: options.cause } : undefined); + this.name = 'SecretBackendUnavailableError'; + this.lastStatus = options?.lastStatus; + } +} diff --git a/src/mcpd/tests/secret-backend-health-route.test.ts b/src/mcpd/tests/secret-backend-health-route.test.ts new file mode 100644 index 0000000..809b3f2 --- /dev/null +++ b/src/mcpd/tests/secret-backend-health-route.test.ts @@ -0,0 +1,98 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import Fastify from 'fastify'; +import type { FastifyInstance } from 'fastify'; +import type { SecretBackend } from '@prisma/client'; +import { registerSecretBackendHealthRoutes } from '../src/routes/secret-backend-health.js'; +import { SecretBackendService } from '../src/services/secret-backend.service.js'; +import type { ISecretBackendRepository } from '../src/repositories/secret-backend.repository.js'; +import type { SecretBackendDriver } from '../src/services/secret-backends/types.js'; + +let app: FastifyInstance; +afterEach(async () => { await app?.close(); }); + +function backendRow(overrides: Partial = {}): SecretBackend { + return { + id: 'b1', name: 'bao-k8s', type: 'openbao', + config: { url: 'http://bao.example:8200', auth: 'kubernetes', role: 'mcpctl' }, + isDefault: true, description: '', version: 1, + createdAt: new Date(), updatedAt: new Date(), + ...overrides, + } as SecretBackend; +} + +/** + * Build the route over a service whose driver is stubbed. We override + * `driverFor` rather than the factory so the test drives the two probes + * directly — the point here is the route's reporting, not driver internals. + */ +async function buildApp( + probes: Pick, + row: SecretBackend = backendRow(), +): Promise { + const repo = { + findById: vi.fn(async (id: string) => (id === row.id ? row : null)), + } as unknown as ISecretBackendRepository; + const svc = new SecretBackendService(repo, { + plaintext: { listAllPlaintext: async () => [] }, + secretRefResolver: { resolve: async () => 'tok' }, + }); + vi.spyOn(svc, 'driverFor').mockReturnValue({ kind: 'openbao', ...probes } as SecretBackendDriver); + vi.spyOn(svc, 'cacheStatsFor').mockReturnValue({ entries: 3, servingStale: 0, oldestStaleSince: undefined }); + + const a = Fastify(); + registerSecretBackendHealthRoutes(a, svc); + await a.ready(); + return a; +} + +describe('GET /api/v1/secretbackends/:id/health', () => { + it('reports live+ready when the backend is fully working', async () => { + app = await buildApp({ + healthCheck: async () => ({ ok: true, detail: 'active' }), + authCheck: async () => ({ ok: true, detail: 'readable at secret/mcpctl' }), + }); + const res = await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' }); + expect(res.statusCode).toBe(200); + expect(res.json()).toMatchObject({ backend: 'bao-k8s', live: true, ready: true }); + }); + + it('reports NOT live when OpenBao is sealed — regardless of rotation state', async () => { + // The bug this endpoint exists for: a kubernetes-auth backend never writes + // tokenMeta.lastRotationError, so the old status line stayed green here. + app = await buildApp({ + healthCheck: async () => ({ ok: false, detail: 'sealed' }), + authCheck: async () => ({ ok: true, detail: 'should not be consulted' }), + }); + const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json(); + expect(body.live).toBe(false); + expect(body.liveDetail).toBe('sealed'); + expect(body.ready).toBe(false); + expect(body.readyDetail).toMatch(/not probed/); + expect(body.rotation.lastRotationError).toBeNull(); + }); + + it('distinguishes reachable-but-unusable (revoked grants) from unreachable', async () => { + app = await buildApp({ + healthCheck: async () => ({ ok: true, detail: 'active' }), + authCheck: async () => ({ ok: false, detail: 'OpenBao list: HTTP 403 permission denied' }), + }); + const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json(); + expect(body).toMatchObject({ live: true, ready: false }); + expect(body.readyDetail).toMatch(/403/); + }); + + it('surfaces cache state so degraded serving is visible', async () => { + app = await buildApp({ + healthCheck: async () => ({ ok: true }), + authCheck: async () => ({ ok: true }), + }); + const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json(); + expect(body.cache).toMatchObject({ entries: 3, servingStale: 0 }); + }); + + it('404s for an unknown backend', async () => { + app = await buildApp({ healthCheck: async () => ({ ok: true }), authCheck: async () => ({ ok: true }) }); + const res = await app.inject({ method: 'GET', url: '/api/v1/secretbackends/nope/health' }); + expect(res.statusCode).toBe(404); + }); +}); diff --git a/src/mcpd/tests/secret-backend-rotator-loop.test.ts b/src/mcpd/tests/secret-backend-rotator-loop.test.ts new file mode 100644 index 0000000..197ff47 --- /dev/null +++ b/src/mcpd/tests/secret-backend-rotator-loop.test.ts @@ -0,0 +1,155 @@ +/** + * SecretBackendRotatorLoop had no coverage at all, despite being the boot-time + * detector added after an upstream OpenBao re-init silently broke every secret + * write for four days (e51b924). These pin the behaviours that matter when that + * recurs: the boot health check fires, it reports through the injected logger + * (so `mcpctl errors` sees it), and stop() genuinely stops. + */ +import { describe, it, expect, vi } from 'vitest'; +import type { SecretBackend } from '@prisma/client'; +import { SecretBackendRotatorLoop } from '../src/services/secret-backend-rotator-loop.js'; +import type { SecretBackendService } from '../src/services/secret-backend.service.js'; +import type { SecretBackendRotator } from '../src/services/secret-backend-rotator.service.js'; + +function backend(overrides: Partial = {}): SecretBackend { + return { + id: 'b1', name: 'bao', type: 'openbao', + config: { url: 'http://bao.example:8200', rotation: { enabled: true, tokenRole: 'r', intervalHours: 24 } }, + isDefault: true, description: '', version: 1, + createdAt: new Date(), updatedAt: new Date(), + ...overrides, + } as SecretBackend; +} + +interface Harness { + loop: SecretBackendRotatorLoop; + rotator: { isRotatable: ReturnType; isOverdue: ReturnType; healthCheck: ReturnType; rotateOne: ReturnType }; + logs: { info: string[]; warn: string[]; error: Array<{ obj: Record; msg: string }> }; + timers: Array<{ cb: () => void; ms: number }>; + cleared: number; +} + +function harness(opts: { + rows?: SecretBackend[]; + rotatable?: boolean; + overdue?: boolean; + health?: { ok: boolean; message?: string } | Error; +} = {}): Harness { + const rows = opts.rows ?? [backend()]; + const logs: Harness['logs'] = { info: [], warn: [], error: [] }; + const timers: Harness['timers'] = []; + const state = { cleared: 0 }; + + const rotator = { + isRotatable: vi.fn(() => opts.rotatable ?? true), + isOverdue: vi.fn(() => opts.overdue ?? false), + healthCheck: vi.fn(async () => { + if (opts.health instanceof Error) throw opts.health; + return opts.health ?? { ok: true }; + }), + rotateOne: vi.fn(async () => ({})), + }; + + const loop = new SecretBackendRotatorLoop({ + backends: { + list: async () => rows, + getById: async (id: string) => rows.find((r) => r.id === id) ?? rows[0]!, + } as unknown as SecretBackendService, + rotator: rotator as unknown as SecretBackendRotator, + setTimeout: ((cb: () => void, ms: number) => { timers.push({ cb, ms }); return { id: timers.length } as unknown as NodeJS.Timeout; }), + clearTimeout: (() => { state.cleared++; }), + log: { + info: (m) => { logs.info.push(m); }, + warn: (m) => { logs.warn.push(m); }, + error: (obj, msg) => { logs.error.push({ obj, msg }); }, + }, + }); + + return { loop, rotator, logs, timers, get cleared() { return state.cleared; } } as Harness; +} + +/** The boot health check is fire-and-forget; let its microtasks settle. */ +const settle = async (): Promise => { await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); }; + +describe('SecretBackendRotatorLoop', () => { + it('stays idle when nothing is rotatable', async () => { + const h = harness({ rotatable: false }); + await h.loop.start(); + expect(h.logs.info.join(' ')).toMatch(/no rotatable backends/); + expect(h.timers).toHaveLength(0); + expect(h.rotator.healthCheck).not.toHaveBeenCalled(); + }); + + it('runs a boot-time health check for every rotatable backend', async () => { + const h = harness({ rows: [backend(), backend({ id: 'b2', name: 'bao2' })] }); + await h.loop.start(); + await settle(); + expect(h.rotator.healthCheck).toHaveBeenCalledTimes(2); + }); + + it('emits BACKEND_TOKEN_DEAD through the logger, not console', async () => { + // The regression that made `mcpctl errors` blind to it: this used to be a + // bare console.error, which bypasses the pino stream feeding ErrorLogBuffer. + const h = harness({ health: { ok: false, message: 'token rejected' } }); + await h.loop.start(); + await settle(); + expect(h.logs.error).toHaveLength(1); + expect(h.logs.error[0]?.obj).toMatchObject({ kind: 'BACKEND_TOKEN_DEAD', backend: 'bao' }); + expect(h.logs.error[0]?.msg).toBe('token rejected'); + }); + + it('does not log a fatal when the backend is healthy', async () => { + const h = harness({ health: { ok: true } }); + await h.loop.start(); + await settle(); + expect(h.logs.error).toHaveLength(0); + }); + + it('survives a health check that throws', async () => { + const h = harness({ health: new Error('network down') }); + await expect(h.loop.start()).resolves.toBeUndefined(); + await settle(); + expect(h.logs.warn.join(' ')).toMatch(/health check threw: network down/); + }); + + it('rotates immediately when a backend is overdue, and still schedules', async () => { + const h = harness({ overdue: true }); + await h.loop.start(); + await settle(); + expect(h.rotator.rotateOne).toHaveBeenCalledWith('b1'); + expect(h.timers).toHaveLength(1); + }); + + it('does not rotate on boot when not overdue', async () => { + const h = harness({ overdue: false }); + await h.loop.start(); + await settle(); + expect(h.rotator.rotateOne).not.toHaveBeenCalled(); + expect(h.timers).toHaveLength(1); + }); + + it('never schedules sooner than the 60s floor, even with adversarial jitter', async () => { + // intervalHours tiny + default jitter would otherwise produce a negative delay. + const rows = [backend({ config: { url: 'u', rotation: { enabled: true, tokenRole: 'r', intervalHours: 0.0001 } } } as Partial)]; + for (let i = 0; i < 50; i++) { + const h = harness({ rows }); + await h.loop.start(); + expect(h.timers[0]?.ms).toBeGreaterThanOrEqual(60_000); + } + }); + + it('stop() clears timers and suppresses further scheduling', async () => { + const h = harness(); + await h.loop.start(); + expect(h.timers).toHaveLength(1); + + h.loop.stop(); + expect(h.cleared).toBeGreaterThan(0); + + // The `stopped` guard has never been exercised: a firing timer must not + // reschedule after stop(). + const before = h.timers.length; + await h.loop.rotateNow('b1').catch(() => undefined); + expect(h.timers).toHaveLength(before); + }); +}); diff --git a/src/mcpd/tests/secret-backends.test.ts b/src/mcpd/tests/secret-backends.test.ts index 0f86075..43785a2 100644 --- a/src/mcpd/tests/secret-backends.test.ts +++ b/src/mcpd/tests/secret-backends.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import { PlaintextDriver } from '../src/services/secret-backends/plaintext.js'; import { OpenBaoDriver } from '../src/services/secret-backends/openbao.js'; +import { SecretNotFoundError, SecretBackendUnavailableError } from '../src/services/secret-backends/types.js'; describe('PlaintextDriver', () => { const driver = new PlaintextDriver({ listAllPlaintext: async () => [{ name: 'a', data: { k: 'v' } }] }); @@ -242,3 +243,91 @@ describe('OpenBaoDriver', () => { }); }); }); + +describe('OpenBaoDriver: resilience', () => { + const resolver = { resolve: vi.fn(async () => 'test-vault-token') }; + /** No real sleeping — otherwise the backoff tests take seconds. */ + const noSleep = async (): Promise => undefined; + + function driverWith(fetchFn: ReturnType, opts: Record = {}): OpenBaoDriver { + return new OpenBaoDriver( + { url: 'http://bao.example:8200', tokenSecretRef: { name: 'bao', key: 'token' } }, + { fetch: fetchFn as unknown as typeof fetch, secretRefResolver: resolver, sleep: noSleep, ...opts }, + ); + } + + it('maps a 404 read to SecretNotFoundError', async () => { + const fetchFn = vi.fn(async () => new Response('', { status: 404 })); + await expect(driverWith(fetchFn).read({ name: 'gone', externalRef: '', data: {} })) + .rejects.toThrow(SecretNotFoundError); + }); + + it('purges the token cache and retries once on 403 — outside the retry budget', async () => { + // This path existed but was never covered; it is the revocation/re-init case. + let n = 0; + const fetchFn = vi.fn(async () => { + n++; + if (n === 1) return new Response('', { status: 403 }); + return new Response(JSON.stringify({ data: { data: { token: 'ok' } } }), { status: 200 }); + }); + const d = driverWith(fetchFn); + await expect(d.read({ name: 's', externalRef: '', data: {} })).resolves.toEqual({ token: 'ok' }); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + it('retries a 503 (sealed) and succeeds', async () => { + let n = 0; + const fetchFn = vi.fn(async () => { + n++; + if (n < 3) return new Response('', { status: 503 }); + return new Response(JSON.stringify({ data: { data: { token: 'ok' } } }), { status: 200 }); + }); + await expect(driverWith(fetchFn).read({ name: 's', externalRef: '', data: {} })) + .resolves.toEqual({ token: 'ok' }); + expect(fetchFn).toHaveBeenCalledTimes(3); + }); + + it('throws SecretBackendUnavailableError once the retry budget is exhausted', async () => { + const fetchFn = vi.fn(async () => new Response('', { status: 503 })); + await expect(driverWith(fetchFn, { maxAttempts: 3 }).read({ name: 's', externalRef: '', data: {} })) + .rejects.toThrow(SecretBackendUnavailableError); + expect(fetchFn).toHaveBeenCalledTimes(3); + }); + + it('classifies a network/abort failure as SecretBackendUnavailableError', async () => { + const fetchFn = vi.fn(async () => { throw new DOMException('timed out', 'TimeoutError'); }); + await expect(driverWith(fetchFn, { maxAttempts: 2 }).read({ name: 's', externalRef: '', data: {} })) + .rejects.toThrow(SecretBackendUnavailableError); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + it('passes an AbortSignal on every request', async () => { + const fetchFn = vi.fn(async () => new Response(JSON.stringify({ data: { data: {} } }), { status: 200 })); + await driverWith(fetchFn, { timeoutMs: 1234 }).read({ name: 's', externalRef: '', data: {} }); + const [, init] = fetchFn.mock.calls[0] as [unknown, RequestInit]; + expect(init.signal).toBeInstanceOf(AbortSignal); + }); + + it('healthCheck is unauthenticated and maps OpenBao status codes', async () => { + const cases: Array<[number, boolean, string]> = [ + [200, true, 'active'], + [429, true, 'standby'], + [501, false, 'not initialized'], + [503, false, 'sealed'], + ]; + for (const [status, ok, detail] of cases) { + const fetchFn = vi.fn(async () => new Response('', { status })); + const result = await driverWith(fetchFn).healthCheck(); + expect(result).toEqual({ ok, detail }); + // The whole point of the split: no token is minted for a liveness probe. + const [, init] = fetchFn.mock.calls[0] as [unknown, RequestInit]; + expect((init.headers as Record)['X-Vault-Token']).toBeUndefined(); + } + }); + + it('authCheck reports false when the token can no longer list', async () => { + const fetchFn = vi.fn(async () => new Response('', { status: 403 })); + const result = await driverWith(fetchFn).authCheck(); + expect(result.ok).toBe(false); + }); +}); diff --git a/src/mcpd/tests/secret-cache.test.ts b/src/mcpd/tests/secret-cache.test.ts new file mode 100644 index 0000000..fbbf047 --- /dev/null +++ b/src/mcpd/tests/secret-cache.test.ts @@ -0,0 +1,198 @@ +import { describe, it, expect, vi } from 'vitest'; +import { + CachingSecretBackendDriver, + type CachingDriverLog, +} from '../src/services/secret-backends/caching.js'; +import { + SecretNotFoundError, + SecretBackendUnavailableError, + type SecretBackendDriver, + type SecretData, +} from '../src/services/secret-backends/types.js'; + +/** Minimal fake backing driver whose read() behaviour the tests drive. */ +function makeInner(overrides: Partial = {}): SecretBackendDriver & { + read: ReturnType; + write: ReturnType; + delete: ReturnType; +} { + return { + kind: 'fake', + read: vi.fn(async () => ({ token: 'live' } as SecretData)), + write: vi.fn(async () => ({ externalRef: 'ref', storedData: {} as SecretData })), + delete: vi.fn(async () => undefined), + list: vi.fn(async () => []), + ...overrides, + } as never; +} + +function makeLog(): CachingDriverLog & { warns: Array>; infos: Array> } { + const warns: Array> = []; + const infos: Array> = []; + return { warns, infos, warn: (o) => { warns.push(o); }, info: (o) => { infos.push(o); } }; +} + +const REQ = { name: 'gitea-creds', externalRef: 'secret/mcpctl/gitea-creds', data: {} }; + +describe('CachingSecretBackendDriver', () => { + it('serves from cache within the TTL without touching the backend', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 5_000, now: () => now }); + + expect(await d.read(REQ)).toEqual({ token: 'live' }); + now += 4_999; + expect(await d.read(REQ)).toEqual({ token: 'live' }); + + expect(inner.read).toHaveBeenCalledTimes(1); + }); + + it('refetches once the TTL has elapsed', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 5_000, now: () => now }); + + await d.read(REQ); + now += 5_001; + await d.read(REQ); + + expect(inner.read).toHaveBeenCalledTimes(2); + }); + + it('serves the stale value when the backend is unavailable', async () => { + const inner = makeInner(); + let now = 1_000; + const log = makeLog(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log, backendName: 'bao' }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + now += 10_000; + + // This is the whole point: no throw, so instance.service never marks ERROR. + expect(await d.read(REQ)).toEqual({ token: 'live' }); + expect(log.warns[0]?.kind).toBe('BACKEND_UNREACHABLE'); + }); + + it('logs BACKEND_UNREACHABLE only on the transition, not on every stale read', async () => { + const inner = makeInner(); + let now = 1_000; + const log = makeLog(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + for (let i = 0; i < 5; i++) { now += 2_000; await d.read(REQ); } + + expect(log.warns.filter((w) => w.kind === 'BACKEND_UNREACHABLE')).toHaveLength(1); + }); + + it('logs BACKEND_RECOVERED once the backend answers again', async () => { + const inner = makeInner(); + let now = 1_000; + const log = makeLog(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + now += 2_000; + await d.read(REQ); + + inner.read.mockResolvedValue({ token: 'rotated' }); + now += 2_000; + expect(await d.read(REQ)).toEqual({ token: 'rotated' }); + expect(log.infos.filter((i) => i.kind === 'BACKEND_RECOVERED')).toHaveLength(1); + }); + + it('NEVER serves stale for a deleted secret — evicts and rethrows', async () => { + // Regression guard. Serving stale here would resurrect a revoked + // credential, which is strictly worse than an outage. + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretNotFoundError('gone')); + now += 2_000; + + await expect(d.read(REQ)).rejects.toThrow(SecretNotFoundError); + expect(d.stats().entries).toBe(0); + + // And the entry really is gone — a later unavailable error has nothing to serve. + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + await expect(d.read(REQ)).rejects.toThrow(SecretBackendUnavailableError); + }); + + it('does not serve stale for a non-transport error (e.g. revoked grants)', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now }); + + await d.read(REQ); + inner.read.mockRejectedValue(new Error('OpenBao read: HTTP 403 permission denied')); + now += 2_000; + + await expect(d.read(REQ)).rejects.toThrow(/403/); + }); + + it('rethrows on a cold cache even when the backend is unavailable', async () => { + const inner = makeInner({ read: vi.fn(async () => { throw new SecretBackendUnavailableError('bao down'); }) as never }); + const d = new CachingSecretBackendDriver(inner); + + await expect(d.read(REQ)).rejects.toThrow(SecretBackendUnavailableError); + }); + + it('write() refreshes the cache so a read-after-write does not lag', async () => { + const inner = makeInner(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000 }); + + await d.read(REQ); + await d.write({ name: REQ.name, data: { token: 'brand-new' } }); + + expect(await d.read(REQ)).toEqual({ token: 'brand-new' }); + expect(inner.read).toHaveBeenCalledTimes(1); + }); + + it('delete() evicts', async () => { + const inner = makeInner(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000 }); + + await d.read(REQ); + await d.delete({ name: REQ.name, externalRef: REQ.externalRef }); + + expect(d.stats().entries).toBe(0); + }); + + it('bounds the map with an LRU eviction', async () => { + const inner = makeInner(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000, maxEntries: 2 }); + + await d.read({ ...REQ, name: 'a' }); + await d.read({ ...REQ, name: 'b' }); + await d.read({ ...REQ, name: 'a' }); // 'a' becomes most-recently-used + await d.read({ ...REQ, name: 'c' }); // evicts 'b' + + expect(d.stats().entries).toBe(2); + inner.read.mockClear(); + await d.read({ ...REQ, name: 'a' }); + expect(inner.read).not.toHaveBeenCalled(); // 'a' survived + await d.read({ ...REQ, name: 'b' }); + expect(inner.read).toHaveBeenCalledTimes(1); // 'b' was evicted + }); + + it('reports stale count and age via stats()', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now }); + + await d.read({ ...REQ, name: 'a' }); + await d.read({ ...REQ, name: 'b' }); + expect(d.stats()).toMatchObject({ entries: 2, servingStale: 0 }); + + inner.read.mockRejectedValue(new SecretBackendUnavailableError('down')); + now += 2_000; + await d.read({ ...REQ, name: 'a' }); + + expect(d.stats()).toMatchObject({ entries: 2, servingStale: 1, oldestStaleSince: 3_000 }); + }); +}); diff --git a/src/mcpd/tests/warm-secret-cache.test.ts b/src/mcpd/tests/warm-secret-cache.test.ts new file mode 100644 index 0000000..dfc6a95 --- /dev/null +++ b/src/mcpd/tests/warm-secret-cache.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect, vi } from 'vitest'; +import { warmSecretCache } from '../src/bootstrap/warm-secret-cache.js'; +import type { PrismaClient } from '@prisma/client'; +import type { SecretService } from '../src/services/secret.service.js'; + +function prismaWith(servers: Array<{ name: string; env: unknown }>): PrismaClient { + return { mcpServer: { findMany: vi.fn(async () => servers) } } as unknown as PrismaClient; +} +const noLog = { info: (): void => undefined, warn: (): void => undefined }; + +const envRef = (name: string, secret: string, key: string): unknown => + ({ name, valueFrom: { secretRef: { name: secret, key } } }); + +describe('warmSecretCache', () => { + it('resolves every distinct secret ref exactly once', async () => { + const prisma = prismaWith([ + { name: 'gitea', env: [envRef('GITEA_ACCESS_TOKEN', 'gitea-creds', 'GITEA_ACCESS_TOKEN')] }, + // Two servers sharing one secret must not cost two reads. + { name: 'a', env: [envRef('T', 'shared', 'TOKEN')] }, + { name: 'b', env: [envRef('T', 'shared', 'TOKEN')] }, + ]); + const resolve = vi.fn(async () => 'value'); + const result = await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog); + + expect(resolve).toHaveBeenCalledTimes(2); + expect(result).toEqual({ warmed: 2, failed: 0 }); + }); + + it('ignores inline env values', async () => { + const prisma = prismaWith([{ name: 's', env: [{ name: 'PLAIN', value: 'x' }] }]); + const resolve = vi.fn(async () => 'v'); + expect(await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog)) + .toEqual({ warmed: 0, failed: 0 }); + expect(resolve).not.toHaveBeenCalled(); + }); + + it('never throws when the backend is down — startup must not block', async () => { + const prisma = prismaWith([ + { name: 'a', env: [envRef('T', 's1', 'K')] }, + { name: 'b', env: [envRef('T', 's2', 'K')] }, + ]); + const resolve = vi.fn(async () => { throw new Error('bao unreachable'); }); + await expect(warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog)) + .resolves.toEqual({ warmed: 0, failed: 2 }); + }); + + it('keeps going after one bad reference', async () => { + const prisma = prismaWith([ + { name: 'a', env: [envRef('T', 'missing', 'K')] }, + { name: 'b', env: [envRef('T', 'present', 'K')] }, + ]); + const resolve = vi.fn(async (n: string) => { + if (n === 'missing') throw new Error('no such secret'); + return 'v'; + }); + expect(await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog)) + .toEqual({ warmed: 1, failed: 1 }); + }); + + it('only considers servers with replicas > 0', async () => { + const prisma = prismaWith([]); + await warmSecretCache(prisma, { resolve: vi.fn() } as unknown as SecretService, noLog); + const findMany = (prisma.mcpServer.findMany as unknown as ReturnType); + expect(findMany.mock.calls[0]?.[0]).toMatchObject({ where: { replicas: { gt: 0 } } }); + }); +}); diff --git a/src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts b/src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts new file mode 100644 index 0000000..358bf84 --- /dev/null +++ b/src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts @@ -0,0 +1,133 @@ +/** + * Smoke tests: secret-backend health honesty + value caching, against live mcpd. + * + * Covers the two behaviours that unit tests cannot prove, because both are + * about what the REAL backend and the REAL CLI do together: + * + * 1. `mcpctl status` reports a probed verdict, not a hard-coded tick. The bug + * being guarded is that the verdict used to come from + * `tokenMeta.lastRotationError`, which a `kubernetes`-auth backend never + * writes — so the line was structurally incapable of going red. + * 2. The value cache does not corrupt reads, and a delete really evicts. + * + * Deliberately does NOT take the real backend down. Simulating an outage + * against shared infrastructure to satisfy a test would be worse than the bug. + * + * Target: mcpd direct (`--direct`), same skip-if-unreachable discipline as the + * other smokes here. + * + * Run with: pnpm test:smoke + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import http from 'node:http'; +import https from 'node:https'; +import { execSync } from 'node:child_process'; + +const MCPD_URL = process.env.MCPD_URL ?? 'https://mcpctl.ad.itaz.eu'; +const SECRET_NAME = `smoke-cache-${Date.now().toString(36)}`; + +interface CliResult { code: number; stdout: string; stderr: string } + +function run(args: string): CliResult { + try { + return { code: 0, stdout: execSync(`mcpctl --direct ${args}`, { encoding: 'utf-8', timeout: 30_000, stdio: ['ignore', 'pipe', 'pipe'] }).trim(), stderr: '' }; + } catch (err) { + const e = err as { status?: number; stdout?: Buffer | string; stderr?: Buffer | string }; + return { + code: e.status ?? 1, + stdout: e.stdout ? String(e.stdout) : '', + stderr: e.stderr ? String(e.stderr) : '', + }; + } +} + +function healthz(url: string, timeoutMs = 5000): Promise { + return new Promise((resolve) => { + const parsed = new URL(`${url.replace(/\/$/, '')}/healthz`); + const driver = parsed.protocol === 'https:' ? https : http; + const req = driver.get( + { hostname: parsed.hostname, port: parsed.port || (parsed.protocol === 'https:' ? 443 : 80), path: parsed.pathname, timeout: timeoutMs }, + (res) => { resolve((res.statusCode ?? 500) < 500); res.resume(); }, + ); + req.on('error', () => resolve(false)); + req.on('timeout', () => { req.destroy(); resolve(false); }); + }); +} + +let mcpdUp = false; + +describe('secret resilience smoke', () => { + beforeAll(async () => { + mcpdUp = await healthz(MCPD_URL); + if (!mcpdUp) { + // eslint-disable-next-line no-console + console.warn(`\n ○ secret resilience smoke: skipped — ${MCPD_URL}/healthz unreachable. Set MCPD_URL to override.\n`); + } + }, 20_000); + + afterAll(() => { + if (!mcpdUp) return; + run(`delete secret ${SECRET_NAME}`); + }); + + it('status reports a probed backend verdict, not an unconditional tick', () => { + if (!mcpdUp) return; + const result = run('status'); + expect(result.code, result.stderr).toBe(0); + const line = result.stdout.split('\n').find((l) => l.startsWith('Secrets:')); + expect(line, 'status must include a Secrets: line').toBeDefined(); + // The verdict must be one the live probe can produce. A bare "name ✓" with + // no qualifier is the OLD rendering and means the probe was not consulted. + expect(line).toMatch(/reachable|degraded|unreachable|auth failed|unknown/); + }); + + it('reports live and ready separately per backend in JSON output', () => { + if (!mcpdUp) return; + const result = run('status -o json'); + expect(result.code, result.stderr).toBe(0); + const parsed = JSON.parse(result.stdout) as { + secretBackends?: Array<{ name: string; healthy: boolean; live: boolean | null; ready: boolean | null }>; + }; + expect(parsed.secretBackends, 'JSON status must carry secretBackends').toBeDefined(); + for (const b of parsed.secretBackends ?? []) { + // Both signals present and independent — not one boolean copied twice. + expect(b, `backend ${b.name}`).toHaveProperty('live'); + expect(b, `backend ${b.name}`).toHaveProperty('ready'); + expect(b.healthy).toBe(b.live === true && b.ready === true); + } + }); + + it('caching does not corrupt repeated reads, and delete evicts', () => { + if (!mcpdUp) return; + const created = run(`create secret ${SECRET_NAME} --data TOKEN=cache-probe-value`); + expect(created.code, created.stderr).toBe(0); + + // Two reads back-to-back: the second is a cache hit. Both must agree. + const first = run(`describe secret ${SECRET_NAME} --show-values`); + const second = run(`describe secret ${SECRET_NAME} --show-values`); + expect(first.code, first.stderr).toBe(0); + expect(second.code, second.stderr).toBe(0); + expect(first.stdout).toContain('cache-probe-value'); + expect(second.stdout).toContain('cache-probe-value'); + + // Delete must evict — a cached value surviving a delete is exactly the + // "resurrected revoked credential" failure the cache guards against. + const deleted = run(`delete secret ${SECRET_NAME}`); + expect(deleted.code, deleted.stderr).toBe(0); + const after = run(`describe secret ${SECRET_NAME} --show-values`); + expect(after.code, 'reading a deleted secret must fail, not serve cache').not.toBe(0); + }); + + it('exposes the per-backend health endpoint used by status', () => { + if (!mcpdUp) return; + const backends = run('get secretbackends -o json'); + expect(backends.code, backends.stderr).toBe(0); + const rows = JSON.parse(backends.stdout) as Array<{ id: string; name: string }>; + expect(rows.length).toBeGreaterThan(0); + // describe must surface the same probe, for every backend type — the old + // Token health block was gated on tokenMeta.rotatable and so rendered + // nothing at all for kubernetes-auth backends. + const described = run(`describe secretbackend ${rows[0]?.name ?? ''}`); + expect(described.code, described.stderr).toBe(0); + }); +});