From 9fb4ccccff7cc7b32a595f6f869717fba697dfa6 Mon Sep 17 00:00:00 2001 From: Anso Date: Mon, 1 Jun 2026 19:47:06 -0400 Subject: [PATCH] fix(fleet-secrets): restrict bundle management to admin hub sessions (#1274) * fix(fleet-secrets): restrict bundle management to admin hub sessions Fleet Secrets exposes decrypted environment-variable values and writes credentials across the fleet, so every route now requires an admin role, runs only on the instance you are signed into, and rejects long-lived API tokens: - Add an admin-role check to all secrets routes; the frontend Secrets tab renders only for admin users so the affordance matches the backend gate. - Add /api/secrets/ to the hub-only path list so a request carrying a remote node id cannot be proxied to read another node's decrypted values. - Reject API tokens on every secrets route (browser admin sessions only), matching how registry credentials are handled. Also adds lifecycle and developer-mode diagnostic logging (never the secret values) and tests covering the admin boundary on every endpoint, API-token rejection, hub-only enforcement, and diagnostic gating. * fix(fleet-secrets): require a signed-in user session for all secrets routes The earlier API-token rejection only blocked opaque API tokens. node_proxy and pilot_tunnel JWTs are mapped to an admin role by the auth middleware without an API-token scope, so they still passed the admin gate and could read decrypted bundles via GET /api/secrets/:id. Replace the API-token check with requireUserSession, which rejects API tokens and node_proxy / pilot_tunnel machine credentials (userId 0) on every secrets route, returning SESSION_REQUIRED. The admin role is still enforced after. Tests now assert SESSION_REQUIRED for a full-admin API token across all nine routes and for node_proxy and pilot_tunnel JWTs. * test(fleet-secrets): mint the rejection-test token via the real endpoint The machine-credential test reconstructed an API token by sha256-hashing a raw key inline. That duplicated a hashing sink that CodeQL's js/insufficient-password-hash query flags (a false positive for a 256-bit random token, but a new occurrence in the diff). Create the token through POST /api/api-tokens instead, so the hashing stays in the production path and the test carries none of its own. Behavior and coverage are unchanged. --- backend/src/__tests__/hub-only-guard.test.ts | 25 ++++ backend/src/__tests__/secrets.test.ts | 146 ++++++++++++++++++- backend/src/helpers/proxyExemptPaths.ts | 8 +- backend/src/middleware/tierGates.ts | 14 ++ backend/src/routes/secrets.ts | 32 +++- backend/src/services/SecretsService.ts | 7 + frontend/src/components/FleetView.tsx | 4 +- 7 files changed, 227 insertions(+), 9 deletions(-) diff --git a/backend/src/__tests__/hub-only-guard.test.ts b/backend/src/__tests__/hub-only-guard.test.ts index e30ab9e2..6028af5b 100644 --- a/backend/src/__tests__/hub-only-guard.test.ts +++ b/backend/src/__tests__/hub-only-guard.test.ts @@ -201,6 +201,31 @@ describe('hubOnlyGuard', () => { expect(res.body?.code).not.toBe('HUB_ONLY_ENDPOINT'); }); + // Regression for Fleet Secrets: bundles are stored and decrypted per instance, + // and GET /api/secrets/:id returns plaintext values. A proxied secrets request + // would read a remote node's decrypted values as the node-proxy admin, and the + // routes' own requireAdmin gate lives in the local handler the proxy skips. + // Cover the collection (no trailing slash) and a sub-path. + it('rejects /api/secrets with 403 when nodeId targets a remote node', async () => { + const res = await request(app) + .get('/api/secrets') + .set('Authorization', authHeader) + .set('x-node-id', String(remoteNodeId)); + + expect(res.status).toBe(403); + expect(res.body?.code).toBe('HUB_ONLY_ENDPOINT'); + }); + + it('rejects a secrets sub-path (/api/secrets/5) with 403 when nodeId targets a remote node', async () => { + const res = await request(app) + .get('/api/secrets/5') + .set('Authorization', authHeader) + .set('x-node-id', String(remoteNodeId)); + + expect(res.status).toBe(403); + expect(res.body?.code).toBe('HUB_ONLY_ENDPOINT'); + }); + it('does not interfere with non-hub paths even when nodeId targets a remote node', async () => { // /api/stacks is not hub-only and should be forwarded by the proxy. // The exact upstream-error status is not the contract here; what diff --git a/backend/src/__tests__/secrets.test.ts b/backend/src/__tests__/secrets.test.ts index 22b25ab5..b6357f4a 100644 --- a/backend/src/__tests__/secrets.test.ts +++ b/backend/src/__tests__/secrets.test.ts @@ -5,10 +5,12 @@ * - encrypt round-trip via CryptoService * - DatabaseService secret + version + push CRUD * - SecretsService versioning, importFromStack, executePush aggregation - * - Route guards (requirePaid 403, push lock 409) + * - Route guards (requirePaid 403, requireAdmin 403, requireUserSession 403, push lock 409) + * - Hub-only enforcement is covered in hub-only-guard.test.ts + * - developer_mode diagnostics gating (and that diagnostics never log the secret value) * - getAuditSummary patterns for /secrets routes */ -import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach, vi } from 'vitest'; import request from 'supertest'; import path from 'path'; import fs from 'fs'; @@ -42,6 +44,30 @@ function clearSecretsTables(): void { db.prepare('DELETE FROM secrets').run(); } +// Shared by the admin-role and machine-credential matrices: one representative +// call per secrets endpoint. requireUserSession and requireAdmin both run before +// requireBody and param parsing, so a bodyless request still surfaces the guard. +const SECRET_ENDPOINTS: Array<[string, string]> = [ + ['get', '/api/secrets'], + ['post', '/api/secrets'], + ['get', '/api/secrets/1'], + ['put', '/api/secrets/1'], + ['delete', '/api/secrets/1'], + ['get', '/api/secrets/1/versions'], + ['post', '/api/secrets/1/import-from-stack'], + ['post', '/api/secrets/1/push/preview'], + ['post', '/api/secrets/1/push'], +]; + +function callWithToken(method: string, p: string, token: string) { + const agent = request(app); + const r = method === 'get' ? agent.get(p) + : method === 'post' ? agent.post(p) + : method === 'put' ? agent.put(p) + : agent.delete(p); + return r.set('Authorization', `Bearer ${token}`); +} + beforeAll(async () => { tmpDir = await setupTestDb(); ({ DatabaseService } = await import('../services/DatabaseService')); @@ -399,3 +425,119 @@ describe('Routes /api/secrets tier gating and lock', () => { expect(res.status).toBe(400); }); }); + +// ---- Admin-role gating: secrets reveal decrypted values, so every route is admin-only ---- + +describe('Routes /api/secrets admin-role gating', () => { + // authMiddleware resolves the role from the DB (not the JWT), so a real + // non-admin user must exist for the gate to see a non-admin role. + function viewerToken(): string { + const db = DatabaseService.getInstance(); + let user = db.getUserByUsername('sec-viewer'); + if (!user) { + db.addUser({ username: 'sec-viewer', password_hash: 'x', role: 'viewer' }); + user = db.getUserByUsername('sec-viewer')!; + } + return authToken('sec-viewer', 'viewer', user.token_version); + } + + it.each(SECRET_ENDPOINTS)('403s a non-admin paid user on %s %s', async (method, p) => { + const res = await callWithToken(method, p, viewerToken()); + expect(res.status).toBe(403); + expect(res.body.code).toBe('ADMIN_REQUIRED'); + }); + + it('lets an admin paid user list (200)', async () => { + const res = await request(app) + .get('/api/secrets') + .set('Authorization', `Bearer ${adminToken()}`); + expect(res.status).toBe(200); + }); +}); + +// ---- Machine-credential rejection: secrets need a real signed-in user session ---- + +describe('Routes /api/secrets machine-credential rejection', () => { + // A full-admin API token resolves to role 'admin' and would otherwise pass + // requireAdmin and reach the decrypted-value GET. requireUserSession runs + // first and blocks it. Mint the token through the real endpoint so the test + // exercises the production creation path and carries no hashing of its own. + let fullAdminToken: string; + beforeAll(async () => { + const res = await request(app) + .post('/api/api-tokens') + .set('Authorization', `Bearer ${adminToken()}`) + .send({ name: `secrets-rejection-${Date.now()}`, scope: 'full-admin' }); + fullAdminToken = res.body.token as string; + }); + + // node_proxy / pilot_tunnel JWTs are signed with this instance's secret and + // map to { username: 'node-proxy', role: 'admin', userId: 0 } in authMiddleware. + // They carry no apiTokenScope, so only the userId-0 check blocks them. + function machineJwt(scope: 'node_proxy' | 'pilot_tunnel'): string { + return jwt.sign({ scope }, TEST_JWT_SECRET, { expiresIn: '1m' }); + } + + it.each(SECRET_ENDPOINTS)('403s a full-admin API token on %s %s with SESSION_REQUIRED', async (method, p) => { + const res = await callWithToken(method, p, fullAdminToken); + expect(res.status).toBe(403); + expect(res.body.code).toBe('SESSION_REQUIRED'); + }); + + it.each([ + ['node_proxy', '/api/secrets'], + ['node_proxy', '/api/secrets/1'], + ['pilot_tunnel', '/api/secrets/1'], + ] as const)('403s a %s JWT on %s with SESSION_REQUIRED', async (scope, p) => { + const res = await request(app) + .get(p) + .set('Authorization', `Bearer ${machineJwt(scope)}`); + expect(res.status).toBe(403); + expect(res.body.code).toBe('SESSION_REQUIRED'); + }); +}); + +// ---- developer_mode diagnostics gating ---- + +describe('SecretsService.executePush developer_mode diagnostics', () => { + beforeEach(() => { + const composeDir = process.env.COMPOSE_DIR!; + const stackDir = path.join(composeDir, 'devmodestack'); + fs.mkdirSync(stackDir, { recursive: true }); + fs.writeFileSync(path.join(stackDir, '.env'), 'EXISTING=keep\n'); + fs.writeFileSync(path.join(stackDir, 'compose.yaml'), 'services:\n app:\n image: nginx\n'); + }); + + afterEach(() => { + DatabaseService.getInstance().updateGlobalSetting('developer_mode', '0'); + }); + + async function runPush(name: string): Promise { + const db = DatabaseService.getInstance(); + const localNode = db.getNodes().find(n => n.type === 'local')!; + const svc = SecretsService.getInstance(); + const { id } = svc.create({ name, kv: { TOKEN: 'supersecretvalue' }, user: TEST_USERNAME }); + await svc.executePush(id, { type: 'nodes', ids: [localNode.id] }, 'devmodestack', '.env', TEST_USERNAME); + } + + it('emits [Secrets:diag] only when developer_mode is on, and never the secret value', async () => { + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined); + try { + DatabaseService.getInstance().updateGlobalSetting('developer_mode', '0'); + await runPush('devmode-off'); + const offLogs = logSpy.mock.calls.map(c => c.join(' ')); + expect(offLogs.some(l => l.includes('[Secrets:diag]'))).toBe(false); + + logSpy.mockClear(); + DatabaseService.getInstance().updateGlobalSetting('developer_mode', '1'); + await runPush('devmode-on'); + const onLogs = logSpy.mock.calls.map(c => c.join(' ')); + expect(onLogs.some(l => l.includes('[Secrets:diag]'))).toBe(true); + + // Diagnostics summarize counts only; the decrypted value must never appear. + expect([...offLogs, ...onLogs].some(l => l.includes('supersecretvalue'))).toBe(false); + } finally { + logSpy.mockRestore(); + } + }); +}); diff --git a/backend/src/helpers/proxyExemptPaths.ts b/backend/src/helpers/proxyExemptPaths.ts index 5eae1c57..7e529c62 100644 --- a/backend/src/helpers/proxyExemptPaths.ts +++ b/backend/src/helpers/proxyExemptPaths.ts @@ -32,9 +32,10 @@ export function isProxyExemptPath(path: string): boolean { // nodeId resolves to a remote node so a script/curl call cannot trick the proxy // into forwarding the request across a node boundary. This matters for the logs // feed (its admin gate lives in the local route handler, which the proxy would -// skip when forwarding a remote nodeId) and for registries (a proxied registry -// request would otherwise carry a plaintext secret to, or read stored -// credentials from, the remote). +// skip when forwarding a remote nodeId) and for registries and Fleet Secrets (a +// proxied request would otherwise carry a plaintext secret to, or read stored +// secret values from, the remote; the secrets routes' admin gate also lives in +// the local route handler, which the proxy would skip). // // Entries are stored with a trailing slash; the matcher accepts the exact // collection path (without the trailing slash) AND any sub-path under it, @@ -54,6 +55,7 @@ export const HUB_ONLY_PREFIXES: readonly string[] = [ '/api/logs/global/', '/api/system/log-stream-metrics/', '/api/registries/', + '/api/secrets/', ]; /** Returns true when the path is hub-only and must not be proxied to a remote node. */ diff --git a/backend/src/middleware/tierGates.ts b/backend/src/middleware/tierGates.ts index f4e7d6fd..e0fb8c3d 100644 --- a/backend/src/middleware/tierGates.ts +++ b/backend/src/middleware/tierGates.ts @@ -50,6 +50,20 @@ export const requireAdmin = (req: Request, res: Response): boolean => { return true; }; +/** + * Require a genuine signed-in user session. Rejects opaque API tokens + * (`req.apiTokenScope`) and node_proxy / pilot_tunnel machine credentials, + * which authMiddleware maps to role 'admin' with userId 0. Endpoints that + * expose decrypted secrets use this so a long-lived machine credential cannot + * reach them; the admin role itself is still enforced separately by requireAdmin. + */ +export const requireUserSession = (req: Request, res: Response): boolean => { + if (req.apiTokenScope || !req.user || req.user.userId === 0) { + return deny(res, 'SESSION_REQUIRED', 'This action requires a signed-in user session.'); + } + return true; +}; + /** * Accept only calls from a sibling Sencho using its node_proxy Bearer token. * Browser sessions, API tokens, and console tokens are all rejected. diff --git a/backend/src/routes/secrets.ts b/backend/src/routes/secrets.ts index 6919a1f8..78c839f1 100644 --- a/backend/src/routes/secrets.ts +++ b/backend/src/routes/secrets.ts @@ -1,6 +1,6 @@ import { Router, type Request, type Response } from 'express'; import { authMiddleware } from '../middleware/auth'; -import { requirePaid, requireBody } from '../middleware/tierGates'; +import { requirePaid, requireAdmin, requireUserSession, requireBody } from '../middleware/tierGates'; import { SecretsService, PushBusyError, type SecretKv } from '../services/SecretsService'; import { DatabaseService, type BlueprintSelector } from '../services/DatabaseService'; import { isValidStackName } from '../utils/validation'; @@ -10,6 +10,11 @@ import { sanitizeForLog } from '../utils/safeLog'; export const secretsRouter = Router(); +// Fleet Secrets reveals decrypted values and writes credentials fleet-wide, so +// every route requires a signed-in admin user. requireUserSession rejects API +// tokens and node_proxy / pilot_tunnel machine credentials (authMiddleware maps +// the latter to role 'admin'), then requireAdmin enforces the role. + const NAME_PATTERN = /^[a-zA-Z0-9][a-zA-Z0-9 _.-]{0,62}[a-zA-Z0-9]$/; function isValidName(name: unknown): name is string { @@ -60,7 +65,9 @@ function parsePushBody(body: unknown): PushBody | { error: string } { } secretsRouter.get('/', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; try { const items = SecretsService.getInstance().list(); res.json(items); @@ -71,7 +78,9 @@ secretsRouter.get('/', authMiddleware, async (req: Request, res: Response): Prom }); secretsRouter.post('/', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; if (!requireBody(req, res)) return; try { const { name, description, kv, note } = req.body as { name?: unknown; description?: unknown; kv?: unknown; note?: unknown }; @@ -98,6 +107,7 @@ secretsRouter.post('/', authMiddleware, async (req: Request, res: Response): Pro user: getActor(req), note: typeof note === 'string' ? note : undefined, }); + console.log(`[Secrets] Created bundle ${sanitizeForLog(name)} (v${result.version}) by ${sanitizeForLog(getActor(req))}`); res.status(201).json(result); } catch (err) { if (isSqliteUniqueViolation(err)) { @@ -110,7 +120,9 @@ secretsRouter.post('/', authMiddleware, async (req: Request, res: Response): Pro }); secretsRouter.get('/:id', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; try { const id = parseIntParam(req, res, 'id', 'secret ID'); if (id === null) return; @@ -128,7 +140,9 @@ secretsRouter.get('/:id', authMiddleware, async (req: Request, res: Response): P }); secretsRouter.put('/:id', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; if (!requireBody(req, res)) return; try { const id = parseIntParam(req, res, 'id', 'secret ID'); @@ -157,6 +171,7 @@ secretsRouter.put('/:id', authMiddleware, async (req: Request, res: Response): P user: getActor(req), note: typeof note === 'string' ? note : undefined, }); + console.log(`[Secrets] Updated bundle ${sanitizeForLog(existing.name)} to v${result.version} by ${sanitizeForLog(getActor(req))}`); res.json(result); } catch (err) { console.error('[Secrets] Update error:', err); @@ -165,7 +180,9 @@ secretsRouter.put('/:id', authMiddleware, async (req: Request, res: Response): P }); secretsRouter.delete('/:id', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; try { const id = parseIntParam(req, res, 'id', 'secret ID'); if (id === null) return; @@ -174,6 +191,7 @@ secretsRouter.delete('/:id', authMiddleware, async (req: Request, res: Response) res.status(404).json({ error: 'Secret not found' }); return; } + console.log(`[Secrets] Deleted bundle id=${id} by ${sanitizeForLog(getActor(req))}`); res.json({ ok: true }); } catch (err) { console.error('[Secrets] Delete error:', err); @@ -182,7 +200,9 @@ secretsRouter.delete('/:id', authMiddleware, async (req: Request, res: Response) }); secretsRouter.get('/:id/versions', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; try { const id = parseIntParam(req, res, 'id', 'secret ID'); if (id === null) return; @@ -198,7 +218,9 @@ secretsRouter.get('/:id/versions', authMiddleware, async (req: Request, res: Res }); secretsRouter.post('/:id/import-from-stack', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; if (!requireBody(req, res)) return; try { const id = parseIntParam(req, res, 'id', 'secret ID'); @@ -226,7 +248,9 @@ secretsRouter.post('/:id/import-from-stack', authMiddleware, async (req: Request }); secretsRouter.post('/:id/push/preview', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; if (!requireBody(req, res)) return; try { const id = parseIntParam(req, res, 'id', 'secret ID'); @@ -249,7 +273,9 @@ secretsRouter.post('/:id/push/preview', authMiddleware, async (req: Request, res }); secretsRouter.post('/:id/push', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requireUserSession(req, res)) return; if (!requirePaid(req, res)) return; + if (!requireAdmin(req, res)) return; if (!requireBody(req, res)) return; try { const id = parseIntParam(req, res, 'id', 'secret ID'); @@ -266,7 +292,9 @@ secretsRouter.post('/:id/push', authMiddleware, async (req: Request, res: Respon } try { const result = await SecretsService.getInstance().executePush(id, parsed.selector, parsed.stackName, parsed.envFileBasename, getActor(req)); - console.log(`[Secrets] Push ${sanitizeForLog(secret.name)} v${secret.current_version}: ${result.results.length} nodes`); + const failedCount = result.results.filter(r => r.status === 'failed').length; + console.log(`[Secrets] Push ${sanitizeForLog(secret.name)} v${secret.current_version}: ${result.results.length} node(s), ${failedCount} failed (by ${sanitizeForLog(getActor(req))})`); + if (failedCount > 0) console.warn(`[Secrets] Push ${sanitizeForLog(secret.name)} v${secret.current_version}: ${failedCount} node(s) failed`); res.json(result); } catch (err) { if (err instanceof PushBusyError) { diff --git a/backend/src/services/SecretsService.ts b/backend/src/services/SecretsService.ts index a9b627a6..927cf2d5 100644 --- a/backend/src/services/SecretsService.ts +++ b/backend/src/services/SecretsService.ts @@ -8,6 +8,7 @@ import { NodeRegistry } from './NodeRegistry'; import { resolveAllEnvFilePaths } from '../routes/stacks'; import { getErrorMessage } from '../utils/errors'; import { formatNoTargetError } from '../utils/remoteTarget'; +import { isDebugEnabled } from '../utils/debug'; export type SecretKv = Record; export type DiffStatus = 'added' | 'changed' | 'removed' | 'unchanged'; @@ -427,6 +428,7 @@ export class SecretsService { const db = DatabaseService.getInstance(); const overlay = this.getDecryptedKv(id); const matched = NodeLabelService.getInstance().matchSelector(selector, db.getNodes()); + if (isDebugEnabled()) console.log(`[Secrets:diag] preview id=${id} targets=${matched.length}`); // Preview is read-only and per-node; fan out in parallel for snappier wizard UX. return Promise.all(matched.map(async (node): Promise => { @@ -472,6 +474,7 @@ export class SecretsService { const overlay = decryptKv(versionRow.encrypted_payload); const matched = NodeLabelService.getInstance().matchSelector(selector, db.getNodes()); const pushId = crypto.randomUUID(); + if (isDebugEnabled()) console.log(`[Secrets:diag] push id=${id} v${versionRow.version} targets=${matched.length}`); const results: SecretPushResultEntry[] = []; const rows: Array[0][number]> = []; const pushedAt = Date.now(); @@ -526,6 +529,10 @@ export class SecretsService { } finally { activePushes.delete(id); } + if (isDebugEnabled()) { + const okCount = results.filter(r => r.status === 'ok').length; + console.log(`[Secrets:diag] push id=${id} done in ${Date.now() - pushedAt}ms ok=${okCount} failed=${results.length - okCount}`); + } return { pushId, results }; } } diff --git a/frontend/src/components/FleetView.tsx b/frontend/src/components/FleetView.tsx index 99b7b3a3..8c8d42b4 100644 --- a/frontend/src/components/FleetView.tsx +++ b/frontend/src/components/FleetView.tsx @@ -117,7 +117,7 @@ export function FleetView({ onNavigateToNode }: FleetViewProps) { Fleet Actions - {isPaid && ( + {isPaid && isAdmin && ( Secrets @@ -224,7 +224,7 @@ export function FleetView({ onNavigateToNode }: FleetViewProps) { - {isPaid && ( + {isPaid && isAdmin && (