feat(network/firewall): support named allow rules and a declarative config file - #999
Conversation
…onfig file The host firewall rendered exactly three hardcoded bindings, with no UDP, no port ranges, one management group on a single scalar port, and no way to grant unmetered ICMP echo to a named set of sources. That is not enough to express a complete host ruleset, which weaver now has to do on operator-managed hardware where no external configuration management supplies one. Generalise to named allow rules over three reserved blocks. `mgmt`, `blocked` and `in_cluster` stay first-class because weaver derives or defaults their content and omitting one is dangerous; everything else is an operator-authored source list x port list x protocol accept, rendered per family as `<family> saddr @<name> <proto> dport @<name>_ports accept`. Port sets gain `flags interval` + `auto-merge` so a range is a single element, and `mgmt.ports` becomes a list with `--ssh-port` as sugar for a one-element one. Structure is declared in a YAML config file; membership is mutable from the CLI via `--name`, which reaches a reserved block and an allow rule alike. That file is also the persisted state and the output of `show --output yaml`, so the round-trip is exact by construction rather than by test. It replaces the rendered ruleset as the source of truth for the mutating verbs, since `auto-merge` means the kernel can read back merged differently from what was written; `Parse` stays as a fallback that recovers the reserved blocks from a pre-existing artifact, so a host that lost its config never loses management access. Every pre-existing invocation keeps working: the per-block flags are retained as shorthands that name their reserved block implicitly, bare `delete` still means `--all`, and a regression test drives each older form end to end. Two asymmetries are deliberate and worth stating. `allow:` is declarative — an entry absent from an applied file is deleted — while a reserved block absent from it is defaulted rather than removed, so a partial file cannot silently drop management access. And an `icmp_echo` rule renders above the rate meter, because the meter drops over-budget echo outright and an accept below it would never be reached under a flood. Both goldens were verified against nftables 1.1.3: they load, re-apply idempotently, and preserve `2379-2380` as a single range element. Signed-off-by: Bruno Marques <bruno.marques@hashgraph.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Bruno Marques <bruno.marques@swirldslabs.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Pull request overview
This PR generalizes the node-level host firewall (internal/network/firewall) from a few hardcoded bindings into a model that supports (1) three reserved blocks (mgmt, blocked, in_cluster) plus (2) operator-declared, named allow rules, with a strict declarative YAML config as the source of truth for mutations and round-trippable show --output yaml.
Changes:
- Introduces a new firewall rule/table model with protocol + port-range support and named allow rules, plus strict YAML config load/round-trip support.
- Updates rendering/template + goldens to emit per-rule sets (including interval/auto-merge port sets) and
icmp_echoaccepts above the ICMP rate meter. - Extends the CLI and workflow wiring to manage rules by name and persist/load the YAML config, including tests to preserve allow rules across re-renders.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/workflows/steps/step_network_firewall.go | Preserves existing named allow rules across workflow-driven re-renders. |
| internal/workflows/steps/step_network_firewall_test.go | Adds coverage to ensure named allow rules survive reconcile re-render. |
| internal/templates/files/network/network-weaver-host-firewall.nft.tmpl | Renders per-rule address/port sets, allow rules, and icmp_echo ordering. |
| internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft | Updates golden for new reserved-block set layout and mgmt ports set. |
| internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft | Adds golden covering named allow rules, UDP, port ranges, and icmp_echo. |
| internal/network/firewall/table.go | Refactors Table to reserved Rule blocks + Allow list; adds validation and collisions checks. |
| internal/network/firewall/rule.go | New Rule/Proto model, reserved names, port-spec parsing/sorting, and per-rule validation. |
| internal/network/firewall/render.go | Flattens rules for template rendering (per-family split, per-rule sets, allow list). |
| internal/network/firewall/paths.go | Adds HostConfigPath for persisted declarative YAML config. |
| internal/network/firewall/parse.go | Updates fallback parsing to recover reserved blocks from legacy/current nft artifacts. |
| internal/network/firewall/manager.go | Adds YAML config persistence/load as source of truth; exposes Config/Table; adds name-based mutators. |
| internal/network/firewall/firewall_test.go | Updates existing tests for new model and adds manager-level behavior tests. |
| internal/network/firewall/config.go | New strict YAML schema, versioning, config↔table conversion, and marshaling helpers. |
| internal/network/firewall/allow_test.go | New tests for allow-rule rendering, ordering, config round-trip, and legacy fallback behaviors. |
| docs/quickstart.md | Documents new declarative config workflow, --from-file, --name, and YAML output. |
| docs/dev/traffic-shaper.md | Documents new host-firewall config source of truth and named allow rules behavior. |
| cmd/cli/commands/network/firewall/show.go | Adds --output yaml (config view) and --name (single-rule view). |
| cmd/cli/commands/network/firewall/set.go | Reworks set to operate by --name (or legacy per-block flags) and adds --cidrs-file. |
| cmd/cli/commands/network/firewall/remove.go | Reworks remove to operate by --name + --cidr/--port (plus legacy shorthands). |
| cmd/cli/commands/network/firewall/firewall.go | Adds shared name-addressed flag plumbing and target-resolution helper logic. |
| cmd/cli/commands/network/firewall/firewall_test.go | Adds end-to-end CLI tests for backward compatibility, from-file, yaml round-trip, and targeting. |
| cmd/cli/commands/network/firewall/delete.go | Adds delete-by-name for allow rules and interactive confirmation for delete-all. |
| cmd/cli/commands/network/firewall/create.go | Adds --from-file path and refactors build logic + podCIDR auto-detection behavior. |
| cmd/cli/commands/network/firewall/add.go | Reworks add to operate by --name + --cidr/--port (plus legacy shorthands). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Named allow rules are not part of this step's input: they are | ||
| // declared with `network firewall create --from-file`, and config.yaml | ||
| // has no field for them. Carry any that already exist across, or a | ||
| // reconfigure (which force re-renders) would silently drop the | ||
| // operator's k8s/Cilium/admin rules while appearing to succeed. | ||
| if existing, err := mgr.Table(ctx); err == nil { | ||
| t.Allow = existing.Allow | ||
| } |
| for _, r := range t.rules() { | ||
| for _, setName := range []string{addrSetName(r.Name), v6SetName(r.Name), portsSetName(r.Name)} { | ||
| if err := claim(setName, r.Name); err != nil { | ||
| return err | ||
| } |
| nft, err := os.ReadFile(m.nftPath) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, errorx.IllegalState.New("inet weaver-host-firewall firewall not found at %s; run `solo-provisioner network firewall create` first", m.nftPath) | ||
| return nil, errorx.IllegalState.New("inet weaver-host-firewall firewall not found at %s; run `solo-provisioner network firewall create` first", m.configPath) | ||
| } |
| - name: admin | ||
| cidrs: ["203.0.113.5/32", "2001:db8:5e5::/64"] | ||
| ports: ["22"] | ||
| icmp_echo: true |
There was a problem hiding this comment.
Make sure icmp_echo should be false by default.
There was a problem hiding this comment.
confirmed, it is false by default. Rule.ICMPEcho is a plain bool (rule.go:67) with yaml:"icmp_echo,omitempty", so it is false unless an allow rule opts in
| ```yaml | ||
| version: 1 | ||
|
|
||
| mgmt: |
There was a problem hiding this comment.
when this yaml file is specified, mgmt, blocked and in_cluster should be mandatory. Is that the case?
In other words, when using the file, everything should be sourced from it instead of trying to merge existing values (which only applies to set)
There was a problem hiding this comment.
makes sense, I'll implement this feature to make "mgmt, blocked and in_cluster" mandatory
| # Remove the table and /etc/solo-provisioner/network-weaver-host-firewall.nft | ||
| sudo solo-provisioner network firewall delete | ||
| # Show the declarative config the ruleset was rendered from | ||
| sudo solo-provisioner network firewall show --output yaml |
There was a problem hiding this comment.
is show displaying the same format that was passed via --from-file or the contents of the nftable? I believe it should be the format of the --from-file which would help when we want to export those values to apply them again later. Let's check if it's worth the complexity.
There was a problem hiding this comment.
confirmed show --output yaml already prints exactly the --from-file schema
| sudo solo-provisioner network firewall delete --all | ||
| ``` | ||
|
|
||
| `show --output yaml` prints exactly the schema `create --from-file` accepts, so it round-trips: |
There was a problem hiding this comment.
what if the --output is not provided? Does it print the nftable format? If not, should we have an output type for that?
There was a problem hiding this comment.
Default is nft (show.go:87), which prints the live kernel ruleset via nft list table inet weaver-host-firewall
…config `--from-file` states the whole table — nothing is inherited from the host's current firewall — but an omitted reserved block still fell back to a compiled-in default. For `mgmt` that default is an empty address list under the input chain's default drop, so a file that simply forgot the block rendered a host with no management allowlist and reported success, with only a log warning to say otherwise. Require all three reserved blocks (`mgmt`, `blocked`, `in_cluster`) to be present, and `cidrs` to be present inside `mgmt` and `blocked`. Omitting `in_cluster.cidrs` still means "auto-detect this node's pod CIDR" — the one address list weaver can legitimately derive, and one whose absence costs a rule rather than access to the host. The check runs in `ParseConfig`, so it covers the persisted config at /etc/solo-provisioner/network-weaver-host-firewall.yaml as well as `--from-file`. `FileConfigFromTable` always writes all three blocks, so a file weaver wrote passes by construction; one that fails has been truncated or hand-edited, and failing loudly beats loading it with a defaulted management allowlist. Raised by @brunodam in review of #999. Signed-off-by: alex-au <alex.w.aus@gmail.com>
Manual UAT on a local VMRan the host firewall end to end on a Debian 13 / arm64 UTM VM (nftables v1.1.3, kernel 11 of 13 planned cases pass. 1 pre-existing bug found and filed as #1002.
What each case actually confirmed1 — every rule kind renders correctly. One Re-applying the identical file produced a byte-identical 2 — the required-block rule added in 3 — round-trip. 8 — delete. 12 — recovery. Deleting the persisted YAML and running 13 — boot persistence. Rebooted the VM; Bug found: #1002 (pre-existing, not introduced here)Adding a CIDR already covered by an existing member of the same rule breaks the apply: $ sudo solo-provisioner network firewall add --name k8s-node --cidr 10.0.0.5/32
Error: service solo-provisioner-network-nft.service start failed
# journalctl:
network-weaver-host-firewall.nft:24: Error: conflicting intervals specified
set k8s-node { type ipv4_addr; flags interval; elements = { 10.0.0.0/24, 10.0.0.5/32 }; }The address sets are declared Two things about it are worth a reviewer's attention anyway:
Also worth knowing while testing: the repeated failed starts trip systemd's start-limit, after Why the existing tests missed itThe unit suite asserts on rendered strings, and Still open
|
UAT-11 now covered end to end — the
|
2cbfbdf
into
00996-collapse-icmp-path-health-accepts
Description
The host firewall rendered exactly three hardcoded bindings.
internal/network/firewall/table.gocontained no
udp, nointervaland noauto-merge, so there was no UDP, no port ranges, onemanagement group on a single scalar port, and no way to grant unmetered ICMP echo to a named set of
sources. That is not enough to express a complete host ruleset, which weaver now has to do on
operator-managed hardware where no external configuration management supplies one.
This generalises the table to named allow rules over three reserved blocks.
mgmt,blockedandin_clusterstay first-class because weaver derives or defaults their content and omitting one isdangerous — an empty mgmt list locks the operator out, an absent in-cluster list breaks the cluster,
and the block list renders on three hooks rather than one. Everything else is an operator-authored
source list x port list x protocol accept:
Structure is file-only; membership is CLI-mutable. Adding a rule is a reviewed change, while
unblocking an operator is sometimes urgent. The same YAML is the
--from-fileinput, the persistedstate, and the output of
show --output yaml, so the round-trip is exact by construction rather thanby test.
Config file
Deliberate decisions a reviewer should weigh
allow:is declarative; the reserved blocks are required. Anallowentry absent from an appliedfile is deleted. A reserved block absent from it is an error, not a silent default — the earlier
revision of this PR defaulted it, and for
mgmtthat default is an empty address list under theinput chain's default drop, i.e. exactly the lockout the asymmetry was meant to prevent.
cidrsisrequired inside
mgmtandblockedfor the same reason. Omittingin_cluster.cidrsstill means"auto-detect this node's pod CIDR" and
in_cluster: {cidrs: []}still means "render no rule", whichis why nil-vs-empty on a decoded slice is load-bearing and has its own test. Raised by @brunodam in
review.
The YAML config replaces the rendered ruleset as the source of truth for the mutating verbs.
auto-mergeis required for port ranges and also collapses adjacent entries, so the kernel can readback differently from what was written — re-deriving intent from the ruleset would be lossy.
Parsestays as a fallback that recovers the reserved blocks from a pre-existing artifact (both the old and
new renderings), so a host that lost its config file still yields its management allowlist rather than
an error leaving the operator no way back in. Named allow rules are deliberately not
reverse-engineered from nft syntax: losing them is recoverable, losing management access is not.
One state file, not one JSON per record — a deviation from what #998 suggested. A change to the
management allowlist must be all-or-nothing, and a partial write across several files could leave a
host reachable by nobody. Happy to split it if you prefer the
policyregistry shape.create --from-filefollows create-if-missing, like every othercreatein this repo, sore-applying an edited file needs
--force. The existing warning names the flag explicitly, so this isfriction rather than silence — but it is the one call here I could see going the other way.
icmp_echorenders above the rate meter. Post-#997 the meter islimit rate over … drop, so anaccept placed below it would never be reached under a flood — exactly when an operator needs their own
ping to work. Pinned by an ordering test in both families.
Reserved blocks keep their shipped set names (
mgmt_addrs,in_cluster_ports) while allow rulesuse the bare name, matching the workload plane's
@bn-publisher. All three derivations go through onefunction, and
Table.Validaterejects collisions — an allow rule namedmgmt_addrswould claim themgmt block's set, and one named
k8s6would claim the v6 set ofk8s. nft accepts a duplicate setdeclaration silently and merges the membership, so this has to be caught before render.
Backward compatibility
Every pre-existing invocation behaves identically. The per-block flags are retained as shorthands that
name their reserved block implicitly,
setstill accepts several of them in one call (as one nfttransaction), and bare
deletestill means--all.TestBackwardCompatibleInvocationsdrives eacholder form end to end.
One behaviour change worth flagging:
delete --allnow asks for confirmation in an interactivesession. Non-interactive callers are unaffected (
prompt.ShouldPromptis false), and--forceskips it.Files changed
internal/network/firewall/rule.goRule,Proto, reserved names, port-spec parsing/ordering, derived set naminginternal/network/firewall/config.goFileConfigschema, strict YAML load, table conversion both waysinternal/network/firewall/table.goTablebecomes three reservedRules plusAllow; name lookup, upsert/delete, collision checkinternal/network/firewall/render.gointernal/network/firewall/manager.goAdd/Remove/Set/SetMany/DeleteRule/Apply/Tableinternal/network/firewall/parse.gointernal/templates/files/.../network-weaver-host-firewall.nft.tmplicmp_echoabove the metercmd/cli/commands/network/firewall/*.go--name,--from-file,--output yaml,--all,--cidrs-fileinternal/workflows/steps/step_network_firewall.godocs/quickstart.md,docs/dev/traffic-shaper.mdReview guide
Start with
rule.goandtable.go(the model), then the template, thenmanager.go'sload/applyAndPersistordering. The CLI is mechanical by comparison.The bug worth checking I actually fixed:
step_network_firewall.goforce re-renders onblock node reconfigure, andconfig.yamlhas no field for allow rules — so a reconfigure would havewiped every operator-authored k8s/Cilium/admin rule while reporting success. The step now carries them
across.
TestNetworkFirewallCreate_PreservesNamedAllowRulescovers it.Real-kernel verification — string tests cannot tell you whether
nftaccepts the document:Both goldens load and re-apply idempotently on nftables 1.1.3, dual-stack, with the range preserved as
a single element:
The full unit suite passes on Linux (
go test ./internal/... ./pkg/... ./cmd/...aftertask mocks).The
network firewallCLI package is Linux-only viainternal/mount, so it does not run on macOS.Manual UAT on the VM harness:
Stacking note
Based on
00996-collapse-icmp-path-health-accepts(#997), notmain: that PR inverts the ICMP ratemeter and rewrites the chains this one inserts into, and the
icmp_echoplacement only makes senseagainst the inverted form. Merge #997 first, then retarget this to
main.Out of scope
Called out in #998 and unchanged here: block-node service ports (that traffic is forwarded, so an
inputrule for it never matches), confirm-or-roll-back apply, migration of externally-managed hosts,cluster-derived k8s/Cilium rules, and
groups:address sugar.Of those, confirm-or-roll-back apply is the one that should land before this is relied on as the only
host firewall on hardware with no console — a bad render is otherwise a truck roll.
Related Issues