Compare commits

...

8 Commits

Author SHA1 Message Date
Michal
7b5136491f fix(secrets): list via GET ?list=true — the LIST verb dies at the proxy
Some checks failed
CI/CD / lint (pull_request) Successful in 1m19s
CI/CD / typecheck (pull_request) Successful in 2m44s
CI/CD / test (pull_request) Successful in 1m30s
CI/CD / build (pull_request) Successful in 2m23s
CI/CD / smoke (pull_request) Failing after 3m4s
CI/CD / publish (pull_request) Has been skipped
Found by the readiness probe added in the previous commit, on its first
run against production: `mcpctl status` reported

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

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

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

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

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

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

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
2026-08-20 22:54:42 +01:00
eb3e558a44 Merge pull request 'feat(secrets): survive OpenBao outages and report real backend health' (#115) from feat/openbao-resilience into main
Some checks failed
CI/CD / typecheck (push) Successful in 1m24s
CI/CD / lint (push) Successful in 2m31s
CI/CD / test (push) Successful in 1m32s
CI/CD / build (push) Successful in 2m28s
CI/CD / smoke (push) Failing after 3m6s
CI/CD / publish (push) Has been skipped
2026-08-20 21:37:57 +00:00
Michal
545e7745da test(secrets): cover the rotator loop's boot-time dead-token detection
Some checks failed
CI/CD / typecheck (pull_request) Successful in 1m25s
CI/CD / lint (pull_request) Successful in 2m34s
CI/CD / test (pull_request) Successful in 1m32s
CI/CD / smoke (pull_request) Failing after 3m3s
CI/CD / build (pull_request) Successful in 2m26s
CI/CD / publish (pull_request) Has been skipped
SecretBackendRotatorLoop had zero tests, despite being the detector added
in e51b924 specifically so a re-initialised OpenBao surfaces the moment
mcpd boots rather than 24h later when the scheduled rotation finally
fires. The class already injects setTimeout/clearTimeout and a logger, so
this needed no production change.

Nine cases, weighted to what actually breaks: the boot health check runs
per rotatable backend; a dead token emits kind BACKEND_TOKEN_DEAD through
the injected logger (the point of the earlier console.error removal — a
bare console call never reaches ErrorLogBuffer, so `mcpctl errors` could
not see it); a throwing health check does not abort start(); overdue
backends rotate immediately and still get scheduled; the 60s floor holds
across 50 adversarial-jitter draws; and stop() both clears timers and
trips the `stopped` guard against rescheduling, which had never been
exercised.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
2026-08-20 22:17:24 +01:00
Michal
fe9987cefd test(secrets): smoke-cover backend health honesty + cache correctness
Two of these four assertions currently FAIL against the live cluster, which
is the point: mcpd there still renders `Secrets: bao-k8s* ✓` from the
rotation field, and its JSON status carries no live/ready pair. Per the
project rule the fix is to deploy, not to relax the assertion.

The status check specifically rejects a bare "name ✓" with no qualifier —
that string is the old rendering and proves the probe was never consulted.

Deliberately does not take the real OpenBao down. Simulating an outage
against shared infrastructure to satisfy a test would be worse than the
bug it covers; the outage paths are unit-tested with an injected clock.

Docs: adds a Reliability section (request hardening, the stale-while-error
table, the cold-cache gap and why persisting values is not the answer,
live-vs-ready) and replaces the stale "Kubernetes ServiceAccount auth is
not shipped yet" note — it shipped in 5152066, and it is what the live
backend has used since June.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
2026-08-20 22:16:04 +01:00
Michal
0fbfc72d68 feat(secrets): report real backend health instead of a hard-coded tick
`mcpctl status` derived its Secrets verdict entirely from
`tokenMeta.lastRotationError`. The rotator writes that field only for
`auth: 'token'` backends (SecretBackendRotator.isRotatable), so a
`kubernetes`-auth backend never wrote it and the line rendered a green
tick unconditionally — including with OpenBao sealed, unreachable, or
answering 403 to every read. The one signal we had was structurally
incapable of going red for the backend we actually run.

New `GET /api/v1/secretbackends/:id/health` reports two signals, kept
separate on purpose:

  live   — reachable at all?          (unauthenticated sys/health)
  ready  — can we read through it?    (uses our credentials)

live-but-not-ready is the exact shape of a re-initialised OpenBao handing
back valid-looking tokens that grant nothing; collapsing both into one
boolean is what hid that for four days. Needs no RBAC mapping — it falls
through to the generic `secretbackends` resource, so a GET is
`view:secretbackends`.

`mcpctl status` now renders four states — reachable / degraded (serving N
cached secrets) / unreachable / auth failed — with rotation error demoted
to a trailing clause rather than the verdict. A failed probe renders
"? unknown", never green: not knowing is not health. JSON output carries
the same probe, so scripts stop being told every k8s-auth backend is fine.

Also adds a boot-time cache warm. The stale-while-error cache can only
absorb an outage for secrets it has already seen, so a cold mcpd during a
backend outage still fails; resolving each running server's refs once at
startup closes that for the common case. Best-effort and deliberately
partial — if the backend is also down at boot this is a no-op and
instances fail loudly, which is correct. Persisting last-known-good to
Postgres or disk would just be plaintext-at-rest again.

Tests: 15 new. The five status assertions were confirmed to fail against
the old rotation-only logic before being kept.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
2026-08-20 22:14:12 +01:00
Michal
bd3e1134c9 feat(secrets): survive OpenBao outages instead of cascading them
mcpd re-read the secret backend on every use — server env resolution, LLM
api keys, chat, git providers, code repos, webhooks — with no value cache,
no request timeout, and a retry that only fired on HTTP 403. A few seconds
of OpenBao unavailability therefore turned into minutes of degraded
service: instances that restarted during the blip failed env resolution,
got marked ERROR, and entered the 30s x5 then 5min backoff.

Three changes, in dependency order:

1. Typed errors. `SecretNotFoundError` (definitive) vs
   `SecretBackendUnavailableError` (transport). 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
   paper over revoked grants, which is how an upstream re-init once broke
   every secret write for four days (e51b924).

2. Driver resilience. Every request now carries an AbortSignal timeout
   (there was none, so an unreachable backend hung its caller) and retries
   5xx/429/network with full-jitter backoff — 503 is what a sealed
   OpenBao returns and used to be an immediate hard failure. The 403
   purge-and-retry stays single-shot and outside the retry budget: it is a
   credential refresh, not a backend-unavailable condition, and looping on
   it would hide a genuinely revoked grant.

   `healthCheck()` no longer routes through the authenticated path, so an
   expired role stops reporting as "OpenBao is down"; it maps OpenBao's
   status codes (sealed/standby/uninitialised) instead. New `authCheck()`
   covers the readiness half via list(), which exercises the capability we
   actually depend on — unlike lookup-self, which only proves the token
   exists.

3. CachingSecretBackendDriver. Fresh reads inside the TTL never touch the
   network; past it we always try the backend, and on a transport failure
   serve the last known-good value instead of throwing. That is what stops
   the ERROR storm. Deleted secrets evict and rethrow — serving those
   stale would resurrect a revoked credential, strictly worse than an
   outage — and non-transport errors rethrow untouched. plaintext is not
   wrapped: its read() is an identity function over the row handed in.

A cold cache during an outage still fails, loudly and by design (e6cd735).

Also routes BACKEND_TOKEN_DEAD / BACKEND_ROTATION_FAILED through pino
rather than bare console.error. They bypassed the multistream feeding
ErrorLogBuffer, so the one failure `mcpctl errors` exists to surface was
the one it never showed.

Tests: 20 new. The two load-bearing guards (never serve a deleted secret
stale; never serve stale for a non-transport error) were confirmed to fail
against deliberately broken code before being kept. The 403 purge-retry
path had no coverage at all until now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
2026-08-20 22:08:10 +01:00
Michal
5fb1154190 Merge remote-tracking branch 'origin/main' into fix/stdio-restart-recovery
Some checks failed
CI/CD / lint (push) Successful in 1m12s
CI/CD / test (push) Successful in 1m25s
CI/CD / typecheck (push) Successful in 3m2s
CI/CD / smoke (push) Failing after 1m55s
CI/CD / build (push) Successful in 4m51s
CI/CD / publish (push) Has been skipped
2026-08-15 23:32:46 +01:00
Michal
b022f322f0 fix(mcpd): recover k8s STDIO instances after in-place container restarts
A container restart (OOMKill, crash, transient npx failure) used to strand
the instance in ERROR forever while its pod sat 1/1 Running, because five
gaps lined up (#114):

- the exec/attach websocket died without ending the stdout PassThrough
  (client-node installs no onclose), so PersistentStdioClient — whose only
  death signal was stdout 'end' — kept believing it was connected and every
  request rode the 120s timeout into a dead pipe;
- the stdioClients cache is keyed by pod name, which survives a restart, so
  nothing ever evicted the corpse;
- syncStatus never re-inspected ERROR rows and never read restartCount, so
  neither the recovery nor the in-place restart was visible;
- its ERROR writes clobbered retry metadata, making the row instantly
  dueForRetry, and the retry recreated the pod under the SAME name — an
  uncaught 409 that looped ERROR against a healthy pod;
- the stuck row consumed the whole replica budget, blocking a fresh-id
  replacement.

The fix, layer by layer:
- ws 'close'/'error' now end stdout in both k8s interactive paths (and the
  docker interactive path mirrors its own one-shot handlers), funneling into
  an identity-guarded teardown in PersistentStdioClient that also listens
  for stream 'close'/'error' — a late event from a previous session cannot
  clobber a reconnected one;
- syncStatus re-inspects ERROR rows (pod running again → back to RUNNING
  with retry metadata cleared), tracks restartCount in instance metadata to
  catch restarts between polls, MERGES retry metadata instead of clobbering
  it, and evicts the cached stdio client through a setter-injected hook
  whenever the pipe is known-dead;
- createContainer adopts an alive pod on 409 instead of throwing (and only
  replaces a genuinely dead one, waiting out the deletion grace period);
- attach mode retries once through a fresh client before surfacing an error,
  so the first call after a detected death succeeds;
- the health probe evicts the cached client when failures cross the
  threshold, and shutdown finally calls closeAll().

Closes #114

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JaFvfHrQyUKCGv6o3N2Wir
2026-08-15 23:32:23 +01:00
34 changed files with 2953 additions and 128 deletions

View File

@@ -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/<authMount>/role/<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/<id>/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`

View File

@@ -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<ServerLlmHealth>;
/** Fetch SecretBackends from mcpd to surface backend health. Null on error. */
fetchSecretBackends: (mcpdUrl: string, token: string | null) => Promise<SecretBackendInfo[] | null>;
probeSecretBackend: (mcpdUrl: string, id: string, token: string | null) => Promise<SecretBackendHealth | null>;
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<SecretBackendHealth | null> {
return new Promise((resolve) => {
let req: http.ClientRequest;
const headers: Record<string, string> = { 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/<name>/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<StatusCommandDeps>): 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<StatusCommandDeps>): 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<StatusCommandDeps>): 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<StatusCommandDeps>): 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<StatusCommandDeps>): 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<StatusCommandDeps>): Command
async function renderSecretBackendsSection(
backendsPromise: Promise<SecretBackendInfo[] | null>,
ansi: boolean,
mcpdUrl: string,
token: string | null,
): Promise<void> {
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

View File

@@ -30,6 +30,7 @@ function baseDeps(overrides?: Partial<StatusCommandDeps>): Partial<StatusCommand
fetchServerLlms: async () => 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' });

View File

@@ -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<string, { name: string; key: string }>();
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 };
}

View File

@@ -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<void> {
},
},
secretRefResolver: secretResolverBridge,
}, {
// Cache-transition events go through pino so BACKEND_UNREACHABLE /
// BACKEND_RECOVERED land in ErrorLogBuffer and `mcpctl errors`.
log: {
warn: (obj: Record<string, unknown>, msg: string): void => { app.log.warn(obj, msg); },
info: (obj: Record<string, unknown>, 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<string, unknown>, 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<string, unknown>, msg: string): void => { app.log.error(obj, msg); },
},
});
const llmAdapters = new LlmAdapterRegistry();
// LlmService takes the adapter registry so create()/update() can run an
@@ -511,6 +529,10 @@ async function main(): Promise<void> {
const authService = new AuthService(prisma);
const templateService = new TemplateService(templateRepo);
const mcpProxyService = new McpProxyService(instanceRepo, serverRepo, orchestrator);
// When syncStatus observes a container restart/replacement, the cached
// STDIO pipe under that containerId is dead — evict it so the next call
// redials (mcpctl#114). Setter injection, same as setInstanceService above.
instanceService.setStdioInvalidator((cid) => mcpProxyService.removeClient(cid));
const rbacDefinitionService = new RbacDefinitionService(rbacDefinitionRepo);
const rbacService = new RbacService(rbacDefinitionRepo, prisma);
const mcpTokenService = new McpTokenService(mcpTokenRepo, projectRepo, rbacDefinitionRepo, rbacService);
@@ -673,6 +695,7 @@ async function main(): Promise<void> {
registerSecretRoutes(app, secretService);
registerSecretBackendRoutes(app, secretBackendService);
registerSecretBackendRotateRoutes(app, secretBackendRotator);
registerSecretBackendHealthRoutes(app, secretBackendService);
registerSecretMigrateRoutes(app, secretMigrateService);
registerLlmRoutes(app, llmService);
registerAgentRoutes(app, agentService);
@@ -960,6 +983,18 @@ async function main(): Promise<void> {
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 () => {
@@ -969,6 +1004,7 @@ async function main(): Promise<void> {
healthProbeRunner.stop();
secretBackendRotatorLoop.stop();
gitBackup.stop();
mcpProxyService.closeAll();
await prisma.$disconnect();
},
});

View File

@@ -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) };
}
});
}

View File

@@ -265,6 +265,16 @@ export class DockerContainerManager implements McpOrchestrator {
const stderr = new PassThrough();
this.docker.modem.demuxStream(stream, stdout, stderr);
// demuxStream never propagates end/close/error to the demuxed streams, so
// a dead container left consumers waiting on a silent pipe — same funnel
// as the one-shot execInContainer path uses (mcpctl#114).
const endStdout = () => {
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
};
stream.on('end', endStdout);
stream.on('close', endStdout);
stream.on('error', endStdout);
return {
stdout,
write(data: string) {

View File

@@ -193,6 +193,13 @@ export class HealthProbeRunner {
? 'unhealthy'
: 'degraded';
// Crossing the threshold means every probe rode the same cached STDIO
// pipe — evict it once so the next probe/call redials instead of failing
// forever against a connection the other side already dropped.
if (!result.healthy && state.consecutiveFailures === failureThreshold && instance.containerId) {
this.mcpProxyService?.removeClient(instance.containerId);
}
// Build event
const probeLabel = probeKind === 'readiness'
? `Readiness check (${healthCheck.tool})`

View File

@@ -32,6 +32,9 @@ interface RetryMetadata {
attemptCount?: number;
lastAttemptAt?: string;
nextRetryAt?: string;
/** containerStatuses[0].restartCount at last sync — a bump means every
* cached STDIO pipe to this (unchanged) containerId is dead. */
lastRestartCount?: number;
[k: string]: unknown;
}
@@ -52,6 +55,8 @@ export class InvalidStateError extends Error {
}
export class InstanceService {
private stdioInvalidator?: (containerId: string) => void;
constructor(
private instanceRepo: IMcpInstanceRepository,
private serverRepo: IMcpServerRepository,
@@ -59,6 +64,25 @@ export class InstanceService {
private secretResolver?: SecretResolver,
) {}
/**
* Hook for evicting cached STDIO clients (McpProxyService.removeClient).
* Setter injection, matching serverService.setInstanceService in main.ts —
* McpProxyService is constructed after this service and already imports
* from this file, so a constructor arg would be a circular import.
*/
setStdioInvalidator(fn: (containerId: string) => void): void {
this.stdioInvalidator = fn;
}
private invalidateStdio(containerId: string | null | undefined): void {
if (!containerId) return;
try {
this.stdioInvalidator?.(containerId);
} catch {
/* best-effort */
}
}
async list(serverId?: string): Promise<McpInstance[]> {
return this.instanceRepo.findAll(serverId);
}
@@ -71,37 +95,96 @@ export class InstanceService {
/**
* Sync instance statuses with actual container state.
* Detects crashed/stopped containers and marks them ERROR.
*
* Beyond marking crashed containers ERROR, this is the recovery path for
* mcpctl#114: ERROR rows whose pod came back are re-adopted (the pod has
* restartPolicy Always, so kubelet restarts the container in place and the
* row must follow it back), and an in-place restart under an unchanged
* containerId — visible only as a restartCount bump — invalidates any
* cached STDIO pipe, which is dead by definition.
*
* Every metadata write here MERGES via readRetryMeta: updateStatus replaces
* the JSON column wholesale, and clobbering nextRetryAt is what used to
* make ERROR rows instantly dueForRetry and hot-loop against a 409.
*/
async syncStatus(): Promise<void> {
const instances = await this.instanceRepo.findAll();
for (const inst of instances) {
if ((inst.status === 'RUNNING' || inst.status === 'STARTING') && inst.containerId) {
try {
const info = await this.orchestrator.inspectContainer(inst.containerId);
if (!inst.containerId) continue;
if (inst.status !== 'RUNNING' && inst.status !== 'STARTING' && inst.status !== 'ERROR') {
continue;
}
if (info.state === 'stopped' || info.state === 'error') {
// Container died — get last logs for error context
let errorMsg = `Container ${info.state}`;
try {
const logs = await this.orchestrator.getContainerLogs(inst.containerId, { tail: 5 });
const lastLog = (logs.stdout || logs.stderr).trim().split('\n').pop();
if (lastLog) errorMsg = lastLog;
} catch { /* best-effort */ }
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
metadata: { error: errorMsg },
});
} else if (info.state === 'starting' && inst.status === 'RUNNING') {
// Pod went back to starting (e.g. CrashLoopBackOff restart)
await this.instanceRepo.updateStatus(inst.id, 'STARTING', {});
} else if (info.state === 'running' && inst.status === 'STARTING') {
// Pod became ready — promote to RUNNING
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {});
}
} catch {
// Container gone entirely
let info: ContainerInfo;
try {
info = await this.orchestrator.inspectContainer(inst.containerId);
} catch {
// Container gone entirely. ERROR rows with a missing pod stay as they
// are — the retry/backoff path owns recreating them.
if (inst.status !== 'ERROR') {
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
metadata: { error: 'Container not found' },
metadata: { ...readRetryMeta(inst), error: 'Container not found' },
});
this.invalidateStdio(inst.containerId);
}
continue;
}
const meta = readRetryMeta(inst);
if (inst.status === 'ERROR') {
// The pod outlived the ERROR verdict (kubelet restarted the container
// in place). Re-adopt instead of leaving the row stuck forever.
if (info.state === 'running') {
const { error: _e, attemptCount: _a, lastAttemptAt: _l, nextRetryAt: _n, ...rest } = meta;
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
metadata: { ...rest, lastRestartCount: info.restartCount ?? 0 },
});
this.invalidateStdio(inst.containerId);
} else if (info.state === 'starting') {
// Keep retry metadata until it is actually running.
await this.instanceRepo.updateStatus(inst.id, 'STARTING', { metadata: meta });
this.invalidateStdio(inst.containerId);
}
// stopped/error: leave for the backoff/retry path.
continue;
}
if (info.state === 'stopped' || info.state === 'error') {
// Container died — get last logs for error context
let errorMsg = `Container ${info.state}`;
try {
const logs = await this.orchestrator.getContainerLogs(inst.containerId, { tail: 5 });
const lastLog = (logs.stdout || logs.stderr).trim().split('\n').pop();
if (lastLog) errorMsg = lastLog;
} catch { /* best-effort */ }
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
metadata: { ...meta, error: errorMsg },
});
this.invalidateStdio(inst.containerId);
} else if (info.state === 'starting' && inst.status === 'RUNNING') {
// Pod went back to starting (e.g. CrashLoopBackOff restart)
await this.instanceRepo.updateStatus(inst.id, 'STARTING', { metadata: meta });
this.invalidateStdio(inst.containerId);
} else if (info.state === 'running' && inst.status === 'STARTING') {
// Pod became ready — promote to RUNNING and clear retry state.
const { error: _e, attemptCount: _a, lastAttemptAt: _l, nextRetryAt: _n, ...rest } = meta;
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
metadata: { ...rest, lastRestartCount: info.restartCount ?? 0 },
});
// A fresh/restarted pod under this name invalidates any cached pipe.
this.invalidateStdio(inst.containerId);
} else if (info.state === 'running' && info.restartCount !== undefined) {
if (typeof meta.lastRestartCount === 'number' && info.restartCount > meta.lastRestartCount) {
// In-place restart between polls: same pod name, dead pipes.
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
metadata: { ...meta, lastRestartCount: info.restartCount },
});
this.invalidateStdio(inst.containerId);
} else if (meta.lastRestartCount === undefined) {
// First sighting: record the baseline without invalidating.
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
metadata: { ...meta, lastRestartCount: info.restartCount },
});
}
}
@@ -232,6 +315,7 @@ export class InstanceService {
} catch {
// Container may already be gone
}
this.invalidateStdio(instance.containerId);
}
await this.instanceRepo.delete(id);
@@ -256,6 +340,7 @@ export class InstanceService {
} catch {
// best-effort
}
this.invalidateStdio(inst.containerId);
}
}
}
@@ -489,6 +574,7 @@ export class InstanceService {
try {
await this.orchestrator.removeContainer(instance.containerId, true);
} catch { /* best-effort */ }
this.invalidateStdio(instance.containerId);
}
await this.instanceRepo.delete(instance.id);
}

View File

@@ -81,6 +81,18 @@ function podToContainerInfo(pod: V1Pod): ContainerInfo {
info.port = ports[0].containerPort;
}
// Restart visibility: a container that crashes and restarts IN PLACE keeps
// the same pod name and reports state=running again — restartCount is the
// only durable evidence, and syncStatus uses it to invalidate stale STDIO
// pipes (mcpctl#114).
const cs = pod.status?.containerStatuses?.[0];
if (cs?.restartCount !== undefined) {
info.restartCount = cs.restartCount;
}
if (cs?.state?.running?.startedAt) {
info.startedAt = new Date(cs.state.running.startedAt as unknown as string);
}
return info;
}
@@ -116,10 +128,39 @@ export class KubernetesOrchestrator implements McpOrchestrator {
}
const manifest = generatePodSpec(spec, this.namespace);
const pod = await this.client.core.createNamespacedPod({
namespace: this.namespace,
body: manifest as V1Pod,
});
let pod;
try {
pod = await this.client.core.createNamespacedPod({
namespace: this.namespace,
body: manifest as V1Pod,
});
} catch (err) {
if (httpStatusOf(err) !== 409) throw err;
// AlreadyExists: a retry raced a pod that is still there. If it is
// alive, ADOPT it — recreating under the same name can never succeed
// and used to loop the instance in ERROR forever (mcpctl#114). Only a
// genuinely dead pod is replaced.
const podName = (manifest as V1Pod).metadata!.name!;
const existing = await this.inspectContainer(podName);
if (existing.state === 'running' || existing.state === 'starting') {
return existing;
}
await this.removeContainer(podName, true);
// deleteNamespacedPod returns before the pod is gone (grace period);
// recreating while it is Terminating would 409 again. Bounded wait.
for (let i = 0; i < 15; i++) {
await new Promise((resolve) => setTimeout(resolve, 1000));
try {
await this.inspectContainer(podName);
} catch {
break; // 404 — pod is gone
}
}
pod = await this.client.core.createNamespacedPod({
namespace: this.namespace,
body: manifest as V1Pod,
});
}
// Wait briefly for pod to start scheduling
await new Promise((resolve) => setTimeout(resolve, 500));
@@ -276,6 +317,17 @@ export class KubernetesOrchestrator implements McpOrchestrator {
// Wait for WebSocket connection to establish
const ws = await wsPromise;
// client-node's WebSocketHandler only ends stdout on an explicit
// CloseStream/Status frame from the apiserver. An abnormal socket death
// (container OOMKilled, node reboot) emits neither — without these
// handlers the PassThrough stays open and the consumer never learns the
// pipe is dead (mcpctl#114).
const endStdout = () => {
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
};
ws.on('close', endStdout);
ws.on('error', endStdout);
return {
stdout,
write(data: string) {
@@ -316,6 +368,13 @@ export class KubernetesOrchestrator implements McpOrchestrator {
false, // tty
);
// Same abnormal-death funnel as execInteractive — see comment there.
const endStdout = () => {
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
};
ws.on('close', endStdout);
ws.on('error', endStdout);
return {
stdout,
write(data: string) {

View File

@@ -180,20 +180,28 @@ export class McpProxyService {
} catch (err) {
this.removeClient(instance.containerId);
// Fall back to one-shot exec when we have a command to run.
// Attach mode has no equivalent one-shot fallback — surface the error.
if (mode.kind === 'exec') {
return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, runtime);
}
const detail = formatError(err);
console.error(`[mcp-proxy] attach to ${instance.containerId} failed:`, err);
return {
jsonrpc: '2.0',
id: 1,
error: {
code: -32000,
message: `STDIO attach to '${instance.containerId}' failed: ${detail}`,
},
};
// Attach mode has no one-shot equivalent, but the failure is usually
// a stale pipe from an in-place container restart — retry once
// through a fresh client (which redials) before surfacing the error,
// so the FIRST call after a detected death succeeds (mcpctl#114).
try {
return await this.sendViaPersistentStdio(instance.containerId, mode, method, params);
} catch (retryErr) {
this.removeClient(instance.containerId);
const detail = formatError(retryErr);
console.error(`[mcp-proxy] attach to ${instance.containerId} failed (after retry):`, retryErr);
return {
jsonrpc: '2.0',
id: 1,
error: {
code: -32000,
message: `STDIO attach to '${instance.containerId}' failed: ${detail}`,
},
};
}
}
}

View File

@@ -69,6 +69,14 @@ export interface ContainerInfo {
/** Container IP on the first non-default network (for internal communication) */
ip?: string;
createdAt: Date;
/**
* Times the container restarted in place (k8s containerStatuses[0]).
* A bump with an unchanged containerId means every cached STDIO pipe to it
* is dead — syncStatus uses this to invalidate them (mcpctl#114).
*/
restartCount?: number;
/** When the current container process started (k8s state.running.startedAt). */
startedAt?: Date;
}
export interface ContainerLogs {

View File

@@ -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<string, unknown>, msg: string) => void;
};
}
const DEFAULT_INTERVAL_MS = 24 * 3600 * 1000;
@@ -36,7 +40,7 @@ export class SecretBackendRotatorLoop {
private readonly timers = new Map<string, NodeJS.Timeout>();
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<SecretBackendRotatorLoopDeps['log']>;
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<string, unknown>, 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'}`);
}
})

