michal/mcpctl
1
0
Fork 0
Code Issues Pull Requests 7 Actions Packages Projects Releases Wiki Activity

feat(secrets): survive OpenBao outages instead of cascading them

Browse Source 
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
This commit is contained in:
Michal
2026-08-20 22:08:10 +01:00
parent db38de7e09
commit bd3e1134c9
9 changed files with 780 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

89
src/mcpd/tests/secret-backends.test.ts
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' } }] });
@@ -242,3 +243,91 @@ describe('OpenBaoDriver', () => {
});
});
});
describe('OpenBaoDriver: resilience', () => {
const resolver = { resolve: vi.fn(async () => 'test-vault-token') };
/** No real sleeping — otherwise the backoff tests take seconds. */
const noSleep = async (): Promise<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);
});
});

198
src/mcpd/tests/secret-cache.test.ts Normal file
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 });
});
});