Skip to content

Commit e6a2721

Browse files
mchwangmchwang
committed
Close remaining network lifecycle gaps
1 parent bf63094 commit e6a2721

5 files changed

Lines changed: 86 additions & 17 deletions

File tree

‎agents/container/profile.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { join } from 'node:path';
66
import type { InvocationInput, Phase } from '../contract.ts';
77
import { assertBuiltAgentImage } from './image.ts';
88
import { assertTaskFilesystems, type TaskFilesystems } from './storage.ts';
9-
import { assertVendorNetwork, type VendorNetwork } from '../network/network.ts';
9+
import { assertVendorNetwork, removeVendorNetwork, type VendorNetwork } from '../network/network.ts';
1010
import { assertAgentCommand, assertPhasePolicy, type AgentCommand, type PhasePolicy } from '../policy.ts';
1111
export interface ContainerProfile {
1212
readonly name: string;
@@ -51,6 +51,7 @@ interface ProfileIdentity { readonly inputDirectory: string; readonly schema: Fi
5151
type InputIdentity = Pick<ProfileIdentity, 'inputDirectory' | 'schema'>;
5252
interface InputCapture extends InputIdentity { readonly content: Buffer }
5353
const identities = new WeakMap<ContainerProfile, ProfileIdentity>();
54+
const claimedNetworks = new WeakSet<VendorNetwork>();
5455
const removeOwnedDirectory = (directory: string) => {
5556
if (!lstatSync(directory, { throwIfNoEntry: false })) return;
5657
chmodSync(directory, 0o700);
@@ -127,7 +128,10 @@ export function assertContainerProfile(profile: ContainerProfile, timeoutMs = 30
127128
export function disposeContainerProfile(profile: ContainerProfile): void {
128129
const identity = identities.get(profile);
129130
if (!identity) return;
130-
removeOwnedDirectories(identity.cleanupDirectories);
131+
const failures: unknown[] = [];
132+
try { removeOwnedDirectories(identity.cleanupDirectories); } catch (error) { failures.push(error); }
133+
try { removeVendorNetwork(identity.network); } catch (error) { failures.push(error); }
134+
if (failures.length) throw new AggregateError(failures, 'Profile resource cleanup did not settle.');
131135
identities.delete(profile);
132136
}
133137

@@ -149,6 +153,7 @@ export function createContainerProfile(options: ProfileOptions): ContainerProfil
149153
assertBuiltAgentImage(options.imageId);
150154
assertTaskFilesystems(filesystems, invocation.clone);
151155
assertVendorNetwork(options.network, invocation);
156+
if (claimedNetworks.has(options.network)) throw new Error('Vendor network already belongs to another container profile.');
152157
assertPhasePolicy(options.policy, invocation);
153158
const command = assertAgentCommand(options.command, options.policy, invocation.vendor);
154159
const sourceInput = captureInput(options.inputDirectory);
@@ -216,6 +221,7 @@ export function createContainerProfile(options: ProfileOptions): ContainerProfil
216221
auth: authIdentity,
217222
cleanupDirectories: Object.freeze([...cleanupDirectories]), filesystems, clone: invocation.clone,
218223
network: options.network, policy: options.policy, invocation }));
224+
claimedNetworks.add(options.network);
219225
return profile;
220226
} catch (error) {
221227
try { removeOwnedDirectories(cleanupDirectories); }

‎agents/container/run.ts‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,11 @@ type Inspect = {
8888
CpuPercent: number; IOMaximumBandwidth: number; IOMaximumIOps: number; DeviceCgroupRules: unknown[] | null;
8989
StorageOpt?: Record<string, string> | null; CgroupParent: string;
9090
Devices: unknown[] | null; DeviceRequests: unknown[] | null; Tmpfs: Record<string, string> | null;
91-
Mounts: Array<{ Type: string; Source: string; Target: string; ReadOnly: boolean }> | null; Dns: string[] };
91+
Mounts: Array<{ Type: string; Source: string; Target: string; ReadOnly: boolean }> | null; Dns: string[];
92+
DnsOptions: string[]; DnsSearch: string[]; ExtraHosts: string[] | null;
93+
PortBindings: Record<string, unknown> | null; PublishAllPorts: boolean };
9294
Mounts: Array<{ Type: string; Name?: string; Source: string; Destination: string; RW: boolean }>;
93-
NetworkSettings: { Networks: Record<string, unknown> };
95+
NetworkSettings: { Networks: Record<string, unknown>; Ports: Record<string, unknown> };
9496
};
9597