View File

@@ -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<string, unknown>, 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<string, unknown>, 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)}`);
}
}
}

View File

@@ -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<SecretBackend[]> {
@@ -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;
}
}

View File

@@ -0,0 +1,215 @@
/**
* 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<string, unknown>, msg: string): void;
info(obj: Record<string, unknown>, 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<string, CacheEntry>();
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<SecretData> {
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<void> {
await this.inner.delete(input);
this.entries.delete(input.name);
}
async list(): Promise<Array<{ name: string; externalRef: ExternalRef }>> {
return this.inner.list();
}
async healthCheck(): Promise<{ ok: boolean; detail?: string }> {
return this.inner.healthCheck?.() ?? { ok: true, detail: 'no probe' };
}
/**
* Identity provisioning is a control-plane operation on the backend itself,
* not a value read — nothing to cache, so pass straight through. Throwing
* when unsupported (rather than silently succeeding) keeps a misconfigured
* backend from looking like it granted access it never did.
*/
async ensureServerIdentity(input: { name: string; namespace: string; secretNames: string[] }): Promise<void> {
if (this.inner.ensureServerIdentity === undefined) {
throw new Error(`backend '${this.backendName}' (${this.inner.kind}) does not support per-server identities`);
}
await this.inner.ensureServerIdentity(input);
}
async removeServerIdentity(input: { name: string }): Promise<void> {
await this.inner.removeServerIdentity?.(input);
}
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);
}
}
}

View File

@@ -7,7 +7,7 @@
* POST <url>/v1/<mount>/data/<path> -- write
* GET <url>/v1/<mount>/data/<path> -- read latest
* DELETE <url>/v1/<mount>/metadata/<path> -- full delete (all versions)
* LIST <url>/v1/<mount>/metadata/ -- for migration
* GET <url>/v1/<mount>/metadata/?list=true -- for migration (see list())
* POST <url>/v1/auth/<mount>/login -- kubernetes auth
*
* Auth strategies (`config.auth`):
@@ -28,6 +28,14 @@
*/
import { readFile } from 'node:fs/promises';
import type { SecretBackendDriver, SecretData, ExternalRef, SecretRefResolver } from './types.js';
import { SecretNotFoundError, SecretBackendUnavailableError } from './types.js';
import {
buildServerSecretPolicyHcl,
writePolicy,
deletePolicy,
ensureKubernetesAuthRole,
deleteKubernetesAuthRole,
} from '@mcpctl/shared';
/** Best-effort read of a response body for error messages. Empty on parse failure. */
async function bodyText(res: Response): Promise<string> {
@@ -77,10 +85,23 @@ export interface OpenBaoDriverDeps {
readServiceAccountToken?: (path: string) => Promise<string>;
/** 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<void>;
}
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 +119,10 @@ export class OpenBaoDriver implements SecretBackendDriver {
private readonly resolver: SecretRefResolver | undefined;
private readonly readSaToken: (path: string) => Promise<string>;
private readonly nowFn: () => number;
private readonly timeoutMs: number;
private readonly maxAttempts: number;
private readonly backoffBaseMs: number;
private readonly sleep: (ms: number) => Promise<void>;
// Cached vault token + when (epoch ms) it should be considered expired and refetched.
private cachedToken: string | undefined;
@@ -131,13 +156,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<void> => new Promise((r) => { setTimeout(r, ms); }));
}
async read(input: { name: string; externalRef: ExternalRef; data: SecretData }): Promise<SecretData> {
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 } };
@@ -161,7 +192,13 @@ export class OpenBaoDriver implements SecretBackendDriver {
async list(): Promise<Array<{ name: string; externalRef: ExternalRef }>> {
const listPath = this.pathPrefix === '' ? '' : `${this.pathPrefix}/`;
const res = await this.request('LIST', `/v1/${this.mount}/metadata/${listPath}`);
// `GET ?list=true`, not the LIST verb. OpenBao accepts both, but LIST is a
// non-standard HTTP method and proxies drop it: through Cilium's ingress
// Envoy (which is how `bao-k8s` is reached) LIST returns a bare
// `400 Bad Request` while the GET form returns normally. Verified live
// 2026-08-20 — this silently broke `mcpctl migrate secrets` against any
// ingress-fronted backend.
const res = await this.request('GET', `/v1/${this.mount}/metadata/${listPath}?list=true`);
if (res.status === 404) return [];
if (!res.ok) throw new Error(`OpenBao list: HTTP ${res.status} ${await bodyText(res)}`);
const body = await res.json() as { data?: { keys?: string[] } };
@@ -174,15 +211,104 @@ 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<string, string> = {};
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
* `<mount>/<pathPrefix>/` — 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) };
}
}
/**
* Create/refresh a per-server OpenBao identity: a policy naming exactly this
* server's secrets, and a Kubernetes auth role bound to exactly its
* ServiceAccount.
*
* Requires mcpd's own token to hold the provisioning grant (see
* `buildServerProvisioningPolicyHcl`) — deliberately confined to the
* `mcpctl-server-` name prefix, so this cannot mint an identity that reads
* anything mcpd itself could not already read.
*
* Only meaningful with `auth: 'kubernetes'`: the role has to live on the same
* auth mount that validates this cluster's ServiceAccount tokens.
*/
async ensureServerIdentity(input: { name: string; namespace: string; secretNames: string[] }): Promise<void> {
if (this.authStrategy !== 'kubernetes') {
throw new Error(
`OpenBao: per-server identities require auth: 'kubernetes' (this backend uses '${this.authStrategy}') — ` +
'a token-auth backend has no ServiceAccount auth mount to bind a role on',
);
}
const token = await this.getToken();
const deps = { fetch: this.fetchImpl, ...(this.namespace !== undefined ? { namespace: this.namespace } : {}) };
// Policy first, then the role that references it. The reverse order would
// briefly leave a role pointing at a non-existent policy, which OpenBao
// resolves as "no capabilities" — a confusing transient 403 for the pod.
await writePolicy(
this.url,
token,
input.name,
buildServerSecretPolicyHcl({ mount: this.mount, pathPrefix: this.pathPrefix, secretNames: input.secretNames }),
deps,
);
await ensureKubernetesAuthRole(this.url, token, this.k8sAuthMount, input.name, {
boundServiceAccountNames: [input.name],
boundServiceAccountNamespaces: [input.namespace],
tokenPolicies: [input.name],
}, deps);
}
/** Role before policy: revoke the ability to log in before the grants vanish. */
async removeServerIdentity(input: { name: string }): Promise<void> {
if (this.authStrategy !== 'kubernetes') return;
const token = await this.getToken();
const deps = { fetch: this.fetchImpl, ...(this.namespace !== undefined ? { namespace: this.namespace } : {}) };
await deleteKubernetesAuthRole(this.url, token, this.k8sAuthMount, input.name, deps);
await deletePolicy(this.url, token, input.name, deps);
}
private pathFor(name: string): string {
const safe = encodeURIComponent(name);
return this.pathPrefix === '' ? safe : `${this.pathPrefix}/${safe}`;
@@ -206,11 +332,28 @@ export class OpenBaoDriver implements SecretBackendDriver {
const loginUrl = `${this.url}/v1/auth/${this.k8sAuthMount}/login`;
const headers: Record<string, string> = { '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 +372,77 @@ export class OpenBaoDriver implements SecretBackendDriver {
return clientToken;
}
private async request(method: string, path: string, body?: unknown): Promise<Response> {
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<string, string> = { '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<string, string> = { '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<Response> {
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 } : {}),
);
}
}

View File

@@ -46,8 +46,49 @@ export interface SecretBackendDriver {
/** List everything the backend knows about. Used for migration + drift detection. */
list(): Promise<Array<{ name: string; externalRef: ExternalRef }>>;
/** 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: provision a per-server identity in the backend, so an MCP server
* pod can fetch its OWN secrets directly (Agent Injector) without mcpd ever
* materialising the value into the pod spec.
*
* Scoping is the entire point. Each server gets a policy naming only the
* secrets it declares, bound to its own ServiceAccount. A single shared role
* would let any opted-in pod — including third-party images we do not
* control — read every secret under the prefix, which is a worse position
* than leaving values in the pod spec.
*
* Idempotent: called on every reconcile, must converge rather than conflict.
*/
ensureServerIdentity?(input: {
/** Identity name — also the policy name and the k8s ServiceAccount name. */
name: string;
/** Namespace the bound ServiceAccount lives in. */
namespace: string;
/** Secrets this server may read. Empty means "revoke everything". */
secretNames: string[];
}): Promise<void>;
/** Optional: tear down what `ensureServerIdentity` created. Idempotent. */
removeServerIdentity?(input: { name: string }): Promise<void>;
/**
* 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 +107,40 @@ export interface BackendRow {
export interface SecretRefResolver {
resolve(secretName: string, key: string): Promise<string>;
}
/**
* 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;
}
}

View File

@@ -0,0 +1,142 @@
/**
* Per-server OpenBao identities, so an MCP server pod can fetch its own secrets
* without mcpd ever writing the value into the pod spec.
*
* ## Why per-server, and not one shared role
*
* The obvious implementation gives every injected pod one shared ServiceAccount
* and one role granting `secret/data/mcpctl/*`. That would be a downgrade, not
* an improvement: today's exposure is "4 credentials readable by anyone with
* `get pod` in this namespace", and the shared-role version is "10 credentials
* readable by any process inside a third-party MCP server image". We do not
* control `gitea-mcp-server` or `ha-mcp`; they should not be able to read the
* Grafana token.
*
* So each server gets:
* - its own Kubernetes ServiceAccount `mcpctl-server-<server>`
* - its own OpenBao policy `mcpctl-server-<server>` (names only
* the secrets that server declares — no wildcards)
* - its own OpenBao k8s auth role `mcpctl-server-<server>` (bound to
* that one ServiceAccount in that one namespace)
*
* ## Convergence
*
* `ensureFor` is idempotent and is safe to call on every start. The generated
* policy is sorted and deduped, so re-writing an unchanged server produces a
* byte-identical body — an unstable policy would churn the OpenBao audit log
* and make real changes invisible.
*
* Removing a secret from a server's env and restarting it narrows the policy on
* the next reconcile. Removing the server entirely revokes the identity.
*/
import type { McpServer } from '@prisma/client';
import type { SecretBackendService } from './secret-backend.service.js';
import type { ServerEnvEntry } from '../validation/mcp-server.schema.js';
/** Minimal Kubernetes surface this needs — keeps the service testable. */
export interface ServiceAccountPort {
/** Namespace MCP server pods run in. */
readonly namespace: string;
/** Create the ServiceAccount if absent. Must tolerate "already exists". */
ensure(name: string): Promise<void>;
/** Delete it. Must tolerate "not found". */
remove(name: string): Promise<void>;
}
export interface ServerIdentityLog {
info(obj: Record<string, unknown>, msg: string): void;
warn(obj: Record<string, unknown>, msg: string): void;
}
const NOOP_LOG: ServerIdentityLog = { info: () => undefined, warn: () => undefined };
/** Shared prefix. mcpd's OpenBao grant is confined to exactly this prefix. */
export const IDENTITY_PREFIX = 'mcpctl-server-';
export class ServerIdentityService {
private readonly log: ServerIdentityLog;
constructor(
private readonly backends: SecretBackendService,
private readonly serviceAccounts: ServiceAccountPort,
log?: ServerIdentityLog,
) {
this.log = log ?? NOOP_LOG;
}
/** Identity name for a server — also the SA, policy and role name. */
identityNameFor(serverName: string): string {
return `${IDENTITY_PREFIX}${serverName}`;
}
/**
* The distinct secrets a server declares. Inline `value` entries are not
* secrets and must not widen the policy.
*/
secretNamesFor(server: Pick<McpServer, 'env'>): string[] {
const entries = (server.env ?? []) as ServerEnvEntry[];
const names = new Set<string>();
for (const entry of entries) {
const ref = entry.valueFrom?.secretRef;
if (ref !== undefined) names.add(ref.name);
}
return [...names].sort((a, b) => a.localeCompare(b));
}
/**
* Converge the identity for one server. Returns the identity name so the
* caller can stamp it onto the pod spec.
*
* Order matters: the ServiceAccount must exist before the role that binds it,
* or the pod can start, fail to log in, and crashloop while the role is still
* being written.
*/
async ensureFor(server: Pick<McpServer, 'name' | 'env'>): Promise<string> {
const name = this.identityNameFor(server.name);
const secretNames = this.secretNamesFor(server);
const backend = await this.backends.getDefault();
const driver = this.backends.driverFor(backend);
if (driver.ensureServerIdentity === undefined) {
throw new Error(
`secret backend '${backend.name}' (${backend.type}) cannot provision per-server identities — ` +
'per-server secret delivery requires an openbao backend using kubernetes auth',
);
}
await this.serviceAccounts.ensure(name);
await driver.ensureServerIdentity({
name,
namespace: this.serviceAccounts.namespace,
secretNames,
});
this.log.info(
{ identity: name, namespace: this.serviceAccounts.namespace, secrets: secretNames },
`provisioned scoped OpenBao identity for server '${server.name}' (${String(secretNames.length)} secret(s))`,
);
return name;
}
/**
* Revoke a server's identity. Best-effort per step: a half-removed identity
* grants nothing useful, and failing the whole teardown because a
* ServiceAccount was already gone would leave the OpenBao side orphaned.
*/
async removeFor(serverName: string): Promise<void> {
const name = this.identityNameFor(serverName);
const backend = await this.backends.getDefault();
const driver = this.backends.driverFor(backend);
try {
await driver.removeServerIdentity?.({ name });
} catch (err) {
this.log.warn({ identity: name, err: String(err) }, `could not remove OpenBao identity '${name}'`);
}
try {
await this.serviceAccounts.remove(name);
} catch (err) {
this.log.warn({ identity: name, err: String(err) }, `could not remove ServiceAccount '${name}'`);
}
}
}

