diff --git a/cmd/cli/commands/network/firewall/firewall_test.go b/cmd/cli/commands/network/firewall/firewall_test.go index be381103..1ec3468c 100644 --- a/cmd/cli/commands/network/firewall/firewall_test.go +++ b/cmd/cli/commands/network/firewall/firewall_test.go @@ -28,6 +28,10 @@ func (c *captureRunner) List(_ context.Context) (string, error) { return "", nil func (c *captureRunner) Delete(_ context.Context) error { c.exists = false; return nil } func (c *captureRunner) Exists(_ context.Context) (bool, error) { return c.exists, nil } +// Check accepts every document: these tests exercise the CLI's flag handling, +// not nft's verdict on the rendered ruleset (that lives in internal/network/firewall). +func (c *captureRunner) Check(_ context.Context, _ string) error { return nil } + func TestFirewallCmd_Structure(t *testing.T) { cmd := GetCmd() require.Equal(t, "firewall", cmd.Use) @@ -162,7 +166,7 @@ func TestBackwardCompatibleInvocations(t *testing.T) { require.Contains(t, doc, "set mgmt_ports { type inet_service; flags interval; auto-merge; elements = { 2222 }; }", "--ssh-port must still work, as a one-element port list") require.Contains(t, doc, "elements = { 6443, 10250 }") - require.Contains(t, doc, "set in_cluster_addrs { type ipv4_addr; flags interval; elements = { 10.4.0.0/24 }; }") + require.Contains(t, doc, "set in_cluster_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 10.4.0.0/24 }; }") require.NoError(t, run(t, "add", "--mgmt-cidr", "192.168.1.0/24")) require.Contains(t, readFile(t, nftPath), "192.168.1.0/24") @@ -395,9 +399,13 @@ func TestCreateCmd_DefaultsInClusterPortsWhenNotPassed(t *testing.T) { origMgr, origDetect := newManager, detectPodCIDR newManager = func() *fw.Manager { return fw.NewManagerWithConfig(fw.Config{ - Runner: r, - NftPath: nftPath, - LockPath: filepath.Join(dir, ".applying"), + Runner: r, + NftPath: nftPath, + // Without this the config falls back to the production + // /etc/solo-provisioner path, so the test writes to the real host + // and only passes as root. + ConfigPath: filepath.Join(dir, "network-weaver-host-firewall.yaml"), + LockPath: filepath.Join(dir, ".applying"), ApplyViaService: func(context.Context) error { r.exists = true return nil diff --git a/docs/dev/traffic-shaper.md b/docs/dev/traffic-shaper.md index e7735d8b..df352a4a 100644 --- a/docs/dev/traffic-shaper.md +++ b/docs/dev/traffic-shaper.md @@ -355,9 +355,25 @@ where no external configuration management supplies one. A rule's addresses are one mixed-family list; `splitCIDRs` routes each entry to `@` (`ipv4_addr`) or `@6` (`ipv6_addr`) and the rule is emitted only into the chains whose -family has members. Port sets carry `flags interval` + `auto-merge`, which is what lets a range -be a single element — and also means the live set can read back merged differently from what was -written, so the persisted config, not the kernel, is the source of truth. +family has members. + +Every set in this table — addresses as well as ports — carries `flags interval` + `auto-merge`. +On a port set the interval flag is what lets a range be a single element. On an address set +auto-merge is what makes overlapping prefixes legal: without it, adding `10.0.0.5/32` to a set +already holding `10.0.0.0/24` makes nft reject the whole document with *conflicting intervals +specified*, which a plain `firewall add --cidr` can reach. The cost is that the live set reads +back merged differently from what was written, so the persisted config — not the kernel — is the +source of truth. `firewall show` dumps the kernel and will print folded prefixes; +`firewall show --output yaml` reads the config and shows what the operator authored. + +Because the kernel is not authoritative, a rejected ruleset must never reach disk either. Every +mutation renders the document, dry-runs it with `nft -c -f`, and only then writes +`network-weaver-host-firewall.yaml` and `.nft` and restarts the unit. The unit has no `ExecStop`, +so a failed load leaves the live table intact and looks harmless — but the persisted artifact is +what replays at boot, and an unloadable one means the host comes up with no weaver firewall at +all. Relatedly the unit sets `StartLimitIntervalSec=0`: it is restarted on every mutation, so +systemd's default start rate limit would otherwise turn one bad apply into an opaque +`start-limit-hit` on every later command until someone ran `systemctl reset-failed`. Two things stay structural and no rule can remove them: the IPv6 ND/MLD accepts with their hop-limit 255 guard (IPv6 is non-functional without them), and the ICMP rate meter. An diff --git a/docs/quickstart.md b/docs/quickstart.md index 1c2c300c..d8b57582 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -792,6 +792,10 @@ sudo solo-provisioner network firewall set --mgmt-cidrs 10.0.0.0/8 --in-clust > Ports are removed by exact spec: removing `2379` from a rule holding `2379-2380` does nothing. An nft range is a single set element, so replace the range with `set --ports` rather than relying on an implicit split. +> Adding a CIDR already covered by one in the rule — `10.0.0.5/32` into a rule holding `10.0.0.0/24` — is accepted. The config keeps both entries, so removing the wider prefix later leaves the narrower one in force, but the kernel folds them into one interval. `show` dumps the kernel and will print the folded form; `show --output yaml` reads the config and prints what you authored. +> +> A ruleset the kernel would refuse is rejected before anything is written: the CLI errors, and `/etc/solo-provisioner/network-weaver-host-firewall.{yaml,nft}` are left exactly as they were, so the ruleset that replays at boot is always one that loads. + #### Show / Delete the Host Firewall ```bash diff --git a/internal/network/firewall/allow_test.go b/internal/network/firewall/allow_test.go index d4c02f3c..5e22cac2 100644 --- a/internal/network/firewall/allow_test.go +++ b/internal/network/firewall/allow_test.go @@ -19,8 +19,8 @@ func TestRender_AllowRules(t *testing.T) { // Each rule gets its own per-family address sets and one port set. An allow // rule's address set is its bare name; only the reserved blocks carry the // `_addrs` suffix they shipped with. - require.Contains(t, doc, "set k8s-node { type ipv4_addr; flags interval; elements = { 10.0.0.0/24 }; }") - require.Contains(t, doc, "set k8s-node6 { type ipv6_addr; flags interval; }") + require.Contains(t, doc, "set k8s-node { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/24 }; }") + require.Contains(t, doc, "set k8s-node6 { type ipv6_addr; flags interval; auto-merge; }") // Port ranges are single elements of an interval set, ordered by lower bound // rather than lexically (10250 after 6443). @@ -59,7 +59,7 @@ func TestRender_AllowRuleFamilyScoping(t *testing.T) { // The sets themselves are always declared for both families, so adding a v6 // address to an existing rule needs no structural change. - require.Contains(t, doc, "set k8s-node6 { type ipv6_addr; flags interval; }") + require.Contains(t, doc, "set k8s-node6 { type ipv6_addr; flags interval; auto-merge; }") } // TestRender_ICMPEchoPrecedesRateMeter is the ordering pin the meter inversion diff --git a/internal/network/firewall/firewall_test.go b/internal/network/firewall/firewall_test.go index afb676da..bac57377 100644 --- a/internal/network/firewall/firewall_test.go +++ b/internal/network/firewall/firewall_test.go @@ -4,12 +4,14 @@ package firewall import ( "context" + "errors" "flag" "os" "path/filepath" "strings" "testing" + "github.com/hashgraph/solo-weaver/internal/templates" "github.com/stretchr/testify/require" ) @@ -25,12 +27,27 @@ type fakeRunner struct { exists bool listOut string deleted bool + // checkErr, when set, makes Check reject every document, standing in for an + // nft that refuses the rendered ruleset. + checkErr error + // checked records the documents Check was handed, so a test can assert the + // dry run saw the ruleset that was about to be persisted. + checked []string } func (f *fakeRunner) List(_ context.Context) (string, error) { return f.listOut, nil } func (f *fakeRunner) Delete(_ context.Context) error { f.deleted = true; f.exists = false; return nil } func (f *fakeRunner) Exists(_ context.Context) (bool, error) { return f.exists, nil } +func (f *fakeRunner) Check(_ context.Context, path string) error { + b, err := os.ReadFile(path) + if err != nil { + return err + } + f.checked = append(f.checked, string(b)) + return f.checkErr +} + func sampleTable() *Table { tbl := NewTable() tbl.Mgmt.CIDRs = []string{"10.0.0.0/8", "192.168.0.0/16"} @@ -129,7 +146,7 @@ func TestRender_SecurityInvariants(t *testing.T) { require.Contains(t, doc, "elements = { 4244, 6443, 7472, 10250 }") // The operator block list is a distinct set from the mgmt allowlist and // must be dropped before anything else, including established/related. - require.Contains(t, doc, "set blocked_addrs { type ipv4_addr; flags interval; elements = { 203.0.113.0/24 }; }") + require.Contains(t, doc, "set blocked_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 203.0.113.0/24 }; }") require.Contains(t, doc, "ip saddr @blocked_addrs drop") // ICMP is a static, safe ruleset: full ICMP from mgmt, and from everyone // else the path-health subset (PMTUD + traceroute) plus rate-limited echo. @@ -241,10 +258,10 @@ func TestRender_NoMgmtNoPod(t *testing.T) { // Empty mgmt set renders without an elements clause; no pod CIDR means no // in-cluster rule line. Both families' sets are always declared (dual-stack). - require.Contains(t, doc, "set mgmt_addrs { type ipv4_addr; flags interval; }") - require.Contains(t, doc, "set mgmt_addrs6 { type ipv6_addr; flags interval; }") - require.Contains(t, doc, "set blocked_addrs { type ipv4_addr; flags interval; }") - require.Contains(t, doc, "set blocked_addrs6 { type ipv6_addr; flags interval; }") + require.Contains(t, doc, "set mgmt_addrs { type ipv4_addr; flags interval; auto-merge; }") + require.Contains(t, doc, "set mgmt_addrs6 { type ipv6_addr; flags interval; auto-merge; }") + require.Contains(t, doc, "set blocked_addrs { type ipv4_addr; flags interval; auto-merge; }") + require.Contains(t, doc, "set blocked_addrs6 { type ipv6_addr; flags interval; auto-merge; }") require.NotContains(t, doc, "tcp dport @in_cluster_ports accept") } @@ -254,10 +271,10 @@ func TestRender_DualStack(t *testing.T) { // Each family's members land in its own set; mixed --mgmt/--blocked lists // are split by family, not smuggled into the wrong-typed set. - require.Contains(t, doc, "set mgmt_addrs { type ipv4_addr; flags interval; elements = { 10.0.0.0/8 }; }") - require.Contains(t, doc, "set mgmt_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:a11::/48 }; }") - require.Contains(t, doc, "set blocked_addrs { type ipv4_addr; flags interval; elements = { 203.0.113.0/24 }; }") - require.Contains(t, doc, "set blocked_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:bad::/48 }; }") + require.Contains(t, doc, "set mgmt_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/8 }; }") + require.Contains(t, doc, "set mgmt_addrs6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:a11::/48 }; }") + require.Contains(t, doc, "set blocked_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 203.0.113.0/24 }; }") + require.Contains(t, doc, "set blocked_addrs6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:bad::/48 }; }") // Parallel v6 match rules. require.Contains(t, doc, "ip6 saddr @blocked_addrs6 drop") @@ -488,8 +505,8 @@ func TestManager_AddAcceptsIPv6(t *testing.T) { require.NoError(t, m.Add(ctx, RuleMgmt, []string{"2001:db8:a11::/48"}, nil)) require.NoError(t, m.Add(ctx, RuleBlocked, []string{"2001:db8:bad::/48"}, nil)) doc := readNft(t, nftPath) - require.Contains(t, doc, "set mgmt_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:a11::/48 }; }") - require.Contains(t, doc, "set blocked_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:bad::/48 }; }") + require.Contains(t, doc, "set mgmt_addrs6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:a11::/48 }; }") + require.Contains(t, doc, "set blocked_addrs6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:bad::/48 }; }") } func TestTable_Validate_AcceptsIPv6(t *testing.T) { @@ -569,3 +586,167 @@ func TestRender_GoldenStable(t *testing.T) { require.NoError(t, err) require.Equal(t, strings.TrimSpace(string(want)), strings.TrimSpace(doc)) } + +// TestRender_OverlappingCIDRsShareAnAutoMergeSet pins the fix for #1002: the +// address sets must carry auto-merge, or nft rejects the whole document with +// "conflicting intervals specified" the moment one member covers another. +func TestRender_OverlappingCIDRsShareAnAutoMergeSet(t *testing.T) { + tbl := NewTable() + tbl.Mgmt.CIDRs = []string{"192.168.50.0/24"} + require.NoError(t, tbl.UpsertAllow(Rule{ + Name: "k8s-node", + CIDRs: []string{"10.0.0.0/24", "10.0.0.5/32"}, + Ports: []string{"6443"}, + })) + + doc, err := tbl.Render() + require.NoError(t, err) + require.Contains(t, doc, "set k8s-node { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/24, 10.0.0.5/32 }; }") + + // Every address set, not just the allow rules — an operator can put + // overlapping prefixes in the reserved blocks too. + for _, decl := range []string{ + "set mgmt_addrs { type ipv4_addr; flags interval; auto-merge;", + "set mgmt_addrs6 { type ipv6_addr; flags interval; auto-merge;", + "set blocked_addrs { type ipv4_addr; flags interval; auto-merge;", + "set blocked_addrs6 { type ipv6_addr; flags interval; auto-merge;", + "set in_cluster_addrs { type ipv4_addr; flags interval; auto-merge;", + "set in_cluster_addrs6 { type ipv6_addr; flags interval; auto-merge;", + "set k8s-node6 { type ipv6_addr; flags interval; auto-merge;", + } { + require.Contains(t, doc, decl) + } +} + +// TestManager_AddCoveredCIDR walks the exact repro from #1002: a rule already +// holding 10.0.0.0/24 takes 10.0.0.5/32. It must apply, and the config must keep +// both entries — the narrower prefix is operator intent that outlives the +// removal of the wider one, even though nft folds the two at load time. +func TestManager_AddCoveredCIDR(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + ctx := context.Background() + + tbl := sampleTable() + require.NoError(t, tbl.UpsertAllow(Rule{Name: "k8s-node", CIDRs: []string{"10.0.0.0/24"}, Ports: []string{"6443"}})) + _, err := m.Create(ctx, tbl, false) + require.NoError(t, err) + + require.NoError(t, m.Add(ctx, "k8s-node", []string{"10.0.0.5/32"}, nil)) + + doc := readNft(t, nftPath) + require.Contains(t, doc, "set k8s-node { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/24, 10.0.0.5/32 }; }") + + got, err := m.Table(ctx) + require.NoError(t, err) + rule, ok := got.Rule("k8s-node") + require.True(t, ok) + require.Equal(t, []string{"10.0.0.0/24", "10.0.0.5/32"}, rule.CIDRs) +} + +// TestManager_RejectedRulesetIsNeverPersisted is the other half of #1002: a +// document nft refuses must not reach disk. The live table is untouched by a +// failed apply (the unit has no ExecStop), so persisting first made the invalid +// ruleset the boot artifact and the host came up with no firewall at all. +func TestManager_RejectedRulesetIsNeverPersisted(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + configPath := filepath.Join(filepath.Dir(nftPath), "network-weaver-host-firewall.yaml") + ctx := context.Background() + + _, err := m.Create(ctx, sampleTable(), false) + require.NoError(t, err) + require.Equal(t, 1, applyCount) + + nftBefore := readNft(t, nftPath) + cfgBefore, err := os.ReadFile(configPath) + require.NoError(t, err) + + r.checkErr = errors.New("conflicting intervals specified") + err = m.Add(ctx, RuleMgmt, []string{"198.51.100.4/32"}, nil) + require.Error(t, err) + require.ErrorContains(t, err, "conflicting intervals specified") + require.ErrorContains(t, err, "nothing was written") + + require.Equal(t, nftBefore, readNft(t, nftPath), "the nft artifact must survive a rejected apply unchanged") + cfgAfter, err := os.ReadFile(configPath) + require.NoError(t, err) + require.Equal(t, cfgBefore, cfgAfter, "the config must survive a rejected apply unchanged") + require.Equal(t, 1, applyCount, "the service must not be restarted for a ruleset nft refused") +} + +// TestManager_ChecksTheDocumentItPersists guards the dry run against drifting +// out of lockstep with what actually lands on disk — a check of some other +// rendering would be worse than no check, since it would read as a guarantee. +func TestManager_ChecksTheDocumentItPersists(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + + _, err := m.Create(context.Background(), allowTable(), false) + require.NoError(t, err) + + require.Len(t, r.checked, 1) + require.Equal(t, readNft(t, nftPath), r.checked[0]) +} + +// TestNetworkNftUnit_HasNoStartLimit pins the third leg of #1002. Every firewall +// and policy mutation restarts this unit, so systemd's default start rate limit +// turns a run of failed applies into an opaque start-limit-hit on every later +// command until someone runs `systemctl reset-failed`. +func TestNetworkNftUnit_HasNoStartLimit(t *testing.T) { + content, err := templates.Files.ReadFile(networkNftServiceTemplate) + require.NoError(t, err) + require.Contains(t, string(content), "StartLimitIntervalSec=0") +} + +// TestIsRulesetDiagnostic pins the classification `Check` uses to tell a ruleset +// nft refused from a host that would not let it look. nft exits non-zero for +// both, so only the source position distinguishes them — and getting it wrong +// points the operator at their config when the real problem is permissions. +func TestIsRulesetDiagnostic(t *testing.T) { + const path = "/etc/solo-provisioner/.network-weaver-host-firewall-123.nft.check" + + for name, tc := range map[string]struct { + stderr string + want bool + }{ + // Verbatim nft 1.1.3 output for the overlapping-CIDR case in #1002. + "conflicting intervals": { + stderr: path + `:33:75-85: Error: conflicting intervals specified + set k8s-node { type ipv4_addr; flags interval; elements = { 10.0.0.0/24, 10.0.0.5/32 }; } + ~~~~~~~~~~~ ^^^^^^^^^^^`, + want: true, + }, + "syntax error": { + stderr: path + ":7:12-19: Error: syntax error, unexpected string", + want: true, + }, + // The cases that must NOT be reported as a bad ruleset. + "not permitted": { + stderr: "Error: Could not process rule: Operation not permitted", + want: false, + }, + "netlink failure": { + stderr: "Error: Could not process rule: Out of memory", + want: false, + }, + "empty stderr": {stderr: "", want: false}, + // A position for some other file is not a verdict on ours. + "diagnostic for another path": { + stderr: "/etc/solo-provisioner/other.nft:3:1-4: Error: syntax error", + want: false, + }, + // Naming the file without a position is a message, not a diagnostic. + "path mentioned without position": { + stderr: "Error: cannot open " + path + ": No such file or directory", + want: false, + }, + } { + t.Run(name, func(t *testing.T) { + require.Equal(t, tc.want, isRulesetDiagnostic(path, tc.stderr)) + }) + } +} diff --git a/internal/network/firewall/manager.go b/internal/network/firewall/manager.go index 68209733..81c4689d 100644 --- a/internal/network/firewall/manager.go +++ b/internal/network/firewall/manager.go @@ -269,16 +269,23 @@ func (m *Manager) mutateRule(ctx context.Context, name string, fn func(*Rule) er }) } -// applyAndPersist atomically rewrites the declarative config and the nft -// artifact, then restarts the systemd service via DBus so the kernel picks up -// the new rules. The rendered file contains the idempotent scoped-replace -// prefix, so it is safe for both the boot-time oneshot and live re-applies. +// applyAndPersist dry-runs the rendered ruleset, then atomically rewrites the +// declarative config and the nft artifact, then restarts the systemd service via +// DBus so the kernel picks up the new rules. The rendered file contains the +// idempotent scoped-replace prefix, so it is safe for both the boot-time oneshot +// and live re-applies. // -// The config is written first: it is what the next mutation loads, so a crash -// between the two writes leaves the operator's intent recorded and the kernel -// merely stale, which the next apply fixes. The reverse order would lose the -// intent while leaving the ruleset live, and there would be nothing left to -// re-derive it from. +// The dry run comes first, and nothing is written unless it passes. The unit has +// no ExecStop, so a ruleset nft refuses leaves the live table untouched and the +// failure looks harmless — but persisting first would have made that document +// the boot artifact, and the host would come up with no weaver firewall at all +// (#1002). Validating up front means a rejected ruleset never reaches disk. +// +// Past the dry run, the config is written before the nft artifact: it is what +// the next mutation loads, so a crash between the two writes leaves the +// operator's intent recorded and the kernel merely stale, which the next apply +// fixes. The reverse order would lose the intent while leaving the ruleset live, +// and there would be nothing left to re-derive it from. func (m *Manager) applyAndPersist(ctx context.Context, t *Table) error { block, err := t.Render() if err != nil { @@ -289,6 +296,11 @@ func (m *Manager) applyAndPersist(ctx context.Context, t *Table) error { if err != nil { return err } + + if err := m.check(ctx, block); err != nil { + return err + } + if err := atomicWriteFile(m.configPath, string(cfg), 0o600); err != nil { return err } @@ -300,6 +312,37 @@ func (m *Manager) applyAndPersist(ctx context.Context, t *Table) error { return m.applyViaService(ctx) } +// check dry-runs a rendered ruleset through nft without committing it. The +// document is staged in the nft artifact's own directory rather than the system +// temp dir, so the check runs on the same filesystem and permissions the real +// load will see, and is removed either way. +func (m *Manager) check(ctx context.Context, block string) error { + dir := filepath.Dir(m.nftPath) + if err := os.MkdirAll(dir, 0o755); err != nil { + return errorx.ExternalError.Wrap(err, "failed to create directory %s", dir) + } + + staged, err := os.CreateTemp(dir, ".network-weaver-host-firewall-*.nft.check") + if err != nil { + return errorx.ExternalError.Wrap(err, "failed to create temp file in %s", dir) + } + name := staged.Name() + defer func() { _ = os.Remove(name) }() + + if _, err := staged.WriteString(block); err != nil { + _ = staged.Close() + return errorx.ExternalError.Wrap(err, "failed to write temp file %s", name) + } + if err := staged.Close(); err != nil { + return errorx.ExternalError.Wrap(err, "failed to close temp file %s", name) + } + + if err := m.runner.Check(ctx, name); err != nil { + return errorx.Decorate(err, "the host firewall ruleset was not applied and nothing was written to %s or %s", m.configPath, m.nftPath) + } + return nil +} + // load returns the currently-configured table. The declarative config is the // source of truth; the rendered nft artifact is a fallback for a host // provisioned before the config file existed, or one that lost it. See Parse for diff --git a/internal/network/firewall/nft.go b/internal/network/firewall/nft.go index 44dae04d..aec3b956 100644 --- a/internal/network/firewall/nft.go +++ b/internal/network/firewall/nft.go @@ -5,6 +5,7 @@ package firewall import ( "bytes" "context" + "errors" "os" "os/exec" "strings" @@ -12,7 +13,7 @@ import ( "github.com/joomcode/errorx" ) -// Runner is the seam over the system `nft` binary for read and delete +// Runner is the seam over the system `nft` binary for read, check and delete // operations. Live rule application is done by writing the on-disk artifact // and restarting the systemd service via DBus — so Apply is not part of this // interface. Tests substitute a fake so the package builds and unit-tests on @@ -21,6 +22,10 @@ type Runner interface { // List returns the rendered ruleset for the inet weaver-host-firewall table // (`nft list table inet weaver-host-firewall`). List(ctx context.Context) (string, error) + // Check dry-runs a rendered ruleset file (`nft -c -f `) without + // committing it, so a document the kernel would reject is caught before it + // becomes the persisted boot artifact. + Check(ctx context.Context, path string) error // Delete removes the inet weaver-host-firewall table (`nft delete table inet weaver-host-firewall`). Delete(ctx context.Context) error // Exists reports whether the inet weaver-host-firewall table is present in the kernel. @@ -67,6 +72,52 @@ func (r *execRunner) List(ctx context.Context) (string, error) { return stdout.String(), nil } +// Check dry-runs the document at path. `-c` makes nft parse and evaluate the +// ruleset against the current kernel state without committing it, so this is +// the same verdict a real load would give without any of the side effects. +func (r *execRunner) Check(ctx context.Context, path string) error { + cmd := exec.CommandContext(ctx, r.bin, "-c", "-f", path) + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + msg := strings.TrimSpace(stderr.String()) + // nft exits non-zero for two unrelated reasons: a ruleset it parsed and + // refused, and an environment failure (no CAP_NET_ADMIN, a netlink error, + // a missing binary). Only the former reports a source position, so the + // `::` prefix — not the exit code — is what separates + // "the ruleset is wrong" from "the host is wrong". Treating every non-zero + // exit as malformed input would style a privilege error as bad input and + // point the operator at the ruleset instead of their permissions. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && isRulesetDiagnostic(path, msg) { + return errorx.IllegalFormat.New("nft rejected the rendered ruleset: %s", msg) + } + return errorx.ExternalError.Wrap(err, "failed to check the rendered ruleset with %s -c -f: %s", r.bin, msg) + } + return nil +} + +// isRulesetDiagnostic reports whether stderr carries an nft source position for +// path — the `::: Error: …` form nft prints when it has read the +// document and objected to its contents. Matching the position rather than the +// message keeps this independent of nft's wording across versions, and requiring +// the path to be the one we handed it stops a diagnostic about some other file +// (an `include`, say) from being read as a verdict on ours. +func isRulesetDiagnostic(path, stderr string) bool { + for line := range strings.SplitSeq(stderr, "\n") { + rest, ok := strings.CutPrefix(strings.TrimSpace(line), path+":") + if !ok { + continue + } + // nft follows the path with ":". A digit is enough to tell a + // position report from a message that merely mentions the file by name. + if rest != "" && rest[0] >= '0' && rest[0] <= '9' { + return true + } + } + return false +} + func (r *execRunner) Delete(ctx context.Context) error { cmd := exec.CommandContext(ctx, r.bin, append([]string{"delete", "table"}, tableArgs...)...) var stderr bytes.Buffer diff --git a/internal/network/firewall/service_linux.go b/internal/network/firewall/service_linux.go index 52d2f9a4..feea7c62 100644 --- a/internal/network/firewall/service_linux.go +++ b/internal/network/firewall/service_linux.go @@ -5,6 +5,7 @@ package firewall import ( + "bytes" "context" "os" "path/filepath" @@ -26,14 +27,26 @@ func defaultApplyViaService(ctx context.Context) error { } // EnsureNetworkNftUnit writes the embedded service unit file to -// NetworkNftServiceUnitPath if it is absent, then daemon-reloads and enables -// the unit for boot. Stat-and-skip so repeated calls are a fast no-op. +// NetworkNftServiceUnitPath, then daemon-reloads and enables the unit for boot. +// +// The on-disk unit is compared against the embedded copy, not merely stat-ed, so +// an already-provisioned host converges on the current unit the next time a +// mutation runs. Stat-and-skip would have stranded every existing host on the +// unit that shipped when it was first provisioned — including the missing +// StartLimitIntervalSec=0 that lets a run of failed applies wedge every later +// command behind systemd's start limit (#1002). An unchanged unit is still a +// fast no-op: no write, no daemon-reload. func EnsureNetworkNftUnit(ctx context.Context) error { - if _, err := os.Stat(NetworkNftServiceUnitPath); err == nil { - return nil // already installed — fast path + content, err := templates.Files.ReadFile(networkNftServiceTemplate) + if err != nil { + return errorx.InternalError.Wrap(err, "failed to read embedded %s", networkNftServiceTemplate) + } + + if current, err := os.ReadFile(NetworkNftServiceUnitPath); err == nil && bytes.Equal(current, content) { + return nil // already installed and current — fast path } - if err := writeEmbedded(networkNftServiceTemplate, NetworkNftServiceUnitPath); err != nil { + if err := writeEmbedded(content, NetworkNftServiceUnitPath); err != nil { return err } if err := soos.DaemonReload(ctx); err != nil { @@ -45,11 +58,7 @@ func EnsureNetworkNftUnit(ctx context.Context) error { return nil } -func writeEmbedded(tmplPath, destPath string) error { - content, err := templates.Files.ReadFile(tmplPath) - if err != nil { - return errorx.InternalError.Wrap(err, "failed to read embedded %s", tmplPath) - } +func writeEmbedded(content []byte, destPath string) error { if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { return errorx.ExternalError.Wrap(err, "failed to create %s", filepath.Dir(destPath)) } diff --git a/internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft b/internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft index 48b3d5d1..1f09aa42 100644 --- a/internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft +++ b/internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft @@ -2,27 +2,36 @@ add table inet weaver-host-firewall delete table inet weaver-host-firewall add table inet weaver-host-firewall table inet weaver-host-firewall { - set mgmt_addrs { type ipv4_addr; flags interval; elements = { 10.0.0.0/8 }; } - set mgmt_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:a11::/48 }; } - # `flags interval` + `auto-merge` is what lets a port set hold a range - # (2379-2380) as one element; a plain inet_service set rejects the range - # syntax outright. auto-merge also collapses adjacent entries, so the live - # set can read back differently from what was written — which is why the - # persisted YAML config, not the kernel, is this table's source of truth. + # Every set below is `flags interval` + `auto-merge`, addresses included. + # + # On an address set, `interval` is what admits a prefix at all, and + # `auto-merge` is what folds an overlapping prefix into the one that covers + # it. Without it, adding 10.0.0.5/32 to a set already holding 10.0.0.0/24 + # makes nft reject the entire document with "conflicting intervals + # specified" — which is reachable from a plain `firewall add --cidr`. + # + # On a port set, `interval` is what lets one element hold a range + # (2379-2380); a plain inet_service set rejects the range syntax outright. + # + # auto-merge collapses overlapping and adjacent entries, so the live set can + # read back differently from what was written — which is why the persisted + # YAML config, not the kernel, is this table's source of truth. + set mgmt_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/8 }; } + set mgmt_addrs6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:a11::/48 }; } set mgmt_ports { type inet_service; flags interval; auto-merge; elements = { 22 }; } - set blocked_addrs { type ipv4_addr; flags interval; elements = { 203.0.113.0/24 }; } - set blocked_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:bad::/48 }; } - set in_cluster_addrs { type ipv4_addr; flags interval; elements = { 10.4.0.0/24 }; } - set in_cluster_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:c0de::/64 }; } + set blocked_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 203.0.113.0/24 }; } + set blocked_addrs6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:bad::/48 }; } + set in_cluster_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 10.4.0.0/24 }; } + set in_cluster_addrs6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:c0de::/64 }; } set in_cluster_ports { type inet_service; flags interval; auto-merge; elements = { 4244, 6443, 7472, 10250 }; } - set admin { type ipv4_addr; flags interval; elements = { 203.0.113.5/32 }; } - set admin6 { type ipv6_addr; flags interval; elements = { 2001:db8:5e5::/64 }; } + set admin { type ipv4_addr; flags interval; auto-merge; elements = { 203.0.113.5/32 }; } + set admin6 { type ipv6_addr; flags interval; auto-merge; elements = { 2001:db8:5e5::/64 }; } set admin_ports { type inet_service; flags interval; auto-merge; elements = { 22 }; } - set cilium-vxlan { type ipv4_addr; flags interval; elements = { 10.0.0.0/24 }; } - set cilium-vxlan6 { type ipv6_addr; flags interval; } + set cilium-vxlan { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/24 }; } + set cilium-vxlan6 { type ipv6_addr; flags interval; auto-merge; } set cilium-vxlan_ports { type inet_service; flags interval; auto-merge; elements = { 8472 }; } - set k8s-node { type ipv4_addr; flags interval; elements = { 10.0.0.0/24 }; } - set k8s-node6 { type ipv6_addr; flags interval; } + set k8s-node { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/24 }; } + set k8s-node6 { type ipv6_addr; flags interval; auto-merge; } set k8s-node_ports { type inet_service; flags interval; auto-merge; elements = { 2379-2380, 6443, 10250, 10256-10259 }; } # Operator block list, dropped as early as the packet can be seen. Priority diff --git a/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft b/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft index 428a6009..76b1c015 100644 --- a/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft +++ b/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft @@ -2,18 +2,27 @@ add table inet weaver-host-firewall delete table inet weaver-host-firewall add table inet weaver-host-firewall table inet weaver-host-firewall { - set mgmt_addrs { type ipv4_addr; flags interval; elements = { 10.0.0.0/8, 192.168.0.0/16 }; } - set mgmt_addrs6 { type ipv6_addr; flags interval; } - # `flags interval` + `auto-merge` is what lets a port set hold a range - # (2379-2380) as one element; a plain inet_service set rejects the range - # syntax outright. auto-merge also collapses adjacent entries, so the live - # set can read back differently from what was written — which is why the - # persisted YAML config, not the kernel, is this table's source of truth. + # Every set below is `flags interval` + `auto-merge`, addresses included. + # + # On an address set, `interval` is what admits a prefix at all, and + # `auto-merge` is what folds an overlapping prefix into the one that covers + # it. Without it, adding 10.0.0.5/32 to a set already holding 10.0.0.0/24 + # makes nft reject the entire document with "conflicting intervals + # specified" — which is reachable from a plain `firewall add --cidr`. + # + # On a port set, `interval` is what lets one element hold a range + # (2379-2380); a plain inet_service set rejects the range syntax outright. + # + # auto-merge collapses overlapping and adjacent entries, so the live set can + # read back differently from what was written — which is why the persisted + # YAML config, not the kernel, is this table's source of truth. + set mgmt_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 10.0.0.0/8, 192.168.0.0/16 }; } + set mgmt_addrs6 { type ipv6_addr; flags interval; auto-merge; } set mgmt_ports { type inet_service; flags interval; auto-merge; elements = { 22 }; } - set blocked_addrs { type ipv4_addr; flags interval; elements = { 203.0.113.0/24 }; } - set blocked_addrs6 { type ipv6_addr; flags interval; } - set in_cluster_addrs { type ipv4_addr; flags interval; elements = { 10.4.0.0/24 }; } - set in_cluster_addrs6 { type ipv6_addr; flags interval; } + set blocked_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 203.0.113.0/24 }; } + set blocked_addrs6 { type ipv6_addr; flags interval; auto-merge; } + set in_cluster_addrs { type ipv4_addr; flags interval; auto-merge; elements = { 10.4.0.0/24 }; } + set in_cluster_addrs6 { type ipv6_addr; flags interval; auto-merge; } set in_cluster_ports { type inet_service; flags interval; auto-merge; elements = { 4244, 6443, 7472, 10250 }; } # Operator block list, dropped as early as the packet can be seen. Priority diff --git a/internal/network/policy/service_linux.go b/internal/network/policy/service_linux.go index 2325dce7..a35a7464 100644 --- a/internal/network/policy/service_linux.go +++ b/internal/network/policy/service_linux.go @@ -5,6 +5,7 @@ package policy import ( + "bytes" "context" "os" "path/filepath" @@ -15,23 +16,28 @@ import ( "github.com/joomcode/errorx" ) -// defaultEnsureService installs the shared network-nft unit if absent and -// enables it for boot. The unit is shared with internal/network/firewall; -// installing it here is idempotent with firewall's own install so whichever -// scope runs first wins. +// defaultEnsureService installs the shared network-nft unit and enables it for +// boot. The unit is shared with internal/network/firewall; installing it here is +// idempotent with firewall's own install so whichever scope runs first wins. +// +// The on-disk unit is compared against the embedded copy rather than merely +// stat-ed, so a host provisioned by an older release converges on the current +// unit the next time either plane runs. Stat-and-skip would have stranded those +// hosts on whatever unit shipped when they were first provisioned. // // This package does NOT restart the unit to apply — `network policy` applies to // the live kernel directly via `nft -f` (Runner.Apply). The unit only matters // for boot replay. func defaultEnsureService(ctx context.Context) error { - if _, err := os.Stat(NetworkNftServiceUnitPath); err == nil { - return nil // already installed by firewall or a prior policy call - } - content, err := templates.Files.ReadFile(networkNftServiceTemplate) if err != nil { return errorx.InternalError.Wrap(err, "failed to read embedded %s", networkNftServiceTemplate) } + + if current, err := os.ReadFile(NetworkNftServiceUnitPath); err == nil && bytes.Equal(current, content) { + return nil // already installed by firewall or a prior policy call + } + if err := os.MkdirAll(filepath.Dir(NetworkNftServiceUnitPath), 0o755); err != nil { return errorx.ExternalError.Wrap(err, "failed to create %s", filepath.Dir(NetworkNftServiceUnitPath)) } diff --git a/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl b/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl index ff28f1bb..664b0c64 100644 --- a/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl +++ b/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl @@ -2,22 +2,31 @@ add table inet weaver-host-firewall delete table inet weaver-host-firewall add table inet weaver-host-firewall table inet weaver-host-firewall { - set mgmt_addrs { type ipv4_addr; flags interval;{{if .Mgmt.Elements}} elements = { {{.Mgmt.Elements}} };{{end}} } - set mgmt_addrs6 { type ipv6_addr; flags interval;{{if .Mgmt.Elements6}} elements = { {{.Mgmt.Elements6}} };{{end}} } - # `flags interval` + `auto-merge` is what lets a port set hold a range - # (2379-2380) as one element; a plain inet_service set rejects the range - # syntax outright. auto-merge also collapses adjacent entries, so the live - # set can read back differently from what was written — which is why the - # persisted YAML config, not the kernel, is this table's source of truth. + # Every set below is `flags interval` + `auto-merge`, addresses included. + # + # On an address set, `interval` is what admits a prefix at all, and + # `auto-merge` is what folds an overlapping prefix into the one that covers + # it. Without it, adding 10.0.0.5/32 to a set already holding 10.0.0.0/24 + # makes nft reject the entire document with "conflicting intervals + # specified" — which is reachable from a plain `firewall add --cidr`. + # + # On a port set, `interval` is what lets one element hold a range + # (2379-2380); a plain inet_service set rejects the range syntax outright. + # + # auto-merge collapses overlapping and adjacent entries, so the live set can + # read back differently from what was written — which is why the persisted + # YAML config, not the kernel, is this table's source of truth. + set mgmt_addrs { type ipv4_addr; flags interval; auto-merge;{{if .Mgmt.Elements}} elements = { {{.Mgmt.Elements}} };{{end}} } + set mgmt_addrs6 { type ipv6_addr; flags interval; auto-merge;{{if .Mgmt.Elements6}} elements = { {{.Mgmt.Elements6}} };{{end}} } set mgmt_ports { type inet_service; flags interval; auto-merge;{{if .Mgmt.PortElements}} elements = { {{.Mgmt.PortElements}} };{{end}} } - set blocked_addrs { type ipv4_addr; flags interval;{{if .Blocked.Elements}} elements = { {{.Blocked.Elements}} };{{end}} } - set blocked_addrs6 { type ipv6_addr; flags interval;{{if .Blocked.Elements6}} elements = { {{.Blocked.Elements6}} };{{end}} } - set in_cluster_addrs { type ipv4_addr; flags interval;{{if .InCluster.Elements}} elements = { {{.InCluster.Elements}} };{{end}} } - set in_cluster_addrs6 { type ipv6_addr; flags interval;{{if .InCluster.Elements6}} elements = { {{.InCluster.Elements6}} };{{end}} } + set blocked_addrs { type ipv4_addr; flags interval; auto-merge;{{if .Blocked.Elements}} elements = { {{.Blocked.Elements}} };{{end}} } + set blocked_addrs6 { type ipv6_addr; flags interval; auto-merge;{{if .Blocked.Elements6}} elements = { {{.Blocked.Elements6}} };{{end}} } + set in_cluster_addrs { type ipv4_addr; flags interval; auto-merge;{{if .InCluster.Elements}} elements = { {{.InCluster.Elements}} };{{end}} } + set in_cluster_addrs6 { type ipv6_addr; flags interval; auto-merge;{{if .InCluster.Elements6}} elements = { {{.InCluster.Elements6}} };{{end}} } set in_cluster_ports { type inet_service; flags interval; auto-merge;{{if .InCluster.PortElements}} elements = { {{.InCluster.PortElements}} };{{end}} } {{- range .Allow}} - set {{.AddrSet}} { type ipv4_addr; flags interval;{{if .Elements}} elements = { {{.Elements}} };{{end}} } - set {{.AddrSet6}} { type ipv6_addr; flags interval;{{if .Elements6}} elements = { {{.Elements6}} };{{end}} } + set {{.AddrSet}} { type ipv4_addr; flags interval; auto-merge;{{if .Elements}} elements = { {{.Elements}} };{{end}} } + set {{.AddrSet6}} { type ipv6_addr; flags interval; auto-merge;{{if .Elements6}} elements = { {{.Elements6}} };{{end}} } {{- if .HasPorts}} set {{.PortsSet}} { type inet_service; flags interval; auto-merge; elements = { {{.PortElements}} }; } {{- end}} diff --git a/internal/templates/files/network/solo-provisioner-network-nft.service b/internal/templates/files/network/solo-provisioner-network-nft.service index 1d79e2fa..1ce3aa0e 100644 --- a/internal/templates/files/network/solo-provisioner-network-nft.service +++ b/internal/templates/files/network/solo-provisioner-network-nft.service @@ -4,6 +4,12 @@ Documentation=https://github.com/hashgraph/solo-weaver DefaultDependencies=no After=local-fs.target Before=solo-provisioner-daemon.service +# Every `network firewall` / `network policy` mutation restarts this unit, so a +# run of failed applies would otherwise trip systemd's default start rate limit +# and leave every later command failing with an opaque "start-limit-hit" that +# outlives the problem — until someone runs `systemctl reset-failed`. There is +# nothing to protect here: the unit is a oneshot that loads a file and exits. +StartLimitIntervalSec=0 [Service] Type=oneshot diff --git a/internal/workflows/steps/step_network_firewall_test.go b/internal/workflows/steps/step_network_firewall_test.go index ef5d72ec..a33df0fb 100644 --- a/internal/workflows/steps/step_network_firewall_test.go +++ b/internal/workflows/steps/step_network_firewall_test.go @@ -26,6 +26,10 @@ func (f *fakeFwRunner) List(context.Context) (string, error) { return "", nil } func (f *fakeFwRunner) Delete(context.Context) error { f.deleted = true; f.exists = false; return nil } func (f *fakeFwRunner) Exists(context.Context) (bool, error) { return f.exists, nil } +// Check accepts every document: the step test covers step wiring, not nft's +// verdict on the rendered ruleset (that lives in internal/network/firewall). +func (f *fakeFwRunner) Check(context.Context, string) error { return nil } + // withStubbedFirewall points newFirewallManager at a manager wired to the given // fake runner, temp paths, and a no-op service apply, restoring it on cleanup. // It returns the on-disk nft path so tests can assert the artifact was written.