From 94546d897a008e1108c5b0013461ca48950586ea Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 23 Aug 2026 23:13:18 +0000 Subject: [PATCH 01/18] Initial plan From a0d68d452887c6d45a060b1258cd0e51a1f4c3cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 23 Aug 2026 23:19:16 +0000 Subject: [PATCH 02/18] fix: move init signal mount outside tmp --- containers/agent/entrypoint.sh | 11 ++- containers/agent/setup-iptables.sh | 6 +- src/constants.ts | 2 + .../agent-environment/core-environment.ts | 3 +- src/services/agent-service-build.test.ts | 12 ++-- src/services/agent-service.ts | 13 +++- src/services/agent-volumes-basic.test.ts | 2 + src/services/agent-volumes/volume-builder.ts | 1 + .../agent-volumes/workspace-mounts.test.ts | 2 +- .../agent-volumes/workspace-mounts.ts | 3 +- tests/fixtures/awf-runner.ts | 9 +++ .../integration/filesystem-allowwrite.test.ts | 67 +++++++++++++++++++ 12 files changed, 116 insertions(+), 15 deletions(-) create mode 100644 tests/integration/filesystem-allowwrite.test.ts diff --git a/containers/agent/entrypoint.sh b/containers/agent/entrypoint.sh index 654a79bbc..7ea38795a 100644 --- a/containers/agent/entrypoint.sh +++ b/containers/agent/entrypoint.sh @@ -166,12 +166,17 @@ else echo "[entrypoint] Waiting for iptables initialization from init container..." INIT_TIMEOUT=300 # 300 * 0.1s = 30 seconds INIT_ELAPSED=0 - while [ ! -f /tmp/awf-init/ready ]; do + INIT_SIGNAL_DIR="${AWF_INIT_SIGNAL_DIR:-/run/awf-init}" + LEGACY_INIT_SIGNAL_DIR="/tmp/awf-init" + while [ ! -f "${INIT_SIGNAL_DIR}/ready" ] && [ ! -f "${LEGACY_INIT_SIGNAL_DIR}/ready" ]; do if [ "$INIT_ELAPSED" -ge "$INIT_TIMEOUT" ]; then echo "[entrypoint][ERROR] Timed out waiting for iptables init container after 30s" - if [ -f /tmp/awf-init/output.log ]; then + if [ -f "${INIT_SIGNAL_DIR}/output.log" ]; then echo "[entrypoint] Init container output:" - cat /tmp/awf-init/output.log + cat "${INIT_SIGNAL_DIR}/output.log" + elif [ -f "${LEGACY_INIT_SIGNAL_DIR}/output.log" ]; then + echo "[entrypoint] Init container output:" + cat "${LEGACY_INIT_SIGNAL_DIR}/output.log" else echo "[entrypoint] No init container output log found" fi diff --git a/containers/agent/setup-iptables.sh b/containers/agent/setup-iptables.sh index 92eee78c1..ccc6eabf2 100644 --- a/containers/agent/setup-iptables.sh +++ b/containers/agent/setup-iptables.sh @@ -497,7 +497,11 @@ dump_nat_rules_for_debugging() { dump_audit_state() { # Dump full iptables state for audit trail # Written to the init signal volume so it can be preserved by the host - local audit_file="/tmp/awf-init/iptables-audit.txt" + local audit_dir="${AWF_INIT_SIGNAL_DIR:-/run/awf-init}" + if [ ! -d "$audit_dir" ] && [ -d /tmp/awf-init ]; then + audit_dir="/tmp/awf-init" + fi + local audit_file="${audit_dir}/iptables-audit.txt" echo "# iptables audit dump - $(date -u '+%Y-%m-%dT%H:%M:%SZ')" > "$audit_file" echo "" >> "$audit_file" echo "## IPv4 NAT rules" >> "$audit_file" diff --git a/src/constants.ts b/src/constants.ts index fd4d63769..abe508483 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -12,6 +12,8 @@ export const CLI_PROXY_CONTAINER_NAME = 'awf-cli-proxy'; export const ENCLAVE_MCP_SERVER_CONTAINER_NAME = 'awf-enclave-mcp-server'; export const ENCLAVE_AGENT_API_PROXY_CONTAINER_NAME = 'awf-enclave-agent-api-proxy'; export const LOCAL_ENCLAVE_MCP_SERVER_IMAGE = 'awf-enclave-mcp-server:local'; +export const INIT_SIGNAL_DIR = '/run/awf-init'; +export const LEGACY_INIT_SIGNAL_DIR = '/tmp/awf-init'; // SQUID_PORT is centralized in src/config/sandbox-network-policy.json and // re-exported here so existing import sites keep working unchanged. diff --git a/src/services/agent-environment/core-environment.ts b/src/services/agent-environment/core-environment.ts index cc9a2ad90..868d9c049 100644 --- a/src/services/agent-environment/core-environment.ts +++ b/src/services/agent-environment/core-environment.ts @@ -1,4 +1,4 @@ -import { SQUID_PORT } from '../../constants'; +import { INIT_SIGNAL_DIR, SQUID_PORT } from '../../constants'; import { getRealUserHome } from '../../host-identity'; import { AgentEnvironmentParams } from './types'; @@ -14,6 +14,7 @@ export function buildCoreEnvironment(params: AgentEnvironmentParams): Record { expect(initService.entrypoint).toEqual(['/bin/bash']); expect(initService.command).toEqual([ '-c', - '/usr/local/bin/setup-iptables.sh > /tmp/awf-init/output.log 2>&1 && touch /tmp/awf-init/ready', + 'mkdir -p "$AWF_INIT_SIGNAL_DIR" && if [ ! -e /tmp/awf-init ]; then ln -s "$AWF_INIT_SIGNAL_DIR" /tmp/awf-init 2>/dev/null || true; fi && /usr/local/bin/setup-iptables.sh > "$AWF_INIT_SIGNAL_DIR/output.log" 2>&1 && touch "$AWF_INIT_SIGNAL_DIR/ready"', ]); expect(initService.security_opt).toBeUndefined(); expect(initService.restart).toBe('no'); @@ -136,8 +136,8 @@ describe('agent service', () => { const initService = result.services['iptables-init'] as any; const volumes = initService.volumes as string[]; - // Source path is the runner-side init-signal dir, container path is /tmp/awf-init - expect(volumes).toContain(`${mockConfig.workDir}/init-signal:/tmp/awf-init:rw`); + // Source path is the runner-side init-signal dir, container path is outside /tmp. + expect(volumes).toContain(`${mockConfig.workDir}/init-signal:/run/awf-init:rw`); }); it('should apply dockerHostPathPrefix to the iptables-init init-signal volume', () => { @@ -158,10 +158,10 @@ describe('agent service', () => { const agentVolumes = result.services.agent.volumes as string[]; const expectedSource = `/host${mockConfig.workDir}/init-signal`; - expect(initVolumes).toContain(`${expectedSource}:/tmp/awf-init:rw`); + expect(initVolumes).toContain(`${expectedSource}:/run/awf-init:rw`); // The agent must mount the SAME daemon-side source so they share the ready file. - expect(agentVolumes).toContain(`${expectedSource}:/tmp/awf-init:rw`); + expect(agentVolumes).toContain(`${expectedSource}:/run/awf-init:rw`); }); it('should normalize trailing slash in dockerHostPathPrefix for iptables-init mount', () => { @@ -174,7 +174,7 @@ describe('agent service', () => { const initService = result.services['iptables-init'] as any; const initVolumes = initService.volumes as string[]; - expect(initVolumes).toContain(`/host${mockConfig.workDir}/init-signal:/tmp/awf-init:rw`); + expect(initVolumes).toContain(`/host${mockConfig.workDir}/init-signal:/run/awf-init:rw`); }); // Symmetric invariant: every absolute, non-kernel-virtual bind-mount source on every diff --git a/src/services/agent-service.ts b/src/services/agent-service.ts index c21a7551c..62d56ca24 100644 --- a/src/services/agent-service.ts +++ b/src/services/agent-service.ts @@ -1,6 +1,8 @@ import * as path from 'path'; import { AGENT_CONTAINER_NAME, + INIT_SIGNAL_DIR, + LEGACY_INIT_SIGNAL_DIR, IPTABLES_INIT_CONTAINER_NAME, SQUID_PORT, } from '../constants'; @@ -292,13 +294,19 @@ interface IptablesInitServiceParams { */ export function buildIptablesInitService(params: IptablesInitServiceParams): any { const { agentService, environment, networkConfig, initSignalDir, dockerHostPathPrefix, hostGatewayIp } = params; + const setupCommand = [ + 'mkdir -p "$AWF_INIT_SIGNAL_DIR"', + `if [ ! -e ${LEGACY_INIT_SIGNAL_DIR} ]; then ln -s "$AWF_INIT_SIGNAL_DIR" ${LEGACY_INIT_SIGNAL_DIR} 2>/dev/null || true; fi`, + '/usr/local/bin/setup-iptables.sh > "$AWF_INIT_SIGNAL_DIR/output.log" 2>&1', + 'touch "$AWF_INIT_SIGNAL_DIR/ready"', + ].join(' && '); // The init-signal mount must use the same source path that the agent container uses, // otherwise the two containers bind to different daemon-side directories and the // ready-file handshake fails. buildAgentVolumes() applies dockerHostPathPrefix to its // mounts, so do the same here via the shared helper. const [initSignalMount] = applyHostPathPrefixToVolumes( - [`${initSignalDir}:/tmp/awf-init:rw`], + [`${initSignalDir}:${INIT_SIGNAL_DIR}:rw`], dockerHostPathPrefix, ); @@ -335,6 +343,7 @@ export function buildIptablesInitService(params: IptablesInitServiceParams): any AWF_SSL_BUMP_ENABLED: environment.AWF_SSL_BUMP_ENABLED || '', AWF_SSL_BUMP_INTERCEPT_PORT: environment.AWF_SSL_BUMP_INTERCEPT_PORT || '', AWF_HOST_GATEWAY_IP: hostGatewayIp || '', + AWF_INIT_SIGNAL_DIR: INIT_SIGNAL_DIR, }, depends_on: { 'agent': { @@ -350,7 +359,7 @@ export function buildIptablesInitService(params: IptablesInitServiceParams): any // The init container only needs to run setup-iptables.sh directly. entrypoint: ['/bin/bash'], // Run setup-iptables.sh then signal readiness; log output to shared volume for diagnostics - command: ['-c', '/usr/local/bin/setup-iptables.sh > /tmp/awf-init/output.log 2>&1 && touch /tmp/awf-init/ready'], + command: ['-c', setupCommand], // Resource limits (init container exits quickly) mem_limit: '128m', pids_limit: 50, diff --git a/src/services/agent-volumes-basic.test.ts b/src/services/agent-volumes-basic.test.ts index 3a1cec599..e2c0410cd 100644 --- a/src/services/agent-volumes-basic.test.ts +++ b/src/services/agent-volumes-basic.test.ts @@ -63,6 +63,8 @@ describe('agent service', () => { expect(volumes).toContain(`${writablePath}:/host${writablePath}:rw`); expect(volumes).toContain('/tmp:/tmp:ro'); expect(volumes).toContain('/tmp:/host/tmp:ro'); + expect(volumes).toContain(`${getConfig().workDir}/init-signal:/run/awf-init:rw`); + expect(volumes).not.toContain(`${getConfig().workDir}/init-signal:/tmp/awf-init:rw`); expect(volumes.some((volume) => volume.includes('/.copilot/logs:rw'))).toBe(true); }); diff --git a/src/services/agent-volumes/volume-builder.ts b/src/services/agent-volumes/volume-builder.ts index 7be25e840..6440a8760 100644 --- a/src/services/agent-volumes/volume-builder.ts +++ b/src/services/agent-volumes/volume-builder.ts @@ -66,6 +66,7 @@ export function buildAgentVolumes(params: AgentVolumesParams): string[] { const alwaysWritableMounts = new Set(agentVolumes.filter((spec) => spec.startsWith(`${agentLogsPath}:`) || spec.startsWith(`${sessionStatePath}:`) || + spec.startsWith(`${initSignalDir}:`) || spec === '/dev/null:/host/dev/null:rw' )); const customMounts = buildCustomVolumeMounts(config.volumeMounts, config.dockerHostPathPrefix); diff --git a/src/services/agent-volumes/workspace-mounts.test.ts b/src/services/agent-volumes/workspace-mounts.test.ts index e8a260f9e..93b2f8eb5 100644 --- a/src/services/agent-volumes/workspace-mounts.test.ts +++ b/src/services/agent-volumes/workspace-mounts.test.ts @@ -48,7 +48,7 @@ describe('buildWorkspaceMounts', () => { expect(mounts).toContain('/workspace:/workspace:rw'); expect(mounts).toContain('/tmp/awf-logs:/home/runner/.copilot/logs:rw'); expect(mounts).toContain('/tmp/awf-session:/home/runner/.copilot/session-state:rw'); - expect(mounts).toContain('/tmp/awf-init:/tmp/awf-init:rw'); + expect(mounts).toContain('/tmp/awf-init:/run/awf-init:rw'); }); }); diff --git a/src/services/agent-volumes/workspace-mounts.ts b/src/services/agent-volumes/workspace-mounts.ts index 4254fd21b..b8047daf6 100644 --- a/src/services/agent-volumes/workspace-mounts.ts +++ b/src/services/agent-volumes/workspace-mounts.ts @@ -2,6 +2,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { logger } from '../../logger'; import { WrapperConfig } from '../../types'; +import { INIT_SIGNAL_DIR } from '../../constants'; import { applyHostPathPrefixToVolumes } from '../host-path-prefix'; import { extractCommandBinaryName, @@ -27,7 +28,7 @@ export function buildWorkspaceMounts(params: WorkspaceMountsParams): string[] { `${workspaceDir}:${workspaceDir}:rw`, `${agentLogsPath}:${effectiveHome}/.copilot/logs:rw`, `${sessionStatePath}:${effectiveHome}/.copilot/session-state:rw`, - `${initSignalDir}:/tmp/awf-init:rw`, + `${initSignalDir}:${INIT_SIGNAL_DIR}:rw`, ]; if (config.enableApiProxy) { diff --git a/tests/fixtures/awf-runner.ts b/tests/fixtures/awf-runner.ts index 6bf994b6e..8248af131 100644 --- a/tests/fixtures/awf-runner.ts +++ b/tests/fixtures/awf-runner.ts @@ -13,6 +13,7 @@ export interface AwfOptions { imageTag?: string; timeout?: number; // milliseconds env?: Record; + configFile?: string; volumeMounts?: string[]; // Volume mounts in format: host_path:container_path[:mode] containerWorkDir?: string; // Working directory inside the container tty?: boolean; // Allocate pseudo-TTY (required for interactive tools like Claude Code) @@ -60,6 +61,10 @@ export class AwfRunner { async run(command: string, options: AwfOptions = {}): Promise { const args: string[] = []; + if (options.configFile) { + args.push('--config', options.configFile); + } + // Add allow-domains if (options.allowDomains && options.allowDomains.length > 0) { args.push('--allow-domains', options.allowDomains.join(',')); @@ -284,6 +289,10 @@ export class AwfRunner { // Add awf path args.push('node', this.awfPath); + if (options.configFile) { + args.push('--config', options.configFile); + } + // runWithSudo uses the legacy iptables path args.push('--legacy-security'); diff --git a/tests/integration/filesystem-allowwrite.test.ts b/tests/integration/filesystem-allowwrite.test.ts new file mode 100644 index 000000000..b9edc287c --- /dev/null +++ b/tests/integration/filesystem-allowwrite.test.ts @@ -0,0 +1,67 @@ +/** + * filesystem.allowWrite integration tests + * + * These tests cover live container startup behavior that unit tests of volume + * rewriting cannot catch, such as Docker/runc mountpoint creation order. + */ + +/// + +import { describe, test, expect, beforeAll, afterAll } from '@jest/globals'; +import { createRunner, AwfRunner } from '../fixtures/awf-runner'; +import { cleanup } from '../fixtures/cleanup'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +describe('filesystem.allowWrite', () => { + let runner: AwfRunner; + let testDir: string; + + beforeAll(async () => { + await cleanup(false); + runner = createRunner(); + }); + + afterAll(async () => { + await cleanup(false); + if (testDir && fs.existsSync(testDir)) { + fs.rmSync(testDir, { recursive: true, force: true }); + } + }); + + test('starts the legacy Docker agent when /tmp is narrowed read-only', async () => { + testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'awf-allowwrite-')); + const writableDir = path.join(testDir, 'agent'); + fs.mkdirSync(writableDir, { recursive: true }); + const configPath = path.join(testDir, 'awf-config.json'); + fs.writeFileSync(configPath, JSON.stringify({ + network: { + allowDomains: ['github.com'], + }, + filesystem: { + allowWrite: [writableDir], + }, + container: { + buildLocal: true, + }, + logging: { + logLevel: 'debug', + }, + security: { + legacySecurity: true, + }, + })); + + const result = await runner.runWithSudo( + `sh -c 'echo started > ${writableDir}/started.txt'`, + { + configFile: configPath, + timeout: 120000, + } + ); + + expect(result).toSucceed(); + expect(fs.readFileSync(path.join(writableDir, 'started.txt'), 'utf8')).toContain('started'); + }, 180000); +}); From 74c60b3c6b923dd7d6fca91ab80b4f592aac149e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 23 Aug 2026 23:37:40 +0000 Subject: [PATCH 03/18] fix: escape init signal dir in compose command --- src/services/agent-service-build.test.ts | 2 +- src/services/agent-service.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/services/agent-service-build.test.ts b/src/services/agent-service-build.test.ts index 4a06e649f..04eeb9f20 100644 --- a/src/services/agent-service-build.test.ts +++ b/src/services/agent-service-build.test.ts @@ -124,7 +124,7 @@ describe('agent service', () => { expect(initService.entrypoint).toEqual(['/bin/bash']); expect(initService.command).toEqual([ '-c', - 'mkdir -p "$AWF_INIT_SIGNAL_DIR" && if [ ! -e /tmp/awf-init ]; then ln -s "$AWF_INIT_SIGNAL_DIR" /tmp/awf-init 2>/dev/null || true; fi && /usr/local/bin/setup-iptables.sh > "$AWF_INIT_SIGNAL_DIR/output.log" 2>&1 && touch "$AWF_INIT_SIGNAL_DIR/ready"', + 'mkdir -p "$$AWF_INIT_SIGNAL_DIR" && if [ ! -e /tmp/awf-init ]; then ln -s "$$AWF_INIT_SIGNAL_DIR" /tmp/awf-init 2>/dev/null || true; fi && /usr/local/bin/setup-iptables.sh > "$$AWF_INIT_SIGNAL_DIR/output.log" 2>&1 && touch "$$AWF_INIT_SIGNAL_DIR/ready"', ]); expect(initService.security_opt).toBeUndefined(); expect(initService.restart).toBe('no'); diff --git a/src/services/agent-service.ts b/src/services/agent-service.ts index 62d56ca24..8457a4d82 100644 --- a/src/services/agent-service.ts +++ b/src/services/agent-service.ts @@ -295,10 +295,10 @@ interface IptablesInitServiceParams { export function buildIptablesInitService(params: IptablesInitServiceParams): any { const { agentService, environment, networkConfig, initSignalDir, dockerHostPathPrefix, hostGatewayIp } = params; const setupCommand = [ - 'mkdir -p "$AWF_INIT_SIGNAL_DIR"', - `if [ ! -e ${LEGACY_INIT_SIGNAL_DIR} ]; then ln -s "$AWF_INIT_SIGNAL_DIR" ${LEGACY_INIT_SIGNAL_DIR} 2>/dev/null || true; fi`, - '/usr/local/bin/setup-iptables.sh > "$AWF_INIT_SIGNAL_DIR/output.log" 2>&1', - 'touch "$AWF_INIT_SIGNAL_DIR/ready"', + 'mkdir -p "$$AWF_INIT_SIGNAL_DIR"', + `if [ ! -e ${LEGACY_INIT_SIGNAL_DIR} ]; then ln -s "$$AWF_INIT_SIGNAL_DIR" ${LEGACY_INIT_SIGNAL_DIR} 2>/dev/null || true; fi`, + '/usr/local/bin/setup-iptables.sh > "$$AWF_INIT_SIGNAL_DIR/output.log" 2>&1', + 'touch "$$AWF_INIT_SIGNAL_DIR/ready"', ].join(' && '); // The init-signal mount must use the same source path that the agent container uses, From e8c4435650dcc5359e20d57091e172d93ae504a0 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 23 Aug 2026 18:07:29 -0700 Subject: [PATCH 04/18] test: run filesystem-allowwrite integration test in CI The new tests/integration/filesystem-allowwrite.test.ts was never executed by CI. Every `npm run test:integration` invocation in test-integration-suite.yml is filtered by an explicit --testPathPatterns allowlist, and `filesystem-allowwrite` matched none of the five groups, so the file was collected by the integration jest config but always filtered out. That made it dead coverage for exactly the gap it was added to close: the runc mountpoint-creation-order failure that unit tests of volume rewriting cannot catch. Add it to the Container & Ops group, which already builds the local squid and agent images that the test needs (it runs with buildLocal and legacySecurity). Verified with `jest --listTests`: the file is excluded under the previous pattern and selected under the new one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .github/workflows/test-integration-suite.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-integration-suite.yml b/.github/workflows/test-integration-suite.yml index c80f560d3..45b3706ce 100644 --- a/.github/workflows/test-integration-suite.yml +++ b/.github/workflows/test-integration-suite.yml @@ -274,7 +274,7 @@ jobs: run: | echo "=== Running container & ops tests ===" npm run test:integration -- \ - --testPathPatterns="(container-workdir|environment-variables|error-handling|exit-code-propagation|log-commands|no-docker|volume-mounts|skip-pull)" \ + --testPathPatterns="(container-workdir|environment-variables|error-handling|exit-code-propagation|filesystem-allowwrite|log-commands|no-docker|volume-mounts|skip-pull)" \ --verbose env: JEST_TIMEOUT: 180000 From d4d06141f6c463cf57f3c9745378f2dbf751d898 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 23 Aug 2026 18:32:29 -0700 Subject: [PATCH 05/18] fix: skip credential overlays for files absent on the host The filesystem.allowWrite integration test added by this PR failed live in CI with a second instance of the bug it set out to fix: error mounting "/dev/null" to rootfs at "/host/home/runner/.npmrc": make mountpoint "/host/home/runner/.npmrc": openat .npmrc: read-only file system buildCredentialHidingOverlays emitted a /dev/null overlay for every file in the central mount policy, whether or not it existed on the host. A bind mount needs its mountpoint to already exist; runc creates a missing one via openat(O_CREAT) on the parent directory. That silently worked while the $HOME bind was rw, but fails with EROFS as soon as filesystem.allowWrite narrows $HOME to read-only, taking the agent container down before start. Filter the overlays to credential files that actually exist on the host. Verified against a real Docker daemon that masking an existing file inside a read-only bind still succeeds and still yields an empty file, so every credential that could leak is masked exactly as before. A file that does not exist cannot leak anything, so skipping it loses no protection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/credential-hiding.test.ts | 101 ++++++++++++++++-- .../agent-volumes/credential-hiding.ts | 17 ++- 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/src/services/agent-volumes/credential-hiding.test.ts b/src/services/agent-volumes/credential-hiding.test.ts index 71116371f..dd20e5dba 100644 --- a/src/services/agent-volumes/credential-hiding.test.ts +++ b/src/services/agent-volumes/credential-hiding.test.ts @@ -1,29 +1,110 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; import { buildCredentialHidingOverlays } from './credential-hiding'; import { credentialFilesToHide } from '../../config/mount-policy'; +/** + * Creates a throwaway home directory containing every credential file in the + * central policy, so the overlay builder sees them as present on the host. + */ +function createPopulatedHome(): string { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'cred-home-')); + for (const rel of credentialFilesToHide()) { + const full = path.join(home, rel); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, 'DUMMY_SECRET_VALUE'); + } + return home; +} + describe('buildCredentialHidingOverlays', () => { + let home: string; + + beforeEach(() => { + home = createPopulatedHome(); + }); + + afterEach(() => { + fs.rmSync(home, { recursive: true, force: true }); + }); + it('hides every policy credential file at both home and /host paths', () => { - const overlays = buildCredentialHidingOverlays('/home/runner'); + const overlays = buildCredentialHidingOverlays(home); const expectedFiles = credentialFilesToHide(); // One overlay at the real $HOME path and one at the chroot /host path. expect(overlays).toHaveLength(expectedFiles.length * 2); for (const rel of expectedFiles) { - expect(overlays).toContain(`/dev/null:/home/runner/${rel}:ro`); - expect(overlays).toContain(`/dev/null:/host/home/runner/${rel}:ro`); + expect(overlays).toContain(`/dev/null:${home}/${rel}:ro`); + expect(overlays).toContain(`/dev/null:/host${home}/${rel}:ro`); } }); it('masks representative credential files from the central policy', () => { - const overlays = buildCredentialHidingOverlays('/home/runner'); + const overlays = buildCredentialHidingOverlays(home); - expect(overlays).toContain('/dev/null:/home/runner/.docker/config.json:ro'); - expect(overlays).toContain('/dev/null:/host/home/runner/.docker/config.json:ro'); - expect(overlays).toContain('/dev/null:/home/runner/.config/gh/hosts.yml:ro'); - expect(overlays).toContain('/dev/null:/host/home/runner/.config/gh/hosts.yml:ro'); + expect(overlays).toContain(`/dev/null:${home}/.docker/config.json:ro`); + expect(overlays).toContain(`/dev/null:/host${home}/.docker/config.json:ro`); + expect(overlays).toContain(`/dev/null:${home}/.config/gh/hosts.yml:ro`); + expect(overlays).toContain(`/dev/null:/host${home}/.config/gh/hosts.yml:ro`); // Newly centralized entries (previously only protected by sbx). - expect(overlays).toContain('/dev/null:/home/runner/.claude/.credentials.json:ro'); - expect(overlays).toContain('/dev/null:/home/runner/.gemini/oauth_creds.json:ro'); + expect(overlays).toContain(`/dev/null:${home}/.claude/.credentials.json:ro`); + expect(overlays).toContain(`/dev/null:${home}/.gemini/oauth_creds.json:ro`); + }); + + it('still masks a credential file that is a symlink to a real file', () => { + const target = path.join(home, 'real-docker-config.json'); + fs.writeFileSync(target, 'DUMMY_SECRET_VALUE'); + const linkPath = path.join(home, '.docker/config.json'); + fs.rmSync(linkPath); + fs.symlinkSync(target, linkPath); + + const overlays = buildCredentialHidingOverlays(home); + + expect(overlays).toContain(`/dev/null:${home}/.docker/config.json:ro`); + expect(overlays).toContain(`/dev/null:/host${home}/.docker/config.json:ro`); + }); + + // Regression: a `/dev/null` overlay needs its mountpoint to already exist. + // runc creates a missing one with openat(O_CREAT) on the parent, which fails + // with EROFS once filesystem.allowWrite narrows the $HOME bind to read-only, + // killing the agent container before it starts. Absent files carry no + // credential, so they must simply be skipped. + describe('credential files that do not exist on the host', () => { + it('omits overlays for them', () => { + const emptyHome = fs.mkdtempSync(path.join(os.tmpdir(), 'cred-empty-')); + try { + expect(buildCredentialHidingOverlays(emptyHome)).toEqual([]); + } finally { + fs.rmSync(emptyHome, { recursive: true, force: true }); + } + }); + + it('omits only the absent ones and keeps the rest', () => { + const removed = '.docker/config.json'; + fs.rmSync(path.join(home, removed)); + + const overlays = buildCredentialHidingOverlays(home); + const remaining = credentialFilesToHide().filter((rel) => rel !== removed); + + expect(overlays).toHaveLength(remaining.length * 2); + expect(overlays).not.toContain(`/dev/null:${home}/${removed}:ro`); + expect(overlays).not.toContain(`/dev/null:/host${home}/${removed}:ro`); + for (const rel of remaining) { + expect(overlays).toContain(`/dev/null:${home}/${rel}:ro`); + } + }); + + it('omits overlays for a dangling symlink, which cannot leak anything', () => { + const linkPath = path.join(home, '.docker/config.json'); + fs.rmSync(linkPath); + fs.symlinkSync(path.join(home, 'nonexistent-target'), linkPath); + + const overlays = buildCredentialHidingOverlays(home); + + expect(overlays).not.toContain(`/dev/null:${home}/.docker/config.json:ro`); + }); }); }); diff --git a/src/services/agent-volumes/credential-hiding.ts b/src/services/agent-volumes/credential-hiding.ts index 0b8c1460d..34dc40d0b 100644 --- a/src/services/agent-volumes/credential-hiding.ts +++ b/src/services/agent-volumes/credential-hiding.ts @@ -1,3 +1,4 @@ +import * as fs from 'fs'; import { logger } from '../../logger'; import { credentialFilesToHide } from '../../config/mount-policy'; @@ -8,9 +9,23 @@ import { credentialFilesToHide } from '../../config/mount-policy'; * * Each credential file is masked twice: once at the real `$HOME` path and once * at the chroot `/host$HOME` path (the agent runs chrooted into `/host`). + * + * Only files that actually exist on the host are masked. A `/dev/null` overlay + * requires its mountpoint to already exist: runc creates a missing one by + * `openat(..., O_CREAT)` on the parent, which fails with EROFS once + * `filesystem.allowWrite` narrows the `$HOME` bind to read-only, taking the + * whole agent container down before it starts. Skipping absent files loses no + * protection, because a file that does not exist cannot leak a credential — + * every file that does exist is still masked, including inside a read-only + * parent (mounting over an existing path does not write to the filesystem). */ export function buildCredentialHidingOverlays(effectiveHome: string): string[] { - const credentialFiles = credentialFilesToHide().map((rel) => `${effectiveHome}/${rel}`); + const allCredentialFiles = credentialFilesToHide().map((rel) => `${effectiveHome}/${rel}`); + const credentialFiles = allCredentialFiles.filter((credFile) => fs.existsSync(credFile)); + const skipped = allCredentialFiles.length - credentialFiles.length; + if (skipped > 0) { + logger.debug(`Skipped ${skipped} credential overlay(s) with no file on the host`); + } const mounts = credentialFiles.map((credFile) => `/dev/null:${credFile}:ro`); logger.debug(`Hidden ${credentialFiles.length} credential file(s) via /dev/null mounts`); From 7b3593da2a207ee2788e93b1971983bea5cdd25a Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 23 Aug 2026 18:51:26 -0700 Subject: [PATCH 06/18] fix: drop credential overlays that cannot be mounted under a policy Replaces the host-existence filter from d4d06141, which was too broad: it also dropped the un-prefixed $HOME overlays that live on the container's own writable rootfs, so credential files stopped appearing as empty files and the credential-hiding integration tests failed with ENOENT. Decide per overlay instead, once the full volume list is known. An overlay is dropped only when the innermost bind covering its mountpoint is read-only *and* the masked path does not exist behind that bind. That is exactly the case runc cannot serve: a bind mount needs its mountpoint to already exist, and creating one under a read-only parent fails with EROFS, killing the agent container before it starts. Nothing is left unmasked. A read-only bind is the only way the agent could reach those paths, and there is nothing behind them to read; the motivating example is $HOME/.docker/config.json, which resolves into the synthesized chroot home because .docker is not a whitelisted home subdirectory. Paths that do exist are still masked, since mounting over an existing path succeeds even inside a read-only bind (verified against a real daemon). The pass is a no-op without filesystem.allowWrite, where every covering bind is read-write; the generated no-policy volume list is byte-identical. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/credential-hiding.test.ts | 188 ++++++++++-------- .../agent-volumes/credential-hiding.ts | 98 +++++++-- src/services/agent-volumes/volume-builder.ts | 6 +- 3 files changed, 191 insertions(+), 101 deletions(-) diff --git a/src/services/agent-volumes/credential-hiding.test.ts b/src/services/agent-volumes/credential-hiding.test.ts index dd20e5dba..0658d83b0 100644 --- a/src/services/agent-volumes/credential-hiding.test.ts +++ b/src/services/agent-volumes/credential-hiding.test.ts @@ -1,110 +1,132 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; -import { buildCredentialHidingOverlays } from './credential-hiding'; +import { buildCredentialHidingOverlays, pruneUnmountableCredentialOverlays } from './credential-hiding'; import { credentialFilesToHide } from '../../config/mount-policy'; -/** - * Creates a throwaway home directory containing every credential file in the - * central policy, so the overlay builder sees them as present on the host. - */ -function createPopulatedHome(): string { - const home = fs.mkdtempSync(path.join(os.tmpdir(), 'cred-home-')); - for (const rel of credentialFilesToHide()) { - const full = path.join(home, rel); - fs.mkdirSync(path.dirname(full), { recursive: true }); - fs.writeFileSync(full, 'DUMMY_SECRET_VALUE'); - } - return home; -} - describe('buildCredentialHidingOverlays', () => { - let home: string; - - beforeEach(() => { - home = createPopulatedHome(); - }); - - afterEach(() => { - fs.rmSync(home, { recursive: true, force: true }); - }); - it('hides every policy credential file at both home and /host paths', () => { - const overlays = buildCredentialHidingOverlays(home); + const overlays = buildCredentialHidingOverlays('/home/runner'); const expectedFiles = credentialFilesToHide(); // One overlay at the real $HOME path and one at the chroot /host path. expect(overlays).toHaveLength(expectedFiles.length * 2); for (const rel of expectedFiles) { - expect(overlays).toContain(`/dev/null:${home}/${rel}:ro`); - expect(overlays).toContain(`/dev/null:/host${home}/${rel}:ro`); + expect(overlays).toContain(`/dev/null:/home/runner/${rel}:ro`); + expect(overlays).toContain(`/dev/null:/host/home/runner/${rel}:ro`); } }); it('masks representative credential files from the central policy', () => { - const overlays = buildCredentialHidingOverlays(home); + const overlays = buildCredentialHidingOverlays('/home/runner'); - expect(overlays).toContain(`/dev/null:${home}/.docker/config.json:ro`); - expect(overlays).toContain(`/dev/null:/host${home}/.docker/config.json:ro`); - expect(overlays).toContain(`/dev/null:${home}/.config/gh/hosts.yml:ro`); - expect(overlays).toContain(`/dev/null:/host${home}/.config/gh/hosts.yml:ro`); + expect(overlays).toContain('/dev/null:/home/runner/.docker/config.json:ro'); + expect(overlays).toContain('/dev/null:/host/home/runner/.docker/config.json:ro'); + expect(overlays).toContain('/dev/null:/home/runner/.config/gh/hosts.yml:ro'); + expect(overlays).toContain('/dev/null:/host/home/runner/.config/gh/hosts.yml:ro'); // Newly centralized entries (previously only protected by sbx). - expect(overlays).toContain(`/dev/null:${home}/.claude/.credentials.json:ro`); - expect(overlays).toContain(`/dev/null:${home}/.gemini/oauth_creds.json:ro`); + expect(overlays).toContain('/dev/null:/home/runner/.claude/.credentials.json:ro'); + expect(overlays).toContain('/dev/null:/home/runner/.gemini/oauth_creds.json:ro'); + }); +}); + +describe('pruneUnmountableCredentialOverlays', () => { + const HOME = '/home/runner'; + const overlay = (target: string) => `/dev/null:${target}:ro`; + + let hostDir: string; + + beforeEach(() => { + hostDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cred-prune-')); }); - it('still masks a credential file that is a symlink to a real file', () => { - const target = path.join(home, 'real-docker-config.json'); - fs.writeFileSync(target, 'DUMMY_SECRET_VALUE'); - const linkPath = path.join(home, '.docker/config.json'); - fs.rmSync(linkPath); - fs.symlinkSync(target, linkPath); + afterEach(() => { + fs.rmSync(hostDir, { recursive: true, force: true }); + }); - const overlays = buildCredentialHidingOverlays(home); + it('keeps every overlay when no covering bind is read-only (the no-policy case)', () => { + const volumes = [ + `${hostDir}:/host${HOME}:rw`, + `${hostDir}/.config:/host${HOME}/.config:rw`, + overlay(`${HOME}/.docker/config.json`), + overlay(`/host${HOME}/.docker/config.json`), + overlay(`/host${HOME}/.config/gh/hosts.yml`), + ]; - expect(overlays).toContain(`/dev/null:${home}/.docker/config.json:ro`); - expect(overlays).toContain(`/dev/null:/host${home}/.docker/config.json:ro`); + expect(pruneUnmountableCredentialOverlays(volumes)).toEqual(volumes); }); - // Regression: a `/dev/null` overlay needs its mountpoint to already exist. - // runc creates a missing one with openat(O_CREAT) on the parent, which fails - // with EROFS once filesystem.allowWrite narrows the $HOME bind to read-only, - // killing the agent container before it starts. Absent files carry no - // credential, so they must simply be skipped. - describe('credential files that do not exist on the host', () => { - it('omits overlays for them', () => { - const emptyHome = fs.mkdtempSync(path.join(os.tmpdir(), 'cred-empty-')); - try { - expect(buildCredentialHidingOverlays(emptyHome)).toEqual([]); - } finally { - fs.rmSync(emptyHome, { recursive: true, force: true }); - } - }); - - it('omits only the absent ones and keeps the rest', () => { - const removed = '.docker/config.json'; - fs.rmSync(path.join(home, removed)); - - const overlays = buildCredentialHidingOverlays(home); - const remaining = credentialFilesToHide().filter((rel) => rel !== removed); - - expect(overlays).toHaveLength(remaining.length * 2); - expect(overlays).not.toContain(`/dev/null:${home}/${removed}:ro`); - expect(overlays).not.toContain(`/dev/null:/host${home}/${removed}:ro`); - for (const rel of remaining) { - expect(overlays).toContain(`/dev/null:${home}/${rel}:ro`); - } - }); - - it('omits overlays for a dangling symlink, which cannot leak anything', () => { - const linkPath = path.join(home, '.docker/config.json'); - fs.rmSync(linkPath); - fs.symlinkSync(path.join(home, 'nonexistent-target'), linkPath); - - const overlays = buildCredentialHidingOverlays(home); - - expect(overlays).not.toContain(`/dev/null:${home}/.docker/config.json:ro`); - }); + it('keeps overlays whose mountpoint exists behind a read-only bind', () => { + fs.mkdirSync(path.join(hostDir, '.config/gh'), { recursive: true }); + fs.writeFileSync(path.join(hostDir, '.config/gh/hosts.yml'), 'DUMMY_SECRET_VALUE'); + const target = `/host${HOME}/.config/gh/hosts.yml`; + + const result = pruneUnmountableCredentialOverlays([ + `${hostDir}:/host${HOME}:ro`, + overlay(target), + ]); + + expect(result).toContain(overlay(target)); + }); + + it('drops overlays whose mountpoint is missing behind a read-only bind', () => { + const target = `/host${HOME}/.docker/config.json`; + + const result = pruneUnmountableCredentialOverlays([ + `${hostDir}:/host${HOME}:ro`, + overlay(target), + ]); + + expect(result).not.toContain(overlay(target)); + expect(result).toContain(`${hostDir}:/host${HOME}:ro`); + }); + + it('resolves the mountpoint against the innermost covering bind', () => { + // An outer read-only bind that does have the file, and an inner read-only + // bind that does not. The inner bind is what supplies the directory, so the + // overlay is unmountable and must be dropped. + fs.mkdirSync(path.join(hostDir, 'outer/.config/gh'), { recursive: true }); + fs.writeFileSync(path.join(hostDir, 'outer/.config/gh/hosts.yml'), 'DUMMY'); + fs.mkdirSync(path.join(hostDir, 'inner'), { recursive: true }); + const target = `/host${HOME}/.config/gh/hosts.yml`; + + const result = pruneUnmountableCredentialOverlays([ + `${hostDir}/outer:/host${HOME}:ro`, + `${hostDir}/inner:/host${HOME}/.config:ro`, + overlay(target), + ]); + + expect(result).not.toContain(overlay(target)); + }); + + it('keeps overlays that land on the container rootfs with no covering bind', () => { + const volumes = [ + `${hostDir}:/host${HOME}:ro`, + '/dev/null:/host/var/run/docker.sock:ro', + '/dev/null:/host/run/docker.sock:ro', + ]; + + const result = pruneUnmountableCredentialOverlays(volumes); + + expect(result).toContain('/dev/null:/host/var/run/docker.sock:ro'); + expect(result).toContain('/dev/null:/host/run/docker.sock:ro'); + }); + + it('keeps overlays covered by a named volume, which cannot be probed', () => { + const target = `/host${HOME}/.docker/config.json`; + + const result = pruneUnmountableCredentialOverlays([ + `awf-home:/host${HOME}:ro`, + overlay(target), + ]); + + expect(result).toContain(overlay(target)); + }); + + it('never drops non-overlay mounts', () => { + const volumes = [`${hostDir}:/host${HOME}:ro`, '/tmp:/tmp:ro', `${hostDir}:/workspace:rw`]; + + expect(pruneUnmountableCredentialOverlays(volumes)).toEqual(volumes); }); }); diff --git a/src/services/agent-volumes/credential-hiding.ts b/src/services/agent-volumes/credential-hiding.ts index 34dc40d0b..86401f303 100644 --- a/src/services/agent-volumes/credential-hiding.ts +++ b/src/services/agent-volumes/credential-hiding.ts @@ -9,23 +9,9 @@ import { credentialFilesToHide } from '../../config/mount-policy'; * * Each credential file is masked twice: once at the real `$HOME` path and once * at the chroot `/host$HOME` path (the agent runs chrooted into `/host`). - * - * Only files that actually exist on the host are masked. A `/dev/null` overlay - * requires its mountpoint to already exist: runc creates a missing one by - * `openat(..., O_CREAT)` on the parent, which fails with EROFS once - * `filesystem.allowWrite` narrows the `$HOME` bind to read-only, taking the - * whole agent container down before it starts. Skipping absent files loses no - * protection, because a file that does not exist cannot leak a credential — - * every file that does exist is still masked, including inside a read-only - * parent (mounting over an existing path does not write to the filesystem). */ export function buildCredentialHidingOverlays(effectiveHome: string): string[] { - const allCredentialFiles = credentialFilesToHide().map((rel) => `${effectiveHome}/${rel}`); - const credentialFiles = allCredentialFiles.filter((credFile) => fs.existsSync(credFile)); - const skipped = allCredentialFiles.length - credentialFiles.length; - if (skipped > 0) { - logger.debug(`Skipped ${skipped} credential overlay(s) with no file on the host`); - } + const credentialFiles = credentialFilesToHide().map((rel) => `${effectiveHome}/${rel}`); const mounts = credentialFiles.map((credFile) => `/dev/null:${credFile}:ro`); logger.debug(`Hidden ${credentialFiles.length} credential file(s) via /dev/null mounts`); @@ -37,3 +23,85 @@ export function buildCredentialHidingOverlays(effectiveHome: string): string[] { return mounts; } + +interface ParsedMount { + source: string; + target: string; + mode: string; +} + +function parseMount(spec: string): ParsedMount | undefined { + const parts = spec.split(':'); + if (parts.length < 2 || !parts[0] || !parts[1]) return undefined; + return { + source: parts[0], + target: parts[1].replace(/\/+$/, '') || '/', + mode: parts[2] || 'rw', + }; +} + +/** + * Finds the innermost real bind whose target contains `target`, i.e. the mount + * that actually supplies the directory the overlay's mountpoint would live in. + */ +function innermostCoveringMount(binds: ParsedMount[], target: string): ParsedMount | undefined { + let best: ParsedMount | undefined; + for (const bind of binds) { + const covers = target === bind.target || target.startsWith(`${bind.target}/`); + if (!covers) continue; + if (!best || bind.target.length > best.target.length) best = bind; + } + return best; +} + +/** + * Drops `/dev/null` overlays whose mountpoint cannot physically be created. + * + * A bind mount requires its mountpoint to already exist; runc creates a missing + * one with `openat`/`mkdirat` on the parent directory. That silently works + * while every mount under `$HOME` is read-write, but once `filesystem.allowWrite` + * narrows those binds to read-only, runc fails with EROFS and the agent + * container dies before it starts. + * + * An overlay is kept unless its containing bind is read-only *and* the path it + * masks does not exist behind that bind. Dropping those loses no protection: + * the read-only bind is the only way the agent could reach the path, and there + * is nothing there to read. Every credential that is actually reachable is + * still masked, because mounting over a path that exists succeeds even inside a + * read-only bind. + * + * This is a no-op unless a write policy is active, since every covering bind is + * read-write otherwise. + */ +export function pruneUnmountableCredentialOverlays(volumes: string[]): string[] { + const binds = volumes + .map(parseMount) + .filter((mount): mount is ParsedMount => mount !== undefined && mount.source !== '/dev/null'); + + const kept = volumes.filter((spec) => { + const overlay = parseMount(spec); + if (!overlay || overlay.source !== '/dev/null') return true; + + const cover = innermostCoveringMount(binds, overlay.target); + // No covering bind: the mountpoint lives on the container's own writable + // rootfs, so runc can always create it. + if (!cover) return true; + if (cover.mode !== 'ro') return true; + // Named volumes and other non-path sources cannot be probed on the host. + if (!cover.source.startsWith('/')) return true; + + const suffix = overlay.target.slice(cover.target.length); + if (!suffix) return true; + return fs.existsSync(`${cover.source}${suffix}`); + }); + + const dropped = volumes.length - kept.length; + if (dropped > 0) { + logger.debug( + `Dropped ${dropped} credential overlay(s) whose target does not exist behind a read-only ` + + 'mount; those paths are unreadable in the container, so nothing is left unmasked', + ); + } + + return kept; +} diff --git a/src/services/agent-volumes/volume-builder.ts b/src/services/agent-volumes/volume-builder.ts index 6440a8760..b3230388f 100644 --- a/src/services/agent-volumes/volume-builder.ts +++ b/src/services/agent-volumes/volume-builder.ts @@ -2,7 +2,7 @@ import { SslConfig } from '../../host-env'; import { logger } from '../../logger'; import { WrapperConfig } from '../../types'; import { applyHostPathPrefixToVolumes } from '../host-path-prefix'; -import { buildCredentialHidingOverlays } from './credential-hiding'; +import { buildCredentialHidingOverlays, pruneUnmountableCredentialOverlays } from './credential-hiding'; import { buildDockerSocketMount } from './docker-socket'; import { buildEtcMounts } from './etc-mounts'; import { buildHomeMounts } from './home-strategy'; @@ -80,12 +80,12 @@ export function buildAgentVolumes(params: AgentVolumesParams): string[] { const localSourceRoots = new Map( customMounts.map((spec, index) => [spec, localCustomMounts[index]?.split(':')[0] ?? '']), ); - const policyVolumes = applyFilesystemWritePolicy( + const policyVolumes = pruneUnmountableCredentialOverlays(applyFilesystemWritePolicy( agentVolumes, resolveComposeFilesystemAllowWrite(config), alwaysWritableMounts, localSourceRoots, - ); + )); if (config.dockerHostPathPrefix) { return applyHostPathPrefixToVolumes(policyVolumes, config.dockerHostPathPrefix); From 82a45a536e73f9f83da3526a1dee84df1136313a Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 23 Aug 2026 19:07:13 -0700 Subject: [PATCH 07/18] fix: prepare the workspace mountpoint inside the chroot home Third and final instance of the same mountpoint-creation bug class, this time for a real bind rather than a credential overlay. On GitHub-hosted runners the workspace lives under $HOME (/home/runner/work//), so its /host-prefixed bind resolves inside the synthesized chroot home. Docker used to create those parent directories itself, but once filesystem.allowWrite narrows the chroot home bind to read-only runc cannot, and container init dies with: error mounting ".../work/gh-aw-firewall" to rootfs at "/host/home/runner/work/gh-aw-firewall/gh-aw-firewall": mkdirat .../merged/host/home/runner/work: read-only file system prepareChrootHomeMounts already solves exactly this for the whitelisted home tool paths and the runner tool cache, so reuse it for the workspace when the workspace is nested under the effective home. Paths outside $HOME are untouched, keeping current behavior for local runs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- src/chroot-home-setup.ts | 13 +++++++++++++ src/workdir-setup.test.ts | 38 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/chroot-home-setup.ts b/src/chroot-home-setup.ts index b2b0322fb..e0d06bcba 100644 --- a/src/chroot-home-setup.ts +++ b/src/chroot-home-setup.ts @@ -116,4 +116,17 @@ export function prepareChrootHomeMounts(config: WrapperConfig): void { logger.debug(`Prepared chroot runner tool cache mountpoint: ${chrootToolCachePath} (${uid}:${gid})`); } } + + // On GitHub-hosted runners the workspace lives under $HOME + // (`/home/runner/work//`), so its `/host`-prefixed bind lands + // inside the chroot home. Docker used to create those parents itself, but + // once filesystem.allowWrite narrows the chroot home bind to read-only runc + // can no longer do so and container init fails with EROFS. Prepare the + // mountpoint up front, exactly as for the tool-cache paths above. + const workspaceDir = process.env.GITHUB_WORKSPACE || process.cwd(); + const relativeWorkspacePath = path.relative(effectiveHome, workspaceDir); + if (relativeWorkspacePath && !relativeWorkspacePath.startsWith('..') && !path.isAbsolute(relativeWorkspacePath)) { + const chrootWorkspacePath = prepareChrootHomeMountpoint(emptyHomeDir, relativeWorkspacePath, uid, gid); + logger.debug(`Prepared chroot workspace mountpoint: ${chrootWorkspacePath} (${uid}:${gid})`); + } } diff --git a/src/workdir-setup.test.ts b/src/workdir-setup.test.ts index 637d816cf..e1c265c03 100644 --- a/src/workdir-setup.test.ts +++ b/src/workdir-setup.test.ts @@ -503,8 +503,7 @@ describe('prepareChrootHomeMounts (sub-function)', () => { expect(fs.existsSync(geminiDir)).toBe(false); }); - it('refuses existing symlinked nested home tool paths', () => { - const sandboxState = path.join(fixture.tempDir, '.local', 'state', 'sandboxes'); + it('refuses existing symlinked nested home tool paths', () => { const sandboxState = path.join(fixture.tempDir, '.local', 'state', 'sandboxes'); const localBin = path.join(fixture.tempDir, '.local', 'bin'); fs.mkdirSync(sandboxState, { recursive: true }); fs.symlinkSync(sandboxState, localBin); @@ -512,6 +511,41 @@ describe('prepareChrootHomeMounts (sub-function)', () => { expect(() => workdirSetupTestHelpers.prepareChrootHomeMounts(buildConfig())) .toThrow(`Refusing to use symlink as directory: ${localBin}`); }); + + // Regression: on GitHub-hosted runners the workspace lives under $HOME, so + // its /host-prefixed bind lands inside the chroot home. Docker used to create + // those parents, but filesystem.allowWrite narrows the chroot home bind to + // read-only and runc then fails container init with EROFS. + describe('workspace mountpoint', () => { + const originalWorkspace = process.env.GITHUB_WORKSPACE; + + afterEach(() => { + if (originalWorkspace === undefined) delete process.env.GITHUB_WORKSPACE; + else process.env.GITHUB_WORKSPACE = originalWorkspace; + }); + + it('prepares it inside the chroot home when the workspace is under $HOME', () => { + const workspace = path.join(fixture.tempDir, 'work', 'gh-aw-firewall', 'gh-aw-firewall'); + process.env.GITHUB_WORKSPACE = workspace; + + workdirSetupTestHelpers.prepareChrootHomeMounts(buildConfig()); + + const chrootHome = `${fixture.tempDir}-chroot-home`; + expect(fs.existsSync(path.join(chrootHome, 'work'))).toBe(true); + expect( + fs.existsSync(path.join(chrootHome, 'work', 'gh-aw-firewall', 'gh-aw-firewall')), + ).toBe(true); + }); + + it('leaves the chroot home alone when the workspace is outside $HOME', () => { + process.env.GITHUB_WORKSPACE = path.join(fixture.tempDir, '..', 'elsewhere', 'workspace'); + + workdirSetupTestHelpers.prepareChrootHomeMounts(buildConfig()); + + expect(fs.existsSync(path.join(`${fixture.tempDir}-chroot-home`, 'elsewhere'))).toBe(false); + expect(fs.existsSync(path.join(`${fixture.tempDir}-chroot-home`, 'workspace'))).toBe(false); + }); + }); }); describe('ensureDirectory EACCES diagnostic', () => { From 0cb81ea080feef6e3093171ff7d96cf09fa2829a Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 23 Aug 2026 19:23:19 -0700 Subject: [PATCH 08/18] fix: stage the chroot command script under /run, not /tmp Fourth instance of the same class, and the one that kept the agent from running its command once the container finally started: /usr/local/bin/entrypoint.sh: line 1295: /host/tmp/awf-cmd-1.sh: Read-only file system The entrypoint writes the user's command to a script inside the chroot to avoid nested-shell quoting problems. That script lived in /tmp, which filesystem.allowWrite narrows to read-only, so the write failed and the run aborted with exit 1. /run inside the chroot is the container's own writable rootfs rather than a host bind, so it stays writable under any host write policy. This is the same reasoning that moved the init signal to /run/awf-init. Cleanup still removes the script, so there is no residue. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- containers/agent/entrypoint.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/containers/agent/entrypoint.sh b/containers/agent/entrypoint.sh index 7ea38795a..b2b732ee3 100644 --- a/containers/agent/entrypoint.sh +++ b/containers/agent/entrypoint.sh @@ -1508,8 +1508,13 @@ run_chroot_command() { fi # Write the command to a temporary script file in the chroot - # This avoids complex quoting issues with nested shells - SCRIPT_FILE="/tmp/awf-cmd-$$.sh" + # This avoids complex quoting issues with nested shells. + # It lives under /run rather than /tmp because filesystem.allowWrite can + # narrow the /tmp bind to read-only, which would make this write fail. /run + # inside the chroot is the container's own writable rootfs, so it is always + # writable regardless of the host write policy. + mkdir -p /host/run 2>/dev/null || true + SCRIPT_FILE="/run/awf-cmd-$$.sh" build_path_script "$@" # Execute inside chroot: From 0f46d2bc65bff86b96e8bfe98b646acd3ffa91e5 Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 20:09:28 -0700 Subject: [PATCH 09/18] fix: prepare nested mountpoints and stage helpers off /tmp A bind mount needs its mountpoint to already exist. runc creates a missing one with mkdirat against the destination, which resolves into whichever bind already covers that path. That silently works while every covering bind is read-write, and fails with EROFS the moment filesystem.allowWrite narrows one to read-only. /tmp/awf-init was one symptom of a class. Derive the requirement from the final, policy-applied volume list rather than from a hand-maintained list of known paths, so internal mounts added later are covered automatically: - planNestedMountpoints() reports every mountpoint nested inside a read-only cover; ensureNestedMountpoints() creates the directories and then fails closed if any is still missing, instead of letting container init die with an opaque read-only filesystem error. This fixes $HOME/.copilot/logs and session-state, which sit under whichever of the empty chroot home or the real ~/.copilot ends up covering them. - Resolve bind sources back to runner-local paths before probing them. On split-filesystem runners custom mounts carry daemon-side sources, so pruneUnmountableCredentialOverlays could drop a /dev/null credential mask based on an unrelated runner path. Unknown sources now keep the overlay. - Stage agent helpers under /run/awf-lib instead of the host-bound /tmp/awf-lib. A write policy could narrow /tmp to read-only, and the failures were swallowed, silently disabling one-shot token protection and the gh CLI proxy wrapper. Both now fail closed when enabled. - Replace the init-container symlink, which lived in that container's own rootfs and was never visible to the agent, with a read-only bind of the same source at the legacy path in the agent service. A newer CLI now genuinely works with an older pinned agent image. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- CLAUDE.md | 2 +- containers/agent/entrypoint.sh | 132 ++++---- containers/agent/one-shot-token/README.md | 4 +- docs/chroot-mode.md | 2 +- src/agent-helper-staging.test.ts | 87 +++++ src/services/agent-service-build.test.ts | 2 +- src/services/agent-service.ts | 7 +- src/services/agent-volumes-basic.test.ts | 4 + .../agent-volumes/credential-hiding.test.ts | 60 ++++ .../agent-volumes/credential-hiding.ts | 51 ++- src/services/agent-volumes/mount-topology.ts | 84 +++++ .../agent-volumes/nested-mountpoints.test.ts | 301 ++++++++++++++++++ .../agent-volumes/nested-mountpoints.ts | 157 +++++++++ src/services/agent-volumes/volume-builder.ts | 25 +- .../agent-volumes/workspace-mounts.ts | 8 +- .../init-signal-compatibility.test.ts | 82 +++++ .../integration/filesystem-allowwrite.test.ts | 56 ++++ 17 files changed, 964 insertions(+), 100 deletions(-) create mode 100644 src/agent-helper-staging.test.ts create mode 100644 src/services/agent-volumes/mount-topology.ts create mode 100644 src/services/agent-volumes/nested-mountpoints.test.ts create mode 100644 src/services/agent-volumes/nested-mountpoints.ts create mode 100644 src/services/init-signal-compatibility.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index d54dd517a..68fac033f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,7 +156,7 @@ The codebase follows a modular architecture with clear separation of concerns: - Selective bind mounts under `/host/`: system binaries `/usr`, `/bin`, `/sbin`, `/lib`, `/lib64`, `/opt`, `/sys`, `/dev` (ro); workspace and `/tmp` (rw); whitelisted `$HOME` subdirs (rw); select `/etc` files — NOT a blanket host FS mount; `/etc/shadow`, unwhitelisted home dirs, and most of `/etc` are excluded - `entrypoint.sh` handles: UID/GID remapping → DNS config → SSL CA import → chroot to `/host` → capability drop → run user command as host user - **procfs mount**: A container-scoped procfs is mounted at `/host/proc` with `hidepid=2` to support runtimes that read `/proc/self/exe` (Java, .NET) while preventing the agent from reading other processes' `/proc/[pid]/environ` (credential isolation) -- **iptables init container** (`awf-iptables-init`): separate container sharing agent's network namespace via `network_mode: service:agent`. Runs `setup-iptables.sh` to configure NAT rules before user command starts. Agent waits for `/tmp/awf-init/ready` signal file. +- **iptables init container** (`awf-iptables-init`): separate container sharing agent's network namespace via `network_mode: service:agent`. Runs `setup-iptables.sh` to configure NAT rules before user command starts. Agent waits for `/run/awf-init/ready` signal file (also accepting the legacy `/tmp/awf-init/ready` so an older pinned agent image still works). - Key iptables rules (in `setup-iptables.sh`): - Allow localhost (for stdio MCP servers) and DNS - Allow traffic to Squid proxy itself diff --git a/containers/agent/entrypoint.sh b/containers/agent/entrypoint.sh index b2b732ee3..bf8f256de 100644 --- a/containers/agent/entrypoint.sh +++ b/containers/agent/entrypoint.sh @@ -602,36 +602,42 @@ mount_host_cgroupfs() { copy_preload_libs() { # Copy one-shot-token library to host filesystem for LD_PRELOAD in chroot # This prevents tokens from being read multiple times by malicious code - # Note: /tmp is always writable in chroot mode (mounted from host /tmp as rw) + # Staged under /run/awf-lib, which lives on the container's own writable + # rootfs. /tmp cannot be used: it is bind-mounted from the host and + # filesystem.allowWrite may narrow it to read-only, which would silently + # disable this protection. # Sets ONE_SHOT_TOKEN_LIB (empty string if unavailable or incompatible) ONE_SHOT_TOKEN_LIB="" if [ -f /usr/local/lib/one-shot-token.so ]; then - # Create the library directory in /tmp (always writable) - if mkdir -p /host/tmp/awf-lib 2>/dev/null; then + # Create the library directory on the container rootfs (always writable) + if mkdir -p /host/run/awf-lib 2>/dev/null; then # Copy the library and verify it exists after copying - if cp /usr/local/lib/one-shot-token.so /host/tmp/awf-lib/one-shot-token.so 2>/dev/null && \ - [ -f /host/tmp/awf-lib/one-shot-token.so ]; then + if cp /usr/local/lib/one-shot-token.so /host/run/awf-lib/one-shot-token.so 2>/dev/null && \ + [ -f /host/run/awf-lib/one-shot-token.so ]; then # Probe compatibility with the host's dynamic linker before committing to LD_PRELOAD. # Run the probe inside chroot /host so the ELF interpreter and all library paths # (e.g. /lib/ld-musl-*.so.1 on Alpine) resolve against the host filesystem, not # the container's. This avoids false negatives where the container's glibc # interpreter would accept the .so even though the host loader cannot. - if chroot /host /bin/sh -c 'LD_PRELOAD=/tmp/awf-lib/one-shot-token.so /bin/true' 2>/dev/null; then - ONE_SHOT_TOKEN_LIB="/tmp/awf-lib/one-shot-token.so" + if chroot /host /bin/sh -c 'LD_PRELOAD=/run/awf-lib/one-shot-token.so /bin/true' 2>/dev/null; then + ONE_SHOT_TOKEN_LIB="/run/awf-lib/one-shot-token.so" echo "[entrypoint] One-shot token library copied to chroot at ${ONE_SHOT_TOKEN_LIB}" else echo "[entrypoint][WARN] one-shot-token.so failed to load on host dynamic linker (host libc incompatibility, e.g. musl/Alpine)" echo "[entrypoint][WARN] Token protection will be disabled (tokens may be readable multiple times)" - rm -f /host/tmp/awf-lib/one-shot-token.so 2>/dev/null || true + rm -f /host/run/awf-lib/one-shot-token.so 2>/dev/null || true fi else - echo "[entrypoint][WARN] Could not copy one-shot-token library to /tmp/awf-lib" - echo "[entrypoint][WARN] Token protection will be disabled (tokens may be readable multiple times)" + # The library exists but could not be staged. Continuing would silently + # drop a security control, so fail closed instead. + echo "[entrypoint][ERROR] Could not copy one-shot-token library to /run/awf-lib" >&2 + echo "[entrypoint][ERROR] Refusing to start without one-shot token protection" >&2 + exit 1 fi else - echo "[entrypoint][ERROR] Could not create /tmp/awf-lib directory" - echo "[entrypoint][ERROR] This should not happen - /tmp is mounted read-write in chroot mode" - echo "[entrypoint][WARN] Token protection will be disabled (tokens may be readable multiple times)" + echo "[entrypoint][ERROR] Could not create /run/awf-lib directory" >&2 + echo "[entrypoint][ERROR] Refusing to start without one-shot token protection" >&2 + exit 1 fi fi } @@ -639,18 +645,19 @@ copy_preload_libs() { copy_agent_helper_scripts() { # Copy get-claude-key.sh and gh CLI proxy wrapper to chroot-accessible paths. # Both scripts are baked into the Docker image but shadowed by host bind mounts - # inside chroot. They are copied to /tmp/awf-lib/ (always writable) so they - # remain accessible after the chroot activates. + # inside chroot. They are copied to /run/awf-lib/ -- on the container's own + # writable rootfs, not the host-bound /tmp which filesystem.allowWrite may + # narrow to read-only -- so they remain accessible after the chroot activates. # Sets CHROOT_KEY_HELPER; may update AWF_HOST_PATH. # Copy get-claude-key.sh to chroot-accessible path # The script is baked into the Docker image at /usr/local/bin/, but the chroot # bind-mounts the host's /usr (read-only), shadowing the container's copy. - # We must copy it to /tmp/awf-lib/ (writable) before the chroot activates. + # We must copy it to /run/awf-lib/ (container rootfs) before the chroot activates. CHROOT_KEY_HELPER="" if [ -n "$CLAUDE_CODE_API_KEY_HELPER" ] && [ -f "$CLAUDE_CODE_API_KEY_HELPER" ]; then - if mkdir -p /host/tmp/awf-lib 2>/dev/null; then - CHROOT_KEY_HELPER="/tmp/awf-lib/$(basename "$CLAUDE_CODE_API_KEY_HELPER")" + if mkdir -p /host/run/awf-lib 2>/dev/null; then + CHROOT_KEY_HELPER="/run/awf-lib/$(basename "$CLAUDE_CODE_API_KEY_HELPER")" if cp "$CLAUDE_CODE_API_KEY_HELPER" "/host${CHROOT_KEY_HELPER}" 2>/dev/null && \ chmod +x "/host${CHROOT_KEY_HELPER}" 2>/dev/null; then echo "[entrypoint] Claude key helper copied to chroot at ${CHROOT_KEY_HELPER}" @@ -680,19 +687,22 @@ copy_agent_helper_scripts() { # Activate gh CLI proxy wrapper when CLI proxy sidecar is enabled. # The wrapper at /usr/local/bin/gh-cli-proxy-wrapper.sh (baked into the image) - # is copied to /tmp/awf-lib/gh so it is accessible inside the chroot at a + # is copied to /run/awf-lib/gh so it is accessible inside the chroot at a # location that takes precedence over the host's /usr/bin/gh mount. if [ -n "$AWF_CLI_PROXY_URL" ] && [ -f /usr/local/bin/gh-cli-proxy-wrapper.sh ]; then - if mkdir -p /host/tmp/awf-lib 2>/dev/null; then - if cp /usr/local/bin/gh-cli-proxy-wrapper.sh /host/tmp/awf-lib/gh 2>/dev/null && \ - chmod +x /host/tmp/awf-lib/gh 2>/dev/null; then - # The chroot will see this as /tmp/awf-lib/gh (the /host prefix is the bind mount) - echo "[entrypoint] gh CLI proxy wrapper installed at /tmp/awf-lib/gh (inside chroot)" - # Prepend /tmp/awf-lib to PATH so the wrapper takes precedence over host gh - export AWF_HOST_PATH="/tmp/awf-lib:${AWF_HOST_PATH:-$PATH}" - else - echo "[entrypoint][WARN] Could not install gh CLI proxy wrapper" - fi + if mkdir -p /host/run/awf-lib 2>/dev/null && \ + cp /usr/local/bin/gh-cli-proxy-wrapper.sh /host/run/awf-lib/gh 2>/dev/null && \ + chmod +x /host/run/awf-lib/gh 2>/dev/null; then + # The chroot will see this as /run/awf-lib/gh (the /host prefix is the bind mount) + echo "[entrypoint] gh CLI proxy wrapper installed at /run/awf-lib/gh (inside chroot)" + # Prepend /run/awf-lib to PATH so the wrapper takes precedence over host gh + export AWF_HOST_PATH="/run/awf-lib:${AWF_HOST_PATH:-$PATH}" + else + # The CLI proxy is enabled, so an unwrapped gh would bypass credential + # mediation entirely. Fail closed rather than silently downgrade. + echo "[entrypoint][ERROR] Could not install gh CLI proxy wrapper at /run/awf-lib/gh" >&2 + echo "[entrypoint][ERROR] Refusing to start with an unmediated gh CLI" >&2 + exit 1 fi fi @@ -701,7 +711,7 @@ copy_agent_helper_scripts() { copy_dind_runner_binary() { # In split-filesystem DinD setups with --docker-host-path-prefix pointing at # a shared /tmp root, docker-manager stages the invoking CLI binary under - # /tmp/awf-runner-bin/. Copy it into /tmp/awf-lib so the chrooted PATH + # /tmp/awf-runner-bin/. Copy it into /run/awf-lib so the chrooted PATH # can resolve the expected command name (copilot, claude, etc.) without # requiring manual bootstrap copies into the daemon's /usr/local/bin. # Sets STAGED_RUNNER_BINARY_CHROOT; may update AWF_HOST_PATH. @@ -710,13 +720,13 @@ copy_dind_runner_binary() { if [[ ! "${AWF_STAGED_RUNNER_BINARY_NAME}" =~ ^[A-Za-z0-9_][A-Za-z0-9_.-]*$ ]]; then echo "[entrypoint][WARN] Ignoring invalid AWF_STAGED_RUNNER_BINARY_NAME=${AWF_STAGED_RUNNER_BINARY_NAME}" elif [ -f "/tmp/awf-runner-bin/${AWF_STAGED_RUNNER_BINARY_NAME}" ]; then - if mkdir -p /host/tmp/awf-lib 2>/dev/null; then - if cp "/tmp/awf-runner-bin/${AWF_STAGED_RUNNER_BINARY_NAME}" "/host/tmp/awf-lib/${AWF_STAGED_RUNNER_BINARY_NAME}" 2>/dev/null && \ - chmod +x "/host/tmp/awf-lib/${AWF_STAGED_RUNNER_BINARY_NAME}" 2>/dev/null; then - STAGED_RUNNER_BINARY_CHROOT="/tmp/awf-lib/${AWF_STAGED_RUNNER_BINARY_NAME}" + if mkdir -p /host/run/awf-lib 2>/dev/null; then + if cp "/tmp/awf-runner-bin/${AWF_STAGED_RUNNER_BINARY_NAME}" "/host/run/awf-lib/${AWF_STAGED_RUNNER_BINARY_NAME}" 2>/dev/null && \ + chmod +x "/host/run/awf-lib/${AWF_STAGED_RUNNER_BINARY_NAME}" 2>/dev/null; then + STAGED_RUNNER_BINARY_CHROOT="/run/awf-lib/${AWF_STAGED_RUNNER_BINARY_NAME}" case ":${AWF_HOST_PATH:-$PATH}:" in - *":/tmp/awf-lib:"*) ;; - *) export AWF_HOST_PATH="/tmp/awf-lib:${AWF_HOST_PATH:-$PATH}" ;; + *":/run/awf-lib:"*) ;; + *) export AWF_HOST_PATH="/run/awf-lib:${AWF_HOST_PATH:-$PATH}" ;; esac echo "[entrypoint] Runner binary staged for chroot at ${STAGED_RUNNER_BINARY_CHROOT}" else @@ -761,7 +771,7 @@ resolve_chroot_binary_path() { if [ -n "${AWF_HOST_PATH:-}" ]; then search_path="${search_path}${AWF_HOST_PATH}:" fi - search_path="${search_path}/tmp/awf-runner-bin:/tmp/awf-lib:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" + search_path="${search_path}/tmp/awf-runner-bin:/run/awf-lib:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" local dir="" local IFS=':' @@ -949,15 +959,15 @@ copy_awf_ca_cert() { # Copy AWF CA certificate to chroot-accessible path for ssl-bump TLS trust. # NODE_EXTRA_CA_CERTS points to /usr/local/share/ca-certificates/awf-ca.crt which # is a Docker volume mount on the container's overlay filesystem. After chroot /host, - # this path is inaccessible. Copy to /tmp/awf-lib/ (always writable) and update the + # this path is inaccessible. Copy to /run/awf-lib/ (container rootfs) and update the # env var so Node.js (Claude Code), curl, git, Python, etc. trust the Squid CA. # Sets AWF_CA_CHROOT; exports NODE_EXTRA_CA_CERTS, SSL_CERT_FILE, REQUESTS_CA_BUNDLE. AWF_CA_CHROOT="" if [ "${AWF_SSL_BUMP_ENABLED}" = "true" ] && [ -f /usr/local/share/ca-certificates/awf-ca.crt ]; then - if mkdir -p /host/tmp/awf-lib 2>/dev/null; then - if cp /usr/local/share/ca-certificates/awf-ca.crt /host/tmp/awf-lib/awf-ca.crt 2>/dev/null && \ - [ -f /host/tmp/awf-lib/awf-ca.crt ]; then - AWF_CA_CHROOT="/tmp/awf-lib/awf-ca.crt" + if mkdir -p /host/run/awf-lib 2>/dev/null; then + if cp /usr/local/share/ca-certificates/awf-ca.crt /host/run/awf-lib/awf-ca.crt 2>/dev/null && \ + [ -f /host/run/awf-lib/awf-ca.crt ]; then + AWF_CA_CHROOT="/run/awf-lib/awf-ca.crt" export NODE_EXTRA_CA_CERTS="$AWF_CA_CHROOT" # SSL_CERT_FILE is respected by curl, git, Python requests, Ruby, and most # OpenSSL-based tools. This ensures non-Node.js tools also trust the AWF CA. @@ -970,7 +980,7 @@ copy_awf_ca_cert() { echo "[entrypoint][WARN] Could not copy AWF CA certificate to chroot — ssl-bump TLS may fail" fi else - echo "[entrypoint][WARN] Could not create /host/tmp/awf-lib for CA cert — ssl-bump TLS may fail in chroot" + echo "[entrypoint][WARN] Could not create /host/run/awf-lib for CA cert — ssl-bump TLS may fail in chroot" fi fi } @@ -979,7 +989,7 @@ copy_system_ca_bundle() { # Detect and copy the host system CA bundle to a chroot-accessible path. # On Amazon Linux / RHEL-family systems, the CA bundle often lives under # /etc/pki/. This function finds the system bundle and, when it is not already - # accessible in the chroot, copies it to /tmp/awf-lib/ so TLS works regardless + # accessible in the chroot, copies it to /run/awf-lib/ so TLS works regardless # of distro. # # In SSL Bump mode, the AWF CA must remain the active trust bundle for MITM @@ -1052,10 +1062,10 @@ copy_system_ca_bundle() { esac # Bundle is not accessible in chroot. Copy it. - if mkdir -p /host/tmp/awf-lib 2>/dev/null; then - if cp "$SYSTEM_BUNDLE" /host/tmp/awf-lib/system-ca-certificates.crt 2>/dev/null && \ - [ -s /host/tmp/awf-lib/system-ca-certificates.crt ]; then - local CA_PATH="/tmp/awf-lib/system-ca-certificates.crt" + if mkdir -p /host/run/awf-lib 2>/dev/null; then + if cp "$SYSTEM_BUNDLE" /host/run/awf-lib/system-ca-certificates.crt 2>/dev/null && \ + [ -s /host/run/awf-lib/system-ca-certificates.crt ]; then + local CA_PATH="/run/awf-lib/system-ca-certificates.crt" SYSTEM_CA_CHROOT="$CA_PATH" export SSL_CERT_FILE="$CA_PATH" export NODE_EXTRA_CA_CERTS="$CA_PATH" @@ -1067,7 +1077,7 @@ copy_system_ca_bundle() { echo "[entrypoint][WARN] Could not copy system CA bundle to chroot — TLS may fail" fi else - echo "[entrypoint][WARN] Could not create /host/tmp/awf-lib for system CA bundle" + echo "[entrypoint][WARN] Could not create /host/run/awf-lib for system CA bundle" fi } @@ -1546,9 +1556,9 @@ run_chroot_command() { CLEANUP_CMD="${CLEANUP_CMD}; sed -i '/^[0-9.]\\+[[:space:]]\\+host\\.docker\\.internal\$/d' /etc/hosts 2>/dev/null || true" echo "[entrypoint] host.docker.internal will be removed from /etc/hosts on exit" fi - # Clean up /tmp/awf-lib if anything was copied (one-shot-token, CA cert, key helper) + # Clean up /run/awf-lib if anything was copied (one-shot-token, CA cert, key helper) if [ -n "${ONE_SHOT_TOKEN_LIB}" ] || [ -n "${AWF_CA_CHROOT}" ] || [ -n "${SYSTEM_CA_CHROOT}" ] || [ -n "${CHROOT_KEY_HELPER}" ] || [ -n "${STAGED_RUNNER_BINARY_CHROOT}" ]; then - CLEANUP_CMD="${CLEANUP_CMD}; rm -rf /tmp/awf-lib 2>/dev/null || true" + CLEANUP_CMD="${CLEANUP_CMD}; rm -rf /run/awf-lib 2>/dev/null || true" fi # NOTE: the /usr/local/bin overlay is torn down by cleanup_usr_local_bin_overlay(), # which is installed as an EXIT trap in the container's root shell — the chroot @@ -1616,16 +1626,20 @@ run_non_chroot_command() { # Drop capabilities and privileges, then execute the user command # Activate gh CLI proxy wrapper in non-chroot mode. - # Copy the wrapper to /tmp/awf-lib/gh so it takes precedence over - # the system gh at /usr/bin/gh (since /tmp/awf-lib is prepended to PATH). + # Copy the wrapper to /run/awf-lib/gh so it takes precedence over + # the system gh at /usr/bin/gh (since /run/awf-lib is prepended to PATH). if [ -n "$AWF_CLI_PROXY_URL" ] && [ -f /usr/local/bin/gh-cli-proxy-wrapper.sh ]; then - mkdir -p /tmp/awf-lib - if cp /usr/local/bin/gh-cli-proxy-wrapper.sh /tmp/awf-lib/gh 2>/dev/null && \ - chmod +x /tmp/awf-lib/gh 2>/dev/null; then - export PATH="/tmp/awf-lib:${PATH}" - echo "[entrypoint] gh CLI proxy wrapper installed at /tmp/awf-lib/gh" + mkdir -p /run/awf-lib + if cp /usr/local/bin/gh-cli-proxy-wrapper.sh /run/awf-lib/gh 2>/dev/null && \ + chmod +x /run/awf-lib/gh 2>/dev/null; then + export PATH="/run/awf-lib:${PATH}" + echo "[entrypoint] gh CLI proxy wrapper installed at /run/awf-lib/gh" else - echo "[entrypoint][WARN] Could not install gh CLI proxy wrapper" + # The CLI proxy is enabled, so an unwrapped gh would bypass credential + # mediation entirely. Fail closed rather than silently downgrade. + echo "[entrypoint][ERROR] Could not install gh CLI proxy wrapper at /run/awf-lib/gh" >&2 + echo "[entrypoint][ERROR] Refusing to start with an unmediated gh CLI" >&2 + exit 1 fi fi diff --git a/containers/agent/one-shot-token/README.md b/containers/agent/one-shot-token/README.md index 84c3190cc..7c93256b7 100644 --- a/containers/agent/one-shot-token/README.md +++ b/containers/agent/one-shot-token/README.md @@ -174,8 +174,8 @@ exec capsh --drop=$CAPS_TO_DROP -- -c "exec gosu awfuser $COMMAND" In chroot mode, the library must be accessible from within the chroot (host filesystem). The entrypoint: -1. Copies the library from container to `/host/tmp/awf-lib/one-shot-token.so` -2. Sets `LD_PRELOAD=/tmp/awf-lib/one-shot-token.so` inside the chroot +1. Copies the library from container to `/host/run/awf-lib/one-shot-token.so` +2. Sets `LD_PRELOAD=/run/awf-lib/one-shot-token.so` inside the chroot 3. Cleans up the library on exit ## Building diff --git a/docs/chroot-mode.md b/docs/chroot-mode.md index 9083eb986..b3e2d09c9 100644 --- a/docs/chroot-mode.md +++ b/docs/chroot-mode.md @@ -193,7 +193,7 @@ When `chroot.binariesSourcePath` is set in stdin config, AWF also mounts: **Note:** As of v0.13.13, `/proc` is no longer bind-mounted. Instead, a fresh container-scoped procfs is mounted at `/host/proc` during entrypoint initialization. This provides dynamic `/proc/self/exe` resolution required by Java and .NET runtimes. -**System CA Bundle Detection:** The entrypoint automatically detects the host system CA bundle from common locations (Debian/Ubuntu `/etc/ssl/certs/ca-certificates.crt`, RHEL/Amazon Linux `/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem`, `/etc/pki/tls/certs/ca-bundle.crt`, `/etc/pki/tls/cert.pem`, macOS `/etc/ssl/cert.pem`). If the bundle is not already accessible in the chroot via the mounted CA paths, it is copied to `/tmp/awf-lib/system-ca-certificates.crt` and `SSL_CERT_FILE`, `NODE_EXTRA_CA_CERTS`, `REQUESTS_CA_BUNDLE`, `CURL_CA_BUNDLE`, and `GIT_SSL_CAINFO` are set to point at it. +**System CA Bundle Detection:** The entrypoint automatically detects the host system CA bundle from common locations (Debian/Ubuntu `/etc/ssl/certs/ca-certificates.crt`, RHEL/Amazon Linux `/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem`, `/etc/pki/tls/certs/ca-bundle.crt`, `/etc/pki/tls/cert.pem`, macOS `/etc/ssl/cert.pem`). If the bundle is not already accessible in the chroot via the mounted CA paths, it is copied to `/run/awf-lib/system-ca-certificates.crt` and `SSL_CERT_FILE`, `NODE_EXTRA_CA_CERTS`, `REQUESTS_CA_BUNDLE`, `CURL_CA_BUNDLE`, and `GIT_SSL_CAINFO` are set to point at it. ### Read-Write Mounts diff --git a/src/agent-helper-staging.test.ts b/src/agent-helper-staging.test.ts new file mode 100644 index 000000000..e8597125e --- /dev/null +++ b/src/agent-helper-staging.test.ts @@ -0,0 +1,87 @@ +import * as fs from 'fs'; +import * as path from 'path'; + +const entrypointSource = fs.readFileSync( + path.resolve(__dirname, '../containers/agent/entrypoint.sh'), + 'utf8', +); + +/** + * AWF stages several helpers on the host side of the chroot before the agent + * runs: the one-shot-token LD_PRELOAD library, the gh CLI proxy wrapper, the + * Claude key helper and CA bundles. + * + * These used to live under `/tmp/awf-lib`. That is a bind mount of the host's + * `/tmp`, so `filesystem.allowWrite` can narrow it to read-only, at which point + * every one of those copies fails. The failures were swallowed, silently + * disabling security controls while the run appeared healthy. + * + * They now stage under `/run/awf-lib`, which lives on the container's own + * writable rootfs and is therefore never affected by a user write policy. + */ +describe('agent helper staging location', () => { + it('never stages helpers under the host-bound /tmp', () => { + expect(entrypointSource).not.toContain('/tmp/awf-lib'); + }); + + it('stages helpers under /run/awf-lib on both sides of the chroot', () => { + expect(entrypointSource).toContain('mkdir -p /host/run/awf-lib'); + expect(entrypointSource).toContain('/host/run/awf-lib/one-shot-token.so'); + expect(entrypointSource).toContain('/host/run/awf-lib/gh'); + }); + + it('preloads the one-shot token library from the chroot-visible path', () => { + expect(entrypointSource).toContain('LD_PRELOAD=/run/awf-lib/one-shot-token.so'); + }); + + it('puts the staging directory on PATH so the gh wrapper shadows the host gh', () => { + expect(entrypointSource).toContain('export AWF_HOST_PATH="/run/awf-lib:${AWF_HOST_PATH:-$PATH}"'); + expect(entrypointSource).toContain('export PATH="/run/awf-lib:${PATH}"'); + }); + + it('cleans up the staging directory at its new location', () => { + expect(entrypointSource).toContain('rm -rf /run/awf-lib'); + }); + + it('no longer claims /tmp is always writable', () => { + expect(entrypointSource).not.toContain('/tmp is mounted read-write in chroot mode'); + expect(entrypointSource).not.toMatch(/\/tmp\/awf-lib\/ \(always writable\)/); + }); + + describe('fail-closed behaviour', () => { + function functionBody(name: string): string { + const start = entrypointSource.indexOf(`${name}() {`); + expect(start).toBeGreaterThan(-1); + const end = entrypointSource.indexOf('\n}\n', start); + return entrypointSource.slice(start, end); + } + + it('refuses to start when the one-shot token library cannot be staged', () => { + const body = functionBody('copy_preload_libs'); + + expect(body).toContain('Refusing to start without one-shot token protection'); + // Both the mkdir and the copy failure paths must abort. + expect(body.match(/exit 1/g) ?? []).toHaveLength(2); + expect(body).not.toContain('Token protection will be disabled (tokens may be readable multiple times)\n fi'); + }); + + it('still tolerates a host libc that cannot load the library', () => { + // An incompatible dynamic linker (musl/Alpine) is an environment property, + // not a staging failure, and must stay a warning. + const body = functionBody('copy_preload_libs'); + + expect(body).toContain('host libc incompatibility'); + expect(body).toContain('Token protection will be disabled'); + }); + + it('refuses to start with an unmediated gh CLI when the proxy is enabled', () => { + const occurrences = entrypointSource.match( + /Refusing to start with an unmediated gh CLI/g, + ) ?? []; + + // Once for the chroot path, once for the non-chroot path. + expect(occurrences).toHaveLength(2); + expect(entrypointSource).not.toContain('[entrypoint][WARN] Could not install gh CLI proxy wrapper'); + }); + }); +}); diff --git a/src/services/agent-service-build.test.ts b/src/services/agent-service-build.test.ts index 04eeb9f20..f8b7c340d 100644 --- a/src/services/agent-service-build.test.ts +++ b/src/services/agent-service-build.test.ts @@ -124,7 +124,7 @@ describe('agent service', () => { expect(initService.entrypoint).toEqual(['/bin/bash']); expect(initService.command).toEqual([ '-c', - 'mkdir -p "$$AWF_INIT_SIGNAL_DIR" && if [ ! -e /tmp/awf-init ]; then ln -s "$$AWF_INIT_SIGNAL_DIR" /tmp/awf-init 2>/dev/null || true; fi && /usr/local/bin/setup-iptables.sh > "$$AWF_INIT_SIGNAL_DIR/output.log" 2>&1 && touch "$$AWF_INIT_SIGNAL_DIR/ready"', + 'mkdir -p "$$AWF_INIT_SIGNAL_DIR" && /usr/local/bin/setup-iptables.sh > "$$AWF_INIT_SIGNAL_DIR/output.log" 2>&1 && touch "$$AWF_INIT_SIGNAL_DIR/ready"', ]); expect(initService.security_opt).toBeUndefined(); expect(initService.restart).toBe('no'); diff --git a/src/services/agent-service.ts b/src/services/agent-service.ts index 8457a4d82..d72c8064a 100644 --- a/src/services/agent-service.ts +++ b/src/services/agent-service.ts @@ -2,7 +2,6 @@ import * as path from 'path'; import { AGENT_CONTAINER_NAME, INIT_SIGNAL_DIR, - LEGACY_INIT_SIGNAL_DIR, IPTABLES_INIT_CONTAINER_NAME, SQUID_PORT, } from '../constants'; @@ -294,9 +293,13 @@ interface IptablesInitServiceParams { */ export function buildIptablesInitService(params: IptablesInitServiceParams): any { const { agentService, environment, networkConfig, initSignalDir, dockerHostPathPrefix, hostGatewayIp } = params; + // No legacy-path compatibility shim here: this container has its own mount + // namespace and rootfs, so anything created at LEGACY_INIT_SIGNAL_DIR inside + // it is invisible to the agent container. Older agent images are supported by + // binding the same host source at the legacy path in the *agent* service + // instead (see buildWorkspaceMounts). const setupCommand = [ 'mkdir -p "$$AWF_INIT_SIGNAL_DIR"', - `if [ ! -e ${LEGACY_INIT_SIGNAL_DIR} ]; then ln -s "$$AWF_INIT_SIGNAL_DIR" ${LEGACY_INIT_SIGNAL_DIR} 2>/dev/null || true; fi`, '/usr/local/bin/setup-iptables.sh > "$$AWF_INIT_SIGNAL_DIR/output.log" 2>&1', 'touch "$$AWF_INIT_SIGNAL_DIR/ready"', ].join(' && '); diff --git a/src/services/agent-volumes-basic.test.ts b/src/services/agent-volumes-basic.test.ts index e2c0410cd..487c5188f 100644 --- a/src/services/agent-volumes-basic.test.ts +++ b/src/services/agent-volumes-basic.test.ts @@ -64,6 +64,10 @@ describe('agent service', () => { expect(volumes).toContain('/tmp:/tmp:ro'); expect(volumes).toContain('/tmp:/host/tmp:ro'); expect(volumes).toContain(`${getConfig().workDir}/init-signal:/run/awf-init:rw`); + // Legacy path stays exposed read-only so a newer CLI still works with an + // older pinned agent image, and it must not become a writable hole in the + // narrowed /tmp tree. + expect(volumes).toContain(`${getConfig().workDir}/init-signal:/tmp/awf-init:ro`); expect(volumes).not.toContain(`${getConfig().workDir}/init-signal:/tmp/awf-init:rw`); expect(volumes.some((volume) => volume.includes('/.copilot/logs:rw'))).toBe(true); }); diff --git a/src/services/agent-volumes/credential-hiding.test.ts b/src/services/agent-volumes/credential-hiding.test.ts index 0658d83b0..fc60b35cb 100644 --- a/src/services/agent-volumes/credential-hiding.test.ts +++ b/src/services/agent-volumes/credential-hiding.test.ts @@ -3,6 +3,7 @@ import * as os from 'os'; import * as path from 'path'; import { buildCredentialHidingOverlays, pruneUnmountableCredentialOverlays } from './credential-hiding'; import { credentialFilesToHide } from '../../config/mount-policy'; +import { createLocalSourceResolver } from './mount-topology'; describe('buildCredentialHidingOverlays', () => { it('hides every policy credential file at both home and /host paths', () => { @@ -129,4 +130,63 @@ describe('pruneUnmountableCredentialOverlays', () => { expect(pruneUnmountableCredentialOverlays(volumes)).toEqual(volumes); }); + + describe('split-filesystem runners (--docker-host-path-prefix)', () => { + it('keeps an overlay when the covering custom mount maps to a real runner path', () => { + const localRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'awf-local-')); + fs.mkdirSync(path.join(localRoot, '.docker'), { recursive: true }); + fs.writeFileSync(path.join(localRoot, '.docker/config.json'), '{}'); + + const resolver = createLocalSourceResolver(new Map([['/daemon' + localRoot, localRoot]]), '/daemon'); + const target = `/host${HOME}/.docker/config.json`; + + const result = pruneUnmountableCredentialOverlays( + [`/daemon${localRoot}:/host${HOME}:ro`, overlay(target)], + resolver, + ); + + expect(result).toContain(overlay(target)); + fs.rmSync(localRoot, { recursive: true, force: true }); + }); + + it('keeps an overlay whose covering custom mount has no known runner path', () => { + // Fail closed: probing '/daemon/staged/...' with runner-local fs would + // report "missing" and silently unmask a real credential file. + const resolver = createLocalSourceResolver(new Map([['/daemon/staged', '']]), '/daemon'); + const target = `/host${HOME}/.docker/config.json`; + + const result = pruneUnmountableCredentialOverlays( + [`/daemon/staged:/host${HOME}:ro`, overlay(target)], + resolver, + ); + + expect(result).toContain(overlay(target)); + }); + + it('keeps an overlay behind an unattributable daemon-prefixed source', () => { + const resolver = createLocalSourceResolver(new Map(), '/daemon'); + const target = `/host${HOME}/.npmrc`; + + const result = pruneUnmountableCredentialOverlays( + [`/daemon/tmp/chroot-home:/host${HOME}:ro`, overlay(target)], + resolver, + ); + + expect(result).toContain(overlay(target)); + }); + + it('still drops an unreachable overlay when the runner path is known', () => { + const localRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'awf-local-')); + const resolver = createLocalSourceResolver(new Map([['/daemon' + localRoot, localRoot]]), '/daemon'); + const target = `/host${HOME}/.docker/config.json`; + + const result = pruneUnmountableCredentialOverlays( + [`/daemon${localRoot}:/host${HOME}:ro`, overlay(target)], + resolver, + ); + + expect(result).not.toContain(overlay(target)); + fs.rmSync(localRoot, { recursive: true, force: true }); + }); + }); }); diff --git a/src/services/agent-volumes/credential-hiding.ts b/src/services/agent-volumes/credential-hiding.ts index 86401f303..901f13c8c 100644 --- a/src/services/agent-volumes/credential-hiding.ts +++ b/src/services/agent-volumes/credential-hiding.ts @@ -1,6 +1,13 @@ import * as fs from 'fs'; import { logger } from '../../logger'; import { credentialFilesToHide } from '../../config/mount-policy'; +import { + LocalSourceResolver, + ParsedMount, + identityLocalSourceResolver, + innermostCoveringMount, + parseMount, +} from './mount-topology'; /** * Builds the compose-mode `/dev/null` overlays that blank known on-disk @@ -24,36 +31,6 @@ export function buildCredentialHidingOverlays(effectiveHome: string): string[] { return mounts; } -interface ParsedMount { - source: string; - target: string; - mode: string; -} - -function parseMount(spec: string): ParsedMount | undefined { - const parts = spec.split(':'); - if (parts.length < 2 || !parts[0] || !parts[1]) return undefined; - return { - source: parts[0], - target: parts[1].replace(/\/+$/, '') || '/', - mode: parts[2] || 'rw', - }; -} - -/** - * Finds the innermost real bind whose target contains `target`, i.e. the mount - * that actually supplies the directory the overlay's mountpoint would live in. - */ -function innermostCoveringMount(binds: ParsedMount[], target: string): ParsedMount | undefined { - let best: ParsedMount | undefined; - for (const bind of binds) { - const covers = target === bind.target || target.startsWith(`${bind.target}/`); - if (!covers) continue; - if (!best || bind.target.length > best.target.length) best = bind; - } - return best; -} - /** * Drops `/dev/null` overlays whose mountpoint cannot physically be created. * @@ -73,7 +50,10 @@ function innermostCoveringMount(binds: ParsedMount[], target: string): ParsedMou * This is a no-op unless a write policy is active, since every covering bind is * read-write otherwise. */ -export function pruneUnmountableCredentialOverlays(volumes: string[]): string[] { +export function pruneUnmountableCredentialOverlays( + volumes: string[], + localSourceResolver: LocalSourceResolver = identityLocalSourceResolver, +): string[] { const binds = volumes .map(parseMount) .filter((mount): mount is ParsedMount => mount !== undefined && mount.source !== '/dev/null'); @@ -92,7 +72,14 @@ export function pruneUnmountableCredentialOverlays(volumes: string[]): string[] const suffix = overlay.target.slice(cover.target.length); if (!suffix) return true; - return fs.existsSync(`${cover.source}${suffix}`); + + // On split-filesystem runners the covering source may be a daemon-side path + // that means nothing to `fs` here. Keep the overlay rather than risk + // unmasking a credential based on an unrelated runner path. + const localCoverSource = localSourceResolver(cover.source); + if (localCoverSource === undefined) return true; + + return fs.existsSync(`${localCoverSource}${suffix}`); }); const dropped = volumes.length - kept.length; diff --git a/src/services/agent-volumes/mount-topology.ts b/src/services/agent-volumes/mount-topology.ts new file mode 100644 index 000000000..9367e203f --- /dev/null +++ b/src/services/agent-volumes/mount-topology.ts @@ -0,0 +1,84 @@ +/** + * Shared helpers for reasoning about the *final* compose bind topology. + * + * Both the credential-overlay prune and the nested-mountpoint preparation need + * to answer the same two questions about a generated volume list: + * + * 1. Which bind covers a given container path once runc has mounted parents + * first (the innermost strictly-shallower bind)? + * 2. Where does that bind's source live *on the runner*, so we can probe or + * prepare it before `docker compose up`? + * + * Question 2 is subtle on split-filesystem runners (`--docker-host-path-prefix`). + * Custom volume mounts are materialised with daemon-side sources, so a runner + * local `fs` call against them is meaningless. Everything else is still a + * runner-local path at this stage because the prefix is applied last. + */ + +export interface ParsedMount { + source: string; + target: string; + mode: string; +} + +export function parseMount(spec: string): ParsedMount | undefined { + const parts = spec.split(':'); + if (parts.length < 2 || !parts[0] || !parts[1]) return undefined; + return { + source: parts[0], + target: parts[1].replace(/\/+$/, '') || '/', + mode: parts[2] || 'rw', + }; +} + +function isPathPrefix(parent: string, child: string): boolean { + return parent !== child && child.startsWith(parent === '/' ? '/' : `${parent}/`); +} + +/** + * Returns the deepest bind whose target strictly contains `target`. runc applies + * binds parent-first, so this is the mount that owns the directory entry runc + * must create for `target`. + */ +export function innermostCoveringMount(binds: ParsedMount[], target: string): ParsedMount | undefined { + let best: ParsedMount | undefined; + for (const bind of binds) { + if (!isPathPrefix(bind.target, target)) continue; + if (!best || bind.target.length > best.target.length) best = bind; + } + return best; +} + +/** + * Resolves a generated bind source to the equivalent path on the runner's own + * filesystem, or `undefined` when that cannot be known. + * + * `undefined` means "do not touch": callers must fail closed (keep a credential + * overlay, skip a mountpoint preparation) rather than act on a path that may + * belong to the Docker daemon's filesystem instead of the runner's. + */ +export type LocalSourceResolver = (source: string) => string | undefined; + +export function createLocalSourceResolver( + customSourceRoots: Map, + dockerHostPathPrefix?: string, +): LocalSourceResolver { + return (source: string): string | undefined => { + for (const [daemonRoot, localRoot] of customSourceRoots) { + if (source !== daemonRoot && !isPathPrefix(daemonRoot, source)) continue; + // A custom mount we cannot map back to a runner path: fail closed. + if (!localRoot) return undefined; + return `${localRoot}${source.slice(daemonRoot.length)}`; + } + + // Not a custom mount. Every other source is still runner-local here because + // `applyHostPathPrefixToVolumes` runs after this stage. If a source already + // carries the daemon prefix we cannot attribute it, so fail closed. + if (dockerHostPathPrefix && isPathPrefix(dockerHostPathPrefix, source)) return undefined; + + return source; + }; +} + +/** Default resolver for single-filesystem runs: sources are runner-local. */ +export const identityLocalSourceResolver: LocalSourceResolver = (source) => source; diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts new file mode 100644 index 000000000..7b37ba206 --- /dev/null +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -0,0 +1,301 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { + ensureNestedMountpoints, + planNestedMountpoints, +} from './nested-mountpoints'; +import { createLocalSourceResolver } from './mount-topology'; +import { HOME_TOOL_PATHS } from '../../config/mount-policy'; +import { buildAgentVolumes } from './volume-builder'; +import { WrapperConfig } from '../../types'; + +jest.mock('../../host-env', () => ({ + ...jest.requireActual('../../host-env'), + // Real runs are root under sudo; the test process is not, so keep chown a no-op + // by targeting the identity the test already owns. + getSafeHostUid: () => String(process.getuid?.() ?? 0), + getSafeHostGid: () => String(process.getgid?.() ?? 0), +})); + +/** + * Models how runc materialises a compose bind list: parents first, and every + * mountpoint has to already exist inside whichever bind currently covers it. + * Returns the mounts runc would fail on. + */ +function simulateRuncMountFailures(volumes: string[]): string[] { + const parsed = volumes + .map((spec) => { + const [source, target, mode] = spec.split(':'); + return { spec, source, target, mode: mode || 'rw' }; + }) + .filter((mount) => Boolean(mount.source && mount.target)); + + const ordered = [...parsed].sort( + (a, b) => a.target.split('/').length - b.target.split('/').length, + ); + + const failures: string[] = []; + const established: typeof ordered = []; + + for (const mount of ordered) { + const cover = established + .filter((candidate) => mount.target.startsWith(`${candidate.target}/`)) + .sort((a, b) => b.target.length - a.target.length)[0]; + + if (cover) { + const hostPath = `${cover.source}${mount.target.slice(cover.target.length)}`; + // runc creates a missing mountpoint with mkdirat; that fails on a + // read-only cover. + if (!fs.existsSync(hostPath) && cover.mode === 'ro') failures.push(mount.spec); + } + + established.push(mount); + } + + return failures; +} + +describe('nested mountpoint preparation', () => { + let tmpRoot: string; + + beforeEach(() => { + tmpRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'awf-nested-'))); + }); + + afterEach(() => { + fs.rmSync(tmpRoot, { recursive: true, force: true }); + }); + + describe('planNestedMountpoints', () => { + it('reports nothing when every covering bind is writable', () => { + const volumes = [ + '/host/home/runner:/host/home/runner:rw', + '/logs:/host/home/runner/.copilot/logs:rw', + ]; + + expect(planNestedMountpoints(volumes)).toEqual([]); + }); + + it('reports the mountpoint a read-only cover cannot create', () => { + const volumes = [ + '/empty-home:/host/home/runner:ro', + '/logs:/host/home/runner/.copilot/logs:rw', + ]; + + expect(planNestedMountpoints(volumes)).toEqual([ + expect.objectContaining({ + containerTarget: '/host/home/runner/.copilot/logs', + coveringTarget: '/host/home/runner', + coveringSource: '/empty-home', + hostPath: '/empty-home/.copilot/logs', + kind: 'directory', + }), + ]); + }); + + it('attributes the mountpoint to the innermost cover, not the outermost', () => { + const volumes = [ + '/empty-home:/host/home/runner:ro', + '/real-copilot:/host/home/runner/.copilot:ro', + '/logs:/host/home/runner/.copilot/logs:rw', + ]; + + const requirement = planNestedMountpoints(volumes).find( + (candidate) => candidate.containerTarget === '/host/home/runner/.copilot/logs', + ); + + expect(requirement?.hostPath).toBe('/real-copilot/logs'); + }); + + it('classifies /dev/null credential overlays as files, not directories', () => { + const volumes = [ + '/empty-home:/host/home/runner:ro', + '/dev/null:/host/home/runner/.netrc:ro', + ]; + + expect(planNestedMountpoints(volumes)).toEqual([ + expect.objectContaining({ containerTarget: '/host/home/runner/.netrc', kind: 'file' }), + ]); + }); + + it('reports no host path when the cover is a daemon-side custom mount', () => { + const resolver = createLocalSourceResolver(new Map(), '/daemon'); + const volumes = [ + '/daemon/mnt:/host/home/runner:ro', + '/logs:/host/home/runner/.copilot/logs:rw', + ]; + + expect(planNestedMountpoints(volumes, resolver)[0].hostPath).toBeUndefined(); + }); + }); + + describe('ensureNestedMountpoints', () => { + const uid = process.getuid?.() ?? 0; + const gid = process.getgid?.() ?? 0; + + function makeTree() { + const emptyHome = path.join(tmpRoot, 'chroot-home'); + const logs = path.join(tmpRoot, 'agent-logs'); + const sessionState = path.join(tmpRoot, 'agent-session-state'); + [emptyHome, logs, sessionState].forEach((dir) => fs.mkdirSync(dir, { recursive: true })); + return { emptyHome, logs, sessionState }; + } + + it('creates the nested .copilot mountpoints runc could not create itself', () => { + const { emptyHome, logs, sessionState } = makeTree(); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `${logs}:/host/home/runner/.copilot/logs:rw`, + `${sessionState}:/host/home/runner/.copilot/session-state:rw`, + ]; + + expect(simulateRuncMountFailures(volumes)).toHaveLength(2); + + const created = ensureNestedMountpoints(volumes, uid, gid); + + expect(created).toEqual([ + path.join(emptyHome, '.copilot/logs'), + path.join(emptyHome, '.copilot/session-state'), + ]); + expect(simulateRuncMountFailures(volumes)).toEqual([]); + }); + + it('prepares mountpoints inside a real ~/.copilot cover as well as an empty home', () => { + const { logs } = makeTree(); + const realCopilot = path.join(tmpRoot, 'home', '.copilot'); + fs.mkdirSync(realCopilot, { recursive: true }); + + const volumes = [ + `${path.join(tmpRoot, 'home')}:/host/home/runner:ro`, + `${realCopilot}:/host/home/runner/.copilot:ro`, + `${logs}:/host/home/runner/.copilot/logs:rw`, + ]; + + ensureNestedMountpoints(volumes, uid, gid); + + expect(fs.existsSync(path.join(realCopilot, 'logs'))).toBe(true); + expect(simulateRuncMountFailures(volumes)).toEqual([]); + }); + + it('creates nothing when no write policy narrowed a cover to read-only', () => { + const { emptyHome, logs } = makeTree(); + const volumes = [ + `${emptyHome}:/host/home/runner:rw`, + `${logs}:/host/home/runner/.copilot/logs:rw`, + ]; + + expect(ensureNestedMountpoints(volumes, uid, gid)).toEqual([]); + expect(fs.existsSync(path.join(emptyHome, '.copilot'))).toBe(false); + }); + + it('never fabricates a credential file for a /dev/null overlay', () => { + const { emptyHome } = makeTree(); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + '/dev/null:/host/home/runner/.netrc:ro', + ]; + + expect(ensureNestedMountpoints(volumes, uid, gid)).toEqual([]); + expect(fs.existsSync(path.join(emptyHome, '.netrc'))).toBe(false); + }); + + it('skips mounts whose source does not exist on this filesystem', () => { + const { emptyHome } = makeTree(); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `${path.join(tmpRoot, 'missing-source')}:/host/home/runner/.copilot/logs:rw`, + ]; + + expect(ensureNestedMountpoints(volumes, uid, gid)).toEqual([]); + }); + + it('skips daemon-side covers instead of creating a runner-local tree', () => { + const { emptyHome, logs } = makeTree(); + const resolver = createLocalSourceResolver(new Map(), tmpRoot); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `${logs}:/host/home/runner/.copilot/logs:rw`, + ]; + + expect(ensureNestedMountpoints(volumes, uid, gid, resolver)).toEqual([]); + expect(fs.existsSync(path.join(emptyHome, '.copilot'))).toBe(false); + }); + }); + + describe('buildAgentVolumes (end-to-end topology)', () => { + function stageRunnerLayout() { + const home = path.join(tmpRoot, 'home', 'runner'); + const workspaceDir = path.join(home, 'work', 'repo', 'repo'); + const workDir = path.join(tmpRoot, 'awf-run'); + const emptyHome = `${workDir}-chroot-home`; + const agentLogsPath = path.join(workDir, 'agent-logs'); + const sessionStatePath = path.join(workDir, 'agent-session-state'); + const initSignalDir = path.join(workDir, 'init-signal'); + + [workspaceDir, workDir, emptyHome, agentLogsPath, sessionStatePath, initSignalDir] + .forEach((dir) => fs.mkdirSync(dir, { recursive: true })); + // Mirror prepareChrootHomeMounts: host tool dirs plus their chroot placeholders. + for (const toolPath of HOME_TOOL_PATHS) { + fs.mkdirSync(path.join(home, toolPath), { recursive: true }); + fs.mkdirSync(path.join(emptyHome, toolPath), { recursive: true }); + } + fs.mkdirSync(path.join(emptyHome, 'work', 'repo', 'repo'), { recursive: true }); + + const config = { + agentCommand: 'true', + allowedDomains: [], + workDir, + volumeMounts: [], + } as unknown as WrapperConfig; + + return { + home, + workspaceDir, + emptyHome, + build: (filesystemAllowWrite?: string[]) => buildAgentVolumes({ + config: { ...config, filesystemAllowWrite } as WrapperConfig, + projectRoot: process.cwd(), + effectiveHome: home, + workspaceDir, + agentLogsPath, + sessionStatePath, + initSignalDir, + }), + }; + } + + it('leaves every nested mountpoint satisfiable when a write policy narrows the home tree', () => { + const layout = stageRunnerLayout(); + const writable = path.join(layout.workspaceDir, 'allowed'); + fs.mkdirSync(writable, { recursive: true }); + + const volumes = layout.build([writable]); + + expect(volumes.some((spec) => spec.includes('/.copilot/logs:rw'))).toBe(true); + expect(simulateRuncMountFailures(volumes)).toEqual([]); + }); + + it('creates the .copilot log and session-state mountpoints runc cannot', () => { + const layout = stageRunnerLayout(); + const writable = path.join(layout.workspaceDir, 'allowed'); + fs.mkdirSync(writable, { recursive: true }); + + layout.build([writable]); + + // The real ~/.copilot bind is the innermost cover for both nested mounts. + expect(fs.existsSync(path.join(layout.home, '.copilot/logs'))).toBe(true); + expect(fs.existsSync(path.join(layout.home, '.copilot/session-state'))).toBe(true); + }); + + it('changes nothing on disk and needs no preparation without a policy', () => { + const layout = stageRunnerLayout(); + + const volumes = layout.build(undefined); + + expect(planNestedMountpoints(volumes).filter((r) => r.kind === 'directory')).toEqual([]); + expect(fs.existsSync(path.join(layout.home, '.copilot/logs'))).toBe(false); + expect(simulateRuncMountFailures(volumes)).toEqual([]); + }); + }); +}); diff --git a/src/services/agent-volumes/nested-mountpoints.ts b/src/services/agent-volumes/nested-mountpoints.ts new file mode 100644 index 000000000..aa62cd5d3 --- /dev/null +++ b/src/services/agent-volumes/nested-mountpoints.ts @@ -0,0 +1,157 @@ +import * as fs from 'fs'; +import { logger } from '../../logger'; +import { createMissingOwnedDirectorySegments } from '../../fs-utils'; +import { + LocalSourceResolver, + ParsedMount, + identityLocalSourceResolver, + innermostCoveringMount, + parseMount, +} from './mount-topology'; + +export interface NestedMountpointRequirement { + /** Container path whose mountpoint runc must create. */ + containerTarget: string; + /** Source of the mount that needs the mountpoint. */ + source: string; + /** Target of the read-only bind that owns the directory entry. */ + coveringTarget: string; + /** Source of that covering bind, as written into the compose file. */ + coveringSource: string; + /** Path on the runner that must exist so runc does not have to create it. */ + hostPath?: string; + /** `/dev/null` credential overlays need a file, every other bind a directory. */ + kind: 'directory' | 'file'; +} + +/** + * Derives every mountpoint runc would have to create inside a read-only bind. + * + * runc applies binds parent-first and creates a missing mountpoint with + * `mkdirat` against the *destination*, which resolves into whichever bind + * already covers that path. While every covering bind is read-write this always + * succeeds, which is why the nesting went unnoticed. As soon as + * `filesystem.allowWrite` narrows a covering bind to read-only the same + * `mkdirat` returns EROFS and container init dies before the agent runs. + * + * Deriving the list from the final, policy-applied volumes (rather than from a + * hand-maintained list of known nested paths) means any internal mount added + * later is covered automatically. + * + * Pure: this only reports requirements, it does not touch the filesystem. + */ +export function planNestedMountpoints( + volumes: string[], + localSourceResolver: LocalSourceResolver = identityLocalSourceResolver, +): NestedMountpointRequirement[] { + const mounts = volumes + .map(parseMount) + .filter((mount): mount is ParsedMount => mount !== undefined); + const binds = mounts.filter((mount) => mount.source !== '/dev/null'); + + const requirements: NestedMountpointRequirement[] = []; + for (const mount of mounts) { + const cover = innermostCoveringMount(binds, mount.target); + // No covering bind: the mountpoint lives on the container's own writable + // rootfs and runc can always create it. + if (!cover) continue; + // A read-write cover can still create its own mountpoints. + if (cover.mode !== 'ro') continue; + if (!cover.source.startsWith('/')) continue; + + const suffix = mount.target.slice(cover.target.length); + if (!suffix) continue; + + const localCoverSource = localSourceResolver(cover.source); + requirements.push({ + containerTarget: mount.target, + source: mount.source, + coveringTarget: cover.target, + coveringSource: cover.source, + hostPath: localCoverSource === undefined ? undefined : `${localCoverSource}${suffix}`, + kind: mount.source === '/dev/null' ? 'file' : 'directory', + }); + } + + return requirements; +} + +function isExistingDirectory(candidate: string): boolean { + try { + return fs.statSync(candidate).isDirectory(); + } catch { + return false; + } +} + +/** + * Creates the directory mountpoints reported by {@link planNestedMountpoints}. + * + * Only directories are prepared. `/dev/null` credential overlays intentionally + * are not: fabricating a credential file just to mount over it would create the + * very path we are hiding. Those are handled by + * {@link ../agent-volumes/credential-hiding.pruneUnmountableCredentialOverlays} + * instead, which drops overlays that are unreachable anyway. + * + * A requirement is skipped when the covering bind's source is not a real + * directory on this filesystem — that means either a fabricated path (unit + * tests) or a daemon-side path we must not touch. Any other failure propagates: + * an unpreparable mountpoint is a launch failure, and failing here is much + * better than an opaque EROFS from container init. + */ +export function ensureNestedMountpoints( + volumes: string[], + uid: number, + gid: number, + localSourceResolver: LocalSourceResolver = identityLocalSourceResolver, +): string[] { + const created: string[] = []; + const requirements = planNestedMountpoints(volumes, localSourceResolver); + const actionable: NestedMountpointRequirement[] = []; + + for (const requirement of requirements) { + if (requirement.kind !== 'directory') continue; + + const { hostPath } = requirement; + if (hostPath === undefined) { + logger.debug( + `Skipping mountpoint preparation for ${requirement.containerTarget}: the covering bind ` + + `source ${requirement.coveringSource} is not resolvable on this filesystem`, + ); + continue; + } + + const localCoverSource = localSourceResolver(requirement.coveringSource); + if (localCoverSource === undefined || !isExistingDirectory(localCoverSource)) continue; + // Only prepare mountpoints for binds that really exist on this filesystem. + // A source that is absent is either a fabricated path or a daemon-side one, + // and in both cases we must not materialise a tree for it. + const localMountSource = localSourceResolver(requirement.source); + if (localMountSource === undefined || !isExistingDirectory(localMountSource)) continue; + + actionable.push(requirement); + if (fs.existsSync(hostPath)) continue; + + createMissingOwnedDirectorySegments(hostPath, uid, gid); + created.push(hostPath); + logger.debug( + `Prepared nested mountpoint ${hostPath} for ${requirement.containerTarget} ` + + `(inside read-only bind ${requirement.coveringTarget})`, + ); + } + + // Fail closed: a mountpoint that is still missing would surface as an opaque + // EROFS from container init, long after the useful context is gone. + const unmet = actionable.filter((requirement) => !fs.existsSync(requirement.hostPath as string)); + if (unmet.length > 0) { + const details = unmet + .map((requirement) => `${requirement.containerTarget} (needs ${requirement.hostPath})`) + .join(', '); + throw new Error( + `Could not prepare bind mountpoints nested inside a read-only mount: ${details}. ` + + 'The agent container would fail to start with a read-only filesystem error.', + ); + } + + return created; +} diff --git a/src/services/agent-volumes/volume-builder.ts b/src/services/agent-volumes/volume-builder.ts index b3230388f..13bb945f9 100644 --- a/src/services/agent-volumes/volume-builder.ts +++ b/src/services/agent-volumes/volume-builder.ts @@ -1,8 +1,11 @@ import { SslConfig } from '../../host-env'; +import { getSafeHostGid, getSafeHostUid } from '../../host-env'; import { logger } from '../../logger'; import { WrapperConfig } from '../../types'; import { applyHostPathPrefixToVolumes } from '../host-path-prefix'; import { buildCredentialHidingOverlays, pruneUnmountableCredentialOverlays } from './credential-hiding'; +import { createLocalSourceResolver } from './mount-topology'; +import { ensureNestedMountpoints } from './nested-mountpoints'; import { buildDockerSocketMount } from './docker-socket'; import { buildEtcMounts } from './etc-mounts'; import { buildHomeMounts } from './home-strategy'; @@ -80,12 +83,32 @@ export function buildAgentVolumes(params: AgentVolumesParams): string[] { const localSourceRoots = new Map( customMounts.map((spec, index) => [spec, localCustomMounts[index]?.split(':')[0] ?? '']), ); + // Same pairing, keyed by source root, so topology passes can map a daemon-side + // custom-mount source back to the runner path (or refuse to, and fail closed). + const customSourceRoots = new Map( + customMounts.map((spec, index) => [ + spec.split(':')[0] ?? '', + localCustomMounts[index]?.split(':')[0] ?? '', + ]), + ); + const localSourceResolver = createLocalSourceResolver(customSourceRoots, config.dockerHostPathPrefix); + const policyVolumes = pruneUnmountableCredentialOverlays(applyFilesystemWritePolicy( agentVolumes, resolveComposeFilesystemAllowWrite(config), alwaysWritableMounts, localSourceRoots, - )); + ), localSourceResolver); + + // A read-only bind cannot host a mountpoint runc still has to create, so any + // internal mount nested inside one must exist up front. Derived from the final + // topology, so mounts added later are covered without another targeted fix. + ensureNestedMountpoints( + policyVolumes, + parseInt(getSafeHostUid(), 10), + parseInt(getSafeHostGid(), 10), + localSourceResolver, + ); if (config.dockerHostPathPrefix) { return applyHostPathPrefixToVolumes(policyVolumes, config.dockerHostPathPrefix); diff --git a/src/services/agent-volumes/workspace-mounts.ts b/src/services/agent-volumes/workspace-mounts.ts index b8047daf6..336418824 100644 --- a/src/services/agent-volumes/workspace-mounts.ts +++ b/src/services/agent-volumes/workspace-mounts.ts @@ -2,7 +2,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { logger } from '../../logger'; import { WrapperConfig } from '../../types'; -import { INIT_SIGNAL_DIR } from '../../constants'; +import { INIT_SIGNAL_DIR, LEGACY_INIT_SIGNAL_DIR } from '../../constants'; import { applyHostPathPrefixToVolumes } from '../host-path-prefix'; import { extractCommandBinaryName, @@ -29,6 +29,12 @@ export function buildWorkspaceMounts(params: WorkspaceMountsParams): string[] { `${agentLogsPath}:${effectiveHome}/.copilot/logs:rw`, `${sessionStatePath}:${effectiveHome}/.copilot/session-state:rw`, `${initSignalDir}:${INIT_SIGNAL_DIR}:rw`, + // Agent images released before the signal directory moved to /run wait on + // the legacy path instead. Exposing the same source there keeps a newer CLI + // working with an older pinned `--image-tag`. Read-only on purpose: the + // agent only polls for `ready`, and the init container writes through its + // own read-write mount at INIT_SIGNAL_DIR. + `${initSignalDir}:${LEGACY_INIT_SIGNAL_DIR}:ro`, ]; if (config.enableApiProxy) { diff --git a/src/services/init-signal-compatibility.test.ts b/src/services/init-signal-compatibility.test.ts new file mode 100644 index 000000000..4c5600df6 --- /dev/null +++ b/src/services/init-signal-compatibility.test.ts @@ -0,0 +1,82 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import { generateDockerCompose, mockNetworkConfig, useAgentVolumesTestConfig } from './service-test-setup.test-utils'; +import { INIT_SIGNAL_DIR, LEGACY_INIT_SIGNAL_DIR } from '../constants'; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +jest.mock('execa', () => require('../test-helpers/mock-execa.test-utils').execaMockFactory()); + +const { getConfig } = useAgentVolumesTestConfig(); + +const entrypointSource = fs.readFileSync( + path.resolve(__dirname, '../../containers/agent/entrypoint.sh'), + 'utf8', +); + +/** + * The init-signal handshake spans two independently versioned artefacts: the + * CLI, which decides where the ready-file is mounted, and the agent image, + * which decides where it waits. These tests pin both directions of that + * contract so a move like `/tmp/awf-init` -> `/run/awf-init` cannot silently + * strand a pinned `--image-tag`. + */ +describe('init signal directory compatibility', () => { + describe('new CLI + new agent image', () => { + it('mounts the ready-file source at the current path, writable', () => { + const volumes = generateDockerCompose(getConfig(), mockNetworkConfig) + .services.agent.volumes as string[]; + + expect(volumes).toContain(`${getConfig().workDir}/init-signal:${INIT_SIGNAL_DIR}:rw`); + }); + + it('waits on the current path', () => { + expect(entrypointSource).toContain('INIT_SIGNAL_DIR="${AWF_INIT_SIGNAL_DIR:-/run/awf-init}"'); + }); + }); + + describe('new CLI + old agent image', () => { + it('also exposes the same source at the legacy path an older image polls', () => { + const volumes = generateDockerCompose(getConfig(), mockNetworkConfig) + .services.agent.volumes as string[]; + + expect(volumes).toContain(`${getConfig().workDir}/init-signal:${LEGACY_INIT_SIGNAL_DIR}:ro`); + }); + + it('keeps the legacy view read-only, since older images only poll it', () => { + const volumes = generateDockerCompose(getConfig(), mockNetworkConfig) + .services.agent.volumes as string[]; + + expect(volumes).not.toContain(`${getConfig().workDir}/init-signal:${LEGACY_INIT_SIGNAL_DIR}:rw`); + }); + + it('keeps the legacy path exposed when a write policy narrows /tmp', () => { + const workspaceDir = process.env.GITHUB_WORKSPACE || process.cwd(); + const volumes = generateDockerCompose( + { ...getConfig(), filesystemAllowWrite: [`${workspaceDir}/src`] }, + mockNetworkConfig, + ).services.agent.volumes as string[]; + + expect(volumes).toContain('/tmp:/tmp:ro'); + expect(volumes).toContain(`${getConfig().workDir}/init-signal:${LEGACY_INIT_SIGNAL_DIR}:ro`); + }); + }); + + describe('old CLI + new agent image', () => { + it('still accepts a ready-file delivered at the legacy path', () => { + expect(entrypointSource).toContain('LEGACY_INIT_SIGNAL_DIR="/tmp/awf-init"'); + expect(entrypointSource).toContain( + 'while [ ! -f "${INIT_SIGNAL_DIR}/ready" ] && [ ! -f "${LEGACY_INIT_SIGNAL_DIR}/ready" ]; do', + ); + }); + }); + + it('does not rely on a symlink inside the init container, which the agent cannot see', () => { + const command = generateDockerCompose(getConfig(), mockNetworkConfig) + .services['iptables-init'].command as string[]; + + // The init container has its own rootfs and mount namespace: anything it + // creates at the legacy path is invisible to the agent container. + expect(command.join(' ')).not.toContain('ln -s'); + expect(command.join(' ')).not.toContain(LEGACY_INIT_SIGNAL_DIR); + }); +}); diff --git a/tests/integration/filesystem-allowwrite.test.ts b/tests/integration/filesystem-allowwrite.test.ts index b9edc287c..a14d55f93 100644 --- a/tests/integration/filesystem-allowwrite.test.ts +++ b/tests/integration/filesystem-allowwrite.test.ts @@ -64,4 +64,60 @@ describe('filesystem.allowWrite', () => { expect(result).toSucceed(); expect(fs.readFileSync(path.join(writableDir, 'started.txt'), 'utf8')).toContain('started'); }, 180000); + + test('keeps security helpers installed when /tmp is narrowed read-only', async () => { + const helperDir = fs.mkdtempSync(path.join(os.tmpdir(), 'awf-allowwrite-helpers-')); + const writableDir = path.join(helperDir, 'agent'); + fs.mkdirSync(writableDir, { recursive: true }); + const configPath = path.join(helperDir, 'awf-config.json'); + fs.writeFileSync(configPath, JSON.stringify({ + network: { allowDomains: ['github.com'] }, + filesystem: { allowWrite: [writableDir] }, + container: { buildLocal: true }, + logging: { logLevel: 'debug' }, + security: { legacySecurity: true }, + })); + + // The helpers used to stage under /tmp/awf-lib. Once a write policy narrows + // /tmp to read-only those copies failed silently, disabling one-shot token + // protection. Prove the library is really present at its new location, not + // merely that the container started. + const result = await runner.runWithSudo( + "sh -c 'ls -l /run/awf-lib/ > " + writableDir + "/helpers.txt 2>&1; " + + "test -s /run/awf-lib/one-shot-token.so && echo ONE_SHOT_TOKEN_PRESENT >> " + writableDir + "/helpers.txt'", + { configFile: configPath, timeout: 120000 }, + ); + + expect(result).toSucceed(); + const helpers = fs.readFileSync(path.join(writableDir, 'helpers.txt'), 'utf8'); + expect(helpers).toContain('ONE_SHOT_TOKEN_PRESENT'); + + fs.rmSync(helperDir, { recursive: true, force: true }); + }, 180000); + + test('leaves no AWF mount residue on the host after teardown', async () => { + const residueDir = fs.mkdtempSync(path.join(os.tmpdir(), 'awf-allowwrite-residue-')); + const writableDir = path.join(residueDir, 'agent'); + fs.mkdirSync(writableDir, { recursive: true }); + const configPath = path.join(residueDir, 'awf-config.json'); + fs.writeFileSync(configPath, JSON.stringify({ + network: { allowDomains: ['github.com'] }, + filesystem: { allowWrite: [writableDir] }, + container: { buildLocal: true }, + logging: { logLevel: 'debug' }, + security: { legacySecurity: true }, + })); + + const result = await runner.runWithSudo( + `sh -c 'echo done > ${writableDir}/done.txt'`, + { configFile: configPath, timeout: 120000 }, + ); + expect(result).toSucceed(); + + const mounts = fs.readFileSync('/proc/mounts', 'utf8'); + expect(mounts).not.toContain(writableDir); + expect(mounts).not.toContain('/run/awf-lib'); + + fs.rmSync(residueDir, { recursive: true, force: true }); + }, 180000); }); From eac540fd07d3091009e28163fd8c99996ab45002 Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 20:29:57 -0700 Subject: [PATCH 10/18] test: assert no preparation, not zero nested mountpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The no-policy case asserted that a policy-free build reports no nested mountpoint requirements at all. That held on macOS but not on a GitHub-hosted runner, where /opt is bound read-only and the tool cache is nested inside it, so a requirement exists with no policy in play. The requirement is real and has always been satisfied: its mountpoint resolves to the mount's own source, so it exists whenever the mount does. Assert what actually matters instead — no requirement is covered by an AWF-owned bind, so nothing in the agent's home tree is created, and runc has nothing left to create anywhere. Pin the system-bind shape directly so this cannot regress based on which machine runs the suite. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/nested-mountpoints.test.ts | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index 7b37ba206..b74c9bfaf 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -128,6 +128,23 @@ describe('nested mountpoint preparation', () => { expect(planNestedMountpoints(volumes, resolver)[0].hostPath).toBeUndefined(); }); + + // GitHub-hosted runners bind `/opt` read-only and nest the tool cache + // inside it, so a requirement exists even with no write policy. It is + // already satisfied — source and mountpoint are the same host path — which + // is why this has always worked and must keep costing nothing. + it('reports already-satisfied system binds without asking for preparation', () => { + const volumes = [ + '/opt:/host/opt:ro', + '/opt/hostedtoolcache:/host/opt/hostedtoolcache:rw', + ]; + + const [requirement] = planNestedMountpoints(volumes); + expect(requirement.containerTarget).toBe('/host/opt/hostedtoolcache'); + // The mountpoint resolves to the mount's own source, so it exists exactly + // when the mount does and never needs preparing. + expect(requirement.hostPath).toBe(requirement.source); + }); }); describe('ensureNestedMountpoints', () => { @@ -293,8 +310,18 @@ describe('nested mountpoint preparation', () => { const volumes = layout.build(undefined); - expect(planNestedMountpoints(volumes).filter((r) => r.kind === 'directory')).toEqual([]); + // `ensureNestedMountpoints` only ever creates a requirement's `hostPath`, + // and that is always `coveringSource` + suffix. No requirement is covered + // by an AWF-owned bind, so nothing in the staged tree can have been + // created... + const staged = planNestedMountpoints(volumes) + .filter((requirement) => requirement.coveringSource.startsWith(tmpRoot)); + expect(staged).toEqual([]); expect(fs.existsSync(path.join(layout.home, '.copilot/logs'))).toBe(false); + expect(fs.existsSync(path.join(layout.home, '.copilot/session-state'))).toBe(false); + // ...and every remaining requirement is a system bind AWF has always + // emitted (on a GitHub-hosted runner `/opt` covers `/opt/hostedtoolcache`) + // whose mountpoint already exists, so nothing was created there either. expect(simulateRuncMountFailures(volumes)).toEqual([]); }); }); From 94c4a32bf79d4e906c0a51d9bb9fd18f0132230c Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 21:05:16 -0700 Subject: [PATCH 11/18] fix: give the init container the legacy signal path and file mountpoints Two remaining startup failures under filesystem.allowWrite. 1. New CLI + old pinned agent image still hung. The init container only received the signal directory at /run/awf-init, but the old setup-iptables.sh runs under `set -e` and hardcodes its audit output to /tmp/awf-init/iptables-audit.txt. That redirect failed, so the script exited 1 before the CLI's chained `touch $AWF_INIT_SIGNAL_DIR/ready`, and the agent timed out waiting for a signal that never arrived. The agent-side legacy bind could not help: the init container has its own mount namespace. Bind the same signal source a second time at /tmp/awf-init in the init container, read-write because the old script writes there. The agent-side legacy bind stays read-only, which matches the old entrypoint that only polls ready and output.log. 2. Nested mountpoint preparation silently skipped regular-file binds under a read-only cover. Every requirement except /dev/null was assumed to be a directory, and any source that did not resolve to a local directory was dropped instead of failing closed. With --docker-host-path-prefix, stageHostFile publishes /awf-docker-host-stage/bin/ to /tmp/awf-runner-bin/:ro while a policy narrows /tmp to read-only, so runc needed a file mountpoint that nothing created. The local source resolver cannot attribute staged paths because they live under the prefix, so record staging in docker-host-staging and derive the kind from that first, then from a stat of the resolved source. Create file mountpoints with an exclusive open, and refuse to launch when a required mountpoint cannot be classified rather than dropping it. Both mountpoint kinds matter: against a real daemon a missing mountpoint fails with "read-only file system" and a directory standing in for a file fails with "not a directory". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- src/services/agent-service.ts | 23 ++- .../agent-volumes/docker-host-staging.ts | 21 ++ .../agent-volumes/nested-mountpoints.test.ts | 193 +++++++++++++++++- .../agent-volumes/nested-mountpoints.ts | 163 +++++++++++---- .../init-signal-compatibility.test.ts | 106 ++++++++++ 5 files changed, 453 insertions(+), 53 deletions(-) diff --git a/src/services/agent-service.ts b/src/services/agent-service.ts index d72c8064a..f7f0cbbe7 100644 --- a/src/services/agent-service.ts +++ b/src/services/agent-service.ts @@ -3,6 +3,7 @@ import { AGENT_CONTAINER_NAME, INIT_SIGNAL_DIR, IPTABLES_INIT_CONTAINER_NAME, + LEGACY_INIT_SIGNAL_DIR, SQUID_PORT, } from '../constants'; import { ACT_PRESET_BASE_IMAGE, getSafeHostUid, getSafeHostGid } from '../host-identity'; @@ -293,11 +294,6 @@ interface IptablesInitServiceParams { */ export function buildIptablesInitService(params: IptablesInitServiceParams): any { const { agentService, environment, networkConfig, initSignalDir, dockerHostPathPrefix, hostGatewayIp } = params; - // No legacy-path compatibility shim here: this container has its own mount - // namespace and rootfs, so anything created at LEGACY_INIT_SIGNAL_DIR inside - // it is invisible to the agent container. Older agent images are supported by - // binding the same host source at the legacy path in the *agent* service - // instead (see buildWorkspaceMounts). const setupCommand = [ 'mkdir -p "$$AWF_INIT_SIGNAL_DIR"', '/usr/local/bin/setup-iptables.sh > "$$AWF_INIT_SIGNAL_DIR/output.log" 2>&1', @@ -308,8 +304,20 @@ export function buildIptablesInitService(params: IptablesInitServiceParams): any // otherwise the two containers bind to different daemon-side directories and the // ready-file handshake fails. buildAgentVolumes() applies dockerHostPathPrefix to its // mounts, so do the same here via the shared helper. - const [initSignalMount] = applyHostPathPrefixToVolumes( - [`${initSignalDir}:${INIT_SIGNAL_DIR}:rw`], + // + // The legacy path is bound here as well, and read-write. This container has its + // own mount namespace, so the agent-side legacy bind is invisible to it and + // cannot help. It matters because setup-iptables.sh in agent images released + // before the signal directory moved to /run writes its audit dump to a + // hardcoded LEGACY_INIT_SIGNAL_DIR path, under `set -e`: with the directory + // absent the redirection fails, the script exits non-zero before the `touch` + // above, and the agent waits out its full ready timeout and then fails. Both + // paths are the same host directory, so a new image simply ignores this one. + const [initSignalMount, legacyInitSignalMount] = applyHostPathPrefixToVolumes( + [ + `${initSignalDir}:${INIT_SIGNAL_DIR}:rw`, + `${initSignalDir}:${LEGACY_INIT_SIGNAL_DIR}:rw`, + ], dockerHostPathPrefix, ); @@ -323,6 +331,7 @@ export function buildIptablesInitService(params: IptablesInitServiceParams): any // Only mount the init signal volume and the iptables setup script volumes: [ initSignalMount, + legacyInitSignalMount, ], environment: { // Pass through environment variables needed by setup-iptables.sh diff --git a/src/services/agent-volumes/docker-host-staging.ts b/src/services/agent-volumes/docker-host-staging.ts index 139302d68..1f56c60d0 100644 --- a/src/services/agent-volumes/docker-host-staging.ts +++ b/src/services/agent-volumes/docker-host-staging.ts @@ -6,6 +6,26 @@ import { WrapperConfig } from '../../types'; const DOCKER_HOST_STAGE_DIR = 'awf-docker-host-stage'; const SAFE_BINARY_NAME_REGEX = /^[a-zA-Z0-9_][a-zA-Z0-9_.-]*$/; +/** + * Regular files AWF copied into the daemon staging root during this run. + * + * Staged sources live *under* `--docker-host-path-prefix`, so a topology pass + * cannot attribute them back to a runner path and must otherwise treat them as + * daemon-side. Recording them here preserves the one fact that would be lost: + * the source is a regular file on this filesystem, so a bind of it needs a + * *file* mountpoint rather than a directory. + */ +const stagedHostFiles = new Set(); + +export function isStagedHostFile(candidate: string): boolean { + return stagedHostFiles.has(candidate); +} + +/** Test seam: staging is process-global, so suites must be able to reset it. */ +export function clearStagedHostFiles(): void { + stagedHostFiles.clear(); +} + function normalizeDockerHostPathPrefix(prefix: string): string { const trimmed = prefix.trim(); if (!trimmed) return ''; @@ -53,6 +73,7 @@ export function stageHostFile(config: WrapperConfig, sourcePath: string, relativ fs.mkdirSync(path.dirname(targetPath), { recursive: true }); fs.copyFileSync(sourcePath, targetPath); fs.chmodSync(targetPath, mode); + stagedHostFiles.add(targetPath); return targetPath; } catch (err) { logger.debug(`Could not stage ${sourcePath} for docker-host-path-prefix: ${err}`); diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index b74c9bfaf..9d0efcfde 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -6,6 +6,7 @@ import { planNestedMountpoints, } from './nested-mountpoints'; import { createLocalSourceResolver } from './mount-topology'; +import { pruneUnmountableCredentialOverlays } from './credential-hiding'; import { HOME_TOOL_PATHS } from '../../config/mount-policy'; import { buildAgentVolumes } from './volume-builder'; import { WrapperConfig } from '../../types'; @@ -78,22 +79,39 @@ describe('nested mountpoint preparation', () => { }); it('reports the mountpoint a read-only cover cannot create', () => { + const emptyHome = path.join(tmpRoot, 'chroot-home'); + const logs = path.join(tmpRoot, 'agent-logs'); + [emptyHome, logs].forEach((dir) => fs.mkdirSync(dir, { recursive: true })); const volumes = [ - '/empty-home:/host/home/runner:ro', - '/logs:/host/home/runner/.copilot/logs:rw', + `${emptyHome}:/host/home/runner:ro`, + `${logs}:/host/home/runner/.copilot/logs:rw`, ]; expect(planNestedMountpoints(volumes)).toEqual([ expect.objectContaining({ containerTarget: '/host/home/runner/.copilot/logs', coveringTarget: '/host/home/runner', - coveringSource: '/empty-home', - hostPath: '/empty-home/.copilot/logs', + coveringSource: emptyHome, + hostPath: path.join(emptyHome, '.copilot/logs'), kind: 'directory', + credentialOverlay: false, }), ]); }); + it('cannot classify a source that does not exist on this filesystem', () => { + const volumes = [ + '/empty-home:/host/home/runner:ro', + '/logs:/host/home/runner/.copilot/logs:rw', + ]; + + // The kind of a bind mountpoint follows its source, so an unreachable + // source is reported as unknown rather than assumed to be a directory. + expect(planNestedMountpoints(volumes)[0]).toEqual( + expect.objectContaining({ kind: 'unknown', credentialOverlay: false }), + ); + }); + it('attributes the mountpoint to the innermost cover, not the outermost', () => { const volumes = [ '/empty-home:/host/home/runner:ro', @@ -213,18 +231,77 @@ describe('nested mountpoint preparation', () => { '/dev/null:/host/home/runner/.netrc:ro', ]; - expect(ensureNestedMountpoints(volumes, uid, gid)).toEqual([]); + // The real pipeline prunes unmountable overlays first, which is what makes + // the mask safe to drop: the path is unreachable behind a read-only bind, + // so nothing is left unmasked. + const pruned = pruneUnmountableCredentialOverlays(volumes); + expect(pruned).not.toContain('/dev/null:/host/home/runner/.netrc:ro'); + expect(ensureNestedMountpoints(pruned, uid, gid)).toEqual([]); + expect(fs.existsSync(path.join(emptyHome, '.netrc'))).toBe(false); + }); + + it('refuses to launch rather than create a credential mountpoint itself', () => { + const { emptyHome } = makeTree(); + // Same list, but without the prune step: AWF must not quietly paper over + // an overlay it cannot satisfy, and it must not create the credential path. + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + '/dev/null:/host/home/runner/.netrc:ro', + ]; + + expect(() => ensureNestedMountpoints(volumes, uid, gid)).toThrow(/must not create/); expect(fs.existsSync(path.join(emptyHome, '.netrc'))).toBe(false); }); - it('skips mounts whose source does not exist on this filesystem', () => { + it('fails closed on a required mountpoint whose source cannot be classified', () => { const { emptyHome } = makeTree(); + const missingSource = path.join(tmpRoot, 'missing-source'); const volumes = [ `${emptyHome}:/host/home/runner:ro`, - `${path.join(tmpRoot, 'missing-source')}:/host/home/runner/.copilot/logs:rw`, + `${missingSource}:/host/home/runner/.copilot/logs:rw`, + ]; + + // The cover is real, so this mountpoint genuinely has to exist before + // launch. Guessing a directory could create the wrong node type, and + // skipping it silently is what produced the opaque EROFS this pass exists + // to prevent. + expect(() => ensureNestedMountpoints(volumes, uid, gid)) + .toThrow(/could not be classified/); + expect(fs.existsSync(path.join(emptyHome, '.copilot'))).toBe(false); + }); + + it('creates a file mountpoint for a regular-file bind under a read-only cover', () => { + const { emptyHome } = makeTree(); + const sourceFile = path.join(tmpRoot, 'runner-binary'); + fs.writeFileSync(sourceFile, '#!/bin/sh\n', { mode: 0o755 }); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `${sourceFile}:/host/home/runner/bin/tool:ro`, + ]; + + const created = ensureNestedMountpoints(volumes, uid, gid); + + const mountpoint = path.join(emptyHome, 'bin/tool'); + expect(created).toEqual([mountpoint]); + expect(fs.statSync(mountpoint).isFile()).toBe(true); + expect(fs.readFileSync(mountpoint, 'utf8')).toBe(''); + // The parent directory has to be created too, or the file cannot land. + expect(fs.statSync(path.join(emptyHome, 'bin')).isDirectory()).toBe(true); + }); + + it('leaves an existing file mountpoint untouched', () => { + const { emptyHome } = makeTree(); + const sourceFile = path.join(tmpRoot, 'runner-binary'); + fs.writeFileSync(sourceFile, '#!/bin/sh\n', { mode: 0o755 }); + fs.mkdirSync(path.join(emptyHome, 'bin'), { recursive: true }); + fs.writeFileSync(path.join(emptyHome, 'bin/tool'), 'PRE-EXISTING'); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `${sourceFile}:/host/home/runner/bin/tool:ro`, ]; expect(ensureNestedMountpoints(volumes, uid, gid)).toEqual([]); + expect(fs.readFileSync(path.join(emptyHome, 'bin/tool'), 'utf8')).toBe('PRE-EXISTING'); }); it('skips daemon-side covers instead of creating a runner-local tree', () => { @@ -325,4 +402,106 @@ describe('nested mountpoint preparation', () => { expect(simulateRuncMountFailures(volumes)).toEqual([]); }); }); + + // A regular-file bind nested inside a read-only cover needs a *file* + // mountpoint. runc cannot create one inside a read-only bind any more than it + // can create a directory, so this has to be prepared too. It is reachable on + // split-filesystem runners: the agent binary is staged under the daemon path + // prefix and published at /tmp/awf-runner-bin/, while a write policy + // narrows the /tmp:/tmp bind to read-only. + describe('buildAgentVolumes (staged runner binary under --docker-host-path-prefix)', () => { + const prefixRoots: string[] = []; + const runnerBinPaths: string[] = []; + + afterEach(() => { + // Staging and the /tmp cover are both real paths by construction: the + // staging root has to sit under the daemon prefix, and the cover bind is + // hardcoded to /tmp. + prefixRoots.splice(0).forEach((dir) => fs.rmSync(dir, { recursive: true, force: true })); + runnerBinPaths.splice(0).forEach((file) => fs.rmSync(file, { force: true })); + }); + + function stageSplitFsLayout() { + const unique = path.basename(tmpRoot).replace(/[^a-zA-Z0-9]/g, ''); + const binaryName = `awfprobe${unique}`; + // shouldUseDockerHostStaging only engages for a prefix under /tmp, so this + // cannot be redirected into the test's own sandbox. + const dockerHostPathPrefix = `/tmp/awf-prefix-${unique}`; + prefixRoots.push(dockerHostPathPrefix); + runnerBinPaths.push(path.join('/tmp/awf-runner-bin', binaryName)); + + const home = path.join(tmpRoot, 'home', 'runner'); + const workspaceDir = path.join(home, 'work', 'repo', 'repo'); + const workDir = path.join(tmpRoot, 'awf-run'); + const emptyHome = `${workDir}-chroot-home`; + const agentLogsPath = path.join(workDir, 'agent-logs'); + const sessionStatePath = path.join(workDir, 'agent-session-state'); + const initSignalDir = path.join(workDir, 'init-signal'); + const binDir = path.join(tmpRoot, 'runner-bin'); + + [workspaceDir, workDir, emptyHome, agentLogsPath, sessionStatePath, initSignalDir, binDir] + .forEach((dir) => fs.mkdirSync(dir, { recursive: true })); + for (const toolPath of HOME_TOOL_PATHS) { + fs.mkdirSync(path.join(home, toolPath), { recursive: true }); + fs.mkdirSync(path.join(emptyHome, toolPath), { recursive: true }); + } + fs.mkdirSync(path.join(emptyHome, 'work', 'repo', 'repo'), { recursive: true }); + + const binarySourcePath = path.join(binDir, binaryName); + fs.writeFileSync(binarySourcePath, '#!/bin/sh\n', { mode: 0o755 }); + + const config = { + agentCommand: binarySourcePath, + allowedDomains: [], + workDir, + volumeMounts: [], + dockerHostPathPrefix, + } as unknown as WrapperConfig; + + return { + binaryName, + workspaceDir, + build: (filesystemAllowWrite?: string[]) => buildAgentVolumes({ + config: { ...config, filesystemAllowWrite } as WrapperConfig, + projectRoot: process.cwd(), + effectiveHome: home, + workspaceDir, + agentLogsPath, + sessionStatePath, + initSignalDir, + }), + }; + } + + it('classifies the staged binary mountpoint as a file, not a directory', () => { + const layout = stageSplitFsLayout(); + const writable = path.join(layout.workspaceDir, 'allowed'); + fs.mkdirSync(writable, { recursive: true }); + + const volumes = layout.build([writable]); + const requirement = planNestedMountpoints(volumes) + .find((candidate) => candidate.containerTarget.endsWith(`/awf-runner-bin/${layout.binaryName}`)); + + expect(requirement).toBeDefined(); + expect(requirement?.kind).toBe('file'); + }); + + it('creates the file mountpoint runc cannot create under a read-only /tmp', () => { + const layout = stageSplitFsLayout(); + const writable = path.join(layout.workspaceDir, 'allowed'); + fs.mkdirSync(writable, { recursive: true }); + + const volumes = layout.build([writable]); + + const mountpoint = path.join('/tmp/awf-runner-bin', layout.binaryName); + expect(fs.statSync(mountpoint).isFile()).toBe(true); + // An empty placeholder: it exists only so runc has something to bind over. + expect(fs.readFileSync(mountpoint, 'utf8')).toBe(''); + // The bind really is published, and the /tmp cover really is read-only — + // otherwise this test would pass without exercising anything. + expect(volumes.some((spec) => spec.endsWith(':/tmp:ro'))).toBe(true); + expect(volumes.some((spec) => spec.endsWith(`:/tmp/awf-runner-bin/${layout.binaryName}:ro`))) + .toBe(true); + }); + }); }); diff --git a/src/services/agent-volumes/nested-mountpoints.ts b/src/services/agent-volumes/nested-mountpoints.ts index aa62cd5d3..b1bd59cbb 100644 --- a/src/services/agent-volumes/nested-mountpoints.ts +++ b/src/services/agent-volumes/nested-mountpoints.ts @@ -1,6 +1,8 @@ import * as fs from 'fs'; +import * as path from 'path'; import { logger } from '../../logger'; import { createMissingOwnedDirectorySegments } from '../../fs-utils'; +import { isStagedHostFile } from './docker-host-staging'; import { LocalSourceResolver, ParsedMount, @@ -20,8 +22,41 @@ export interface NestedMountpointRequirement { coveringSource: string; /** Path on the runner that must exist so runc does not have to create it. */ hostPath?: string; - /** `/dev/null` credential overlays need a file, every other bind a directory. */ - kind: 'directory' | 'file'; + /** + * What runc needs at the mountpoint. A bind's target must match its source's + * type, so this is derived from the source, not guessed from the target. + * `unknown` means the source could not be classified on this filesystem. + */ + kind: 'directory' | 'file' | 'unknown'; + /** + * `/dev/null` credential masks. These need a file mountpoint too, but AWF must + * never fabricate one: creating the credential path is exactly what the mask + * exists to prevent. Unmountable overlays are dropped upstream instead. + */ + credentialOverlay: boolean; +} + +function statKind(candidate: string): 'directory' | 'file' | 'unknown' { + try { + const stats = fs.statSync(candidate); + if (stats.isDirectory()) return 'directory'; + if (stats.isFile()) return 'file'; + return 'unknown'; + } catch { + return 'unknown'; + } +} + +function resolveSourceKind( + source: string, + localSource: string | undefined, +): 'directory' | 'file' | 'unknown' { + // Staged files sit under the daemon prefix, so `localSource` is deliberately + // undefined for them. The staging record is the only surviving evidence of + // their type. + if (isStagedHostFile(source)) return 'file'; + if (localSource === undefined) return 'unknown'; + return statKind(localSource); } /** @@ -63,13 +98,17 @@ export function planNestedMountpoints( if (!suffix) continue; const localCoverSource = localSourceResolver(cover.source); + const credentialOverlay = mount.source === '/dev/null'; requirements.push({ containerTarget: mount.target, source: mount.source, coveringTarget: cover.target, coveringSource: cover.source, hostPath: localCoverSource === undefined ? undefined : `${localCoverSource}${suffix}`, - kind: mount.source === '/dev/null' ? 'file' : 'directory', + kind: credentialOverlay + ? 'file' + : resolveSourceKind(mount.source, localSourceResolver(mount.source)), + credentialOverlay, }); } @@ -85,19 +124,54 @@ function isExistingDirectory(candidate: string): boolean { } /** - * Creates the directory mountpoints reported by {@link planNestedMountpoints}. + * Creates an empty file for runc to bind over. + * + * Deliberately exclusive (`wx`): if something already occupies the path we must + * not truncate it, and a racing creator means the mountpoint exists anyway. + * Contents are never written — the bind replaces the file's contents wholesale, + * so the placeholder only has to exist and be of the right type. + */ +function createOwnedPlaceholderFile(filePath: string, uid: number, gid: number): void { + let handle: number | undefined; + try { + handle = fs.openSync(filePath, 'wx', 0o644); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'EEXIST') return; + throw err; + } finally { + if (handle !== undefined) fs.closeSync(handle); + } + + try { + fs.chownSync(filePath, uid, gid); + } catch (err) { + // Ownership is a convenience for the in-container user; the mountpoint + // itself is what runc needs, and the bind masks the placeholder entirely. + logger.debug(`Could not chown placeholder mountpoint ${filePath}: ${err}`); + } +} + +/** + * Creates the mountpoints reported by {@link planNestedMountpoints}. + * + * A requirement is *inert* only when the covering bind's source is not a real + * directory on this filesystem: that means a fabricated path or a daemon-side + * one, and in both cases there is nothing here we could or should write into. * - * Only directories are prepared. `/dev/null` credential overlays intentionally - * are not: fabricating a credential file just to mount over it would create the - * very path we are hiding. Those are handled by - * {@link ../agent-volumes/credential-hiding.pruneUnmountableCredentialOverlays} - * instead, which drops overlays that are unreachable anyway. + * Everything else is *required*. A required mountpoint is either already + * present, or gets created as the same type as its source — a directory for a + * directory bind, an empty placeholder file for a regular-file bind. Nothing + * required is skipped silently: a requirement we cannot classify or cannot + * satisfy fails the launch, because the alternative is an opaque EROFS from + * container init long after the useful context is gone. * - * A requirement is skipped when the covering bind's source is not a real - * directory on this filesystem — that means either a fabricated path (unit - * tests) or a daemon-side path we must not touch. Any other failure propagates: - * an unpreparable mountpoint is a launch failure, and failing here is much - * better than an opaque EROFS from container init. + * The one thing never fabricated is a `/dev/null` credential mask. Creating the + * credential path is precisely what the mask exists to prevent, so unmountable + * overlays are dropped upstream by + * {@link ../agent-volumes/credential-hiding.pruneUnmountableCredentialOverlays}. + * That runs first and probes the same paths, so a surviving overlay always has + * an existing mountpoint; if one ever does not, that is a real inconsistency and + * is reported rather than papered over. */ export function ensureNestedMountpoints( volumes: string[], @@ -107,48 +181,59 @@ export function ensureNestedMountpoints( ): string[] { const created: string[] = []; const requirements = planNestedMountpoints(volumes, localSourceResolver); - const actionable: NestedMountpointRequirement[] = []; + const unmet: string[] = []; for (const requirement of requirements) { - if (requirement.kind !== 'directory') continue; + const localCoverSource = localSourceResolver(requirement.coveringSource); + // Not a place we can write: fabricated or daemon-side cover. + if (localCoverSource === undefined || !isExistingDirectory(localCoverSource)) { + logger.debug( + `Skipping mountpoint preparation for ${requirement.containerTarget}: covering bind ` + + `source ${requirement.coveringSource} is not a directory on this filesystem`, + ); + continue; + } const { hostPath } = requirement; - if (hostPath === undefined) { - logger.debug( - `Skipping mountpoint preparation for ${requirement.containerTarget}: the covering bind ` + - `source ${requirement.coveringSource} is not resolvable on this filesystem`, + // The cover resolved, so `planNestedMountpoints` always produced a hostPath. + if (hostPath === undefined || fs.existsSync(hostPath)) continue; + + if (requirement.credentialOverlay) { + unmet.push( + `${requirement.containerTarget} (credential mask needs ${hostPath}, which AWF must not create)`, ); continue; } - const localCoverSource = localSourceResolver(requirement.coveringSource); - if (localCoverSource === undefined || !isExistingDirectory(localCoverSource)) continue; - // Only prepare mountpoints for binds that really exist on this filesystem. - // A source that is absent is either a fabricated path or a daemon-side one, - // and in both cases we must not materialise a tree for it. - const localMountSource = localSourceResolver(requirement.source); - if (localMountSource === undefined || !isExistingDirectory(localMountSource)) continue; + if (requirement.kind === 'unknown') { + unmet.push( + `${requirement.containerTarget} (needs ${hostPath}, but source ${requirement.source} ` + + 'could not be classified as a file or a directory)', + ); + continue; + } - actionable.push(requirement); - if (fs.existsSync(hostPath)) continue; + if (requirement.kind === 'file') { + createMissingOwnedDirectorySegments(path.dirname(hostPath), uid, gid); + createOwnedPlaceholderFile(hostPath, uid, gid); + } else { + createMissingOwnedDirectorySegments(hostPath, uid, gid); + } - createMissingOwnedDirectorySegments(hostPath, uid, gid); created.push(hostPath); logger.debug( - `Prepared nested mountpoint ${hostPath} for ${requirement.containerTarget} ` + - `(inside read-only bind ${requirement.coveringTarget})`, + `Prepared nested ${requirement.kind} mountpoint ${hostPath} for ` + + `${requirement.containerTarget} (inside read-only bind ${requirement.coveringTarget})`, ); + + if (!fs.existsSync(hostPath)) { + unmet.push(`${requirement.containerTarget} (needs ${hostPath})`); + } } - // Fail closed: a mountpoint that is still missing would surface as an opaque - // EROFS from container init, long after the useful context is gone. - const unmet = actionable.filter((requirement) => !fs.existsSync(requirement.hostPath as string)); if (unmet.length > 0) { - const details = unmet - .map((requirement) => `${requirement.containerTarget} (needs ${requirement.hostPath})`) - .join(', '); throw new Error( - `Could not prepare bind mountpoints nested inside a read-only mount: ${details}. ` + + `Could not prepare bind mountpoints nested inside a read-only mount: ${unmet.join(', ')}. ` + 'The agent container would fail to start with a read-only filesystem error.', ); } diff --git a/src/services/init-signal-compatibility.test.ts b/src/services/init-signal-compatibility.test.ts index 4c5600df6..957b1bb18 100644 --- a/src/services/init-signal-compatibility.test.ts +++ b/src/services/init-signal-compatibility.test.ts @@ -1,5 +1,7 @@ import * as fs from 'fs'; +import * as os from 'os'; import * as path from 'path'; +import { execFileSync } from 'child_process'; import { generateDockerCompose, mockNetworkConfig, useAgentVolumesTestConfig } from './service-test-setup.test-utils'; import { INIT_SIGNAL_DIR, LEGACY_INIT_SIGNAL_DIR } from '../constants'; @@ -70,6 +72,110 @@ describe('init signal directory compatibility', () => { }); }); + describe('new CLI + old agent image: the init container runs the old script', () => { + /** + * The audit step of `setup-iptables.sh` as shipped in agent images released + * before the signal directory moved to /run. Two properties matter and both + * are load-bearing: the script runs under `set -e`, and the audit file path + * is hardcoded to the legacy directory rather than read from + * `$AWF_INIT_SIGNAL_DIR`. A redirection into a missing directory is a + * command failure, so `set -e` aborts the script before the CLI's + * `&& touch "$AWF_INIT_SIGNAL_DIR/ready"` ever runs, and the agent then + * waits out its full ready timeout. + */ + const OLD_AUDIT_STEP = [ + 'set -e', + `audit_file="${LEGACY_INIT_SIGNAL_DIR}/iptables-audit.txt"`, + 'echo "# iptables audit dump" > "$audit_file"', + 'echo "## IPv4 NAT rules" >> "$audit_file"', + 'echo OLD_SCRIPT_COMPLETED', + ].join('\n'); + + /** + * Materialises the init container's filesystem view: every bind target in + * the generated service exists as a directory, and nothing else does. + */ + function stageInitContainerRootfs(volumes: string[]): string { + const rootfs = fs.mkdtempSync(path.join(os.tmpdir(), 'awf-initns-')); + for (const spec of volumes) { + const target = spec.split(':')[1]; + if (target) fs.mkdirSync(path.join(rootfs, target), { recursive: true }); + } + return rootfs; + } + + function runOldSetupScript(rootfs: string): { stdout: string; readyExists: boolean } { + const script = [ + `cd "${rootfs}"`, + // Rebase the container-absolute paths onto the staged rootfs. + OLD_AUDIT_STEP.replace( + `audit_file="${LEGACY_INIT_SIGNAL_DIR}`, + `audit_file="${rootfs}${LEGACY_INIT_SIGNAL_DIR}`, + ), + // Exactly how the CLI chains the ready signal after the script. + `touch "${rootfs}${INIT_SIGNAL_DIR}/ready"`, + ].join('\n'); + + let stdout = ''; + try { + stdout = execFileSync('/bin/sh', ['-c', script], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + } catch (err) { + stdout = String((err as { stdout?: string }).stdout ?? ''); + } + return { + stdout, + readyExists: fs.existsSync(path.join(rootfs, INIT_SIGNAL_DIR.slice(1), 'ready')), + }; + } + + it('lets the old audit dump succeed so the ready signal is still written', () => { + const volumes = generateDockerCompose(getConfig(), mockNetworkConfig) + .services['iptables-init'].volumes as string[]; + const rootfs = stageInitContainerRootfs(volumes); + + try { + const result = runOldSetupScript(rootfs); + + expect(result.stdout).toContain('OLD_SCRIPT_COMPLETED'); + expect(result.readyExists).toBe(true); + } finally { + fs.rmSync(rootfs, { recursive: true, force: true }); + } + }); + + it('proves the test would catch the regression it is guarding', () => { + // Same script, but with only the current signal directory mounted: this is + // the state that stranded an older image, and it must be detectable. + const rootfs = stageInitContainerRootfs([`/src:${INIT_SIGNAL_DIR}:rw`]); + + try { + const result = runOldSetupScript(rootfs); + + expect(result.stdout).not.toContain('OLD_SCRIPT_COMPLETED'); + expect(result.readyExists).toBe(false); + } finally { + fs.rmSync(rootfs, { recursive: true, force: true }); + } + }); + + it('mounts the legacy path writable, because the old script writes there', () => { + const volumes = generateDockerCompose(getConfig(), mockNetworkConfig) + .services['iptables-init'].volumes as string[]; + + expect(volumes).toContain(`${getConfig().workDir}/init-signal:${LEGACY_INIT_SIGNAL_DIR}:rw`); + expect(volumes).toContain(`${getConfig().workDir}/init-signal:${INIT_SIGNAL_DIR}:rw`); + }); + + it('keeps both init-signal views on one source, so either path signals the agent', () => { + const volumes = (generateDockerCompose(getConfig(), mockNetworkConfig) + .services['iptables-init'].volumes as string[]) + .filter((spec) => spec.includes('init-signal')) + .map((spec) => spec.split(':')[0]); + + expect(new Set(volumes).size).toBe(1); + }); + }); + it('does not rely on a symlink inside the init container, which the agent cannot see', () => { const command = generateDockerCompose(getConfig(), mockNetworkConfig) .services['iptables-init'].command as string[]; From 319add7c53252cbdb3936d833f789603b8b36ae7 Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 21:29:10 -0700 Subject: [PATCH 12/18] fix: treat an absent bind source as a directory mountpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fail-closed path added for unclassifiable sources was too broad: it also caught sources that simply do not exist yet. Several AWF-owned sources, the init signal directory among them, are materialised after the volume list is built, so classification ran against a path that was not there and the launch was refused with "could not be classified". Absent is not ambiguous. Verified against a real daemon: a bind whose source is missing yields a directory, on the host and in the guest alike. So an absent-but-resolvable source is classified as a directory, which is the node type runc will need. Only a source with no local path at all — a daemon-side path under --docker-host-path-prefix that we did not stage — remains unclassifiable and still fails closed. This surfaced on Linux CI only. macOS resolves /tmp through /private/tmp, so the temporary directories these suites build sit outside the /tmp cover and never nest. The new regression builds its own cover and reproduces the failure on any platform; the two suites that caught it in CI now also pass when run with TMPDIR=/tmp. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/nested-mountpoints.test.ts | 47 ++++++++++++++----- .../agent-volumes/nested-mountpoints.ts | 18 +++++-- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index 9d0efcfde..ab1e877c4 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -99,15 +99,20 @@ describe('nested mountpoint preparation', () => { ]); }); - it('cannot classify a source that does not exist on this filesystem', () => { + it('cannot classify a source that this filesystem cannot see at all', () => { const volumes = [ '/empty-home:/host/home/runner:ro', - '/logs:/host/home/runner/.copilot/logs:rw', + '/daemon-only/logs:/host/home/runner/.copilot/logs:rw', ]; - - // The kind of a bind mountpoint follows its source, so an unreachable - // source is reported as unknown rather than assumed to be a directory. - expect(planNestedMountpoints(volumes)[0]).toEqual( + // Under --docker-host-path-prefix a source can belong to the daemon's + // filesystem, not the runner's. There is nothing here to stat, so the kind + // is reported as unknown rather than assumed. A source that is merely + // absent is different: the daemon materialises those as directories, and + // `resolveSourceKind` classifies them accordingly. + const resolver = (source: string): string | undefined => + source.startsWith('/daemon-only/') ? undefined : source; + + expect(planNestedMountpoints(volumes, resolver)[0]).toEqual( expect.objectContaining({ kind: 'unknown', credentialOverlay: false }), ); }); @@ -253,19 +258,37 @@ describe('nested mountpoint preparation', () => { expect(fs.existsSync(path.join(emptyHome, '.netrc'))).toBe(false); }); - it('fails closed on a required mountpoint whose source cannot be classified', () => { + it('creates a directory for a source the daemon has not materialised yet', () => { const { emptyHome } = makeTree(); - const missingSource = path.join(tmpRoot, 'missing-source'); + // AWF creates the init signal directory after the volume list is built, + // so at this point the source legitimately does not exist. This is the + // real shape of the agent's legacy `/tmp/awf-init` bind under a narrowed + // `/tmp`, and it must not be mistaken for an unclassifiable source. + const notYetCreated = path.join(tmpRoot, 'init-signal'); const volumes = [ `${emptyHome}:/host/home/runner:ro`, - `${missingSource}:/host/home/runner/.copilot/logs:rw`, + `${notYetCreated}:/host/home/runner/.copilot/logs:rw`, ]; - // The cover is real, so this mountpoint genuinely has to exist before - // launch. Guessing a directory could create the wrong node type, and + expect(() => ensureNestedMountpoints(volumes, uid, gid)).not.toThrow(); + const mountpoint = path.join(emptyHome, '.copilot/logs'); + expect(fs.statSync(mountpoint).isDirectory()).toBe(true); + }); + + it('fails closed on a required mountpoint whose source cannot be classified', () => { + const { emptyHome } = makeTree(); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `/daemon-only/opaque:/host/home/runner/.copilot/logs:rw`, + ]; + // A daemon-side source we never staged: there is nothing on this + // filesystem to inspect. Guessing could create the wrong node type, and // skipping it silently is what produced the opaque EROFS this pass exists // to prevent. - expect(() => ensureNestedMountpoints(volumes, uid, gid)) + const resolver = (source: string): string | undefined => + source.startsWith('/daemon-only/') ? undefined : source; + + expect(() => ensureNestedMountpoints(volumes, uid, gid, resolver)) .toThrow(/could not be classified/); expect(fs.existsSync(path.join(emptyHome, '.copilot'))).toBe(false); }); diff --git a/src/services/agent-volumes/nested-mountpoints.ts b/src/services/agent-volumes/nested-mountpoints.ts index b1bd59cbb..c5293e13f 100644 --- a/src/services/agent-volumes/nested-mountpoints.ts +++ b/src/services/agent-volumes/nested-mountpoints.ts @@ -36,13 +36,14 @@ export interface NestedMountpointRequirement { credentialOverlay: boolean; } -function statKind(candidate: string): 'directory' | 'file' | 'unknown' { +function statKind(candidate: string): 'directory' | 'file' | 'missing' | 'unknown' { try { const stats = fs.statSync(candidate); if (stats.isDirectory()) return 'directory'; if (stats.isFile()) return 'file'; return 'unknown'; - } catch { + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return 'missing'; return 'unknown'; } } @@ -55,8 +56,19 @@ function resolveSourceKind( // undefined for them. The staging record is the only surviving evidence of // their type. if (isStagedHostFile(source)) return 'file'; + // No local path at all: a daemon-side source we cannot inspect and did not + // stage. Guessing here is what the fail-closed path exists to prevent. if (localSource === undefined) return 'unknown'; - return statKind(localSource); + + const kind = statKind(localSource); + // A source that simply does not exist yet is not ambiguous. Some AWF-owned + // sources (the init signal directory among them) are materialised after the + // volume list is built, and the daemon creates a *directory* for any bind + // source still missing at launch — so the mountpoint has to be a directory to + // match. Verified against a real daemon: binding a missing source yields a + // directory on both the host and inside the guest. + if (kind === 'missing') return 'directory'; + return kind; } /** From e831ac6516369563c8ea7f64b3af4f1f04055005 Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 21:39:25 -0700 Subject: [PATCH 13/18] fix: refuse an unusable file mountpoint instead of trusting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged the placeholder created for file mountpoints, and it was right to. The path can be `/tmp/awf-runner-bin/` — inside a world-writable directory — so it is created owner-only now rather than 0644. The bind does not care about its mountpoint's permissions, verified against a real daemon. The mode was the smaller half. An exclusive create refuses to follow a symlink, so a planted one came back as EEXIST and was treated as "someone else got there first, fine". A dangling symlink needs no race to plant: existsSync() follows it and reports the path absent, preparation proceeds, and the exclusive create then fails. Both paths ended up handing runc a mountpoint of someone else's choosing. An existing entry of the wrong type was accepted for the same reason, and the daemon rejects that bind with "not a directory" once the run is already under way. Both now fail closed, matching the symlink stance createMissingOwnedDirectorySegments already takes for the parent segments. Nothing is deleted or repaired: removing a path AWF did not create is how a mountpoint helper becomes a destructive one. The check is scoped to file mountpoints, the one kind this pass introduced, so existing directory mountpoints behave exactly as before. Also removes two check-then-use races in the tests (one stat now answers both type and emptiness) and escapes a shell expansion that CodeQL read as a mistaken JS template literal. The assertion string is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/nested-mountpoints.test.ts | 51 ++++++++++++++++-- .../agent-volumes/nested-mountpoints.ts | 53 ++++++++++++++++--- .../init-signal-compatibility.test.ts | 4 +- 3 files changed, 97 insertions(+), 11 deletions(-) diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index ab1e877c4..b5d80b746 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -306,12 +306,54 @@ describe('nested mountpoint preparation', () => { const mountpoint = path.join(emptyHome, 'bin/tool'); expect(created).toEqual([mountpoint]); - expect(fs.statSync(mountpoint).isFile()).toBe(true); - expect(fs.readFileSync(mountpoint, 'utf8')).toBe(''); + // A single stat answers both questions: an empty regular file. The + // placeholder is never written to, so its size is the assertion. + const placeholder = fs.statSync(mountpoint); + expect(placeholder.isFile()).toBe(true); + expect(placeholder.size).toBe(0); + // Owner-only: these paths can land in a world-writable directory, and a + // bind does not need its mountpoint to be readable by anyone else. + expect(placeholder.mode & 0o077).toBe(0); // The parent directory has to be created too, or the file cannot land. expect(fs.statSync(path.join(emptyHome, 'bin')).isDirectory()).toBe(true); }); + it('refuses a symlink planted where a file mountpoint belongs', () => { + const { emptyHome } = makeTree(); + const sourceFile = path.join(tmpRoot, 'runner-binary'); + fs.writeFileSync(sourceFile, '#!/bin/sh\n', { mode: 0o755 }); + fs.mkdirSync(path.join(emptyHome, 'bin'), { recursive: true }); + // A dangling symlink needs no race to plant: existsSync() follows it and + // reports false, so preparation proceeds, and an exclusive create then + // fails with EEXIST. Returning quietly there would hand runc an + // attacker-chosen mountpoint in a world-writable directory. + fs.symlinkSync('/nonexistent-target', path.join(emptyHome, 'bin/tool')); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `${sourceFile}:/host/home/runner/bin/tool:ro`, + ]; + + expect(() => ensureNestedMountpoints(volumes, uid, gid)).toThrow(/symlink/i); + // The plant is reported, never followed and never replaced. + expect(fs.lstatSync(path.join(emptyHome, 'bin/tool')).isSymbolicLink()).toBe(true); + }); + + it('refuses a directory planted where a file mountpoint belongs', () => { + const { emptyHome } = makeTree(); + const sourceFile = path.join(tmpRoot, 'runner-binary'); + fs.writeFileSync(sourceFile, '#!/bin/sh\n', { mode: 0o755 }); + fs.mkdirSync(path.join(emptyHome, 'bin/tool'), { recursive: true }); + const volumes = [ + `${emptyHome}:/host/home/runner:ro`, + `${sourceFile}:/host/home/runner/bin/tool:ro`, + ]; + + // A directory standing in for a file is not a usable mountpoint: the + // daemon rejects the bind with "not a directory" once the run is under + // way, which is far later and far more opaque than failing here. + expect(() => ensureNestedMountpoints(volumes, uid, gid)).toThrow(/not a regular file/i); + }); + it('leaves an existing file mountpoint untouched', () => { const { emptyHome } = makeTree(); const sourceFile = path.join(tmpRoot, 'runner-binary'); @@ -517,9 +559,10 @@ describe('nested mountpoint preparation', () => { const volumes = layout.build([writable]); const mountpoint = path.join('/tmp/awf-runner-bin', layout.binaryName); - expect(fs.statSync(mountpoint).isFile()).toBe(true); // An empty placeholder: it exists only so runc has something to bind over. - expect(fs.readFileSync(mountpoint, 'utf8')).toBe(''); + const placeholder = fs.statSync(mountpoint); + expect(placeholder.isFile()).toBe(true); + expect(placeholder.size).toBe(0); // The bind really is published, and the /tmp cover really is read-only — // otherwise this test would pass without exercising anything. expect(volumes.some((spec) => spec.endsWith(':/tmp:ro'))).toBe(true); diff --git a/src/services/agent-volumes/nested-mountpoints.ts b/src/services/agent-volumes/nested-mountpoints.ts index c5293e13f..4d3ebb7b2 100644 --- a/src/services/agent-volumes/nested-mountpoints.ts +++ b/src/services/agent-volumes/nested-mountpoints.ts @@ -139,16 +139,25 @@ function isExistingDirectory(candidate: string): boolean { * Creates an empty file for runc to bind over. * * Deliberately exclusive (`wx`): if something already occupies the path we must - * not truncate it, and a racing creator means the mountpoint exists anyway. - * Contents are never written — the bind replaces the file's contents wholesale, - * so the placeholder only has to exist and be of the right type. + * not truncate it. Contents are never written — the bind replaces the file's + * contents wholesale, so the placeholder only has to exist and be of the right + * type. The mode is owner-only because these paths can land in a world-writable + * directory (`/tmp/awf-runner-bin` under a narrowed `/tmp`), and a bind does not + * care about its mountpoint's permissions. + * + * An exclusive create is also what makes the symlink check below reachable + * rather than theoretical: `O_CREAT | O_EXCL` refuses to follow a symlink, so a + * planted one surfaces as `EEXIST` instead of being silently written through. */ function createOwnedPlaceholderFile(filePath: string, uid: number, gid: number): void { let handle: number | undefined; try { - handle = fs.openSync(filePath, 'wx', 0o644); + handle = fs.openSync(filePath, 'wx', 0o600); } catch (err) { - if ((err as NodeJS.ErrnoException).code === 'EEXIST') return; + if ((err as NodeJS.ErrnoException).code === 'EEXIST') { + assertUsableFileMountpoint(filePath); + return; + } throw err; } finally { if (handle !== undefined) fs.closeSync(handle); @@ -163,6 +172,28 @@ function createOwnedPlaceholderFile(filePath: string, uid: number, gid: number): } } +/** + * Accepts an entry that appeared underneath us only if runc could actually bind + * over it. + * + * Reaching here means the path was absent when preparation looked and present a + * moment later. A regular file is the benign explanation and is fine to reuse. + * Anything else is not: a symlink hands runc a target of someone else's + * choosing, and a directory makes the daemon reject the bind with "not a + * directory" once the run is already under way. Neither is repaired here — + * deleting a path we did not create is how a mountpoint helper turns into a + * destructive one — so both fail closed while the context is still useful. + */ +function assertUsableFileMountpoint(filePath: string): void { + const entry = fs.lstatSync(filePath); + if (entry.isSymbolicLink()) { + throw new Error(`Refusing to use symlink as bind mountpoint: ${filePath}`); + } + if (!entry.isFile()) { + throw new Error(`Bind mountpoint exists but is not a regular file: ${filePath}`); + } +} + /** * Creates the mountpoints reported by {@link planNestedMountpoints}. * @@ -208,7 +239,17 @@ export function ensureNestedMountpoints( const { hostPath } = requirement; // The cover resolved, so `planNestedMountpoints` always produced a hostPath. - if (hostPath === undefined || fs.existsSync(hostPath)) continue; + if (hostPath === undefined) continue; + if (fs.existsSync(hostPath)) { + // An existing directory mountpoint is the overwhelmingly common case and + // is left exactly as it was found. A *file* mountpoint is different: the + // daemon rejects the bind outright if the entry is the wrong type, so the + // one kind this pass introduces is also the one kind worth checking. + if (requirement.kind === 'file' && !requirement.credentialOverlay) { + assertUsableFileMountpoint(hostPath); + } + continue; + } if (requirement.credentialOverlay) { unmet.push( diff --git a/src/services/init-signal-compatibility.test.ts b/src/services/init-signal-compatibility.test.ts index 957b1bb18..8fd0be535 100644 --- a/src/services/init-signal-compatibility.test.ts +++ b/src/services/init-signal-compatibility.test.ts @@ -66,8 +66,10 @@ describe('init signal directory compatibility', () => { describe('old CLI + new agent image', () => { it('still accepts a ready-file delivered at the legacy path', () => { expect(entrypointSource).toContain('LEGACY_INIT_SIGNAL_DIR="/tmp/awf-init"'); + // Escaped `\${...}` so this stays a literal shell expansion rather than a + // JavaScript one: the assertion is a byte-for-byte excerpt of the script. expect(entrypointSource).toContain( - 'while [ ! -f "${INIT_SIGNAL_DIR}/ready" ] && [ ! -f "${LEGACY_INIT_SIGNAL_DIR}/ready" ]; do', + `while [ ! -f "\${INIT_SIGNAL_DIR}/ready" ] && [ ! -f "\${LEGACY_INIT_SIGNAL_DIR}/ready" ]; do`, ); }); }); From e09f0e102b037e140256e7f64deeb2a7df3a568b Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 21:56:46 -0700 Subject: [PATCH 14/18] fix: keep a shared /tmp path prefix runner-resolvable `--docker-host-path-prefix /tmp` is the ARC/DinD shared-volume shape: the runner and the daemon see the same bytes at the same path. AWF already relies on that -- it stages files there with local `fs` calls, and prefix translation deliberately leaves an already-/tmp source unrewritten. `createLocalSourceResolver` disagreed, treating *any* source under the prefix as daemon-only. With a shared prefix the run's own workDir sits inside it, so `/tmp/awf-/{init-signal,agent-logs,agent-session-state}` all became unresolvable. Under a `filesystem.allowWrite` policy those binds are nested inside a narrowed read-only cover, so their mountpoint kind could not be classified and the run failed closed before launch; the chroot home lost its covering source too, silently skipping preparation and restoring the EROFS failure this PR exists to fix. Name the distinction once as `isSharedDockerHostPathPrefix` and use it in all three places that had grown their own copy of the /tmp test -- the resolver, daemon staging, and /etc identity-file preservation. Duplicating that rule is what let a shared prefix be mistaken for a daemon-only one. Daemon-only prefixes such as `/host` still fail closed unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/docker-host-staging.ts | 7 +- src/services/agent-volumes/mount-topology.ts | 25 +++- .../agent-volumes/nested-mountpoints.test.ts | 107 ++++++++++++++++++ src/services/host-path-prefix.ts | 22 +++- 4 files changed, 154 insertions(+), 7 deletions(-) diff --git a/src/services/agent-volumes/docker-host-staging.ts b/src/services/agent-volumes/docker-host-staging.ts index 1f56c60d0..045b0a7de 100644 --- a/src/services/agent-volumes/docker-host-staging.ts +++ b/src/services/agent-volumes/docker-host-staging.ts @@ -1,6 +1,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { logger } from '../../logger'; +import { isSharedDockerHostPathPrefix } from '../host-path-prefix'; import { WrapperConfig } from '../../types'; const DOCKER_HOST_STAGE_DIR = 'awf-docker-host-stage'; @@ -34,9 +35,9 @@ function normalizeDockerHostPathPrefix(prefix: string): string { } export function shouldUseDockerHostStaging(prefix: string | undefined): boolean { - if (!prefix) return false; - const normalized = normalizeDockerHostPathPrefix(prefix); - return normalized === '/tmp' || normalized.startsWith('/tmp/'); + // Staging writes with local `fs` and expects the daemon to read it back, so + // it is only sound when the prefix is genuinely shared. + return isSharedDockerHostPathPrefix(prefix); } export function getDockerHostStageRoot(config: WrapperConfig): string { diff --git a/src/services/agent-volumes/mount-topology.ts b/src/services/agent-volumes/mount-topology.ts index 9367e203f..424031a04 100644 --- a/src/services/agent-volumes/mount-topology.ts +++ b/src/services/agent-volumes/mount-topology.ts @@ -13,8 +13,15 @@ * Custom volume mounts are materialised with daemon-side sources, so a runner * local `fs` call against them is meaningless. Everything else is still a * runner-local path at this stage because the prefix is applied last. + * + * Being *under* the prefix is therefore not the same as being daemon-only. A + * shared prefix (see `isSharedDockerHostPathPrefix`) names one filesystem both + * sides can see, and the run's own workDir routinely lives inside it — treating + * that as unresolvable would make the whole run directory unclassifiable. */ +import { isSharedDockerHostPathPrefix } from '../host-path-prefix'; + export interface ParsedMount { source: string; target: string; @@ -72,9 +79,21 @@ export function createLocalSourceResolver( } // Not a custom mount. Every other source is still runner-local here because - // `applyHostPathPrefixToVolumes` runs after this stage. If a source already - // carries the daemon prefix we cannot attribute it, so fail closed. - if (dockerHostPathPrefix && isPathPrefix(dockerHostPathPrefix, source)) return undefined; + // `applyHostPathPrefixToVolumes` runs after this stage. A source that + // already carries a *daemon-only* prefix cannot be attributed, so fail + // closed. + // + // A shared prefix is not that case: there the same path is valid on both + // sides, so the run's own workDir (and everything AWF stages) legitimately + // sits under the prefix and stays runner-resolvable. Failing closed on it + // instead would misclassify the entire run directory. + if ( + dockerHostPathPrefix + && !isSharedDockerHostPathPrefix(dockerHostPathPrefix) + && isPathPrefix(dockerHostPathPrefix, source) + ) { + return undefined; + } return source; }; diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index b5d80b746..e84b2c536 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -570,4 +570,111 @@ describe('nested mountpoint preparation', () => { .toBe(true); }); }); + + // A `--docker-host-path-prefix` under /tmp is *shared*, not daemon-only: the + // runner and the daemon see the same paths there, which is why AWF stages + // files into it with local fs calls and why prefix translation deliberately + // leaves an already-/tmp source unrewritten. The run's own workDir then sits + // *inside* the prefix, so topology passes still have to resolve it locally. + describe('buildAgentVolumes (--docker-host-path-prefix shares /tmp with the runner)', () => { + const sharedRoots: string[] = []; + + afterEach(() => { + sharedRoots.splice(0).forEach((dir) => fs.rmSync(dir, { recursive: true, force: true })); + }); + + function stageSharedTmpLayout() { + const unique = path.basename(tmpRoot).replace(/[^a-zA-Z0-9]/g, ''); + // The prefix under test is the literal /tmp, and the whole run has to sit + // beneath it, so this cannot be redirected into the suite's sandbox (on + // macOS os.tmpdir() is /private/var/... and would not nest). + const sharedRoot = `/tmp/awf-shared-${unique}`; + sharedRoots.push(sharedRoot); + + const home = path.join(sharedRoot, 'home', 'runner'); + const workspaceDir = path.join(home, 'work', 'repo', 'repo'); + const workDir = path.join(sharedRoot, 'awf-run'); + const emptyHome = `${workDir}-chroot-home`; + const agentLogsPath = path.join(workDir, 'agent-logs'); + const sessionStatePath = path.join(workDir, 'agent-session-state'); + const initSignalDir = path.join(workDir, 'init-signal'); + + [workspaceDir, workDir, emptyHome, agentLogsPath, sessionStatePath, initSignalDir] + .forEach((dir) => fs.mkdirSync(dir, { recursive: true })); + for (const toolPath of HOME_TOOL_PATHS) { + fs.mkdirSync(path.join(home, toolPath), { recursive: true }); + fs.mkdirSync(path.join(emptyHome, toolPath), { recursive: true }); + } + fs.mkdirSync(path.join(emptyHome, 'work', 'repo', 'repo'), { recursive: true }); + + const config = { + agentCommand: 'echo', + allowedDomains: [], + workDir, + volumeMounts: [], + dockerHostPathPrefix: '/tmp', + } as unknown as WrapperConfig; + + return { + workspaceDir, + initSignalDir, + build: (filesystemAllowWrite?: string[]) => buildAgentVolumes({ + config: { ...config, filesystemAllowWrite } as WrapperConfig, + projectRoot: process.cwd(), + effectiveHome: home, + workspaceDir, + agentLogsPath, + sessionStatePath, + initSignalDir, + }), + }; + } + + function stageWritable(layout: ReturnType): string { + const writable = path.join(layout.workspaceDir, 'allowed'); + fs.mkdirSync(writable, { recursive: true }); + return writable; + } + + it('resolves a workDir nested inside the prefix instead of failing closed', () => { + const layout = stageSharedTmpLayout(); + const writable = stageWritable(layout); + + expect(() => layout.build([writable])).not.toThrow(); + }); + + it('classifies the legacy init signal mountpoint nested under a narrowed /tmp', () => { + const layout = stageSharedTmpLayout(); + const volumes = layout.build([stageWritable(layout)]); + + const legacy = planNestedMountpoints(volumes) + .find((requirement) => requirement.containerTarget === '/tmp/awf-init'); + + // The source is the run's own init-signal directory, which the CLI just + // created locally — being under the shared prefix must not make it + // unclassifiable. + expect(legacy?.source).toBe(layout.initSignalDir); + expect(legacy?.kind).toBe('directory'); + expect(legacy?.hostPath).toBeDefined(); + expect(fs.existsSync(legacy?.hostPath as string)).toBe(true); + }); + + it('prepares the nested home mountpoints when the chroot home is under the prefix', () => { + const layout = stageSharedTmpLayout(); + const volumes = layout.build([stageWritable(layout)]); + + const nestedHomeMounts = planNestedMountpoints(volumes) + .filter((requirement) => requirement.containerTarget.includes('/.copilot/')); + + // Guards against passing vacuously: the policy really does narrow a home + // bind that covers these, so there is something to prepare. + expect(nestedHomeMounts.length).toBeGreaterThan(0); + for (const requirement of nestedHomeMounts) { + // An unresolvable covering source leaves hostPath undefined, which + // silently skips preparation and restores the EROFS failure. + expect(requirement.hostPath).toBeDefined(); + expect(fs.existsSync(requirement.hostPath as string)).toBe(true); + } + }); + }); }); diff --git a/src/services/host-path-prefix.ts b/src/services/host-path-prefix.ts index 32d6b90c6..7fa3942f8 100644 --- a/src/services/host-path-prefix.ts +++ b/src/services/host-path-prefix.ts @@ -20,9 +20,29 @@ function normalizeDockerHostPathPrefix(prefix: string): string { return withoutTrailingSlash || '/'; } +/** + * Is this prefix a directory the runner and the Docker daemon *share*, rather + * than a daemon-only staging root? + * + * A /tmp-rooted prefix is the ARC/DinD shared-volume shape: the same path + * resolves to the same bytes on both sides. That is why AWF can stage files + * into it with ordinary local `fs` calls, why an already-/tmp source is never + * rewritten below, and why a source under it stays runner-resolvable. A prefix + * like `/host` is the opposite: it exists only inside the daemon. + * + * Exported because every pass that reasons about prefixed paths needs the same + * answer — keeping separate copies of this test is what let a shared prefix be + * mistaken for a daemon-only one. + */ +export function isSharedDockerHostPathPrefix(prefix: string | undefined): boolean { + if (!prefix) return false; + const normalized = normalizeDockerHostPathPrefix(prefix); + return normalized === '/tmp' || normalized.startsWith('/tmp/'); +} + function shouldPreserveUnprefixedEtcIdentityFile(hostPath: string, dockerHostPathPrefix: string): boolean { return ( - (dockerHostPathPrefix === '/tmp' || dockerHostPathPrefix.startsWith('/tmp/')) && + isSharedDockerHostPathPrefix(dockerHostPathPrefix) && (hostPath === '/etc/passwd' || hostPath === '/etc/group') ); } From 12eb70dc63b0cae25ffd902e06a3140390934f51 Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 22:18:59 -0700 Subject: [PATCH 15/18] test: pin prefix resolution to a platform-independent shape The coverage job caught a test that had encoded the bug rather than the behaviour: `skips daemon-side covers` built its prefix from the suite's own tmpRoot. Under Linux CI that is `/tmp/awf-...` -- a *shared* prefix, which is runner-resolvable by design -- so the test asserted the opposite of its name. It stayed green on macOS only because `realpathSync` turns `/tmp` into `/private/var/...`, and `TMPDIR=/tmp` cannot reproduce it for the same reason. It now uses a genuinely daemon-only prefix. Add string-level resolver coverage that hardcodes both shapes, so this class of Linux-only divergence cannot hide behind a green local run again. Doing that surfaced an adjacent fail-closed weakening: the CLI only trims `--docker-host-path-prefix`, but the resolver compared against the raw value, so `/host/` failed its own prefix test and a daemon-side source was handed back as runner-local. Normalise once before comparing, and share a single exported normaliser instead of a second private copy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/docker-host-staging.ts | 9 +- .../agent-volumes/mount-topology.test.ts | 97 +++++++++++++++++++ src/services/agent-volumes/mount-topology.ts | 15 +-- .../agent-volumes/nested-mountpoints.test.ts | 15 ++- src/services/host-path-prefix.ts | 7 +- 5 files changed, 124 insertions(+), 19 deletions(-) create mode 100644 src/services/agent-volumes/mount-topology.test.ts diff --git a/src/services/agent-volumes/docker-host-staging.ts b/src/services/agent-volumes/docker-host-staging.ts index 045b0a7de..a147a3f7a 100644 --- a/src/services/agent-volumes/docker-host-staging.ts +++ b/src/services/agent-volumes/docker-host-staging.ts @@ -1,7 +1,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { logger } from '../../logger'; -import { isSharedDockerHostPathPrefix } from '../host-path-prefix'; +import { isSharedDockerHostPathPrefix, normalizeDockerHostPathPrefix } from '../host-path-prefix'; import { WrapperConfig } from '../../types'; const DOCKER_HOST_STAGE_DIR = 'awf-docker-host-stage'; @@ -27,13 +27,6 @@ export function clearStagedHostFiles(): void { stagedHostFiles.clear(); } -function normalizeDockerHostPathPrefix(prefix: string): string { - const trimmed = prefix.trim(); - if (!trimmed) return ''; - const withLeadingSlash = trimmed.startsWith('/') ? trimmed : `/${trimmed}`; - return withLeadingSlash.replace(/\/+$/, '') || '/'; -} - export function shouldUseDockerHostStaging(prefix: string | undefined): boolean { // Staging writes with local `fs` and expects the daemon to read it back, so // it is only sound when the prefix is genuinely shared. diff --git a/src/services/agent-volumes/mount-topology.test.ts b/src/services/agent-volumes/mount-topology.test.ts new file mode 100644 index 000000000..b10b5d9ca --- /dev/null +++ b/src/services/agent-volumes/mount-topology.test.ts @@ -0,0 +1,97 @@ +import { createLocalSourceResolver } from './mount-topology'; + +/** + * Pure string-level coverage of local-source resolution. + * + * The filesystem-backed suites cannot pin this down on their own: they build a + * prefix from `os.tmpdir()`, which is `/tmp/...` on Linux CI but + * `/private/var/...` on macOS after `realpathSync`. That makes "is this prefix + * shared?" answer differently per platform, so a Linux-only regression can hide + * behind a green local run. These cases hardcode both shapes instead. + */ +describe('createLocalSourceResolver', () => { + const workDirSource = '/tmp/awf-1730000000/init-signal'; + + describe('shared prefix (/tmp)', () => { + // /tmp-rooted prefixes are the ARC/DinD shared-volume shape: the same path + // is valid on the runner and in the daemon. AWF stages files there with + // local fs calls, and prefix translation leaves such a source unrewritten. + it.each(['/tmp', '/tmp/', '/tmp/shared', 'tmp'])( + 'keeps a source under %p runner-resolvable', + (prefix) => { + const resolve = createLocalSourceResolver(new Map(), prefix); + + expect(resolve('/tmp/shared/awf-run/init-signal')).toBe('/tmp/shared/awf-run/init-signal'); + }, + ); + + it('resolves the run workDir that sits inside the prefix', () => { + const resolve = createLocalSourceResolver(new Map(), '/tmp'); + + // Returning undefined here made the whole run directory unclassifiable: + // every nested bind under a policy-narrowed read-only cover then failed + // closed before launch. + expect(resolve(workDirSource)).toBe(workDirSource); + expect(resolve('/tmp/awf-1730000000-chroot-home')).toBe('/tmp/awf-1730000000-chroot-home'); + }); + }); + + describe('daemon-only prefix', () => { + it.each(['/host', '/dind', '/var/runner'])( + 'refuses to attribute a source already under %p', + (prefix) => { + const resolve = createLocalSourceResolver(new Map(), prefix); + + expect(resolve(`${prefix}/tmp/awf-run/init-signal`)).toBeUndefined(); + }, + ); + + it.each(['/host/', ' /host ', 'host'])( + 'fails closed for %p, which the CLI only trims', + (prefix) => { + // The raw value reaches the resolver, so an unnormalised comparison + // would miss and hand back a daemon path as if it were runner-local. + const resolve = createLocalSourceResolver(new Map(), prefix); + + expect(resolve('/host/data/inner')).toBeUndefined(); + }, + ); + + it('still resolves a runner path that merely resembles the prefix', () => { + const resolve = createLocalSourceResolver(new Map(), '/host'); + + // Sibling, not nested: `/hostage` must not be mistaken for `/host/...`. + expect(resolve('/hostage/thing')).toBe('/hostage/thing'); + // The prefix itself is not "under" the prefix. + expect(resolve('/host')).toBe('/host'); + }); + }); + + describe('custom mounts', () => { + it('maps a daemon-side custom source back to its runner path', () => { + const resolve = createLocalSourceResolver(new Map([['/host/data', '/data']]), '/host'); + + expect(resolve('/host/data/inner')).toBe('/data/inner'); + }); + + it('fails closed when a custom source cannot be mapped back', () => { + const resolve = createLocalSourceResolver(new Map([['/host/data', '']]), '/host'); + + expect(resolve('/host/data/inner')).toBeUndefined(); + }); + + it('applies the custom mapping even under a shared prefix', () => { + // Custom mounts are resolved before any prefix reasoning, so a shared + // prefix must not quietly bypass their fail-closed mapping. + const resolve = createLocalSourceResolver(new Map([['/tmp/data', '']]), '/tmp'); + + expect(resolve('/tmp/data/inner')).toBeUndefined(); + }); + }); + + it('resolves to itself when no prefix is configured', () => { + const resolve = createLocalSourceResolver(new Map()); + + expect(resolve(workDirSource)).toBe(workDirSource); + }); +}); diff --git a/src/services/agent-volumes/mount-topology.ts b/src/services/agent-volumes/mount-topology.ts index 424031a04..b8b18f591 100644 --- a/src/services/agent-volumes/mount-topology.ts +++ b/src/services/agent-volumes/mount-topology.ts @@ -20,7 +20,7 @@ * that as unresolvable would make the whole run directory unclassifiable. */ -import { isSharedDockerHostPathPrefix } from '../host-path-prefix'; +import { isSharedDockerHostPathPrefix, normalizeDockerHostPathPrefix } from '../host-path-prefix'; export interface ParsedMount { source: string; @@ -70,6 +70,13 @@ export function createLocalSourceResolver( customSourceRoots: Map, dockerHostPathPrefix?: string, ): LocalSourceResolver { + // The CLI only trims this value, so compare against the canonical form: a raw + // `/host/` would otherwise fail the `/host/` prefix test and let a daemon-side + // source be treated as runner-local. + const daemonOnlyPrefix = dockerHostPathPrefix && !isSharedDockerHostPathPrefix(dockerHostPathPrefix) + ? normalizeDockerHostPathPrefix(dockerHostPathPrefix) + : ''; + return (source: string): string | undefined => { for (const [daemonRoot, localRoot] of customSourceRoots) { if (source !== daemonRoot && !isPathPrefix(daemonRoot, source)) continue; @@ -87,11 +94,7 @@ export function createLocalSourceResolver( // sides, so the run's own workDir (and everything AWF stages) legitimately // sits under the prefix and stays runner-resolvable. Failing closed on it // instead would misclassify the entire run directory. - if ( - dockerHostPathPrefix - && !isSharedDockerHostPathPrefix(dockerHostPathPrefix) - && isPathPrefix(dockerHostPathPrefix, source) - ) { + if (daemonOnlyPrefix && isPathPrefix(daemonOnlyPrefix, source)) { return undefined; } diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index e84b2c536..79288fc71 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -370,15 +370,22 @@ describe('nested mountpoint preparation', () => { }); it('skips daemon-side covers instead of creating a runner-local tree', () => { - const { emptyHome, logs } = makeTree(); - const resolver = createLocalSourceResolver(new Map(), tmpRoot); + const { logs } = makeTree(); + // A *daemon-only* prefix. This deliberately does not use the suite's own + // tmpRoot: under `TMPDIR=/tmp` that is a shared prefix, which is runner + // resolvable by design, so the test would assert the opposite of its name + // on Linux while still passing on macOS (where realpath yields + // /private/var/...). + const daemonHome = '/daemon-only/home/runner'; + const resolver = createLocalSourceResolver(new Map(), '/daemon-only'); const volumes = [ - `${emptyHome}:/host/home/runner:ro`, + `${daemonHome}:/host/home/runner:ro`, `${logs}:/host/home/runner/.copilot/logs:rw`, ]; expect(ensureNestedMountpoints(volumes, uid, gid, resolver)).toEqual([]); - expect(fs.existsSync(path.join(emptyHome, '.copilot'))).toBe(false); + // The cover was left alone rather than fabricated on this filesystem. + expect(fs.existsSync('/daemon-only')).toBe(false); }); }); diff --git a/src/services/host-path-prefix.ts b/src/services/host-path-prefix.ts index 7fa3942f8..8d25f3498 100644 --- a/src/services/host-path-prefix.ts +++ b/src/services/host-path-prefix.ts @@ -12,7 +12,12 @@ // squid, api-proxy, cli-proxy) so the rewrite is symmetric across services // that share daemon-side directories. -function normalizeDockerHostPathPrefix(prefix: string): string { +/** + * Canonical form of a `--docker-host-path-prefix` value: leading slash, no + * trailing slash. Exported so every pass compares against the same string — + * a raw `/host/` fails a `/host/`-prefix test that a normalised `/host` passes. + */ +export function normalizeDockerHostPathPrefix(prefix: string): string { const trimmed = prefix.trim(); if (!trimmed) return ''; const withLeadingSlash = trimmed.startsWith('/') ? trimmed : `/${trimmed}`; From f85b0d498479214f8a8dbf9bda2ea18834285d87 Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 23:20:58 -0700 Subject: [PATCH 16/18] fix: distinguish shared from daemon-only Docker host path prefixes A `--docker-host-path-prefix` means the daemon sees runner path X at /X, so sources under it are daemon-only. The previous exemption treated every /tmp-rooted prefix as shared with the runner, which is wrong for /tmp/gh-aw: dind-probe offers it as a candidate prefix precisely when the runner and daemon filesystems are split, and workspace-mounts still translates sources already under it. Marking it shared would resolve those sources to the wrong namespace and bypass fail-closed classification. Split the concept in two so each consumer keeps its own contract: - isSharedDockerHostPathPrefix is now exactly /tmp. Only a user can supply it, AWF's workDir lives under it, and its binds are never translated, so the run only works when /tmp really is shared. - isTmpRootedDockerHostPathPrefix keeps the old meaning and still gates where AWF stages binaries and /etc identity files. Narrowing that would silently disable staging for the /tmp/gh-aw runners it was built for. Also treat a normalized prefix of `/` as no prefix. translateBindMountHostPath already ignores it, but the resolver called it daemon-only and so marked every absolute source unresolvable, which aborted mountpoint preparation. Tests: exercise the real createLocalSourceResolver rather than the identity default, so a resolver regression is caught by the hostPath assertions; assert the planned hostPath instead of existsSync, which a leftover /tmp/awf-init would satisfy vacuously; and reclaim the artefacts each run provably owns (uniquely named staged binary, its mountpoint, per-run chroot staging) while leaving the shared roots alone, since other suites create them concurrently. Preparing a mountpoint no longer dies on a chown that cannot succeed: an unprivileged caller cannot hand a directory to another owner, and macOS denies chown to non-root outright, so only that case is tolerated. Any other failure, and any failure while privileged, still propagates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- src/fs-utils.ts | 25 ++- .../agent-volumes/docker-host-staging.ts | 6 +- .../agent-volumes/mount-topology.test.ts | 44 ++++- src/services/agent-volumes/mount-topology.ts | 16 +- .../agent-volumes/nested-mountpoints.test.ts | 184 +++++++++++++++--- src/services/host-path-prefix.ts | 46 +++-- src/workdir-setup-branches.test.ts | 85 ++++++++ 7 files changed, 352 insertions(+), 54 deletions(-) diff --git a/src/fs-utils.ts b/src/fs-utils.ts index b62274318..c78d22386 100644 --- a/src/fs-utils.ts +++ b/src/fs-utils.ts @@ -111,7 +111,30 @@ export function createMissingOwnedDirectorySegments(dirPath: string, uid: number } if (created) { - fs.chownSync(currentPath, uid, gid); + // Only chown when ownership actually has to change, and only when this + // process could possibly succeed. A freshly created directory already + // belongs to the caller, so an owner-preserving chown is a no-op that + // macOS still denies to non-root; and a non-root caller can never hand a + // directory to a *different* owner, so attempting it only turns a usable + // directory into a hard EPERM failure. AWF runs privileged in production, + // where this is unchanged. + if (stat.uid !== uid || stat.gid !== gid) { + try { + fs.chownSync(currentPath, uid, gid); + } catch (error) { + // An unprivileged caller cannot hand a directory to another owner, so + // this can never succeed however often it is retried. Turning that + // into a hard failure would abort on a directory that is already + // usable, so tolerate exactly that case and nothing else. AWF runs + // privileged in production, where the chown still happens and still + // propagates any failure. + const code = (error as NodeJS.ErrnoException).code; + const unprivileged = process.getuid?.() !== 0; + if (!unprivileged || (code !== 'EPERM' && code !== 'EACCES')) { + throw error; + } + } + } fs.chmodSync(currentPath, 0o755); } } diff --git a/src/services/agent-volumes/docker-host-staging.ts b/src/services/agent-volumes/docker-host-staging.ts index a147a3f7a..8cde0503c 100644 --- a/src/services/agent-volumes/docker-host-staging.ts +++ b/src/services/agent-volumes/docker-host-staging.ts @@ -1,7 +1,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { logger } from '../../logger'; -import { isSharedDockerHostPathPrefix, normalizeDockerHostPathPrefix } from '../host-path-prefix'; +import { isTmpRootedDockerHostPathPrefix, normalizeDockerHostPathPrefix } from '../host-path-prefix'; import { WrapperConfig } from '../../types'; const DOCKER_HOST_STAGE_DIR = 'awf-docker-host-stage'; @@ -28,9 +28,7 @@ export function clearStagedHostFiles(): void { } export function shouldUseDockerHostStaging(prefix: string | undefined): boolean { - // Staging writes with local `fs` and expects the daemon to read it back, so - // it is only sound when the prefix is genuinely shared. - return isSharedDockerHostPathPrefix(prefix); + return isTmpRootedDockerHostPathPrefix(prefix); } export function getDockerHostStageRoot(config: WrapperConfig): string { diff --git a/src/services/agent-volumes/mount-topology.test.ts b/src/services/agent-volumes/mount-topology.test.ts index b10b5d9ca..60e8b1859 100644 --- a/src/services/agent-volumes/mount-topology.test.ts +++ b/src/services/agent-volumes/mount-topology.test.ts @@ -13,15 +13,15 @@ describe('createLocalSourceResolver', () => { const workDirSource = '/tmp/awf-1730000000/init-signal'; describe('shared prefix (/tmp)', () => { - // /tmp-rooted prefixes are the ARC/DinD shared-volume shape: the same path - // is valid on the runner and in the daemon. AWF stages files there with - // local fs calls, and prefix translation leaves such a source unrewritten. - it.each(['/tmp', '/tmp/', '/tmp/shared', 'tmp'])( + // Only the exact /tmp shape is structurally shared: AWF's own workDir then + // sits under the prefix and its binds are never translated, so the daemon + // has to resolve them to the same bytes. + it.each(['/tmp', '/tmp/', ' /tmp ', 'tmp'])( 'keeps a source under %p runner-resolvable', (prefix) => { const resolve = createLocalSourceResolver(new Map(), prefix); - expect(resolve('/tmp/shared/awf-run/init-signal')).toBe('/tmp/shared/awf-run/init-signal'); + expect(resolve('/tmp/awf-run/init-signal')).toBe('/tmp/awf-run/init-signal'); }, ); @@ -37,6 +37,30 @@ describe('createLocalSourceResolver', () => { }); describe('daemon-only prefix', () => { + // A /tmp *descendant* is daemon-only, not shared. `/tmp/gh-aw` is one of + // dind-probe's CANDIDATE_PREFIXES, and that loop is reached only after the + // probe confirms the daemon cannot see the runner's filesystem — it means + // the daemon sees runner path X at /tmp/gh-aw/X. buildCustomVolumeMounts + // agrees, translating sources that already start with /tmp/gh-aw. + it.each([ + ['/tmp/gh-aw', '/tmp/gh-aw/awf-docker-host-stage/bin/copilot'], + ['/tmp/gh-aw/', '/tmp/gh-aw/tmp/awf-1730000000/init-signal'], + ['/tmp/shared', '/tmp/shared/awf-run/init-signal'], + ['/tmp/gh-aw/nested', '/tmp/gh-aw/nested/anything'], + ])('fails closed for the /tmp descendant %p', (prefix, source) => { + const resolve = createLocalSourceResolver(new Map(), prefix); + + expect(resolve(source)).toBeUndefined(); + }); + + it('still resolves a plain /tmp path under a /tmp descendant prefix', () => { + const resolve = createLocalSourceResolver(new Map(), '/tmp/gh-aw'); + + // Not under the prefix, so it is an ordinary runner path that + // translation will rewrite later. + expect(resolve('/tmp/awf-1730000000/init-signal')).toBe('/tmp/awf-1730000000/init-signal'); + }); + it.each(['/host', '/dind', '/var/runner'])( 'refuses to attribute a source already under %p', (prefix) => { @@ -94,4 +118,14 @@ describe('createLocalSourceResolver', () => { expect(resolve(workDirSource)).toBe(workDirSource); }); + + // `/` normalises to `/`, which translateBindMountHostPath returns unchanged — + // it prefixes nothing. Reading it as a daemon root would make every absolute + // source unattributable and fail an otherwise ordinary run closed. + it.each(['/', '//', ' / '])('treats %p as no prefix at all', (prefix) => { + const resolve = createLocalSourceResolver(new Map(), prefix); + + expect(resolve(workDirSource)).toBe(workDirSource); + expect(resolve('/home/runner/work/repo/repo')).toBe('/home/runner/work/repo/repo'); + }); }); diff --git a/src/services/agent-volumes/mount-topology.ts b/src/services/agent-volumes/mount-topology.ts index b8b18f591..294e20e98 100644 --- a/src/services/agent-volumes/mount-topology.ts +++ b/src/services/agent-volumes/mount-topology.ts @@ -16,8 +16,10 @@ * * Being *under* the prefix is therefore not the same as being daemon-only. A * shared prefix (see `isSharedDockerHostPathPrefix`) names one filesystem both - * sides can see, and the run's own workDir routinely lives inside it — treating - * that as unresolvable would make the whole run directory unclassifiable. + * sides can see at the same path, and the run's own workDir lives inside it — + * treating that as unresolvable would make the whole run directory + * unclassifiable. The mirror-image mistake is just as bad: a daemon-only + * prefix that merely looks shared (`/tmp/gh-aw`) must keep failing closed. */ import { isSharedDockerHostPathPrefix, normalizeDockerHostPathPrefix } from '../host-path-prefix'; @@ -73,9 +75,17 @@ export function createLocalSourceResolver( // The CLI only trims this value, so compare against the canonical form: a raw // `/host/` would otherwise fail the `/host/` prefix test and let a daemon-side // source be treated as runner-local. - const daemonOnlyPrefix = dockerHostPathPrefix && !isSharedDockerHostPathPrefix(dockerHostPathPrefix) + const normalizedPrefix = dockerHostPathPrefix ? normalizeDockerHostPathPrefix(dockerHostPathPrefix) : ''; + // `/` prefixes nothing: `translateBindMountHostPath` returns every mount + // unchanged for it, so the generated sources are plain runner paths. Reading + // it as a daemon root instead would make *every* absolute source + // unattributable and fail the run closed before launch. + const isNoOpPrefix = normalizedPrefix === '' || normalizedPrefix === '/'; + const daemonOnlyPrefix = !isNoOpPrefix && !isSharedDockerHostPathPrefix(normalizedPrefix) + ? normalizedPrefix + : ''; return (source: string): string | undefined => { for (const [daemonRoot, localRoot] of customSourceRoots) { diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index 79288fc71..5057ce468 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -57,6 +57,79 @@ function simulateRuncMountFailures(volumes: string[]): string[] { return failures; } +/** + * Cleanup for the product-owned, *fixed* /tmp paths that a /tmp-rooted + * `--docker-host-path-prefix` writes to (`/tmp/awf-init`, + * `/tmp/awf-runner-bin`, `/tmp/awf-docker-host-stage`). + * + * Deliberately removes *files only*, and only files this run provably owns: + * the uniquely named staged binary, the mountpoint that was materialised for + * it, the per-run random `chroot-*` staging directory, and staged copies that + * did not exist before the build. + * + * It never removes the shared root directories themselves, for two measured + * reasons: + * - Several pre-existing suites create them concurrently under + * `maxWorkers`, so deleting one races with a worker that still needs it. + * - `ensureNestedMountpoints` chowns any directory it creates, and macOS + * denies chown to non-root. So on macOS a *missing* `/tmp/awf-init` makes + * unrelated suites fail with EPERM, where a pre-existing one succeeds. + * Tests must therefore never depend on these roots being absent or present; + * the assertions below read the planned `hostPath` instead of `existsSync`. + */ +function makeFixedTmpArtifactTracker(stagedCopies: string[] = []) { + const files: string[] = []; + const dirs: string[] = []; + + return { + /** Call before building, so pre-existing copies are never removed. */ + noteBeforeBuild(): void { + for (const copy of stagedCopies) { + if (!fs.existsSync(copy)) files.push(copy); + } + }, + /** Attribute this run's artefacts from the specs it actually generated. */ + trackGenerated(volumes: string[], binaryName: string): void { + for (const spec of volumes) { + const [source = '', target = ''] = spec.split(':'); + const chroot = /^(\/tmp\/awf-docker-host-stage\/chroot-[A-Za-z0-9_-]+)\//.exec(source); + if (chroot) { + dirs.push(chroot[1]); + continue; + } + if (!binaryName || path.basename(source) !== binaryName) continue; + // The staged copy is the bind *source*; the mountpoint that + // ensureNestedMountpoints had to materialise under the narrowed /tmp + // cover is the bind *target*. Both are real files on this machine. + for (const candidate of [source, target]) { + if (candidate.startsWith('/tmp/')) files.push(candidate); + } + } + }, + cleanup(): void { + files.splice(0).forEach((file) => { + try { + fs.unlinkSync(file); + } catch { + // Never created, or already gone. + } + }); + // Random per-run name, so a recursive remove here cannot reach anything + // another worker owns. + dirs.splice(0).forEach((dir) => fs.rmSync(dir, { recursive: true, force: true })); + }, + }; +} + +// Staged under fixed, stable names rather than per-run ones, so they can only +// be reclaimed when this suite is the run that created them. +const FIXED_TMP_STAGED_COPIES = [ + '/tmp/awf-docker-host-stage/etc/passwd', + '/tmp/awf-docker-host-stage/etc/group', + '/tmp/awf-docker-host-stage/identity/passwd', + '/tmp/awf-docker-host-stage/identity/group', +]; + describe('nested mountpoint preparation', () => { let tmpRoot: string; @@ -390,6 +463,9 @@ describe('nested mountpoint preparation', () => { }); describe('buildAgentVolumes (end-to-end topology)', () => { + // Narrowing /tmp makes the legacy init bind materialise the shared + // /tmp/awf-init mountpoint. That root is intentionally left in place; see + // makeFixedTmpArtifactTracker for why removing it is unsafe. function stageRunnerLayout() { const home = path.join(tmpRoot, 'home', 'runner'); const workspaceDir = path.join(home, 'work', 'repo', 'repo'); @@ -484,6 +560,9 @@ describe('nested mountpoint preparation', () => { describe('buildAgentVolumes (staged runner binary under --docker-host-path-prefix)', () => { const prefixRoots: string[] = []; const runnerBinPaths: string[] = []; + // This case narrows /tmp too, so it also materialises the fixed + // /tmp/awf-init and /tmp/awf-runner-bin mountpoints. + const fixedTmp = makeFixedTmpArtifactTracker(FIXED_TMP_STAGED_COPIES); afterEach(() => { // Staging and the /tmp cover are both real paths by construction: the @@ -491,6 +570,7 @@ describe('nested mountpoint preparation', () => { // hardcoded to /tmp. prefixRoots.splice(0).forEach((dir) => fs.rmSync(dir, { recursive: true, force: true })); runnerBinPaths.splice(0).forEach((file) => fs.rmSync(file, { force: true })); + fixedTmp.cleanup(); }); function stageSplitFsLayout() { @@ -522,6 +602,8 @@ describe('nested mountpoint preparation', () => { const binarySourcePath = path.join(binDir, binaryName); fs.writeFileSync(binarySourcePath, '#!/bin/sh\n', { mode: 0o755 }); + fixedTmp.noteBeforeBuild(); + const config = { agentCommand: binarySourcePath, allowedDomains: [], @@ -533,15 +615,19 @@ describe('nested mountpoint preparation', () => { return { binaryName, workspaceDir, - build: (filesystemAllowWrite?: string[]) => buildAgentVolumes({ - config: { ...config, filesystemAllowWrite } as WrapperConfig, - projectRoot: process.cwd(), - effectiveHome: home, - workspaceDir, - agentLogsPath, - sessionStatePath, - initSignalDir, - }), + build: (filesystemAllowWrite?: string[]) => { + const volumes = buildAgentVolumes({ + config: { ...config, filesystemAllowWrite } as WrapperConfig, + projectRoot: process.cwd(), + effectiveHome: home, + workspaceDir, + agentLogsPath, + sessionStatePath, + initSignalDir, + }); + fixedTmp.trackGenerated(volumes, binaryName); + return volumes; + }, }; } @@ -578,16 +664,19 @@ describe('nested mountpoint preparation', () => { }); }); - // A `--docker-host-path-prefix` under /tmp is *shared*, not daemon-only: the - // runner and the daemon see the same paths there, which is why AWF stages - // files into it with local fs calls and why prefix translation deliberately - // leaves an already-/tmp source unrewritten. The run's own workDir then sits - // *inside* the prefix, so topology passes still have to resolve it locally. + // A `--docker-host-path-prefix` of exactly /tmp is *shared*: the runner and + // the daemon see the same paths there, which is why prefix translation leaves + // an already-/tmp source unrewritten. The run's own workDir then sits *inside* + // the prefix, so topology passes still have to resolve it locally. (A /tmp + // *descendant* such as /tmp/gh-aw is daemon-only and must keep failing + // closed — covered in mount-topology.test.ts.) describe('buildAgentVolumes (--docker-host-path-prefix shares /tmp with the runner)', () => { const sharedRoots: string[] = []; + const fixedTmp = makeFixedTmpArtifactTracker(FIXED_TMP_STAGED_COPIES); afterEach(() => { sharedRoots.splice(0).forEach((dir) => fs.rmSync(dir, { recursive: true, force: true })); + fixedTmp.cleanup(); }); function stageSharedTmpLayout() { @@ -605,8 +694,9 @@ describe('nested mountpoint preparation', () => { const agentLogsPath = path.join(workDir, 'agent-logs'); const sessionStatePath = path.join(workDir, 'agent-session-state'); const initSignalDir = path.join(workDir, 'init-signal'); + const binDir = path.join(sharedRoot, 'runner-bin'); - [workspaceDir, workDir, emptyHome, agentLogsPath, sessionStatePath, initSignalDir] + [workspaceDir, workDir, emptyHome, agentLogsPath, sessionStatePath, initSignalDir, binDir] .forEach((dir) => fs.mkdirSync(dir, { recursive: true })); for (const toolPath of HOME_TOOL_PATHS) { fs.mkdirSync(path.join(home, toolPath), { recursive: true }); @@ -614,8 +704,18 @@ describe('nested mountpoint preparation', () => { } fs.mkdirSync(path.join(emptyHome, 'work', 'repo', 'repo'), { recursive: true }); + // A /tmp-rooted prefix also turns on ARC/DinD binary staging, which + // publishes to fixed paths named after the command. A unique name keeps + // this run's artefacts distinguishable from a concurrent suite's, and + // means they can never be mistaken for pre-existing ones. + const binaryName = `awfshared${unique}`; + const binarySourcePath = path.join(binDir, binaryName); + fs.writeFileSync(binarySourcePath, '#!/bin/sh\n', { mode: 0o755 }); + + fixedTmp.noteBeforeBuild(); + const config = { - agentCommand: 'echo', + agentCommand: binarySourcePath, allowedDomains: [], workDir, volumeMounts: [], @@ -625,15 +725,19 @@ describe('nested mountpoint preparation', () => { return { workspaceDir, initSignalDir, - build: (filesystemAllowWrite?: string[]) => buildAgentVolumes({ - config: { ...config, filesystemAllowWrite } as WrapperConfig, - projectRoot: process.cwd(), - effectiveHome: home, - workspaceDir, - agentLogsPath, - sessionStatePath, - initSignalDir, - }), + build: (filesystemAllowWrite?: string[]) => { + const volumes = buildAgentVolumes({ + config: { ...config, filesystemAllowWrite } as WrapperConfig, + projectRoot: process.cwd(), + effectiveHome: home, + workspaceDir, + agentLogsPath, + sessionStatePath, + initSignalDir, + }); + fixedTmp.trackGenerated(volumes, binaryName); + return volumes; + }, }; } @@ -643,6 +747,16 @@ describe('nested mountpoint preparation', () => { return writable; } + /** + * Plans against the resolver the production pipeline actually builds for + * this config. `planNestedMountpoints` defaults to an identity resolver, + * which would resolve every source regardless of prefix handling and so + * could not detect a regression in `createLocalSourceResolver` at all. + */ + function planWithProductionResolver(volumes: string[]) { + return planNestedMountpoints(volumes, createLocalSourceResolver(new Map(), '/tmp')); + } + it('resolves a workDir nested inside the prefix instead of failing closed', () => { const layout = stageSharedTmpLayout(); const writable = stageWritable(layout); @@ -654,23 +768,24 @@ describe('nested mountpoint preparation', () => { const layout = stageSharedTmpLayout(); const volumes = layout.build([stageWritable(layout)]); - const legacy = planNestedMountpoints(volumes) + const legacy = planWithProductionResolver(volumes) .find((requirement) => requirement.containerTarget === '/tmp/awf-init'); // The source is the run's own init-signal directory, which the CLI just // created locally — being under the shared prefix must not make it - // unclassifiable. + // unclassifiable. Asserted on the plan rather than on disk: a + // /tmp/awf-init left behind by another suite would otherwise satisfy an + // existsSync check without this code path ever running. expect(legacy?.source).toBe(layout.initSignalDir); expect(legacy?.kind).toBe('directory'); - expect(legacy?.hostPath).toBeDefined(); - expect(fs.existsSync(legacy?.hostPath as string)).toBe(true); + expect(legacy?.hostPath).toBe('/tmp/awf-init'); }); it('prepares the nested home mountpoints when the chroot home is under the prefix', () => { const layout = stageSharedTmpLayout(); const volumes = layout.build([stageWritable(layout)]); - const nestedHomeMounts = planNestedMountpoints(volumes) + const nestedHomeMounts = planWithProductionResolver(volumes) .filter((requirement) => requirement.containerTarget.includes('/.copilot/')); // Guards against passing vacuously: the policy really does narrow a home @@ -683,5 +798,14 @@ describe('nested mountpoint preparation', () => { expect(fs.existsSync(requirement.hostPath as string)).toBe(true); } }); + + it('leaves no unsatisfiable mountpoint for runc', () => { + const layout = stageSharedTmpLayout(); + const volumes = layout.build([stageWritable(layout)]); + + // Sources are runner-local here (a shared prefix is not rewritten), so + // the runc model applies directly to the generated list. + expect(simulateRuncMountFailures(volumes)).toEqual([]); + }); }); }); diff --git a/src/services/host-path-prefix.ts b/src/services/host-path-prefix.ts index 8d25f3498..be148634c 100644 --- a/src/services/host-path-prefix.ts +++ b/src/services/host-path-prefix.ts @@ -26,20 +26,44 @@ export function normalizeDockerHostPathPrefix(prefix: string): string { } /** - * Is this prefix a directory the runner and the Docker daemon *share*, rather - * than a daemon-only staging root? + * Is this prefix a directory the runner and the Docker daemon see at the *same + * path*, rather than a daemon-only view of the runner's filesystem? * - * A /tmp-rooted prefix is the ARC/DinD shared-volume shape: the same path - * resolves to the same bytes on both sides. That is why AWF can stage files - * into it with ordinary local `fs` calls, why an already-/tmp source is never - * rewritten below, and why a source under it stays runner-resolvable. A prefix - * like `/host` is the opposite: it exists only inside the daemon. + * Only the exact `/tmp` shape qualifies, and the reason is structural rather + * than conventional. AWF's own workDir (`/tmp/awf-`) then sits under the + * prefix, so `translateBindMountHostPath` leaves every one of its binds + * unrewritten — which can only work if the daemon resolves those paths to the + * same bytes. A prefix of `/tmp` is therefore a claim that /tmp is shared. * - * Exported because every pass that reasons about prefixed paths needs the same - * answer — keeping separate copies of this test is what let a shared prefix be - * mistaken for a daemon-only one. + * Descendants are the opposite. `/tmp/gh-aw` is one of `dind-probe`'s + * CANDIDATE_PREFIXES, and that loop runs *only* after the probe has confirmed + * the daemon cannot see the runner's filesystem: it means the daemon sees + * runner path `X` at `/tmp/gh-aw/X`. `buildCustomVolumeMounts` says the same, + * translating sources that already start with `/tmp/gh-aw`. Treating such a + * prefix as shared would hand back a daemon-namespace path as if it were + * runner-local, skipping the fail-closed guard that exists to prevent exactly + * that. + * + * Note this is deliberately narrower than `isTmpRootedDockerHostPathPrefix`, + * which gates staging-root selection. Those two questions look alike but are + * not the same, so they must not share an answer. */ export function isSharedDockerHostPathPrefix(prefix: string | undefined): boolean { + if (!prefix) return false; + return normalizeDockerHostPathPrefix(prefix) === '/tmp'; +} + +/** + * Does this prefix select the /tmp-rooted staging layout introduced for ARC and + * DinD (see docs/arc-dind.md)? + * + * This gates *where AWF stages* chroot prerequisites, not whether a path can be + * attributed to the runner, so it keeps its original `/tmp`-or-below meaning. + * It is intentionally not narrowed to the shared case: doing so would silently + * disable binary and /etc staging for `/tmp/gh-aw` runners, which is precisely + * the topology that feature was built for. + */ +export function isTmpRootedDockerHostPathPrefix(prefix: string | undefined): boolean { if (!prefix) return false; const normalized = normalizeDockerHostPathPrefix(prefix); return normalized === '/tmp' || normalized.startsWith('/tmp/'); @@ -47,7 +71,7 @@ export function isSharedDockerHostPathPrefix(prefix: string | undefined): boolea function shouldPreserveUnprefixedEtcIdentityFile(hostPath: string, dockerHostPathPrefix: string): boolean { return ( - isSharedDockerHostPathPrefix(dockerHostPathPrefix) && + isTmpRootedDockerHostPathPrefix(dockerHostPathPrefix) && (hostPath === '/etc/passwd' || hostPath === '/etc/group') ); } diff --git a/src/workdir-setup-branches.test.ts b/src/workdir-setup-branches.test.ts index 6964e23d8..b91dd86e6 100644 --- a/src/workdir-setup-branches.test.ts +++ b/src/workdir-setup-branches.test.ts @@ -144,6 +144,91 @@ describe('workdir-setup – createMissingOwnedDirectorySegments non-directory se workdirSetupTestHelpers.createMissingOwnedDirectorySegments(childPath, 1000, 1000) ).toThrow(`Expected directory but found non-directory path: ${fileSegment}`); }); + + it('skips an owner-preserving chown so non-root callers do not hit EPERM', () => { + // A freshly created directory already belongs to the caller, so chowning it + // back to the same owner changes nothing -- but macOS still denies that + // syscall to non-root, which used to abort mountpoint preparation. + const created = path.join(tempDir, 'nested', 'mountpoint'); + // Read the ownership the platform actually assigns to a new directory here + // rather than assuming the caller's ids: BSD (macOS) gives a new directory + // its parent's gid, so under /tmp that is wheel, not the caller's group. + const probe = path.join(tempDir, 'ownership-probe'); + fs.mkdirSync(probe); + const { uid, gid } = fs.statSync(probe); + + workdirSetupTestHelpers.createMissingOwnedDirectorySegments(created, uid, gid); + + expect(fs.existsSync(created)).toBe(true); + expect(fs.chownSync as unknown as jest.Mock).not.toHaveBeenCalled(); + }); + + it('still chowns when a privileged caller must change owner', () => { + const created = path.join(tempDir, 'other-owner'); + const foreignUid = (process.getuid?.() ?? 0) + 1; + const getuid = jest.spyOn(process, 'getuid').mockReturnValue(0); + + try { + workdirSetupTestHelpers.createMissingOwnedDirectorySegments(created, foreignUid, 0); + } finally { + getuid.mockRestore(); + } + + expect(fs.chownSync as unknown as jest.Mock).toHaveBeenCalledWith(created, foreignUid, 0); + }); + + it('tolerates an EPERM chown a non-root caller could never satisfy', () => { + // getSafeHostGid can map the caller onto a different gid (macOS maps a + // system gid into the regular range). Unprivileged AWF cannot grant a + // directory away, so this must not abort mountpoint preparation. + const created = path.join(tempDir, 'unprivileged'); + const foreignGid = (process.getgid?.() ?? 0) + 1000; + const getuid = jest.spyOn(process, 'getuid').mockReturnValue(501); + (fs.chownSync as unknown as jest.Mock).mockImplementation(() => { + const error = new Error('EPERM') as NodeJS.ErrnoException; + error.code = 'EPERM'; + throw error; + }); + + try { + workdirSetupTestHelpers.createMissingOwnedDirectorySegments(created, 501, foreignGid); + } finally { + getuid.mockRestore(); + } + + expect(fs.existsSync(created)).toBe(true); + }); + + it('still propagates a chown failure that is not a privilege limit', () => { + const created = path.join(tempDir, 'broken'); + (fs.chownSync as unknown as jest.Mock).mockImplementation(() => { + const error = new Error('EIO') as NodeJS.ErrnoException; + error.code = 'EIO'; + throw error; + }); + + expect(() => + workdirSetupTestHelpers.createMissingOwnedDirectorySegments(created, 4242, 4242) + ).toThrow('EIO'); + }); + + it('still propagates an EPERM chown when running privileged', () => { + const created = path.join(tempDir, 'privileged-eperm'); + const getuid = jest.spyOn(process, 'getuid').mockReturnValue(0); + (fs.chownSync as unknown as jest.Mock).mockImplementation(() => { + const error = new Error('EPERM') as NodeJS.ErrnoException; + error.code = 'EPERM'; + throw error; + }); + + try { + expect(() => + workdirSetupTestHelpers.createMissingOwnedDirectorySegments(created, 4242, 4242) + ).toThrow('EPERM'); + } finally { + getuid.mockRestore(); + } + }); }); describe('workdir-setup – prepareLogDirectories mcp-logs already-exists branch (lines 194-195)', () => { From 4af3832ce75d428bc75f029974b6e676575eb348 Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 23:33:19 -0700 Subject: [PATCH 17/18] test: assert prepared mountpoints instead of modelling runc on daemon paths A shared /tmp prefix still rewrites every source that was not already under /tmp, so on a GitHub runner /opt/hostedtoolcache becomes /tmp/opt/hostedtoolcache: a path only the daemon can resolve. The runner-local runc model therefore reported it as an unsatisfiable mountpoint. macOS has no hostedtoolcache, so this only failed in CI. Assert instead that every mountpoint the plan demands was classified and actually materialised, which is the property runc depends on and does not vary by runner. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/nested-mountpoints.test.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index 5057ce468..1d1f6d696 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -802,10 +802,20 @@ describe('nested mountpoint preparation', () => { it('leaves no unsatisfiable mountpoint for runc', () => { const layout = stageSharedTmpLayout(); const volumes = layout.build([stageWritable(layout)]); - - // Sources are runner-local here (a shared prefix is not rewritten), so - // the runc model applies directly to the generated list. - expect(simulateRuncMountFailures(volumes)).toEqual([]); + const requirements = planWithProductionResolver(volumes); + + // Not simulateRuncMountFailures: the prefix rewrites every source that + // was not already under /tmp, so on a GitHub runner /opt/hostedtoolcache + // becomes /tmp/opt/hostedtoolcache -- a path only the daemon can see, and + // one a runner-local model would wrongly report as missing. Assert + // instead that every mountpoint the plan demands was actually + // materialised, which is what runc needs and is platform-independent. + expect(requirements.length).toBeGreaterThan(0); + for (const requirement of requirements) { + expect(requirement.kind).not.toBe('unknown'); + expect(requirement.hostPath).toBeDefined(); + expect(fs.existsSync(requirement.hostPath as string)).toBe(true); + } }); }); }); From 43d47a22354ed6871fd28c3380199d7a0c733e5f Mon Sep 17 00:00:00 2001 From: Lawrence Cox Date: Sun, 23 Aug 2026 23:44:03 -0700 Subject: [PATCH 18/18] test: scope the shared /tmp assertions to prefix-invariant mounts ensureNestedMountpoints runs on the topology before the host path prefix is applied, so re-planning the returned list is only faithful for paths the prefix leaves alone. On a GitHub runner /opt/hostedtoolcache is rewritten to the daemon-only /tmp/opt/hostedtoolcache, which no runner-local check can resolve, so assert over the mounts landing in the guest's /tmp instead. Reverting the shared-prefix exemption still fails these assertions. Create the shared root with mkdtemp rather than a predictable /tmp name, so nothing under it can be pre-created or redirected by another user on a shared machine. The prefix under test is the literal /tmp, so the root itself still cannot move into the suite's sandbox. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- .../agent-volumes/nested-mountpoints.test.ts | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/services/agent-volumes/nested-mountpoints.test.ts b/src/services/agent-volumes/nested-mountpoints.test.ts index 1d1f6d696..e736c592c 100644 --- a/src/services/agent-volumes/nested-mountpoints.test.ts +++ b/src/services/agent-volumes/nested-mountpoints.test.ts @@ -680,12 +680,14 @@ describe('nested mountpoint preparation', () => { }); function stageSharedTmpLayout() { - const unique = path.basename(tmpRoot).replace(/[^a-zA-Z0-9]/g, ''); // The prefix under test is the literal /tmp, and the whole run has to sit // beneath it, so this cannot be redirected into the suite's sandbox (on - // macOS os.tmpdir() is /private/var/... and would not nest). - const sharedRoot = `/tmp/awf-shared-${unique}`; + // macOS os.tmpdir() is /private/var/... and would not nest). mkdtemp + // rather than a predictable name, so nothing here can be pre-created or + // redirected by another user on a shared machine. + const sharedRoot = fs.mkdtempSync('/tmp/awf-shared-'); sharedRoots.push(sharedRoot); + const unique = path.basename(sharedRoot).replace(/[^a-zA-Z0-9]/g, ''); const home = path.join(sharedRoot, 'home', 'runner'); const workspaceDir = path.join(home, 'work', 'repo', 'repo'); @@ -804,14 +806,18 @@ describe('nested mountpoint preparation', () => { const volumes = layout.build([stageWritable(layout)]); const requirements = planWithProductionResolver(volumes); - // Not simulateRuncMountFailures: the prefix rewrites every source that - // was not already under /tmp, so on a GitHub runner /opt/hostedtoolcache - // becomes /tmp/opt/hostedtoolcache -- a path only the daemon can see, and - // one a runner-local model would wrongly report as missing. Assert - // instead that every mountpoint the plan demands was actually - // materialised, which is what runc needs and is platform-independent. - expect(requirements.length).toBeGreaterThan(0); - for (const requirement of requirements) { + // Only the mounts landing inside the guest's /tmp are asserted here. + // ensureNestedMountpoints runs on the topology *before* the prefix is + // applied, so re-planning the returned list is only faithful where the + // prefix changes nothing -- which is exactly the already-/tmp-rooted + // paths this case is about. Anything else is rewritten: on a GitHub + // runner /opt/hostedtoolcache becomes the daemon-only + // /tmp/opt/hostedtoolcache, which no runner-local check can resolve. + const guestTmpMounts = requirements.filter((requirement) => + requirement.containerTarget.startsWith('/tmp/')); + + expect(guestTmpMounts.length).toBeGreaterThan(0); + for (const requirement of guestTmpMounts) { expect(requirement.kind).not.toBe('unknown'); expect(requirement.hostPath).toBeDefined(); expect(fs.existsSync(requirement.hostPath as string)).toBe(true);