fix: allow host service ports for GitHub Actions services containers#1436
fix: allow host service ports for GitHub Actions services containers#1436
Conversation
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.69% | 82.45% | 📉 -0.24% |
| Statements | 82.35% | 82.10% | 📉 -0.25% |
| Functions | 81.11% | 80.83% | 📉 -0.28% |
| Branches | 75.88% | 75.53% | 📉 -0.35% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
60.5% → 59.4% (-1.09%) | 60.9% → 59.7% (-1.19%) |
src/docker-manager.ts |
86.3% → 86.6% (+0.33%) | 85.7% → 86.0% (+0.32%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Mossaka
left a comment
Mossaka
left a comment
There was a problem hiding this comment.
Security Review: --allow-host-service-ports
Reviewed the PR diff, containers/agent/setup-iptables.sh, src/squid-config.ts, src/cli.ts, src/docker-manager.ts, and src/types.ts.
Overall Assessment
The approach is fundamentally sound. The design correctly restricts dangerous-port access to the host gateway IP only, using destination-scoped iptables rules rather than blanket allowances. The use case (GitHub Actions services: containers) is legitimate and the implementation avoids weakening the external egress firewall. There are a few issues worth addressing, categorized below.
1. Bypass Risk — Can this reach non-host destinations? LOW RISK - SOUND
The iptables rules are properly destination-restricted:
iptables -A OUTPUT -p tcp -d "$HSP_HOST_GW_IP" --dport "$port" -j ACCEPTThe -d constraint ensures only the resolved host.docker.internal IP (and optionally the network gateway) can be reached on these ports. External IPs on dangerous ports still hit the final DROP rule. The integration test should block dangerous port to non-host destinations (internet) correctly validates this.
One observation: the NAT RETURN rules in the new block are redundant when AWF_ENABLE_HOST_ACCESS is set. The existing host access block (line 175) already does a blanket iptables -t nat -A OUTPUT -d "$HOST_GATEWAY_IP" -j RETURN for all ports to the host gateway. The new per-port NAT RETURN rules will never be evaluated because the blanket RETURN already matched. This isn't a security bug — the FILTER chain's per-port ACCEPT is what actually controls access — but the redundant NAT rules may confuse future maintainers into thinking they're load-bearing. Consider adding a comment clarifying that the effective security enforcement is in the FILTER chain, not NAT.
2. Port Validation / Injection — LOW RISK, ONE CONCERN
CLI-side validation (cli.ts): Good. Validates parseInt(port, 10) and checks 1 <= port <= 65535. Rejects NaN. This prevents command injection via the --allow-host-service-ports CLI flag.
Shell-side (setup-iptables.sh): The port values are passed via environment variable AWF_HOST_SERVICE_PORTS and used directly in --dport "$port". The xargs trim is the only sanitization. Unlike the existing --allow-host-ports code path in squid-config.ts (which does port.replace(/[^0-9-]/g, '')), the new code has no shell-side sanitization. Since iptables --dport expects a numeric value and rejects non-numeric input (causing the iptables command to fail with an error, not execute injected commands), and the CLI validates before passing to the container, this is low risk. However, for defense-in-depth consistency, consider adding numeric validation in the shell script as well:
if ! [[ "$port" =~ ^[0-9]+$ ]]; then
echo "[iptables] WARNING: Skipping invalid port: $port"
continue
fi3. Privilege Escalation — NO ISSUE
The iptables rules are set by the awf-iptables-init init container, which runs in a separate container sharing the agent's network namespace. The agent container never gets NET_ADMIN capability. CAP_SYS_CHROOT and CAP_SYS_ADMIN are dropped via capsh before user code runs. The agent cannot modify iptables rules after startup. No escalation path introduced.
4. Data Exfiltration via Host Services — MEDIUM RISK - ACCEPTED BY DESIGN
This is the most significant trade-off. With --allow-host-service-ports 5432, a compromised agent could:
- Write arbitrary data to a PostgreSQL/Redis/MySQL service running on the host
- Use the database as a side channel to exfiltrate data if the database is externally accessible
This is inherent to the feature's design and documented in the PR. The mitigation is that this is opt-in and requires the workflow author to explicitly specify ports. The alternative (no database access) makes the services: use case impossible. Recommendation: Consider adding a CLI warning when this flag is used, similar to the existing --ssl-bump warning:
[WARN] --allow-host-service-ports bypasses dangerous port restrictions for host-local traffic.
[WARN] Ensure host services on these ports do not provide external network access.
5. Defense in Depth (Squid Layer) — MEDIUM - GAP EXISTS BUT ACCEPTABLE
Traffic to host service ports bypasses Squid entirely. The iptables NAT RETURN ensures packets go directly to the host gateway, not through the Squid proxy. This means:
- No domain-based filtering (acceptable — it's an IP, not a domain)
- No Squid access logging for these connections (audit gap)
- No
Safe_portsACL check at the Squid layer
This is the correct design — Squid can't meaningfully filter direct TCP connections to host.docker.internal:5432 (it's not HTTP/HTTPS traffic). The iptables FILTER chain is the right enforcement layer here. However, the audit gap is worth noting: connections to host service ports won't appear in Squid's access.log or audit.jsonl. The iptables audit dump (iptables-audit.txt) will show the rules but not individual connections. Consider whether the iptables LOG rules should cover accepted host-service-port traffic for observability, not just blocked traffic.
6. Race Conditions — NO ISSUE
The iptables-init container runs as an init container (depends_on with condition: service_completed_successfully). The agent's entrypoint waits for /tmp/awf-init/ready before executing the user command. The capability drop happens in the agent's entrypoint after init completes. No race window exists.
7. Interaction with Other Flags — LOW RISK
--enable-dind: DinD uses the same network namespace. Host service port rules apply within the DinD context. No conflict.--enable-api-proxy: API proxy sidecar is at172.30.0.30, not the host gateway IP. No interaction.--allow-host-ports: These two flags have different semantics (--allow-host-portsrejects dangerous ports,--allow-host-service-portsallows them). Both can be used simultaneously. No conflict — they append independent FILTER ACCEPT rules.--ssl-bump: SSL bump only applies to Squid-proxied traffic. Host service port traffic bypasses Squid, so no interaction.
8. Additional Observations
DANGEROUS_PORTS list drift: The DANGEROUS_PORTS array in setup-iptables.sh has fewer entries than in squid-config.ts (missing 5984, 6984, 8086, 8088, 9200, 9300). This is a pre-existing issue, not introduced by this PR, but worth noting since --allow-host-service-ports interacts with the iptables-level list. If a user passes --allow-host-service-ports 9200 (Elasticsearch), the iptables dangerous-port RETURN + DROP won't block it to external IPs because port 9200 isn't in the shell-side list. The Squid Safe_ports ACL would still block it for proxied traffic, but direct connections (proxy-unaware) on that port would not be caught by the iptables dangerous-port defense.
No unit tests for Squid config interaction: The PR doesn't need to modify squid-config.ts (correct — this feature is iptables-only), but there are no unit tests verifying that --allow-host-service-ports values are NOT validated against DANGEROUS_PORTS at the Squid layer. This is fine since the code paths are separate, but a comment in cli.ts validation explaining why DANGEROUS_PORTS check is intentionally skipped would help.
Summary
| Category | Severity | Status |
|---|---|---|
| Bypass to non-host destinations | Low | Sound — destination-scoped rules |
| Port injection | Low | CLI validates; recommend shell-side sanitization |
| Privilege escalation | None | No issue |
| Data exfiltration via host services | Medium | Accepted by design; recommend warning |
| Squid-level defense gap | Medium | Acceptable — correct layer; audit gap |
| Race conditions | None | No issue |
| Flag interactions | Low | No conflicts |
Verdict: Approve with minor suggestions. The security model is correct. The feature intentionally and narrowly relaxes dangerous-port restrictions for a specific destination (host gateway only). The main recommendations are: (1) shell-side port validation for defense-in-depth, (2) CLI warning about the security implications, and (3) documenting the audit gap for host service port traffic.
Mossaka
left a comment
Mossaka
left a comment
There was a problem hiding this comment.
Code Review: --allow-host-service-ports
Overall the feature is well-motivated and the approach is sound. There are a few issues ranging from a bug to minor suggestions.
Bugs
1. HSP_PORTS may be unset when used for network gateway rules
containers/agent/setup-iptables.sh — The HSP_PORTS array is only set inside the if [ -n "$HSP_HOST_GW_IP" ] && is_valid_ipv4 "$HSP_HOST_GW_IP" block. If host.docker.internal fails to resolve but the network gateway IP is valid, the second for port in "${HSP_PORTS[@]}" loop iterates over an unset variable — silently doing nothing. This is a logic bug: service ports would not be allowed to the network gateway even though the condition was met.
Fix: Move the IFS=',' read -ra HSP_PORTS <<< "$AWF_HOST_SERVICE_PORTS" line above both if blocks (right after HSP_NET_GW_IP=...), matching the pattern used by the existing AWF_ALLOW_HOST_PORTS handling.
Correctness Concerns
2. NAT RETURN rules in the new block are redundant (no-ops)
The existing AWF_ENABLE_HOST_ACCESS block at line 175 already adds a blanket iptables -t nat -A OUTPUT -d "$HOST_GATEWAY_IP" -j RETURN (all ports, all protocols). Since --allow-host-service-ports auto-enables host access, this blanket NAT RETURN is always present. The port-specific NAT RETURN rules in the new block (iptables -t nat -A OUTPUT -p tcp -d "$HSP_HOST_GW_IP" --dport "$port" -j RETURN) will never match because the blanket rule already consumed the traffic.
This isn't a correctness bug (the FILTER ACCEPT rules that follow are needed and do the real work), but the NAT rules are dead code. Worth either:
- (a) Removing the NAT RETURN rules from the new block and adding a comment explaining why they're not needed, or
- (b) Adding a comment clarifying they're defense-in-depth in case the blanket RETURN is ever removed.
Same applies to the network gateway section.
3. Test should allow TCP connection to host service on a dangerous port uses port 15432
Port 15432 is NOT in the DANGEROUS_PORTS list (which includes 5432 but not 15432). This means the test would pass even without --allow-host-service-ports, since port 15432 is not blocked by the dangerous ports rules. The test name is misleading and doesn't validate the core feature.
The real validation comes from the port 5432 test, but that one may be skipped if the port is already in use. Consider either:
- Using a port that IS in
DANGEROUS_PORTSfor the primary test (e.g., 6379 for Redis, which is less likely to be in use) - Or starting the echo server on 5432 as the primary test, with a clearer skip message
Suggestions
4. Port range support inconsistency
--allow-host-ports supports port ranges (e.g., 3000-3010), but --allow-host-service-ports validation in cli.ts uses parseInt() which rejects ranges. The iptables --dport flag supports ranges natively. If port ranges aren't intended for service ports, that's fine, but it should be documented. If they are intended, the validation needs to handle the start-end syntax.
5. CLI option placement
The new --allow-host-service-ports option is placed after --allow-host-ports but before --enable-dind. This is good grouping. Minor nit: considering the auto-enable behavior, the help text should mention that --enable-host-access is not required (currently it says "Auto-enables host access" which is clear enough).
6. Consider extracting shared port-parsing logic
The port parsing pattern (IFS=',' read -ra ... ; for port in ...; port=$(echo "$port" | xargs)) is now repeated three times in setup-iptables.sh (for AWF_ALLOW_HOST_PORTS, AWF_HOST_SERVICE_PORTS host gateway, and AWF_HOST_SERVICE_PORTS network gateway). A helper function would reduce duplication, though this is minor.
7. AWF_ALLOW_HOST_PORTS env var passthrough to iptables-init
The existing AWF_ALLOW_HOST_PORTS is passed to the iptables-init container at line 1213 in docker-manager.ts. The new AWF_HOST_SERVICE_PORTS is correctly added at line 1219+. Good.
Test Coverage Assessment
The integration tests cover:
- Auto-enable host access ✓
- TCP connection on a (non-dangerous) port to host ✓ (but see issue #3)
- TCP connection on actual dangerous port (5432) ✓ (but may skip)
- Blocking dangerous port to internet ✓
- Multiple service ports ✓
Missing test cases:
- What happens when
--allow-host-service-portsis combined with--allow-host-portson the same port? (should work, but good to verify no duplicate rule issues) - Invalid port validation (port 0, port 99999, non-numeric) — these are validated in cli.ts but no test exercises the error path
- Empty string input (
--allow-host-service-ports "")
Verdict
The feature design is correct and the iptables rules will work as intended (traffic flows via the FILTER ACCEPT rules; the NAT RETURN is redundant but harmless). The HSP_PORTS scoping bug (#1) should be fixed before merge. The test on port 15432 (#3) should be reworked to actually test a dangerous port. The other items are suggestions for improvement.
Status: Request changes (for bug #1; rest are suggestions)
Smoke Test Results — Copilot Engine
Overall: PASS cc
|
|
Smoke test results (run 23564569383)
Overall: PASS
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new CLI flag to allow TCP access to GitHub Actions services: containers via the host gateway on ports that are otherwise considered “dangerous”, while keeping dangerous-port egress blocked for non-host destinations.
Changes:
- Added
--allow-host-service-ports <ports>CLI option andWrapperConfig.allowHostServicePortsplumbing. - Passed
AWF_HOST_SERVICE_PORTSinto the agent/iptables init path and added iptables exceptions for host-gateway-only service ports. - Added integration tests and runner fixture support for the new flag.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/host-tcp-services.test.ts | New integration tests covering host-service-port allowlisting behavior. |
| tests/fixtures/awf-runner.ts | Adds allowHostServicePorts option wiring for test runs. |
| src/types.ts | Extends WrapperConfig with allowHostServicePorts and docs. |
| src/docker-manager.ts | Exposes AWF_HOST_SERVICE_PORTS to containers (incl. iptables-init). |
| src/cli.ts | Adds the new CLI option, validates it, and auto-enables host access. |
| containers/agent/setup-iptables.sh | Adds NAT/FILTER rules to allow specified service ports to host gateway prior to dangerous-port enforcement. |
Comments suppressed due to low confidence (2)
tests/integration/host-tcp-services.test.ts:146
- The "block dangerous port to non-host destinations" assertion currently treats any connection failure as "BLOCKED" (curl failure -> echo BLOCKED). This can pass even if the firewall accidentally allows the traffic (e.g., if the remote port is simply closed/refuses). Consider asserting something that specifically indicates firewall enforcement (e.g., check iptables-audit.txt / init logs for the expected ACCEPT rule being limited to host gateway, or use a controlled non-host destination inside the test environment where the port is known-open).
test('should block dangerous port to non-host destinations (internet)', async () => {
// Even with --allow-host-service-ports 5432, traffic to external IPs
// on port 5432 should still be blocked
const result = await runner.runWithSudo(
'bash -c \'curl -s --connect-timeout 5 http://example.com:5432/ 2>&1 || echo "BLOCKED"\'',
{
allowDomains: ['example.com'],
allowHostServicePorts: '5432',
logLevel: 'debug',
timeout: 60000,
}
);
expect(result).toSucceed();
// The connection to example.com:5432 should fail because the port is only
// allowed to the host gateway, not to internet destinations
expect(result.stdout).toContain('BLOCKED');
}, 120000);
tests/integration/host-tcp-services.test.ts:109
- If binding to 5432 fails, the test logs and returns, which records as a pass rather than a skip. To avoid masking coverage gaps, consider using a runtime-conditional skip pattern (e.g., decide in beforeAll whether the port is available, then choose test vs test.skip) so CI output clearly shows the test was skipped.
try {
server = await startTcpEchoServer(TEST_PORT);
} catch {
// If we can't bind to 5432 (e.g., already in use or no privileges), skip
console.log(`Skipping test: could not bind to port ${TEST_PORT}`);
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if [ -n "$HSP_HOST_GW_IP" ] && is_valid_ipv4 "$HSP_HOST_GW_IP"; then | ||
| echo "[iptables] Allowing host service ports to host gateway ($HSP_HOST_GW_IP): $AWF_HOST_SERVICE_PORTS" | ||
| for port in "${HSP_PORTS[@]}"; do | ||
| port=$(echo "$port" | xargs) | ||
| if ! [[ "$port" =~ ^[0-9]+$ ]]; then | ||
| echo "[iptables] WARNING: Skipping invalid port: $port" | ||
| continue | ||
| fi | ||
| if [ -n "$port" ]; then |
There was a problem hiding this comment.
Ports from $AWF_HOST_SERVICE_PORTS are passed directly into iptables --dport. Since this env var can come from the host environment (or from the CLI with currently-loose parsing), a malformed value can cause iptables failures and abort the script (set -e). Consider validating each port token in setup-iptables.sh (numeric only, 1-65535) and emitting a clear warning/error for invalid entries before running iptables.
There was a problem hiding this comment.
Fixed. Service port validation in setup-iptables.sh now uses a strict single-port regex (^[1-9][0-9]{0,4}$) with range check 1-65535. Invalid entries are warned and skipped. This is more restrictive than is_valid_port_spec (which accepts ranges) — aligned with the TypeScript validator that only accepts single numeric ports.
| // Pass host service ports to container for setup-iptables.sh (if specified) | ||
| // These ports bypass DANGEROUS_PORTS validation and are only allowed to host gateway | ||
| if (config.allowHostServicePorts) { | ||
| environment.AWF_HOST_SERVICE_PORTS = config.allowHostServicePorts; |
There was a problem hiding this comment.
generateDockerCompose only applies host access wiring (extra_hosts + AWF_ENABLE_HOST_ACCESS) when config.enableHostAccess is set. If WrapperConfig.allowHostServicePorts is used programmatically without also setting enableHostAccess, AWF_HOST_SERVICE_PORTS will be passed but the iptables rule block won't run (it requires AWF_ENABLE_HOST_ACCESS) and host.docker.internal may not resolve. Consider treating allowHostServicePorts as implicitly enabling host access here to match the flag/docs behavior.
| environment.AWF_HOST_SERVICE_PORTS = config.allowHostServicePorts; | |
| environment.AWF_HOST_SERVICE_PORTS = config.allowHostServicePorts; | |
| // Treat allowHostServicePorts as implicitly enabling host access so that | |
| // setup-iptables.sh applies the required host access rules when this is set | |
| if (!config.enableHostAccess) { | |
| environment.AWF_ENABLE_HOST_ACCESS = 'true'; | |
| } |
There was a problem hiding this comment.
Good catch. Added a safety net in docker-manager.ts: when allowHostServicePorts is set but AWF_ENABLE_HOST_ACCESS is not, it's now automatically set to 'true'. This ensures programmatic usage works even without the CLI's auto-enable logic.
| // Start a TCP echo server on a dangerous port (Redis 6379, in DANGEROUS_PORTS list) | ||
| const TEST_PORT = 6379; |
There was a problem hiding this comment.
This test is labeled as exercising a "dangerous" port, but TEST_PORT is set to 15432 (not in the dangerous port list). As written it only verifies basic host connectivity, not that dangerous-port blocking is bypassed specifically for host.docker.internal. Consider either using a port that is actually in DANGEROUS_PORTS (when available) or renaming/reframing the test so its assertions match what it covers.
This issue also appears in the following locations of the same file:
- line 103
- line 129
| // Start a TCP echo server on a dangerous port (Redis 6379, in DANGEROUS_PORTS list) | |
| const TEST_PORT = 6379; | |
| // Start a TCP echo server on a "dangerous" port (PostgreSQL default 5432) | |
| const TEST_PORT = 5432; |
There was a problem hiding this comment.
The tests already use actual dangerous ports: Redis 6379 (line 75) and PostgreSQL 5432 (line 107). The comment may have been based on a stale diff. The test at line 73-101 explicitly names Redis 6379 and verifies TCP connectivity through the firewall bypass.
src/cli.ts
Outdated
| // `services:` containers that publish database ports to the host via port mapping. | ||
| if (config.allowHostServicePorts) { | ||
| // Validate port numbers (must be valid ports, but no DANGEROUS_PORTS check — see above) | ||
| const servicePorts = config.allowHostServicePorts.split(',').map(p => p.trim()); | ||
| for (const port of servicePorts) { | ||
| const portNum = parseInt(port, 10); | ||
| if (isNaN(portNum) || portNum < 1 || portNum > 65535) { | ||
| logger.error(`❌ Invalid port in --allow-host-service-ports: ${port}. Must be a number between 1 and 65535`); | ||
| process.exit(1); |
There was a problem hiding this comment.
The --allow-host-service-ports validation uses parseInt(), which will accept non-numeric inputs like "5432abc" (and even "5432-6000" parses as 5432). Those values then get passed through to iptables as --dport and can fail at runtime or behave unexpectedly. Consider validating each entry with a strict numeric regex (e.g. /^\d+$/), rejecting ranges/extra characters, and optionally normalizing back to a comma-separated canonical list.
| // `services:` containers that publish database ports to the host via port mapping. | |
| if (config.allowHostServicePorts) { | |
| // Validate port numbers (must be valid ports, but no DANGEROUS_PORTS check — see above) | |
| const servicePorts = config.allowHostServicePorts.split(',').map(p => p.trim()); | |
| for (const port of servicePorts) { | |
| const portNum = parseInt(port, 10); | |
| if (isNaN(portNum) || portNum < 1 || portNum > 65535) { | |
| logger.error(`❌ Invalid port in --allow-host-service-ports: ${port}. Must be a number between 1 and 65535`); | |
| process.exit(1); | |
| const rawServicePorts = config.allowHostServicePorts.split(','); | |
| const validatedPorts: string[] = []; | |
| for (const rawPort of rawServicePorts) { | |
| const port = rawPort.trim(); | |
| // Skip empty entries (e.g., from trailing commas) | |
| if (!port) { | |
| continue; | |
| } | |
| // Require strictly numeric port entries (no ranges or extra characters) | |
| if (!/^\d+$/.test(port)) { | |
| logger.error( | |
| `❌ Invalid port in --allow-host-service-ports: ${port}. Must be a numeric value between 1 and 65535` | |
| ); | |
| process.exit(1); | |
| } | |
| const portNum = Number(port); | |
| if (portNum < 1 || portNum > 65535) { | |
| logger.error( | |
| `❌ Invalid port in --allow-host-service-ports: ${port}. Must be a number between 1 and 65535` | |
| ); | |
| process.exit(1); | |
| } | |
| // Normalize ports back to canonical numeric strings | |
| validatedPorts.push(String(portNum)); | |
| } | |
| // If, after filtering, no valid ports remain, treat as an error | |
| if (validatedPorts.length === 0) { | |
| logger.error('❌ --allow-host-service-ports must specify at least one valid port between 1 and 65535'); | |
| process.exit(1); | |
| } | |
| // Normalize config value to a canonical, comma-separated list | |
| config.allowHostServicePorts = validatedPorts.join(','); |
There was a problem hiding this comment.
Already handled. validateAllowHostServicePorts() checks /^\d+$/.test(port) before parseInt, so inputs like 5432abc or 5432-6000 are rejected. The regex runs first and only strictly numeric strings reach the parseInt/range check.
| IFS=',' read -ra HSP_PORTS <<< "$AWF_HOST_SERVICE_PORTS" | ||
|
|
||
| # Resolve host gateway IP | ||
| HSP_HOST_GW_IP=$(getent hosts host.docker.internal 2>/dev/null | awk 'NR==1 { print $1 }') | ||
| HSP_NET_GW_IP=$(route -n 2>/dev/null | awk '/^0\.0\.0\.0/ { print $2; exit }') | ||
|
|
||
| if [ -n "$HSP_HOST_GW_IP" ] && is_valid_ipv4 "$HSP_HOST_GW_IP"; then | ||
| echo "[iptables] Allowing host service ports to host gateway ($HSP_HOST_GW_IP): $AWF_HOST_SERVICE_PORTS" | ||
| for port in "${HSP_PORTS[@]}"; do | ||
| port=$(echo "$port" | xargs) | ||
| if ! [[ "$port" =~ ^[0-9]+$ ]]; then | ||
| echo "[iptables] WARNING: Skipping invalid port: $port" | ||
| continue | ||
| fi | ||
| if [ -n "$port" ]; then | ||
| echo "[iptables] Allow host service port $port to $HSP_HOST_GW_IP" | ||
| # FILTER: allow traffic to host gateway on this port | ||
| # (NAT bypass is already handled by the blanket RETURN rule in the host access block above) | ||
| iptables -A OUTPUT -p tcp -d "$HSP_HOST_GW_IP" --dport "$port" -j ACCEPT | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| # Also allow to network gateway (same as the host access block does) | ||
| if [ -n "$HSP_NET_GW_IP" ] && is_valid_ipv4 "$HSP_NET_GW_IP" && [ "$HSP_NET_GW_IP" != "$HSP_HOST_GW_IP" ]; then | ||
| echo "[iptables] Allowing host service ports to network gateway ($HSP_NET_GW_IP): $AWF_HOST_SERVICE_PORTS" | ||
| for port in "${HSP_PORTS[@]}"; do | ||
| port=$(echo "$port" | xargs) |
There was a problem hiding this comment.
HSP_PORTS is only initialized inside the "host gateway" branch, but the "network gateway" branch iterates over "${HSP_PORTS[@]}" unconditionally. If host.docker.internal doesn't resolve (but a network gateway exists), no rules will be added for the network gateway. Parse AWF_HOST_SERVICE_PORTS into HSP_PORTS before both branches (or guard the second loop) so the fallback path works reliably.
There was a problem hiding this comment.
HSP_PORTS is parsed at line 246 (IFS=',' read -ra HSP_PORTS <<< "$AWF_HOST_SERVICE_PORTS"), before both the host gateway branch (line 252) and network gateway branch (line 270). The comment at line 245 explicitly notes this: "Parse port list once, before resolving gateway IPs, so both blocks can use it."
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.69% | 82.40% | 📉 -0.29% |
| Statements | 82.35% | 82.04% | 📉 -0.31% |
| Functions | 81.11% | 80.83% | 📉 -0.28% |
| Branches | 75.88% | 75.53% | 📉 -0.35% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
60.5% → 59.2% (-1.28%) | 60.9% → 59.5% (-1.38%) |
src/docker-manager.ts |
86.3% → 86.6% (+0.33%) | 85.7% → 86.0% (+0.32%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Mossaka
left a comment
Mossaka
left a comment
There was a problem hiding this comment.
Second-Round Review: --allow-host-service-ports
All six first-round issues have been addressed in commit a37fcb8d:
| Issue | Status |
|---|---|
| HSP_PORTS scoping bug (unset for network gateway) | Fixed -- IFS=',' read -ra HSP_PORTS moved above both gateway blocks |
| Redundant NAT RETURN rules | Fixed -- removed; comments explain blanket RETURN handles NAT bypass |
| Test port 15432 (not in DANGEROUS_PORTS) | Fixed -- changed to 6379 (Redis), actually in the dangerous list; added try/catch skip |
| Shell-side port validation | Fixed -- ^[0-9]+$ regex check added to both loops |
| CLI warning about security implications | Fixed -- two logger.warn() lines added |
| Comment about intentional DANGEROUS_PORTS skip | Fixed -- explanatory comment added in cli.ts |
Minor remaining nit (non-blocking)
The CLI-side validation still uses parseInt(port, 10) which silently accepts inputs like "5432abc" (parsed as 5432). The shell-side ^[0-9]+$ regex catches this as defense-in-depth, so it's not a security issue. For consistency, a strict /^\d+$/ check on the CLI side (as Copilot suggested) would be cleaner, but not required for merge.
CI Status
All completed checks pass. Integration tests are still running -- no failures observed so far.
Verdict
All requested changes are properly addressed. No new issues introduced by the fixes. The implementation is clean and ready to merge once CI completes green.
Mossaka
left a comment
Mossaka
left a comment
There was a problem hiding this comment.
Second-Round Security Review: --allow-host-service-ports
Reviewed the full PR diff, the updated containers/agent/setup-iptables.sh, src/cli.ts, src/docker-manager.ts, and src/types.ts on the PR branch.
First-Round Suggestions — All Addressed
| Suggestion | Status |
|---|---|
Shell-side numeric port validation (^[0-9]+$ regex) |
Addressed — lines 233-236 and 251-254 in setup-iptables.sh |
| CLI warning about security implications | Addressed — logger.warn() at lines 1694-1695 in cli.ts |
HSP_PORTS scoping bug (was set inside first if block only) |
Fixed — IFS=',' read -ra HSP_PORTS now at line 223, before both gateway IP blocks |
| Test using non-dangerous port 15432 | Fixed — tests now use actual dangerous ports (6379, 5432) |
| Redundant NAT RETURN rules in new block | Resolved — new block only adds FILTER ACCEPT rules, with comments explaining that NAT bypass is handled by the blanket RETURN in the host access block above |
Rule Ordering Verification
The iptables rule chain is correctly ordered:
- Localhost/DNS/Squid/API-proxy RETURN/ACCEPT rules (unchanged)
- Host access blanket NAT RETURN + FILTER ACCEPT for ports 80/443 +
--allow-host-ports(unchanged) - New: Host service port FILTER ACCEPT rules (lines 216-262) — destination-scoped to gateway IPs only
- Dangerous port NAT RETURN rules (lines 264-294) — these cause traffic to fall through to the final DROP
- HTTP/HTTPS DNAT to Squid (lines 300-301)
- FILTER chain: DNS ACCEPT, Squid ACCEPT, LOG, DROP (lines 329-379)
This ordering is correct: host service port ACCEPT rules are evaluated before the dangerous-port RETURN+DROP, so traffic to host.docker.internal:5432 hits the ACCEPT before it can be dropped. Traffic to external-ip:5432 does not match the destination-scoped ACCEPT and falls through to the DROP.
Destination Scoping Verification
All FILTER ACCEPT rules for host service ports remain properly destination-scoped:
iptables -A OUTPUT -p tcp -d "$HSP_HOST_GW_IP" --dport "$port" -j ACCEPT
iptables -A OUTPUT -p tcp -d "$HSP_NET_GW_IP" --dport "$port" -j ACCEPTThe -d constraint ensures these rules only match traffic to the resolved host gateway IPs. External destinations on dangerous ports still hit the final DROP rule. The integration test should block dangerous port to non-host destinations (internet) validates this.
No New Security Concerns
- No new injection vectors: CLI validates port range (1-65535), shell validates numeric-only regex, iptables rejects non-numeric
--dportvalues. Three layers of defense. - No privilege changes: iptables-init container still the only one with
NET_ADMIN; agent still dropsSYS_CHROOTandSYS_ADMIN. - Auto-enable of
--enable-host-accessis correctly placed before thevalidateAllowHostPortscheck, so the existing validation logic still runs. - The
AWF_HOST_SERVICE_PORTSenv var is correctly passed to the iptables-init container viadocker-manager.ts(line 1222).
Recommendation
Security posture is satisfactory. All first-round issues have been addressed. The feature correctly and narrowly relaxes dangerous-port restrictions for host-gateway-only traffic with proper destination scoping, input validation at multiple layers, and clear warnings. Recommend approval.
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.69% | 82.31% | 📉 -0.38% |
| Statements | 82.35% | 81.96% | 📉 -0.39% |
| Functions | 81.11% | 80.83% | 📉 -0.28% |
| Branches | 75.88% | 75.48% | 📉 -0.40% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
60.5% → 58.9% (-1.57%) | 60.9% → 59.2% (-1.67%) |
src/docker-manager.ts |
86.3% → 86.6% (+0.33%) | 85.7% → 86.0% (+0.32%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
Smoke test results (run 23564909311) ✅ GitHub MCP — #1414 fix: write apiKeyHelper to ~/.claude/settings.json for Claude Code v2.1.81+; #1350 docs: Fix proxy env var docs and add missing CLI flags Overall: PASS
|
Smoke Test Results — PASS ✅
Overall: PASS —
|
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.
This comment has been minimized.
This comment has been minimized.
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.69% | 82.52% | 📉 -0.17% |
| Statements | 82.35% | 82.20% | 📉 -0.15% |
| Functions | 81.11% | 81.25% | 📈 +0.14% |
| Branches | 75.88% | 75.78% | 📉 -0.10% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
60.5% → 60.1% (-0.37%) | 60.9% → 60.6% (-0.32%) |
src/docker-manager.ts |
86.3% → 86.6% (+0.33%) | 85.7% → 86.0% (+0.32%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test Results
Overall: PASS
|
Smoke Test Results✅ GitHub MCP — Last 2 merged PRs:
✅ Playwright — github.com title contains "GitHub" Overall: PASS | PR by
|
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.
This comment has been minimized.
This comment has been minimized.
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (3 files)
Coverage comparison generated by |
Smoke Test Results
Overall: PASS
|
🚬 Smoke Test Results
Overall: PASS | Author:
|
This comment has been minimized.
This comment has been minimized.
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.
Mossaka
force-pushed
the
fix/allow-host-service-ports
branch
from
7b97939 to
c1fdc26
Compare
🤖 Smoke Test Results✅ GitHub MCP: #1435 "fix: allow host gateway traffic for localhost/Playwright" ( Overall: PASS PR author:
|
|
Smoke Test Results — PASS ✅ GitHub MCP: #1435 fix: allow host gateway traffic for localhost/Playwright, #1432 Pre-install commonly needed system packages in agent container image
|
Mossaka
force-pushed
the
fix/allow-host-service-ports
branch
from
c1fdc26 to
75166eb
Compare
Adds --allow-host-service-ports CLI flag that allows TCP connections to the host gateway IP on ports normally blocked as dangerous (e.g., PostgreSQL 5432, Redis 6379). Designed for GitHub Actions services: containers that publish to the host — the agent can reach these services but still cannot reach databases on the internet. Changes: - New CLI flag with strict port validation (single numeric ports 1-65535) - Host-level FW_WRAPPER ACCEPT rules for service ports on gateway IPs - Container-level iptables ACCEPT rules for service ports - Auto-enables --enable-host-access with explicit warning about 80/443 - Safety net in docker-manager: enables host access if not already set - SSH risk documented in help text - Bash validation uses single-port regex (no ranges), aligned with TS - Integration tests with actual dangerous ports (5432, 6379) Security: Service port traffic is restricted to host gateway IPs only (172.17.0.1 and 172.30.0.1). Dangerous port restrictions are bypassed intentionally since traffic cannot reach the internet. Both host-level (FW_WRAPPER) and container-level iptables enforce the restriction. Closes gh-aw#22939 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mossaka
force-pushed
the
fix/allow-host-service-ports
branch
from
75166eb to
d4dd7d7
Compare
|
Smoke Test Results
Overall: PASS
|
Smoke Test Results✅ GitHub MCP — Last 2 merged PRs:
✅ Playwright — github.com title contains "GitHub" Overall: PASS | Author:
|
Chroot Version Comparison Results
Warning Not all versions match. Python and Node.js versions differ between host and chroot. Go versions match.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test ResultsPR titles: fix: allow host service ports for GitHub Actions services containers; fix: allow host gateway traffic for localhost/Playwright
Warning
|
Summary
--allow-host-service-ports <ports>CLI flag that allows TCP connections to the host gateway IP on ports normally blocked as "dangerous" (e.g., PostgreSQL 5432, Redis 6379, MySQL 3306)services:containers that publish to the host via port mapping — the agent can reach these services on the host but still cannot reach databases on the internet--enable-host-accesswhen used; bypassesDANGEROUS_PORTSvalidation since traffic is restricted to host gateway onlyFiles changed
src/types.ts— AddedallowHostServicePortsfield toWrapperConfigsrc/cli.ts— Added--allow-host-service-portsCLI option with validation and auto-enable logicsrc/docker-manager.ts— PassesAWF_HOST_SERVICE_PORTSenv var to agent and iptables-init containerscontainers/agent/setup-iptables.sh— Adds iptables NAT RETURN + FILTER ACCEPT rules for service ports to host gateway, before dangerous port blockingtests/fixtures/awf-runner.ts— AddedallowHostServicePortsto test runner optionstests/integration/host-tcp-services.test.ts— Integration tests for the new flagCloses gh-aw#22939
Test plan
npm run buildpassesnpm testpasses (1167 unit tests)npm run lintpasses (0 errors)--allow-host-service-ports 5432allows TCP to host gateway on port 5432🤖 Generated with Claude Code