View File

@@ -121,15 +121,15 @@ export class PersistentStdioClient {
this.processBuffer();
});
exec.stdout.on('end', () => {
this.initialized = false;
this.exec = null;
for (const [, pending] of this.pendingRequests) {
clearTimeout(pending.timer);
pending.reject(new Error('STDIO process exited'));
}
this.pendingRequests.clear();
});
// All three events funnel into the same teardown: 'end' is the graceful
// path, but an abnormal websocket death may only surface as 'close' or
// 'error' on the stream — before this, isConnected stayed true and every
// request rode out the full timeout against a dead pipe (mcpctl#114).
exec.stdout.on('end', () => this.teardown(exec, 'STDIO process exited'));
exec.stdout.on('close', () => this.teardown(exec, 'STDIO stream closed'));
exec.stdout.on('error', (err: Error) =>
this.teardown(exec, `STDIO stream error: ${err.message}`),
);
// Run MCP init handshake
const initId = this.nextId++;
@@ -174,6 +174,23 @@ export class PersistentStdioClient {
this.initialized = true;
}
/**
* Tear down a dead session: reject in-flight requests and mark the client
* disconnected so the next send() redials via ensureReady(). The identity
* guard is load-bearing: after a reconnect, a LATE 'close'/'end' from the
* previous session's stream must not clobber the new session.
*/
private teardown(exec: InteractiveExec, reason: string): void {
if (this.exec !== exec) return;
this.initialized = false;
this.exec = null;
for (const [, pending] of this.pendingRequests) {
clearTimeout(pending.timer);
pending.reject(new Error(reason));
}
this.pendingRequests.clear();
}
private write(msg: Record<string, unknown>): void {
if (!this.exec) throw new Error('Not connected');
this.exec.write(JSON.stringify(msg) + '\n');

View File

@@ -511,3 +511,130 @@ describe('InstanceService', () => {
});
});
});
// ── syncStatus (mcpctl#114) — first-ever coverage of this method ──
describe('syncStatus', () => {
let instanceRepo: IMcpInstanceRepository;
let serverRepo: IMcpServerRepository;
let orchestrator: McpOrchestrator;
let service: InstanceService;
let invalidator: ReturnType<typeof vi.fn>;
function inspect(info: Partial<{ state: string; restartCount: number }>) {
vi.mocked(orchestrator.inspectContainer).mockResolvedValue({
containerId: 'ctr-abc',
name: 'test',
state: (info.state ?? 'running') as 'running',
createdAt: new Date(),
...(info.restartCount !== undefined ? { restartCount: info.restartCount } : {}),
});
}
beforeEach(() => {
instanceRepo = mockInstanceRepo();
serverRepo = mockServerRepo();
orchestrator = mockOrchestrator();
service = new InstanceService(instanceRepo, serverRepo, orchestrator);
invalidator = vi.fn();
service.setStdioInvalidator(invalidator);
});
it('marks a stopped container ERROR and MERGES retry metadata instead of clobbering it', async () => {
const future = new Date(Date.now() + 60_000).toISOString();
vi.mocked(instanceRepo.findAll).mockResolvedValue([
makeInstance({ status: 'RUNNING', metadata: { attemptCount: 3, nextRetryAt: future } }),
]);
inspect({ state: 'stopped' });
await service.syncStatus();
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
expect(status).toBe('ERROR');
const meta = fields!.metadata as Record<string, unknown>;
expect(meta['error']).toBe('log output'); // last log line
expect(meta['attemptCount']).toBe(3); // preserved — was clobbered before
expect(meta['nextRetryAt']).toBe(future); // preserved — hot-loop killer
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
});
it('recovers an ERROR instance whose pod is running again (the stuck state)', async () => {
vi.mocked(instanceRepo.findAll).mockResolvedValue([
makeInstance({
status: 'ERROR',
metadata: { error: 'Container stopped', attemptCount: 4, nextRetryAt: 'x', lastAttemptAt: 'y' },
}),
]);
inspect({ state: 'running', restartCount: 2 });
await service.syncStatus();
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
expect(status).toBe('RUNNING');
const meta = fields!.metadata as Record<string, unknown>;
expect(meta['error']).toBeUndefined();
expect(meta['attemptCount']).toBeUndefined();
expect(meta['nextRetryAt']).toBeUndefined();
expect(meta['lastRestartCount']).toBe(2);
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
});
it('leaves an ERROR instance alone when its pod is genuinely gone', async () => {
vi.mocked(instanceRepo.findAll).mockResolvedValue([
makeInstance({ status: 'ERROR', metadata: { error: 'x' } }),
]);
vi.mocked(orchestrator.inspectContainer).mockRejectedValue(new Error('not found'));
await service.syncStatus();
expect(instanceRepo.updateStatus).not.toHaveBeenCalled();
expect(invalidator).not.toHaveBeenCalled();
});
it('detects an in-place restart via restartCount and invalidates the stdio pipe', async () => {
vi.mocked(instanceRepo.findAll).mockResolvedValue([
makeInstance({ status: 'RUNNING', metadata: { lastRestartCount: 1 } }),
]);
inspect({ state: 'running', restartCount: 2 });
await service.syncStatus();
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
expect(status).toBe('RUNNING');
expect((fields!.metadata as Record<string, unknown>)['lastRestartCount']).toBe(2);
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
});
it('baselines a first-seen restartCount without invalidating', async () => {
vi.mocked(instanceRepo.findAll).mockResolvedValue([
makeInstance({ status: 'RUNNING', metadata: {} }),
]);
inspect({ state: 'running', restartCount: 0 });
await service.syncStatus();
const [, , fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
expect((fields!.metadata as Record<string, unknown>)['lastRestartCount']).toBe(0);
expect(invalidator).not.toHaveBeenCalled();
});
it('does nothing for a steady RUNNING instance with an unchanged restartCount', async () => {
vi.mocked(instanceRepo.findAll).mockResolvedValue([
makeInstance({ status: 'RUNNING', metadata: { lastRestartCount: 2 } }),
]);
inspect({ state: 'running', restartCount: 2 });
await service.syncStatus();
expect(instanceRepo.updateStatus).not.toHaveBeenCalled();
expect(invalidator).not.toHaveBeenCalled();
});
it('remove() evicts the cached stdio client', async () => {
vi.mocked(instanceRepo.findById).mockResolvedValue(makeInstance({ status: 'RUNNING' }));
await service.remove('inst-1');
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
});
});

View File

@@ -63,12 +63,22 @@ vi.mock('@kubernetes/client-node', () => {
makeApiClient = vi.fn(() => mockCore);
}
// Track the live Exec/Attach instances so tests can program their vi.fn()s
// (each K8sOfficialClient constructs fresh ones).
const instances: { exec?: { exec: ReturnType<typeof vi.fn> }; attach?: { attach: ReturnType<typeof vi.fn> } } = {};
class MockExec {
exec = vi.fn();
constructor() {
instances.exec = this;
}
}
class MockAttach {
attach = vi.fn();
constructor() {
instances.attach = this;
}
}
class MockLog {
@@ -82,7 +92,7 @@ vi.mock('@kubernetes/client-node', () => {
Attach: MockAttach,
Log: MockLog,
// Export test helpers
__testHelpers: { setHandler, getHandler, clearHandlers, mockCore },
__testHelpers: { setHandler, getHandler, clearHandlers, mockCore, instances },
};
});
@@ -379,3 +389,134 @@ describe('httpStatusOf', () => {
expect(httpStatusOf({ code: 'ECONNREFUSED' })).toBeUndefined();
});
});
// ── mcpctl#114: interactive-session death signals, restartCount, 409-adopt ──
import { EventEmitter } from 'node:events';
function makeFakeWs(): EventEmitter & { close: ReturnType<typeof vi.fn> } {
const ws = new EventEmitter() as EventEmitter & { close: ReturnType<typeof vi.fn> };
ws.close = vi.fn();
return ws;
}
const podRestarted = {
...podRunning,
status: {
...podRunning.status,
containerStatuses: [{
state: { running: { startedAt: '2026-01-02T00:00:00Z' } },
restartCount: 3,
lastState: { terminated: { reason: 'OOMKilled', exitCode: 137 } },
}],
},
};
const podTerminated = {
...podRunning,
status: {
phase: 'Failed',
containerStatuses: [{
state: { terminated: { reason: 'Error', exitCode: 1 } },
restartCount: 1,
}],
},
};
describe('KubernetesOrchestrator interactive sessions (mcpctl#114)', () => {
let orch: KubernetesOrchestrator;
beforeEach(() => {
clearHandlers();
vi.clearAllMocks();
orch = new KubernetesOrchestrator({ serversNamespace: 'mcpctl-servers' });
});
it("execInteractive: ws 'close' ends stdout so the consumer sees the death", async () => {
setHandler('readNamespacedPod:my-server', podRunning);
const ws = makeFakeWs();
k8sMock.__testHelpers.instances.exec!.exec.mockResolvedValue(ws);
const iexec = await orch.execInteractive!('my-server', ['node', 'index.js']);
iexec.stdout.resume(); // PassThrough only emits 'end' once it is being read
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
ws.emit('close');
await expect(ended).resolves.toBeUndefined();
});
it("execInteractive: ws 'error' also ends stdout, and close() closes the ws", async () => {
setHandler('readNamespacedPod:my-server', podRunning);
const ws = makeFakeWs();
k8sMock.__testHelpers.instances.exec!.exec.mockResolvedValue(ws);
const iexec = await orch.execInteractive!('my-server', ['node']);
iexec.stdout.resume();
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
ws.emit('error', new Error('abnormal closure'));
await expect(ended).resolves.toBeUndefined();
iexec.close();
expect(ws.close).toHaveBeenCalled();
});
it("attachInteractive: ws 'close' ends stdout (first-ever attach coverage)", async () => {
setHandler('readNamespacedPod:my-server', podRunning);
const ws = makeFakeWs();
k8sMock.__testHelpers.instances.attach!.attach.mockResolvedValue(ws);
const iexec = await orch.attachInteractive!('my-server');
iexec.stdout.resume();
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
ws.emit('close');
await expect(ended).resolves.toBeUndefined();
expect(k8sMock.__testHelpers.instances.attach!.attach).toHaveBeenCalledOnce();
});
it('inspectContainer surfaces restartCount and startedAt', async () => {
setHandler('readNamespacedPod:my-server', podRestarted);
const info = await orch.inspectContainer('my-server');
expect(info.state).toBe('running');
expect(info.restartCount).toBe(3);
expect(info.startedAt).toEqual(new Date('2026-01-02T00:00:00Z'));
});
it('createContainer ADOPTS an alive pod on 409 instead of throwing', async () => {
setHandler('readNamespace:mcpctl-servers', {});
setHandler('createNamespacedPod', undefined, { code: 409 });
setHandler('readNamespacedPod:my-server', podRestarted);
const info = await orch.createContainer(testSpec);
expect(info.containerId).toBe('my-server');
expect(info.state).toBe('running');
expect(mockCore.deleteNamespacedPod).not.toHaveBeenCalled();
});
it('createContainer replaces a genuinely dead pod on 409', async () => {
setHandler('readNamespace:mcpctl-servers', {});
setHandler('createNamespacedPod', undefined, { code: 409 });
setHandler('readNamespacedPod:my-server', podTerminated);
setHandler('deleteNamespacedPod:my-server', {});
const promise = orch.createContainer(testSpec);
// Give it a tick to inspect + delete, then flip the world: pod is gone,
// create succeeds.
await new Promise((r) => setTimeout(r, 50));
// The gone-wait loop polls once per second: serve 404 so it breaks, then
// restore the pod before the post-create inspect (~500ms later) runs.
setHandler('readNamespacedPod:my-server', undefined, { code: 404 });
setHandler('createNamespacedPod', podRunning);
setTimeout(() => setHandler('readNamespacedPod:my-server', podRunning), 1200);
const info = await promise;
expect(info.state).toBe('running');
expect(mockCore.deleteNamespacedPod).toHaveBeenCalled();
expect(mockCore.createNamespacedPod).toHaveBeenCalledTimes(2);
});
it('createContainer still throws on non-409 errors', async () => {
setHandler('readNamespace:mcpctl-servers', {});
setHandler('createNamespacedPod', undefined, { code: 500 });
await expect(orch.createContainer(testSpec)).rejects.toBeTruthy();
});
});

