diff --git a/src/mcpd/src/routes/auth.ts b/src/mcpd/src/routes/auth.ts index 72a24b3..e69eba5 100644 --- a/src/mcpd/src/routes/auth.ts +++ b/src/mcpd/src/routes/auth.ts @@ -71,9 +71,18 @@ export function registerAuthRoutes(app: FastifyInstance, deps: AuthRouteDeps): v return session; }); - // GET /api/v1/auth/me — returns current user identity - app.get('/api/v1/auth/me', { preHandler: [authMiddleware] }, async (request) => { - const user = await deps.userService.getById(request.userId!); + // GET /api/v1/auth/me — returns current user identity. + // The authMiddleware guards this route, but if it ever falls through with + // `request.userId === undefined` (e.g. an McpToken bearer that authenticated + // a service principal but has no associated User row), Prisma blows up on + // findUnique({ where: { id: undefined } }) with PrismaClientValidationError + // — surface a clear 401 instead. + app.get('/api/v1/auth/me', { preHandler: [authMiddleware] }, async (request, reply) => { + if (request.userId === undefined) { + reply.code(401); + return { error: 'No user identity on this request (service-account or token-bound principal cannot be queried via /me)' }; + } + const user = await deps.userService.getById(request.userId); return { id: user.id, email: user.email, name: user.name ?? null }; }); diff --git a/src/mcpd/src/services/secret-backend-rotator-loop.ts b/src/mcpd/src/services/secret-backend-rotator-loop.ts index 82ce70b..2fae8ce 100644 --- a/src/mcpd/src/services/secret-backend-rotator-loop.ts +++ b/src/mcpd/src/services/secret-backend-rotator-loop.ts @@ -61,6 +61,29 @@ export class SecretBackendRotatorLoop { this.log.info(`starting rotation loop for ${String(backends.length)} backend(s)`); for (const b of backends) { + // Boot-time health check: catches "upstream re-init invalidated our + // stored token" the moment mcpd starts, not 24 hours later when the + // scheduled rotation finally fires. Logs loudly with explicit + // remediation; the rotator service has already persisted the same + // message to tokenMeta.lastRotationError so `describe secretbackend` + // surfaces it too. + 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.warn(`backend '${b.name}' health check failed: ${res.message ?? 'unknown'}`); + } + }) + .catch((err) => { + this.log.warn(`backend '${b.name}' health check threw: ${err instanceof Error ? err.message : String(err)}`); + }); + if (this.deps.rotator.isOverdue(b)) { this.log.info(`backend '${b.name}' is overdue — rotating now`); this.runOnce(b.id, b.name).catch((err) => { diff --git a/src/mcpd/src/services/secret-backend-rotator.service.ts b/src/mcpd/src/services/secret-backend-rotator.service.ts index 3144688..f041cad 100644 --- a/src/mcpd/src/services/secret-backend-rotator.service.ts +++ b/src/mcpd/src/services/secret-backend-rotator.service.ts @@ -123,8 +123,33 @@ export class SecretBackendRotator { await this.deps.secrets.update(secretRow.id, { data: nextData }); } catch (err) { const msg = err instanceof Error ? err.message : String(err); - await this.recordError(backendId, meta, msg); - throw err; + // Classify "current token is dead" (HTTP 403 from mint OR lookup-self). + // This happens when the upstream OpenBao was re-initialized — every + // pre-existing token is invalidated, including ours. The rotator can + // never self-heal from this state because it needs the (dead) token + // to mint a successor. Surface explicit remediation so the operator + // doesn't have to spelunk through 500s to figure it out. + const tokenDead = /HTTP 403|permission denied|invalid token|HTTP 401/i.test(msg); + const wrapped = tokenDead + ? new Error( + `BACKEND_TOKEN_DEAD: rotator could not authenticate to ${cfg.url} as the stored token. ` + + `This is unrecoverable from inside mcpd — likely cause: OpenBao was re-initialized and all old tokens are invalid. ` + + `Remediation: mint a fresh token under role '${cfg.rotation.tokenRole}' using a working OpenBao admin token, ` + + `then \`mcpctl create secret ${cfg.tokenSecretRef.name} --data ${cfg.tokenSecretRef.key}= --force\`. ` + + `Original error: ${msg}`) + : 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, + })); + throw wrapped; } // 5. Revoke predecessor (best-effort — old tokens expire anyway). @@ -162,6 +187,46 @@ export class SecretBackendRotator { return nextMeta; } + /** + * Probe the backend's stored token by calling `auth/token/lookup-self` + * (cheap, idempotent). Returns `{ok:true}` if the token is valid, or + * `{ok:false, message}` with a clear remediation message if dead. Used + * by the loop on startup so an OpenBao re-init that invalidated all old + * tokens shows up in mcpd logs immediately, not 24 hours later when the + * scheduled rotation finally runs. + */ + async healthCheck(backendId: string): Promise<{ ok: boolean; message?: string }> { + const backend = await this.deps.backends.getById(backendId); + if (!this.isRotatable(backend)) return { ok: true }; + const cfg = backend.config as unknown as RotatableOpenBaoConfig; + const vaultDeps: VaultDeps = {}; + if (this.deps.fetch !== undefined) vaultDeps.fetch = this.deps.fetch; + if (cfg.namespace !== undefined) vaultDeps.namespace = cfg.namespace; + try { + const secretRow = await this.deps.secrets.getByName(cfg.tokenSecretRef.name); + const data = await this.deps.secrets.resolveData(secretRow); + const token = data[cfg.tokenSecretRef.key]; + if (token === undefined || token === '') { + return { ok: false, message: `Stored token at ${cfg.tokenSecretRef.name}/${cfg.tokenSecretRef.key} is empty` }; + } + await lookupSelf(cfg.url, token, vaultDeps); + return { ok: true }; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + const tokenDead = /HTTP 403|permission denied|invalid token|HTTP 401/i.test(msg); + const wrapped = tokenDead + ? `BACKEND_TOKEN_DEAD: ${cfg.url} rejected the stored token (likely upstream re-init). ` + + `Remediation: mint a fresh token under role '${cfg.rotation.tokenRole}' and run ` + + `\`mcpctl create secret ${cfg.tokenSecretRef.name} --data ${cfg.tokenSecretRef.key}= --force\`. ` + + `Original: ${msg}` + : `health check failed: ${msg}`; + // Persist on the row so describe shows it. + const meta = (backend.tokenMeta as unknown as TokenMeta | null | undefined) ?? {}; + await this.recordError(backendId, meta, wrapped).catch(() => undefined); + return { ok: false, message: wrapped }; + } + } + /** Is this backend overdue for rotation? Used by the loop on startup. */ isOverdue(backend: SecretBackend): boolean { const meta = (backend.tokenMeta as unknown as TokenMeta | null | undefined) ?? {}; diff --git a/src/mcplocal/src/http/token-auth.ts b/src/mcplocal/src/http/token-auth.ts index 8cebbce..f58f074 100644 --- a/src/mcplocal/src/http/token-auth.ts +++ b/src/mcplocal/src/http/token-auth.ts @@ -51,7 +51,13 @@ interface CacheEntry { } export function createTokenAuthMiddleware(opts: TokenAuthOptions) { - const positiveTtl = opts.positiveTtlMs ?? 30_000; + // Positive TTL must be tight enough that token revocation propagates + // quickly. mcpd's introspection endpoint is a single DB lookup — the cache + // only protects against burst restart storms, not steady-state load. A 30s + // positive cache let revoked tokens keep working for the full window + // (caught by mcptoken.smoke negative-cache-window assertion); 5s matches + // negativeTtl and aligns with the test's `wait 7s after revoke` expectation. + const positiveTtl = opts.positiveTtlMs ?? 5_000; const negativeTtl = opts.negativeTtlMs ?? 5_000; const fetchImpl = opts.fetch ?? (globalThis.fetch as typeof fetch); const cache = new Map(); diff --git a/src/mcplocal/tests/smoke/agent.smoke.test.ts b/src/mcplocal/tests/smoke/agent.smoke.test.ts index 7944d22..b412dc2 100644 --- a/src/mcplocal/tests/smoke/agent.smoke.test.ts +++ b/src/mcplocal/tests/smoke/agent.smoke.test.ts @@ -146,8 +146,11 @@ describe('agent smoke', () => { const applied = run(`apply -f ${path}`); expect(applied.code, applied.stderr || applied.stdout).toBe(0); const second = run(`get agent ${AGENT_NAME} -o json`); - const parsed = JSON.parse(second.stdout) as { description: string }; - expect(parsed.description).toBe('smoke agent (amended)'); + // `mcpctl get -o json` always returns an array (one + // element when fetching a single item) — formatted via toApplyDocs so it + // round-trips through `apply -f`. + const parsed = JSON.parse(second.stdout) as Array<{ description: string }>; + expect(parsed[0]!.description).toBe('smoke agent (amended)'); } finally { unlinkSync(path); } @@ -222,7 +225,7 @@ function httpRequest(method: string, urlStr: string, body: unknown): Promise(path: string, retries = 3): Promise { const url = new URL(path, MCPD_EFFECTIVE_URL); const headers: Record = { 'Accept': 'application/json' }; if (MCPD_CREDS.token) headers['Authorization'] = `Bearer ${MCPD_CREDS.token}`; - http.get(url, { timeout: 10_000, headers }, (res) => { + const driver = url.protocol === 'https:' ? https : http; + driver.get(url, { timeout: 10_000, headers }, (res) => { const chunks: Buffer[] = []; res.on('data', (chunk: Buffer) => chunks.push(chunk)); res.on('end', () => { diff --git a/src/mcplocal/tests/smoke/mcp-client.ts b/src/mcplocal/tests/smoke/mcp-client.ts index 64dbc41..cb25e74 100644 --- a/src/mcplocal/tests/smoke/mcp-client.ts +++ b/src/mcplocal/tests/smoke/mcp-client.ts @@ -3,6 +3,10 @@ * Sends JSON-RPC messages to mcplocal's HTTP endpoint and parses SSE responses. */ import http from 'node:http'; +import https from 'node:https'; +import { readFileSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { homedir } from 'node:os'; export interface McpResponse { status: number; @@ -21,6 +25,45 @@ export function getMcpdUrl(): string { return MCPD_URL; } +/** + * Resolve the live mcpd `{ token, url }` the way the CLI itself does: + * - URL from `~/.mcpctl/config.json`'s `mcpdUrl` (with $MCPD_URL override) + * - token from `~/.mcpctl/credentials`'s `token` field + * + * Critically, **the URL does NOT come from credentials**. credentials carries + * an `mcpdUrl` field for legacy reasons that goes stale (left over from old + * `mcpctl login --mcpd-url localhost:3xxx` invocations). Tests that read the + * URL from credentials end up hitting whatever URL the user last logged into, + * not the URL the CLI is actually using right now. + */ +export function loadMcpdAuth(): { token: string; url: string } { + const url = readConfigMcpdUrl() ?? MCPD_URL; + const token = readCredentialsToken() ?? ''; + return { token, url }; +} + +function readConfigMcpdUrl(): string | null { + const path = join(homedir(), '.mcpctl', 'config.json'); + if (!existsSync(path)) return null; + try { + const parsed = JSON.parse(readFileSync(path, 'utf-8')) as { mcpdUrl?: string }; + return typeof parsed.mcpdUrl === 'string' && parsed.mcpdUrl.length > 0 ? parsed.mcpdUrl : null; + } catch { + return null; + } +} + +function readCredentialsToken(): string | null { + const path = join(homedir(), '.mcpctl', 'credentials'); + if (!existsSync(path)) return null; + try { + const parsed = JSON.parse(readFileSync(path, 'utf-8')) as { token?: string }; + return typeof parsed.token === 'string' && parsed.token.length > 0 ? parsed.token : null; + } catch { + return null; + } +} + function httpRequest(opts: { url: string; method: string; @@ -30,10 +73,11 @@ function httpRequest(opts: { }): Promise<{ status: number; headers: http.IncomingHttpHeaders; body: string }> { return new Promise((resolve, reject) => { const parsed = new URL(opts.url); - const req = http.request( + const driver = parsed.protocol === 'https:' ? https : http; + const req = driver.request( { hostname: parsed.hostname, - port: parsed.port, + port: parsed.port || (parsed.protocol === 'https:' ? 443 : 80), path: parsed.pathname + parsed.search, method: opts.method, headers: opts.headers, @@ -178,7 +222,12 @@ export class SmokeMcpSession { } async callTool(name: string, args: Record = {}, timeout?: number): Promise<{ content: Array<{ type: string; text?: string }>; isError?: boolean }> { - return await this.send('tools/call', { name, arguments: args }, timeout) as { content: Array<{ type: string; text?: string }>; isError?: boolean }; + // Default 60s — many real MCP tools (web fetch, doc retrieval, query + // execution) routinely take 10-30s under normal load. The previous 30s + // floor was tight enough that occasional upstream latency tripped the + // proxy-pipeline hot-reload smoke. Tests that need a tighter bound can + // pass an explicit value. + return await this.send('tools/call', { name, arguments: args }, timeout ?? 60_000) as { content: Array<{ type: string; text?: string }>; isError?: boolean }; } async close(): Promise { diff --git a/src/mcplocal/tests/smoke/secretbackend.smoke.test.ts b/src/mcplocal/tests/smoke/secretbackend.smoke.test.ts index f01c1aa..247ee9b 100644 --- a/src/mcplocal/tests/smoke/secretbackend.smoke.test.ts +++ b/src/mcplocal/tests/smoke/secretbackend.smoke.test.ts @@ -79,15 +79,19 @@ describe('secretbackend smoke', () => { run(`delete secretbackend ${BACKEND_NAME}`); }); - it('lists at least one secretbackend (the seeded plaintext default)', () => { + it('lists at least one secretbackend with a default flagged', () => { if (!mcpdUp) return; + // The seeded `plaintext` backend is the bootstrap default, but operators + // routinely promote a remote backend (openbao etc.) to default once it's + // healthy. Asserting a specific *name* here is implementation detail — + // the invariant we care about is that exactly one row is the default. const result = run('get secretbackends -o json'); expect(result.code, result.stderr).toBe(0); const rows = JSON.parse(result.stdout) as Array<{ name: string; type: string; isDefault: boolean }>; expect(rows.length).toBeGreaterThan(0); - const defaultRow = rows.find((r) => r.isDefault === true); - expect(defaultRow, 'a default backend must exist').toBeDefined(); - expect(defaultRow!.type).toBe('plaintext'); + const defaults = rows.filter((r) => r.isDefault === true); + expect(defaults, 'exactly one default backend must exist').toHaveLength(1); + expect(['plaintext', 'openbao']).toContain(defaults[0]!.type); }); it('creates a plaintext backend and round-trips it through describe', () => { @@ -118,10 +122,13 @@ describe('secretbackend smoke', () => { expect(def).toBeDefined(); const del = run(`delete secretbackend ${def!.name}`); - // 409 surfaces as exit 1 with a descriptive error + // 409 surfaces as exit 1 with a descriptive error. The exact wording has + // changed across releases ("is the default", "is in use", "cannot delete", + // "is still referenced by N secret(s); migrate them first") — accept any + // refusal that mentions one of: default, in use, cannot delete, referenced. expect(del.code).toBe(1); const combined = (del.stderr + del.stdout).toLowerCase(); - expect(combined).toMatch(/default|in use|cannot delete/); + expect(combined).toMatch(/default|in use|cannot delete|referenced/); }); it('round-trips get -o yaml → apply -f', () => { diff --git a/src/mcplocal/tests/smoke/security.test.ts b/src/mcplocal/tests/smoke/security.test.ts index c363f7b..19a4848 100644 --- a/src/mcplocal/tests/smoke/security.test.ts +++ b/src/mcplocal/tests/smoke/security.test.ts @@ -15,29 +15,15 @@ */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import http from 'node:http'; -import { readFileSync } from 'node:fs'; -import { join } from 'node:path'; -import { homedir } from 'node:os'; -import { isMcplocalRunning, getMcplocalUrl, getMcpdUrl } from './mcp-client.js'; +import https from 'node:https'; +import { isMcplocalRunning, getMcplocalUrl, loadMcpdAuth } from './mcp-client.js'; const MCPLOCAL_URL = getMcplocalUrl(); -const MCPD_URL = getMcpdUrl(); -function loadMcpdCredentials(): { token: string; url: string } { - try { - const raw = readFileSync(join(homedir(), '.mcpctl', 'credentials'), 'utf-8'); - const parsed = JSON.parse(raw) as { token?: string; mcpdUrl?: string }; - return { - token: parsed.token ?? '', - url: parsed.mcpdUrl ?? MCPD_URL, - }; - } catch { - return { token: '', url: MCPD_URL }; - } -} - -const MCPD_CREDS = loadMcpdCredentials(); -const MCPD_EFFECTIVE_URL = MCPD_CREDS.url || MCPD_URL; +// URL from config.json, token from credentials (matches the CLI itself). +// See loadMcpdAuth() JSDoc for why credentials.mcpdUrl is intentionally ignored. +const MCPD_CREDS = loadMcpdAuth(); +const MCPD_EFFECTIVE_URL = MCPD_CREDS.url; /** Low-level HTTP request helper. */ function httpRequest(opts: { @@ -49,10 +35,11 @@ function httpRequest(opts: { }): Promise<{ status: number; headers: http.IncomingHttpHeaders; body: string }> { return new Promise((resolve, reject) => { const parsed = new URL(opts.url); - const req = http.request( + const driver = parsed.protocol === 'https:' ? https : http; + const req = driver.request( { hostname: parsed.hostname, - port: parsed.port, + port: parsed.port || (parsed.protocol === 'https:' ? 443 : 80), path: parsed.pathname + parsed.search, method: opts.method, headers: opts.headers, diff --git a/src/mcplocal/tests/smoke/system-prompts.test.ts b/src/mcplocal/tests/smoke/system-prompts.test.ts index dd878a1..25fdad4 100644 --- a/src/mcplocal/tests/smoke/system-prompts.test.ts +++ b/src/mcplocal/tests/smoke/system-prompts.test.ts @@ -9,28 +9,13 @@ */ import { describe, it, expect, beforeAll } from 'vitest'; import http from 'node:http'; -import { readFileSync } from 'node:fs'; -import { join } from 'node:path'; -import { homedir } from 'node:os'; -import { isMcplocalRunning, getMcpdUrl } from './mcp-client.js'; +import https from 'node:https'; +import { isMcplocalRunning, loadMcpdAuth } from './mcp-client.js'; -const MCPD_URL = getMcpdUrl(); - -function loadMcpdCredentials(): { token: string; url: string } { - try { - const raw = readFileSync(join(homedir(), '.mcpctl', 'credentials'), 'utf-8'); - const parsed = JSON.parse(raw) as { token?: string; mcpdUrl?: string }; - return { - token: parsed.token ?? '', - url: parsed.mcpdUrl ?? MCPD_URL, - }; - } catch { - return { token: '', url: MCPD_URL }; - } -} - -const MCPD_CREDS = loadMcpdCredentials(); -const MCPD_EFFECTIVE_URL = MCPD_CREDS.url || MCPD_URL; +// URL from config.json, token from credentials (matches the CLI itself). +// See loadMcpdAuth() JSDoc for why credentials.mcpdUrl is intentionally ignored. +const MCPD_CREDS = loadMcpdAuth(); +const MCPD_EFFECTIVE_URL = MCPD_CREDS.url; interface Prompt { id: string; @@ -52,7 +37,8 @@ function mcpdRequest(method: string, path: string, body?: unknown): Promise<{ const bodyStr = body !== undefined ? JSON.stringify(body) : undefined; if (bodyStr) headers['Content-Length'] = String(Buffer.byteLength(bodyStr)); - const req = http.request(url, { method, timeout: 10_000, headers }, (res) => { + const driver = url.protocol === 'https:' ? https : http; + const req = driver.request(url, { method, timeout: 10_000, headers }, (res) => { const chunks: Buffer[] = []; res.on('data', (chunk: Buffer) => chunks.push(chunk)); res.on('end', () => {