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/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); + }); +});