View File

@@ -725,3 +725,103 @@ describe('MCP server full flow', () => {
});
});
});
// ── mcpctl#114: attach server survives an in-place container restart ──
import { PassThrough } from 'node:stream';
import type { InteractiveExec } from '../src/services/orchestrator.js';
/**
* A scripted MCP server on the other end of an attach session: answers
* initialize and every request with a result carrying its session number,
* so the test can prove which session served which call.
*/
function makeScriptedAttach(session: number): InteractiveExec {
const stdout = new PassThrough();
return {
stdout,
write(data: string) {
for (const line of data.split('\n')) {
if (!line.trim()) continue;
const msg = JSON.parse(line) as { id?: number; method?: string };
if (msg.id === undefined) continue; // notifications
const result = msg.method === 'initialize'
? { capabilities: {} }
: { ok: true, session };
stdout.write(JSON.stringify({ jsonrpc: '2.0', id: msg.id, result }) + '\n');
}
},
close() {
stdout.destroy();
},
};
}
describe('attach-mode STDIO restart recovery (mcpctl#114)', () => {
it('survives an in-place container restart: syncStatus invalidates, next call rides a new session', async () => {
const serverRepo = createInMemoryServerRepo();
const instanceRepo = createInMemoryInstanceRepo();
const server = await serverRepo.create({
name: 'gitea',
transport: 'STDIO',
dockerImage: 'ghcr.io/gitea-mcp:latest',
replicas: 1,
} as never);
const instance = await instanceRepo.create({
serverId: server.id,
containerId: 'pod-gitea',
status: 'RUNNING',
metadata: { lastRestartCount: 0 },
} as never);
let restartCount = 0;
const sessions: InteractiveExec[] = [];
const orchestrator = {
ping: vi.fn(async () => true),
pullImage: vi.fn(async () => {}),
createContainer: vi.fn(),
stopContainer: vi.fn(async () => {}),
removeContainer: vi.fn(async () => {}),
inspectContainer: vi.fn(async () => ({
containerId: 'pod-gitea',
name: 'pod-gitea',
state: 'running' as const,
createdAt: new Date(),
restartCount,
})),
getContainerLogs: vi.fn(async () => ({ stdout: '', stderr: '' })),
execInContainer: vi.fn(),
attachInteractive: vi.fn(async () => {
const s = makeScriptedAttach(sessions.length + 1);
sessions.push(s);
return s;
}),
} as unknown as McpOrchestrator;
const proxyService = new McpProxyService(instanceRepo, serverRepo, orchestrator);
const instanceService = new InstanceService(instanceRepo, serverRepo, orchestrator);
instanceService.setStdioInvalidator((cid) => proxyService.removeClient(cid));
// 1. A call round-trips on session 1.
const first = await proxyService.execute({ serverId: server.id, method: 'tools/list' });
expect((first.result as { session: number }).session).toBe(1);
// 2. The container crashes and kubelet restarts it in place: same pod
// name, restartCount bumps, the old session's pipe dies.
sessions[0]!.stdout.end();
restartCount = 1;
// 3. One reconcile tick detects the bump, keeps the row RUNNING, and
// evicts the stale client.
await instanceService.syncStatus();
const after = await instanceRepo.findById(instance.id);
expect(after!.status).toBe('RUNNING');
expect((after!.metadata as { lastRestartCount: number }).lastRestartCount).toBe(1);
// 4. The next call succeeds first-try on a fresh attach session.
const second = await proxyService.execute({ serverId: server.id, method: 'tools/list' });
expect((second.result as { session: number }).session).toBe(2);
expect(orchestrator.attachInteractive).toHaveBeenCalledTimes(2);
});
});

