From 9839006d48fbb2bf6bbfe069684e5dac2c2bb8c4 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 22 Sep 2026 19:54:43 +0200 Subject: [PATCH] Retire the found firewall only once the mesh's own filter is loaded on the machine (hq ADR 0100) --- internal/apply/opening.go | 13 +++++++++++++ internal/apply/opening_test.go | 35 ++++++++++++++++++++++++++++++++-- internal/firewall/firewall.go | 24 +++++++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/internal/apply/opening.go b/internal/apply/opening.go index 8bd415e..216fc7c 100644 --- a/internal/apply/opening.go +++ b/internal/apply/opening.go @@ -69,6 +69,19 @@ func retireFirewall(ctx context.Context, d *declaration.Declaration, origin stri rec.DisabledByMesh { return nil } + // **Nothing is retired until what replaces it is in force** (novox/hq ADR 0100). The flip + // loads the mesh's derived filter in ufw's place; disabling ufw before that table is actually + // loaded — a filter module not assigned, or a unit that did not load — leaves the machine with + // no filter at all. + loaded, err := firewall.MeshTableLoaded(ctx, run) + if err != nil { + return err + } + if !loaded { + return fmt.Errorf("this node is converged and the mesh's own filter (table %s) is not loaded on "+ + "this machine, so ufw was left in force: retiring it would leave the machine filtering "+ + "nothing. Assign a filter module to this node, or return it to adopted", firewall.MeshTable) + } 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). diff --git a/internal/apply/opening_test.go b/internal/apply/opening_test.go index d14fbcf..5f976ee 100644 --- a/internal/apply/opening_test.go +++ b/internal/apply/opening_test.go @@ -163,8 +163,10 @@ func TestConvergingRetiresTheFoundFirewallAndReturningRestoresIt(t *testing.T) { t.Fatalf("adopted: firewall recorded as %+v", state.Firewall) } - // Converged: the opening's rule goes, and only then is ufw disabled — never reset. + // Converged: the derived filter is loaded, the opening's rule goes, and only then is ufw + // disabled — never reset. u.asked = nil + u.ruleset = "table inet mesh\n" converged := parse(t, `{"declaration":1,"resources":[`+withConf(dir)+`]}`) _, state, err = applyWith(t, converged, state, u.run) if err != nil { @@ -274,6 +276,7 @@ func TestAFlipThatFailsKeepsTheGuardAndTheOpenings(t *testing.T) { filter := `{"id":"nftables.config","type":"file","path":"` + filepath.Join(blocked, "nftables.conf") + `","content":"table inet mesh {}\n"}` converged := parse(t, `{"declaration":1,"resources":[`+withConf(dir)+`,`+filter+`]}`) u.asked = nil + u.ruleset = "table inet mesh\n" // what the filter's unit loads once the file is written _, state, err = applyWith(t, converged, state, u.run) if err == nil { t.Fatal("the failing flip reported success") @@ -403,7 +406,7 @@ 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} + u := &ufwMachine{installed: true, active: true, forward: "DROP", failP: 1, ruleset: "table inet mesh\n"} _, state, err := applyWith(t, adopted(t, `{"taken":[]}`, withConf(dir)), store.State{}, u.run) if err != nil { t.Fatal(err) @@ -459,3 +462,31 @@ func TestAGuardThatFailsLeavesTheDerivedFilterInForce(t *testing.T) { t.Error("the filter was forgotten, so nothing would ever stop it") } } + +func TestUfwIsNotRetiredUntilTheMeshsOwnFilterIsLoaded(t *testing.T) { + // The flip retires the found firewall because the mesh's derived filter takes its place. If + // that table is not loaded, retiring would leave the machine filtering nothing (novox/hq ADR 0100). + dir := t.TempDir() + u := &ufwMachine{installed: true, active: true, rules: []string{"allow 22/tcp"}} + _, state, err := applyWith(t, adopted(t, `{"taken":[]}`, busOpening+","+withConf(dir)), store.State{}, u.run) + if err != nil { + t.Fatal(err) + } + converged := parse(t, `{"declaration":1,"resources":[`+withConf(dir)+`]}`) + _, state, err = applyWith(t, converged, state, u.run) + if err == nil || !strings.Contains(err.Error(), "table inet mesh") { + t.Fatalf("ufw was retired with nothing in its place: %v", err) + } + if !u.active || state.Firewall.DisabledByMesh { + t.Errorf("ufw was disabled: active %v, %+v", u.active, state.Firewall) + } + + // Once the table is loaded, the same declaration retires it. + u.ruleset = "table inet mesh\n" + if _, state, err = applyWith(t, converged, state, u.run); err != nil { + t.Fatal(err) + } + if u.active || !state.Firewall.DisabledByMesh { + t.Errorf("ufw was not retired once the mesh's filter was loaded: active %v, %+v", u.active, state.Firewall) + } +} diff --git a/internal/firewall/firewall.go b/internal/firewall/firewall.go index 8851659..91a0f7c 100644 --- a/internal/firewall/firewall.go +++ b/internal/firewall/firewall.go @@ -848,6 +848,30 @@ func Disable(ctx context.Context, run Runner, before map[string]string) error { return nil } +// MeshTable is the derived filter's table, the thing that must be in force before the firewall +// found on a machine is retired. +const MeshTable = "inet mesh" + +// MeshTableLoaded asks the machine whether the mesh's own filter is loaded. Read from the machine +// rather than assumed from the declaration: a table declared and not loaded is exactly the case +// where disabling the found firewall would leave the machine with nothing. +func MeshTableLoaded(ctx context.Context, run Runner) (bool, error) { + out, err := run(ctx, "nft", "list", "tables") + if err != nil { + if missing(err) { + return false, nil + } + return false, fmt.Errorf("cannot read which tables this machine has loaded: %w", err) + } + for _, line := range strings.Split(out, "\n") { + rest, ok := strings.CutPrefix(strings.TrimSpace(line), "table "+MeshTable) + if ok && (rest == "" || strings.HasPrefix(rest, " ") || strings.HasPrefix(rest, "{")) { + return true, nil + } + } + return false, 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.