-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add configurable agent base image for GitHub Actions runner parity #214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.19% | 76.95% | 📉 -0.24% |
| Statements | 77.27% | 77.05% | 📉 -0.22% |
| Functions | 77.17% | 77.41% | 📈 +0.24% |
| Branches | 69.76% | 69.21% | 📉 -0.55% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
75.9% → 76.1% (+0.23%) | 75.2% → 75.5% (+0.23%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Security Review: Custom Base Image SupportI've reviewed PR #214 and identified security considerations regarding the custom base image feature. SummaryThis PR allows users to specify arbitrary Docker base images via Security Concerns1. Third-Party Image Trust (Medium Risk)Location: ARG BASE_IMAGE=ubuntu:22.04
FROM ${BASE_IMAGE}Issue: The PR documentation recommends third-party images (
Why this matters:
Mitigating factors (existing controls):
2. Documentation GuidanceLocation: Recommendation: The documentation should include stronger security warnings about custom base images: ### Security Considerations
**⚠️ IMPORTANT:** Custom base images introduce supply chain risk. When using third-party images:
1. **Verify image sources** - Only use images from trusted publishers
2. **Review image contents** - Understand what tools and configurations are included
3. **Pin specific versions** - Use image digests (e.g., `@sha256:...`) instead of tags to prevent tag manipulation
4. **Monitor for vulnerabilities** - Third-party images may not receive timely security updates
For maximum security, use the default `ubuntu:22.04` image. Custom base images are recommended only when:
- You trust the image publisher
- You've reviewed the image build process
- The benefits outweigh the supply chain risksSuggested Actions
VerdictThe PR does not introduce new security vulnerabilities in the firewall implementation itself - all existing security controls remain intact. However, it does introduce supply chain risk by recommending third-party images. This is an acceptable trade-off if:
Recommendation: Approve with request for enhanced security documentation.
|
Mossaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address the failing CI and the security review comments in this PR.
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Addressed in 39594c9. Security review feedback: Added "Security Considerations" section to CI failures (both unrelated to this PR):
|
The previous implementation only checked /usr/bin/docker specifically, which could fail when docker is installed in other locations like /usr/local/bin/docker (common in runner images). This caused the wrapper to create a symlink at /usr/bin/docker but fail to create docker-real, breaking the wrapper functionality. Changes: - Use 'command -v docker' to find docker regardless of installation path - Copy docker binary to docker-real from any location - Only remove /usr/bin/docker if that's where docker was found - Add explicit error handling for broken symlinks This ensures the wrapper works correctly with both minimal ubuntu:22.04 and GitHub Actions runner images that may have docker in different paths. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolved conflicts in containers/agent/Dockerfile: - Kept BASE_IMAGE argument for custom base image support - Combined Node.js installation approaches: check if v22 exists first (for runner images), otherwise install with verification from main - Adopted docker-stub.sh from main (Docker-in-Docker support removed in v0.9.1) - Removed Docker CLI installation and docker wrapper logic (no longer needed) Docker-in-Docker support was removed in main via PR #205, so the docker wrapper improvements from this branch are now obsolete. The BASE_IMAGE feature and Node.js handling improvements remain relevant for runner parity. All tests pass (551 tests).
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Overall Status: PASS
|
Security Review: PR #214I've completed a security review of this PR. Overall, the changes are acceptable from a security perspective, but I have one recommendation for improvement. ✅ Security Controls MaintainedAll critical security hardening remains intact:
✅ Good Security Practices
|
|
@copilot please take the suggestion from the security review and implement it |
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Implemented in 7b856e7. Added
If an invalid image is specified, the CLI exits with an error message explaining the approved options. |
Added more edge case tests for the validation function in 8976ec4 (5 additional test cases). The per-file coverage for both modified files ( The |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.19% | 76.95% | 📉 -0.24% |
| Statements | 77.27% | 77.05% | 📉 -0.22% |
| Functions | 77.17% | 77.41% | 📈 +0.24% |
| Branches | 69.76% | 69.21% | 📉 -0.55% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
75.9% → 76.1% (+0.23%) | 75.2% → 75.5% (+0.23%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
@copilot please bump up the test coverage: https://github.com/githubnext/gh-aw-firewall/actions/runs/21092298632/job/60683405279?pr=214 |
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Added more tests in c87c634:
Per-file coverage for modified files has increased (+0.38% for cli.ts, +0.23% for docker-manager.ts). The small overall regression (-0.24%) is due to the CLI integration code (lines 685-699) which uses |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.88% | 77.63% | 📉 -0.25% |
| Statements | 77.94% | 77.71% | 📉 -0.23% |
| Functions | 77.29% | 77.54% | 📈 +0.25% |
| Branches | 71.00% | 70.44% | 📉 -0.56% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
79.1% → 79.3% (+0.19%) | 78.4% → 78.6% (+0.19%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Security Review: Base Image Validation ScopeI've reviewed this PR for security implications and found one concern regarding the base image validation allowlist:
|
- Add gh (GitHub CLI) package to agent container - Add PATH manipulation to prefer system binaries over toolcache - Make user/group creation idempotent (check if exists first) - Add catthehacker act-XX.XX images to allowlist Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
Smoke Test Results (Run ID: 21269309104) Last 2 Merged PRs:
✅ GitHub MCP: Retrieved recent PRs Status: PASS cc: @Mossaka
|
Smoke Test Results (Claude)Last 2 merged PRs:
Test Results:
Overall: PASS
|
Cover the act image type in validation tests to fix coverage regression. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 77.88% | 77.63% | 📉 -0.25% |
| Statements | 77.94% | 77.71% | 📉 -0.23% |
| Functions | 77.29% | 77.54% | 📈 +0.25% |
| Branches | 71.00% | 70.44% | 📉 -0.56% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
79.1% → 79.3% (+0.19%) | 78.4% → 78.6% (+0.19%) |
src/cli.ts |
35.5% → 35.9% (+0.38%) | 35.5% → 36.2% (+0.62%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test Results ✅GitHub MCP: ✅
Playwright: ✅ Page title: "GitHub · Change is constant. GitHub keeps you ahead. · GitHub" Overall Status: PASS cc @Mossaka
|
|
Smoke Test Results - Claude Engine Last 2 Merged PRs:
Test Results:
Overall Status: PASS
|
|
🧪 Smoke Test Results (Copilot) Last 2 Merged PRs:
✅ GitHub MCP - Retrieved PR data Status: PASS 🎉 cc: @Mossaka
|
BASE_IMAGEbuild argument to the agent Dockerfile--agent-base-imageCLI option to allow specifying custom base image when using--build-local^v22\.)validateAgentBaseImage()function with allowlist of approved base image patternsCI Notes:
Security Summary:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.