View File

@@ -109,3 +109,110 @@ describe('PersistentStdioClient', () => {
await expect(client.send('tools/list')).rejects.toThrow(/interactive exec/i);
});
});
// Drive a fake session through the init handshake so send() can round-trip.
async function completeHandshake(fake: ReturnType<typeof makeFakeExec>): Promise<void> {
await new Promise((r) => setTimeout(r, 10));
const init = JSON.parse(fake.written[0]!);
fake.emit({ jsonrpc: '2.0', id: init.id, result: { capabilities: {} } });
await new Promise((r) => setTimeout(r, 150));
}
describe('PersistentStdioClient teardown & reconnect (mcpctl#114)', () => {
it("stdout 'end' rejects in-flight requests and the next send() redials", async () => {
const fakes = [makeFakeExec(), makeFakeExec()];
let call = 0;
const execInteractive = vi.fn(async () => fakes[call++]!.iexec);
const client = new PersistentStdioClient(
makeOrchestrator({ execInteractive }),
'c1',
{ kind: 'exec', command: ['node'] },
);
const first = client.send('tools/list');
await completeHandshake(fakes[0]!);
// Kill the session while tools/list is in flight.
fakes[0]!.iexec.stdout.end();
await expect(first).rejects.toThrow('STDIO process exited');
expect(client.isConnected).toBe(false);
// Next send must redial through a fresh interactive session.
const second = client.send('tools/list');
await completeHandshake(fakes[1]!);
const req = JSON.parse(fakes[1]!.written[2]!);
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [] } });
const res = await second;
expect(res.result).toEqual({ tools: [] });
expect(execInteractive).toHaveBeenCalledTimes(2);
client.close();
});
it("stdout 'error' tears down with the error message", async () => {
const fake = makeFakeExec();
const client = new PersistentStdioClient(
makeOrchestrator({ execInteractive: vi.fn(async () => fake.iexec) }),
'c1',
{ kind: 'exec', command: ['node'] },
);
const pending = client.send('tools/list');
await completeHandshake(fake);
fake.iexec.stdout.emit('error', new Error('socket hang up'));
await expect(pending).rejects.toThrow('STDIO stream error: socket hang up');
expect(client.isConnected).toBe(false);
client.close();
});
it("a LATE 'end' from the previous session does not clobber the reconnected one", async () => {
const fakes = [makeFakeExec(), makeFakeExec()];
let call = 0;
const client = new PersistentStdioClient(
makeOrchestrator({ attachInteractive: vi.fn(async () => fakes[call++]!.iexec) }),
'c1',
{ kind: 'attach' },
);
const first = client.send('tools/list');
await completeHandshake(fakes[0]!);
fakes[0]!.iexec.stdout.end();
await expect(first).rejects.toThrow('STDIO process exited');
// Reconnect on a fresh session.
const second = client.send('tools/list');
await completeHandshake(fakes[1]!);
// The old session's stream fires a stale 'close' AFTER the reconnect —
// the identity guard must ignore it.
fakes[0]!.iexec.stdout.emit('close');
expect(client.isConnected).toBe(true);
const req = JSON.parse(fakes[1]!.written[2]!);
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [{ name: 't' }] } });
const res = await second;
expect((res.result as { tools: unknown[] }).tools).toHaveLength(1);
client.close();
});
it('attach mode also redials after teardown', async () => {
const fakes = [makeFakeExec(), makeFakeExec()];
let call = 0;
const attachInteractive = vi.fn(async () => fakes[call++]!.iexec);
const client = new PersistentStdioClient(
makeOrchestrator({ attachInteractive }),
'c-gitea',
{ kind: 'attach' },
);
const first = client.send('tools/list');
await completeHandshake(fakes[0]!);
fakes[0]!.iexec.stdout.emit('close');
await expect(first).rejects.toThrow('STDIO stream closed');
const second = client.send('tools/list');
await completeHandshake(fakes[1]!);
const req = JSON.parse(fakes[1]!.written[2]!);
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [] } });
await expect(second).resolves.toMatchObject({ result: { tools: [] } });
expect(attachInteractive).toHaveBeenCalledTimes(2);
client.close();
});
});

