From 23493af45876b37ba0daaae10e0276c5e3d7ba16 Mon Sep 17 00:00:00 2001 From: Steven Miller Date: Tue, 17 Feb 2026 10:17:49 -0500 Subject: [PATCH 1/3] Don't break docker networking --- lib/network/bridge_linux.go | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/lib/network/bridge_linux.go b/lib/network/bridge_linux.go index c3cf3304..62bef884 100644 --- a/lib/network/bridge_linux.go +++ b/lib/network/bridge_linux.go @@ -251,6 +251,13 @@ func (m *manager) setupIPTablesRules(ctx context.Context, subnet, bridgeName str log.InfoContext(ctx, "iptables FORWARD ready", "outbound", fwdOutStatus, "inbound", fwdInStatus) + // Restore Docker's FORWARD chain jumps if they were lost. + // On systems where an external tool (e.g., hypervisor firewall management) periodically + // rebuilds the FORWARD chain, Docker's jump rules can be wiped out. Docker only inserts + // them at daemon start, so they stay missing until Docker is restarted. Since hypeman + // already re-ensures its own rules here, we also restore Docker's if needed. + m.ensureDockerForwardJump(ctx) + return nil } @@ -409,6 +416,44 @@ func (m *manager) deleteForwardRuleByComment(comment string) { } } +// ensureDockerForwardJump checks if Docker's DOCKER-FORWARD chain exists but is +// unreachable from the FORWARD chain, and restores the jump if missing. +// This is a no-op if Docker is not installed or the jump already exists. +func (m *manager) ensureDockerForwardJump(ctx context.Context) { + log := logger.FromContext(ctx) + + // Check if DOCKER-FORWARD chain exists (Docker is installed and configured) + checkChain := exec.Command("iptables", "-L", "DOCKER-FORWARD", "-n") + checkChain.SysProcAttr = &syscall.SysProcAttr{ + AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, + } + if checkChain.Run() != nil { + return // Chain doesn't exist — Docker not installed or not configured + } + + // Check if jump already exists in FORWARD + checkJump := exec.Command("iptables", "-C", "FORWARD", "-j", "DOCKER-FORWARD") + checkJump.SysProcAttr = &syscall.SysProcAttr{ + AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, + } + if checkJump.Run() == nil { + return // Jump already present + } + + // DOCKER-FORWARD chain exists but the jump from FORWARD is missing — restore it. + // Append so it's evaluated after hypeman's specific accept rules. + addJump := exec.Command("iptables", "-A", "FORWARD", "-j", "DOCKER-FORWARD") + addJump.SysProcAttr = &syscall.SysProcAttr{ + AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, + } + if err := addJump.Run(); err != nil { + log.WarnContext(ctx, "failed to restore Docker FORWARD chain jump", "error", err) + return + } + + log.WarnContext(ctx, "restored missing jump to DOCKER-FORWARD in FORWARD chain") +} + // createTAPDevice creates TAP device and attaches to bridge. // downloadBps: rate limit for download (external→VM), applied as TBF on TAP egress // uploadBps/uploadCeilBps: rate limit for upload (VM→external), applied as HTB class on bridge From b80350b72dd0c28665ca1591db5c1cb0cab1201e Mon Sep 17 00:00:00 2001 From: Steven Miller Date: Tue, 17 Feb 2026 10:31:13 -0500 Subject: [PATCH 2/3] Add test --- lib/instances/network_test.go | 30 ++++++++++++++++++++++++++++++ lib/network/bridge_linux.go | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/lib/instances/network_test.go b/lib/instances/network_test.go index 70ac861c..ca6bb10c 100644 --- a/lib/instances/network_test.go +++ b/lib/instances/network_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "os" + "os/exec" "strings" "testing" "time" @@ -59,6 +60,35 @@ func TestCreateInstanceWithNetwork(t *testing.T) { require.NoError(t, err) t.Log("Network initialized") + // Verify that ensureDockerForwardJump restores Docker's FORWARD chain + t.Run("DockerForwardChainRestored", func(t *testing.T) { + // Check if DOCKER-FORWARD chain exists (Docker must be running on host) + checkChain := exec.Command("iptables", "-L", "DOCKER-FORWARD", "-n") + if checkChain.Run() != nil { + t.Skip("DOCKER-FORWARD chain not present (Docker not running), skipping") + } + + // Verify jump currently exists + checkJump := exec.Command("iptables", "-C", "FORWARD", "-j", "DOCKER-FORWARD") + require.NoError(t, checkJump.Run(), "DOCKER-FORWARD jump should exist before test") + + // Simulate the hypervisor flush: delete the jump + delJump := exec.Command("iptables", "-D", "FORWARD", "-j", "DOCKER-FORWARD") + require.NoError(t, delJump.Run(), "should be able to delete DOCKER-FORWARD jump") + + // Confirm it's gone + checkGone := exec.Command("iptables", "-C", "FORWARD", "-j", "DOCKER-FORWARD") + require.Error(t, checkGone.Run(), "DOCKER-FORWARD jump should be gone after delete") + + // Re-initialize network — this should restore the jump + err := manager.networkManager.Initialize(ctx, nil) + require.NoError(t, err) + + // Verify jump is restored + checkRestored := exec.Command("iptables", "-C", "FORWARD", "-j", "DOCKER-FORWARD") + require.NoError(t, checkRestored.Run(), "ensureDockerForwardJump should have restored the DOCKER-FORWARD jump") + }) + // Create instance with nginx:alpine and default network t.Log("Creating instance with default network...") inst, err := manager.CreateInstance(ctx, CreateInstanceRequest{ diff --git a/lib/network/bridge_linux.go b/lib/network/bridge_linux.go index 62bef884..d48aedee 100644 --- a/lib/network/bridge_linux.go +++ b/lib/network/bridge_linux.go @@ -441,8 +441,10 @@ func (m *manager) ensureDockerForwardJump(ctx context.Context) { } // DOCKER-FORWARD chain exists but the jump from FORWARD is missing — restore it. - // Append so it's evaluated after hypeman's specific accept rules. - addJump := exec.Command("iptables", "-A", "FORWARD", "-j", "DOCKER-FORWARD") + // Insert right after hypeman's last rule so the jump is evaluated before any + // explicit DROP/REJECT rules that an external firewall tool may have added. + insertPos := m.lastHypemanForwardRulePosition() + 1 + addJump := exec.Command("iptables", "-I", "FORWARD", fmt.Sprintf("%d", insertPos), "-j", "DOCKER-FORWARD") addJump.SysProcAttr = &syscall.SysProcAttr{ AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, } @@ -451,7 +453,32 @@ func (m *manager) ensureDockerForwardJump(ctx context.Context) { return } - log.WarnContext(ctx, "restored missing jump to DOCKER-FORWARD in FORWARD chain") + log.WarnContext(ctx, "restored missing jump to DOCKER-FORWARD in FORWARD chain", "position", insertPos) +} + +// lastHypemanForwardRulePosition returns the line number of the last hypeman-managed +// rule in the FORWARD chain, or 0 if none are found. +func (m *manager) lastHypemanForwardRulePosition() int { + cmd := exec.Command("iptables", "-L", "FORWARD", "--line-numbers", "-n") + cmd.SysProcAttr = &syscall.SysProcAttr{ + AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, + } + output, err := cmd.Output() + if err != nil { + return 0 + } + + lastPos := 0 + for _, line := range strings.Split(string(output), "\n") { + if !strings.Contains(line, "hypeman-") { + continue + } + var pos int + if _, err := fmt.Sscanf(line, "%d", &pos); err == nil && pos > lastPos { + lastPos = pos + } + } + return lastPos } // createTAPDevice creates TAP device and attaches to bridge. From 20f3bbd8f1a52dd0e2f83d72b757fa933212eede Mon Sep 17 00:00:00 2001 From: Steven Miller Date: Tue, 17 Feb 2026 10:41:38 -0500 Subject: [PATCH 3/3] address review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Fixed (2):** - **"Hypeman rule scan may miss comments"** — Added `-v` flag to the `iptables -L` call in `lastHypemanForwardRulePosition`, matching what `isForwardRuleCorrect` already does. Without `-v`, some iptables versions don't show comment text. - **"Test can leave host iptables modified"** — Added a `t.Cleanup` that checks if the DOCKER-FORWARD jump is missing and restores it, so a mid-test failure won't leave the host broken. **Dismissed with inline comments (2):** - **"Docker jump may bypass DOCKER-USER rules"** — Added a comment on `ensureDockerForwardJump` explaining that it only inserts when the jump is completely absent (the `-C` check returns early otherwise), so it can't reorder existing Docker rules. - **"Docker iptables test lacks privilege guard"** — Added a comment on the test explaining that `make test-linux` runs the entire suite under `sudo`, so iptables permissions are always available. --- lib/instances/network_test.go | 13 +++++++++++++ lib/network/bridge_linux.go | 7 ++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/instances/network_test.go b/lib/instances/network_test.go index ca6bb10c..1de9921e 100644 --- a/lib/instances/network_test.go +++ b/lib/instances/network_test.go @@ -61,6 +61,9 @@ func TestCreateInstanceWithNetwork(t *testing.T) { t.Log("Network initialized") // Verify that ensureDockerForwardJump restores Docker's FORWARD chain + // when it gets wiped (e.g., by a hypervisor firewall rebuild). + // Note: no extra privilege guard needed — make test-linux runs the entire + // suite under sudo, so iptables commands have the required permissions. t.Run("DockerForwardChainRestored", func(t *testing.T) { // Check if DOCKER-FORWARD chain exists (Docker must be running on host) checkChain := exec.Command("iptables", "-L", "DOCKER-FORWARD", "-n") @@ -72,6 +75,16 @@ func TestCreateInstanceWithNetwork(t *testing.T) { checkJump := exec.Command("iptables", "-C", "FORWARD", "-j", "DOCKER-FORWARD") require.NoError(t, checkJump.Run(), "DOCKER-FORWARD jump should exist before test") + // Safety net: restore the jump if the test fails or aborts after we delete it, + // so we don't leave the host's Docker networking broken. + t.Cleanup(func() { + check := exec.Command("iptables", "-C", "FORWARD", "-j", "DOCKER-FORWARD") + if check.Run() != nil { + restore := exec.Command("iptables", "-A", "FORWARD", "-j", "DOCKER-FORWARD") + _ = restore.Run() + } + }) + // Simulate the hypervisor flush: delete the jump delJump := exec.Command("iptables", "-D", "FORWARD", "-j", "DOCKER-FORWARD") require.NoError(t, delJump.Run(), "should be able to delete DOCKER-FORWARD jump") diff --git a/lib/network/bridge_linux.go b/lib/network/bridge_linux.go index d48aedee..c315737e 100644 --- a/lib/network/bridge_linux.go +++ b/lib/network/bridge_linux.go @@ -419,6 +419,11 @@ func (m *manager) deleteForwardRuleByComment(comment string) { // ensureDockerForwardJump checks if Docker's DOCKER-FORWARD chain exists but is // unreachable from the FORWARD chain, and restores the jump if missing. // This is a no-op if Docker is not installed or the jump already exists. +// +// Note: this cannot mis-order DOCKER-FORWARD vs DOCKER-USER because it only acts +// when the jump is completely absent (chain was flushed). If DOCKER-USER's jump +// still exists, DOCKER-FORWARD's jump is almost certainly still there too — they +// get wiped together — and the early -C check returns before we insert anything. func (m *manager) ensureDockerForwardJump(ctx context.Context) { log := logger.FromContext(ctx) @@ -459,7 +464,7 @@ func (m *manager) ensureDockerForwardJump(ctx context.Context) { // lastHypemanForwardRulePosition returns the line number of the last hypeman-managed // rule in the FORWARD chain, or 0 if none are found. func (m *manager) lastHypemanForwardRulePosition() int { - cmd := exec.Command("iptables", "-L", "FORWARD", "--line-numbers", "-n") + cmd := exec.Command("iptables", "-L", "FORWARD", "--line-numbers", "-n", "-v") cmd.SysProcAttr = &syscall.SysProcAttr{ AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, }