Skip to content

Commit 7acc50d

Browse files
mchwangmchwang
committed
Close remaining network lifecycle gaps
1 parent 5494bb2 commit 7acc50d

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
@@ -87,9 +87,11 @@ type Inspect = {
8787
CpuPercent: number; IOMaximumBandwidth: number; IOMaximumIOps: number; DeviceCgroupRules: unknown[] | null;
8888
StorageOpt?: Record<string, string> | null; CgroupParent: string;
8989
Devices: unknown[] | null; DeviceRequests: unknown[] | null; Tmpfs: Record<string, string> | null;
90-
Mounts: Array<{ Type: string; Source: string; Target: string; ReadOnly: boolean }> | null; Dns: string[] };
90+
Mounts: Array<{ Type: string; Source: string; Target: string; ReadOnly: boolean }> | null; Dns: string[];
91+
DnsOptions: string[]; DnsSearch: string[]; ExtraHosts: string[] | null;
92+
PortBindings: Record<string, unknown> | null; PublishAllPorts: boolean };
9193
Mounts: Array<{ Type: string; Name?: string; Source: string; Destination: string; RW: boolean }>;
92-
NetworkSettings: { Networks: Record<string, unknown> };
94+
NetworkSettings: { Networks: Record<string, unknown>; Ports: Record<string, unknown> };
9395
};
9496

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

256-
export function runContainer(profile: ContainerProfile, timeoutMs = 60_000,
262+
export function startValidatedContainer(profile: ContainerProfile, timeoutMs = 60_000,
257263
secrets: Readonly<Record<string, string>> = {}): string {
258264
const remaining = createDeadline(timeoutMs);
259-
const container = createValidatedContainer(profile, remaining(), secrets);
260265
let failure: unknown;
261266
try {
262-
assertContainerProfile(profile, remaining());
263-
const output = docker(['start', '--attach', container], { timeoutMs: remaining(), secrets });
267+
validateSecrets(profile, secrets);
268+
validateContainer(profile.name, profile, remaining());
269+
const output = docker(['start', '--attach', profile.name], { timeoutMs: remaining(), secrets });
264270
remaining();
265271
return output;
266272
}
@@ -273,3 +279,17 @@ export function runContainer(profile: ContainerProfile, timeoutMs = 60_000,
273279
}
274280
}
275281
}
282+
283+
export function runContainer(profile: ContainerProfile, timeoutMs = 60_000,
284+
secrets: Readonly<Record<string, string>> = {}): string {
285+
const remaining = createDeadline(timeoutMs);
286+
createValidatedContainer(profile, remaining(), secrets);
287+
let startBudget: number;
288+
try { startBudget = remaining(); }
289+
catch (error) {
290+
try { removeContainerOrThrow(profile); }
291+
catch (cleanupError) { throw new AggregateError([error, cleanupError], 'Agent deadline and cleanup both failed.'); }
292+
throw error;
293+
}
294+
return startValidatedContainer(profile, startBudget, secrets);
295+
}

‎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';
@@ -96,6 +96,13 @@ describe('real Docker agent isolation', () => {
9696
expect(runContainer(profile(data, phase, 'phase-worktree'))).toBe('');
9797
}, 60_000);
9898

99+
it('removes the invocation proxy and network after the container settles', () => {
100+
const data = fixture(), valid = profile(data, 'planning', 'noop');
101+
expect(runContainer(valid)).toBe('');
102+
expect(spawnSync('docker', ['container', 'inspect', valid.network.proxyContainer]).status).not.toBe(0);
103+
expect(spawnSync('docker', ['network', 'inspect', valid.network.name]).status).not.toBe(0);
104+
}, 60_000);
105+
99106
it('runs read-only with no root capabilities, host paths, inherited secrets, or writable tools', () => {
100107
const data = fixture();
101108
process.env.HOST_SECRET_SENTINEL = 'must-not-reach-container';
@@ -230,10 +237,10 @@ describe('real Docker agent isolation', () => {
230237

231238
it('does not remove an active container when a duplicate attempt name collides', () => {
232239
const data = fixture(), captured = invocation(data.clone, 'planning');
233-
const common = governed(captured);
234-
const first = createContainerProfile({ ...common, filesystems: data.filesystems,
240+
const duplicateInvocation = captureInvocation({ ...captured });
241+
const first = createContainerProfile({ ...governed(captured), filesystems: data.filesystems,
235242
inputDirectory: data.input, codexAuthFile: data.fakeAuth, imageId });
236-
const duplicate = createContainerProfile({ ...common, filesystems: data.filesystems,
243+
const duplicate = createContainerProfile({ ...governed(duplicateInvocation), filesystems: data.filesystems,
237244
inputDirectory: data.input, codexAuthFile: data.fakeAuth, imageId });
238245
profiles.push(first, duplicate);
239246
docker(...first.args); containers.add(first.name);
@@ -274,6 +281,14 @@ describe('real Docker agent isolation', () => {
274281
expect(() => validateContainer(valid.name, valid)).toThrow('DNS configuration');
275282
docker('rm', '--force', valid.name); containers.delete(valid.name);
276283

284+
for (const extra of [['--add-host=api.openai.com:127.0.0.1'], ['--publish=127.0.0.1::3128']]) {
285+
const changedArgs = [...valid.args.slice(0, valid.args.indexOf(imageId)), ...extra,
286+
...valid.args.slice(valid.args.indexOf(imageId))];
287+
docker(...changedArgs); containers.add(valid.name);
288+
expect(() => validateContainer(valid.name, valid)).toThrow('host or port configuration');
289+
docker('rm', '--force', valid.name); containers.delete(valid.name);
290+
}
291+
277292
const imageIndex = valid.args.indexOf(imageId);
278293
const namespaceArgs = [...valid.args.slice(0, imageIndex), '--uts=host', ...valid.args.slice(imageIndex)];
279294
docker(...namespaceArgs); containers.add(valid.name);
@@ -292,10 +307,23 @@ describe('real Docker agent isolation', () => {
292307
try {
293308
docker('run', '--detach', '--name', rogue, `--network=${valid.network.name}`, '--entrypoint', 'node', imageId,
294309
'-e', 'setInterval(()=>{},1000)');
295-
expect(() => createValidatedContainer(valid)).toThrow('network or proxy changed');
310+
expect(() => createValidatedContainer(valid)).toThrow('cleanup did not settle');
296311
const absent = spawnSync('docker', ['container', 'inspect', valid.name], { encoding: 'utf8' });
297312
expect(absent.status).not.toBe(0);
298-
} finally { spawnSync('docker', ['rm', '--force', rogue], { stdio: 'ignore' }); }
313+
} finally {
314+
spawnSync('docker', ['rm', '--force', rogue], { stdio: 'ignore' });
315+
disposeContainerProfile(valid);
316+
}
317+
}, 60_000);
318+
319+
it('revalidates the agent attachment immediately before start', () => {
320+
const data = fixture(), valid = profile(data, 'planning', 'must-not-run');
321+
createValidatedContainer(valid); containers.add(valid.name);
322+
docker('network', 'disconnect', valid.network.name, valid.name);
323+
docker('network', 'connect', 'bridge', valid.name);
324+
expect(() => startValidatedContainer(valid)).toThrow(/network attachment|lockdown/);
325+
containers.delete(valid.name);
326+
expect(spawnSync('docker', ['container', 'inspect', valid.name]).status).not.toBe(0);
299327
}, 60_000);
300328

301329
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)