From 9f4a902c50a6b8071fba38326f0d579dd014a0c0 Mon Sep 17 00:00:00 2001 From: Bruno Marques Date: Wed, 12 Aug 2026 19:18:07 +1000 Subject: [PATCH 1/2] feat(network/firewall): support named allow rules and a declarative config file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` saddr @ dport @_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 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Bruno Marques --- cmd/cli/commands/network/firewall/add.go | 29 +- cmd/cli/commands/network/firewall/create.go | 158 +++--- cmd/cli/commands/network/firewall/delete.go | 57 ++- cmd/cli/commands/network/firewall/firewall.go | 81 +++- .../network/firewall/firewall_test.go | 302 +++++++++++- cmd/cli/commands/network/firewall/remove.go | 28 +- cmd/cli/commands/network/firewall/set.go | 139 +++++- cmd/cli/commands/network/firewall/show.go | 68 ++- docs/dev/traffic-shaper.md | 60 ++- docs/quickstart.md | 137 +++++- internal/network/firewall/allow_test.go | 451 ++++++++++++++++++ internal/network/firewall/config.go | 198 ++++++++ internal/network/firewall/firewall_test.go | 224 ++++++--- internal/network/firewall/manager.go | 187 ++++++-- internal/network/firewall/parse.go | 127 ++--- internal/network/firewall/paths.go | 11 + internal/network/firewall/render.go | 118 ++--- internal/network/firewall/rule.go | 374 +++++++++++++++ internal/network/firewall/table.go | 341 +++++++------ ...work-weaver-host-firewall-allow.golden.nft | 185 +++++++ .../network-weaver-host-firewall.golden.nft | 23 +- .../network-weaver-host-firewall.nft.tmpl | 70 ++- .../workflows/steps/step_network_firewall.go | 24 +- .../steps/step_network_firewall_test.go | 40 ++ 24 files changed, 2835 insertions(+), 597 deletions(-) create mode 100644 internal/network/firewall/allow_test.go create mode 100644 internal/network/firewall/config.go create mode 100644 internal/network/firewall/rule.go create mode 100644 internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft diff --git a/cmd/cli/commands/network/firewall/add.go b/cmd/cli/commands/network/firewall/add.go index 4d5856d8..cc96f4ed 100644 --- a/cmd/cli/commands/network/firewall/add.go +++ b/cmd/cli/commands/network/firewall/add.go @@ -3,31 +3,30 @@ package firewall import ( - "github.com/joomcode/errorx" "github.com/spf13/cobra" ) var addCmd = &cobra.Command{ Use: "add", - Short: "Add a single --mgmt-cidr, --blocked-cidr, or --in-cluster-port", + Short: "Add CIDRs and/or ports to a rule (--name), merging with what is already there", + Long: "Add addresses and/or ports to one rule of the host firewall. --name selects the rule: a reserved block " + + "(mgmt, blocked, in_cluster) or a named allow rule. Adding is idempotent — an entry already present is " + + "left alone.\n\n" + + "The --mgmt-cidr, --blocked-cidr and --in-cluster-port flags are retained shorthands that name their " + + "reserved block implicitly.", RunE: func(cmd *cobra.Command, _ []string) error { - mgr := newManager() - switch { - case cmd.Flags().Changed("mgmt-cidr"): - return mgr.AddMgmtCIDR(cmd.Context(), flagMgmtCIDR) - case cmd.Flags().Changed("blocked-cidr"): - return mgr.AddBlockedCIDR(cmd.Context(), flagBlockedCIDR) - case cmd.Flags().Changed("in-cluster-port"): - return mgr.AddPort(cmd.Context(), flagInClusterPort) - default: - return errorx.IllegalArgument.New("one of --mgmt-cidr, --blocked-cidr, or --in-cluster-port is required") + name, cidrs, ports, err := resolveTarget(cmd) + if err != nil { + return err } + return newManager().Add(cmd.Context(), name, cidrs, ports) }, } func init() { - addCmd.Flags().StringVar(&flagMgmtCIDR, "mgmt-cidr", "", "A single management CIDR to add") - addCmd.Flags().StringVar(&flagBlockedCIDR, "blocked-cidr", "", "A single operator block-list CIDR to add (dropped inbound, outbound, and forwarded)") - addCmd.Flags().IntVar(&flagInClusterPort, "in-cluster-port", 0, "A single in-cluster host-service port to add") + registerTargetFlags(addCmd, "add") + addCmd.Flags().StringVar(&flagMgmtCIDR, "mgmt-cidr", "", "A single management CIDR to add (shorthand for --name mgmt --cidr)") + addCmd.Flags().StringVar(&flagBlockedCIDR, "blocked-cidr", "", "A single operator block-list CIDR to add (shorthand for --name blocked --cidr)") + addCmd.Flags().IntVar(&flagInClusterPort, "in-cluster-port", 0, "A single in-cluster host-service port to add (shorthand for --name in_cluster --port)") addCmd.MarkFlagsMutuallyExclusive("mgmt-cidr", "blocked-cidr", "in-cluster-port") } diff --git a/cmd/cli/commands/network/firewall/create.go b/cmd/cli/commands/network/firewall/create.go index e6c59325..73cecf93 100644 --- a/cmd/cli/commands/network/firewall/create.go +++ b/cmd/cli/commands/network/firewall/create.go @@ -4,12 +4,12 @@ package firewall import ( "context" + "strconv" "github.com/automa-saga/logx" "github.com/hashgraph/solo-weaver/cmd/cli/commands/common" "github.com/hashgraph/solo-weaver/internal/kube" fw "github.com/hashgraph/solo-weaver/internal/network/firewall" - "github.com/hashgraph/solo-weaver/pkg/sanity" "github.com/spf13/cobra" ) @@ -26,62 +26,17 @@ var detectPodCIDR = func(ctx context.Context) (string, error) { var createCmd = &cobra.Command{ Use: "create", Short: "Create the `inet weaver-host-firewall` table (create-if-missing; --force re-renders)", - Long: "Render and apply the full `inet weaver-host-firewall` table. create-if-missing: if the table already " + - "exists, no changes are made unless --force is passed, which re-renders from the flags.", + Long: "Render and apply the full `inet weaver-host-firewall` table, either from flags or from a declarative " + + "config file (--from-file). create-if-missing: if the table already exists, no changes are made unless " + + "--force is passed, which re-renders from the current flags or file.\n\n" + + "--from-file is the only way to declare named allow rules. It is fully declarative: an allow rule absent " + + "from the file is removed. The reserved blocks behave differently — one absent from the file is derived or " + + "defaulted, never removed, so a partial file cannot silently drop management access. To disable a reserved " + + "block, give it an empty address list (`in_cluster: {cidrs: []}`).", RunE: func(cmd *cobra.Command, args []string) error { - // NewTable() seeds the design defaults (SSH 22, the stack in-cluster - // port set). Override a field only when its flag was explicitly set: - // the flag-binding vars are shared across verbs (see firewall.go), so a - // later verb's registration clobbers another verb's default in the - // shared variable. Reading the shared value unconditionally would wipe - // --in-cluster-ports to nil on a plain `create --force`; gating on - // Changed() keeps NewTable()'s default authoritative. - t := fw.NewTable() - if cmd.Flags().Changed("mgmt-cidrs") { - t.MgmtCIDRs = flagMgmtCIDRs - } - if cmd.Flags().Changed("blocked-cidrs") { - t.BlockedCIDRs = flagBlockedCIDRs - } - if cmd.Flags().Changed("in-cluster-ports") { - t.InClusterPorts = flagInClusterPorts - } - if cmd.Flags().Changed("ssh-port") { - t.SSHPort = flagSSHPort - } - - // --pod-cidr accepts a mixed v4/v6 list; route each entry to the matching - // family slot so a dual-stack node can admit in-cluster traffic over both. - // A value that fails family classification is slotted as v4 so Table.Validate - // surfaces a clear --pod-cidr error rather than dropping it silently. - // - // When the operator passes nothing, auto-detection resolves the local - // node's .spec.podCIDR (a single, v4 value today). Detection is - // best-effort — `network firewall create` is node-agnostic and may run - // before a cluster exists, so if no cluster is reachable we fall back to - // omitting the in-cluster-ports rule and tell the operator how to set it. - if len(flagPodCIDR) > 0 { - for _, c := range flagPodCIDR { - if isV6, err := sanity.CIDRIsIPv6(c); err == nil && isV6 { - t.PodCIDR6 = c - } else { - t.PodCIDR = c - } - } - } else { - if cidr, err := detectPodCIDR(cmd.Context()); err != nil { - logx.As().Warn().Err(err).Msg( - "could not auto-detect pod CIDR; the in-cluster host-service ports rule will be omitted — pass --pod-cidr to set it explicitly") - } else { - t.PodCIDR = cidr - logx.As().Info().Str("pod_cidr", cidr).Msg("auto-detected pod CIDR from the local node") - } - } - - if len(t.MgmtCIDRs) == 0 { - logx.As().Warn().Msg( - "no --mgmt-cidrs set: the SSH allow rule will match no sources under the default-drop policy — " + - "you will be locked out of new SSH connections; pass --mgmt-cidrs to set the management allowlist") + t, err := buildTable(cmd) + if err != nil { + return err } force, err := common.FlagForce().Value(cmd, args) @@ -100,10 +55,99 @@ var createCmd = &cobra.Command{ }, } +// buildTable assembles the desired table from --from-file or from the individual +// flags. +func buildTable(cmd *cobra.Command) (*fw.Table, error) { + if cmd.Flags().Changed("from-file") { + cfg, err := fw.LoadConfigFile(flagFromFile) + if err != nil { + return nil, err + } + t, err := cfg.Table() + if err != nil { + return nil, err + } + // An omitted in-cluster block means "use the cluster's pod CIDR"; an + // explicitly empty list means "render no in-cluster rule". Only the former + // triggers detection. + if cfg.InClusterCIDRsUnset() { + applyDetectedPodCIDR(cmd, t) + } + warnOnEmptyMgmt(t) + return t, nil + } + + // NewTable() seeds the design defaults (SSH 22, the stack in-cluster port + // set). Override a field only when its flag was explicitly set: the + // flag-binding vars are shared across verbs (see firewall.go), so a later + // verb's registration clobbers another verb's default in the shared + // variable. Reading the shared value unconditionally would wipe + // --in-cluster-ports to nil on a plain `create --force`; gating on Changed() + // keeps NewTable()'s default authoritative. + t := fw.NewTable() + if cmd.Flags().Changed("mgmt-cidrs") { + t.Mgmt.CIDRs = flagMgmtCIDRs + } + if cmd.Flags().Changed("blocked-cidrs") { + t.Blocked.CIDRs = flagBlockedCIDRs + } + if cmd.Flags().Changed("in-cluster-ports") { + t.InCluster.Ports = fw.PortStrings(flagInClusterPorts) + } + if cmd.Flags().Changed("ssh-port") { + t.Mgmt.Ports = []string{strconv.Itoa(flagSSHPort)} + } + + // --pod-cidr accepts a mixed v4/v6 list; the renderer routes each entry to + // its family's set, so no slotting is needed here. + // + // When the operator passes nothing, auto-detection resolves the local node's + // .spec.podCIDR. Detection is best-effort — `network firewall create` is + // node-agnostic and may run before a cluster exists — so if no cluster is + // reachable we fall back to omitting the in-cluster rule and tell the + // operator how to set it. + if len(flagPodCIDR) > 0 { + t.InCluster.CIDRs = flagPodCIDR + } else { + applyDetectedPodCIDR(cmd, t) + } + + warnOnEmptyMgmt(t) + return t, nil +} + +func applyDetectedPodCIDR(cmd *cobra.Command, t *fw.Table) { + cidr, err := detectPodCIDR(cmd.Context()) + if err != nil { + logx.As().Warn().Err(err).Msg( + "could not auto-detect pod CIDR; the in-cluster host-service ports rule will be omitted — pass --pod-cidr to set it explicitly") + return + } + t.InCluster.CIDRs = []string{cidr} + logx.As().Info().Str("pod_cidr", cidr).Msg("auto-detected pod CIDR from the local node") +} + +func warnOnEmptyMgmt(t *fw.Table) { + if len(t.Mgmt.CIDRs) == 0 { + logx.As().Warn().Msg( + "no management CIDRs set: the management allow rule will match no sources under the default-drop policy — " + + "you will be locked out of new SSH connections; pass --mgmt-cidrs to set the management allowlist") + } +} + func init() { createCmd.Flags().StringSliceVar(&flagMgmtCIDRs, "mgmt-cidrs", nil, "Management/SSH allowlist CIDRs (comma-separated or repeated)") - createCmd.Flags().StringSliceVar(&flagBlockedCIDRs, "blocked-cidrs", nil, "Operator-curated block list CIDRs, dropped inbound, outbound, and forwarded, ahead of conntrack (comma-separated or repeated)") + createCmd.Flags().StringSliceVar(&flagBlockedCIDRs, "blocked-cidrs", nil, "Operator-curated block list CIDRs, dropped before any other rule (comma-separated or repeated)") createCmd.Flags().IntSliceVar(&flagInClusterPorts, "in-cluster-ports", fw.DefaultInClusterPorts, "Host-service ports reachable from the pod CIDR") createCmd.Flags().IntVar(&flagSSHPort, "ssh-port", fw.DefaultSSHPort, "SSH/management TCP port accepted from the allowlist") createCmd.Flags().StringSliceVar(&flagPodCIDR, "pod-cidr", nil, "Pod CIDR(s) allowed to reach the in-cluster host-service ports; may be IPv4 and/or IPv6 (comma-separated or repeated). Default: auto-detected from the local node's .spec.podCIDR; the rule is omitted if no cluster is reachable") + createCmd.Flags().StringVar(&flagFromFile, "from-file", "", "Declarative YAML config to render the whole table from (the only way to declare named allow rules); mutually exclusive with the individual flags") + + // A file states the whole table, so mixing it with a flag that states part of + // one would leave the precedence between them to guesswork. + createCmd.MarkFlagsMutuallyExclusive("from-file", "mgmt-cidrs") + createCmd.MarkFlagsMutuallyExclusive("from-file", "blocked-cidrs") + createCmd.MarkFlagsMutuallyExclusive("from-file", "in-cluster-ports") + createCmd.MarkFlagsMutuallyExclusive("from-file", "ssh-port") + createCmd.MarkFlagsMutuallyExclusive("from-file", "pod-cidr") } diff --git a/cmd/cli/commands/network/firewall/delete.go b/cmd/cli/commands/network/firewall/delete.go index 3a197297..55d04e58 100644 --- a/cmd/cli/commands/network/firewall/delete.go +++ b/cmd/cli/commands/network/firewall/delete.go @@ -4,20 +4,65 @@ package firewall import ( "github.com/automa-saga/logx" + "github.com/hashgraph/solo-weaver/cmd/cli/commands/common" + "github.com/hashgraph/solo-weaver/internal/ui/prompt" + "github.com/joomcode/errorx" "github.com/spf13/cobra" ) var deleteCmd = &cobra.Command{ Use: "delete", - Short: "Remove the `inet weaver-host-firewall` table and its on-disk artifact", - Long: "Remove the `inet weaver-host-firewall` table and /etc/solo-provisioner/network-weaver-host-firewall.nft. This does NOT " + - "disable the shared solo-provisioner-network-nft.service (shared with `inet weaver-workload-policy`); host-level " + - "teardown is orchestrated by `kube cluster uninstall`.", - RunE: func(cmd *cobra.Command, _ []string) error { - if err := newManager().Delete(cmd.Context()); err != nil { + Short: "Delete one allow rule (--name), or the whole table (--all)", + Long: "Delete a single named allow rule with --name, or tear the whole `inet weaver-host-firewall` table down " + + "with --all (the default when no flag is given, which is what this verb has always done).\n\n" + + "The reserved blocks cannot be deleted individually — clear their addresses instead (`network firewall set " + + "--name mgmt --cidrs \"\"`). --all removes the table and " + + "/etc/solo-provisioner/network-weaver-host-firewall.{nft,yaml}, leaving the host with no weaver-managed " + + "firewall at all, so it asks for confirmation in an interactive session. It does NOT disable the shared " + + "solo-provisioner-network-nft.service (shared with `inet weaver-workload-policy`); host-level teardown is " + + "orchestrated by `kube cluster uninstall`.", + RunE: func(cmd *cobra.Command, args []string) error { + mgr := newManager() + + if cmd.Flags().Changed("name") { + if err := mgr.DeleteRule(cmd.Context(), flagName); err != nil { + return err + } + logx.As().Info().Str("rule", flagName).Msg("allow rule removed from the host firewall") + return nil + } + + force, err := common.FlagForce().Value(cmd, args) + if err != nil { + return err + } + // No flag at all means --all: that is the behaviour this verb shipped with, + // and the callers relying on it are non-interactive, where ShouldPrompt is + // false and nothing changes for them. + if prompt.ShouldPrompt(force) { + ok, err := prompt.RunConfirm( + "Delete the whole host firewall?", + "This removes the inet weaver-host-firewall table and its on-disk artifacts. The host will have no "+ + "weaver-managed firewall — including no management allowlist — until one is created again.", + false) + if err != nil { + return err + } + if !ok { + return errorx.IllegalState.New("aborted: host firewall not deleted") + } + } + + if err := mgr.Delete(cmd.Context()); err != nil { return err } logx.As().Info().Msg("inet weaver-host-firewall firewall removed") return nil }, } + +func init() { + deleteCmd.Flags().StringVar(&flagName, "name", "", "Named allow rule to delete (the reserved blocks cannot be deleted)") + deleteCmd.Flags().BoolVar(&flagAll, "all", false, "Delete the whole table and its on-disk artifacts (the default when --name is omitted)") + deleteCmd.MarkFlagsMutuallyExclusive("name", "all") +} diff --git a/cmd/cli/commands/network/firewall/firewall.go b/cmd/cli/commands/network/firewall/firewall.go index 9cef69f9..bca6f16b 100644 --- a/cmd/cli/commands/network/firewall/firewall.go +++ b/cmd/cli/commands/network/firewall/firewall.go @@ -2,13 +2,24 @@ // Package firewall wires the `solo-provisioner network firewall` verbs to the // internal/network/firewall manager. The verbs manage the node-agnostic -// `inet weaver-host-firewall` nftables table (SSH/mgmt allowlist, ICMP policy, in-cluster -// host-service ports). +// `inet weaver-host-firewall` nftables table: three reserved blocks (the +// management allowlist, the operator block list, the in-cluster host-service +// allowance) plus any number of named allow rules. +// +// The verbs split along a deliberate line. Structure — which rules exist, and +// what protocol each matches — is declared in a config file, because adding a +// rule is a reviewed change. Membership — the addresses and ports inside a rule +// — is mutable straight from the CLI, because unblocking an operator or opening +// a port is sometimes urgent. package firewall import ( + "strconv" + "strings" + "github.com/hashgraph/solo-weaver/cmd/cli/commands/common" fw "github.com/hashgraph/solo-weaver/internal/network/firewall" + "github.com/joomcode/errorx" "github.com/spf13/cobra" ) @@ -21,6 +32,20 @@ import ( // Verbs must therefore take their defaults from the model (NewTable) and gate // overrides on cmd.Flags().Changed(), never trust the shared variable's default. var ( + // Name-addressed flags, which reach any rule including the reserved blocks. + // --cidr/--port (add, remove) and --cidrs/--ports (set) bind to the same + // variables: the spelling differs to signal incremental vs replace, but the + // value is a list either way. + flagName string + flagCIDRs []string + flagPorts []string + flagCIDRsFile string + flagFromFile string + flagOutput string + flagAll bool + + // Per-block flags that predate --name, retained so every invocation that + // worked before still works and the interactive install flow is unchanged. flagMgmtCIDRs []string flagMgmtCIDR string flagBlockedCIDRs []string @@ -35,8 +60,10 @@ var firewallCmd = &cobra.Command{ Use: "firewall", Short: "Manage the node-level host firewall (`inet weaver-host-firewall` nftables table)", Long: "Manage the node-agnostic host firewall: the `inet weaver-host-firewall` nftables table that protects the " + - "bare-metal host (SSH/management allowlist, ICMP policy, in-cluster host-service ports). " + - "This table is separate from the `inet weaver-workload-policy` workload plane and applies to every node type.", + "bare-metal host. It carries three reserved blocks — `mgmt` (management allowlist), `blocked` (operator " + + "block list) and `in_cluster` (host-service ports reachable from the pod CIDR) — plus any number of named " + + "allow rules declared in a config file. This table is separate from the `inet weaver-workload-policy` " + + "workload plane and applies to every node type.", RunE: common.DefaultRunE, } @@ -52,3 +79,49 @@ func GetCmd() *cobra.Command { // newManager constructs the production manager (live nft kernel apply + systemd // service enable). Indirected through a var so command tests can stub it. var newManager = func() *fw.Manager { return fw.NewManager() } + +// registerTargetFlags registers the name-addressed flags for an incremental verb +// (add, remove), where the singular spelling signals that the values are merged +// into the rule's existing lists rather than replacing them. +func registerTargetFlags(cmd *cobra.Command, verb string) { + cmd.Flags().StringVar(&flagName, "name", "", + "Rule to modify: a reserved block (mgmt, blocked, in_cluster) or a named allow rule") + cmd.Flags().StringSliceVar(&flagCIDRs, "cidr", nil, + "CIDR(s) to "+verb+" (comma-separated or repeated)") + cmd.Flags().StringSliceVar(&flagPorts, "port", nil, + "Port(s) to "+verb+"; a single port (6443) or an inclusive range (2379-2380) (comma-separated or repeated)") +} + +// resolveTarget determines which rule an incremental verb operates on and the +// values to apply. --name addresses any rule; the per-block flags that predate +// it address their reserved block implicitly, which is what keeps older +// invocations working. The two forms are mutually exclusive — combining them +// would leave it ambiguous which rule the general --cidr belongs to. +func resolveTarget(cmd *cobra.Command) (name string, cidrs, ports []string, err error) { + f := cmd.Flags() + general := f.Changed("name") || f.Changed("cidr") || f.Changed("port") + + switch { + case f.Changed("mgmt-cidr"): + name, cidrs = fw.RuleMgmt, []string{flagMgmtCIDR} + case f.Changed("blocked-cidr"): + name, cidrs = fw.RuleBlocked, []string{flagBlockedCIDR} + case f.Changed("in-cluster-port"): + name, ports = fw.RuleInCluster, []string{strconv.Itoa(flagInClusterPort)} + default: + if !f.Changed("name") { + return "", nil, nil, errorx.IllegalArgument.New( + "--name is required: name a reserved block (%s) or an allow rule", strings.Join(fw.ReservedNames, ", ")) + } + if !f.Changed("cidr") && !f.Changed("port") { + return "", nil, nil, errorx.IllegalArgument.New("at least one of --cidr or --port is required") + } + return flagName, flagCIDRs, flagPorts, nil + } + + if general { + return "", nil, nil, errorx.IllegalArgument.New( + "--mgmt-cidr, --blocked-cidr and --in-cluster-port already name the rule they edit; use --name with --cidr/--port instead of combining the two forms") + } + return name, cidrs, ports, nil +} diff --git a/cmd/cli/commands/network/firewall/firewall_test.go b/cmd/cli/commands/network/firewall/firewall_test.go index a1bdecd1..6710a2a4 100644 --- a/cmd/cli/commands/network/firewall/firewall_test.go +++ b/cmd/cli/commands/network/firewall/firewall_test.go @@ -3,6 +3,7 @@ package firewall import ( + "bytes" "context" "errors" "io" @@ -12,6 +13,7 @@ import ( fw "github.com/hashgraph/solo-weaver/internal/network/firewall" "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/stretchr/testify/require" ) @@ -42,22 +44,318 @@ func TestFirewallCmd_Structure(t *testing.T) { } func TestCreateCmd_Flags(t *testing.T) { - for _, name := range []string{"mgmt-cidrs", "blocked-cidrs", "in-cluster-ports", "ssh-port", "pod-cidr"} { + for _, name := range []string{"mgmt-cidrs", "blocked-cidrs", "in-cluster-ports", "ssh-port", "pod-cidr", "from-file"} { require.NotNil(t, createCmd.Flags().Lookup(name), "create is missing --%s", name) } // Defaults must match the firewall package defaults. require.Equal(t, "22", createCmd.Flags().Lookup("ssh-port").DefValue) - // ICMP is a static ruleset, not flag-driven: there must be no icmp toggles. + // ICMP is a static ruleset apart from the per-rule icmp_echo grant, which is + // a config-file field: there must be no icmp toggles here. require.Nil(t, createCmd.Flags().Lookup("icmp-mgmt"), "icmp-mgmt flag should be removed") require.Nil(t, createCmd.Flags().Lookup("icmp-public"), "icmp-public flag should be removed") } +// TestVerbs_NameAddressedFlags pins the name-addressed surface added alongside +// the per-block flags. +func TestVerbs_NameAddressedFlags(t *testing.T) { + for _, tc := range []struct { + cmd *cobra.Command + verb string + flags []string + }{ + {addCmd, "add", []string{"name", "cidr", "port"}}, + {removeCmd, "remove", []string{"name", "cidr", "port"}}, + {setCmd, "set", []string{"name", "cidrs", "cidrs-file", "ports"}}, + {showCmd, "show", []string{"name", "output"}}, + {deleteCmd, "delete", []string{"name", "all"}}, + } { + for _, f := range tc.flags { + require.NotNil(t, tc.cmd.Flags().Lookup(f), "%s is missing --%s", tc.verb, f) + } + } + require.Equal(t, "nft", showCmd.Flags().Lookup("output").DefValue) +} + +// stubManager points the CLI at a Manager backed by temp paths and a fake runner, +// returning the artifact paths so a test can assert on what a verb rendered. +func stubManager(t *testing.T) (nftPath, configPath string) { + t.Helper() + r := &captureRunner{} + dir := t.TempDir() + nftPath = filepath.Join(dir, "network-weaver-host-firewall.nft") + configPath = filepath.Join(dir, "network-weaver-host-firewall.yaml") + + origMgr, origDetect := newManager, detectPodCIDR + newManager = func() *fw.Manager { + return fw.NewManagerWithConfig(fw.Config{ + Runner: r, + NftPath: nftPath, + ConfigPath: configPath, + LockPath: filepath.Join(dir, ".applying"), + ApplyViaService: func(context.Context) error { + r.exists = true + return nil + }, + }) + } + detectPodCIDR = func(context.Context) (string, error) { return "", errors.New("no cluster") } + t.Cleanup(func() { newManager, detectPodCIDR = origMgr, origDetect }) + return nftPath, configPath +} + +// run executes one `firewall …` invocation through a fresh root command, the way +// the real CLI would. +func run(t *testing.T, args ...string) error { + t.Helper() + _, err := runOut(t, args...) + return err +} + +// runOut is run() with stdout captured, for the verbs that print. +func runOut(t *testing.T, args ...string) (string, error) { + t.Helper() + resetFlagState(t) + + var out bytes.Buffer + root := &cobra.Command{Use: "test"} + root.PersistentFlags().Bool("force", false, "force") + root.AddCommand(GetCmd()) + root.SetArgs(append([]string{"firewall"}, args...)) + root.SetOut(&out) + root.SetErr(io.Discard) + err := root.Execute() + return out.String(), err +} + +// resetFlagState clears the flag state left behind by a previous invocation. +// GetCmd returns package-level cobra commands, so pflag's per-flag Changed +// survives from one Execute to the next within a test binary — which would make +// mutual-exclusion checks fire on a flag an earlier test set, and would let a +// value leak into a later verb. A real CLI process runs one command and never +// sees this. +func resetFlagState(t *testing.T) { + t.Helper() + for _, sub := range GetCmd().Commands() { + sub.Flags().VisitAll(func(f *pflag.Flag) { f.Changed = false }) + } + // The shared binding variables are read directly (not via Changed) in a + // couple of places, so zero them too. + flagName, flagCIDRsFile, flagFromFile, flagOutput, flagAll = "", "", "", outputNft, false + flagCIDRs, flagPorts, flagPodCIDR = nil, nil, nil + flagMgmtCIDRs, flagBlockedCIDRs, flagInClusterPorts = nil, nil, nil + flagMgmtCIDR, flagBlockedCIDR = "", "" + flagInClusterPort, flagSSHPort = 0, 0 +} + +// TestBackwardCompatibleInvocations is the regression gate the generalisation +// rests on: every `network firewall` invocation that worked before --name existed +// must still behave identically. The per-block flags are shorthands now, but +// nothing about them changed for a caller. +func TestBackwardCompatibleInvocations(t *testing.T) { + nftPath, _ := stubManager(t) + + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8", "--blocked-cidrs", "203.0.113.0/24", + "--ssh-port", "2222", "--in-cluster-ports", "6443,10250", "--pod-cidr", "10.4.0.0/24")) + doc := readFile(t, nftPath) + require.Contains(t, doc, "elements = { 10.0.0.0/8 }") + require.Contains(t, doc, "elements = { 203.0.113.0/24 }") + 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.NoError(t, run(t, "add", "--mgmt-cidr", "192.168.1.0/24")) + require.Contains(t, readFile(t, nftPath), "192.168.1.0/24") + + require.NoError(t, run(t, "add", "--blocked-cidr", "198.51.100.0/24")) + require.Contains(t, readFile(t, nftPath), "198.51.100.0/24") + + require.NoError(t, run(t, "add", "--in-cluster-port", "9100")) + require.Contains(t, readFile(t, nftPath), "9100") + + require.NoError(t, run(t, "remove", "--mgmt-cidr", "192.168.1.0/24")) + require.NotContains(t, readFile(t, nftPath), "192.168.1.0/24") + + require.NoError(t, run(t, "remove", "--in-cluster-port", "9100")) + require.NotContains(t, readFile(t, nftPath), "9100") + + // The multi-block form of `set`: three reserved blocks replaced in one call, + // which must stay a single apply. + require.NoError(t, run(t, "set", "--mgmt-cidrs", "172.16.0.0/12", + "--blocked-cidrs", "203.0.113.9/32", "--in-cluster-ports", "6443")) + doc = readFile(t, nftPath) + require.Contains(t, doc, "172.16.0.0/12") + require.Contains(t, doc, "203.0.113.9/32") + require.Contains(t, doc, "set in_cluster_ports { type inet_service; flags interval; auto-merge; elements = { 6443 }; }") + require.NotContains(t, doc, "10250") + + // Bare `delete` still tears the whole table down. Non-interactive, so the new + // confirmation prompt does not fire. + require.NoError(t, run(t, "delete")) + require.NoFileExists(t, nftPath) +} + +func TestCreateCmd_FromFile(t *testing.T) { + nftPath, _ := stubManager(t) + dir := t.TempDir() + path := filepath.Join(dir, "rules.yaml") + require.NoError(t, os.WriteFile(path, []byte(`version: 1 +mgmt: + cidrs: ["192.168.68.0/24"] + ports: ["22", "1024"] +blocked: + cidrs: [] +in_cluster: + cidrs: ["10.4.0.0/14"] + ports: ["4244", "6443"] +allow: + - name: k8s-node + cidrs: ["10.0.0.0/24"] + ports: ["6443", "2379-2380"] + proto: tcp + - name: cilium-vxlan + cidrs: ["10.0.0.0/24"] + ports: ["8472"] + proto: udp + - name: admin + cidrs: ["203.0.113.5/32"] + ports: ["22"] + icmp_echo: true +`), 0o600)) + + require.NoError(t, run(t, "create", "--from-file", path)) + doc := readFile(t, nftPath) + require.Contains(t, doc, "set mgmt_ports { type inet_service; flags interval; auto-merge; elements = { 22, 1024 }; }") + require.Contains(t, doc, "ip saddr @k8s-node tcp dport @k8s-node_ports accept") + require.Contains(t, doc, "ip saddr @cilium-vxlan udp dport @cilium-vxlan_ports accept") + require.Contains(t, doc, "ip saddr @admin icmp type echo-request accept") + require.Contains(t, doc, "elements = { 2379-2380, 6443 }") + + // --from-file and the individual flags are mutually exclusive: a file states + // the whole table, so the precedence between the two would be guesswork. + require.Error(t, run(t, "create", "--from-file", path, "--mgmt-cidrs", "10.0.0.0/8")) +} + +func TestShowCmd_YAMLRoundTrips(t *testing.T) { + nftPath, _ := stubManager(t) + dir := t.TempDir() + path := filepath.Join(dir, "rules.yaml") + require.NoError(t, os.WriteFile(path, []byte(`version: 1 +mgmt: + cidrs: ["192.168.68.0/24"] + ports: ["22"] +in_cluster: + cidrs: [] +allow: + - name: k8s-node + cidrs: ["10.0.0.0/24"] + ports: ["6443", "2379-2380"] +`), 0o600)) + require.NoError(t, run(t, "create", "--from-file", path)) + firstDoc := readFile(t, nftPath) + + shown, err := runOut(t, "show", "--output", "yaml") + require.NoError(t, err) + require.Contains(t, shown, "version: 1") + require.Contains(t, shown, "name: k8s-node") + + // Feeding the shown config back in changes nothing — the acceptance criterion + // that makes `show --output yaml` safe to keep in version control. + back := filepath.Join(dir, "shown.yaml") + require.NoError(t, os.WriteFile(back, []byte(shown), 0o600)) + require.NoError(t, run(t, "create", "--from-file", back, "--force")) + require.Equal(t, firstDoc, readFile(t, nftPath)) + + // An explicitly empty in_cluster block survives the round-trip as empty + // rather than reverting to the auto-detected pod CIDR. + require.NotContains(t, readFile(t, nftPath), "tcp dport @in_cluster_ports accept") + + // --name narrows to one rule. + one, err := runOut(t, "show", "--name", "k8s-node") + require.NoError(t, err) + require.Contains(t, one, "name: k8s-node") + require.NotContains(t, one, "mgmt") + + _, err = runOut(t, "show", "--name", "nope") + require.Error(t, err) + + _, err = runOut(t, "show", "--output", "json") + require.Error(t, err, "--output must reject a format it does not render") +} + +func TestDeleteCmd_ByName(t *testing.T) { + nftPath, _ := stubManager(t) + dir := t.TempDir() + path := filepath.Join(dir, "rules.yaml") + require.NoError(t, os.WriteFile(path, []byte(`version: 1 +mgmt: + cidrs: ["192.168.68.0/24"] +allow: + - name: k8s-node + cidrs: ["10.0.0.0/24"] + ports: ["6443"] +`), 0o600)) + require.NoError(t, run(t, "create", "--from-file", path)) + require.Contains(t, readFile(t, nftPath), "@k8s-node") + + require.NoError(t, run(t, "delete", "--name", "k8s-node")) + require.NotContains(t, readFile(t, nftPath), "@k8s-node") + require.FileExists(t, nftPath, "deleting one rule must not tear the table down") + + // The reserved blocks are structural and cannot be deleted individually. + require.Error(t, run(t, "delete", "--name", "mgmt")) + require.Error(t, run(t, "delete", "--name", "k8s-node", "--all")) +} + +func TestElementVerbs_RequireATarget(t *testing.T) { + stubManager(t) + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + + // --name is required once no per-block shorthand names the rule. + require.Error(t, run(t, "add", "--cidr", "10.1.0.0/16")) + require.Error(t, run(t, "remove", "--cidr", "10.1.0.0/16")) + require.Error(t, run(t, "set", "--cidrs", "10.1.0.0/16")) + // --name with no values to apply is a no-op worth rejecting. + require.Error(t, run(t, "add", "--name", "mgmt")) + require.Error(t, run(t, "set", "--name", "mgmt")) + // Mixing the two forms leaves it ambiguous which rule --cidr belongs to. + require.Error(t, run(t, "add", "--mgmt-cidr", "10.1.0.0/16", "--name", "blocked")) + require.Error(t, run(t, "set", "--mgmt-cidrs", "10.1.0.0/16", "--name", "blocked", "--cidrs", "10.2.0.0/16")) + // --cidrs and --cidrs-file are alternatives, not a merge. + require.Error(t, run(t, "set", "--name", "mgmt", "--cidrs", "10.1.0.0/16", "--cidrs-file", "/nonexistent")) +} + +func TestSetCmd_CIDRsFile(t *testing.T) { + nftPath, _ := stubManager(t) + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + + dir := t.TempDir() + path := filepath.Join(dir, "cidrs.txt") + // The flat list format `network policy --cidrs-file` already uses: newlines + // and/or commas, with `#` comments. + require.NoError(t, os.WriteFile(path, []byte("# management\n192.168.68.0/24\n10.9.0.0/16, 172.16.0.0/12\n"), 0o600)) + + require.NoError(t, run(t, "set", "--name", "mgmt", "--cidrs-file", path)) + doc := readFile(t, nftPath) + require.Contains(t, doc, "elements = { 10.9.0.0/16, 172.16.0.0/12, 192.168.68.0/24 }") + require.NotContains(t, doc, "10.0.0.0/8") +} + +func readFile(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + return string(data) +} + func TestCreateCmd_DefaultsInClusterPortsWhenNotPassed(t *testing.T) { // Regression: `create` (even with --force) without --in-cluster-ports must // render the stack port set. The flag-binding var is shared with `set` // (nil default), which clobbers create's default in the shared variable — // so create must source the default from NewTable(), gated on Changed(). // This executes the real command so the shared-var registration is exercised. + resetFlagState(t) + r := &captureRunner{} dir := t.TempDir() nftPath := filepath.Join(dir, "network-weaver-host-firewall.nft") diff --git a/cmd/cli/commands/network/firewall/remove.go b/cmd/cli/commands/network/firewall/remove.go index 1776e834..97cf58d0 100644 --- a/cmd/cli/commands/network/firewall/remove.go +++ b/cmd/cli/commands/network/firewall/remove.go @@ -3,31 +3,29 @@ package firewall import ( - "github.com/joomcode/errorx" "github.com/spf13/cobra" ) var removeCmd = &cobra.Command{ Use: "remove", - Short: "Remove a single --mgmt-cidr, --blocked-cidr, or --in-cluster-port", + Short: "Remove CIDRs and/or ports from a rule (--name)", + Long: "Remove addresses and/or ports from one rule of the host firewall. --name selects the rule: a reserved " + + "block (mgmt, blocked, in_cluster) or a named allow rule. Removing an entry that is not there is a no-op.\n\n" + + "Ports are removed by exact spec, so removing 2379 from a rule holding the range 2379-2380 does nothing — " + + "replace the range with `set --ports` instead of splitting it implicitly.", RunE: func(cmd *cobra.Command, _ []string) error { - mgr := newManager() - switch { - case cmd.Flags().Changed("mgmt-cidr"): - return mgr.RemoveMgmtCIDR(cmd.Context(), flagMgmtCIDR) - case cmd.Flags().Changed("blocked-cidr"): - return mgr.RemoveBlockedCIDR(cmd.Context(), flagBlockedCIDR) - case cmd.Flags().Changed("in-cluster-port"): - return mgr.RemovePort(cmd.Context(), flagInClusterPort) - default: - return errorx.IllegalArgument.New("one of --mgmt-cidr, --blocked-cidr, or --in-cluster-port is required") + name, cidrs, ports, err := resolveTarget(cmd) + if err != nil { + return err } + return newManager().Remove(cmd.Context(), name, cidrs, ports) }, } func init() { - removeCmd.Flags().StringVar(&flagMgmtCIDR, "mgmt-cidr", "", "A single management CIDR to remove") - removeCmd.Flags().StringVar(&flagBlockedCIDR, "blocked-cidr", "", "A single operator block-list CIDR to remove") - removeCmd.Flags().IntVar(&flagInClusterPort, "in-cluster-port", 0, "A single in-cluster host-service port to remove") + registerTargetFlags(removeCmd, "remove") + removeCmd.Flags().StringVar(&flagMgmtCIDR, "mgmt-cidr", "", "A single management CIDR to remove (shorthand for --name mgmt --cidr)") + removeCmd.Flags().StringVar(&flagBlockedCIDR, "blocked-cidr", "", "A single operator block-list CIDR to remove (shorthand for --name blocked --cidr)") + removeCmd.Flags().IntVar(&flagInClusterPort, "in-cluster-port", 0, "A single in-cluster host-service port to remove (shorthand for --name in_cluster --port)") removeCmd.MarkFlagsMutuallyExclusive("mgmt-cidr", "blocked-cidr", "in-cluster-port") } diff --git a/cmd/cli/commands/network/firewall/set.go b/cmd/cli/commands/network/firewall/set.go index bc4c8649..e1e1195d 100644 --- a/cmd/cli/commands/network/firewall/set.go +++ b/cmd/cli/commands/network/firewall/set.go @@ -3,47 +3,134 @@ package firewall import ( + "os" + "strings" + + fw "github.com/hashgraph/solo-weaver/internal/network/firewall" "github.com/joomcode/errorx" "github.com/spf13/cobra" ) var setCmd = &cobra.Command{ Use: "set", - Short: "Atomically replace the full --mgmt-cidrs, --blocked-cidrs, and/or --in-cluster-ports list", + Short: "Atomically replace a rule's full CIDR and/or port list", + Long: "Replace the addresses and/or ports of one rule (--name), or of several reserved blocks at once via the " + + "per-block flags. Every replacement in a single invocation lands as one nft transaction, so a `set` that " + + "touches the management allowlist is never half-applied.\n\n" + + "A flag left off leaves that list unchanged; a flag given an empty value clears it. Clearing a reserved " + + "block's addresses is how you disable it without deleting it.", RunE: func(cmd *cobra.Command, _ []string) error { - var mgmt, blocked []string - var ports []int - - // A nil slice leaves that dimension unchanged; a changed flag (even with - // an empty value) replaces it. - if cmd.Flags().Changed("mgmt-cidrs") { - mgmt = flagMgmtCIDRs - if mgmt == nil { - mgmt = []string{} - } + updates, err := resolveSetUpdates(cmd) + if err != nil { + return err } - if cmd.Flags().Changed("blocked-cidrs") { - blocked = flagBlockedCIDRs - if blocked == nil { - blocked = []string{} - } + return newManager().SetMany(cmd.Context(), updates) + }, +} + +// resolveSetUpdates builds the replacement membership for this invocation. The +// per-block flags may name several reserved blocks in one call — that predates +// --name and stays supported — while --name addresses exactly one rule, which is +// the only form that can reach a named allow rule. +func resolveSetUpdates(cmd *cobra.Command) ([]fw.Update, error) { + f := cmd.Flags() + general := f.Changed("name") || f.Changed("cidrs") || f.Changed("cidrs-file") || f.Changed("ports") + + var legacy []fw.Update + if f.Changed("mgmt-cidrs") { + legacy = append(legacy, fw.Update{Name: fw.RuleMgmt, CIDRs: orEmpty(flagMgmtCIDRs)}) + } + if f.Changed("blocked-cidrs") { + legacy = append(legacy, fw.Update{Name: fw.RuleBlocked, CIDRs: orEmpty(flagBlockedCIDRs)}) + } + if f.Changed("in-cluster-ports") { + legacy = append(legacy, fw.Update{Name: fw.RuleInCluster, Ports: orEmpty(fw.PortStrings(flagInClusterPorts))}) + } + + switch { + case len(legacy) > 0 && general: + return nil, errorx.IllegalArgument.New( + "--mgmt-cidrs, --blocked-cidrs and --in-cluster-ports already name the rule they replace; use --name with --cidrs/--ports instead of combining the two forms") + case len(legacy) > 0: + return legacy, nil + case !f.Changed("name"): + return nil, errorx.IllegalArgument.New( + "--name is required: name a reserved block (%s) or an allow rule", strings.Join(fw.ReservedNames, ", ")) + } + + cidrs, err := resolveCIDRs(cmd) + if err != nil { + return nil, err + } + var ports []string + if f.Changed("ports") { + ports = orEmpty(flagPorts) + } + if cidrs == nil && ports == nil { + return nil, errorx.IllegalArgument.New("at least one of --cidrs, --cidrs-file or --ports is required") + } + return []fw.Update{{Name: flagName, CIDRs: cidrs, Ports: ports}}, nil +} + +// resolveCIDRs returns the replacement address list from --cidrs or --cidrs-file +// (mutually exclusive), or nil when neither was given, meaning "leave the +// addresses alone". +func resolveCIDRs(cmd *cobra.Command) ([]string, error) { + f := cmd.Flags() + switch { + case f.Changed("cidrs") && f.Changed("cidrs-file"): + return nil, errorx.IllegalArgument.New("--cidrs and --cidrs-file are mutually exclusive") + case f.Changed("cidrs-file"): + return readCIDRsFile(flagCIDRsFile) + case f.Changed("cidrs"): + return orEmpty(flagCIDRs), nil + } + return nil, nil +} + +// readCIDRsFile reads a newline- and/or comma-separated CIDR list from a file, +// skipping blank lines and `#` comments. Deliberately the same flat format as +// `network policy --cidrs-file`, not the structured --from-file config: this is a +// bulk address list, and an operator pasting one should not have to wrap it in +// YAML. Per-entry syntax is validated downstream. +func readCIDRsFile(path string) ([]string, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, errorx.ExternalError.Wrap(err, "failed to read --cidrs-file %s", path) + } + // Non-nil even when the file is empty: an empty --cidrs-file is an explicit + // instruction to clear the list, not an instruction to leave it alone. + out := []string{} + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue } - if cmd.Flags().Changed("in-cluster-ports") { - ports = flagInClusterPorts - if ports == nil { - ports = []int{} + for _, tok := range strings.Split(line, ",") { + if v := strings.TrimSpace(tok); v != "" { + out = append(out, v) } } + } + return out, nil +} - if mgmt == nil && blocked == nil && ports == nil { - return errorx.IllegalArgument.New("at least one of --mgmt-cidrs, --blocked-cidrs, or --in-cluster-ports is required") - } - - return newManager().Set(cmd.Context(), mgmt, blocked, ports) - }, +// orEmpty substitutes an empty slice for a nil one, so a flag that was set to an +// empty value clears the list rather than reading as "unchanged". +func orEmpty(in []string) []string { + if in == nil { + return []string{} + } + return in } func init() { + setCmd.Flags().StringVar(&flagName, "name", "", + "Rule to replace: a reserved block (mgmt, blocked, in_cluster) or a named allow rule") + setCmd.Flags().StringSliceVar(&flagCIDRs, "cidrs", nil, "Full CIDR list for --name (comma-separated; replaces the existing list)") + setCmd.Flags().StringVar(&flagCIDRsFile, "cidrs-file", "", "Alternative to --cidrs: a file of CIDRs (one per line or comma-separated)") + setCmd.Flags().StringSliceVar(&flagPorts, "ports", nil, "Full port list for --name; single ports and inclusive ranges (2379-2380) (comma-separated; replaces the existing list)") + setCmd.Flags().StringSliceVar(&flagMgmtCIDRs, "mgmt-cidrs", nil, "Full management allowlist (comma-separated; replaces the existing list)") setCmd.Flags().StringSliceVar(&flagBlockedCIDRs, "blocked-cidrs", nil, "Full operator block list (comma-separated; replaces the existing list)") setCmd.Flags().IntSliceVar(&flagInClusterPorts, "in-cluster-ports", nil, "Full in-cluster host-service port list (comma-separated; replaces the existing list)") diff --git a/cmd/cli/commands/network/firewall/show.go b/cmd/cli/commands/network/firewall/show.go index 8bb24524..799aec92 100644 --- a/cmd/cli/commands/network/firewall/show.go +++ b/cmd/cli/commands/network/firewall/show.go @@ -4,14 +4,45 @@ package firewall import ( "fmt" + "strings" + "github.com/joomcode/errorx" "github.com/spf13/cobra" + "gopkg.in/yaml.v3" +) + +// Output formats for `show`. +const ( + outputNft = "nft" + outputYAML = "yaml" ) var showCmd = &cobra.Command{ Use: "show", - Short: "Show the live `inet weaver-host-firewall` table", + Short: "Show the live `inet weaver-host-firewall` table, or its config with --output yaml", + Long: "Show the host firewall. By default this prints the live nftables ruleset, which is ground truth for what " + + "the kernel is enforcing. --output yaml instead prints the declarative config the ruleset was rendered " + + "from, in the exact schema `create --from-file` accepts — so `show --output yaml > rules.yaml` followed by " + + "`create --from-file rules.yaml --force` is a no-op.\n\n" + + "--name narrows the output to one rule, for inspection. That view is not a config file: re-applying a " + + "single rule as if it were the whole config would delete every other allow rule.", RunE: func(cmd *cobra.Command, _ []string) error { + switch flagOutput { + case outputNft, outputYAML: + default: + return errorx.IllegalArgument.New("invalid --output %q: expected %q or %q", flagOutput, outputNft, outputYAML) + } + + // --name addresses a rule, which only the config knows about — the kernel + // dump has no notion of which rule a set belongs to — so it implies the + // config view regardless of --output. + if cmd.Flags().Changed("name") { + return showRule(cmd) + } + if flagOutput == outputYAML { + return showConfig(cmd) + } + out, err := newManager().Show(cmd.Context()) if err != nil { return err @@ -20,3 +51,38 @@ var showCmd = &cobra.Command{ return nil }, } + +func showConfig(cmd *cobra.Command) error { + cfg, err := newManager().Config(cmd.Context()) + if err != nil { + return err + } + data, err := cfg.Marshal() + if err != nil { + return err + } + fmt.Fprint(cmd.OutOrStdout(), string(data)) + return nil +} + +func showRule(cmd *cobra.Command) error { + t, err := newManager().Table(cmd.Context()) + if err != nil { + return err + } + r, ok := t.Rule(flagName) + if !ok { + return errorx.IllegalArgument.New("no rule named %q; known rules are %s", flagName, strings.Join(t.Names(), ", ")) + } + data, err := yaml.Marshal(r) + if err != nil { + return errorx.InternalError.Wrap(err, "failed to render rule %q as YAML", flagName) + } + fmt.Fprint(cmd.OutOrStdout(), string(data)) + return nil +} + +func init() { + showCmd.Flags().StringVar(&flagName, "name", "", "Show only this rule: a reserved block (mgmt, blocked, in_cluster) or a named allow rule") + showCmd.Flags().StringVar(&flagOutput, "output", outputNft, "Output format: nft (the live ruleset) or yaml (the declarative config)") +} diff --git a/docs/dev/traffic-shaper.md b/docs/dev/traffic-shaper.md index 69212e39..e0ba3a92 100644 --- a/docs/dev/traffic-shaper.md +++ b/docs/dev/traffic-shaper.md @@ -185,6 +185,7 @@ the systemd units under `/usr/lib/systemd/system/`. ``` /etc/solo-provisioner/ network-weaver-host-firewall.nft # inet weaver-host-firewall table (full ruleset) + network-weaver-host-firewall.yaml # its declarative config; source of truth for the host-firewall verbs network-weaver-workload-policy.nft # inet weaver-workload-policy table (chain + set decls) policies/ # one JSON per policy; source of truth for workload-policy rules network/shape/ @@ -337,12 +338,36 @@ all. The `input` copy is redundant with `prerouting` for anything arriving on a kept so the block list's ordering relative to the conntrack fast-path stays a property of the `input` chain itself rather than a consequence of a chain on another hook. -On `input`, the only broad escapes are the mgmt allowlist on the SSH port, `in_cluster_ports` -from the pod CIDR, the ICMP path-health subset, and `ct state established,related`. Everything -else delivered to that host is dropped on new connections. On a single-purpose block-node host -that is the intent, but it is a node-wide decision, not a block-node-scoped one — a second CNI, -a docker bridge, a VPN, DHCPv6, or cross-node kubelet/etcd/NodePort traffic all need an -explicit rule or they are dropped. +On `input`, the only broad escapes are the mgmt allowlist on `mgmt_ports`, `in_cluster_ports` +from the pod CIDR, whatever named allow rules the operator declared, the ICMP path-health +subset, and `ct state established,related`. Everything else delivered to that host is dropped on +new connections. On a single-purpose block-node host that is the intent, but it is a node-wide +decision, not a block-node-scoped one — a second CNI, a docker bridge, a VPN, DHCPv6, or +cross-node kubelet/etcd/NodePort traffic all need an explicit rule or they are dropped. + +Those explicit rules are what the **named allow rules** are for. Each is a source list x port +list x protocol accept, rendered per family as +` saddr @ dport @_ports accept` into `input_ipv4` / `input_ipv6`. +They cover the axes the three reserved blocks cannot: UDP (Cilium's VXLAN 8472), port ranges +(`2379-2380`, `10256-10259`), more than one management group, and per-source unmetered ICMP +echo. They exist because weaver has to be able to express a *complete* host ruleset on hardware +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. + +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 +`icmp_echo` rule renders *above* the meter, because the meter drops over-budget echo outright — +an accept placed after it would never be reached under a flood, which is exactly when an +operator needs their own ping to work. + +Block-node service ports deliberately have no home here. That traffic is forwarded rather than +delivered locally, so an `input` rule for it would never match; peer access to block-node ports +is the workload policy plane's concern. On `forward`, weaver constrains nothing. A packet matching no classification rule is accepted carrying no `meta priority` and lands in the HTB default class. Workload isolation on that hook @@ -397,7 +422,9 @@ asked first**, then traffic shaping: - `--firewall-enabled` — install the `inet weaver-host-firewall` plane. Configured by `--mgmt-cidrs`, `--blocked-cidrs`, `--ssh-port`, `--pod-cidr`, - `--in-cluster-ports`. + `--in-cluster-ports` — i.e. the three reserved blocks only. Named allow rules + are not part of install: they are declared afterwards with + `network firewall create --from-file`, and a later `reconfigure` preserves them. - `--traffic-shaping-enabled` — the single switch that wires up **all three** shaping pieces: the workload policy plane (`inet weaver-workload-policy`), the tc HTB hierarchies, and the traffic-shaper daemon. Only when this is accepted @@ -419,8 +446,13 @@ provisioned node. - **`network firewall`** (`create`/`add`/`remove`/`set`/`show`/`delete`) — the host firewall. `create` takes `--mgmt-cidrs`, `--blocked-cidrs`, - `--in-cluster-ports`, `--ssh-port`, `--pod-cidr`; `add`/`remove` take the - singular forms; `set` atomically replaces a full list. + `--in-cluster-ports`, `--ssh-port`, `--pod-cidr`, or `--from-file` for the + whole table; `add`/`remove`/`set`/`delete` take `--name` to address one rule — + a reserved block (`mgmt`, `blocked`, `in_cluster`) or a named allow rule — with + the per-block flags retained as shorthands. Structure (which rules exist, and + their protocol) is file-only; membership is CLI-mutable, because adding a rule + is a reviewed change while unblocking an operator is sometimes urgent. + `show --output yaml` emits the same schema `--from-file` accepts. - **`network policy`** (`create`/`add`/`remove`/`set`/`show`/`delete`) — the workload policy plane. `create` takes `--name` (the nft set name), `--stamp` (the HTB class to classify into, which also fixes direction) or `--deny`, plus @@ -437,8 +469,14 @@ provisioned node. During `block node install` / `reconfigure` the workflow lays these down: - **Host firewall** — the firewall-create step renders - `network-weaver-host-firewall.nft`, then `EnsureNetworkNftUnit` installs and - enables `solo-provisioner-network-nft.service` and restarts it. + `network-weaver-host-firewall.yaml` and then `network-weaver-host-firewall.nft` + (config first: a crash between the two leaves the operator's intent recorded + and the kernel merely stale, which the next apply fixes), then + `EnsureNetworkNftUnit` installs and enables + `solo-provisioner-network-nft.service` and restarts it. The step owns only the + reserved blocks, which come from `config.yaml` / the install flags; it carries + any named allow rules across unchanged, so a `reconfigure` force re-render does + not drop rules `config.yaml` has no field for. - **Workload policy** — `NftWeaverPersist` (`internal/workflows/steps/step_network_nft_weaver.go`) re-renders `network-weaver-workload-policy.nft` from the policy registry, ensures the diff --git a/docs/quickstart.md b/docs/quickstart.md index 9a986b02..b0c4a3c3 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -645,11 +645,18 @@ sudo solo-provisioner kube cluster uninstall --continue-on-error ### Network Commands -Manage node-level network state behind the traffic shaper. The `firewall` scope manages the node-agnostic `inet weaver-host-firewall` nftables table — the host's own SSH/management allowlist, ICMP policy, and in-cluster host-service ports. It is separate from the `inet weaver-workload-policy` workload plane and applies to every node type (block, consensus, mirror, relay). +Manage node-level network state behind the traffic shaper. The `firewall` scope manages the node-agnostic `inet weaver-host-firewall` nftables table — the host's own management allowlist, ICMP policy, in-cluster host-service ports, and any number of named allow rules. It is separate from the `inet weaver-workload-policy` workload plane and applies to every node type (block, consensus, mirror, relay). + +The table holds two kinds of record: + +- **Three reserved blocks** — `mgmt` (management allowlist), `blocked` (operator block list, dropped on `prerouting`, `input` and `output`), and `in_cluster` (host-service ports reachable from the pod CIDR). They are first-class because weaver derives or defaults their content and omitting one is dangerous. +- **Named allow rules** — an operator-authored source list x port list x protocol accept, for anything else the host must admit (Kubernetes control-plane ports, Cilium VXLAN, an admin jump host). + +Structure — which rules exist, and what protocol each matches — is declared in a config file. Membership — the addresses and ports inside a rule — is mutable straight from the CLI. That split is deliberate: adding a rule is a reviewed change, while unblocking an operator is sometimes urgent. #### Create the Host Firewall -create-if-missing: if the `inet weaver-host-firewall` table already exists, the command makes no changes unless `--force` is passed (which re-renders from the flags). Every mutation applies to the live kernel in one atomic `nft -f` transaction and atomically rewrites `/etc/solo-provisioner/network-weaver-host-firewall.nft`. +create-if-missing: if the `inet weaver-host-firewall` table already exists, the command makes no changes unless `--force` is passed (which re-renders from the flags or file). Every mutation applies to the live kernel in one atomic `nft -f` transaction and atomically rewrites both `/etc/solo-provisioner/network-weaver-host-firewall.nft` and the config it was rendered from, `/etc/solo-provisioner/network-weaver-host-firewall.yaml`. ```bash # Create with a management allowlist and the default in-cluster ports @@ -667,40 +674,106 @@ sudo solo-provisioner network firewall create --mgmt-cidrs 10.0.0.0/8,192.168.0. | Flag | Description | Default | |----------------------|-------------------------------------------------------------------|--------------------| -| `--mgmt-cidrs` | Management/SSH allowlist CIDRs (comma-separated or repeated) — **omitting this flag leaves the SSH allow rule with an empty source set under the default-drop policy, which will lock you out of new SSH connections** | (none) | +| `--mgmt-cidrs` | Management/SSH allowlist CIDRs (comma-separated or repeated) — **omitting this flag leaves the management allow rule with an empty source set under the default-drop policy, which will lock you out of new SSH connections** | (none) | +| `--blocked-cidrs` | Operator block list CIDRs, dropped before any other rule | (none) | | `--in-cluster-ports` | Host-service ports reachable from the pod CIDR | `4244,6443,7472,10250` | -| `--ssh-port` | SSH/management TCP port accepted from the allowlist | `22` | +| `--ssh-port` | Management TCP port accepted from the allowlist (shorthand for a one-element `mgmt.ports`) | `22` | | `--pod-cidr` | Pod CIDR allowed to reach the in-cluster host-service ports | auto-detected | +| `--from-file` | Declarative YAML config to render the whole table from (mutually exclusive with the flags above) | (none) | | `--force` | Re-render the table even if it already exists (global flag) | `false` | When `--pod-cidr` is omitted it is **auto-detected** from the local node's `.spec.podCIDR` via the Kubernetes API (the node is matched by hostname, or the sole node on a single-node host). Detection is best-effort: `network firewall create` is node-agnostic and may run before a cluster exists, so if no cluster is reachable the command logs a warning and **omits the in-cluster-ports rule** — pass `--pod-cidr` explicitly to render it anyway. -ICMP is a fixed, safe ruleset (not configurable): full ICMP from the management allowlist, and from every other source the path-health subset — `destination-unreachable` (Path MTU Discovery) and `time-exceeded` (traceroute) always accepted, with `echo-request` (ping) rate-limited to 10/second. There are deliberately no ICMP flags: dropping ICMP errors would silently break PMTUD for legitimate clients. +ICMP is a fixed, safe ruleset: full ICMP from the management allowlist, and from every other source the path-health subset — `destination-unreachable` (Path MTU Discovery) and `time-exceeded` (traceroute) always accepted, with `echo-request` (ping) rate-limited to 10/second. There are deliberately no ICMP flags: dropping ICMP errors would silently break PMTUD for legitimate clients. The one configurable part is `icmp_echo` on an allow rule, which grants that rule's sources unmetered `echo-request`. + +> There is no `--service-ports`: BN ports live only in `network policy --ports`. That traffic is forwarded rather than delivered locally, so an `input` rule for it would never match. + +#### Declare Named Allow Rules + +`--from-file` is the only way to declare a named allow rule, and it renders the whole table: + +```yaml +version: 1 + +mgmt: + cidrs: ["192.168.68.0/24"] + ports: ["22"] -> There is no `--service-ports`: BN ports live only in `network policy --ports` (the host firewall is bypassed by the eBPF datapath). +blocked: + cidrs: [] -#### Modify the Allowlist / Ports +in_cluster: # both fields optional + cidrs: ["10.4.0.0/14"] # omitted -> auto-detected + ports: ["4244", "6443", "7472", "10250"] # omitted -> the defaults above -`add`/`remove` operate on a single element; `set` atomically replaces the full list. +allow: + - name: k8s-node + cidrs: ["10.0.0.0/24"] + ports: ["6443", "2379-2380", "10250", "10256-10259"] + proto: tcp + + - name: cilium-vxlan + cidrs: ["10.0.0.0/24"] + ports: ["8472"] + proto: udp + + - name: admin + cidrs: ["203.0.113.5/32", "2001:db8:5e5::/64"] + ports: ["22"] + icmp_echo: true +``` ```bash -sudo solo-provisioner network firewall add --mgmt-cidr 10.1.0.0/16 -sudo solo-provisioner network firewall remove --mgmt-cidr 10.0.0.0/8 -sudo solo-provisioner network firewall set --mgmt-cidrs 10.0.0.0/8,192.168.0.0/16 +sudo solo-provisioner network firewall create --from-file rules.yaml --force +``` -sudo solo-provisioner network firewall add --in-cluster-port 9100 -sudo solo-provisioner network firewall remove --in-cluster-port 10250 -sudo solo-provisioner network firewall set --in-cluster-ports 6443,4244 +| Field | Required | Notes | +|-------------|----------|------------------------------------------------------------------------------| +| `name` | yes | Also the nft set name. `mgmt`, `blocked` and `in_cluster` are reserved. | +| `cidrs` | yes | IPv4 and/or IPv6 in one list; each entry is routed to `@` or `@6` by family | +| `ports` | yes\* | Single ports and inclusive ranges (`2379-2380`). \*Optional when `icmp_echo` is set, for an echo-only rule | +| `proto` | no | `tcp` (default) or `udp`. nft has no combined match, so a service on both is two rules | +| `icmp_echo` | no | Grants unmetered `echo-request`, rendered above the rate meter | + +Two semantics differ deliberately between the record kinds: + +- **`allow:` is declarative** — a rule absent from the file is **deleted**. +- **Reserved blocks absent from the file are derived or defaulted, never deleted**, so a partial file cannot silently drop management access. To disable one, give it an empty list (`in_cluster: {cidrs: []}`). + +#### Modify a Rule's Addresses / Ports + +`add`/`remove` merge with what is already there; `set` atomically replaces the full list. `--name` selects the rule — a reserved block or an allow rule: + +```bash +sudo solo-provisioner network firewall add --name mgmt --cidr 10.1.0.0/16 +sudo solo-provisioner network firewall add --name blocked --cidr 203.0.113.9/32 +sudo solo-provisioner network firewall add --name k8s-node --cidr 10.0.0.5/32 --port 9345 +sudo solo-provisioner network firewall remove --name k8s-node --port 9345 +sudo solo-provisioner network firewall set --name mgmt --cidrs 10.0.0.0/8,192.168.0.0/16 +sudo solo-provisioner network firewall set --name mgmt --cidrs-file /etc/mgmt-cidrs.txt ``` **Flags**: -| Verb | Flag | Description | -|----------------|----------------------|----------------------------------------------------------------------| -| `add`/`remove` | `--mgmt-cidr` | A single management CIDR (mutually exclusive with `--in-cluster-port`) | -| `add`/`remove` | `--in-cluster-port` | A single in-cluster host-service port | -| `set` | `--mgmt-cidrs` | Full management allowlist (replaces the existing list) | -| `set` | `--in-cluster-ports` | Full in-cluster host-service port list (replaces the existing list) | +| Verb | Flag | Description | +|-----------------------|----------------|----------------------------------------------------------------------| +| `add`/`remove`/`set` | `--name` | Rule to modify: `mgmt`, `blocked`, `in_cluster`, or an allow rule name | +| `add`/`remove` | `--cidr` | CIDR(s) to add/remove (comma-separated or repeated) | +| `add`/`remove` | `--port` | Port(s) to add/remove; single ports or ranges | +| `set` | `--cidrs` | Full CIDR list (replaces the existing list; an empty value clears it) | +| `set` | `--cidrs-file` | Alternative to `--cidrs`: a flat file of CIDRs, one per line or comma-separated, `#` comments allowed | +| `set` | `--ports` | Full port list (replaces the existing list) | + +The pre-existing per-block flags are retained as shorthands that name their reserved block implicitly, so every earlier invocation still works unchanged: + +```bash +sudo solo-provisioner network firewall add --mgmt-cidr 10.1.0.0/16 # = --name mgmt --cidr +sudo solo-provisioner network firewall remove --blocked-cidr 203.0.113.9/32 +sudo solo-provisioner network firewall add --in-cluster-port 9100 +sudo solo-provisioner network firewall set --mgmt-cidrs 10.0.0.0/8 --in-cluster-ports 6443,4244 +``` + +> 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. #### Show / Delete the Host Firewall @@ -708,11 +781,29 @@ sudo solo-provisioner network firewall set --in-cluster-ports 6443,4244 # Show the live inet weaver-host-firewall table sudo solo-provisioner network firewall show -# 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 + +# Inspect one rule +sudo solo-provisioner network firewall show --name k8s-node + +# Delete one named allow rule +sudo solo-provisioner network firewall delete --name k8s-node + +# Remove the whole table and its on-disk artifacts +sudo solo-provisioner network firewall delete --all +``` + +`show --output yaml` prints exactly the schema `create --from-file` accepts, so it round-trips: + +```bash +sudo solo-provisioner network firewall show --output yaml > rules.yaml +sudo solo-provisioner network firewall create --from-file rules.yaml --force # a no-op ``` -> `delete` removes the table and `/etc/solo-provisioner/network-weaver-host-firewall.nft` but does not disable the shared `solo-provisioner-network-nft.service` (shared with `inet weaver-workload-policy`); disable it manually if you need it off. +> `delete --all` (the default when `--name` is omitted, which is what this verb has always done) removes the table and both `/etc/solo-provisioner/network-weaver-host-firewall.{nft,yaml}`, leaving the host with no weaver-managed firewall — including no management allowlist. It asks for confirmation in an interactive session; pass `--force` to skip the prompt. It does not disable the shared `solo-provisioner-network-nft.service` (shared with `inet weaver-workload-policy`); disable it manually if you need it off. +> +> The reserved blocks cannot be deleted individually — clear their addresses instead (`network firewall set --name mgmt --cidrs ""`). #### Create a Traffic Policy diff --git a/internal/network/firewall/allow_test.go b/internal/network/firewall/allow_test.go new file mode 100644 index 00000000..a868dbd1 --- /dev/null +++ b/internal/network/firewall/allow_test.go @@ -0,0 +1,451 @@ +// SPDX-License-Identifier: Apache-2.0 + +package firewall + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRender_AllowRules(t *testing.T) { + doc, err := allowTable().Render() + require.NoError(t, err) + + // 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; }") + + // Port ranges are single elements of an interval set, ordered by lower bound + // rather than lexically (10250 after 6443). + require.Contains(t, doc, "set k8s-node_ports { type inet_service; flags interval; auto-merge; elements = { 2379-2380, 6443, 10250, 10256-10259 }; }") + + // UDP renders as its own rule; nft has no combined tcp/udp dport match. + require.Contains(t, doc, "ip saddr @cilium-vxlan udp dport @cilium-vxlan_ports accept") + require.Contains(t, doc, "ip saddr @k8s-node tcp dport @k8s-node_ports accept") + + // A dual-family source list renders in both chains, against its own family's + // set each time. + require.Contains(t, doc, "ip saddr @admin tcp dport @admin_ports accept") + require.Contains(t, doc, "ip6 saddr @admin6 tcp dport @admin_ports accept") + + // Rules are emitted in name order, so the document does not churn with the + // order the operator happened to author them in. + v4 := chainBody(t, doc, "input_ipv4") + require.Less(t, strings.Index(v4, "@admin "), strings.Index(v4, "@cilium-vxlan ")) + require.Less(t, strings.Index(v4, "@cilium-vxlan "), strings.Index(v4, "@k8s-node ")) +} + +// TestRender_AllowRuleFamilyScoping pins that a rule whose sources are all one +// family emits no rule in the other family's chain. A dead `ip6 saddr @foo6` +// against an empty set would match nothing, but it would also make the rendered +// document lie about which families a rule covers. +func TestRender_AllowRuleFamilyScoping(t *testing.T) { + doc, err := allowTable().Render() + require.NoError(t, err) + + v6 := chainBody(t, doc, "input_ipv6") + // k8s-node and cilium-vxlan are IPv4-only in the fixture. + require.NotContains(t, v6, "@k8s-node6") + require.NotContains(t, v6, "@cilium-vxlan6") + // admin is dual-family, so it is present. + require.Contains(t, v6, "@admin6") + + // 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; }") +} + +// TestRender_ICMPEchoPrecedesRateMeter is the ordering pin the meter inversion +// makes necessary: the meter drops over-budget echo outright, so a named accept +// placed after it would never be reached under a flood — precisely when an +// operator needs their own ping to still work. +func TestRender_ICMPEchoPrecedesRateMeter(t *testing.T) { + doc, err := allowTable().Render() + require.NoError(t, err) + + for _, tc := range []struct { + chain, accept, mgmt, meter string + }{ + { + chain: "input_icmp_ipv4", + accept: "ip saddr @admin icmp type echo-request accept", + mgmt: "ip saddr @mgmt_addrs icmp type {", + meter: "icmp type echo-request limit rate over 10/second drop", + }, + { + chain: "input_icmp_ipv6", + accept: "ip6 saddr @admin6 icmpv6 type echo-request accept", + mgmt: "ip6 saddr @mgmt_addrs6 icmpv6 type {", + meter: "icmpv6 type echo-request limit rate over 10/second drop", + }, + } { + t.Run(tc.chain, func(t *testing.T) { + body := chainBody(t, doc, tc.chain) + acceptIdx := strings.Index(body, tc.accept) + mgmtIdx := strings.Index(body, tc.mgmt) + meterIdx := strings.Index(body, tc.meter) + require.Greater(t, acceptIdx, -1, "icmp_echo rule must render an accept in %s", tc.chain) + require.Greater(t, mgmtIdx, -1) + require.Greater(t, meterIdx, -1) + require.Less(t, mgmtIdx, acceptIdx, "the named echo accept must follow the mgmt accept") + require.Less(t, acceptIdx, meterIdx, "the named echo accept must precede the rate meter") + }) + } + + // A rule without icmp_echo gets no ICMP accept at all. + require.NotContains(t, doc, "@k8s-node icmp type") +} + +// TestRender_ICMPEchoWithoutPorts covers an echo-only grant: a rule may exist to +// permit ping and nothing else, which renders no transport rule. +func TestRender_ICMPEchoWithoutPorts(t *testing.T) { + tbl := sampleTable() + require.NoError(t, tbl.UpsertAllow(Rule{Name: "ping-probe", CIDRs: []string{"198.51.100.7/32"}, ICMPEcho: true})) + + doc, err := tbl.Render() + require.NoError(t, err) + require.Contains(t, doc, "ip saddr @ping-probe icmp type echo-request accept") + require.NotContains(t, doc, "@ping-probe tcp dport") + // No ports means no port set is declared for it. + require.NotContains(t, doc, "set ping-probe_ports") +} + +func TestTable_ValidateRejects(t *testing.T) { + cases := map[string]func(*Table) error{ + "reserved name for an allow rule": func(tbl *Table) error { + return tbl.UpsertAllow(Rule{Name: RuleMgmt, CIDRs: []string{"10.0.0.0/8"}, Ports: []string{"22"}}) + }, + "blocked with ports": func(tbl *Table) error { + tbl.Blocked.Ports = []string{"22"} + return tbl.Validate() + }, + "blocked with proto": func(tbl *Table) error { + tbl.Blocked.Proto = ProtoTCP + return tbl.Validate() + }, + "mgmt with icmp_echo": func(tbl *Table) error { + tbl.Mgmt.ICMPEcho = true + return tbl.Validate() + }, + "mgmt with udp": func(tbl *Table) error { + tbl.Mgmt.Proto = ProtoUDP + return tbl.Validate() + }, + "unknown proto": func(tbl *Table) error { + return tbl.UpsertAllow(Rule{Name: "x", CIDRs: []string{"10.0.0.0/8"}, Ports: []string{"22"}, Proto: "sctp"}) + }, + "allow rule with no cidrs": func(tbl *Table) error { + return tbl.UpsertAllow(Rule{Name: "x", Ports: []string{"22"}}) + }, + "allow rule with no ports and no echo": func(tbl *Table) error { + return tbl.UpsertAllow(Rule{Name: "x", CIDRs: []string{"10.0.0.0/8"}}) + }, + "name with nft metacharacters": func(tbl *Table) error { + return tbl.UpsertAllow(Rule{Name: "a b", CIDRs: []string{"10.0.0.0/8"}, Ports: []string{"22"}}) + }, + } + for name, mutate := range cases { + t.Run(name, func(t *testing.T) { + require.Error(t, mutate(sampleTable())) + }) + } +} + +// TestTable_RejectsSetNameCollision covers the trap in deriving set names by +// suffix: two distinct rule names can claim the same nft set, and nft would +// accept the duplicate declaration and silently merge their membership. +func TestTable_RejectsSetNameCollision(t *testing.T) { + // "mgmt_addrs" as an allow rule would claim the mgmt block's address set. + shadowsReserved := sampleTable() + require.NoError(t, shadowsReserved.UpsertAllow(Rule{Name: "mgmt_addrs", CIDRs: []string{"10.0.0.0/8"}, Ports: []string{"22"}})) + require.ErrorContains(t, shadowsReserved.Validate(), "derive the nft set name") + + // "k8s6" would claim the v6 set of a rule named "k8s". + shadowsV6 := sampleTable() + require.NoError(t, shadowsV6.UpsertAllow(Rule{Name: "k8s", CIDRs: []string{"10.0.0.0/8"}, Ports: []string{"22"}})) + require.NoError(t, shadowsV6.UpsertAllow(Rule{Name: "k8s6", CIDRs: []string{"10.0.0.0/8"}, Ports: []string{"22"}})) + require.ErrorContains(t, shadowsV6.Validate(), "derive the nft set name") +} + +func TestTable_DeleteRule(t *testing.T) { + tbl := allowTable() + require.NoError(t, tbl.DeleteRule("cilium-vxlan")) + _, ok := tbl.Rule("cilium-vxlan") + require.False(t, ok) + + require.Error(t, tbl.DeleteRule("cilium-vxlan"), "deleting an absent rule is an error, not a silent no-op") + + // The reserved blocks are structural: deleting mgmt would leave a + // default-drop input chain with no way in. + for _, name := range ReservedNames { + require.ErrorContains(t, tbl.DeleteRule(name), "reserved block") + } +} + +func TestPortSpec(t *testing.T) { + for _, ok := range []string{"1", "22", "65535", "2379-2380", "10256-10259", "1-65535"} { + require.NoError(t, validatePortSpec(ok), "%q should be a valid port spec", ok) + } + for _, bad := range []string{"", "0", "65536", "-1", "22-", "-22", "2380-2379", "22-70000", "http", "22,23", "2379..2380"} { + require.Error(t, validatePortSpec(bad), "%q should be rejected", bad) + } +} + +// TestRemovePortsIsExact pins that removal does not split a range. Removing 2379 +// from a rule holding 2379-2380 leaves the range intact: an nft range is one set +// element, and silently rewriting it into 2380 would be a surprising way for a +// firewall to change. +func TestRemovePortsIsExact(t *testing.T) { + r := Rule{Name: "x", CIDRs: []string{"10.0.0.0/8"}, Ports: []string{"2379-2380", "6443"}} + r.RemovePorts([]string{"2379"}) + require.Equal(t, []string{"2379-2380", "6443"}, r.Ports) + + r.RemovePorts([]string{"2379-2380"}) + require.Equal(t, []string{"6443"}, r.Ports) +} + +func TestConfig_RoundTrip(t *testing.T) { + first, err := FileConfigFromTable(allowTable()).Marshal() + require.NoError(t, err) + + cfg, err := ParseConfig(first) + require.NoError(t, err) + tbl, err := cfg.Table() + require.NoError(t, err) + + second, err := FileConfigFromTable(tbl).Marshal() + require.NoError(t, err) + require.Equal(t, string(first), string(second), "config→YAML→config must be the identity") + + // And the ruleset the reloaded config renders is byte-identical. + wantDoc, err := allowTable().Render() + require.NoError(t, err) + gotDoc, err := tbl.Render() + require.NoError(t, err) + require.Equal(t, wantDoc, gotDoc) +} + +// TestConfig_OmittedVsEmptyReservedBlock pins the distinction the reserved-block +// semantics rest on, which is carried by nil-vs-empty on a decoded slice: an +// omitted block is derived or defaulted, while a block present with an empty +// list renders no rule. If the YAML decoder ever stopped distinguishing the two, +// `in_cluster: {cidrs: []}` would silently start auto-detecting the pod CIDR +// again. +func TestConfig_OmittedVsEmptyReservedBlock(t *testing.T) { + omitted, err := ParseConfig([]byte("version: 1\nmgmt:\n cidrs: [\"10.0.0.0/8\"]\n")) + require.NoError(t, err) + require.True(t, omitted.InClusterCIDRsUnset(), "an omitted in_cluster block must be reported as unset") + + present, err := ParseConfig([]byte("version: 1\nmgmt:\n cidrs: [\"10.0.0.0/8\"]\nin_cluster:\n cidrs: []\n")) + require.NoError(t, err) + require.False(t, present.InClusterCIDRsUnset(), "an explicitly empty cidrs list must be reported as set") + + // The explicitly-empty form renders no in-cluster rule; the omitted form + // leaves the caller to fill the addresses in. + tbl, err := present.Table() + require.NoError(t, err) + doc, err := tbl.Render() + require.NoError(t, err) + require.NotContains(t, doc, "tcp dport @in_cluster_ports accept") + + // Ports omitted still means "the stack default set", not "no ports". + require.Equal(t, PortStrings(DefaultInClusterPorts), tbl.InCluster.Ports) +} + +func TestConfig_Rejects(t *testing.T) { + cases := map[string]string{ + "unknown top-level key": "version: 1\nallowed:\n - name: x\n", + "unknown rule key": "version: 1\nallow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n protocol: tcp\n", + "future version": "version: 99\nmgmt:\n cidrs: []\n", + "reserved allow name": "version: 1\nallow:\n - name: mgmt\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n", + "duplicate allow name": "version: 1\nallow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n - name: x\n cidrs: [\"10.1.0.0/16\"]\n ports: [\"80\"]\n", + "bad cidr": "version: 1\nmgmt:\n cidrs: [\"10.0.0.0\"]\n", + "bad port range": "version: 1\nallow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"2380-2379\"]\n", + "blocked with ports": "version: 1\nblocked:\n cidrs: []\n ports: [\"22\"]\n", + } + for name, doc := range cases { + t.Run(name, func(t *testing.T) { + _, err := ParseConfig([]byte(doc)) + require.Error(t, err) + }) + } +} + +// TestConfig_MissingVersionIsAccepted keeps a hand-written file that forgot +// `version:` working, while a version this build does not know is still refused +// (covered above) — ignoring a field a newer weaver understands could leave the +// host with a firewall that does not match the file. +func TestConfig_MissingVersionIsAccepted(t *testing.T) { + cfg, err := ParseConfig([]byte("mgmt:\n cidrs: [\"10.0.0.0/8\"]\n")) + require.NoError(t, err) + tbl, err := cfg.Table() + require.NoError(t, err) + require.Equal(t, []string{"10.0.0.0/8"}, tbl.Mgmt.CIDRs) +} + +// TestManager_ApplyIsDeclarativeForAllowOnly pins the deliberate asymmetry +// between the two kinds of record: an allow rule absent from an applied config is +// removed, while a reserved block absent from it is defaulted rather than wiped — +// so a partial file cannot silently drop management access. +func TestManager_ApplyIsDeclarativeForAllowOnly(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + ctx := context.Background() + + require.NoError(t, m.Apply(ctx, allowTable())) + require.Contains(t, readNft(t, nftPath), "@cilium-vxlan") + + // A config naming only one allow rule drops the others. + cfg, err := ParseConfig([]byte( + "version: 1\n" + + "mgmt:\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n" + + "allow:\n - name: k8s-node\n cidrs: [\"10.0.0.0/24\"]\n ports: [\"6443\"]\n")) + require.NoError(t, err) + tbl, err := cfg.Table() + require.NoError(t, err) + require.NoError(t, m.Apply(ctx, tbl)) + + doc := readNft(t, nftPath) + require.Contains(t, doc, "@k8s-node") + require.NotContains(t, doc, "@cilium-vxlan") + require.NotContains(t, doc, "@admin") + + // The omitted in_cluster block came back as the default port set rather than + // as nothing. + require.Contains(t, doc, "set in_cluster_ports { type inet_service; flags interval; auto-merge; elements = { 4244, 6443, 7472, 10250 }; }") +} + +// TestManager_ConfigRoundTripsThroughDisk is the operator-facing round-trip: +// `show --output yaml` piped back into `create --from-file` changes nothing. +func TestManager_ConfigRoundTripsThroughDisk(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + ctx := context.Background() + + require.NoError(t, m.Apply(ctx, allowTable())) + firstDoc := readNft(t, nftPath) + + shown, err := m.Config(ctx) + require.NoError(t, err) + data, err := shown.Marshal() + require.NoError(t, err) + + reloaded, err := ParseConfig(data) + require.NoError(t, err) + tbl, err := reloaded.Table() + require.NoError(t, err) + require.NoError(t, m.Apply(ctx, tbl)) + + require.Equal(t, firstDoc, readNft(t, nftPath), "re-applying the shown config must be a no-op") +} + +// TestManager_LoadsFromLegacyNftWhenConfigMissing covers the upgrade path: a host +// provisioned before the config file existed must still be mutable, recovering +// its management allowlist from the rendered ruleset rather than erroring out. +func TestManager_LoadsFromLegacyNftWhenConfigMissing(t *testing.T) { + r := &fakeRunner{exists: true} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + ctx := context.Background() + + // The pre-allow-rules rendering: the management port and the pod CIDR were + // rule literals rather than sets, and neither in_cluster_addrs nor mgmt_ports + // existed. + legacy := `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 = { 192.168.68.0/24 }; } + set mgmt_addrs6 { type ipv6_addr; flags interval; } + 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_ports { type inet_service; elements = { 6443 }; } + + chain input_ipv4 { + ip saddr @mgmt_addrs tcp dport 2222 accept + ip saddr 10.4.0.0/24 tcp dport @in_cluster_ports accept + } +} +` + require.NoError(t, os.WriteFile(nftPath, []byte(legacy), 0o644)) + + tbl, err := m.Table(ctx) + require.NoError(t, err) + require.Equal(t, []string{"192.168.68.0/24"}, tbl.Mgmt.CIDRs) + require.Equal(t, []string{"2222"}, tbl.Mgmt.Ports, "the literal SSH port must be recovered as a one-element list") + require.Equal(t, []string{"203.0.113.0/24"}, tbl.Blocked.CIDRs) + require.Equal(t, []string{"10.4.0.0/24"}, tbl.InCluster.CIDRs, "the literal pod CIDR must be recovered into the set") + require.Equal(t, []string{"6443"}, tbl.InCluster.Ports) + + // And a mutation now works, writing the config file so the fallback is not + // needed again. + require.NoError(t, m.Add(ctx, RuleMgmt, []string{"198.51.100.4/32"}, nil)) + doc := readNft(t, nftPath) + require.Contains(t, doc, "198.51.100.4/32") + require.Contains(t, doc, "192.168.68.0/24", "the recovered allowlist must survive the mutation") + require.Contains(t, doc, "tcp dport @mgmt_ports accept", "the mutation re-renders in the current form") +} + +// TestParse_RecoversReservedBlocksOnly states the fallback's limit outright: it +// is not a general nft parser, and named allow rules are not recovered from a +// ruleset. Losing the config file loses the allow rules, which are re-appliable; +// it must never lose management access, which is not. +func TestParse_RecoversReservedBlocksOnly(t *testing.T) { + doc, err := allowTable().Render() + require.NoError(t, err) + + parsed, err := Parse(doc) + require.NoError(t, err) + require.Equal(t, allowTable().Mgmt.CIDRs, parsed.Mgmt.CIDRs) + require.Empty(t, parsed.Allow, "allow rules are deliberately not reverse-engineered from the ruleset") +} + +func TestManager_DeleteRemovesConfigFile(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + dir := t.TempDir() + nftPath := filepath.Join(dir, "network-weaver-host-firewall.nft") + configPath := filepath.Join(dir, "network-weaver-host-firewall.yaml") + m := NewManagerWithConfig(Config{ + Runner: r, + NftPath: nftPath, + ConfigPath: configPath, + LockPath: filepath.Join(dir, ".applying"), + ApplyViaService: func(context.Context) error { + applyCount++ + r.exists = true + return nil + }, + }) + ctx := context.Background() + + require.NoError(t, m.Apply(ctx, allowTable())) + require.FileExists(t, configPath) + + require.NoError(t, m.Delete(ctx)) + require.NoFileExists(t, nftPath) + require.NoFileExists(t, configPath) +} + +func TestRender_AllowGoldenStable(t *testing.T) { + goldenPath := filepath.Join("testdata", "network-weaver-host-firewall-allow.golden.nft") + doc, err := allowTable().Render() + require.NoError(t, err) + + if *update { + require.NoError(t, os.WriteFile(goldenPath, []byte(doc), 0o644)) + } + + want, err := os.ReadFile(goldenPath) + require.NoError(t, err) + require.Equal(t, strings.TrimSpace(string(want)), strings.TrimSpace(doc)) +} diff --git a/internal/network/firewall/config.go b/internal/network/firewall/config.go new file mode 100644 index 00000000..0502becb --- /dev/null +++ b/internal/network/firewall/config.go @@ -0,0 +1,198 @@ +// SPDX-License-Identifier: Apache-2.0 + +package firewall + +import ( + "bytes" + "os" + + "github.com/hashgraph/solo-weaver/pkg/sanity" + "github.com/joomcode/errorx" + "gopkg.in/yaml.v3" +) + +// ConfigVersion is the schema version this build writes. A file may omit +// `version` (treated as the current schema) but may not declare a newer one: +// silently ignoring a field a future weaver understands could leave a host with +// a firewall narrower — or wider — than the file says. +const ConfigVersion = 1 + +// FileConfig is the declarative form of a Table: the schema of the +// `network firewall create --from-file` input, of `network firewall show +// --output yaml`, and of the persisted config the mutating verbs load. Those +// three being one type is what makes the round-trip exact — the output of +// `show --output yaml` re-applied via `--from-file` is a no-op by construction, +// not by coincidence. +// +// The reserved blocks are pointers so an absent section is distinguishable from +// an empty one, which the semantics depend on: an omitted block is derived or +// defaulted, while a block present with an empty list renders no rule. `allow` +// needs no such distinction because it is wholly declarative — an entry absent +// from the file is deleted. +type FileConfig struct { + Version int `yaml:"version"` + Mgmt *Block `yaml:"mgmt,omitempty"` + Blocked *Block `yaml:"blocked,omitempty"` + InCluster *Block `yaml:"in_cluster,omitempty"` + Allow []Rule `yaml:"allow,omitempty"` +} + +// Block is a reserved section of the config file: the subset of Rule an operator +// may set on mgmt, blocked or in_cluster. It deliberately has no `name` (the +// section key is the name), no `proto` and no `icmp_echo` — the reserved blocks +// either fix those or have no use for them, and accepting the fields only to +// reject them in validation would suggest they mean something. +// +// Neither field carries `omitempty`: an empty list must survive a write as +// `cidrs: []`, because collapsing it to an absent key would turn "render no +// rule" back into "derive the default" on the next load. +type Block struct { + CIDRs []string `yaml:"cidrs"` + Ports []string `yaml:"ports"` +} + +// LoadConfigFile reads and validates a declarative firewall config. Decoding is +// strict: an unrecognised key is an error rather than a silent no-op, since a +// typo in a firewall config would otherwise present as a rule that quietly +// never took effect. +func LoadConfigFile(path string) (*FileConfig, error) { + clean, err := sanity.ValidateInputFile(path) + if err != nil { + return nil, errorx.IllegalArgument.Wrap(err, "invalid --from-file %q", path) + } + data, err := os.ReadFile(clean) + if err != nil { + return nil, errorx.ExternalError.Wrap(err, "failed to read --from-file %s", clean) + } + return ParseConfig(data) +} + +// ParseConfig decodes and validates a declarative firewall config from YAML. +func ParseConfig(data []byte) (*FileConfig, error) { + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + + var c FileConfig + if err := dec.Decode(&c); err != nil { + return nil, errorx.IllegalFormat.Wrap(err, "failed to parse firewall config") + } + if c.Version != 0 && c.Version != ConfigVersion { + return nil, errorx.IllegalFormat.New( + "unsupported firewall config version %d: this build understands version %d", c.Version, ConfigVersion) + } + + // Validate through the model rather than field by field, so the config path + // and the CLI path can never disagree about what is acceptable. + t, err := c.Table() + if err != nil { + return nil, err + } + if err := t.Validate(); err != nil { + return nil, err + } + return &c, nil +} + +// Table builds the Table this config describes, applying the defaults for any +// omitted reserved field. The in-cluster address list is the one value it cannot +// resolve on its own — see InClusterCIDRsUnset. +func (c *FileConfig) Table() (*Table, error) { + t := NewTable() + + if c.Mgmt != nil { + t.Mgmt.CIDRs = c.Mgmt.CIDRs + if c.Mgmt.Ports != nil { + t.Mgmt.Ports = c.Mgmt.Ports + } + } + if c.Blocked != nil { + t.Blocked.CIDRs = c.Blocked.CIDRs + // Carried across rather than dropped so Rule.Validate is the one place + // that rejects a port on the block list — silently ignoring the field + // would leave the operator believing they had narrowed the block. + t.Blocked.Ports = c.Blocked.Ports + } + if c.InCluster != nil { + t.InCluster.CIDRs = c.InCluster.CIDRs + if c.InCluster.Ports != nil { + t.InCluster.Ports = c.InCluster.Ports + } + } + + // UpsertAllow replaces a same-named rule, which is what the CLI wants but + // would make a file listing one name twice silently keep only the last. In a + // firewall config that is a typo worth failing on. + seen := make(map[string]struct{}, len(c.Allow)) + for _, r := range c.Allow { + if _, dup := seen[r.Name]; dup { + return nil, errorx.IllegalFormat.New("duplicate allow rule %q", r.Name) + } + seen[r.Name] = struct{}{} + if err := t.UpsertAllow(r); err != nil { + return nil, err + } + } + return t, nil +} + +// InClusterCIDRsUnset reports whether the config left the in-cluster address +// list unspecified, so the caller knows to auto-detect the node's pod CIDR. An +// explicitly empty list (`in_cluster: {cidrs: []}`) is *specified* — it means +// "render no in-cluster rule" — and must not trigger detection. +func (c *FileConfig) InClusterCIDRsUnset() bool { + return c.InCluster == nil || c.InCluster.CIDRs == nil +} + +// FileConfigFromTable is the inverse of Table: the declarative view of a table, +// with every reserved block written out explicitly so a subsequent load resolves +// to the same table without consulting a default or the cluster. +func FileConfigFromTable(t *Table) *FileConfig { + return &FileConfig{ + Version: ConfigVersion, + Mgmt: &Block{CIDRs: nonNil(t.Mgmt.CIDRs), Ports: nonNil(t.Mgmt.Ports)}, + Blocked: &Block{CIDRs: nonNil(t.Blocked.CIDRs)}, + InCluster: &Block{CIDRs: nonNil(t.InCluster.CIDRs), Ports: nonNil(t.InCluster.Ports)}, + Allow: t.Allow, + } +} + +// Marshal renders the config as YAML, for `show --output yaml` and for the +// persisted state file. +func (c *FileConfig) Marshal() ([]byte, error) { + var buf bytes.Buffer + enc := yaml.NewEncoder(&buf) + enc.SetIndent(2) + if err := enc.Encode(c); err != nil { + return nil, errorx.InternalError.Wrap(err, "failed to render firewall config as YAML") + } + if err := enc.Close(); err != nil { + return nil, errorx.InternalError.Wrap(err, "failed to render firewall config as YAML") + } + return buf.Bytes(), nil +} + +// nonNil substitutes an empty slice for a nil one, so Block's non-omitempty +// fields marshal as `[]` rather than `null`. Both re-load as an explicitly empty +// list, but `[]` is what an operator would have written by hand. +func nonNil(in []string) []string { + if in == nil { + return []string{} + } + return in +} + +// Blocked's port list is never populated: the block list drops every port, and +// Rule.Validate rejects a port on it. The field exists on Block only because one +// type serves all three reserved sections; writing it out for `blocked` would +// invite an operator to fill it in. +func (b *Block) MarshalYAML() (any, error) { + if b.Ports == nil { + return struct { + CIDRs []string `yaml:"cidrs"` + }{CIDRs: nonNil(b.CIDRs)}, nil + } + return struct { + CIDRs []string `yaml:"cidrs"` + Ports []string `yaml:"ports"` + }{CIDRs: nonNil(b.CIDRs), Ports: nonNil(b.Ports)}, nil +} diff --git a/internal/network/firewall/firewall_test.go b/internal/network/firewall/firewall_test.go index 0f64cd5a..afb676da 100644 --- a/internal/network/firewall/firewall_test.go +++ b/internal/network/firewall/firewall_test.go @@ -32,26 +32,37 @@ func (f *fakeRunner) Delete(_ context.Context) error { f.deleted = true; func (f *fakeRunner) Exists(_ context.Context) (bool, error) { return f.exists, nil } func sampleTable() *Table { - return &Table{ - MgmtCIDRs: []string{"10.0.0.0/8", "192.168.0.0/16"}, - BlockedCIDRs: []string{"203.0.113.0/24"}, - InClusterPorts: []int{4244, 6443, 7472, 10250}, - SSHPort: 22, - PodCIDR: "10.4.0.0/24", - } + tbl := NewTable() + tbl.Mgmt.CIDRs = []string{"10.0.0.0/8", "192.168.0.0/16"} + tbl.Blocked.CIDRs = []string{"203.0.113.0/24"} + tbl.InCluster.CIDRs = []string{"10.4.0.0/24"} + return tbl } // dualStackTable is sampleTable with IPv6 members mixed into every dimension, // exercising the ipv6_addr sets, the `ip6` rules, and the v6 in-cluster rule. func dualStackTable() *Table { - return &Table{ - MgmtCIDRs: []string{"10.0.0.0/8", "2001:db8:a11::/48"}, - BlockedCIDRs: []string{"203.0.113.0/24", "2001:db8:bad::/48"}, - InClusterPorts: []int{4244, 6443, 7472, 10250}, - SSHPort: 22, - PodCIDR: "10.4.0.0/24", - PodCIDR6: "2001:db8:c0de::/64", + tbl := NewTable() + tbl.Mgmt.CIDRs = []string{"10.0.0.0/8", "2001:db8:a11::/48"} + tbl.Blocked.CIDRs = []string{"203.0.113.0/24", "2001:db8:bad::/48"} + tbl.InCluster.CIDRs = []string{"10.4.0.0/24", "2001:db8:c0de::/64"} + return tbl +} + +// allowTable is sampleTable plus the named allow rules, covering every axis they +// add: UDP, a port range, a dual-family source list, and an icmp_echo grant. +func allowTable() *Table { + tbl := dualStackTable() + for _, r := range []Rule{ + {Name: "k8s-node", CIDRs: []string{"10.0.0.0/24"}, Ports: []string{"6443", "2379-2380", "10250", "10256-10259"}}, + {Name: "cilium-vxlan", CIDRs: []string{"10.0.0.0/24"}, Ports: []string{"8472"}, Proto: ProtoUDP}, + {Name: "admin", CIDRs: []string{"203.0.113.5/32", "2001:db8:5e5::/64"}, Ports: []string{"22"}, ICMPEcho: true}, + } { + if err := tbl.UpsertAllow(r); err != nil { + panic(err) + } } + return tbl } // chainBody returns the rules of the named chain from a rendered document, @@ -85,9 +96,10 @@ func newTestManager(t *testing.T, r *fakeRunner, applyCount *int) (*Manager, str dir := t.TempDir() nftPath := filepath.Join(dir, "network-weaver-host-firewall.nft") m := NewManagerWithConfig(Config{ - Runner: r, - NftPath: nftPath, - LockPath: filepath.Join(dir, ".applying"), + Runner: r, + NftPath: nftPath, + ConfigPath: filepath.Join(dir, "network-weaver-host-firewall.yaml"), + LockPath: filepath.Join(dir, ".applying"), ApplyViaService: func(context.Context) error { *applyCount++ r.exists = true @@ -97,6 +109,14 @@ func newTestManager(t *testing.T, r *fakeRunner, applyCount *int) (*Manager, str return m, nftPath } +// readNft returns the rendered artifact the manager last persisted. +func readNft(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + return string(data) +} + func TestRender_SecurityInvariants(t *testing.T) { doc, err := sampleTable().Render() require.NoError(t, err) @@ -104,7 +124,7 @@ func TestRender_SecurityInvariants(t *testing.T) { // The chain must default-drop and must always admit SSH from the mgmt // allowlist — a default-drop without an SSH allow would lock the host out. require.Contains(t, doc, "policy drop;") - require.Contains(t, doc, "ip saddr @mgmt_addrs tcp dport 22 accept") + require.Contains(t, doc, "ip saddr @mgmt_addrs tcp dport @mgmt_ports accept") require.Contains(t, doc, "elements = { 10.0.0.0/8, 192.168.0.0/16 }") require.Contains(t, doc, "elements = { 4244, 6443, 7472, 10250 }") // The operator block list is a distinct set from the mgmt allowlist and @@ -119,7 +139,7 @@ func TestRender_SecurityInvariants(t *testing.T) { // set would share one bucket and let a ping flood starve the error signals. require.Contains(t, doc, "icmp type echo-request limit rate over 10/second drop") require.Contains(t, doc, "icmp type { destination-unreachable, time-exceeded, echo-request } accept") - require.Contains(t, doc, "ip saddr 10.4.0.0/24 tcp dport @in_cluster_ports accept") + require.Contains(t, doc, "ip saddr @in_cluster_addrs tcp dport @in_cluster_ports accept") // Ordering is load-bearing. ICMP must be evaluated BEFORE the conntrack // fast-path: netfilter conntrack tracks ICMP echo flows, so if the @@ -198,11 +218,11 @@ func TestRender_FamilySplit(t *testing.T) { "meta nfproto vmap { ipv4 : jump input_ipv4, ipv6 : jump input_ipv6 }") v4 := chainBody(t, doc, "input_ipv4") - require.Contains(t, v4, "ip saddr @mgmt_addrs tcp dport 22 accept") + require.Contains(t, v4, "ip saddr @mgmt_addrs tcp dport @mgmt_ports accept") require.NotContains(t, v4, "ip6 ") v6 := chainBody(t, doc, "input_ipv6") - require.Contains(t, v6, "ip6 saddr @mgmt_addrs6 tcp dport 22 accept") + require.Contains(t, v6, "ip6 saddr @mgmt_addrs6 tcp dport @mgmt_ports accept") require.NotContains(t, v6, "ip saddr") icmp4 := chainBody(t, doc, "input_icmp_ipv4") @@ -241,9 +261,9 @@ func TestRender_DualStack(t *testing.T) { // Parallel v6 match rules. require.Contains(t, doc, "ip6 saddr @blocked_addrs6 drop") - require.Contains(t, doc, "ip6 saddr @mgmt_addrs6 tcp dport 22 accept") - require.Contains(t, doc, "ip saddr 10.4.0.0/24 tcp dport @in_cluster_ports accept") - require.Contains(t, doc, "ip6 saddr 2001:db8:c0de::/64 tcp dport @in_cluster_ports accept") + require.Contains(t, doc, "ip6 saddr @mgmt_addrs6 tcp dport @mgmt_ports accept") + require.Contains(t, doc, "ip saddr @in_cluster_addrs tcp dport @in_cluster_ports accept") + require.Contains(t, doc, "ip6 saddr @in_cluster_addrs6 tcp dport @in_cluster_ports accept") // ICMPv6 Neighbor Discovery + MLD are mandatory under the default-drop policy // or IPv6 is dead; packet-too-big is the v6 PMTUD signal. nd-redirect must NOT @@ -273,14 +293,35 @@ func TestRender_DualStack(t *testing.T) { require.Less(t, icmpDispatchIdx, ctIdx, "ICMPv6 dispatch must precede the conntrack fast-path") } +// TestRoundTrip_RenderParseRender pins the nft-artifact fallback path: for a +// table of reserved blocks only, render→parse→render is the identity, so a host +// that lost its config file recovers its full state from the ruleset. Tables +// carrying named allow rules are deliberately excluded — Parse recovers the +// reserved blocks only, which TestParse_RecoversReservedBlocksOnly covers. func TestRoundTrip_RenderParseRender(t *testing.T) { + mgmtOnly := NewTable() + mgmtOnly.Mgmt.CIDRs = []string{"10.1.0.0/16"} + mgmtOnly.Mgmt.Ports = []string{"2222"} + mgmtOnly.InCluster.Ports = nil + + v6Only := NewTable() + v6Only.Mgmt.CIDRs = []string{"2001:db8:a11::/48"} + v6Only.Blocked.CIDRs = []string{"2001:db8:bad::/48"} + v6Only.InCluster.CIDRs = []string{"2001:db8:c0de::/64"} + v6Only.InCluster.Ports = []string{"6443"} + + ranges := NewTable() + ranges.Mgmt.CIDRs = []string{"10.1.0.0/16"} + ranges.Mgmt.Ports = []string{"22", "1024-1030"} + cases := map[string]*Table{ "full": sampleTable(), "dual-stack": dualStackTable(), "defaults": NewTable(), - "mgmt-only": {MgmtCIDRs: []string{"10.1.0.0/16"}, SSHPort: 2222}, - "v6-only": {MgmtCIDRs: []string{"2001:db8:a11::/48"}, BlockedCIDRs: []string{"2001:db8:bad::/48"}, SSHPort: 22, PodCIDR6: "2001:db8:c0de::/64", InClusterPorts: []int{6443}}, - "no-mgmt": {SSHPort: 22}, + "mgmt-only": mgmtOnly, + "v6-only": v6Only, + "port-range": ranges, + "no-mgmt": NewTable(), } for name, tbl := range cases { t.Run(name, func(t *testing.T) { @@ -300,9 +341,18 @@ func TestRoundTrip_RenderParseRender(t *testing.T) { func TestRender_RejectsInjection(t *testing.T) { tbl := NewTable() - tbl.MgmtCIDRs = []string{"10.0.0.0/8; reboot"} + tbl.Mgmt.CIDRs = []string{"10.0.0.0/8; reboot"} _, err := tbl.Render() require.Error(t, err) + + // The same gate applies to an allow rule's name, which reaches the document + // as an nft set identifier rather than as a set element. + named := NewTable() + require.Error(t, named.UpsertAllow(Rule{ + Name: "evil; reboot", + CIDRs: []string{"10.0.0.0/8"}, + Ports: []string{"22"}, + })) } func TestManager_CreateIsCreateIfMissing(t *testing.T) { @@ -347,34 +397,63 @@ func TestManager_AddRemoveSet(t *testing.T) { _, err := m.Create(ctx, NewTable(), false) require.NoError(t, err) - require.NoError(t, m.AddMgmtCIDR(ctx, "10.5.0.0/16")) - data, err := os.ReadFile(nftPath) - require.NoError(t, err) - require.Contains(t, string(data), "10.5.0.0/16") + require.NoError(t, m.Add(ctx, RuleMgmt, []string{"10.5.0.0/16"}, nil)) + require.Contains(t, readNft(t, nftPath), "10.5.0.0/16") - require.NoError(t, m.RemoveMgmtCIDR(ctx, "10.5.0.0/16")) - data, err = os.ReadFile(nftPath) - require.NoError(t, err) - require.NotContains(t, string(data), "10.5.0.0/16") + require.NoError(t, m.Remove(ctx, RuleMgmt, []string{"10.5.0.0/16"}, nil)) + require.NotContains(t, readNft(t, nftPath), "10.5.0.0/16") - require.NoError(t, m.AddBlockedCIDR(ctx, "203.0.113.0/24")) - data, err = os.ReadFile(nftPath) - require.NoError(t, err) - require.Contains(t, string(data), "203.0.113.0/24") + require.NoError(t, m.Add(ctx, RuleBlocked, []string{"203.0.113.0/24"}, nil)) + require.Contains(t, readNft(t, nftPath), "203.0.113.0/24") - require.NoError(t, m.RemoveBlockedCIDR(ctx, "203.0.113.0/24")) - data, err = os.ReadFile(nftPath) - require.NoError(t, err) - require.NotContains(t, string(data), "203.0.113.0/24") + require.NoError(t, m.Remove(ctx, RuleBlocked, []string{"203.0.113.0/24"}, nil)) + require.NotContains(t, readNft(t, nftPath), "203.0.113.0/24") - require.NoError(t, m.Set(ctx, []string{"172.16.0.0/12"}, []string{"198.51.100.0/24"}, []int{9100})) - data, err = os.ReadFile(nftPath) - require.NoError(t, err) - require.Contains(t, string(data), "172.16.0.0/12") - require.Contains(t, string(data), "198.51.100.0/24") - require.Contains(t, string(data), "9100") + require.NoError(t, m.SetMany(ctx, []Update{ + {Name: RuleMgmt, CIDRs: []string{"172.16.0.0/12"}}, + {Name: RuleBlocked, CIDRs: []string{"198.51.100.0/24"}}, + {Name: RuleInCluster, Ports: []string{"9100"}}, + })) + doc := readNft(t, nftPath) + require.Contains(t, doc, "172.16.0.0/12") + require.Contains(t, doc, "198.51.100.0/24") + require.Contains(t, doc, "9100") // Set replaced the port list entirely. - require.NotContains(t, string(data), "6443") + require.NotContains(t, doc, "6443") +} + +// TestManager_AddRemoveByNameReachesAllowRules pins that the element verbs treat +// a named allow rule exactly like a reserved block — the whole point of the +// --name form. +func TestManager_AddRemoveByNameReachesAllowRules(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + ctx := context.Background() + + require.NoError(t, m.Apply(ctx, allowTable())) + + require.NoError(t, m.Add(ctx, "k8s-node", []string{"10.9.0.0/24"}, []string{"9345"})) + doc := readNft(t, nftPath) + require.Contains(t, doc, "10.9.0.0/24") + require.Contains(t, doc, "9345") + + require.NoError(t, m.Remove(ctx, "k8s-node", []string{"10.9.0.0/24"}, []string{"9345"})) + doc = readNft(t, nftPath) + require.NotContains(t, doc, "10.9.0.0/24") + require.NotContains(t, doc, "9345") + + // Deleting a rule takes its sets and its input-chain rules with it. + require.NoError(t, m.DeleteRule(ctx, "cilium-vxlan")) + doc = readNft(t, nftPath) + require.NotContains(t, doc, "cilium-vxlan") + require.NotContains(t, doc, "8472") + + // A name that is not in the table is rejected rather than created: new rules + // are declared in the config file, not conjured by a membership verb. + require.Error(t, m.Add(ctx, "not-a-rule", []string{"10.0.0.0/8"}, nil)) + // And the reserved blocks cannot be deleted at all. + require.Error(t, m.DeleteRule(ctx, RuleMgmt)) } func TestManager_AddRejectsBadInput(t *testing.T) { @@ -385,11 +464,16 @@ func TestManager_AddRejectsBadInput(t *testing.T) { _, err := m.Create(ctx, NewTable(), false) require.NoError(t, err) - require.Error(t, m.AddMgmtCIDR(ctx, "not-a-cidr")) - require.Error(t, m.AddPort(ctx, 70000)) - require.Error(t, m.AddBlockedCIDR(ctx, "not-a-cidr")) + require.Error(t, m.Add(ctx, RuleMgmt, []string{"not-a-cidr"}, nil)) + require.Error(t, m.Add(ctx, RuleInCluster, nil, []string{"70000"})) + require.Error(t, m.Add(ctx, RuleBlocked, []string{"not-a-cidr"}, nil)) // A bare IP without a prefix length is still rejected (both families). - require.Error(t, m.AddMgmtCIDR(ctx, "2001:db8::1")) + require.Error(t, m.Add(ctx, RuleMgmt, []string{"2001:db8::1"}, nil)) + // An inverted or malformed range is rejected too. + require.Error(t, m.Add(ctx, RuleInCluster, nil, []string{"2380-2379"})) + require.Error(t, m.Add(ctx, RuleInCluster, nil, []string{"2379-"})) + // The block list drops every port, so accepting one here would only mislead. + require.Error(t, m.Add(ctx, RuleBlocked, nil, []string{"22"})) } func TestManager_AddAcceptsIPv6(t *testing.T) { @@ -400,31 +484,30 @@ func TestManager_AddAcceptsIPv6(t *testing.T) { _, err := m.Create(ctx, NewTable(), false) require.NoError(t, err) - // IPv6 CIDRs are now accepted and land in the ipv6_addr sets. - require.NoError(t, m.AddMgmtCIDR(ctx, "2001:db8:a11::/48")) - require.NoError(t, m.AddBlockedCIDR(ctx, "2001:db8:bad::/48")) - data, err := os.ReadFile(nftPath) - require.NoError(t, err) - require.Contains(t, string(data), "set mgmt_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:a11::/48 }; }") - require.Contains(t, string(data), "set blocked_addrs6 { type ipv6_addr; flags interval; elements = { 2001:db8:bad::/48 }; }") + // IPv6 CIDRs are accepted and land in the ipv6_addr sets. + 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 }; }") } func TestTable_Validate_AcceptsIPv6(t *testing.T) { mgmt := sampleTable() - mgmt.MgmtCIDRs = []string{"2001:db8::/32"} + mgmt.Mgmt.CIDRs = []string{"2001:db8::/32"} require.NoError(t, mgmt.Validate()) pod := sampleTable() - pod.PodCIDR6 = "2001:db8::/32" + pod.InCluster.CIDRs = []string{"2001:db8::/32"} require.NoError(t, pod.Validate()) blocked := sampleTable() - blocked.BlockedCIDRs = []string{"2001:db8::/32"} + blocked.Blocked.CIDRs = []string{"2001:db8::/32"} require.NoError(t, blocked.Validate()) // A bare IPv6 address without a prefix length is still rejected. bare := sampleTable() - bare.MgmtCIDRs = []string{"2001:db8::1"} + bare.Mgmt.CIDRs = []string{"2001:db8::1"} require.Error(t, bare.Validate()) } @@ -432,7 +515,7 @@ func TestManager_MutateBeforeCreateFails(t *testing.T) { r := &fakeRunner{} applyCount := 0 m, _ := newTestManager(t, r, &applyCount) - require.Error(t, m.AddMgmtCIDR(context.Background(), "10.0.0.0/8")) + require.Error(t, m.Add(context.Background(), RuleMgmt, []string{"10.0.0.0/8"}, nil)) } func TestManager_DeleteIsIdempotent(t *testing.T) { @@ -457,9 +540,10 @@ func TestManager_ServiceFailureReturnsError(t *testing.T) { r := &fakeRunner{} dir := t.TempDir() m := NewManagerWithConfig(Config{ - Runner: r, - NftPath: filepath.Join(dir, "network-weaver-host-firewall.nft"), - LockPath: filepath.Join(dir, ".applying"), + Runner: r, + NftPath: filepath.Join(dir, "network-weaver-host-firewall.nft"), + ConfigPath: filepath.Join(dir, "network-weaver-host-firewall.yaml"), + LockPath: filepath.Join(dir, ".applying"), ApplyViaService: func(context.Context) error { return context.DeadlineExceeded }, diff --git a/internal/network/firewall/manager.go b/internal/network/firewall/manager.go index b29971f8..68209733 100644 --- a/internal/network/firewall/manager.go +++ b/internal/network/firewall/manager.go @@ -6,6 +6,7 @@ import ( "context" "os" "path/filepath" + "strings" "syscall" "github.com/automa-saga/logx" @@ -19,6 +20,7 @@ import ( type Manager struct { runner Runner nftPath string + configPath string lockPath string applyViaService func(ctx context.Context) error } @@ -29,6 +31,7 @@ type Manager struct { type Config struct { Runner Runner NftPath string + ConfigPath string LockPath string ApplyViaService func(ctx context.Context) error } @@ -45,6 +48,7 @@ func NewManagerWithConfig(cfg Config) *Manager { m := &Manager{ runner: cfg.Runner, nftPath: cfg.NftPath, + configPath: cfg.ConfigPath, lockPath: cfg.LockPath, applyViaService: cfg.ApplyViaService, } @@ -54,6 +58,9 @@ func NewManagerWithConfig(cfg Config) *Manager { if m.nftPath == "" { m.nftPath = HostNftPath } + if m.configPath == "" { + m.configPath = HostConfigPath + } if m.lockPath == "" { m.lockPath = LockPath } @@ -86,60 +93,104 @@ func (m *Manager) Create(ctx context.Context, t *Table, force bool) (bool, error return changed, err } -// AddMgmtCIDR adds one CIDR to the management allowlist and re-renders. -func (m *Manager) AddMgmtCIDR(ctx context.Context, cidr string) error { - return m.mutate(ctx, func(t *Table) error { return t.AddMgmtCIDR(cidr) }) -} - -// RemoveMgmtCIDR removes one CIDR from the management allowlist and re-renders. -func (m *Manager) RemoveMgmtCIDR(ctx context.Context, cidr string) error { - return m.mutate(ctx, func(t *Table) error { t.RemoveMgmtCIDR(cidr); return nil }) +// Apply replaces the whole table from a declarative config and re-renders, +// regardless of whether one already exists. It is what `create --from-file` +// runs: unlike Create it is not create-if-missing, because a config file the +// operator just edited is an instruction, not a default. +func (m *Manager) Apply(ctx context.Context, t *Table) error { + if err := t.Validate(); err != nil { + return err + } + return m.withLock(func() error { return m.applyAndPersist(ctx, t) }) } -// AddBlockedCIDR adds one CIDR to the operator block list and re-renders. -func (m *Manager) AddBlockedCIDR(ctx context.Context, cidr string) error { - return m.mutate(ctx, func(t *Table) error { return t.AddBlockedCIDR(cidr) }) +// Add adds CIDRs and/or port specs to the named rule and re-renders. Adding is +// idempotent: an entry already present is left alone. +func (m *Manager) Add(ctx context.Context, name string, cidrs, ports []string) error { + return m.mutateRule(ctx, name, func(r *Rule) error { + if err := r.AddCIDRs(cidrs); err != nil { + return err + } + return r.AddPorts(ports) + }) } -// RemoveBlockedCIDR removes one CIDR from the operator block list and re-renders. -func (m *Manager) RemoveBlockedCIDR(ctx context.Context, cidr string) error { - return m.mutate(ctx, func(t *Table) error { t.RemoveBlockedCIDR(cidr); return nil }) +// Remove drops CIDRs and/or port specs from the named rule and re-renders. +// Removing an absent entry is a no-op. +func (m *Manager) Remove(ctx context.Context, name string, cidrs, ports []string) error { + return m.mutateRule(ctx, name, func(r *Rule) error { + r.RemoveCIDRs(cidrs) + r.RemovePorts(ports) + return nil + }) } -// AddPort adds one in-cluster host-service port and re-renders. -func (m *Manager) AddPort(ctx context.Context, port int) error { - return m.mutate(ctx, func(t *Table) error { return t.AddPort(port) }) +// Update is one rule's replacement membership for SetMany. A nil slice leaves +// that dimension unchanged; an empty (non-nil) slice clears it. +type Update struct { + Name string + CIDRs []string + Ports []string } -// RemovePort removes one in-cluster host-service port and re-renders. -func (m *Manager) RemovePort(ctx context.Context, port int) error { - return m.mutate(ctx, func(t *Table) error { t.RemovePort(port); return nil }) +// Set atomically replaces the named rule's address list and/or port list. +func (m *Manager) Set(ctx context.Context, name string, cidrs, ports []string) error { + return m.SetMany(ctx, []Update{{Name: name, CIDRs: cidrs, Ports: ports}}) } -// Set atomically replaces the management CIDR list, the operator block list, -// and/or the in-cluster port list. A nil slice leaves that dimension unchanged; -// an empty (non-nil) slice clears it. -func (m *Manager) Set(ctx context.Context, mgmtCIDRs, blockedCIDRs []string, ports []int) error { +// SetMany applies several rules' replacement membership in a single re-render, +// so a `set` naming more than one block lands as one nft transaction rather than +// several — a half-applied management allowlist is exactly the state worth +// avoiding here. +func (m *Manager) SetMany(ctx context.Context, updates []Update) error { return m.mutate(ctx, func(t *Table) error { - if mgmtCIDRs != nil { - if err := t.SetMgmtCIDRs(mgmtCIDRs); err != nil { - return err + for _, u := range updates { + r, ok := t.Rule(u.Name) + if !ok { + return errorx.IllegalArgument.New( + "no rule named %q; known rules are %s. New allow rules are declared in the config file (`network firewall create --from-file`)", + u.Name, strings.Join(t.Names(), ", ")) } - } - if blockedCIDRs != nil { - if err := t.SetBlockedCIDRs(blockedCIDRs); err != nil { - return err + if u.CIDRs != nil { + if err := r.SetCIDRs(u.CIDRs); err != nil { + return err + } } - } - if ports != nil { - if err := t.SetPorts(ports); err != nil { - return err + if u.Ports != nil { + if err := r.SetPorts(u.Ports); err != nil { + return err + } } } return nil }) } +// DeleteRule removes one named allow rule and re-renders. The reserved blocks +// cannot be deleted; see Table.DeleteRule. +func (m *Manager) DeleteRule(ctx context.Context, name string) error { + return m.mutate(ctx, func(t *Table) error { return t.DeleteRule(name) }) +} + +// Config returns the declarative config of the currently-configured table, for +// `show --output yaml`. Unlike Show it reads the persisted config rather than +// the kernel, so its output is the same shape that produced the ruleset — a +// kernel dump has already lost the distinction between an authored rule and a +// default, and auto-merge may have rewritten the port sets. +func (m *Manager) Config(ctx context.Context) (*FileConfig, error) { + t, err := m.Table(ctx) + if err != nil { + return nil, err + } + return FileConfigFromTable(t), nil +} + +// Table returns the currently-configured table. Read-only: no lock is taken, +// because a torn read cannot happen — the config is replaced by rename. +func (m *Manager) Table(_ context.Context) (*Table, error) { + return m.load() +} + // IsActive reports whether the inet weaver-host-firewall table is currently present in the // kernel. It is a read-only probe (no lock, no mutation) used by callers that // need the firewall's current on-host state — e.g. seeding the reconfigure @@ -178,8 +229,10 @@ func (m *Manager) Delete(ctx context.Context) error { return err } } - if err := os.Remove(m.nftPath); err != nil && !os.IsNotExist(err) { - return errorx.ExternalError.Wrap(err, "failed to remove %s", m.nftPath) + for _, p := range []string{m.nftPath, m.configPath} { + if err := os.Remove(p); err != nil && !os.IsNotExist(err) { + return errorx.ExternalError.Wrap(err, "failed to remove %s", p) + } } return nil }) @@ -200,16 +253,46 @@ func (m *Manager) mutate(ctx context.Context, fn func(*Table) error) error { }) } -// applyAndPersist atomically rewrites the on-disk artifact and then restarts -// the systemd service via DBus so the kernel picks up the new rules. The -// rendered file already contains the idempotent `add table / flush table` +// mutateRule resolves name to a rule and applies fn to it. An unknown name is +// rejected with the valid names listed, rather than silently creating a rule: +// structure (which rules exist, and their protocol) is config-file territory, +// while the CLI verbs move membership in and out of rules that already exist. +func (m *Manager) mutateRule(ctx context.Context, name string, fn func(*Rule) error) error { + return m.mutate(ctx, func(t *Table) error { + r, ok := t.Rule(name) + if !ok { + return errorx.IllegalArgument.New( + "no rule named %q; known rules are %s. New allow rules are declared in the config file (`network firewall create --from-file`)", + name, strings.Join(t.Names(), ", ")) + } + return fn(r) + }) +} + +// 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. +// +// 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. func (m *Manager) applyAndPersist(ctx context.Context, t *Table) error { block, err := t.Render() if err != nil { return err } + cfg, err := FileConfigFromTable(t).Marshal() + if err != nil { + return err + } + if err := atomicWriteFile(m.configPath, string(cfg), 0o600); err != nil { + return err + } + if err := atomicWriteFile(m.nftPath, block, 0o644); err != nil { return err } @@ -217,15 +300,33 @@ func (m *Manager) applyAndPersist(ctx context.Context, t *Table) error { return m.applyViaService(ctx) } +// 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 +// what the fallback can and cannot recover. func (m *Manager) load() (*Table, error) { - data, err := os.ReadFile(m.nftPath) + data, err := os.ReadFile(m.configPath) + switch { + case err == nil: + cfg, err := ParseConfig(data) + if err != nil { + return nil, errorx.Decorate(err, "failed to load %s", m.configPath) + } + return cfg.Table() + case !os.IsNotExist(err): + return nil, errorx.ExternalError.Wrap(err, "failed to read %s", m.configPath) + } + + 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) } return nil, errorx.ExternalError.Wrap(err, "failed to read %s", m.nftPath) } - return Parse(string(data)) + logx.As().Info().Str("path", m.nftPath).Msg( + "no host firewall config file; recovering the reserved blocks from the rendered ruleset (any named allow rules must be re-applied with --from-file)") + return Parse(string(nft)) } // withLock serialises a mutation behind the shared cross-command flock so a diff --git a/internal/network/firewall/parse.go b/internal/network/firewall/parse.go index 2ea64661..d18ba605 100644 --- a/internal/network/firewall/parse.go +++ b/internal/network/firewall/parse.go @@ -4,83 +4,102 @@ package firewall import ( "regexp" - "strconv" "strings" "github.com/joomcode/errorx" ) -// Parse reconstructs a Table from the on-disk network-weaver-host-firewall.nft artifact. It -// understands only the exact format this package renders (see the embedded -// template) — it is not a general nft parser. A render→parse→render round-trip -// is the identity, which is pinned by TestRoundTrip. Element verbs (add/remove/ -// set) use this to load prior state so they don't need the full flag set re-spec. +// Parse recovers the three reserved blocks of a Table from a rendered +// network-weaver-host-firewall.nft artifact. It understands only the exact +// formats this package renders — it is not a general nft parser. +// +// It is the fallback path, not the normal one: the persisted YAML config is the +// source of truth for the mutating verbs (see Manager.load). Parse exists so a +// host provisioned before named allow rules existed — or one whose config file +// was lost — still yields its management allowlist rather than an error that +// leaves the operator with no way to add their address back. Named allow rules +// are deliberately NOT recovered here: reverse-engineering arbitrary named rules +// out of nft syntax would be fragile in exactly the situation where being wrong +// costs the most. Recovering management access is the goal; the allow rules can +// be re-applied from the config file that describes them. +// +// Both the current and the pre-allow-rules renderings are accepted, since an +// upgraded host still has the old artifact on disk until its first mutation. func Parse(content string) (*Table, error) { - t := &Table{SSHPort: DefaultSSHPort} - if !strings.Contains(content, "table "+TableName+" {") { return nil, errorx.IllegalFormat.New("not a recognised inet weaver-host-firewall ruleset") } - // Merge each family's set back into the single mixed list the Table holds. - // Render re-splits by family, so a render→parse→render round-trip is the - // identity regardless of the mixed-list order (pinned by TestRoundTrip). - if cidrs, ok := parseElements(content, reMgmtSet); ok { - t.MgmtCIDRs = append(t.MgmtCIDRs, splitElements(cidrs)...) - } - if cidrs, ok := parseElements(content, reMgmtSet6); ok { - t.MgmtCIDRs = append(t.MgmtCIDRs, splitElements(cidrs)...) - } - if cidrs, ok := parseElements(content, reBlockedSet); ok { - t.BlockedCIDRs = append(t.BlockedCIDRs, splitElements(cidrs)...) - } - if cidrs, ok := parseElements(content, reBlockedSet6); ok { - t.BlockedCIDRs = append(t.BlockedCIDRs, splitElements(cidrs)...) + t := NewTable() + + // Merge each family's set back into the single mixed list the rule holds. + // Render re-splits by family, so the round-trip is the identity regardless of + // the mixed-list order (pinned by TestRoundTrip). + t.Mgmt.CIDRs = parseSetElements(content, reMgmtSet, reMgmtSet6) + t.Blocked.CIDRs = parseSetElements(content, reBlockedSet, reBlockedSet6) + t.InCluster.CIDRs = parseSetElements(content, reInClusterSet, reInClusterSet6) + + // A port set declared but carrying no elements means "no ports", which is + // distinct from a document that predates the set entirely. So the presence of + // the declaration decides whether to read the element list, and the element + // list — empty or not — is then authoritative. + if reMgmtPortDecl.MatchString(content) { + t.Mgmt.Ports = parseSetElements(content, reMgmtPortSet) + } else if m := reLegacySSHPort.FindStringSubmatch(content); m != nil { + // Pre-allow-rules artifact: the management port was a rule literal rather + // than a set. + t.Mgmt.Ports = []string{m[1]} } - if ports, ok := parseElements(content, rePortSet); ok { - for _, p := range splitElements(ports) { - n, err := strconv.Atoi(p) - if err != nil { - return nil, errorx.IllegalFormat.Wrap(err, "invalid in-cluster port %q in %s", p, HostNftPath) - } - t.InClusterPorts = append(t.InClusterPorts, n) - } + if reInClusterPortDecl.MatchString(content) { + t.InCluster.Ports = parseSetElements(content, reInClusterPortSet) } - if m := reSSHPort.FindStringSubmatch(content); m != nil { - n, err := strconv.Atoi(m[1]) - if err != nil { - return nil, errorx.IllegalFormat.Wrap(err, "invalid ssh port %q in %s", m[1], HostNftPath) + // Pre-allow-rules artifact: the pod CIDRs were rule literals rather than a + // set. Only consulted when the set-based form found nothing, so a current + // document is never second-guessed. + if len(t.InCluster.CIDRs) == 0 { + for _, re := range []*regexp.Regexp{reLegacyPodCIDR, reLegacyPodCIDR6} { + if m := re.FindStringSubmatch(content); m != nil { + t.InCluster.CIDRs = append(t.InCluster.CIDRs, m[1]) + } } - t.SSHPort = n - } - if m := rePodCIDR.FindStringSubmatch(content); m != nil { - t.PodCIDR = m[1] - } - if m := rePodCIDR6.FindStringSubmatch(content); m != nil { - t.PodCIDR6 = m[1] } return t, nil } var ( - reMgmtSet = regexp.MustCompile(`set mgmt_addrs \{[^}]*elements = \{ ([^}]*) \}`) - reMgmtSet6 = regexp.MustCompile(`set mgmt_addrs6 \{[^}]*elements = \{ ([^}]*) \}`) - reBlockedSet = regexp.MustCompile(`set blocked_addrs \{[^}]*elements = \{ ([^}]*) \}`) - reBlockedSet6 = regexp.MustCompile(`set blocked_addrs6 \{[^}]*elements = \{ ([^}]*) \}`) - rePortSet = regexp.MustCompile(`set in_cluster_ports \{[^}]*elements = \{ ([^}]*) \}`) - reSSHPort = regexp.MustCompile(`ip saddr @mgmt_addrs tcp dport (\d+) accept`) - rePodCIDR = regexp.MustCompile(`ip saddr (\S+) tcp dport @in_cluster_ports accept`) - rePodCIDR6 = regexp.MustCompile(`ip6 saddr (\S+) tcp dport @in_cluster_ports accept`) + reMgmtSet = regexp.MustCompile(`set mgmt_addrs \{[^}]*elements = \{ ([^}]*) \}`) + reMgmtSet6 = regexp.MustCompile(`set mgmt_addrs6 \{[^}]*elements = \{ ([^}]*) \}`) + reMgmtPortSet = regexp.MustCompile(`set mgmt_ports \{[^}]*elements = \{ ([^}]*) \}`) + reMgmtPortDecl = regexp.MustCompile(`set mgmt_ports \{`) + reInClusterPortDecl = regexp.MustCompile(`set in_cluster_ports \{`) + reBlockedSet = regexp.MustCompile(`set blocked_addrs \{[^}]*elements = \{ ([^}]*) \}`) + reBlockedSet6 = regexp.MustCompile(`set blocked_addrs6 \{[^}]*elements = \{ ([^}]*) \}`) + reInClusterSet = regexp.MustCompile(`set in_cluster_addrs \{[^}]*elements = \{ ([^}]*) \}`) + reInClusterSet6 = regexp.MustCompile(`set in_cluster_addrs6 \{[^}]*elements = \{ ([^}]*) \}`) + reInClusterPortSet = regexp.MustCompile(`set in_cluster_ports \{[^}]*elements = \{ ([^}]*) \}`) + + // The legacy patterns match the pre-allow-rules rendering, where the + // management port and the pod CIDRs were rule literals. The address patterns + // exclude a leading `@` so they cannot match the current set-based rule. + reLegacySSHPort = regexp.MustCompile(`ip saddr @mgmt_addrs tcp dport (\d+) accept`) + reLegacyPodCIDR = regexp.MustCompile(`ip saddr ([^@\s]\S*) tcp dport @in_cluster_ports accept`) + reLegacyPodCIDR6 = regexp.MustCompile(`ip6 saddr ([^@\s]\S*) tcp dport @in_cluster_ports accept`) ) -func parseElements(content string, re *regexp.Regexp) (string, bool) { - m := re.FindStringSubmatch(content) - if m == nil { - return "", false +// parseSetElements returns the merged element lists of the named sets, skipping +// any that are absent or declared without an `elements` clause. +func parseSetElements(content string, res ...*regexp.Regexp) []string { + var out []string + for _, re := range res { + m := re.FindStringSubmatch(content) + if m == nil { + continue + } + out = append(out, splitElements(m[1])...) } - return strings.TrimSpace(m[1]), true + return out } func splitElements(s string) []string { diff --git a/internal/network/firewall/paths.go b/internal/network/firewall/paths.go index 9c856835..5b53fa6d 100644 --- a/internal/network/firewall/paths.go +++ b/internal/network/firewall/paths.go @@ -26,6 +26,17 @@ const ( // boot. HostNftPath = "/etc/solo-provisioner/network-weaver-host-firewall.nft" + // HostConfigPath is the declarative config this table is rendered from, and + // the source of truth for every mutating verb. It sits beside the nft + // artifact and holds exactly the schema `network firewall create --from-file` + // accepts, so `show --output yaml` re-applied through --from-file is a no-op + // by construction. + // + // A single file rather than one per rule: 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. + HostConfigPath = "/etc/solo-provisioner/network-weaver-host-firewall.yaml" + // WeaverNftPath is the inet weaver-workload-policy artifact, owned by `block node install` // (TS_2 #743). This package never writes it; it only checks for its presence // to decide whether the shared oneshot may be disabled (teardown is #791). diff --git a/internal/network/firewall/render.go b/internal/network/firewall/render.go index 67cde3ee..fc6811cc 100644 --- a/internal/network/firewall/render.go +++ b/internal/network/firewall/render.go @@ -5,27 +5,41 @@ package firewall import ( "os" "path/filepath" - "strconv" "strings" "github.com/hashgraph/solo-weaver/internal/templates" - "github.com/hashgraph/solo-weaver/pkg/sanity" "github.com/joomcode/errorx" ) -// renderData is the flattened view of a Table passed to the nft template. -// Strings are pre-joined here because templates.Render parses without a FuncMap, -// so the template itself cannot call join. The `*6` fields carry the IPv6-family -// members so the template can declare parallel ipv6_addr sets and `ip6` rules. +// renderData is the flattened view of a Table passed to the nft template. The +// three reserved blocks get their own fields because each renders into a +// position an allow rule cannot reach; Allow is ranged over uniformly. type renderData struct { - MgmtElements string - MgmtElements6 string - BlockedElements string - BlockedElements6 string - PortElements string - SSHPort int - PodCIDR string - PodCIDR6 string + Mgmt ruleRender + Blocked ruleRender + InCluster ruleRender + Allow []ruleRender +} + +// ruleRender is one Rule flattened for the template. Element lists are +// pre-joined here because templates.Render parses without a FuncMap, so the +// template itself cannot call join. The `*6` fields carry the IPv6-family +// members so the template can declare parallel ipv6_addr sets and `ip6` rules. +type ruleRender struct { + Name string + AddrSet string + AddrSet6 string + PortsSet string + Elements string + Elements6 string + PortElements string + Proto string + // HasV4/HasV6 gate the per-family rule, so a rule whose sources are all one + // family does not emit a dead rule in the other family's chain. + HasV4 bool + HasV6 bool + HasPorts bool + ICMPEcho bool } // Render produces the full `inet weaver-host-firewall` nft document for this table. The same @@ -36,24 +50,13 @@ func (t *Table) Render() (string, error) { return "", err } - ports := make([]string, len(t.InClusterPorts)) - for i, p := range t.InClusterPorts { - ports[i] = strconv.Itoa(p) - } - - mgmtV4, mgmtV6 := splitCIDRsByFamily(t.MgmtCIDRs) - blockedV4, blockedV6 := splitCIDRsByFamily(t.BlockedCIDRs) - podV4, podV6 := routePodCIDRs(t.PodCIDR, t.PodCIDR6) - data := renderData{ - MgmtElements: strings.Join(mgmtV4, ", "), - MgmtElements6: strings.Join(mgmtV6, ", "), - BlockedElements: strings.Join(blockedV4, ", "), - BlockedElements6: strings.Join(blockedV6, ", "), - PortElements: strings.Join(ports, ", "), - SSHPort: t.SSHPort, - PodCIDR: podV4, - PodCIDR6: podV6, + Mgmt: flattenRule(&t.Mgmt), + Blocked: flattenRule(&t.Blocked), + InCluster: flattenRule(&t.InCluster), + } + for i := range t.Allow { + data.Allow = append(data.Allow, flattenRule(&t.Allow[i])) } rendered, err := templates.Render(hostNftTemplate, data) @@ -64,42 +67,25 @@ func (t *Table) Render() (string, error) { return rendered, nil } -// splitCIDRsByFamily partitions a validated mixed CIDR list into its IPv4 and -// IPv6 members, preserving order. Table.Validate has already run, so a -// classification error is not expected here; a value that somehow fails to -// classify is dropped from both lists rather than smuggled into the wrong-family -// nft set (which nft would reject at apply time anyway). -func splitCIDRsByFamily(cidrs []string) (v4, v6 []string) { - for _, c := range cidrs { - isV6, err := sanity.CIDRIsIPv6(c) - if err != nil { - continue - } - if isV6 { - v6 = append(v6, c) - } else { - v4 = append(v4, c) - } - } - return v4, v6 -} - -// routePodCIDRs assigns the two pod-CIDR fields to the v4/v6 render slots by -// their actual family, so a value placed in either Table field renders into the -// correct `ip`/`ip6` in-cluster rule even if a caller slotted it by the wrong -// field. When both fields carry the same family the later one wins that slot. -func routePodCIDRs(pod, pod6 string) (v4, v6 string) { - for _, c := range []string{pod, pod6} { - if c == "" { - continue - } - if isV6, err := sanity.CIDRIsIPv6(c); err == nil && isV6 { - v6 = c - } else { - v4 = c - } +// flattenRule converts a validated Rule into its template view, splitting the +// address list by family and joining each list into an nft `elements = { … }` +// body. +func flattenRule(r *Rule) ruleRender { + v4, v6 := splitCIDRs(r.CIDRs) + return ruleRender{ + Name: r.Name, + AddrSet: addrSetName(r.Name), + AddrSet6: v6SetName(r.Name), + PortsSet: portsSetName(r.Name), + Elements: strings.Join(v4, ", "), + Elements6: strings.Join(v6, ", "), + PortElements: strings.Join(r.Ports, ", "), + Proto: string(r.proto()), + HasV4: len(v4) > 0, + HasV6: len(v6) > 0, + HasPorts: len(r.Ports) > 0, + ICMPEcho: r.ICMPEcho, } - return v4, v6 } // atomicWriteFile writes content to path via a temp file in the same directory diff --git a/internal/network/firewall/rule.go b/internal/network/firewall/rule.go new file mode 100644 index 00000000..6c958171 --- /dev/null +++ b/internal/network/firewall/rule.go @@ -0,0 +1,374 @@ +// SPDX-License-Identifier: Apache-2.0 + +package firewall + +import ( + "sort" + "strconv" + "strings" + + "github.com/hashgraph/solo-weaver/pkg/sanity" + "github.com/joomcode/errorx" +) + +// Proto is the L4 protocol a rule matches. A rule names exactly one: nft has no +// combined tcp/udp dport match, so a service reachable over both families of +// protocol is two rules. +type Proto string + +const ( + // ProtoTCP matches TCP destination ports. + ProtoTCP Proto = "tcp" + // ProtoUDP matches UDP destination ports. + ProtoUDP Proto = "udp" +) + +// Reserved rule names. These three are first-class rather than operator-authored +// because weaver derives or defaults their content and omitting them is +// dangerous: 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 ordinary named allow rule. +const ( + // RuleMgmt is the management allowlist: the only source of host-local + // administrative access under the input chain's default drop. + RuleMgmt = "mgmt" + // RuleBlocked is the operator-curated deny list. It renders on prerouting, + // input and output, so it is not expressible as an allow rule. + RuleBlocked = "blocked" + // RuleInCluster is the pod-CIDR-to-host-service allowance. Its address list + // is auto-detected from the node's .spec.podCIDR when the operator omits it. + RuleInCluster = "in_cluster" +) + +// ReservedNames are the rule names an `allow` entry may not take, in render +// order. +var ReservedNames = []string{RuleMgmt, RuleBlocked, RuleInCluster} + +// Rule is one named record in the host firewall: a source address list, a +// destination port list, and the protocol they apply to. The three reserved +// names render into fixed positions and ignore some fields (see Validate); an +// allow rule renders uniformly as +// ` saddr @ dport @_ports accept`. +// +// Ports are strings, not ints, so an inclusive range ("2379-2380") is +// expressible without a mixed int/string list. CIDRs may mix address families; +// the renderer routes each to the matching per-family set. +type Rule struct { + Name string `yaml:"name" json:"name"` + CIDRs []string `yaml:"cidrs,omitempty" json:"cidrs,omitempty"` + Ports []string `yaml:"ports,omitempty" json:"ports,omitempty"` + // Proto defaults to ProtoTCP when empty. Meaningless on mgmt (which renders + // a fixed TCP accept plus its own ICMP type list) and on blocked (which + // drops every protocol). + Proto Proto `yaml:"proto,omitempty" json:"proto,omitempty"` + // ICMPEcho grants this rule's sources unmetered echo-request, rendered into + // the per-family ICMP chains above the rate meter. Meaningless on mgmt, + // which already carries a broader ICMP type list, and on blocked. + ICMPEcho bool `yaml:"icmp_echo,omitempty" json:"icmp_echo,omitempty"` +} + +// IsReserved reports whether name is one of the three reserved blocks. +func IsReserved(name string) bool { + return sanity.Contains(name, ReservedNames) +} + +// addrSetName returns the nft address-set name holding a rule's IPv4 members. +// The reserved blocks keep the `_addrs` suffix they shipped with — those names +// appear in the ICMP chains, in the docs, and in the on-disk artifact of every +// already-provisioned host — while an allow rule uses its bare name, matching +// the workload plane's `@bn-publisher` convention. +func addrSetName(name string) string { + if IsReserved(name) { + return name + "_addrs" + } + return name +} + +// v6SetName returns the IPv6 companion of addrSetName. Every derived set name +// goes through one of these three functions so the renderer, the parser and the +// collision check can never disagree on a spelling. +func v6SetName(name string) string { return addrSetName(name) + "6" } + +// portsSetName returns the nft set name holding a rule's destination ports. +func portsSetName(name string) string { return name + "_ports" } + +// proto returns the rule's effective protocol, applying the tcp default. +func (r *Rule) proto() Proto { + if r.Proto == "" { + return ProtoTCP + } + return r.Proto +} + +// Validate rejects any field that would be unsafe or nonsensical to render. +// Every untrusted token goes through pkg/sanity, so a malformed value can never +// break the atomic nft transaction or smuggle in nft syntax. flagFor names the +// CLI flag in the error so an operator sees the input they supplied rather than +// an internal field name. +func (r *Rule) Validate() error { + if err := sanity.ValidateIdentifier(r.Name); err != nil { + return errorx.IllegalArgument.Wrap(err, "invalid rule name %q", r.Name) + } + + cidrFlag, portFlag := r.flagNames() + for _, c := range r.CIDRs { + if err := sanity.ValidateCIDR(c); err != nil { + return errorx.IllegalArgument.Wrap(err, "invalid %s %q", cidrFlag, c) + } + } + for _, p := range r.Ports { + if err := validatePortSpec(p); err != nil { + return errorx.IllegalArgument.Wrap(err, "invalid %s %q", portFlag, p) + } + } + + switch r.Proto { + case "", ProtoTCP, ProtoUDP: + default: + return errorx.IllegalArgument.New("invalid proto %q for rule %q: expected %q or %q", r.Proto, r.Name, ProtoTCP, ProtoUDP) + } + + switch r.Name { + case RuleBlocked: + // The block list is a drop on three hooks, matching every protocol and + // port. A port or protocol here would silently narrow it, which is the + // opposite of what an operator adding a CIDR to a block list expects. + if len(r.Ports) > 0 { + return errorx.IllegalArgument.New("%q does not take ports: the block list drops every port and protocol", RuleBlocked) + } + if r.Proto != "" { + return errorx.IllegalArgument.New("%q does not take proto: the block list drops every port and protocol", RuleBlocked) + } + if r.ICMPEcho { + return errorx.IllegalArgument.New("%q does not take icmp_echo: the block list drops ICMP too", RuleBlocked) + } + case RuleMgmt: + // mgmt renders a fixed ICMP type list that is strictly broader than an + // echo-request accept, so icmp_echo could only mislead. Its transport + // accept is TCP by construction (this is administrative access). + if r.Proto != "" && r.Proto != ProtoTCP { + return errorx.IllegalArgument.New("%q does not take proto: management access is TCP", RuleMgmt) + } + if r.ICMPEcho { + return errorx.IllegalArgument.New("%q does not take icmp_echo: management sources already receive the full ICMP type list", RuleMgmt) + } + case RuleInCluster: + // Nothing further: a reserved block may be empty, which is how an + // operator disables it without deleting it. + default: + // An allow rule with no sources or no destinations renders a rule that + // matches nothing. Silently keeping it would leave the operator with a + // config that reads as if access were granted, so require the rule to + // say something. Deleting is the way to remove one. + if len(r.CIDRs) == 0 { + return errorx.IllegalArgument.New( + "allow rule %q has no cidrs; delete the rule rather than emptying it", r.Name) + } + if len(r.Ports) == 0 && !r.ICMPEcho { + return errorx.IllegalArgument.New( + "allow rule %q has no ports; set ports, or set icmp_echo to grant echo alone", r.Name) + } + } + + // Order the port list numerically here rather than in each mutator, so a rule + // authored in a config file renders the same as one built up through the CLI. + // Without this the rendered document would depend on the order the operator + // happened to list ports in, and every re-render would churn the artifact. + sortPortSpecs(r.Ports) + + return nil +} + +// flagNames returns the CLI flag names to quote in a validation error for this +// rule, so the message points at the input the operator actually typed. +func (r *Rule) flagNames() (cidrFlag, portFlag string) { + switch r.Name { + case RuleMgmt: + return "--mgmt-cidrs", "--ssh-port" + case RuleBlocked: + return "--blocked-cidrs", "--ports" + case RuleInCluster: + return "--pod-cidr", "--in-cluster-ports" + default: + return "--cidrs", "--ports" + } +} + +// AddCIDRs adds CIDRs to the rule, ignoring ones already present. The list is +// kept sorted so a render is stable regardless of the order entries arrived in. +func (r *Rule) AddCIDRs(cidrs []string) error { + for _, c := range cidrs { + if err := sanity.ValidateCIDR(c); err != nil { + cidrFlag, _ := r.flagNames() + return errorx.IllegalArgument.Wrap(err, "invalid %s %q", cidrFlag, c) + } + if !sanity.Contains(c, r.CIDRs) { + r.CIDRs = append(r.CIDRs, c) + } + } + sort.Strings(r.CIDRs) + return nil +} + +// RemoveCIDRs drops CIDRs from the rule. Removing an absent entry is a no-op. +func (r *Rule) RemoveCIDRs(cidrs []string) { + r.CIDRs = without(r.CIDRs, cidrs) +} + +// SetCIDRs atomically replaces the rule's full address list. An empty +// (non-nil) slice clears it. +func (r *Rule) SetCIDRs(cidrs []string) error { + for _, c := range cidrs { + if err := sanity.ValidateCIDR(c); err != nil { + cidrFlag, _ := r.flagNames() + return errorx.IllegalArgument.Wrap(err, "invalid %s %q", cidrFlag, c) + } + } + r.CIDRs = sortedDedupe(cidrs) + return nil +} + +// AddPorts adds port specs to the rule, ignoring ones already present. +func (r *Rule) AddPorts(ports []string) error { + for _, p := range ports { + if err := validatePortSpec(p); err != nil { + _, portFlag := r.flagNames() + return errorx.IllegalArgument.Wrap(err, "invalid %s %q", portFlag, p) + } + if !sanity.Contains(p, r.Ports) { + r.Ports = append(r.Ports, p) + } + } + sortPortSpecs(r.Ports) + return nil +} + +// RemovePorts drops port specs from the rule. Removal is by exact spec, so +// removing "2379" from a rule holding "2379-2380" is a no-op rather than a +// partial range split — nft ranges are single set elements and splitting one +// silently would be a surprising way to change a firewall. +func (r *Rule) RemovePorts(ports []string) { + r.Ports = without(r.Ports, ports) +} + +// SetPorts atomically replaces the rule's full port list. An empty (non-nil) +// slice clears it. +func (r *Rule) SetPorts(ports []string) error { + for _, p := range ports { + if err := validatePortSpec(p); err != nil { + _, portFlag := r.flagNames() + return errorx.IllegalArgument.Wrap(err, "invalid %s %q", portFlag, p) + } + } + out := dedupeStrings(ports) + sortPortSpecs(out) + r.Ports = out + return nil +} + +// validatePortSpec accepts a single port ("6443") or an inclusive range +// ("2379-2380"). Each endpoint goes through sanity.ValidatePort, so the range +// form gains no ground on what a bare port is allowed to be. +func validatePortSpec(s string) error { + lo, hi, err := parsePortSpec(s) + if err != nil { + return err + } + if lo > hi { + return errorx.IllegalArgument.New("port range %q is inverted: %d is above %d", s, lo, hi) + } + return nil +} + +// parsePortSpec splits a port spec into its inclusive bounds. A single port +// yields lo == hi, so callers can order and compare both forms uniformly. +func parsePortSpec(s string) (lo, hi int, err error) { + spec := strings.TrimSpace(s) + loStr, hiStr, isRange := strings.Cut(spec, "-") + if !isRange { + if err := sanity.ValidatePort(spec); err != nil { + return 0, 0, err + } + n, _ := strconv.Atoi(spec) // safe: ValidatePort already parsed and range-checked it + return n, n, nil + } + if err := sanity.ValidatePort(loStr); err != nil { + return 0, 0, errorx.IllegalArgument.Wrap(err, "invalid range start in %q", spec) + } + if err := sanity.ValidatePort(hiStr); err != nil { + return 0, 0, errorx.IllegalArgument.Wrap(err, "invalid range end in %q", spec) + } + lo, _ = strconv.Atoi(loStr) + hi, _ = strconv.Atoi(hiStr) + return lo, hi, nil +} + +// sortPortSpecs orders port specs numerically by their lower bound, so the +// rendered elements list reads in ascending port order rather than lexically +// (where "10250" would precede "6443"). Unparseable specs sort last; Validate +// rejects them before a render, so this only keeps the ordering total. +func sortPortSpecs(ports []string) { + sort.SliceStable(ports, func(i, j int) bool { + loI, hiI, errI := parsePortSpec(ports[i]) + loJ, hiJ, errJ := parsePortSpec(ports[j]) + if (errI == nil) != (errJ == nil) { + return errI == nil + } + if errI != nil { + return ports[i] < ports[j] + } + if loI != loJ { + return loI < loJ + } + return hiI < hiJ + }) +} + +// splitCIDRs partitions a validated mixed CIDR list into its IPv4 and IPv6 +// members, preserving order. Validate has already run, so a value that somehow +// fails to classify is dropped from both lists rather than smuggled into the +// wrong-family nft set (which nft would reject at apply time anyway). +func splitCIDRs(cidrs []string) (v4, v6 []string) { + for _, c := range cidrs { + isV6, err := sanity.CIDRIsIPv6(c) + if err != nil { + continue + } + if isV6 { + v6 = append(v6, c) + } else { + v4 = append(v4, c) + } + } + return v4, v6 +} + +// without returns in with every element of drop removed, preserving order. +func without(in, drop []string) []string { + out := make([]string, 0, len(in)) + for _, v := range in { + if !sanity.Contains(v, drop) { + out = append(out, v) + } + } + return out +} + +func sortedDedupe(in []string) []string { + out := dedupeStrings(in) + sort.Strings(out) + return out +} + +func dedupeStrings(in []string) []string { + seen := make(map[string]struct{}, len(in)) + out := make([]string, 0, len(in)) + for _, s := range in { + if _, ok := seen[s]; ok { + continue + } + seen[s] = struct{}{} + out = append(out, s) + } + return out +} diff --git a/internal/network/firewall/table.go b/internal/network/firewall/table.go index b2ddcb26..62326aca 100644 --- a/internal/network/firewall/table.go +++ b/internal/network/firewall/table.go @@ -6,7 +6,6 @@ import ( "sort" "strconv" - "github.com/hashgraph/solo-weaver/pkg/sanity" "github.com/joomcode/errorx" ) @@ -23,18 +22,29 @@ const ( var DefaultInClusterPorts = []int{6443, 4244, 7472, 10250} // Table is the in-memory model of the `inet weaver-host-firewall` nftables table. It is the -// single source of truth that both the kernel apply (via `nft -f`) and the -// on-disk artifact are rendered from, so the two can never diverge. +// single source of truth that the kernel apply (via `nft -f`), the on-disk nft +// artifact, and the persisted YAML config are all rendered from, so no two of +// them can diverge. +// +// The three reserved blocks are separate fields rather than entries in Allow +// because each renders into a position an allow rule cannot reach: Mgmt also +// feeds the ICMP chains, Blocked renders as a drop on three hooks, and +// InCluster's address list is auto-detected rather than authored. Every other +// rule is uniform, so the CLI addresses all four kinds by name through +// Table.Rule. type Table struct { - // MgmtCIDRs is the management/SSH allowlist (set @mgmt_addrs). - MgmtCIDRs []string - // BlockedCIDRs is the operator-curated deny list (set @blocked_addrs). It is - // purely operator-managed for its whole lifecycle — nothing in this package - // or the daemon ever writes to it automatically. This is deliberately - // distinct from the BN workload plane's `bn-restricted` set (`inet weaver-workload-policy`), - // which the traffic-shaper daemon reconciles from the block node's statusz - // "restricted" category; an operator block list needs a home the daemon - // never overwrites. + // Mgmt is the management allowlist (sets @mgmt_addrs / @mgmt_addrs6) and the + // ports reachable from it (@mgmt_ports). Under the input chain's default + // drop this is the only path to host-local administrative access, so an + // empty address list locks the operator out of new connections. + Mgmt Rule + // Blocked is the operator-curated deny list (sets @blocked_addrs / + // @blocked_addrs6). It is purely operator-managed for its whole lifecycle — + // nothing in this package or the daemon ever writes to it. This is + // deliberately distinct from the BN workload plane's `bn-restricted` set + // (`inet weaver-workload-policy`), which the traffic-shaper daemon + // reconciles from the block node's statusz "restricted" category; an + // operator block list needs a home the daemon never overwrites. // // A blocked CIDR means "blocked on this node", not "blocked from the host's // own services": it is dropped on prerouting (which covers pod-bound @@ -42,217 +52,186 @@ type Table struct { // destination on output — because blocking a peer inbound does not stop this // host from dialing it, and the replies to a host-initiated connection are // admitted by the input chain's established accept. - BlockedCIDRs []string - // InClusterPorts are host-service ports reachable from PodCIDR (set - // @in_cluster_ports). Per design there is deliberately no --service-ports: - // BN ports live only in `network policy --ports`. - InClusterPorts []int - // SSHPort is the TCP port accepted from @mgmt_addrs for management access. - SSHPort int - // PodCIDR is the source range allowed to reach @in_cluster_ports. Empty - // means no in-cluster port rule is rendered. It may hold either an IPv4 or - // IPv6 CIDR; Render routes it to the matching (`ip`/`ip6`) rule by family. - PodCIDR string - // PodCIDR6 is the optional IPv6 companion to PodCIDR so a dual-stack node can - // admit in-cluster traffic over both families. It is a separate field (rather - // than folding PodCIDR into a slice) to keep the existing single-value callers - // — the block-node install/reconfigure wiring — untouched. Render family-routes - // whichever of PodCIDR/PodCIDR6 is set, so mis-slotting a value by family still - // renders correctly. - PodCIDR6 string + Blocked Rule + // InCluster admits host-service ports (@in_cluster_ports) from the pod CIDR + // (@in_cluster_addrs / @in_cluster_addrs6). Per design there is deliberately + // no rule here for block-node service ports: that traffic is forwarded + // rather than delivered locally, so an input rule for it would never match. + // It lives in `network policy --ports` instead. + // + // An empty address list renders no in-cluster rule at all, which is how an + // operator disables the block without deleting it. + InCluster Rule + // Allow holds the operator-authored rules, each a source list x port list x + // protocol accept. Order within the input chains is by name, so a render is + // stable across CLI invocations; evaluation order does not matter because + // every entry is an accept and none overlap a drop. + Allow []Rule } // NewTable returns a Table populated with the design defaults. Callers override -// fields from CLI flags before rendering. +// fields from CLI flags or a config file before rendering. func NewTable() *Table { - ports := append([]int(nil), DefaultInClusterPorts...) - sort.Ints(ports) return &Table{ - MgmtCIDRs: nil, - InClusterPorts: ports, - SSHPort: DefaultSSHPort, + Mgmt: Rule{Name: RuleMgmt, Ports: []string{strconv.Itoa(DefaultSSHPort)}}, + Blocked: Rule{Name: RuleBlocked}, + InCluster: Rule{Name: RuleInCluster, Ports: PortStrings(DefaultInClusterPorts)}, } } -// Validate rejects any field that would be unsafe to render into the nft -// ruleset. It is the last gate before the renderer; every untrusted value -// (CIDRs, ports) is checked through pkg/sanity so a malformed token can never -// break the atomic transaction or smuggle in nft syntax. -func (t *Table) Validate() error { - for _, c := range t.MgmtCIDRs { - if err := sanity.ValidateCIDR(c); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --mgmt-cidr %q", c) - } - } - - for _, c := range t.BlockedCIDRs { - if err := sanity.ValidateCIDR(c); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --blocked-cidr %q", c) - } - } - - if t.PodCIDR != "" { - if err := sanity.ValidateCIDR(t.PodCIDR); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --pod-cidr %q", t.PodCIDR) - } - } - - if t.PodCIDR6 != "" { - if err := sanity.ValidateCIDR(t.PodCIDR6); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --pod-cidr %q", t.PodCIDR6) - } +// PortStrings converts an int port list to the string port specs a Rule holds. +// It is the boundary conversion for callers whose own schema is still int-typed +// — the --in-cluster-ports / --ssh-port flags and models.HostConfig — so the +// int-vs-range mismatch is resolved in exactly one place. +func PortStrings(ports []int) []string { + out := make([]string, len(ports)) + for i, p := range ports { + out[i] = strconv.Itoa(p) } + sortPortSpecs(out) + return out +} - for _, p := range t.InClusterPorts { - if err := sanity.ValidatePort(strconv.Itoa(p)); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --in-cluster-port %d", p) +// Rule returns a pointer to the named rule so a caller can mutate it in place. +// It resolves the three reserved names and any allow rule through one lookup, +// which is what lets `network firewall add --name ` treat them uniformly. +func (t *Table) Rule(name string) (*Rule, bool) { + switch name { + case RuleMgmt: + return &t.Mgmt, true + case RuleBlocked: + return &t.Blocked, true + case RuleInCluster: + return &t.InCluster, true + } + for i := range t.Allow { + if t.Allow[i].Name == name { + return &t.Allow[i], true } } - - if err := sanity.ValidatePort(strconv.Itoa(t.SSHPort)); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --ssh-port %d", t.SSHPort) - } - - return nil + return nil, false } -// AddMgmtCIDR adds a single CIDR to the management allowlist (idempotent). -func (t *Table) AddMgmtCIDR(cidr string) error { - if err := sanity.ValidateCIDR(cidr); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --mgmt-cidr %q", cidr) - } - if sanity.Contains(cidr, t.MgmtCIDRs) { - return nil +// Names returns every rule name in the table, reserved blocks first and allow +// rules sorted, for error messages that list the valid --name values. +func (t *Table) Names() []string { + out := append([]string(nil), ReservedNames...) + for _, r := range t.Allow { + out = append(out, r.Name) } - t.MgmtCIDRs = append(t.MgmtCIDRs, cidr) - sort.Strings(t.MgmtCIDRs) - return nil + return out } -// RemoveMgmtCIDR removes a single CIDR from the management allowlist -// (idempotent; removing an absent CIDR is a no-op). -func (t *Table) RemoveMgmtCIDR(cidr string) { - out := t.MgmtCIDRs[:0] - for _, c := range t.MgmtCIDRs { - if c != cidr { - out = append(out, c) - } +// rules returns every rule in the table in render order, for the checks and +// derivations that treat all four kinds alike. +func (t *Table) rules() []*Rule { + out := []*Rule{&t.Mgmt, &t.Blocked, &t.InCluster} + for i := range t.Allow { + out = append(out, &t.Allow[i]) } - t.MgmtCIDRs = out + return out } -// SetMgmtCIDRs atomically replaces the full management allowlist. -func (t *Table) SetMgmtCIDRs(cidrs []string) error { - for _, c := range cidrs { - if err := sanity.ValidateCIDR(c); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --mgmt-cidr %q", c) - } +// UpsertAllow adds or replaces an allow rule, rejecting the reserved names. The +// list is kept sorted by name so the rendered document is independent of the +// order rules were authored in. +func (t *Table) UpsertAllow(r Rule) error { + if IsReserved(r.Name) { + return errorx.IllegalArgument.New( + "%q is a reserved name and cannot be used for an allow rule; configure it under its own %q block instead", r.Name, r.Name) } - dedup := dedupeStrings(cidrs) - sort.Strings(dedup) - t.MgmtCIDRs = dedup - return nil -} - -// AddBlockedCIDR adds a single CIDR to the operator block list (idempotent). -func (t *Table) AddBlockedCIDR(cidr string) error { - if err := sanity.ValidateCIDR(cidr); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --blocked-cidr %q", cidr) + if err := r.Validate(); err != nil { + return err } - if sanity.Contains(cidr, t.BlockedCIDRs) { - return nil + for i := range t.Allow { + if t.Allow[i].Name == r.Name { + t.Allow[i] = r + return nil + } } - t.BlockedCIDRs = append(t.BlockedCIDRs, cidr) - sort.Strings(t.BlockedCIDRs) + t.Allow = append(t.Allow, r) + t.sortAllow() return nil } -// RemoveBlockedCIDR removes a single CIDR from the operator block list -// (idempotent; removing an absent CIDR is a no-op). -func (t *Table) RemoveBlockedCIDR(cidr string) { - out := t.BlockedCIDRs[:0] - for _, c := range t.BlockedCIDRs { - if c != cidr { - out = append(out, c) +// DeleteRule removes an allow rule. The reserved blocks cannot be deleted — +// they are structural, and deleting mgmt in particular would render a +// default-drop input chain with no way in. Emptying a block's address list is +// the supported way to disable it. +func (t *Table) DeleteRule(name string) error { + if IsReserved(name) { + return errorx.IllegalArgument.New( + "%q cannot be deleted: it is a reserved block. Clear its addresses instead (`network firewall set --name %s --cidrs \"\"`)", name, name) + } + for i := range t.Allow { + if t.Allow[i].Name == name { + t.Allow = append(t.Allow[:i], t.Allow[i+1:]...) + return nil } } - t.BlockedCIDRs = out + return errorx.IllegalArgument.New("no rule named %q", name) } -// SetBlockedCIDRs atomically replaces the full operator block list. -func (t *Table) SetBlockedCIDRs(cidrs []string) error { - for _, c := range cidrs { - if err := sanity.ValidateCIDR(c); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --blocked-cidr %q", c) - } - } - dedup := dedupeStrings(cidrs) - sort.Strings(dedup) - t.BlockedCIDRs = dedup - return nil +func (t *Table) sortAllow() { + sort.Slice(t.Allow, func(i, j int) bool { return t.Allow[i].Name < t.Allow[j].Name }) } -// AddPort adds a single in-cluster host-service port (idempotent). -func (t *Table) AddPort(port int) error { - if err := sanity.ValidatePort(strconv.Itoa(port)); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --in-cluster-port %d", port) - } - for _, p := range t.InClusterPorts { - if p == port { - return nil +// Validate rejects any table that would be unsafe to render. It is the last +// gate before the renderer: it validates every rule, holds the reserved names +// against the allow list, and checks that no two rules derive the same nft set +// name. +func (t *Table) Validate() error { + // Reserved-block names are structural. A caller that built the table by hand + // (rather than through NewTable) could leave them empty, which would render + // a set named "_addrs"; fix them up rather than failing, since the field a + // value sits in is what identifies the block. + t.Mgmt.Name = RuleMgmt + t.Blocked.Name = RuleBlocked + t.InCluster.Name = RuleInCluster + + for _, r := range t.rules() { + if err := r.Validate(); err != nil { + return err } } - t.InClusterPorts = append(t.InClusterPorts, port) - sort.Ints(t.InClusterPorts) - return nil -} -// RemovePort removes a single in-cluster host-service port (idempotent). -func (t *Table) RemovePort(port int) { - out := t.InClusterPorts[:0] - for _, p := range t.InClusterPorts { - if p != port { - out = append(out, p) + seenName := make(map[string]struct{}, len(t.Allow)) + for _, r := range t.Allow { + if IsReserved(r.Name) { + return errorx.IllegalArgument.New( + "%q is a reserved name and cannot be used for an allow rule; configure it under its own %q block instead", r.Name, r.Name) } - } - t.InClusterPorts = out -} - -// SetPorts atomically replaces the full in-cluster host-service port list. -func (t *Table) SetPorts(ports []int) error { - for _, p := range ports { - if err := sanity.ValidatePort(strconv.Itoa(p)); err != nil { - return errorx.IllegalArgument.Wrap(err, "invalid --in-cluster-port %d", p) + if _, dup := seenName[r.Name]; dup { + return errorx.IllegalArgument.New("duplicate allow rule %q", r.Name) } + seenName[r.Name] = struct{}{} } - dedup := dedupeInts(ports) - sort.Ints(dedup) - t.InClusterPorts = dedup - return nil + + return t.checkSetNameCollisions() } -func dedupeStrings(in []string) []string { - seen := make(map[string]struct{}, len(in)) - out := make([]string, 0, len(in)) - for _, s := range in { - if _, ok := seen[s]; ok { - continue +// checkSetNameCollisions rejects a table where two rules derive the same nft set +// name. The derivations append suffixes, so distinct rule names can still +// collide — an allow rule named "mgmt_addrs" would claim the mgmt block's +// address set, and one named "k8s6" would claim the v6 set of a rule named +// "k8s". nft would accept the duplicate declaration silently and merge the two +// rules' membership, so this has to be caught here. +func (t *Table) checkSetNameCollisions() error { + owner := make(map[string]string) + claim := func(setName, ruleName string) error { + if prev, ok := owner[setName]; ok { + return errorx.IllegalArgument.New( + "rules %q and %q both derive the nft set name %q; rename one of them", prev, ruleName, setName) } - seen[s] = struct{}{} - out = append(out, s) + owner[setName] = ruleName + return nil } - return out -} - -func dedupeInts(in []int) []int { - seen := make(map[int]struct{}, len(in)) - out := make([]int, 0, len(in)) - for _, n := range in { - if _, ok := seen[n]; ok { - continue + 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 + } } - seen[n] = struct{}{} - out = append(out, n) } - return out + return nil } 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 new file mode 100644 index 00000000..48b3d5d1 --- /dev/null +++ b/internal/network/firewall/testdata/network-weaver-host-firewall-allow.golden.nft @@ -0,0 +1,185 @@ +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. + 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 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_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_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_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 + # -300 is the `raw` band, ahead of conntrack at -200, so a blocked source + # never gets a conntrack lookup or a provisional entry allocated. + # + # This hook covers the forward path as well as the host path, so a blocked + # CIDR is blocked for pod-bound traffic too — the block list means "this peer + # is blocked on this node", not "blocked from the host's own services". + chain prerouting_blocklist { + type filter hook prerouting priority -300; policy accept; + ip saddr @blocked_addrs drop + ip6 saddr @blocked_addrs6 drop + } + + # The hooked chain carries only what applies to every packet regardless of + # address family, then dispatches into the regular chains below — so an IPv4 + # packet never evaluates an IPv6 rule and vice versa. A jump that returns + # without a verdict falls through to the rest of this chain, ending at the + # `policy drop`. + chain input { + type filter hook input priority 0; policy drop; + + # Operator-curated block list (`network firewall --blocked-cidrs`). Runs + # before every other rule, including the conntrack fast-path below, so an + # entry added here drops already-open connections too. Purely + # operator-managed: nothing else ever writes to these sets. One rule per + # family — two address compares are cheaper than a dispatch. + # + # Redundant for anything arriving on a wire, since prerouting_blocklist + # already dropped it. Kept because this ordering — block list ahead of the + # conntrack fast-path — is the tested definition of what the block list + # does on the host path, and it should not become contingent on a chain + # registered on a different hook. + ip saddr @blocked_addrs drop + ip6 saddr @blocked_addrs6 drop + + # Admit loopback. `iif "lo"` covers both 127.0.0.0/8 and ::1 since this is + # an `inet` (dual-family) table. It precedes the ICMP dispatch so pinging + # localhost is not rate-limited, which also means loopback is admitted + # without a conntrack state check — the host talking to itself. + iif "lo" accept + + # ICMP is dispatched BEFORE the conntrack fast-path below. netfilter + # conntrack DOES track ICMP echo as a flow (keyed on the echo id), so a + # sustained ping shares one entry and every packet after the first would + # otherwise match `established` and bypass the echo-request rate limit. + # Handling ICMP first keeps the limit effective. `meta l4proto` selects + # the family on its own (protocol 1 vs 58) and resolves past IPv6 + # extension headers, so it doubles as the family split for ICMP. + meta l4proto vmap { icmp : jump input_icmp_ipv4, icmpv6 : jump input_icmp_ipv6 } + + # Conntrack fast-path for everything else (TCP/UDP). A single state + # lookup covers both the invalid drop and the established/related accept. + # ICMP that fell through the chains above lands here too, so a solicited + # echo-reply is still admitted as `established`. + ct state vmap { established : accept, related : accept, invalid : drop } + + meta nfproto vmap { ipv4 : jump input_ipv4, ipv6 : jump input_ipv6 } + } + + # ICMPv4. Runs ahead of the base chain's conntrack vmap, so it re-drops + # invalid itself: that ordering is what stops a forged ICMP error from being + # admitted by the blanket path-health accepts below. + chain input_icmp_ipv4 { + ct state invalid drop + + # Full ICMP from management sources (ping, traceroute, diagnostics) — no rate limit. + ip saddr @mgmt_addrs icmp type { echo-request, echo-reply, destination-unreachable, time-exceeded, parameter-problem } accept + # Unmetered echo for the admin rule's sources. Must stay above the + # rate meter below: the meter drops over-budget echo outright, so a named + # accept placed after it would never be reached under a flood — which is + # exactly when an operator needs ping to still work. + ip saddr @admin icmp type echo-request accept + + # From everyone else: always allow the path-health subset (Path MTU + # Discovery + traceroute), and rate-limit echo-request to prevent floods. + # Discarding the excess first lets one accept cover every admitted type, + # and keeps over-budget echo from falling back to the base chain's + # established accept (a sustained ping is established after the first + # reply). The meter must stay scoped to echo-request: metering the whole + # set would share one bucket, so a ping flood would starve the error + # signals below and blackhole PMTUD exactly when the host is loaded. + icmp type echo-request limit rate over 10/second drop + icmp type { destination-unreachable, time-exceeded, echo-request } accept + } + + # ICMPv6. Same invalid-first ordering, and for the same reason, as the IPv4 + # chain above. + chain input_icmp_ipv6 { + ct state invalid drop + + # --- IPv6 Neighbor Discovery + MLD (REQUIRED under policy drop) --- + # IPv6 is non-functional without Neighbor Discovery: address resolution + # (neighbor solicit/advert) and router discovery (router solicit/advert) + # ride on ICMPv6 and would otherwise be dropped, breaking all IPv6. NDP + # packets use a hop limit of 255 (RFC 4861 §11.2); enforce it so an + # off-link (routed) forgery cannot satisfy the accept. ICMPv6 Redirect is + # deliberately excluded here (accepting it enables on-link MITM). These + # are structural, not policy: no rule can remove them. + icmpv6 type { nd-neighbor-solicit, nd-neighbor-advert, nd-router-solicit, nd-router-advert } ip6 hoplimit 255 accept + # Multicast Listener Discovery — the switch needs these reports to forward + # the solicited-node multicast that NDP relies on. + icmpv6 type { mld-listener-query, mld-listener-report, mld-listener-done } accept + + # Full ICMPv6 from management sources — no rate limit. + ip6 saddr @mgmt_addrs6 icmpv6 type { echo-request, echo-reply, destination-unreachable, time-exceeded, parameter-problem, packet-too-big } accept + # Unmetered echo for the admin rule's sources — above the meter, same + # reasoning as the IPv4 chain. + ip6 saddr @admin6 icmpv6 type echo-request accept + + # IPv6 path health for everyone. packet-too-big is the IPv6 PMTUD signal — + # IPv6 routers never fragment, so dropping it silently blackholes any flow + # whose path MTU is smaller than the sender's. Discarding over-budget echo + # first lets one accept cover every admitted type; the meter must stay + # scoped to echo-request, since metering the whole set would share one + # bucket and let a ping flood starve packet-too-big. + icmpv6 type echo-request limit rate over 10/second drop + icmpv6 type { packet-too-big, destination-unreachable, time-exceeded, parameter-problem, echo-request } accept + } + + # Transport accepts, one chain per family. Every rule here is an accept + # against a named source set, so evaluation order within the chain carries no + # meaning — a packet either matches one of them or falls through to the base + # chain's `policy drop`. Rules are emitted in name order for a stable render. + chain input_ipv4 { + # SSH / management access from the allowlist only. + ip saddr @mgmt_addrs tcp dport @mgmt_ports accept + + # In-cluster host-service ports, reachable from the pod CIDR only. + ip saddr @in_cluster_addrs tcp dport @in_cluster_ports accept + ip saddr @admin tcp dport @admin_ports accept + ip saddr @cilium-vxlan udp dport @cilium-vxlan_ports accept + ip saddr @k8s-node tcp dport @k8s-node_ports accept + } + + chain input_ipv6 { + # SSH / management access from the allowlist only. + ip6 saddr @mgmt_addrs6 tcp dport @mgmt_ports accept + + # In-cluster host-service ports, reachable from the pod CIDR only. + ip6 saddr @in_cluster_addrs6 tcp dport @in_cluster_ports accept + ip6 saddr @admin6 tcp dport @admin_ports accept + } + + # Block-list symmetry on locally-generated traffic. Dropping a peer inbound + # does not stop this host from dialing it, and once the host initiates, the + # replies are admitted by the input chain's `ct state established` accept — + # so an inbound-only block list does not actually block the connection. + # + # `policy accept`: this is not an egress allowlist. Enumerating legitimate + # outbound traffic on a Kubernetes node (kubelet to the API server, etcd, + # DNS, NTP, image pulls from arbitrary registries, Cilium, Teleport) is both + # large and brittle, and getting it wrong strands the node. + chain output { + type filter hook output priority 0; policy accept; + ip daddr @blocked_addrs drop + ip6 daddr @blocked_addrs6 drop + } +} 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 08ae0a42..428a6009 100644 --- a/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft +++ b/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft @@ -4,9 +4,17 @@ 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. + 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_ports { type inet_service; elements = { 4244, 6443, 7472, 10250 }; } + 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 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 # -300 is the `raw` band, ahead of conntrack at -200, so a blocked source @@ -99,7 +107,8 @@ table inet weaver-host-firewall { # ride on ICMPv6 and would otherwise be dropped, breaking all IPv6. NDP # packets use a hop limit of 255 (RFC 4861 §11.2); enforce it so an # off-link (routed) forgery cannot satisfy the accept. ICMPv6 Redirect is - # deliberately excluded here (accepting it enables on-link MITM). + # deliberately excluded here (accepting it enables on-link MITM). These + # are structural, not policy: no rule can remove them. icmpv6 type { nd-neighbor-solicit, nd-neighbor-advert, nd-router-solicit, nd-router-advert } ip6 hoplimit 255 accept # Multicast Listener Discovery — the switch needs these reports to forward # the solicited-node multicast that NDP relies on. @@ -118,17 +127,21 @@ table inet weaver-host-firewall { icmpv6 type { packet-too-big, destination-unreachable, time-exceeded, parameter-problem, echo-request } accept } + # Transport accepts, one chain per family. Every rule here is an accept + # against a named source set, so evaluation order within the chain carries no + # meaning — a packet either matches one of them or falls through to the base + # chain's `policy drop`. Rules are emitted in name order for a stable render. chain input_ipv4 { # SSH / management access from the allowlist only. - ip saddr @mgmt_addrs tcp dport 22 accept + ip saddr @mgmt_addrs tcp dport @mgmt_ports accept # In-cluster host-service ports, reachable from the pod CIDR only. - ip saddr 10.4.0.0/24 tcp dport @in_cluster_ports accept + ip saddr @in_cluster_addrs tcp dport @in_cluster_ports accept } chain input_ipv6 { # SSH / management access from the allowlist only. - ip6 saddr @mgmt_addrs6 tcp dport 22 accept + ip6 saddr @mgmt_addrs6 tcp dport @mgmt_ports accept } # Block-list symmetry on locally-generated traffic. Dropping a peer inbound 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 33a2341e..ff28f1bb 100644 --- a/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl +++ b/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl @@ -2,11 +2,26 @@ 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 .MgmtElements}} elements = { {{.MgmtElements}} };{{end}} } - set mgmt_addrs6 { type ipv6_addr; flags interval;{{if .MgmtElements6}} elements = { {{.MgmtElements6}} };{{end}} } - set blocked_addrs { type ipv4_addr; flags interval;{{if .BlockedElements}} elements = { {{.BlockedElements}} };{{end}} } - set blocked_addrs6 { type ipv6_addr; flags interval;{{if .BlockedElements6}} elements = { {{.BlockedElements6}} };{{end}} } - set in_cluster_ports { type inet_service;{{if .PortElements}} elements = { {{.PortElements}} };{{end}} } + 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. + 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 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}} } +{{- if .HasPorts}} + set {{.PortsSet}} { type inet_service; flags interval; auto-merge; elements = { {{.PortElements}} }; } +{{- end}} +{{- end}} # Operator block list, dropped as early as the packet can be seen. Priority # -300 is the `raw` band, ahead of conntrack at -200, so a blocked source @@ -75,6 +90,15 @@ table inet weaver-host-firewall { # Full ICMP from management sources (ping, traceroute, diagnostics) — no rate limit. ip saddr @mgmt_addrs icmp type { echo-request, echo-reply, destination-unreachable, time-exceeded, parameter-problem } accept +{{- range .Allow}} +{{- if and .ICMPEcho .HasV4}} + # Unmetered echo for the {{.Name}} rule's sources. Must stay above the + # rate meter below: the meter drops over-budget echo outright, so a named + # accept placed after it would never be reached under a flood — which is + # exactly when an operator needs ping to still work. + ip saddr @{{.AddrSet}} icmp type echo-request accept +{{- end}} +{{- end}} # From everyone else: always allow the path-health subset (Path MTU # Discovery + traceroute), and rate-limit echo-request to prevent floods. @@ -99,7 +123,8 @@ table inet weaver-host-firewall { # ride on ICMPv6 and would otherwise be dropped, breaking all IPv6. NDP # packets use a hop limit of 255 (RFC 4861 §11.2); enforce it so an # off-link (routed) forgery cannot satisfy the accept. ICMPv6 Redirect is - # deliberately excluded here (accepting it enables on-link MITM). + # deliberately excluded here (accepting it enables on-link MITM). These + # are structural, not policy: no rule can remove them. icmpv6 type { nd-neighbor-solicit, nd-neighbor-advert, nd-router-solicit, nd-router-advert } ip6 hoplimit 255 accept # Multicast Listener Discovery — the switch needs these reports to forward # the solicited-node multicast that NDP relies on. @@ -107,6 +132,13 @@ table inet weaver-host-firewall { # Full ICMPv6 from management sources — no rate limit. ip6 saddr @mgmt_addrs6 icmpv6 type { echo-request, echo-reply, destination-unreachable, time-exceeded, parameter-problem, packet-too-big } accept +{{- range .Allow}} +{{- if and .ICMPEcho .HasV6}} + # Unmetered echo for the {{.Name}} rule's sources — above the meter, same + # reasoning as the IPv4 chain. + ip6 saddr @{{.AddrSet6}} icmpv6 type echo-request accept +{{- end}} +{{- end}} # IPv6 path health for everyone. packet-too-big is the IPv6 PMTUD signal — # IPv6 routers never fragment, so dropping it silently blackholes any flow @@ -118,23 +150,37 @@ table inet weaver-host-firewall { icmpv6 type { packet-too-big, destination-unreachable, time-exceeded, parameter-problem, echo-request } accept } + # Transport accepts, one chain per family. Every rule here is an accept + # against a named source set, so evaluation order within the chain carries no + # meaning — a packet either matches one of them or falls through to the base + # chain's `policy drop`. Rules are emitted in name order for a stable render. chain input_ipv4 { # SSH / management access from the allowlist only. - ip saddr @mgmt_addrs tcp dport {{.SSHPort}} accept -{{- if .PodCIDR}} + ip saddr @mgmt_addrs tcp dport @mgmt_ports accept +{{- if .InCluster.HasV4}} # In-cluster host-service ports, reachable from the pod CIDR only. - ip saddr {{.PodCIDR}} tcp dport @in_cluster_ports accept + ip saddr @in_cluster_addrs tcp dport @in_cluster_ports accept +{{- end}} +{{- range .Allow}} +{{- if and .HasV4 .HasPorts}} + ip saddr @{{.AddrSet}} {{.Proto}} dport @{{.PortsSet}} accept +{{- end}} {{- end}} } chain input_ipv6 { # SSH / management access from the allowlist only. - ip6 saddr @mgmt_addrs6 tcp dport {{.SSHPort}} accept -{{- if .PodCIDR6}} + ip6 saddr @mgmt_addrs6 tcp dport @mgmt_ports accept +{{- if .InCluster.HasV6}} # In-cluster host-service ports, reachable from the pod CIDR only. - ip6 saddr {{.PodCIDR6}} tcp dport @in_cluster_ports accept + ip6 saddr @in_cluster_addrs6 tcp dport @in_cluster_ports accept +{{- end}} +{{- range .Allow}} +{{- if and .HasV6 .HasPorts}} + ip6 saddr @{{.AddrSet6}} {{.Proto}} dport @{{.PortsSet}} accept +{{- end}} {{- end}} } diff --git a/internal/workflows/steps/step_network_firewall.go b/internal/workflows/steps/step_network_firewall.go index a65844c3..952cf103 100644 --- a/internal/workflows/steps/step_network_firewall.go +++ b/internal/workflows/steps/step_network_firewall.go @@ -79,6 +79,8 @@ func NetworkFirewallCreate(reconcile bool) *automa.StepBuilder { automa.WithDetail("no management CIDRs configured; host firewall skipped to avoid SSH lock-out")) } + mgr := newFirewallManager() + // NewTable() seeds the design defaults (SSH 22, the stack in-cluster // port set). hostCfg is already the fully resolved effective config // (ResolveHostFirewallConfig applies flag > prompt > config file > @@ -88,15 +90,25 @@ func NetworkFirewallCreate(reconcile bool) *automa.StepBuilder { // keeps a zero-value guard, since 0 is never a valid port and would // otherwise indicate a config the resolver never touched. t := firewall.NewTable() - t.MgmtCIDRs = hostCfg.ManagementCIDRs - t.BlockedCIDRs = hostCfg.BlockedCIDRs + t.Mgmt.CIDRs = hostCfg.ManagementCIDRs + t.Blocked.CIDRs = hostCfg.BlockedCIDRs if hostCfg.SSHPort != 0 { - t.SSHPort = hostCfg.SSHPort + t.Mgmt.Ports = firewall.PortStrings([]int{hostCfg.SSHPort}) + } + t.InCluster.Ports = firewall.PortStrings(hostCfg.InClusterPorts) + t.InCluster.CIDRs = nil + if hostCfg.PodCIDR != "" { + t.InCluster.CIDRs = []string{hostCfg.PodCIDR} } - t.InClusterPorts = hostCfg.InClusterPorts - t.PodCIDR = hostCfg.PodCIDR - mgr := newFirewallManager() + // 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 + } // Determine whether the table pre-existed so rollback only deletes a // table this step actually introduced. In create-if-missing mode diff --git a/internal/workflows/steps/step_network_firewall_test.go b/internal/workflows/steps/step_network_firewall_test.go index 3d982ec9..ef5d72ec 100644 --- a/internal/workflows/steps/step_network_firewall_test.go +++ b/internal/workflows/steps/step_network_firewall_test.go @@ -38,6 +38,7 @@ func withStubbedFirewall(t *testing.T, r *fakeFwRunner) string { return firewall.NewManagerWithConfig(firewall.Config{ Runner: r, NftPath: nftPath, + ConfigPath: filepath.Join(dir, "network-weaver-host-firewall.yaml"), LockPath: filepath.Join(dir, "lock"), ApplyViaService: func(context.Context) error { return nil }, }) @@ -193,3 +194,42 @@ func TestNetworkFirewallCreate_ReconcileReRendersExistingTable(t *testing.T) { require.Equal(t, automa.StatusSkipped, rollback.Status) require.False(t, r.deleted, "rollback must not delete a table this step only re-rendered") } + +// TestNetworkFirewallCreate_PreservesNamedAllowRules pins that a reconfigure does +// not silently drop the operator's named allow rules. config.yaml has no field +// for them — they are declared with `network firewall create --from-file` — so a +// force re-render built purely from hostCfg would wipe every k8s, Cilium and +// admin rule on the host while reporting success. +func TestNetworkFirewallCreate_PreservesNamedAllowRules(t *testing.T) { + r := &fakeFwRunner{} + nftPath := withStubbedFirewall(t, r) + + // Seed a table that carries an allow rule, as `create --from-file` would. + seeded := firewall.NewTable() + seeded.Mgmt.CIDRs = []string{"10.0.0.0/8"} + require.NoError(t, seeded.UpsertAllow(firewall.Rule{ + Name: "k8s-node", + CIDRs: []string{"10.0.0.0/24"}, + Ports: []string{"6443", "2379-2380"}, + })) + require.NoError(t, newFirewallManager().Apply(context.Background(), seeded)) + r.exists = true + + setHostConfig(t, models.HostConfig{ + ManagementCIDRs: []string{"192.168.68.0/24"}, + SSHPort: 22, + PodCIDR: models.DefaultClusterPodCIDR, + InClusterPorts: []int{6443}, + }) + + step, err := NetworkFirewallCreate(true).Build() + require.NoError(t, err) + report := step.Execute(context.Background()) + require.NoError(t, report.Error) + + rendered, err := os.ReadFile(nftPath) + require.NoError(t, err) + require.Contains(t, string(rendered), "192.168.68.0/24", "the reconfigured mgmt allowlist must be applied") + require.Contains(t, string(rendered), "@k8s-node", "the named allow rule must survive a reconfigure") + require.Contains(t, string(rendered), "2379-2380") +} From 42a52e614603b4050baddb79126103dc7740a595 Mon Sep 17 00:00:00 2001 From: alex-au Date: Thu, 13 Aug 2026 16:22:00 +1000 Subject: [PATCH 2/2] feat(network/firewall): require the reserved blocks in a declarative config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--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 --- cmd/cli/commands/network/firewall/create.go | 11 ++- .../network/firewall/firewall_test.go | 32 ++++++++ docs/dev/traffic-shaper.md | 6 +- docs/quickstart.md | 35 ++++++--- internal/network/firewall/allow_test.go | 76 +++++++++++++------ internal/network/firewall/config.go | 60 ++++++++++++++- 6 files changed, 179 insertions(+), 41 deletions(-) diff --git a/cmd/cli/commands/network/firewall/create.go b/cmd/cli/commands/network/firewall/create.go index 73cecf93..1ff78486 100644 --- a/cmd/cli/commands/network/firewall/create.go +++ b/cmd/cli/commands/network/firewall/create.go @@ -29,10 +29,13 @@ var createCmd = &cobra.Command{ Long: "Render and apply the full `inet weaver-host-firewall` table, either from flags or from a declarative " + "config file (--from-file). create-if-missing: if the table already exists, no changes are made unless " + "--force is passed, which re-renders from the current flags or file.\n\n" + - "--from-file is the only way to declare named allow rules. It is fully declarative: an allow rule absent " + - "from the file is removed. The reserved blocks behave differently — one absent from the file is derived or " + - "defaulted, never removed, so a partial file cannot silently drop management access. To disable a reserved " + - "block, give it an empty address list (`in_cluster: {cidrs: []}`).", + "--from-file is the only way to declare named allow rules. It is fully declarative: the file states the whole " + + "table, nothing is inherited from the host's current firewall, and an allow rule absent from the file is " + + "removed. The three reserved blocks (mgmt, blocked, in_cluster) are therefore required, as is `cidrs` inside " + + "mgmt and blocked — a block left out would fall back to a weaver default the file never stated, which for " + + "mgmt is an empty allowlist under the default-drop policy. To disable a reserved block, give it an empty " + + "address list (`in_cluster: {cidrs: []}`); omitting `in_cluster.cidrs` instead means \"auto-detect this " + + "node's pod CIDR\".", RunE: func(cmd *cobra.Command, args []string) error { t, err := buildTable(cmd) if err != nil { diff --git a/cmd/cli/commands/network/firewall/firewall_test.go b/cmd/cli/commands/network/firewall/firewall_test.go index 6710a2a4..be381103 100644 --- a/cmd/cli/commands/network/firewall/firewall_test.go +++ b/cmd/cli/commands/network/firewall/firewall_test.go @@ -236,6 +236,32 @@ allow: require.Error(t, run(t, "create", "--from-file", path, "--mgmt-cidrs", "10.0.0.0/8")) } +// TestCreateCmd_FromFileRequiresReservedBlocks is the operator-facing half of the +// required-block rule: a file that omits mgmt must fail before anything is +// rendered, rather than applying a default-drop table with an empty management +// allowlist and reporting success. +func TestCreateCmd_FromFileRequiresReservedBlocks(t *testing.T) { + nftPath, _ := stubManager(t) + dir := t.TempDir() + + partial := filepath.Join(dir, "partial.yaml") + require.NoError(t, os.WriteFile(partial, []byte(`version: 1 +blocked: + cidrs: [] +in_cluster: + cidrs: [] +allow: + - name: k8s-node + cidrs: ["10.0.0.0/24"] + ports: ["6443"] +`), 0o600)) + + err := run(t, "create", "--from-file", partial) + require.Error(t, err) + require.Contains(t, err.Error(), "mgmt") + require.NoFileExists(t, nftPath, "a rejected config must not render a ruleset") +} + func TestShowCmd_YAMLRoundTrips(t *testing.T) { nftPath, _ := stubManager(t) dir := t.TempDir() @@ -244,6 +270,8 @@ func TestShowCmd_YAMLRoundTrips(t *testing.T) { mgmt: cidrs: ["192.168.68.0/24"] ports: ["22"] +blocked: + cidrs: [] in_cluster: cidrs: [] allow: @@ -290,6 +318,10 @@ func TestDeleteCmd_ByName(t *testing.T) { require.NoError(t, os.WriteFile(path, []byte(`version: 1 mgmt: cidrs: ["192.168.68.0/24"] +blocked: + cidrs: [] +in_cluster: + cidrs: [] allow: - name: k8s-node cidrs: ["10.0.0.0/24"] diff --git a/docs/dev/traffic-shaper.md b/docs/dev/traffic-shaper.md index e0ba3a92..50bb274d 100644 --- a/docs/dev/traffic-shaper.md +++ b/docs/dev/traffic-shaper.md @@ -452,7 +452,11 @@ provisioned node. the per-block flags retained as shorthands. Structure (which rules exist, and their protocol) is file-only; membership is CLI-mutable, because adding a rule is a reviewed change while unblocking an operator is sometimes urgent. - `show --output yaml` emits the same schema `--from-file` accepts. + `show --output yaml` emits the same schema `--from-file` accepts. A file is + the whole table and inherits nothing from the host, so all three reserved + blocks must be stated in it (as must `cidrs` inside `mgmt` and `blocked`) — + otherwise a file that forgot `mgmt` would render an empty management + allowlist under the default-drop policy. - **`network policy`** (`create`/`add`/`remove`/`set`/`show`/`delete`) — the workload policy plane. `create` takes `--name` (the nft set name), `--stamp` (the HTB class to classify into, which also fixes direction) or `--deny`, plus diff --git a/docs/quickstart.md b/docs/quickstart.md index b0c4a3c3..b60b74c9 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -695,15 +695,15 @@ ICMP is a fixed, safe ruleset: full ICMP from the management allowlist, and from ```yaml version: 1 -mgmt: - cidrs: ["192.168.68.0/24"] - ports: ["22"] +mgmt: # required + cidrs: ["192.168.68.0/24"] # required + ports: ["22"] # omitted -> 22 -blocked: - cidrs: [] +blocked: # required + cidrs: [] # required; [] means block nobody -in_cluster: # both fields optional - cidrs: ["10.4.0.0/14"] # omitted -> auto-detected +in_cluster: # required + cidrs: ["10.4.0.0/14"] # omitted -> auto-detected; [] -> no rule ports: ["4244", "6443", "7472", "10250"] # omitted -> the defaults above allow: @@ -735,10 +735,27 @@ sudo solo-provisioner network firewall create --from-file rules.yaml --force | `proto` | no | `tcp` (default) or `udp`. nft has no combined match, so a service on both is two rules | | `icmp_echo` | no | Grants unmetered `echo-request`, rendered above the rate meter | -Two semantics differ deliberately between the record kinds: +**The file is the whole table.** Nothing is inherited from the host's current firewall — only `add`/`remove`/`set` merge with what is already there. Two consequences: - **`allow:` is declarative** — a rule absent from the file is **deleted**. -- **Reserved blocks absent from the file are derived or defaulted, never deleted**, so a partial file cannot silently drop management access. To disable one, give it an empty list (`in_cluster: {cidrs: []}`). +- **All three reserved blocks are required**, as is `cidrs` inside `mgmt` and `blocked`. An omitted block would fall back to a weaver default the file never stated, and for `mgmt` that default is an empty allowlist under the default-drop policy — a lockout nobody wrote down. To render no rule for a block, state it with an empty list (`in_cluster: {cidrs: []}`); the block still cannot be removed. + +| Key | Required | Omitted means | +|--------------------|----------|-------------------------------------------------------------------| +| `version` | no | the current schema version (`1`) | +| `mgmt` | **yes** | — (rejected) | +| `blocked` | **yes** | — (rejected) | +| `in_cluster` | **yes** | — (rejected) | +| `mgmt.cidrs` | **yes** | — (rejected: no safe default exists) | +| `mgmt.ports` | no | `22` | +| `blocked.cidrs` | **yes** | — (rejected; write `[]` to block nobody) | +| `in_cluster.cidrs` | no | auto-detect this node's pod CIDR (`[]` renders no in-cluster rule) | +| `in_cluster.ports` | no | `4244,6443,7472,10250` | +| `allow` | no | no named allow rules — **and any that exist are deleted** | + +`in_cluster.cidrs` is the one address list weaver can legitimately derive on its own, which is why it stays optional; its absence costs a rule rather than access to the host. + +The same rule applies to the persisted config at `/etc/solo-provisioner/network-weaver-host-firewall.yaml`: a truncated or hand-edited file is refused rather than loaded with a defaulted management allowlist. Re-run `create --from-file` to repair one. #### Modify a Rule's Addresses / Ports diff --git a/internal/network/firewall/allow_test.go b/internal/network/firewall/allow_test.go index a868dbd1..d4c02f3c 100644 --- a/internal/network/firewall/allow_test.go +++ b/internal/network/firewall/allow_test.go @@ -211,6 +211,18 @@ func TestRemovePortsIsExact(t *testing.T) { require.Equal(t, []string{"6443"}, r.Ports) } +// mgmtBlockedYAML is the required-block preamble a config must carry before it +// can say anything else: every reserved block has to be stated, so a fixture +// exercising one field still has to write the other blocks down. Callers append +// their own `in_cluster:` section; allReservedYAML closes it off for the cases +// that do not care about in-cluster at all. +const ( + mgmtBlockedYAML = "version: 1\n" + + "mgmt:\n cidrs: [\"10.0.0.0/8\"]\n" + + "blocked:\n cidrs: []\n" + allReservedYAML = mgmtBlockedYAML + "in_cluster:\n cidrs: []\n" +) + func TestConfig_RoundTrip(t *testing.T) { first, err := FileConfigFromTable(allowTable()).Marshal() require.NoError(t, err) @@ -232,18 +244,18 @@ func TestConfig_RoundTrip(t *testing.T) { require.Equal(t, wantDoc, gotDoc) } -// TestConfig_OmittedVsEmptyReservedBlock pins the distinction the reserved-block +// TestConfig_OmittedVsEmptyInClusterCIDRs pins the distinction the in-cluster // semantics rest on, which is carried by nil-vs-empty on a decoded slice: an -// omitted block is derived or defaulted, while a block present with an empty -// list renders no rule. If the YAML decoder ever stopped distinguishing the two, +// omitted `cidrs` is auto-detected, while an explicitly empty one renders no +// rule. If the YAML decoder ever stopped distinguishing the two, // `in_cluster: {cidrs: []}` would silently start auto-detecting the pod CIDR // again. -func TestConfig_OmittedVsEmptyReservedBlock(t *testing.T) { - omitted, err := ParseConfig([]byte("version: 1\nmgmt:\n cidrs: [\"10.0.0.0/8\"]\n")) +func TestConfig_OmittedVsEmptyInClusterCIDRs(t *testing.T) { + omitted, err := ParseConfig([]byte(mgmtBlockedYAML + "in_cluster:\n ports: [\"6443\"]\n")) require.NoError(t, err) - require.True(t, omitted.InClusterCIDRsUnset(), "an omitted in_cluster block must be reported as unset") + require.True(t, omitted.InClusterCIDRsUnset(), "an omitted in_cluster cidrs list must be reported as unset") - present, err := ParseConfig([]byte("version: 1\nmgmt:\n cidrs: [\"10.0.0.0/8\"]\nin_cluster:\n cidrs: []\n")) + present, err := ParseConfig([]byte(mgmtBlockedYAML + "in_cluster:\n cidrs: []\n")) require.NoError(t, err) require.False(t, present.InClusterCIDRsUnset(), "an explicitly empty cidrs list must be reported as set") @@ -261,14 +273,28 @@ func TestConfig_OmittedVsEmptyReservedBlock(t *testing.T) { func TestConfig_Rejects(t *testing.T) { cases := map[string]string{ - "unknown top-level key": "version: 1\nallowed:\n - name: x\n", - "unknown rule key": "version: 1\nallow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n protocol: tcp\n", - "future version": "version: 99\nmgmt:\n cidrs: []\n", - "reserved allow name": "version: 1\nallow:\n - name: mgmt\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n", - "duplicate allow name": "version: 1\nallow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n - name: x\n cidrs: [\"10.1.0.0/16\"]\n ports: [\"80\"]\n", - "bad cidr": "version: 1\nmgmt:\n cidrs: [\"10.0.0.0\"]\n", - "bad port range": "version: 1\nallow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"2380-2379\"]\n", - "blocked with ports": "version: 1\nblocked:\n cidrs: []\n ports: [\"22\"]\n", + "unknown top-level key": allReservedYAML + "allowed:\n - name: x\n", + "unknown rule key": allReservedYAML + "allow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n protocol: tcp\n", + "future version": "version: 99\nmgmt:\n cidrs: []\nblocked:\n cidrs: []\nin_cluster:\n cidrs: []\n", + "reserved allow name": allReservedYAML + "allow:\n - name: mgmt\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n", + "duplicate allow name": allReservedYAML + "allow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n - name: x\n cidrs: [\"10.1.0.0/16\"]\n ports: [\"80\"]\n", + "bad cidr": "version: 1\nmgmt:\n cidrs: [\"10.0.0.0\"]\nblocked:\n cidrs: []\nin_cluster:\n cidrs: []\n", + "bad port range": allReservedYAML + "allow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"2380-2379\"]\n", + "blocked with ports": "version: 1\nmgmt:\n cidrs: [\"10.0.0.0/8\"]\nblocked:\n cidrs: []\n ports: [\"22\"]\nin_cluster:\n cidrs: []\n", + + // The reserved blocks are structural: a file that omits one is refused + // rather than quietly rendered against a weaver default the operator never + // wrote down. mgmt is the dangerous one — its default is an empty + // allowlist under a default-drop input chain — but all three are required + // so the file alone tells you the whole posture. + "missing mgmt block": "version: 1\nblocked:\n cidrs: []\nin_cluster:\n cidrs: []\n", + "missing blocked block": "version: 1\nmgmt:\n cidrs: [\"10.0.0.0/8\"]\nin_cluster:\n cidrs: []\n", + "missing in_cluster": mgmtBlockedYAML, + "empty file": "version: 1\n", + "null mgmt block": "version: 1\nmgmt:\nblocked:\n cidrs: []\nin_cluster:\n cidrs: []\n", + "mgmt without cidrs": "version: 1\nmgmt:\n ports: [\"22\"]\nblocked:\n cidrs: []\nin_cluster:\n cidrs: []\n", + "blocked without cidrs": "version: 1\nmgmt:\n cidrs: [\"10.0.0.0/8\"]\nblocked: {}\nin_cluster:\n cidrs: []\n", + "allow-only partial file": "version: 1\nallow:\n - name: x\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n", } for name, doc := range cases { t.Run(name, func(t *testing.T) { @@ -281,19 +307,21 @@ func TestConfig_Rejects(t *testing.T) { // TestConfig_MissingVersionIsAccepted keeps a hand-written file that forgot // `version:` working, while a version this build does not know is still refused // (covered above) — ignoring a field a newer weaver understands could leave the -// host with a firewall that does not match the file. +// host with a firewall that does not match the file. `version` is the one +// top-level key that may be omitted; the reserved blocks may not. func TestConfig_MissingVersionIsAccepted(t *testing.T) { - cfg, err := ParseConfig([]byte("mgmt:\n cidrs: [\"10.0.0.0/8\"]\n")) + cfg, err := ParseConfig([]byte("mgmt:\n cidrs: [\"10.0.0.0/8\"]\nblocked:\n cidrs: []\nin_cluster:\n cidrs: []\n")) require.NoError(t, err) tbl, err := cfg.Table() require.NoError(t, err) require.Equal(t, []string{"10.0.0.0/8"}, tbl.Mgmt.CIDRs) } -// TestManager_ApplyIsDeclarativeForAllowOnly pins the deliberate asymmetry -// between the two kinds of record: an allow rule absent from an applied config is -// removed, while a reserved block absent from it is defaulted rather than wiped — -// so a partial file cannot silently drop management access. +// TestManager_ApplyIsDeclarativeForAllowOnly pins that an allow rule absent from +// an applied config is removed. The reserved blocks cannot go missing from a file +// at all — they are required — so the only thing a config can leave to a default +// is a field inside a block it stated, which is checked here for in_cluster's +// port list. func TestManager_ApplyIsDeclarativeForAllowOnly(t *testing.T) { r := &fakeRunner{} applyCount := 0 @@ -307,6 +335,8 @@ func TestManager_ApplyIsDeclarativeForAllowOnly(t *testing.T) { cfg, err := ParseConfig([]byte( "version: 1\n" + "mgmt:\n cidrs: [\"10.0.0.0/8\"]\n ports: [\"22\"]\n" + + "blocked:\n cidrs: []\n" + + "in_cluster:\n cidrs: [\"10.4.0.0/14\"]\n" + "allow:\n - name: k8s-node\n cidrs: [\"10.0.0.0/24\"]\n ports: [\"6443\"]\n")) require.NoError(t, err) tbl, err := cfg.Table() @@ -318,8 +348,8 @@ func TestManager_ApplyIsDeclarativeForAllowOnly(t *testing.T) { require.NotContains(t, doc, "@cilium-vxlan") require.NotContains(t, doc, "@admin") - // The omitted in_cluster block came back as the default port set rather than - // as nothing. + // in_cluster's ports were omitted inside a block that was stated, so they came + // back as the default port set rather than as nothing. require.Contains(t, doc, "set in_cluster_ports { type inet_service; flags interval; auto-merge; elements = { 4244, 6443, 7472, 10250 }; }") } diff --git a/internal/network/firewall/config.go b/internal/network/firewall/config.go index 0502becb..9e5d52fc 100644 --- a/internal/network/firewall/config.go +++ b/internal/network/firewall/config.go @@ -25,10 +25,11 @@ const ConfigVersion = 1 // not by coincidence. // // The reserved blocks are pointers so an absent section is distinguishable from -// an empty one, which the semantics depend on: an omitted block is derived or -// defaulted, while a block present with an empty list renders no rule. `allow` -// needs no such distinction because it is wholly declarative — an entry absent -// from the file is deleted. +// an empty one. A file must state all three (see requireReservedBlocks): the +// file is the whole table, and a block left out would silently fall back to a +// compiled-in default — for `mgmt` that default is an empty allowlist under a +// default-drop policy, i.e. a lockout the operator never wrote down. A block +// present with an empty list renders no rule, which is how one is disabled. type FileConfig struct { Version int `yaml:"version"` Mgmt *Block `yaml:"mgmt,omitempty"` @@ -80,6 +81,9 @@ func ParseConfig(data []byte) (*FileConfig, error) { return nil, errorx.IllegalFormat.New( "unsupported firewall config version %d: this build understands version %d", c.Version, ConfigVersion) } + if err := c.requireReservedBlocks(); err != nil { + return nil, err + } // Validate through the model rather than field by field, so the config path // and the CLI path can never disagree about what is acceptable. @@ -93,6 +97,50 @@ func ParseConfig(data []byte) (*FileConfig, error) { return &c, nil } +// requireReservedBlocks rejects a config that leaves a reserved block to a +// compiled-in default. The file states the whole table — nothing is carried over +// from the host's current firewall — so an omitted block is not "keep what is +// there", it is "take weaver's default". For `mgmt` that default is an empty +// address list under the input chain's default drop, which locks the operator +// out of new connections without the file ever saying so. Requiring all three to +// be written down makes the rendered posture readable from the file alone. +// +// `cidrs` is required inside `mgmt` and `blocked` for the same reason: a block +// header with only `ports` under it would re-open the same hole one level down. +// It stays optional inside `in_cluster`, where omitting it 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. +// +// This runs on the persisted state file too, not only on --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. +func (c *FileConfig) requireReservedBlocks() error { + for _, b := range []struct { + name string + // cidrsRequired is false only for in_cluster, whose omitted address list + // means "auto-detect the pod CIDR" rather than "fall back to a default". + cidrsRequired bool + block *Block + }{ + {RuleMgmt, true, c.Mgmt}, + {RuleBlocked, true, c.Blocked}, + {RuleInCluster, false, c.InCluster}, + } { + if b.block == nil { + return errorx.IllegalFormat.New( + "firewall config is missing the required %q block: the file states the whole table and reserved blocks are never "+ + "inherited from the host, so each one must be written down (use %q to render no rule for it)", + b.name, b.name+": {cidrs: []}") + } + if b.cidrsRequired && b.block.CIDRs == nil { + return errorx.IllegalFormat.New( + "firewall config block %q is missing \"cidrs\": state the address list explicitly (use \"cidrs: []\" for none)", b.name) + } + } + return nil +} + // Table builds the Table this config describes, applying the defaults for any // omitted reserved field. The in-cluster address list is the one value it cannot // resolve on its own — see InClusterCIDRsUnset. @@ -139,6 +187,10 @@ func (c *FileConfig) Table() (*Table, error) { // list unspecified, so the caller knows to auto-detect the node's pod CIDR. An // explicitly empty list (`in_cluster: {cidrs: []}`) is *specified* — it means // "render no in-cluster rule" — and must not trigger detection. +// +// A parsed config always has the block itself (requireReservedBlocks), so in +// practice this reports on the `cidrs` field alone; the nil-block arm covers a +// config assembled in Go rather than decoded. func (c *FileConfig) InClusterCIDRsUnset() bool { return c.InCluster == nil || c.InCluster.CIDRs == nil }