fix(docker): simplify to localhost+Squid-only iptables#1270
Conversation
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 84.23% | 84.23% | ➡️ +0.00% |
| Statements | 84.21% | 84.20% | ➡️ -0.01% |
| Functions | 84.37% | 84.16% | 📉 -0.21% |
| Branches | 77.09% | 77.31% | 📈 +0.22% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/host-iptables.ts |
81.0% → 78.4% (-2.55%) | 81.2% → 78.4% (-2.72%) |
src/docker-manager.ts |
86.8% → 87.3% (+0.50%) | 86.1% → 86.6% (+0.48%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Mossaka
changed the title
Smoke Test Results
Overall: PASS
|
|
Smoke test results for ✅ GitHub MCP: #1267 fix: drop -f from curl to avoid GitHub API rate-limit flakiness, #1265 fix: add missing formatItem and program imports in cli.test.ts Overall: PASS
|
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 84.23% | 85.66% | 📈 +1.43% |
| Statements | 84.21% | 85.59% | 📈 +1.38% |
| Functions | 84.37% | 84.16% | 📉 -0.21% |
| Branches | 77.09% | 78.53% | 📈 +1.44% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
86.8% → 87.3% (+0.50%) | 86.1% → 86.6% (+0.48%) |
src/host-iptables.ts |
81.0% → 97.6% (+16.61%) | 81.2% → 97.6% (+16.44%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Pull request overview
This PR simplifies the project’s network security model by removing DNS allowlisting/exception rules and relying on Docker’s embedded DNS (127.0.0.11) plus Squid-only egress, while keeping --dns-servers to configure Docker DNS forwarding and Squid’s resolver behavior.
Changes:
- Remove
AWF_DNS_SERVERSpropagation and DNS exception rules from host/container iptables setup, leaving a localhost + Squid-only model. - Add configurable Squid
dns_nameserversgeneration viaSquidConfig.dnsServers. - Update DNS integration tests to assert that direct DNS to external resolvers is blocked while embedded DNS resolution still works.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/dns-servers.test.ts | Updates integration coverage to match the embedded-DNS + “block direct external DNS” model. |
| src/types.ts | Extends SquidConfig with dnsServers to drive Squid DNS configuration. |
| src/squid-config.ts | Renders dns_nameservers based on optional dnsServers, falling back to defaults. |
| src/host-iptables.ts | Removes host-level DNS allowlisting rules and updates the host firewall model. |
| src/host-iptables.test.ts | Aligns unit tests with the simplified host iptables rule set. |
| src/docker-manager.ts | Stops setting AWF_DNS_SERVERS and passes dnsServers into Squid config generation. |
| src/docker-manager.test.ts | Updates tests to assert AWF_DNS_SERVERS is no longer set while dns: is still configured. |
| src/cli-workflow.ts | Updates workflow dependency signature and call for setupHostIptables (drops dnsServers arg). |
| containers/agent/setup-iptables.sh | Removes container-level DNS NAT/filter exceptions; relies on localhost rules + Squid routing. |
| containers/agent/entrypoint.sh | Writes resolv.conf with only 127.0.0.11 to enforce embedded DNS usage. |
Comments suppressed due to low confidence (1)
src/squid-config.ts:558
- This introduces configurable
dns_nameserversoutput, but squid-config tests currently only assert that the directive exists (not that customdnsServersare rendered correctly or that defaults apply when unset). Adding a couple of unit tests for the newdnsServersbehavior would help prevent regressions.
# DNS settings - Squid resolves all domains for HTTP/HTTPS traffic
dns_nameservers ${(dnsServers && dnsServers.length > 0) ? dnsServers.join(' ') : '8.8.8.8 8.8.4.4'}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # DNS settings - Squid resolves all domains for HTTP/HTTPS traffic | ||
| dns_nameservers ${(dnsServers && dnsServers.length > 0) ? dnsServers.join(' ') : '8.8.8.8 8.8.4.4'} |
| # Docker embedded DNS handles all name resolution: | ||
| # - Container names (e.g., squid-proxy) → resolved directly | ||
| # - External domains → forwarded to upstream DNS servers configured via docker-compose dns: field | ||
| # No external DNS servers are listed in resolv.conf to prevent DNS-based data exfiltration |
src/host-iptables.ts
Outdated
| // all name resolution. It forwards to upstream servers at the Docker daemon level, | ||
| // which bypasses container iptables entirely. Squid resolves DNS internally for | ||
| // all HTTP/HTTPS traffic. |
src/docker-manager.ts
Outdated
| environment.AWF_DNS_SERVERS = dnsServers.join(','); | ||
| // Note: AWF_DNS_SERVERS env var is no longer set. The container uses only Docker's | ||
| // embedded DNS (127.0.0.11), which forwards to upstream servers configured via the | ||
| // docker-compose dns: field. This prevents DNS-based data exfiltration. |
Smoke Test Results
Overall: PASS
|
Smoke Test Results ✅ PASSLast 2 merged PRs:
PR author:
|
|
fix(docker): simplify to localhost+Squid-only iptables
|
Chroot Version Comparison Results
Result: Not all versions match. Python and Node.js versions differ between host and chroot environments.
|
This comment has been minimized.
This comment has been minimized.
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 84.37% | 85.83% | 📈 +1.46% |
| Statements | 84.32% | 85.72% | 📈 +1.40% |
| Functions | 84.88% | 84.68% | 📉 -0.20% |
| Branches | 77.40% | 78.75% | 📈 +1.35% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
86.9% → 87.4% (+0.50%) | 86.2% → 86.7% (+0.48%) |
src/host-iptables.ts |
81.0% → 97.7% (+16.68%) | 81.2% → 97.7% (+16.51%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test Results✅ GitHub MCP — Last 2 merged PRs retrieved: #1264 feat(proxy): add GitHub Enterprise Cloud/Server support, #1267 fix: drop -f from curl to avoid GitHub API rate-limit flakiness Overall: PASS
|
Smoke Test Results ✅ PASS
Author:
|
|
Merged PRs: feat(proxy): add GitHub Enterprise Cloud/Server support with automatic endpoint detection | fix: drop -f from curl to avoid GitHub API rate-limit flakiness
|
Chroot Version Comparison Results
Overall: ❌ Not all versions match. Go matches, but Python and Node.js differ between host and chroot environments.
|
This comment has been minimized.
This comment has been minimized.
Smoke Test Results
Overall: PASS
|
|
Smoke test results for
Overall: PASS
|
|
PRs (merged) titles: feat(ci): add weekly performance monitoring workflow; fix(squid): run Squid container as non-root proxy user
|
Chroot Version Comparison Results
Overall: ❌ Not all runtimes match — Python and Node.js versions differ between host and chroot.
|
Mossaka
force-pushed
the
fix/011-simplify-security-model
branch
from
be6e994 to
d9c1ce6
Compare
|
Smoke test results
|
Smoke Test Results
Overall: PASS
|
Mossaka
force-pushed
the
fix/011-simplify-security-model
branch
from
ae2406f to
8770717
Compare
Smoke Test Results
Overall: PASS
|
|
GitHub MCP merged PRs: ✅ feat(docker): switch seccomp profile to deny-by-default; feat: add DLP scanning to block credential exfiltration in URLs
|
|
Smoke test results for ✅ GitHub MCP — Last 2 merged PRs: #1288 Overall: PASS
|
Chroot Version Comparison Results
Result: ❌ FAILED — Python and Node.js versions differ between host and chroot environments.
|
Simplify iptables to a minimal security model: - ALLOW: localhost (127.0.0.0/8) for MCP servers + Docker DNS - ALLOW: Squid proxy IP — single egress point for HTTP/HTTPS - REJECT: everything else Remove DNS server exception rules from container and host iptables. Docker embedded DNS (127.0.0.11) handles name resolution via localhost. Squid resolves DNS internally via dns_nameservers. Remove AWF_DNS_SERVERS env var. Keep --dns-servers for Docker embedded DNS upstream forwarding and Squid config. Prevents DNS-based data exfiltration. Fixes #11 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds tests for untested code paths to fix coverage regression: - API proxy sidecar rule generation - Bridge name not found error path - DOCKER-USER chain creation fallback - Skip duplicate DOCKER-USER jump rule insertion - IPv6 cleanup with ip6tables available - IPv6 cleanup skip when ip6tables unavailable Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Docker's embedded DNS (127.0.0.11) works by redirecting port 53 queries to its internal DNS server on a random high port via iptables DNAT rules. When setup-iptables.sh flushes the NAT OUTPUT chain, these rules are destroyed, causing DNS resolution via 127.0.0.11 to fail silently. Save Docker's DNS DNAT rules before flushing and restore them after, so Docker embedded DNS continues to work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Docker's embedded DNS (127.0.0.11) forwards queries to upstream servers through the container's network interface, which traverses the Docker bridge and DOCKER-USER chain. The previous commit incorrectly assumed Docker DNS bypasses container iptables entirely, but the DNS proxy runs within the container's network namespace. Without DNS ACCEPT rules in DOCKER-USER, forwarded queries are blocked, causing SERVFAIL. Add UDP/TCP port 53 ACCEPT rules for configured upstream DNS servers in the AWF_EGRESS chain, while keeping the simplified model where containers can only use Docker embedded DNS (no direct external DNS). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The OUTPUT filter chain only dropped TCP traffic, leaving UDP unfiltered. This allowed direct DNS queries to external servers (e.g., dig @8.8.8.8) to succeed, defeating the DNS exfiltration prevention. Add iptables -A OUTPUT -p udp -j DROP alongside the existing TCP DROP rule. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The curl call to api.github.com/repos/.../releases/latest was unauthenticated, causing intermittent 403 rate limit errors in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
The blanket `iptables -A OUTPUT -p udp -j DROP` blocked Docker embedded DNS forwarding to upstream servers, causing SERVFAIL for nslookup. Changes: - Restore AWF_DNS_SERVERS env var so setup-iptables.sh knows which upstream DNS servers Docker embedded DNS forwards to - Add explicit iptables ACCEPT rules for configured upstream DNS servers (UDP/TCP port 53) before the DROP rules in the container OUTPUT chain - Fix host-iptables.ts to use ip6tables for IPv6 DNS servers instead of iptables (which rejects IPv6 addresses with exit code 2) - Update DNS exfiltration tests to use non-configured DNS servers (Quad9 9.9.9.9, OpenDNS 208.67.222.222, Cloudflare 1.1.1.1) since the default upstream (8.8.8.8) must be allowed for DNS forwarding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merge PR #1270's simplified DNS model with DoH support from main: - DoH mode: route DNS through DoH proxy (from main) - Non-DoH mode: Docker embedded DNS with upstream forwarding only - Fix test calls to include required dnsServers parameter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mossaka
force-pushed
the
fix/011-simplify-security-model
branch
from
8770717 to
88f4f55
Compare
Smoke Test Results — Claude✅ GitHub MCP: PR #1286 feat(docker): switch seccomp profile to deny-by-default / PR #1280 feat(cli): add DNS-over-HTTPS support via --dns-over-https flag Overall: PASS
|
|
Smoke test results for PR #1270 (
Overall: FAIL (Playwright blocked by firewall)
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
This comment has been minimized.
This comment has been minimized.
When IPv6 DNS servers are configured, the code tried to append rules to the FW_WRAPPER_V6 chain without creating it first, causing "No chain/target/match by that name" error from ip6tables. Create the chain lazily when IPv6 DNS servers are present and ip6tables is available. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Smoke Test Results — PASS
|
Smoke Test Results — Copilot Engine✅ GitHub MCP: Last 2 merged PRs:
✅ Playwright: Overall: PASS | PR author:
|
|
PR titles:
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Summary
dns_nameserversdirective (now configurable via--dns-servers)AWF_DNS_SERVERSenv var from agent container (no longer needed)--dns-serversflag for Docker embedded DNS upstream forwarding and Squid DNS configSecurity Model (Before → After)
Before: localhost + Squid + DNS servers (8.8.8.8, etc.) + Docker DNS
After: localhost + Squid proxy only
This prevents DNS-based data exfiltration since containers cannot directly query external DNS servers. Docker's embedded DNS at 127.0.0.11 is covered by localhost rules and forwards to upstream servers at the Docker daemon level.
Test plan
npm run build)npm test)npm run lint)Fixes #11
🤖 Generated with Claude Code