9698
/** Validate daemon-resolved configuration before starting an agent. */
@@ -134,6 +136,10 @@ export function validateContainer(container: string, profile: ContainerProfile,
134136
throw new Error('Container daemon configuration is missing required lockdown.');
135137
if (JSON.stringify(host.Dns) !== JSON.stringify(['127.0.0.1']))
136138
throw new Error('Container DNS configuration changed.');
139+
if (host.DnsOptions.length || host.DnsSearch.length || (host.ExtraHosts?.length ?? 0)
140+
|| Object.keys(host.PortBindings ?? {}).length || host.PublishAllPorts
141+
|| Object.keys(inspect.NetworkSettings.Ports ?? {}).length)
142+
throw new Error('Container host or port configuration changed.');
137143
if (JSON.stringify(Object.keys(inspect.NetworkSettings.Networks)) !== JSON.stringify([profile.network.name]))
138144
throw new Error('Container network attachment changed.');
139145
const tmpfs = host.Tmpfs ?? {};
@@ -254,14 +260,14 @@ export function createValidatedContainer(profile: ContainerProfile, timeoutMs =
254260
}
255261
}
256262

257-
export function runContainer(profile: ContainerProfile, timeoutMs = 60_000,
263+
export function startValidatedContainer(profile: ContainerProfile, timeoutMs = 60_000,
258264
secrets: Readonly<Record<string, string>> = {}): string {
259265
const remaining = createDeadline(timeoutMs);
260-
const container = createValidatedContainer(profile, remaining(), secrets);
261266
let failure: unknown;
262267
try {
263-
assertContainerProfile(profile, remaining());
264-
const output = docker(['start', '--attach', container], { timeoutMs: remaining(), secrets });
268+
validateSecrets(profile, secrets);
269+
validateContainer(profile.name, profile, remaining());
270+
const output = docker(['start', '--attach', profile.name], { timeoutMs: remaining(), secrets });
265271
remaining();
266272
return output;
267273
}
@@ -274,3 +280,17 @@ export function runContainer(profile: ContainerProfile, timeoutMs = 60_000,
274280
}
275281
}
276282
}
283+
284+
export function runContainer(profile: ContainerProfile, timeoutMs = 60_000,
285+
secrets: Readonly<Record<string, string>> = {}): string {
286+
const remaining = createDeadline(timeoutMs);
287+
createValidatedContainer(profile, remaining(), secrets);
288+
let startBudget: number;
289+
try { startBudget = remaining(); }
290+
catch (error) {
291+
try { removeContainerOrThrow(profile); }
292+
catch (cleanupError) { throw new AggregateError([error, cleanupError], 'Agent deadline and cleanup both failed.'); }
293+
throw error;
294+
}
295+
return startValidatedContainer(profile, startBudget, secrets);
296+
}

