Skip to content

fix(docker): simplify security model - remove direct DNS exceptions#1246

Open
Mossaka wants to merge 3 commits intomainfrom
fix/011-simplify-security-model
Open

fix(docker): simplify security model - remove direct DNS exceptions#1246
Mossaka wants to merge 3 commits intomainfrom
fix/011-simplify-security-model

Conversation

@Mossaka
Copy link
Collaborator

@Mossaka Mossaka commented Mar 11, 2026

Summary

  • Removes explicit DNS server exception rules from container-level iptables (setup-iptables.sh) and host-level iptables (host-iptables.ts)
  • DNS is now handled exclusively by Docker's embedded DNS (127.0.0.11), which forwards to upstream servers configured via Docker's dns: field
  • Eliminates AWF_DNS_SERVERS environment variable from the agent container
  • Simplifies the entrypoint DNS config to use Docker embedded DNS only

Security Model (Before → After)

Before: localhost + Squid + DNS servers (8.8.8.8, 8.8.4.4) + Docker DNS
After: localhost + Squid proxy only (Docker embedded DNS covered by localhost rules)

This prevents DNS-based data exfiltration since containers can no longer directly query external DNS servers. Docker's embedded DNS at 127.0.0.11 is already covered by the localhost exception (127.0.0.0/8), and it handles forwarding to upstream DNS servers at the Docker daemon level.

Test plan

  • Build passes (npm run build)
  • All 855 tests pass (npm test)
  • Lint passes (npm run lint)
  • CI integration tests verify DNS resolution still works via Docker embedded DNS
  • Verify Squid proxy can resolve allowed domains
  • Verify DNS exfiltration to external servers is blocked

Fixes #11

🤖 Generated with Claude Code

Remove explicit DNS server exception rules from both container-level
iptables (setup-iptables.sh) and host-level iptables (host-iptables.ts).

Docker's embedded DNS (127.0.0.11) handles all DNS resolution,
forwarding to upstream servers configured via Docker's dns: field.
This is already covered by the localhost rules (127.0.0.0/8 RETURN),
so no separate DNS exceptions are needed.

Benefits:
- Simpler iptables rules with fewer exceptions to audit
- Prevents DNS-based data exfiltration (no direct external DNS)
- Single security boundary: Squid domain allowlist + localhost
- Container can only talk to: localhost, Squid proxy, API proxy

Fixes #11

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 11, 2026 19:14
@github-actions
Copy link
Contributor

⚠️ Coverage Regression Detected

This PR decreases test coverage. Please add tests to maintain coverage levels.

Overall Coverage