View File

@@ -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> = {}): 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<SecretBackendDriver, 'healthCheck' | 'authCheck'>,
row: SecretBackend = backendRow(),
): Promise<FastifyInstance> {
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);
});
});

View File

@@ -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> = {}): 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<typeof vi.fn>; isOverdue: ReturnType<typeof vi.fn>; healthCheck: ReturnType<typeof vi.fn>; rotateOne: ReturnType<typeof vi.fn> };
logs: { info: string[]; warn: string[]; error: Array<{ obj: Record<string, unknown>; 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<void> => { 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<SecretBackend>)];
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);
});
});

View File

@@ -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' } }] });
@@ -86,9 +87,9 @@ describe('OpenBaoDriver', () => {
await expect(driver.delete({ name: 'gone', externalRef: '' })).resolves.toBeUndefined();
});
it('list returns names from the metadata LIST call', async () => {
it('list returns names from the metadata listing', async () => {
const fetchFn = makeFetch([{
url: /\/v1\/secret\/metadata\/mcpctl\/$/,
url: /\/v1\/secret\/metadata\/mcpctl\/\?list=true$/,
status: 200,
body: { data: { keys: ['token1', 'token2', 'sub-folder/'] } },
}]);
@@ -97,6 +98,10 @@ describe('OpenBaoDriver', () => {
{ fetch: fetchFn as unknown as typeof fetch, secretRefResolver: resolver },
);
const result = await driver.list();
// GET ?list=true rather than the LIST verb: proxies (Cilium's ingress
// Envoy among them) answer a bare 400 to the non-standard method.
const [, listInit] = fetchFn.mock.calls[0] as [unknown, RequestInit];
expect(listInit.method).toBe('GET');
// Sub-folders (trailing slash) are excluded; only leaf keys are returned.
expect(result).toEqual([
{ name: 'token1', externalRef: 'secret/mcpctl/token1' },
@@ -242,3 +247,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<void> => undefined;
function driverWith(fetchFn: ReturnType<typeof vi.fn>, opts: Record<string, unknown> = {}): 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<string, string>)['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);
});
});

View File

@@ -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> = {}): SecretBackendDriver & {
read: ReturnType<typeof vi.fn>;
write: ReturnType<typeof vi.fn>;
delete: ReturnType<typeof vi.fn>;
} {
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<Record<string, unknown>>; infos: Array<Record<string, unknown>> } {
const warns: Array<Record<string, unknown>> = [];
const infos: Array<Record<string, unknown>> = [];
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 });
});
});

