From a7144d4e71b20d91e30fb62f69e099799433aa50 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 28 Jun 2026 20:16:45 -0400 Subject: [PATCH] fix: probe remote RBAC capability live and enforce exact stop-result membership (#1510) The cross-node capability gate cached its verdict, so a remote replaced by older code at the same URL stayed trusted until the cache expired, reopening the non-admin HTTP escalation and the over-broad stop. The probe now hits the remote's live /api/meta on every gated action (concurrent calls deduped, never cached across requests, fail-closed), so a downgraded remote is detected immediately. Two stop-result gaps are also closed: - A remote stop result must now cover exactly the confirmed stacks (one per stack, no extras, no omissions), not merely exclude extras, so a dropped confirmed stack is no longer accepted as clean. runLocalLabelStop reports one result per confirmed stack even when the label has vanished, so a current remote always satisfies the check. - The local stop exception path now reports the full confirmed set, so a confirmed stack that lost its label is not dropped when the local stop throws. --- .../fleet-action-card-endpoints.test.ts | 57 ++++++++++++++- backend/src/__tests__/fleet-actions.test.ts | 16 +++++ .../proxy-cross-node-rbac-gate.test.ts | 25 ++++++- .../src/__tests__/remote-capabilities.test.ts | 57 ++++++++++----- backend/src/helpers/fleetLabelStop.ts | 14 +++- backend/src/helpers/remoteCapabilities.ts | 72 +++++++++++-------- backend/src/routes/fleet.ts | 31 +++++--- 7 files changed, 210 insertions(+), 62 deletions(-) diff --git a/backend/src/__tests__/fleet-action-card-endpoints.test.ts b/backend/src/__tests__/fleet-action-card-endpoints.test.ts index f622cdbd..e14956fc 100644 --- a/backend/src/__tests__/fleet-action-card-endpoints.test.ts +++ b/backend/src/__tests__/fleet-action-card-endpoints.test.ts @@ -14,6 +14,7 @@ import jwt from 'jsonwebtoken'; import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_JWT_SECRET } from './helpers/setupTestDb'; let mockFsStacks: string[] = []; +let mockFsStacksError: Error | null = null; const pruneManagedOnly = vi.fn(); const pruneSystem = vi.fn(); const estimateManagedReclaim = vi.fn(); @@ -31,7 +32,10 @@ vi.mock('../helpers/remoteCapabilities', () => ({ vi.mock('../services/FileSystemService', () => ({ FileSystemService: { getInstance: vi.fn(() => ({ - getStacks: vi.fn(async () => mockFsStacks), + getStacks: vi.fn(async () => { + if (mockFsStacksError) throw mockFsStacksError; + return mockFsStacks; + }), })), }, })); @@ -81,6 +85,7 @@ beforeEach(() => { vi.restoreAllMocks(); vi.clearAllMocks(); mockFsStacks = ['alpha', 'beta']; + mockFsStacksError = null; pruneManagedOnly.mockResolvedValue({ success: true, reclaimedBytes: 0 }); pruneSystem.mockResolvedValue({ success: true, reclaimedBytes: 0 }); estimateManagedReclaim.mockResolvedValue({ reclaimableBytes: 0 }); @@ -878,7 +883,7 @@ describe('POST /api/fleet/labels/fleet-stop confirmed-target allowlist', () => { } }); - it('fails a node whose stop results name stacks outside the confirmed set', async () => { + it('fails a node whose stop results include a stack outside the confirmed set', async () => { remoteSupportsCrossNodeRbac.mockResolvedValue(true); const remoteId = db.addNode({ name: 'lying-remote', type: 'remote', api_url: 'http://lying.example:1852', @@ -902,11 +907,57 @@ describe('POST /api/fleet/labels/fleet-stop confirmed-target allowlist', () => { expect(res.status).toBe(200); const node = res.body.results.find((r: { nodeId: number }) => r.nodeId === remoteId); expect(node.reachable).toBe(false); - expect(node.error).toMatch(/outside the confirmed set/i); + expect(node.error).toMatch(/exactly the confirmed stacks/i); } finally { db.deleteNode(remoteId); } }); + + it('fails a node whose stop results omit a confirmed stack (partial result)', async () => { + remoteSupportsCrossNodeRbac.mockResolvedValue(true); + const remoteId = db.addNode({ + name: 'dropping-remote', type: 'remote', api_url: 'http://dropping.example:1852', + api_token: 'tok', compose_dir: '/app/compose', is_default: false, + }); + try { + vi.spyOn(globalThis, 'fetch').mockResolvedValue({ + ok: true, status: 200, + // Two stacks confirmed, but the remote reports only one. + json: async () => ({ matched: true, results: [{ stackName: 'alpha', success: true }] }), + } as unknown as Response); + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', authHeader) + .send({ labelName: 'any-label', targets: [{ nodeId: remoteId, stackNames: ['alpha', 'beta'] }] }); + expect(res.status).toBe(200); + const node = res.body.results.find((r: { nodeId: number }) => r.nodeId === remoteId); + expect(node.reachable).toBe(false); + expect(node.error).toMatch(/exactly the confirmed stacks/i); + } finally { + db.deleteNode(remoteId); + } + }); + + it('local exception path reports every confirmed stack, including one that lost its label', async () => { + // runLocalLabelStop throws (filesystem read fails) after the label is + // resolved; the catch must report the full confirmed set, not the current + // assignment, so a confirmed stack that lost its label still surfaces. + const localNodeId = db.getNodes().find(n => n.type === 'local')!.id; + const label = await createAssignedLabel('local-throw', ['alpha']); // only alpha is still assigned + expect(label.node_id).toBe(localNodeId); + mockFsStacksError = new Error('compose dir unreadable'); + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', authHeader) + .send({ labelName: label.name, targets: [{ nodeId: localNodeId, stackNames: ['alpha', 'lost-label-stack'] }] }); + expect(res.status).toBe(200); + const node = res.body.results.find((r: { nodeId: number }) => r.nodeId === localNodeId); + const byName = Object.fromEntries(node.stackResults.map((s: { stackName: string }) => [s.stackName, s])); + // Both confirmed stacks are failed; the one that lost its label is not dropped. + expect(byName['alpha']).toMatchObject({ success: false }); + expect(byName['lost-label-stack']).toMatchObject({ success: false }); + expect(node.stackResults).toHaveLength(2); + }); }); describe('POST /api/fleet/labels/fleet-stop with dryRun: true', () => { diff --git a/backend/src/__tests__/fleet-actions.test.ts b/backend/src/__tests__/fleet-actions.test.ts index d7261865..a1f74bf5 100644 --- a/backend/src/__tests__/fleet-actions.test.ts +++ b/backend/src/__tests__/fleet-actions.test.ts @@ -420,6 +420,22 @@ describe('local-stop behavior', () => { expect(res.status).toBe(200); expect(res.body.results).toEqual([{ stackName: 'ghostdisk-stack', success: false, error: 'Stack not found on this node' }]); }); + + it('reports one failure per confirmed stack when the label no longer exists', async () => { + // The label vanished between preview and execution. With a confirmed + // allowlist the receiver must return one result per confirmed stack (so the + // control's exact-membership check sees a complete set), not an empty body. + const res = await request(app) + .post('/api/fleet-actions/labels/local-stop') + .set('Authorization', authHeader) + .send({ labelName: 'no-such-label', dryRun: true, stackNames: ['gone-a', 'gone-b'] }); + expect(res.status).toBe(200); + expect(res.body.matched).toBe(false); + expect(res.body.results).toEqual([ + { stackName: 'gone-a', success: false, error: 'No longer carries this label' }, + { stackName: 'gone-b', success: false, error: 'No longer carries this label' }, + ]); + }); }); // Orchestrator-level binding: the control sends the exact node + stack list the diff --git a/backend/src/__tests__/proxy-cross-node-rbac-gate.test.ts b/backend/src/__tests__/proxy-cross-node-rbac-gate.test.ts index 10e5e4f8..1c16f1e8 100644 --- a/backend/src/__tests__/proxy-cross-node-rbac-gate.test.ts +++ b/backend/src/__tests__/proxy-cross-node-rbac-gate.test.ts @@ -23,8 +23,9 @@ let noCapNodeId: number; const PROXIED_PATH = '/api/stacks'; -function metaServer(capabilities: string[]): http.Server { +function metaServer(capabilities: string[], seen?: string[]): http.Server { return http.createServer((req, res) => { + if (seen && req.url) seen.push(req.url); res.writeHead(200, { 'Content-Type': 'application/json' }); if (req.url?.startsWith('/api/meta')) { res.end(JSON.stringify({ version: '0.93.0', capabilities })); @@ -35,6 +36,8 @@ function metaServer(capabilities: string[]): http.Server { }); } +const noCapPaths: string[] = []; + async function listen(server: http.Server): Promise { await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); return (server.address() as import('net').AddressInfo).port; @@ -53,7 +56,7 @@ beforeAll(async () => { adminBearer = jwt.sign({ username: TEST_USERNAME }, TEST_JWT_SECRET, { expiresIn: '1m' }); capServer = metaServer(['cross-node-rbac']); - noCapServer = metaServer(['fleet']); // older remote: no cross-node-rbac + noCapServer = metaServer(['fleet'], noCapPaths); // older remote: no cross-node-rbac const capPort = await listen(capServer); const noCapPort = await listen(noCapServer); @@ -99,4 +102,22 @@ describe('remote proxy cross-node-rbac gate', () => { .set('x-node-id', String(noCapNodeId)); expect(res.status).not.toBe(403); }); + + it('refuses a real stop to a remote lacking cross-node-rbac via the live probe, never contacting its local-stop receiver', async () => { + // Exercises the fleet-stop gate end to end through the REAL capability + // helper (not a mock): the live /api/meta probe of the no-cap remote returns + // no capability, so the stop is refused and the destructive receiver on that + // remote is never contacted. + noCapPaths.length = 0; + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', `Bearer ${adminBearer}`) + .send({ labelName: 'any-label', targets: [{ nodeId: noCapNodeId, stackNames: ['x'] }] }); + expect(res.status).toBe(200); + const node = res.body.results.find((r: { nodeId: number }) => r.nodeId === noCapNodeId); + expect(node.reachable).toBe(false); + expect(node.error).toMatch(/upgrade/i); + expect(noCapPaths.some(p => p.startsWith('/api/meta'))).toBe(true); + expect(noCapPaths.some(p => p.includes('/api/fleet-actions/labels/local-stop'))).toBe(false); + }); }); diff --git a/backend/src/__tests__/remote-capabilities.test.ts b/backend/src/__tests__/remote-capabilities.test.ts index 9868fc78..1abc1cdc 100644 --- a/backend/src/__tests__/remote-capabilities.test.ts +++ b/backend/src/__tests__/remote-capabilities.test.ts @@ -1,8 +1,9 @@ /** * Unit coverage for the cross-node-rbac capability probe used to gate - * mixed-version cross-node operations. It reads the shared remote-meta cache - * (fetching once on a cold miss) and must fail closed: a node whose capability - * cannot be determined is treated as unsupported. + * mixed-version cross-node operations. It probes the remote's live /api/meta on + * every call (no cross-request caching), must fail closed when the capability + * cannot be determined, and must re-verify each time so a downgraded remote is + * detected immediately rather than trusted from a stale verdict. */ import { describe, it, expect, beforeAll, afterAll, afterEach, vi } from 'vitest'; import { setupTestDb, cleanupTestDb } from './helpers/setupTestDb'; @@ -10,7 +11,6 @@ import type { RemoteMeta } from '../services/CapabilityRegistry'; let remoteSupportsCrossNodeRbac: typeof import('../helpers/remoteCapabilities').remoteSupportsCrossNodeRbac; let NodeRegistry: typeof import('../services/NodeRegistry').NodeRegistry; -let CacheService: typeof import('../services/CacheService').CacheService; let tmpDir: string; const NODE_ID = 4242; @@ -19,40 +19,63 @@ beforeAll(async () => { tmpDir = await setupTestDb(); ({ remoteSupportsCrossNodeRbac } = await import('../helpers/remoteCapabilities')); ({ NodeRegistry } = await import('../services/NodeRegistry')); - ({ CacheService } = await import('../services/CacheService')); }); afterAll(() => cleanupTestDb(tmpDir)); -afterEach(() => { - vi.restoreAllMocks(); - CacheService.getInstance().invalidate(`remote-meta:${NODE_ID}`); -}); - -function mockMeta(meta: RemoteMeta): void { - vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode').mockResolvedValue(meta); -} +afterEach(() => vi.restoreAllMocks()); const ONLINE = { startedAt: null, updateError: null, online: true } as const; +const capable: RemoteMeta = { version: '0.93.0', capabilities: ['fleet', 'cross-node-rbac'], ...ONLINE }; +const incapable: RemoteMeta = { version: '0.92.0', capabilities: ['fleet', 'labels'], ...ONLINE }; describe('remoteSupportsCrossNodeRbac', () => { it('returns true when the remote advertises cross-node-rbac', async () => { - mockMeta({ version: '0.93.0', capabilities: ['fleet', 'cross-node-rbac'], ...ONLINE }); + vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode').mockResolvedValue(capable); expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(true); }); it('returns false when the remote does not advertise it', async () => { - mockMeta({ version: '0.92.0', capabilities: ['fleet', 'labels'], ...ONLINE }); + vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode').mockResolvedValue(incapable); expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(false); }); - it('fails closed when the remote meta has no resolvable version', async () => { - mockMeta({ version: null, capabilities: [], startedAt: null, updateError: null, online: false }); + it('fails closed when the remote is offline (empty capabilities)', async () => { + vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode') + .mockResolvedValue({ version: null, capabilities: [], startedAt: null, updateError: null, online: false }); expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(false); }); + it('trusts a reachable remote that advertises the capability even with a non-semver version', async () => { + // A 0.0.0-dev image reports version null (non-semver) but is reachable and + // genuinely advertises the capability; it must not be wrongly denied. + vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode') + .mockResolvedValue({ version: null, capabilities: ['fleet', 'cross-node-rbac'], startedAt: null, updateError: null, online: true }); + expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(true); + }); + it('fails closed when the meta fetch throws (unreachable)', async () => { vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode').mockRejectedValue(new Error('unreachable')); expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(false); }); + + it('re-probes on every call so a downgraded remote is detected immediately (no stale verdict)', async () => { + const spy = vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode') + .mockResolvedValueOnce(capable) // first probe: current remote + .mockResolvedValue(incapable); // after the remote is swapped for older code + expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(true); + expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(false); + expect(spy).toHaveBeenCalledTimes(2); + }); + + it('dedupes concurrent probes for the same node into a single fetch', async () => { + const spy = vi.spyOn(NodeRegistry.getInstance(), 'fetchMetaForNode').mockResolvedValue(capable); + const [a, b] = await Promise.all([ + remoteSupportsCrossNodeRbac(NODE_ID), + remoteSupportsCrossNodeRbac(NODE_ID), + ]); + expect(a).toBe(true); + expect(b).toBe(true); + expect(spy).toHaveBeenCalledTimes(1); + }); }); diff --git a/backend/src/helpers/fleetLabelStop.ts b/backend/src/helpers/fleetLabelStop.ts index 14a83378..47743d4f 100644 --- a/backend/src/helpers/fleetLabelStop.ts +++ b/backend/src/helpers/fleetLabelStop.ts @@ -79,7 +79,19 @@ export async function runLocalLabelStop( ): Promise { const db = DatabaseService.getInstance(); const label = db.getLabels(nodeId).find(l => l.name === labelName); - if (!label) return { matched: false, stackResults: [] }; + if (!label) { + // With a confirmed allowlist, report each confirmed stack as a failure + // rather than an empty result: the label vanished here between preview and + // execution, so none can be stopped, and the control's exact-membership + // check must still see one result per confirmed stack (not a silent no-op). + if (allowedStacks) { + return { + matched: false, + stackResults: [...allowedStacks].map(stackName => ({ stackName, success: false, error: 'No longer carries this label' })), + }; + } + return { matched: false, stackResults: [] }; + } const stackNames = db.getStacksForLabel(label.id, nodeId); // With no confirmed allowlist this is the unbound path: an unassigned label is // a clean no-op. When stacks were confirmed we fall through so any that left diff --git a/backend/src/helpers/remoteCapabilities.ts b/backend/src/helpers/remoteCapabilities.ts index 5076939e..ea336610 100644 --- a/backend/src/helpers/remoteCapabilities.ts +++ b/backend/src/helpers/remoteCapabilities.ts @@ -1,44 +1,54 @@ -import { CacheService } from '../services/CacheService'; import { NodeRegistry } from '../services/NodeRegistry'; -import { CROSS_NODE_RBAC_CAPABILITY, type RemoteMeta } from '../services/CapabilityRegistry'; -import { REMOTE_META_NAMESPACE } from './cacheInvalidation'; +import { CROSS_NODE_RBAC_CAPABILITY } from '../services/CapabilityRegistry'; import { getErrorMessage } from '../utils/errors'; -// Mirrors the node-meta endpoint's TTL and shares its `remote-meta:` cache -// key, so a recent /api/nodes/:id/meta read warms this check and vice versa. -const REMOTE_META_CACHE_TTL = 3 * 60 * 1000; +// In-flight probes deduped per node so a burst of concurrent gated requests +// shares one /api/meta round-trip. The entry is dropped as soon as it settles, +// so the NEXT request re-probes. The verdict is deliberately NOT cached across +// requests: a remote can be replaced by older code at the same URL (a rollback +// or image pin), and a stale "supported" verdict would reopen the cross-node +// escalation, so each gated action re-verifies against the live remote. +const inFlight = new Map>(); /** * Whether a remote node advertises that it enforces cross-node RBAC: the * forwarded actor role on HTTP requests and the exact-stack allowlist on - * stop-by-label. Reads the shared remote-meta cache, fetching once on a cold - * miss. + * stop-by-label. Probes the remote's live /api/meta on every call (concurrent + * calls for the same node share one probe). * - * Fails closed: when the capability cannot be established (an un-upgraded - * remote, or a cold cache whose meta fetch fails) it returns false, so the - * caller denies rather than risk escalating a non-admin request or over-stopping - * on an un-upgraded node. A node previously cached as supported may be served - * that value while a later refresh is failing (getOrFetch serves stale on - * error); that is safe because capabilities are append-only and a request to an - * unreachable node fails at the transport regardless. + * Fails closed: a remote that does not advertise the capability, that cannot be + * read (offline/unreachable, which yields empty capabilities), or that errors + * is treated as unsupported, so the caller denies rather than risk escalating a + * non-admin request or over-stopping. Because the probe is live, a remote + * downgraded to older code is detected on the next gated action rather than + * trusted until a cache expires. */ export async function remoteSupportsCrossNodeRbac(nodeId: number): Promise { + const existing = inFlight.get(nodeId); + if (existing) return existing; + + const probe = (async (): Promise => { + try { + const meta = await NodeRegistry.getInstance().fetchMetaForNode(nodeId); + // Check the advertised capability directly. An offline/unreadable remote + // yields OFFLINE_META with empty capabilities, so this already fails + // closed; keying off the capability (not the version) also correctly + // trusts a reachable remote whose version string is non-semver, e.g. a + // 0.0.0-dev image, but that genuinely advertises the capability. + return meta.capabilities.includes(CROSS_NODE_RBAC_CAPABILITY); + } catch (err) { + console.warn( + `[CrossNodeRBAC] Could not verify capability for node ${nodeId}; treating as unsupported:`, + getErrorMessage(err, 'unknown'), + ); + return false; + } + })(); + + inFlight.set(nodeId, probe); try { - const meta = await CacheService.getInstance().getOrFetch( - `${REMOTE_META_NAMESPACE}:${nodeId}`, - REMOTE_META_CACHE_TTL, - async () => { - const fetched = await NodeRegistry.getInstance().fetchMetaForNode(nodeId); - if (fetched.version === null) throw new Error('Remote meta fetch returned null version'); - return fetched; - }, - ); - return meta.capabilities.includes(CROSS_NODE_RBAC_CAPABILITY); - } catch (err) { - console.warn( - `[CrossNodeRBAC] Could not determine capability for node ${nodeId}; treating as unsupported:`, - getErrorMessage(err, 'unknown'), - ); - return false; + return await probe; + } finally { + inFlight.delete(nodeId); } } diff --git a/backend/src/routes/fleet.ts b/backend/src/routes/fleet.ts index f6bc223c..f11094aa 100644 --- a/backend/src/routes/fleet.ts +++ b/backend/src/routes/fleet.ts @@ -1447,8 +1447,13 @@ fleetRouter.post('/labels/fleet-stop', authMiddleware, async (req: Request, res: } catch (err) { const errorMsg = getErrorMessage(err, 'Failed to stop local stacks'); const localLabel = db.getLabels(node.id).find(l => l.name === trimmed); - const localStacks = (localLabel ? db.getStacksForLabel(localLabel.id, node.id) : []) - .filter(s => !allowedStacks || allowedStacks.has(s)); + // With a confirmed allowlist, report the full confirmed set as + // failures rather than the current label assignment: a confirmed stack + // that lost its label after the preview must still surface, not vanish + // because it is no longer assigned when the exception is reconstructed. + const localStacks = allowedStacks + ? [...allowedStacks] + : (localLabel ? db.getStacksForLabel(localLabel.id, node.id) : []); return { nodeId: node.id, nodeName: node.name, reachable: true, matched: !!localLabel, stackResults: failAllStacks(localStacks, errorMsg), @@ -1508,12 +1513,22 @@ fleetRouter.post('/labels/fleet-stop', authMiddleware, async (req: Request, res: return { nodeId: node.id, nodeName: node.name, reachable: false, matched: false, stackResults: [], error: 'Remote returned a malformed response' }; } // Defense in depth behind the capability gate: if a confirmed allowlist - // was sent, the remote must report only stacks from it. A result naming - // a stack outside the confirmed set means the remote ignored the - // allowlist (an over-broad stop), so fail the node rather than render - // the extra stops as a clean result. - if (allowedStacks && !remote.results.every(r => allowedStacks.has(r.stackName))) { - return { nodeId: node.id, nodeName: node.name, reachable: false, matched: false, stackResults: [], error: 'Remote stopped stacks outside the confirmed set' }; + // was sent, the remote must report exactly the confirmed set: one result + // per confirmed stack, no extras, no duplicates. Extras mean the remote + // ignored the allowlist (an over-broad stop); a missing confirmed stack + // means it silently dropped one. Either way fail the node rather than + // render an over-broad or partial result as clean. A current remote + // always reconciles to one result per confirmed stack, so this only + // rejects an older or misbehaving receiver. + if (allowedStacks) { + const returned = remote.results.map(r => r.stackName); + const returnedSet = new Set(returned); + const exact = returned.length === allowedStacks.size + && returnedSet.size === returned.length + && [...allowedStacks].every(s => returnedSet.has(s)); + if (!exact) { + return { nodeId: node.id, nodeName: node.name, reachable: false, matched: false, stackResults: [], error: 'Remote did not report exactly the confirmed stacks' }; + } } return { nodeId: node.id, nodeName: node.name, reachable: true,