diff --git a/internal/apply/opening.go b/internal/apply/opening.go index 95012e9..8bd415e 100644 --- a/internal/apply/opening.go +++ b/internal/apply/opening.go @@ -30,6 +30,7 @@ func foundFirewall(ctx context.Context, d *declaration.Declaration, known *store return "", err } rec.DisabledByMesh = false + rec.Forward = nil log(" enabled ufw again: this node is adopted, and the firewall found on it is in force") } kind, name, err := firewall.Detect(ctx, run) @@ -68,7 +69,12 @@ func retireFirewall(ctx context.Context, d *declaration.Declaration, origin stri rec.DisabledByMesh { return nil } - if err := firewall.Disable(ctx, run); err != nil { + if rec.Forward == nil { + // Recorded before ufw is touched: disabling it opens the forward policy, and a retry + // must know what it was (novox/hq ADR 0100). + rec.Forward = firewall.ForwardPolicies(ctx, run) + } + if err := firewall.Disable(ctx, run, rec.Forward); err != nil { return err } rec.DisabledByMesh = true diff --git a/internal/apply/opening_test.go b/internal/apply/opening_test.go index ccdf79d..a81a317 100644 --- a/internal/apply/opening_test.go +++ b/internal/apply/opening_test.go @@ -21,6 +21,23 @@ type ufwMachine struct { rules []string ruleset string asked []string + + // forward is iptables' forward policy when set; empty is a machine without iptables. failP + // is how many -P calls fail before one succeeds. + forward string + failP int +} + +func (u *ufwMachine) iptables(args []string) (string, error) { + if len(args) == 3 && args[0] == "-P" { + if u.failP > 0 { + u.failP-- + return "", errors.New("iptables: resource temporarily unavailable") + } + u.forward = args[2] + return "", nil + } + return "-P FORWARD " + u.forward + "\n-A FORWARD -j DOCKER-USER\n", nil } func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string, error) { @@ -28,6 +45,11 @@ func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string switch name { case "nft": return u.ruleset, nil + case "iptables": + if u.forward == "" { + return "", &exec.Error{Name: name, Err: exec.ErrNotFound} + } + return u.iptables(args) case "ufw": if !u.installed { return "", &exec.Error{Name: name, Err: exec.ErrNotFound} @@ -52,6 +74,9 @@ func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string return "", nil case "disable": u.active = false + if u.forward != "" { + u.forward = "ACCEPT" // as measured: ufw disable opens the forward policy + } return "", nil case "delete": want := strings.Join(args[1:], " ") @@ -370,3 +395,27 @@ func TestAStaleOpeningOnAnAdoptedNodeIsRemovedAsAnyOrphan(t *testing.T) { t.Errorf("the stale opening was not removed before the new one was added: %v", u.asked) } } + +func TestARetiredFirewallRetriedStillPutsBackTheForwardPolicy(t *testing.T) { + // The forward policy is recorded before ufw is disabled, so a retirement that failed after + // the disable restores what the machine had, not what the disable left (novox/hq ADR 0100). + dir := t.TempDir() + u := &ufwMachine{installed: true, active: true, forward: "DROP", failP: 1} + _, state, err := applyWith(t, adopted(t, `{"taken":[]}`, withConf(dir)), store.State{}, u.run) + if err != nil { + t.Fatal(err) + } + converged := parse(t, `{"declaration":1,"resources":[`+withConf(dir)+`]}`) + if _, state, err = applyWith(t, converged, state, u.run); err == nil { + t.Fatal("the failed restore was not reported") + } + if u.active || u.forward != "ACCEPT" || state.Firewall.Forward["iptables"] != "DROP" { + t.Fatalf("after the failed attempt: active %v, forward %s, recorded %+v", u.active, u.forward, state.Firewall) + } + if _, state, err = applyWith(t, converged, state, u.run); err != nil { + t.Fatal(err) + } + if u.forward != "DROP" || !state.Firewall.DisabledByMesh { + t.Errorf("the retry did not put the forward policy back: %s, %+v", u.forward, state.Firewall) + } +} diff --git a/internal/firewall/firewall.go b/internal/firewall/firewall.go index 789af22..262310d 100644 --- a/internal/firewall/firewall.go +++ b/internal/firewall/firewall.go @@ -806,40 +806,46 @@ func Enable(ctx context.Context, run Runner) error { // runtime had set that one to drop when it turned forwarding on, and it does not set it again while // forwarding stays on — not even on a restart. Left so, a retired ufw turns the machine into a // router for anyone who can reach it. So each family's forward policy is read before, and one that -// was drop is put back and read back. -func Disable(ctx context.Context, run Runner) error { - type family struct{ tool, policy string } - var before []family - for _, tool := range []string{"iptables", "ip6tables"} { - if policy, ok := forwardPolicy(ctx, run, tool); ok { - before = append(before, family{tool, policy}) - } - } +// was drop is put back and read back. before is ForwardPolicies as read before the first attempt. +func Disable(ctx context.Context, run Runner, before map[string]string) error { if _, err := run(ctx, "ufw", "disable"); err != nil { return fmt.Errorf("disabling ufw: %w", err) } if err := expectActive(ctx, run, false); err != nil { return err } - for _, f := range before { - if f.policy != "DROP" { + for _, tool := range []string{"iptables", "ip6tables"} { + if before[tool] != "DROP" { continue } - if now, ok := forwardPolicy(ctx, run, f.tool); ok && now == "DROP" { + if now, ok := forwardPolicy(ctx, run, tool); ok && now == "DROP" { continue } - if _, err := run(ctx, f.tool, "-P", "FORWARD", "DROP"); err != nil { + if _, err := run(ctx, tool, "-P", "FORWARD", "DROP"); err != nil { return fmt.Errorf("ufw is disabled, and %s's forward policy, which was drop, could not be put back: %w", - f.tool, err) + tool, err) } - if now, ok := forwardPolicy(ctx, run, f.tool); !ok || now != "DROP" { + if now, ok := forwardPolicy(ctx, run, tool); !ok || now != "DROP" { return fmt.Errorf("ufw is disabled, and %s's forward policy was put back to drop and reads %q", - f.tool, now) + tool, now) } } return nil } +// ForwardPolicies reads each family's forward policy, by the tool that sets it. Read before ufw is +// disabled and kept by the caller, so a retirement that fails half-way is retried with what the +// machine had — not with what the half-done disable left. +func ForwardPolicies(ctx context.Context, run Runner) map[string]string { + out := map[string]string{} + for _, tool := range []string{"iptables", "ip6tables"} { + if policy, ok := forwardPolicy(ctx, run, tool); ok { + out[tool] = policy + } + } + return out +} + // forwardPolicy reads the forward chain's policy the way iptables prints it: "-P FORWARD DROP". // Not ok when the tool is absent or says nothing readable. func forwardPolicy(ctx context.Context, run Runner, tool string) (string, bool) { diff --git a/internal/firewall/firewall_test.go b/internal/firewall/firewall_test.go index 6d0d98c..02d14bf 100644 --- a/internal/firewall/firewall_test.go +++ b/internal/firewall/firewall_test.go @@ -356,7 +356,7 @@ func TestRemovingAnOpeningRemovesOnlyWhatWasMarkedForIt(t *testing.T) { func TestEnableAndDisableReadBack(t *testing.T) { f := &fakeUFW{installed: true, active: true} - if err := Disable(context.Background(), f.run); err != nil || f.active { + if err := Disable(context.Background(), f.run, nil); err != nil || f.active { t.Fatalf("disable: %v", err) } if err := Enable(context.Background(), f.run); err != nil || !f.active { @@ -493,7 +493,7 @@ func TestRetiringUfwKeepsTheMachineFromRoutingForOthers(t *testing.T) { t.Fatal("the captures no longer show ufw disable opening the forward policy") } f := &fakeUFW{installed: true, active: true, iptablesActive: string(before), iptablesInactive: string(after)} - if err := Disable(context.Background(), f.run); err != nil { + if err := Disable(context.Background(), f.run, ForwardPolicies(context.Background(), f.run)); err != nil { t.Fatal(err) } if f.active { @@ -511,7 +511,7 @@ func TestRetiringUfwKeepsTheMachineFromRoutingForOthers(t *testing.T) { func TestRetiringUfwOnAMachineWithoutIptablesStillRetiresIt(t *testing.T) { f := &fakeUFW{installed: true, active: true} - if err := Disable(context.Background(), f.run); err != nil || f.active { + if err := Disable(context.Background(), f.run, nil); err != nil || f.active { t.Fatalf("disable: %v, active %v", err, f.active) } } diff --git a/internal/store/store.go b/internal/store/store.go index 3f135a6..60aa169 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -116,6 +116,10 @@ type FoundFirewall struct { // DisabledByMesh is set when converging retired it, so returning to adopted enables it again // and nothing else ever does. DisabledByMesh bool `json:"disabled_by_mesh,omitempty"` + // Forward is each family's forward policy as it was before the mesh disabled the firewall, + // by the tool that sets it — recorded before, so a retirement retried puts back what the + // machine had. + Forward map[string]string `json:"forward,omitempty"` FoundAt time.Time `json:"found_at"` }