View File

@@ -544,3 +544,32 @@ describe('HealthProbeRunner', () => {
expect(result.message).toBe('ECONNREFUSED 10.0.0.1:3000');
});
});
describe('HealthProbeRunner stale-pipe eviction (mcpctl#114)', () => {
it('evicts the cached stdio client exactly once, when failures cross the threshold', async () => {
const instanceRepo = mockInstanceRepo();
const serverRepo = mockServerRepo();
const orchestrator = mockOrchestrator();
const mcpProxyService = mockMcpProxyService();
const runner = new HealthProbeRunner(instanceRepo, serverRepo, orchestrator, undefined, mcpProxyService);
const instance = makeInstance();
// intervalSeconds: 0 so every tick actually probes; failureThreshold: 3.
const server = makeServer({
healthCheck: { tool: 'list_datasources', arguments: {}, intervalSeconds: 0, timeoutSeconds: 10, failureThreshold: 3 },
} as Partial<McpServer>);
vi.mocked(instanceRepo.findAll).mockResolvedValue([instance]);
vi.mocked(serverRepo.findById).mockResolvedValue(server);
vi.mocked(mcpProxyService.execute).mockRejectedValue(new Error('pipe is dead'));
await runner.tick(); // failure 1 — degraded
await runner.tick(); // failure 2 — degraded
expect(mcpProxyService.removeClient).not.toHaveBeenCalled();
await runner.tick(); // failure 3 — crosses threshold → evict once
expect(mcpProxyService.removeClient).toHaveBeenCalledExactlyOnceWith('container-abc');
await runner.tick(); // failure 4 — already past threshold, no re-evict
expect(mcpProxyService.removeClient).toHaveBeenCalledTimes(1);
});
});

View File

@@ -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<typeof vi.fn>);
expect(findMany.mock.calls[0]?.[0]).toMatchObject({ where: { replicas: { gt: 0 } } });
});
});

View File

@@ -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<boolean> {
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);
});
});

View File

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

View File

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

View File

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