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
parent bd3e1134c9
commit 0fbfc72d68
7 changed files with 488 additions and 21 deletions
--- a/src/cli/src/commands/status.ts
+++ b/src/cli/src/commands/status.ts
@@ -50,6 +50,7 @@ interface ServerLlm {
 * of the last credential-rotation failure (e.g. a dead OpenBao token).
 */
 interface SecretBackendInfo {
+  id: string;
  name: string;
  type: string;
  isDefault?: boolean;
@@ -60,6 +61,22 @@ interface SecretBackendInfo {
  } | null;
 }

+/**
+ * Live probe result from GET /api/v1/secretbackends/:id/health.
+ *
+ * `live` and `ready` are deliberately separate: a backend that is reachable but
+ * whose credentials no longer grant anything is the failure mode that hid an
+ * OpenBao re-init for four days. `null` means the probe itself failed, which we
+ * report as unknown rather than pretending it means healthy.
+ */
+interface SecretBackendHealth {
+  live: boolean;
+  liveDetail?: string;
+  ready: boolean;
+  readyDetail?: string;
+  cache?: { entries: number; servingStale: number; oldestStaleSince?: number | null } | null;
+}
+
 /**
 * Result of a live "say hi" probe against a server LLM. `ok` says we got a
 * 200 + non-empty content back; `say` is the trimmed first 16 chars of the
@@ -99,6 +116,7 @@ export interface StatusCommandDeps {
  probeServerLlm: (mcpdUrl: string, name: string, token: string | null) => Promise<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
--- a/src/cli/tests/commands/status.test.ts
+++ b/src/cli/tests/commands/status.test.ts
@@ -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' });