fix: gate cross-node HTTP and stop-by-label on remote RBAC capability (#1509)

An older remote node ignores the forwarded actor-role header (running proxied
requests as admin) and ignores the stop-by-label stack allowlist (stopping
every label-matched stack). The control could neither detect nor prevent this
on a mixed-version fleet.

Instances now advertise a cross-node-rbac capability, and the control refuses
to act when a remote lacks it:

- HTTP proxy: a non-admin user's request is not forwarded to a remote that does
  not advertise the capability (fails closed when it cannot be determined).
  Admins are unaffected.
- Stop-by-label: a real stop bound to a confirmed stack set is not sent to a
  remote lacking the capability; the node is reported as needing an upgrade. As
  defense in depth, a node whose results name stacks outside the confirmed set
  is failed rather than rendered as a clean stop.

Separately, the stop's lock-contention path now reports every confirmed stack
as a contention failure (including one that lost its label), so a confirmed
stack is never silently dropped and the result is never empty.
This commit is contained in:
Anso
2026-06-28 18:28:47 -04:00
committed by GitHub
parent ef164f0e5b
commit 997a6bb79a
11 changed files with 365 additions and 1 deletions
@@ -22,6 +22,11 @@ const getContainersByStack = vi.fn();
const stopContainer = vi.fn();
const restartContainer = vi.fn();
const invalidateNodeCaches = vi.fn();
const remoteSupportsCrossNodeRbac = vi.fn();
vi.mock('../helpers/remoteCapabilities', () => ({
remoteSupportsCrossNodeRbac,
}));
vi.mock('../services/FileSystemService', () => ({
FileSystemService: {
@@ -83,6 +88,9 @@ beforeEach(() => {
getContainersByStack.mockResolvedValue([{ Id: 'container-1' }]);
stopContainer.mockResolvedValue(undefined);
restartContainer.mockResolvedValue(undefined);
// Default: remotes are upgraded and honor the exact-stack allowlist. The
// mixed-version cases below flip this to false to exercise the gate.
remoteSupportsCrossNodeRbac.mockResolvedValue(true);
activeBulkActions.clear();
db.getDb().prepare('DELETE FROM stack_label_assignments').run();
db.getDb().prepare('DELETE FROM stack_labels').run();
@@ -845,6 +853,60 @@ describe('POST /api/fleet/labels/fleet-stop confirmed-target allowlist', () => {
expect(res.status).toBe(400);
expect(res.body.error).toMatch(/nodeId/);
});
it('refuses a real stop to a remote lacking cross-node-rbac and reports it as needing upgrade', async () => {
remoteSupportsCrossNodeRbac.mockResolvedValue(false);
const remoteId = db.addNode({
name: 'old-remote', type: 'remote', api_url: 'http://old.example:1852',
api_token: 'tok', compose_dir: '/app/compose', is_default: false,
});
try {
const fetchSpy = vi.spyOn(globalThis, 'fetch');
const res = await request(app)
.post('/api/fleet/labels/fleet-stop')
.set('Authorization', authHeader)
.send({ labelName: 'any-label', targets: [{ nodeId: remoteId, stackNames: ['alpha'] }] });
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(/upgrade/i);
expect(node.stackResults).toEqual([{ stackName: 'alpha', success: false, error: 'Node must be upgraded to honor an exact-stack stop' }]);
// The destructive local-stop receiver is never contacted on an old remote.
expect(fetchSpy.mock.calls.some(c => String(c[0]).endsWith('/api/fleet-actions/labels/local-stop'))).toBe(false);
} finally {
db.deleteNode(remoteId);
}
});
it('fails a node whose stop results name stacks outside the confirmed set', async () => {
remoteSupportsCrossNodeRbac.mockResolvedValue(true);
const remoteId = db.addNode({
name: 'lying-remote', type: 'remote', api_url: 'http://lying.example:1852',
api_token: 'tok', compose_dir: '/app/compose', is_default: false,
});
try {
vi.spyOn(globalThis, 'fetch').mockResolvedValue({
ok: true, status: 200,
json: async () => ({
matched: true,
results: [
{ stackName: 'alpha', success: true },
{ stackName: 'unconfirmed-extra', 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'] }] });
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);
} finally {
db.deleteNode(remoteId);
}
});
});
describe('POST /api/fleet/labels/fleet-stop with dryRun: true', () => {
@@ -321,6 +321,30 @@ describe('local-stop behavior', () => {
}
});
it('reports every confirmed stack under lock contention, including one that lost its label', async () => {
const label = db.createLabel(nodeId, 'contend-label', 'slate');
db.setStackLabels('contend-kept', nodeId, [label.id]);
// contend-lost was confirmed in the preview but no longer carries the label.
const { activeBulkActions } = await import('../routes/labels');
activeBulkActions.add(`bulk:${nodeId}`);
try {
const res = await request(app)
.post('/api/fleet-actions/labels/local-stop')
.set('Authorization', authHeader)
.send({ labelName: 'contend-label', stackNames: ['contend-kept', 'contend-lost'] });
expect(res.status).toBe(200);
// Both confirmed stacks surface as contention failures; the lost one is
// not dropped (which would read as "no stacks assigned"), and the result
// is never empty when stacks were confirmed.
expect(res.body.results).toHaveLength(2);
const byName = Object.fromEntries(res.body.results.map((r: { stackName: string }) => [r.stackName, r]));
expect(byName['contend-kept'].error).toMatch(/already running/);
expect(byName['contend-lost'].error).toMatch(/already running/);
} finally {
activeBulkActions.delete(`bulk:${nodeId}`);
}
});
it('dry run returns dryRun:true per on-disk stack without touching Docker', async () => {
makeStack('dry-stack');
const label = db.createLabel(nodeId, 'dry-label', 'slate');
@@ -0,0 +1,102 @@
/**
* Mixed-version HTTP proxy gate. A remote that does not advertise the
* cross-node-rbac capability ignores the forwarded actor role and runs a
* proxied request as admin, so the hub must refuse to proxy a non-admin user's
* request to such a remote. Admins are unaffected. Each remote is a small
* loopback server that answers /api/meta with or without the capability.
*/
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import http from 'http';
import bcrypt from 'bcrypt';
import request from 'supertest';
import jwt from 'jsonwebtoken';
import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_JWT_SECRET } from './helpers/setupTestDb';
let tmpDir: string;
let app: import('express').Express;
let viewerBearer: string;
let adminBearer: string;
let capServer: http.Server;
let noCapServer: http.Server;
let capNodeId: number;
let noCapNodeId: number;
const PROXIED_PATH = '/api/stacks';
function metaServer(capabilities: string[]): http.Server {
return http.createServer((req, res) => {
res.writeHead(200, { 'Content-Type': 'application/json' });
if (req.url?.startsWith('/api/meta')) {
res.end(JSON.stringify({ version: '0.93.0', capabilities }));
} else {
// Any proxied request that clears the gate lands here.
res.end('[]');
}
});
}
async function listen(server: http.Server): Promise<number> {
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve));
return (server.address() as import('net').AddressInfo).port;
}
beforeAll(async () => {
tmpDir = await setupTestDb();
({ app } = await import('../index'));
const { DatabaseService } = await import('../services/DatabaseService');
const db = DatabaseService.getInstance();
const hash = await bcrypt.hash('password123', 1);
db.addUser({ username: 'gate-viewer', password_hash: hash, role: 'viewer' });
const viewer = db.getUserByUsername('gate-viewer')!;
viewerBearer = jwt.sign({ username: 'gate-viewer', role: 'viewer', tv: viewer.token_version }, TEST_JWT_SECRET, { expiresIn: '1m' });
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
const capPort = await listen(capServer);
const noCapPort = await listen(noCapServer);
capNodeId = db.addNode({
name: 'cap-remote', type: 'remote', mode: 'proxy', compose_dir: '/tmp',
is_default: false, api_url: `http://127.0.0.1:${capPort}`, api_token: 'cap-token',
});
noCapNodeId = db.addNode({
name: 'nocap-remote', type: 'remote', mode: 'proxy', compose_dir: '/tmp',
is_default: false, api_url: `http://127.0.0.1:${noCapPort}`, api_token: 'nocap-token',
});
});
afterAll(async () => {
await new Promise<void>((resolve) => capServer.close(() => resolve()));
await new Promise<void>((resolve) => noCapServer.close(() => resolve()));
cleanupTestDb(tmpDir);
});
describe('remote proxy cross-node-rbac gate', () => {
it('denies a non-admin proxied request to a remote that lacks cross-node-rbac', async () => {
const res = await request(app)
.get(PROXIED_PATH)
.set('Authorization', `Bearer ${viewerBearer}`)
.set('x-node-id', String(noCapNodeId));
expect(res.status).toBe(403);
expect(res.body.error).toMatch(/upgrade/i);
});
it('allows a non-admin proxied request to a remote that advertises cross-node-rbac', async () => {
const res = await request(app)
.get(PROXIED_PATH)
.set('Authorization', `Bearer ${viewerBearer}`)
.set('x-node-id', String(capNodeId));
// Forwarded to the capability-advertising remote, which answers 200.
expect(res.status).not.toBe(403);
});
it('does not gate an admin proxied request even to a remote lacking cross-node-rbac', async () => {
const res = await request(app)
.get(PROXIED_PATH)
.set('Authorization', `Bearer ${adminBearer}`)
.set('x-node-id', String(noCapNodeId));
expect(res.status).not.toBe(403);
});
});
@@ -171,6 +171,14 @@ describe('remote proxy gateway - actor role header forwarding', () => {
beforeAll(async () => {
captureServer = http.createServer((req, res) => {
// The proxy gate probes /api/meta first to confirm the remote enforces
// cross-node RBAC before forwarding a non-admin request; advertise it so
// the viewer forward is allowed and we can assert the forwarded headers.
if (req.url?.startsWith('/api/meta')) {
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(JSON.stringify({ version: '0.93.0', capabilities: ['cross-node-rbac'] }));
return;
}
captured = req.headers;
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end('[]');
@@ -0,0 +1,58 @@
/**
* 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.
*/
import { describe, it, expect, beforeAll, afterAll, afterEach, vi } from 'vitest';
import { setupTestDb, cleanupTestDb } from './helpers/setupTestDb';
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;
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);
}
const ONLINE = { startedAt: null, updateError: null, online: true } as const;
describe('remoteSupportsCrossNodeRbac', () => {
it('returns true when the remote advertises cross-node-rbac', async () => {
mockMeta({ version: '0.93.0', capabilities: ['fleet', 'cross-node-rbac'], ...ONLINE });
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 });
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 });
expect(await remoteSupportsCrossNodeRbac(NODE_ID)).toBe(false);
});
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);
});
});
+6 -1
View File
@@ -88,7 +88,12 @@ export async function runLocalLabelStop(
const lockKey = `bulk:${nodeId}`;
if (activeBulkActions.has(lockKey)) {
const busyStacks = allowedStacks ? stackNames.filter(name => allowedStacks.has(name)) : stackNames;
// Report every confirmed stack as a contention failure, not just the
// currently label-matched subset: a confirmed stack that lost its label
// must still surface (the operator asked to stop it), and the result must
// not be empty when stacks were confirmed, or the UI reads it as "label
// matched but no stacks assigned" instead of a contention failure.
const busyStacks = allowedStacks ? [...allowedStacks] : stackNames;
return {
matched: true,
stackResults: busyStacks.map(stackName => ({
+44
View File
@@ -0,0 +1,44 @@
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 { getErrorMessage } from '../utils/errors';
// Mirrors the node-meta endpoint's TTL and shares its `remote-meta:<id>` cache
// key, so a recent /api/nodes/:id/meta read warms this check and vice versa.
const REMOTE_META_CACHE_TTL = 3 * 60 * 1000;
/**
* 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.
*
* 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.
*/
export async function remoteSupportsCrossNodeRbac(nodeId: number): Promise<boolean> {
try {
const meta = await CacheService.getInstance().getOrFetch<RemoteMeta>(
`${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;
}
}
+25
View File
@@ -4,6 +4,7 @@ import { NodeRegistry } from '../services/NodeRegistry';
import { PROXY_TIER_HEADER, PROXY_ROLE_HEADER } from '../services/license-headers';
import { LicenseService } from '../services/LicenseService';
import { isProxyExemptPath } from '../helpers/proxyExemptPaths';
import { remoteSupportsCrossNodeRbac } from '../helpers/remoteCapabilities';
import { getErrorMessage } from '../utils/errors';
import { DatabaseService } from '../services/DatabaseService';
import { redactSensitiveText } from '../utils/safeLog';
@@ -145,6 +146,30 @@ export function createRemoteProxyMiddleware(): RequestHandler {
return;
}
// Mixed-version RBAC gate. The forwarded actor role is enforced only by a
// remote that advertises cross-node-rbac; an older remote ignores the
// header and runs the proxied request as admin. So a non-admin must not be
// forwarded to a remote that does not advertise the capability. Admins are
// unaffected (they are admin on the remote regardless), and the check is
// skipped for them so it never adds latency to the admin path. Fails closed
// when the capability cannot be determined. Using `?.` so an unresolved user
// (not reachable past authGate, but defensive) is gated, never waved through.
if (req.user?.role !== 'admin') {
remoteSupportsCrossNodeRbac(req.nodeId)
.then((supported) => {
if (!supported) {
res.status(403).json({
error: `Remote node "${node.name}" is running a version that does not enforce per-user permissions. Upgrade it before non-admin users can act on it.`,
});
return;
}
req.proxyTarget = target;
proxy(req, res, next);
})
.catch(next);
return;
}
req.proxyTarget = target;
proxy(req, res, next);
};
+25
View File
@@ -20,6 +20,7 @@ import { requirePaid, requireAdmin, requireNodeProxy } from '../middleware/tierG
import { requirePermission } from '../middleware/permissions';
import { scheduleLocalUpdate } from './license';
import { runPolicyGate, assertPolicyGateAllows, buildPolicyGateOptions } from '../helpers/policyGate';
import { remoteSupportsCrossNodeRbac } from '../helpers/remoteCapabilities';
import { captureLocalNodeFiles, captureRemoteNodeFiles, buildSnapshotDocumentation, pickDossierFields, dossierHasContent, type SnapshotNodeData, type SnapshotDocumentation } from '../utils/snapshot-capture';
import { getLatestVersion, getLatestRelease } from '../utils/version-check';
import { isValidStackName } from '../utils/validation';
@@ -1464,6 +1465,22 @@ fleetRouter.post('/labels/fleet-stop', authMiddleware, async (req: Request, res:
if (!target) {
return { nodeId: node.id, nodeName: node.name, reachable: false, matched: false, stackResults: [], error: formatNoTargetError(node) };
}
// A real stop bound to a confirmed stack set must only go to a remote that
// honors the allowlist (cross-node-rbac). An older remote ignores
// `stackNames` and would stop every label-matched stack, including ones
// labelled after the preview. Refuse to send and report the node as
// needing an upgrade instead. Dry runs carry no allowlist and only
// preview, so they are safe to send to any version.
if (!isDryRun && allowedStacks) {
const supported = await remoteSupportsCrossNodeRbac(node.id);
if (!supported) {
return {
nodeId: node.id, nodeName: node.name, reachable: false, matched: false,
stackResults: failAllStacks([...allowedStacks], 'Node must be upgraded to honor an exact-stack stop'),
error: 'Node is running a version that cannot limit the stop to the confirmed stacks; upgrade it and retry.',
};
}
}
try {
const headers: Record<string, string> = { 'Content-Type': 'application/json' };
if (target.apiToken) headers.Authorization = `Bearer ${target.apiToken}`;
@@ -1490,6 +1507,14 @@ fleetRouter.post('/labels/fleet-stop', authMiddleware, async (req: Request, res:
if (!isLabelLocalStopResponse(remote)) {
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' };
}
return {
nodeId: node.id, nodeName: node.name, reachable: true,
matched: remote.matched,
@@ -41,8 +41,18 @@ export const CAPABILITIES = [
'env-inventory',
'project-env-files',
'compose-storage',
'cross-node-rbac',
] as const;
/**
* Advertised by instances that enforce the proxied actor's role (instead of
* treating every node-to-node request as admin) and honor the exact-stack
* allowlist on stop-by-label. The control instance refuses to forward a
* non-admin's request, or a confirmed stop, to a remote lacking this flag so a
* mixed-version fleet cannot escalate or over-stop on an un-upgraded node.
*/
export const CROSS_NODE_RBAC_CAPABILITY = 'cross-node-rbac';
export type Capability = (typeof CAPABILITIES)[number];
/** Returns true when the string is a usable semver version. */
+1
View File
@@ -29,6 +29,7 @@ export const CAPABILITIES = [
'env-inventory',
'project-env-files',
'compose-storage',
'cross-node-rbac',
] as const;
export type Capability = (typeof CAPABILITIES)[number];