‎agents/network/network.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ export interface VendorNetwork {
1717
interface NetworkIdentity { readonly allocationId: string; readonly imageId: string; readonly invocation: InvocationInput;
1818
readonly subnet: string; readonly proxyIp: string }
1919
const identities = new WeakMap<VendorNetwork, NetworkIdentity>();
20+
const removedNetworks = new WeakSet<VendorNetwork>();
2021
const environment = () => ({ PATH: process.env.PATH, DOCKER_HOST: process.env.DOCKER_HOST });
2122
const deadline = (timeoutMs: number) => {
2223
if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1) throw new Error('Network deadline must be a positive integer.');
@@ -65,8 +66,11 @@ const validateVendorNetwork = (network: VendorNetwork, invocation: InvocationInp
6566
HostConfig?: { ReadonlyRootfs?: boolean; Privileged?: boolean; CapDrop?: string[]; CapAdd?: string[] | null;
6667
SecurityOpt?: string[]; Memory?: number; MemorySwap?: number; NanoCpus?: number; PidsLimit?: number;
6768
NetworkMode?: string; PidMode?: string; IpcMode?: string; UTSMode?: string; UsernsMode?: string;
68-
CgroupnsMode?: string; Devices?: unknown[] | null; DeviceRequests?: unknown[] | null };
69-
NetworkSettings?: { Networks?: Record<string, { IPAddress?: string }> }; Mounts?: unknown[] } | undefined;
69+
CgroupnsMode?: string; Devices?: unknown[] | null; DeviceRequests?: unknown[] | null;
70+
Dns?: string[]; DnsOptions?: string[]; DnsSearch?: string[]; ExtraHosts?: string[] | null;
71+
PortBindings?: Record<string, unknown> | null; PublishAllPorts?: boolean };
72+
NetworkSettings?: { Networks?: Record<string, { IPAddress?: string }>; Ports?: Record<string, unknown> };
73+
Mounts?: unknown[] } | undefined;
7074
const image = JSON.parse(docker(['image', 'inspect', identity.imageId], remaining()))[0] as
7175
{ Config?: { Env?: string[] } } | undefined;
7276
const inspectedNetwork = JSON.parse(docker(['network', 'inspect', network.name], remaining()))[0] as
@@ -88,6 +92,10 @@ const validateVendorNetwork = (network: VendorNetwork, invocation: InvocationInp
8892
|| inspect.HostConfig.IpcMode !== 'private' || inspect.HostConfig.UTSMode !== ''
8993
|| inspect.HostConfig.UsernsMode !== '' || inspect.HostConfig.CgroupnsMode !== 'private'
9094
|| (inspect.HostConfig.Devices?.length ?? 0) !== 0 || (inspect.HostConfig.DeviceRequests?.length ?? 0) !== 0
95+
|| (inspect.HostConfig.Dns?.length ?? 0) !== 0 || (inspect.HostConfig.DnsOptions?.length ?? 0) !== 0
96+
|| (inspect.HostConfig.DnsSearch?.length ?? 0) !== 0 || (inspect.HostConfig.ExtraHosts?.length ?? 0) !== 0
97+
|| Object.keys(inspect.HostConfig.PortBindings ?? {}).length !== 0 || inspect.HostConfig.PublishAllPorts
98+
|| Object.keys(inspect.NetworkSettings?.Ports ?? {}).length !== 0
9199
|| JSON.stringify(networks) !== JSON.stringify(['bridge', network.name].sort()) || inspect.Mounts?.length
92100
|| JSON.stringify(inspect.Config?.Entrypoint) !== JSON.stringify(['node'])
93101
|| JSON.stringify(inspect.Config?.Cmd) !== JSON.stringify(['/usr/local/lib/codeboost-egress-proxy.mjs'])
@@ -156,7 +164,10 @@ export function createVendorNetwork(invocation: InvocationInput, imageId: string
156164

157165
export function removeVendorNetwork(network: VendorNetwork): void {
158166
const identity = identities.get(network);
159-
if (!identity) throw new Error('Vendor network was not created by the trusted network builder.');
167+
if (!identity) {
168+
if (removedNetworks.has(network)) return;
169+
throw new Error('Vendor network was not created by the trusted network builder.');
170+
}
160171
assertBuiltAgentImage(identity.imageId);
161172
const allocationId = identity.allocationId;
162173
const remaining = deadline(30_000), failures: unknown[] = [];
@@ -166,4 +177,5 @@ export function removeVendorNetwork(network: VendorNetwork): void {
166177
remaining, 'vendor network', allocationId); } catch (error) { failures.push(error); }
167178
if (failures.length) throw new AggregateError(failures, 'Vendor network cleanup did not settle.');
168179
identities.delete(network);
180+
removedNetworks.add(network);
169181
}

‎test/agent-container.test.ts‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest';
77
import { captureInvocation, type InvocationInput, type Phase } from '../agents/contract.ts';
88
import { AGENT_IMAGE, assertBuiltAgentImage, buildAgentImage } from '../agents/container/image.ts';
99
import { createContainerProfile, disposeContainerProfile } from '../agents/container/profile.ts';
10-
import { createValidatedContainer, prepareTaskFilesystems, removeTaskFilesystems, runContainer,
10+
import { createValidatedContainer, prepareTaskFilesystems, removeTaskFilesystems, runContainer, startValidatedContainer,
1111
hasExactOptions, validateContainer } from '../agents/container/run.ts';
1212
import { createTaskClone } from '../git/clone.ts';
1313
import { createVendorNetwork, removeVendorNetwork, type VendorNetwork } from '../agents/network/network.ts';
@@ -102,6 +102,13 @@ describe('real Docker agent isolation', () => {
102102
expect(runContainer(profile(data, phase, 'phase-worktree'))).toBe('');
103103
}, 60_000);
104104

105+
it('removes the invocation proxy and network after the container settles', () => {
106+
const data = fixture(), valid = profile(data, 'planning', 'noop');
107+
expect(runContainer(valid)).toBe('');
108+
expect(spawnSync('docker', ['container', 'inspect', valid.network.proxyContainer]).status).not.toBe(0);
109+
expect(spawnSync('docker', ['network', 'inspect', valid.network.name]).status).not.toBe(0);
110+
}, 60_000);
111+
105112
it('runs read-only with no root capabilities, host paths, inherited secrets, or writable tools', () => {
106113
const data = fixture();
107114
process.env.HOST_SECRET_SENTINEL = 'must-not-reach-container';
@@ -257,10 +264,10 @@ describe('real Docker agent isolation', () => {
257264

258265
it('does not remove an active container when a duplicate attempt name collides', () => {
259266
const data = fixture(), captured = invocation(data.clone, 'planning');
260-
const common = governed(captured);
261-
const first = createContainerProfile({ ...common, filesystems: data.filesystems,
267+
const duplicateInvocation = captureInvocation({ ...captured });
268+
const first = createContainerProfile({ ...governed(captured), filesystems: data.filesystems,
262269
inputDirectory: data.input, codexAuthFile: data.fakeAuth, imageId });
263-
const duplicate = createContainerProfile({ ...common, filesystems: data.filesystems,
270+
const duplicate = createContainerProfile({ ...governed(duplicateInvocation), filesystems: data.filesystems,
264271
inputDirectory: data.input, codexAuthFile: data.fakeAuth, imageId });
265272
profiles.push(first, duplicate);
266273
docker(...first.args); containers.add(first.name);
@@ -301,6 +308,14 @@ describe('real Docker agent isolation', () => {
301308
expect(() => validateContainer(valid.name, valid)).toThrow('DNS configuration');
302309
docker('rm', '--force', valid.name); containers.delete(valid.name);
303310

311+
for (const extra of [['--add-host=api.openai.com:127.0.0.1'], ['--publish=127.0.0.1::3128']]) {
312+
const changedArgs = [...valid.args.slice(0, valid.args.indexOf(imageId)), ...extra,
313+
...valid.args.slice(valid.args.indexOf(imageId))];
314+
docker(...changedArgs); containers.add(valid.name);
315+
expect(() => validateContainer(valid.name, valid)).toThrow('host or port configuration');
316+
docker('rm', '--force', valid.name); containers.delete(valid.name);
317+
}
318+
304319
const imageIndex = valid.args.indexOf(imageId);
305320
const namespaceArgs = [...valid.args.slice(0, imageIndex), '--uts=host', ...valid.args.slice(imageIndex)];
306321
docker(...namespaceArgs); containers.add(valid.name);
@@ -319,10 +334,23 @@ describe('real Docker agent isolation', () => {
319334
try {
320335
docker('run', '--detach', '--name', rogue, `--network=${valid.network.name}`, '--entrypoint', 'node', imageId,
321336
'-e', 'setInterval(()=>{},1000)');
322-
expect(() => createValidatedContainer(valid)).toThrow('network or proxy changed');
337+
expect(() => createValidatedContainer(valid)).toThrow('cleanup did not settle');
323338
const absent = spawnSync('docker', ['container', 'inspect', valid.name], { encoding: 'utf8' });
324339
expect(absent.status).not.toBe(0);
325-
} finally { spawnSync('docker', ['rm', '--force', rogue], { stdio: 'ignore' }); }
340+
} finally {
341+
spawnSync('docker', ['rm', '--force', rogue], { stdio: 'ignore' });
342+
disposeContainerProfile(valid);
343+
}
344+
}, 60_000);
345+
346+
it('revalidates the agent attachment immediately before start', () => {
347+
const data = fixture(), valid = profile(data, 'planning', 'must-not-run');
348+
createValidatedContainer(valid); containers.add(valid.name);
349+
docker('network', 'disconnect', valid.network.name, valid.name);
350+
docker('network', 'connect', 'bridge', valid.name);
351+
expect(() => startValidatedContainer(valid)).toThrow(/network attachment|lockdown/);
352+
containers.delete(valid.name);
353+
expect(spawnSync('docker', ['container', 'inspect', valid.name]).status).not.toBe(0);
326354
}, 60_000);
327355

328356
it('creates containers from the captured immutable image rather than its mutable tag', () => {

‎test/agent-network.test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,9 @@ describe('vendor-only egress', () => {
8888
it.each([
8989
['host namespace', ['--pid=host']],
9090
['extra Node environment', ['--env', 'NODE_OPTIONS=--trace-warnings']],
91+
['DNS override', ['--dns=8.8.8.8']],
92+
['host override', ['--add-host=api.anthropic.com:127.0.0.1']],
93+
['published proxy port', ['--publish=127.0.0.1::3128']],
9194
] as const)('rejects a proxy replaced with %s before launch', (_label, extra) => {
9295
const replacementInvocation = captureInvocation({ ...invocation, attemptId: 'mutated-proxy-probe',
9396
deadline: Date.now() + 60_000 });

0 commit comments

Comments
 (0)