Metric Base PR Delta
Lines 82.50% 82.50% ➡️ +0.00%
Statements 82.50% 82.47% 📉 -0.03%
Functions 82.69% 82.52% 📉 -0.17%
Branches 74.78% 74.82% 📈 +0.04%
📁 Per-file Coverage Changes (2 files)
File Lines (Before → After) Statements (Before → After)
src/host-iptables.ts 81.0% → 79.5% (-1.49%) 81.2% → 79.5% (-1.66%)
src/docker-manager.ts 84.0% → 84.4% (+0.48%) 83.3% → 83.8% (+0.46%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

@Mossaka Mossaka changed the title fix(security): simplify security model - remove direct DNS exceptions fix(docker): simplify security model - remove direct DNS exceptions Mar 11, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies the firewall/DNS security model by removing explicit “trusted DNS server” exception rules and relying on Docker’s embedded DNS resolver (127.0.0.11) with upstream resolvers set via the generated docker-compose dns: field, while also removing the container env wiring (AWF_DNS_SERVERS).

Changes:

  • Remove host-level and container-level iptables DNS exception rules; keep a default-deny posture with egress primarily via Squid (and optional API proxy sidecar).
  • Remove AWF_DNS_SERVERS propagation and update docker-compose generation/tests to configure upstream DNS via dns:.
  • Update agent entrypoint DNS configuration to use only Docker embedded DNS (127.0.0.11) and adjust unit tests accordingly.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/host-iptables.ts Removes DNS exception rules and updates IPv6 handling/docs for the simplified egress model.
src/host-iptables.test.ts Updates tests to reflect removal of DNS-specific iptables rules and new IPv6 behavior.
src/docker-manager.ts Stops passing AWF_DNS_SERVERS and configures upstream DNS via compose dns:.
src/docker-manager.test.ts Updates tests to assert dns: usage and absence of AWF_DNS_SERVERS.
src/cli-workflow.ts Updates workflow dependency signature/call to match new setupHostIptables parameters.
containers/agent/setup-iptables.sh Removes DNS allowlisting/exception logic; relies on localhost coverage for 127.0.0.11.
containers/agent/entrypoint.sh Writes resolv.conf with only 127.0.0.11 and removes env-driven external nameservers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +324 to +325
// This simplifies the security model by eliminating direct DNS exceptions,
// preventing DNS-based data exfiltration.
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment claims removing direct DNS exceptions “prevents DNS-based data exfiltration”, but the agent/container can still exfiltrate data via DNS query names sent to Docker’s embedded resolver (127.0.0.11), which will forward queries to the configured upstream resolvers. Consider rewording to the narrower guarantee (e.g., prevents choosing arbitrary external DNS servers) and/or explicitly blocking DNS queries from the agent if DNS exfiltration is in scope.

Suggested change
// This simplifies the security model by eliminating direct DNS exceptions,
// preventing DNS-based data exfiltration.
// This simplifies the security model by eliminating direct DNS exceptions and
// prevents the container from selecting arbitrary external DNS servers. Note:
// DNS-based data exfiltration via Docker's embedded resolver is still possible
// unless additional DNS restrictions are applied.

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +80
# DNS: All DNS queries go through Docker's embedded DNS server (127.0.0.11),
# which is already covered by the localhost rules above (127.0.0.0/8 RETURN).
# Docker's embedded DNS forwards to the upstream DNS servers configured via
# the 'dns:' field in docker-compose.yml. This simplifies the security model
# by eliminating direct DNS exceptions - no container can talk to external
# DNS servers directly, preventing DNS-based data exfiltration.
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment says eliminating direct DNS exceptions prevents DNS-based exfiltration, but the container can still send arbitrary DNS queries to 127.0.0.11 and have them forwarded upstream (data can be encoded in query names). Suggest rewording to reflect the actual security property (no direct access to arbitrary DNS servers) or add enforcement that blocks DNS from the agent entirely if required.

Suggested change
# DNS: All DNS queries go through Docker's embedded DNS server (127.0.0.11),
# which is already covered by the localhost rules above (127.0.0.0/8 RETURN).
# Docker's embedded DNS forwards to the upstream DNS servers configured via
# the 'dns:' field in docker-compose.yml. This simplifies the security model
# by eliminating direct DNS exceptions - no container can talk to external
# DNS servers directly, preventing DNS-based data exfiltration.
# DNS: All DNS queries are expected to go through Docker's embedded DNS server (127.0.0.11),
# which is already covered by the localhost rules above (127.0.0.0/8 RETURN).
# Docker's embedded DNS forwards to the upstream DNS servers configured via
# the 'dns:' field in docker-compose.yml (or Docker defaults). This simplifies the
# security model by eliminating direct DNS exceptions: containers cannot talk directly
# to arbitrary external DNS servers, and all DNS egress is mediated by Docker's resolver.

Copilot uses AI. Check for mistakes.
# Docker's embedded DNS handles both container name resolution (squid-proxy, etc.)
# and external domain resolution (forwarding to upstream DNS servers configured
# via Docker's dns: field in docker-compose.yml). No direct external DNS access
# is needed, which prevents DNS-based data exfiltration.
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment asserts that using only Docker embedded DNS “prevents DNS-based data exfiltration”, but DNS queries to 127.0.0.11 can still carry data in the requested names and will be forwarded upstream. Please adjust the wording to the narrower guarantee (no direct external DNS access) or add controls to disallow DNS queries from the agent if DNS exfil is part of the threat model.

Suggested change
# is needed, which prevents DNS-based data exfiltration.
# is needed; DNS queries are mediated via Docker's embedded DNS.

Copilot uses AI. Check for mistakes.
// Pass DNS servers to container for setup-iptables.sh and entrypoint.sh
const dnsServers = config.dnsServers || ['8.8.8.8', '8.8.4.4'];
environment.AWF_DNS_SERVERS = dnsServers.join(',');
// DNS servers are configured via Docker's dns: field in docker-compose.yml (line ~824).
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment references a specific source line number ("line ~824") in docker-compose output, which is brittle and will drift as the file evolves. Consider removing the line-number reference and just describe that DNS servers are configured via the generated compose service’s dns: field.

Suggested change
// DNS servers are configured via Docker's dns: field in docker-compose.yml (line ~824).
// DNS servers are configured via the generated Docker Compose service's dns: field.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Contributor

Smoke test results for run 22970074430:

  • GitHub MCP: Last 2 merged PRs: #1243 feat(cli): add --memory-limit flag | #1240 test(docker): verify capsh execution chain (both by @Mossaka)
  • Playwright: github.com title contains "GitHub"
  • File Write: /tmp/gh-aw/agent/smoke-test-copilot-22970074430.txt created
  • Bash: File verified via cat

Overall: PASS

📰 BREAKING: Report filed by Smoke Copilot for issue #1246

@github-actions
Copy link
Contributor

Smoke Test Results

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude for issue #1246

@github-actions
Copy link
Contributor

feat(cli): add --memory-limit flag for configurable container memory
test: expand credential hiding tests to all 14 protected paths
GitHub MCP merged PRs review: ✅
safeinputs-gh PR list: ✅
Playwright title check: ✅
Tavily search: ❌ (tool unavailable)
File write + cat: ✅
Discussion comment: ✅
Build (npm ci && npm run build): ✅
Overall: FAIL

🔮 The oracle has spoken through Smoke Codex for issue #1246

@github-actions
Copy link
Contributor

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.12 Python 3.12.3
Node.js v24.14.0 v20.20.0
Go go1.22.12 go1.22.12

Result: ❌ FAILED — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot for issue #1246

@github-actions

This comment has been minimized.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Contributor

⚠️ Coverage Regression Detected

This PR decreases test coverage. Please add tests to maintain coverage levels.

Overall Coverage

Metric Base PR Delta
Lines 82.50% 83.87% 📈 +1.37%
Statements 82.50% 83.81% 📈 +1.31%
Functions 82.69% 82.60% 📉 -0.09%
Branches 74.78% 76.08% 📈 +1.30%
📁 Per-file Coverage Changes (3 files)
File Lines (Before → After) Statements (Before → After)
src/docker-manager.ts 84.0% → 84.4% (+0.48%) 83.3% → 83.8% (+0.46%)
src/cli.ts 47.0% → 47.8% (+0.72%) 47.5% → 48.1% (+0.69%)
src/host-iptables.ts 81.0% → 95.0% (+14.01%) 81.2% → 95.0% (+13.84%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

@github-actions
Copy link
Contributor

Smoke test results — run 22970451788

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude for issue #1246

@github-actions
Copy link
Contributor

Smoke Test Results ✅ PASS

Test Result
GitHub MCP (last 2 merged PRs) #1243 feat(cli): add --memory-limit flag · #1240 test(docker): verify capsh execution chain
Playwright (github.com title) ✅ "GitHub · Change is constant..."
File write /tmp/gh-aw/agent/smoke-test-copilot-22970451815.txt
Bash verification (cat)

PR author: @Mossaka

📰 BREAKING: Report filed by Smoke Copilot for issue #1246

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Contributor

GitHub MCP merged PRs: ✅ feat(cli): add --memory-limit flag for configurable container memory; ✅ test: expand credential hiding tests to all 14 protected paths
safeinputs-gh PR list: ✅ fix(docker): simplify security model - remove direct DNS exceptions; ✅ feat(cli): add predownload command to pre-pull container images
Playwright title check: ✅
Tavily search: ❌ (tool unavailable)
File write: ✅
Bash cat: ✅
Discussion comment: ✅ (1227)
Build (npm ci && npm run build): ✅
Overall: FAIL

🔮 The oracle has spoken through Smoke Codex for issue #1246

@github-actions
Copy link
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 82.50% 84.06% 📈 +1.56%
Statements 82.50% 83.99% 📈 +1.49%
Functions 82.69% 83.09% 📈 +0.40%
Branches 74.78% 76.08% 📈 +1.30%
📁 Per-file Coverage Changes (4 files)
File Lines (Before → After) Statements (Before → After)
src/docker-manager.ts 84.0% → 84.4% (+0.48%) 83.3% → 83.8% (+0.46%)
src/cli.ts 47.0% → 47.8% (+0.72%) 47.5% → 48.1% (+0.69%)
src/ssl-bump.ts 90.5% → 94.3% (+3.81%) 90.7% → 94.4% (+3.74%)
src/host-iptables.ts 81.0% → 95.0% (+14.01%) 81.2% → 95.0% (+13.84%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

@github-actions
Copy link
Contributor

Smoke Test Results — PASS

✅ GitHub MCP: #1243 feat(cli): add --memory-limit flag for configurable container memory; #1163 test: expand credential hiding tests to all 14 protected paths
✅ Playwright: github.com title contains "GitHub"
✅ File write: smoke-test-claude-22970656034.txt created
✅ Bash verify: file content confirmed

💥 [THE END] — Illustrated by Smoke Claude for issue #1246

@github-actions
Copy link
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia N/A ❌ FAIL
Bun hono N/A ❌ FAIL
C++ fmt N/A ❌ FAIL
C++ json N/A ❌ FAIL
Deno oak N/A ❌ FAIL
Deno std N/A ❌ FAIL
.NET hello-world N/A ❌ FAIL
.NET json-parse N/A ❌ FAIL
Go color N/A ❌ FAIL
Go env N/A ❌ FAIL
Go uuid N/A ❌ FAIL
Java gson N/A ❌ FAIL
Java caffeine N/A ❌ FAIL
Node.js clsx N/A ❌ FAIL
Node.js execa N/A ❌ FAIL
Node.js p-limit N/A ❌ FAIL
Rust fd N/A ❌ FAIL
Rust zoxide N/A ❌ FAIL

Overall: 0/8 ecosystems passed — ❌ FAIL


❌ Errors

All repository clones failed. The gh CLI is not authenticated (GH_TOKEN environment variable is not set).

Error for all ecosystems:

gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable. Example:
  env:
    GH_TOKEN: ${{ github.token }}
Exit code: 4

This affected all 8 ecosystems: Bun, C++, Deno, .NET, Go, Java, Node.js, Rust.

Generated by Build Test Suite for issue #1246 ·

@github-actions
Copy link
Contributor

Smoke Test Results ✅ PASS

Author: @Mossaka | No assignees

📰 BREAKING: Report filed by Smoke Copilot for issue #1246

@github-actions
Copy link
Contributor

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.12 Python 3.12.3 ❌ NO
Node.js v24.14.0 v20.20.0 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Overall: ❌ Tests did not pass — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot for issue #1246

@github-actions
Copy link
Contributor

PR titles: feat(cli): add --memory-limit flag for configurable container memory; test: expand credential hiding tests to all 14 protected paths
GitHub MCP (last 2 merged PRs): ✅
Safeinputs gh pr list: ✅
Playwright title contains GitHub: ✅
Tavily search: ❌ (tool unavailable)
File write + bash cat: ✅
Discussion comment: ✅
Build (npm ci && npm run build): ✅
Overall: FAIL

🔮 The oracle has spoken through Smoke Codex for issue #1246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Simplify security model - reject all non-localhost IP traffic by default

2 participants