-
Notifications
You must be signed in to change notification settings - Fork 2
feat(docker): propagate host.docker.internal DNS to spawned containers #210
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
When --enable-host-access is used, containers spawned by the agent via docker run now also get host.docker.internal DNS resolution. Changes: - Pass AWF_ENABLE_HOST_ACCESS env var to agent container - Update docker-wrapper.sh to: - Allow --add-host host.docker.internal:host-gateway when enabled - Inject --add-host to spawned containers when enabled - Add unit tests for the new environment variable - Update documentation with new "How It Works" section Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Address code review feedback: - Extract validate_add_host() function to reduce duplication - Track has_host_docker_internal separately from has_add_host - Remove extra blank line for consistent formatting Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
Security Review: PR #210I've reviewed this PR for security implications. Overall, the changes are well-designed with appropriate security controls, but there is one security concern that should be addressed:
|
|
@copilot please address the comments from the security guard #210 (comment) |
Address security concern from Security Guard review. Make the AWF_ENABLE_HOST_ACCESS environment variable readonly in entrypoint.sh to prevent malicious code from enabling host access when it wasn't authorized by the user via --enable-host-access flag. The attack scenario this prevents: 1. User runs awf WITHOUT --enable-host-access 2. Malicious code sets: export AWF_ENABLE_HOST_ACCESS=true 3. Malicious code spawns container with --add-host host.docker.internal:host-gateway 4. Previously this would bypass the restriction; now it's blocked because the readonly variable cannot be modified by user code. Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Fixed in commit a27a2f8. Made readonly AWF_ENABLE_HOST_ACCESS="${AWF_ENABLE_HOST_ACCESS:-false}"
export AWF_ENABLE_HOST_ACCESSThis prevents the attack scenario where malicious code could |
🔒 Security Review: Critical Vulnerability FoundAfter reviewing this PR, I've identified a critical security vulnerability that must be fixed before merging. ❌ Critical Issue: Readonly Protection Can Be Bypassed in SubshellsLocation: The Problem: The readonly AWF_ENABLE_HOST_ACCESS="${AWF_ENABLE_HOST_ACCESS:-false}"
export AWF_ENABLE_HOST_ACCESSWhy this is vulnerable: Bash's Proof of Concept Attack: # Attacker's malicious code inside the agent container:
bash -c 'AWF_ENABLE_HOST_ACCESS=true docker run alpine ping host.docker.internal'This works because:
✅ Recommended FixReplace the environment variable check with a filesystem-based check: In # Make host access configuration immutable via filesystem
# AWF_ENABLE_HOST_ACCESS controls host.docker.internal access in docker-wrapper.sh
if [ "$AWF_ENABLE_HOST_ACCESS" = "true" ]; then
mkdir -p /etc/awf
touch /etc/awf/host-access-enabled
chmod 444 /etc/awf/host-access-enabled
chown root:root /etc/awf/host-access-enabled
echo "[entrypoint] Host access enabled (file marker created at /etc/awf/host-access-enabled)"
else
echo "[entrypoint] Host access disabled"
fiIn validate_add_host() {
local value="$1"
# Check for immutable file marker instead of environment variable
if [[ "$value" == "host.docker.internal:host-gateway" ]] && [ -f "/etc/awf/host-access-enabled" ]; then
return 0 # Allowed
fi
return 1 # Blocked
}Also update line 118: if [ -f "/etc/awf/host-access-enabled" ] && [ "$has_host_docker_internal" = false ]; thenThis is secure because:
Additional NotesThe rest of the PR implementation looks good:
Please apply the recommended fix to close this security vulnerability.
|
…ker-host-access # Conflicts: # containers/agent/docker-wrapper.sh
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
Smoke Test Results (Claude)Last 2 Merged PRs:
Test Results:
Overall: PASS
|
Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Status: FAIL (Playwright timeout) cc: @Mossaka
|
|
Closing PR to reduce backlog. Created issue #422 to track this work. Will fix it later. |
--enable-host-accessis used, spawned containers via docker-wrapper needhost.docker.internalDNS resolutionAWF_ENABLE_HOST_ACCESSenvironment variable to agent container--add-host host.docker.internal:host-gatewayspecifically whenAWF_ENABLE_HOST_ACCESS=true--add-host host.docker.internal:host-gatewayto spawned containers when enabledAWF_ENABLE_HOST_ACCESSreadonly in entrypoint.sh to prevent malicious code from tampering with the variableSecurity Summary: Fixed environment variable tampering vulnerability identified by Security Guard. The
AWF_ENABLE_HOST_ACCESSvariable is now declared readonly in entrypoint.sh, preventing malicious code from enabling host access when it wasn't authorized.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.