refactor: centralize network policy into sandbox-network-policy.json#6387
Conversation
Introduce a single declarative source of truth for network configuration across all runtimes, mirroring the existing sandbox-mount-policy.json pattern. The default, iptables-less network-isolation (topology) path is the focus: its topology (network name, subnet, static sidecar IPs), proxy ports, and DNS config now live in src/config/sandbox-network-policy.json and are loaded and validated by src/config/network-policy.ts. The legacy host/container iptables deny lists (blocked ports/CIDRs, host gateway) are captured in the same file under `legacyIptables` for the uncommon --legacy-security path (consolidation of their call sites is a follow-up). All values are logical Docker network addresses with no host-filesystem assumptions, so the policy is ARC/DinD safe and needs no path translation. This is a non-behavioral refactor: existing modules (host-iptables-shared, constants, dns-resolver, topology, types/ports, compose-network) now derive their constants from the policy, preserving every exported name and value. Parity tests assert the refactor is behavior-preserving. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4bdbe67a-cb99-41a2-944b-49cfd949360e
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.64% | 98.46% | 📉 -0.18% |
| Statements | 98.46% | 98.29% | 📉 -0.17% |
| Functions | 99.24% | 99.15% | 📉 -0.09% |
| Branches | 94.55% | 94.25% | 📉 -0.30% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/topology.ts |
98.8% → 98.8% (+0.01%) | 98.8% → 98.8% (+0.02%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (1 files)
src/config/network-policy.ts: 85.7% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Pull request overview
Centralizes network topology, proxy, DNS, and legacy firewall values into a declarative policy.
Changes:
- Adds a validated network policy and compatibility accessors.
- Rewires existing constants and Compose configuration.
- Adds policy invariant and parity tests.
Show a summary per file
| File | Description |
|---|---|
src/config/sandbox-network-policy.json |
Defines centralized network policy. |
src/config/network-policy.ts |
Validates and exposes policy values. |
src/config/network-policy.test.ts |
Tests invariants and compatibility. |
src/compose-network.ts |
Uses policy bridge and DNS values. |
src/constants.ts |
Re-exports Squid port. |
src/dns-resolver.ts |
Re-exports DNS defaults. |
src/host-iptables-shared.ts |
Re-exports topology constants. |
src/topology.ts |
Derives topology network name. |
src/types/ports.ts |
Maps policy ports to legacy exports. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Medium
| function validate(input: unknown): NetworkPolicy { | ||
| const p = asObject(input, 'root'); | ||
| return { | ||
| topology: parseTopology(p.topology), | ||
| proxies: parseProxies(p.proxies), | ||
| dns: parseDns(p.dns), | ||
| legacyIptables: parseLegacyIptables(p.legacyIptables), | ||
| }; | ||
| } | ||
|
|
||
| /** The validated, frozen network policy loaded from the JSON config. */ | ||
| export const networkPolicy: NetworkPolicy = validate(rawPolicy); |
There was a problem hiding this comment.
Good catch — readonly is erased at runtime. Added a deepFreeze() and now export deepFreeze(validate(rawPolicy)), so the whole tree (including the object returned by apiProxyPorts()) is frozen. Added tests asserting nested objects/arrays are frozen and that mutating apiProxyPorts() throws. (5917b29)
| squidService.networks = { | ||
| ...(squidService.networks || {}), | ||
| 'awf-ext': {}, | ||
| [EXTERNAL_BRIDGE_NAME]: {}, |
There was a problem hiding this comment.
Fixed — EXTERNAL_BRIDGE_NAME is now used at all three sites: the Squid attachment, the network declaration (compose-network.ts:58), and the sbx api-proxy attachment (compose-generator.ts:156). (5917b29)
| /** Default IPv4 subnet (CIDR) of the internal topology network. */ | ||
| export const NETWORK_SUBNET: string = networkPolicy.topology.subnet; |
There was a problem hiding this comment.
Fixed — main-action.ts now imports NETWORK_SUBNET (re-exported from host-iptables-shared) and uses it at both sbx sites (:282 and :300) so the sbx environment/credential config tracks the policy. (5917b29)
| "$comment": "Central declarative NETWORK policy shared by ALL runtimes. The PRIMARY, default path is network-isolation (topology) mode: the agent runs on an `internal` Docker network whose only egress is the dual-homed Squid proxy — NO iptables (host or container), no NET_ADMIN. In that mode egress restriction is STRUCTURAL (default-deny by routing): blocked ports/CIDRs have no route out, so they are denied by construction rather than by rules. The `legacyIptables` section below is consumed ONLY by the uncommon --legacy-security / --enable-host-access path (host iptables + the container setup-iptables.sh NAT script). Every value here is a logical Docker network address (subnet, static IP, port, network name) that the daemon resolves — it carries NO host-filesystem or host-network assumptions, so the policy is ARC/DinD safe and needs no --docker-host-path-prefix translation. Loaded and validated by src/config/network-policy.ts.", | ||
|
|
||
| "topology": { | ||
| "$comment": "CORE — consumed by the default iptables-less path. Compose IPAM assigns these static IPs on the internal `networkName`; Squid is dual-homed onto `externalBridgeName` as the sole egress. `subnet` is overridable via CLI to avoid collisions on a shared DinD daemon. `hostGateway` lives in `legacyIptables`, not here — the default path never touches the host network.", |
There was a problem hiding this comment.
Removed — dropped the CLI-override claim from the policy JSON $comment and reworded the loader doc. There is no subnet CLI option, and this PR is non-behavioral. (5917b29)
- Deep-freeze the validated policy tree so immutability holds at runtime (not just via erased `readonly` types); prevents live accessors like apiProxyPorts() from diverging through stray mutation. Add freeze tests. - Use EXTERNAL_BRIDGE_NAME at all three awf-ext sites: the Compose network declaration and the sbx api-proxy attachment in compose-generator, not just the Squid attachment. - Use NETWORK_SUBNET in the sbx path (main-action.ts) so editing the policy updates the sbx environment/credential config too, not only Docker/iptables. - Drop the inaccurate "subnet is CLI-overridable" claim from the policy JSON and loader doc — there is no subnet CLI option. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4bdbe67a-cb99-41a2-944b-49cfd949360e
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.64% | 98.46% | 📉 -0.18% |
| Statements | 98.46% | 98.30% | 📉 -0.16% |
| Functions | 99.24% | 99.16% | 📉 -0.08% |
| Branches | 94.55% | 94.22% | 📉 -0.33% |
📁 Per-file Coverage Changes (4 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/services/agent-environment/env-passthrough.ts |
96.8% → 96.3% (-0.48%) | 96.8% → 96.3% (-0.48%) |
src/topology.ts |
98.8% → 98.8% (+0.01%) | 98.8% → 98.8% (+0.02%) |
src/services/cli-proxy-service.ts |
94.7% → 95.0% (+0.27%) | 94.7% → 95.0% (+0.27%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (2 files)
src/config/network-policy.ts: 86.4% linessrc/services/no-proxy-utils.ts: 100.0% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
✅ Smoke Claude passed |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Build Test Suite completed successfully! |
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅ |
|
📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤 |
|
❌ Security Guard failed. Please review the logs for details. |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
❌ Contribution Check failed. Please review the logs for details. |
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
Smoke Test: Claude Engine Validation
Overall Result: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔬 Smoke Test Results
Overall: PASS @lpcox — all smoke tests passed. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: Services Connectivity
Overall: FAIL — Service containers are not reachable from this runner (no Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Results
Overall status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Smoke test: network isolation egress enforcement EGRESS_RESULT allow=pass deny=pass ✅ Test 1 (allowed domain): Overall: PASS — @lpcox Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"
- "example.com"See Network Configuration for more information.
|
github-actions
Bot
added
the
smoke-copilot-network-isolation
Smoke Test: Copilot PAT Auth — PASS ✅
Overall: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
Smoke Test: Copilot BYOK (Direct) ✅ GitHub MCP connectivity (MCP server responding) Mode: Direct BYOK via COPILOT_PROVIDER_API_KEY Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: API Proxy OpenTelemetry Tracing
All scenarios pass. OTEL tracing integration is fully functional. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"
- "registry.npmjs.org"See Network Configuration for more information.
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔥 Smoke Test Results
Overall: PASS (2/2 functional tests passed; file test skipped due to unresolved template variables in workflow)
|
|
test body Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
Introduces a single declarative source of truth for network configuration across all runtimes —
src/config/sandbox-network-policy.json+ a validating loadersrc/config/network-policy.ts— mirroring the existingsandbox-mount-policy.jsonpattern.This is step 1 of centralizing network policy: a non-behavioral refactor. Every existing exported constant keeps its name and value; the definitions now derive from the policy JSON instead of being scattered across the codebase.
Why
Network parameters were duplicated and drift-prone across
host-iptables-shared.ts,constants.ts,types/ports.ts(which literally documented a manual "keep in sync" list),dns-resolver.ts,topology.ts, andcompose-network.ts. Centralizing them removes the drift risk and gives us one place to reason about the network model.Design focus: the default, iptables-less path
The schema is organized around network-isolation (topology) mode — the default (
--no-network-isolationrequires--legacy-security). In that mode the agent runs on aninternalDocker network whose only egress is the dual-homed Squid proxy: no host or container iptables. Egress restriction is structural (default-deny by routing), so blocked ports/CIDRs are denied by construction.topology,proxies,dns) — the values the default path actually consumes.legacyIptables(host gateway, blocked ports, blocked CIDRs) — consumed only by the uncommon--legacy-security/--enable-host-accesspath. Captured here now so a follow-up can consolidate the bash-onlyDANGEROUS_PORTSarray and the host-iptables-only blocked CIDRs against one list.ARC / DinD safety
Every value is a logical Docker network address (subnet, static IP, port, network name) resolved by the daemon — none carries a host-filesystem assumption, so the policy is ARC/DinD safe and needs no
--docker-host-path-prefixtranslation (unlike the mount policy). The host-network touch points (host gateway,host.docker.internal, NET_ADMIN) are confined to thelegacyIptablessection, reinforcing why topology mode is the default.Changes
src/config/sandbox-network-policy.json—$comment-documented schema (topology / proxies / dns / legacyIptables).src/config/network-policy.ts— static import +validate()(IPv4/CIDR/port/uniqueness checks) → frozen typed policy + typed accessors, no runtime file read.src/config/network-policy.test.ts— 20 tests: schema invariants + non-behavioral parity + downstream-module sync.host-iptables-shared.ts,constants.ts,dns-resolver.ts,topology.ts,types/ports.ts, andcompose-network.ts(replacesawf-ext/127.0.0.11literals).Validation
npm run build✅ ·npm run type-check✅ ·npm run lint✅ (0 errors)npm test✅ — 3872 passed (3852 baseline + 20 new)Follow-up (out of scope)
Step 2 — legacy consolidation: feed
legacyIptables.blockedPortsinto anAWF_BLOCKED_PORTSenv consumed bysetup-iptables.sh(retiring the bash array) and applyblockedCidrsin sbx. Only touches the uncommon iptables path.