feat: mount host filesystem read-only with isolate.sh fallback#439
feat: mount host filesystem read-only with isolate.sh fallback#439
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
81698d0 to
9b2c73d
Compare
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
✅ Coverage Check PassedOverall Coverage
Coverage comparison generated by |
|
Smoke Test Results ✅ GitHub MCP: #437 fix: address code review feedback for isolate.sh, #435 docs: update AGENTS.md with read-only mount and isolate.sh documentation Status: PASS
|
|
Smoke Test Results for Copilot Engine 🧪 Last 2 Merged PRs:
Test Results: Overall Status: PASS ✅ cc @Mossaka
|
🔒 Security Review: Critical Issue FoundThis PR aims to strengthen security by mounting the host filesystem as read-only, which is commendable. However, the implementation introduces a critical security vulnerability related to the ❌ Critical: chroot Escape Risk with awfuserFile: Issue: The
Vulnerable Code: # isolate.sh line 50
exec chroot /host "$COMMAND" "${@:2}"
# isolate.sh line 61
exec chroot /host "${BINDIR}/${COMMAND}" "${@:2}"Why this is critical:
Recommended fixes: Option 1 (Most Secure): Remove chroot entirely
Option 2 (Requires Rearchitecture): Use proper namespaces
Option 3 (Band-aid): Restrict what can be executed via chroot
✅ Positive Security ChangesThe following changes are security improvements:
RecommendationDo not merge this PR until the chroot escape vulnerability is addressed. The most pragmatic approach is Option 1 (remove chroot entirely and document container customization), as it provides the security benefits of read-only mounts without introducing new attack vectors. If host binary fallback is a hard requirement, consider Option 3 with a very restricted whitelist of specific binaries that have been audited for safety when run in a non-root chroot environment.
|
Chroot Mode Binary Version Test ResultsSummaryThe test reveals PATH configuration differences between the host setup step and the chroot environment, resulting in different binary versions being executed. Version Comparison
AnalysisRoot CauseThe host has multiple versions of Python and Node installed: Python versions available:
Node versions available:
PATH DifferenceHost (setup step):
Chroot (current PATH): The chroot PATH prioritizes hostedtoolcache versions, with newer versions first. Why Go MatchesGo version 1.24.12 is both:
Test Result❌ FAIL - Python and Node versions do not match host versions. Recommendations
|
|
@copilot-swe-agent review |
Remove the isolate.sh script which used non-root chroot for host binary fallback. As identified in the security review, non-root chroot is not a security boundary and can be escaped. Changes: - Remove containers/agent/isolate.sh - Update entrypoint.sh to run commands directly without isolate.sh - Update Dockerfile to not copy isolate.sh - Update AGENTS.md documentation The read-only host mount (/:/host:ro) is preserved as it provides security benefits. Users who need host binary access should use the --enable-chroot flag which provides proper isolation. Addresses: Security Guard review comment
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
Code Review ResultsFound a critical functional issue that prevents the feature from working as designed: ❌ CRITICAL:
|
|
Smoke Test: Claude Engine - PASS Last 2 merged PRs:
✅ GitHub MCP - retrieved PRs
|
Smoke Test: Verify Language Runtimes Match Host - CANNOT COMPLETEIssueThe test cannot be completed as designed because:
Environment AnalysisHost Versions (from
Current Container Versions (without chroot):
RecommendationsTo make this test work, restructure the workflow to: Option 1: Run test directly on GitHub Actions runner host (not inside AWF) - name: Test chroot mode
run: |
sudo node dist/cli.js --enable-chroot --allow-domains localhost -- python3 --version
sudo node dist/cli.js --enable-chroot --allow-domains localhost -- node --version
sudo node dist/cli.js --enable-chroot --allow-domains localhost -- go versionOption 2: Check if current AWF instance has chroot enabled
Option 3: Have workflow setup step run awf --enable-chroot and capture output for comparison Error DetailsAttempted to run: Result:
|
|
@copilot please merge origin/main |
…copilot/secure-host-filesystem-mount
Mossaka
left a comment
There was a problem hiding this comment.
Review: REQUEST_CHANGES
@copilot Please fix the following critical issues:
Critical Issue: Branch is Behind Main
Your branch is 4 commits behind main and would delete recent features if merged:
7c5d99f- Version bump to 0.13.43099a14- GOROOT/bin PATH fix for chroot mode5b1c63c---skip-pullflag feature (#493)
If merged, this PR would:
- Delete the
skipPulloption fromtypes.ts - Revert version number
- Remove the GOROOT/bin PATH fix
- Remove 47 test cases from
cli.test.ts
Required Fix
-
Rebase your branch on top of current
main:git fetch origin git rebase origin/main
-
Remove stale comment in
docker-manager.tsthat references the removedisolate.sh:// The isolate.sh script enables execution of host binaries via chroot when neededThis comment should be deleted since
isolate.shwas correctly removed. -
Re-run CI after rebasing to verify all tests pass.
The core security change (read-only host mount) is sound - just needs the rebase.
* feat(cli): add --skip-pull flag to use pre-downloaded images Add a new --skip-pull CLI flag that prevents Docker Compose from pulling images from the registry, allowing users to use pre-downloaded or cached images locally. This is useful for: - Air-gapped environments where registry access is unavailable - CI systems with pre-warmed image caches - Local development when images are already cached When --skip-pull is enabled, Docker Compose runs with --pull never. If the required images are not available locally, container startup will fail with a clear error message. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test: add tests for skipPull parameter in startContainers Add unit tests to verify: - --pull never is passed when skipPull is true - --pull never is not passed when skipPull is false Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address review comments for --skip-pull flag - Add validation to reject --skip-pull + --build-local combination since building images requires pulling base images - Add security warning when using --skip-pull to inform users about verifying image authenticity - Add documentation for --skip-pull in CLI reference: - Options Summary table entry - Detailed explanation with usage examples - Security caution about image verification - Note about incompatibility with --build-local Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test: add tests for validateSkipPullWithBuildLocal function Extract flag validation logic into a testable function and add comprehensive tests to improve coverage on the new --skip-pull validation code. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor: use validation function in CLI action handler Simplify the --skip-pull validation by using the extracted validateSkipPullWithBuildLocal function instead of inline checks. This reduces code duplication and improves coverage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test: improve test coverage for docker-manager - Add test for when removing existing containers fails (covers catch block) - Add tests for allowHostPorts option in generateDockerCompose Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test: improve coverage for skipPull and related features - Add test for container removal failure handling in startContainers - Add tests for allowHostPorts environment variable - Add tests for GOROOT/CARGO_HOME/JAVA_HOME passthrough in chroot mode These tests improve overall coverage from 82.15% to 82.37%, exceeding the baseline of 82.25%. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
When GOROOT is provided (via AWF_GOROOT), the entrypoint now prepends $GOROOT/bin to PATH before exporting GOROOT. This ensures the correct Go version is found when actions/setup-go installs a specific version. Previously, only GOROOT was exported without adding its bin directory to PATH. This caused the system Go (e.g., 1.24.12) to be found instead of the setup-go version (e.g., 1.25.0) because AWF_HOST_PATH might have wrong ordering due to sudo resetting PATH. This matches how JAVA_HOME and CARGO_HOME are handled - their bin directories are prepended to PATH. Fixes gh-aw smoke-copilot test failure: "Go 1.24.12 < required 1.25.0" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…de (#456) * Initial plan * fix: replace head with awk in setup-iptables for chroot mode Replace `head -n 1` with awk's NR==1 to avoid GLIBC version mismatch when running in chroot mode. In chroot mode, host binaries are mounted at /host/*, and setup-iptables.sh runs before the chroot happens. The host's head binary may require a newer GLIBC than available in the container (Ubuntu 22.04 has GLIBC 2.35, GitHub Actions runners have GLIBC 2.38+). This fixes the error: head: /lib/x86_64-linux-gnu/libc.so.6: version 'GLIBC_2.38' not found Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
* Initial plan * feat: allow empty allowDomains to block all network access Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> * test: add integration tests for empty domains (no network access) Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> * test: fix misleading test title for DNS behavior test Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> * ci: re-trigger workflow checks * chore: merge origin/main and fix integration test Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> Co-authored-by: Jiaxiao (mossaka) Zhou <duibao55328@gmail.com>
* Initial plan * feat: filter benign operational logs from Squid access.log - Add ACL and log_access directive to filter localhost healthcheck probes - Update log aggregator to skip transaction-end-before-headers entries - Add comprehensive tests for both changes Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Problem
Host filesystem mounted read-write (
/:/host:rw) allows accidental or malicious writes during command execution. Need security hardening while preserving ability to execute host binaries not present in container.Changes
Mount configuration
docker-manager.ts: Changed default mount from/:/host:rw→/:/host:roCommand isolation wrapper
isolate.shscript provides transparent host binary fallback:/hostmount → chroot to host if binary found thereIntegration
Dockerfile: Install isolate.sh to/usr/local/bin/entrypoint.sh: Wrap all user commands with isolate.sh automaticallyExample behavior:
Security model
set -e, POSIX-compliant path testsDocumentation
Updated
AGENTS.mdwith architecture details andtypes.tswith new default mount mode.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.