From acbd22fbba03c61501ea6b4f1e608147b22ce8c9 Mon Sep 17 00:00:00 2001 From: alex-au Date: Sat, 15 Aug 2026 12:32:44 +1000 Subject: [PATCH] feat(network/firewall): declare a named allow rule from the CLI `create --from-file` was the only way to bring a named allow rule into existence, so admitting one monitoring host meant learning a schema and authoring a file that states the whole table -- which is fully declarative, removes any rule it omits, and can fall back to an empty management allowlist under the default-drop policy. Add `network firewall create-allow-rule --name `, which declares a rule that `add` then populates. Both `--cidr` and `--port` already take lists applied in one transaction, so a rule goes live in a single follow-up command. Expose the two Rule fields that had no flag, `--proto` and `--icmp-echo`, on the new verb and on `set`, so every field of every rule is now reachable from the CLI rather than only from a file. Declaring stays a separate verb from `add`: an unknown --name on add/remove/set still fails, so a typo edits nothing instead of quietly creating a second rule alongside the intended one. Re-declaring an existing name is create-if-missing, matching `create`. This required relaxing Rule.Validate, which rejected an allow rule with no CIDRs, or with no ports and no icmp_echo. A declared-but-unpopulated rule has to be representable for the sequence to work in any order. It is fail-closed: the template gates every emission on the address and port sets being non-empty, so an incomplete rule renders no accept rule at all -- verified against a live kernel. applyAndPersist warns about them instead. Also reject --proto/--icmp-echo on the reserved blocks, which previously accepted and silently ignored them: in_cluster took both, and mgmt and in_cluster accepted proto=tcp because they only rejected a mismatching value. The renderer fixes all three to TCP and the config schema carries no proto field for them, so any value accepted here reported a change that never happened. And fix create-allow-rule --name mgmt reporting "already exists" rather than naming the reserved block -- the reserved-name check has to run ahead of the create-if-missing branch, since those blocks always exist. Refs #1009 Signed-off-by: alex-au --- cmd/cli/commands/network/firewall/create.go | 5 +- .../network/firewall/create_allow_rule.go | 71 ++++++ cmd/cli/commands/network/firewall/firewall.go | 15 +- .../network/firewall/firewall_test.go | 115 ++++++++- cmd/cli/commands/network/firewall/set.go | 30 ++- docs/dev/traffic-shaper.md | 19 +- docs/quickstart.md | 47 +++- internal/network/firewall/allow_test.go | 218 +++++++++++++++++- internal/network/firewall/manager.go | 89 ++++++- internal/network/firewall/parse.go | 6 +- internal/network/firewall/rule.go | 51 ++-- internal/network/firewall/table.go | 14 ++ 12 files changed, 624 insertions(+), 56 deletions(-) create mode 100644 cmd/cli/commands/network/firewall/create_allow_rule.go diff --git a/cmd/cli/commands/network/firewall/create.go b/cmd/cli/commands/network/firewall/create.go index f866647c..ac34dc86 100644 --- a/cmd/cli/commands/network/firewall/create.go +++ b/cmd/cli/commands/network/firewall/create.go @@ -29,7 +29,8 @@ 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: the file states the whole " + + "--from-file states the whole table at once; `create-allow-rule` declares a single named allow rule without " + + "a file. --from-file 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 " + @@ -151,7 +152,7 @@ func init() { 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") + createCmd.Flags().StringVar(&flagFromFile, "from-file", "", "Declarative YAML config to render the whole table from; 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. diff --git a/cmd/cli/commands/network/firewall/create_allow_rule.go b/cmd/cli/commands/network/firewall/create_allow_rule.go new file mode 100644 index 00000000..2ee7d90d --- /dev/null +++ b/cmd/cli/commands/network/firewall/create_allow_rule.go @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: Apache-2.0 + +package firewall + +import ( + "strings" + + "github.com/automa-saga/logx" + "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" +) + +var createAllowRuleCmd = &cobra.Command{ + Use: "create-allow-rule", + Short: "Declare a named allow rule, then populate it with `add`", + Long: "Declare a named allow rule on the host firewall. The rule is created empty: `add --name ` " + + "supplies its addresses and ports afterwards, and both lists take comma-separated values, so one `add` " + + "finishes the rule in a single atomic apply.\n\n" + + "A declared rule renders no nft rule until it has at least one CIDR and either a port or --icmp-echo, so " + + "running the declare and the populate as separate commands never opens access early.\n\n" + + "With --force an existing rule is replaced outright: every field not supplied again returns to its " + + "default, so --proto and --icmp-echo are reset along with the addresses and ports. Use `set` to change " + + "one field of a populated rule.\n\n" + + "Declaring is a separate verb from `create`, which states the whole table. It is also separate from `add`: " + + "an unknown --name on add/remove/set keeps failing, so a typo edits nothing rather than quietly creating a " + + "second rule alongside the intended one. The reserved blocks (" + strings.Join(fw.ReservedNames, ", ") + + ") cannot be declared this way — they always exist and are configured through `create` and `set`.", + RunE: func(cmd *cobra.Command, args []string) error { + if !cmd.Flags().Changed("name") { + return errorx.IllegalArgument.New("--name is required: the name of the allow rule to declare") + } + + force, err := common.FlagForce().Value(cmd, args) + if err != nil { + return err + } + + // Only carry a field the operator actually set. Leaving Proto empty keeps + // it absent from `show --output yaml` and lets the model apply its own tcp + // default, so a declared rule round-trips through the config unchanged. + r := fw.Rule{Name: flagName} + if cmd.Flags().Changed("proto") { + r.Proto = fw.Proto(flagProto) + } + if cmd.Flags().Changed("icmp-echo") { + r.ICMPEcho = flagICMPEcho + } + + changed, err := newManager().CreateRule(cmd.Context(), r, force) + if err != nil { + return err + } + + if changed { + logx.As().Info().Str("rule", r.Name).Msg( + "allow rule declared; populate it with `network firewall add --name " + r.Name + " --cidr --port `") + } + return nil + }, +} + +func init() { + createAllowRuleCmd.Flags().StringVar(&flagName, "name", "", + "Name of the allow rule to declare (may not be a reserved block: "+strings.Join(fw.ReservedNames, ", ")+")") + createAllowRuleCmd.Flags().StringVar(&flagProto, "proto", "", + "L4 protocol the rule's ports match: tcp or udp (default tcp)") + createAllowRuleCmd.Flags().BoolVar(&flagICMPEcho, "icmp-echo", false, + "Grant this rule's sources unmetered ICMP echo-request, above the rate meter") +} diff --git a/cmd/cli/commands/network/firewall/firewall.go b/cmd/cli/commands/network/firewall/firewall.go index bca6f16b..8b78f640 100644 --- a/cmd/cli/commands/network/firewall/firewall.go +++ b/cmd/cli/commands/network/firewall/firewall.go @@ -7,10 +7,11 @@ // 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. +// what protocol each matches — is declared by its own verb (create for the whole +// table, create-allow-rule for one named rule), so bringing a rule into +// existence is always explicit. Membership — the addresses and ports inside a +// rule — is moved by add/remove/set, which refuse an unknown --name so a typo +// edits nothing rather than declaring something new. package firewall import ( @@ -43,6 +44,8 @@ var ( flagFromFile string flagOutput string flagAll bool + flagProto string + flagICMPEcho bool // Per-block flags that predate --name, retained so every invocation that // worked before still works and the interactive install flow is unchanged. @@ -62,13 +65,13 @@ var firewallCmd = &cobra.Command{ Long: "Manage the node-agnostic host firewall: the `inet weaver-host-firewall` nftables table that protects the " + "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` " + + "allow rules declared with `create-allow-rule`. This table is separate from the `inet weaver-workload-policy` " + "workload plane and applies to every node type.", RunE: common.DefaultRunE, } func init() { - firewallCmd.AddCommand(createCmd, addCmd, removeCmd, setCmd, showCmd, deleteCmd) + firewallCmd.AddCommand(createCmd, createAllowRuleCmd, addCmd, removeCmd, setCmd, showCmd, deleteCmd) } // GetCmd returns the root of the `network firewall` command group. diff --git a/cmd/cli/commands/network/firewall/firewall_test.go b/cmd/cli/commands/network/firewall/firewall_test.go index 65fe6a84..fa80bb46 100644 --- a/cmd/cli/commands/network/firewall/firewall_test.go +++ b/cmd/cli/commands/network/firewall/firewall_test.go @@ -36,7 +36,7 @@ func TestFirewallCmd_Structure(t *testing.T) { cmd := GetCmd() require.Equal(t, "firewall", cmd.Use) - want := map[string]bool{"create": false, "add": false, "remove": false, "set": false, "show": false, "delete": false} + want := map[string]bool{"create": false, "create-allow-rule": false, "add": false, "remove": false, "set": false, "show": false, "delete": false} for _, sub := range cmd.Commands() { if _, ok := want[sub.Use]; ok { want[sub.Use] = true @@ -69,7 +69,8 @@ func TestVerbs_NameAddressedFlags(t *testing.T) { }{ {addCmd, "add", []string{"name", "cidr", "port"}}, {removeCmd, "remove", []string{"name", "cidr", "port"}}, - {setCmd, "set", []string{"name", "cidrs", "cidrs-file", "ports"}}, + {setCmd, "set", []string{"name", "cidrs", "cidrs-file", "ports", "proto", "icmp-echo"}}, + {createAllowRuleCmd, "create-allow-rule", []string{"name", "proto", "icmp-echo"}}, {showCmd, "show", []string{"name", "output"}}, {deleteCmd, "delete", []string{"name", "all"}}, } { @@ -154,6 +155,7 @@ func resetFlagState(t *testing.T) { flagMgmtCIDRs, flagBlockedCIDRs, flagInClusterPorts = nil, nil, nil flagMgmtCIDR, flagBlockedCIDR = "", "" flagInClusterPort, flagSSHPort = 0, 0 + flagProto, flagICMPEcho = "", false } // TestBackwardCompatibleInvocations is the regression gate the generalisation @@ -204,6 +206,115 @@ func TestBackwardCompatibleInvocations(t *testing.T) { require.NoFileExists(t, nftPath) } +// TestCreateAllowRuleCmd is the end-to-end shape #1009 exists to deliver: a +// named allow rule declared, populated and deleted with no config file anywhere. +func TestCreateAllowRuleCmd(t *testing.T) { + nftPath, _ := stubManager(t) + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + + // Declared, but rendering nothing yet — the operator has opened no access by + // running only the first half of the sequence. + require.NoError(t, run(t, "create-allow-rule", "--name", "rudder_server", "--proto", "udp", "--icmp-echo")) + doc := readFile(t, nftPath) + require.NotContains(t, doc, "@rudder_server udp dport") + + // One add carries every CIDR and port, so the rule goes live in a single + // atomic apply rather than one per element. + require.NoError(t, run(t, "add", "--name", "rudder_server", + "--cidr", "200.201.203.205/32,10.1.0.0/16", "--port", "5309,8443,9000-9100")) + doc = readFile(t, nftPath) + require.Contains(t, doc, "ip saddr @rudder_server udp dport @rudder_server_ports accept", + "--proto udp must reach the rendered rule") + require.Contains(t, doc, "ip saddr @rudder_server icmp type echo-request accept", + "--icmp-echo must reach the rendered rule") + require.Contains(t, doc, "elements = { 10.1.0.0/16, 200.201.203.205/32 }") + require.Contains(t, doc, "elements = { 5309, 8443, 9000-9100 }", + "a port range must survive as one element, and the list must be sorted") + + // Deletion needs no new verb. + require.NoError(t, run(t, "delete", "--name", "rudder_server")) + require.NotContains(t, readFile(t, nftPath), "@rudder_server") +} + +func TestCreateAllowRuleCmd_Rejections(t *testing.T) { + stubManager(t) + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + + require.ErrorContains(t, run(t, "create-allow-rule"), "--name is required") + + // A reserved block is not an allow rule. It also already exists, so this must + // not be mistaken for the create-if-missing no-op path. + for _, name := range fw.ReservedNames { + require.ErrorContains(t, run(t, "create-allow-rule", "--name", name), "reserved name", name) + } + + require.Error(t, run(t, "create-allow-rule", "--name", "bad name")) + require.Error(t, run(t, "create-allow-rule", "--name", "x", "--proto", "sctp")) + // Would silently claim the mgmt block's nft set. + require.ErrorContains(t, run(t, "create-allow-rule", "--name", "mgmt_addrs"), "derive the nft set name") +} + +// TestCreateAllowRuleCmd_ForceRedeclares pins create-if-missing, matching the +// table-level `create`. +func TestCreateAllowRuleCmd_ForceRedeclares(t *testing.T) { + nftPath, _ := stubManager(t) + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + require.NoError(t, run(t, "create-allow-rule", "--name", "svc")) + require.NoError(t, run(t, "add", "--name", "svc", "--cidr", "203.0.113.5/32", "--port", "9000")) + require.Contains(t, readFile(t, nftPath), "ip saddr @svc tcp dport @svc_ports accept") + + // Without --force the existing rule and its membership survive. + require.NoError(t, run(t, "create-allow-rule", "--name", "svc", "--proto", "udp")) + require.Contains(t, readFile(t, nftPath), "ip saddr @svc tcp dport @svc_ports accept") + + // With --force the declaration replaces it, membership included. + require.NoError(t, run(t, "create-allow-rule", "--name", "svc", "--proto", "udp", "--force")) + require.NotContains(t, readFile(t, nftPath), "@svc tcp dport") + require.NotContains(t, readFile(t, nftPath), "@svc udp dport") +} + +// TestSetCmd_ProtoAndICMPEcho covers the other half of "every Rule field is +// reachable from the CLI": the two fields must be editable after declaration, +// not only settable at declaration time. +func TestSetCmd_ProtoAndICMPEcho(t *testing.T) { + nftPath, _ := stubManager(t) + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + require.NoError(t, run(t, "create-allow-rule", "--name", "svc")) + require.NoError(t, run(t, "add", "--name", "svc", "--cidr", "203.0.113.5/32", "--port", "9000")) + + require.NoError(t, run(t, "set", "--name", "svc", "--proto", "udp", "--icmp-echo")) + doc := readFile(t, nftPath) + require.Contains(t, doc, "ip saddr @svc udp dport @svc_ports accept") + require.Contains(t, doc, "ip saddr @svc icmp type echo-request accept") + + // Revoking echo is expressible. + require.NoError(t, run(t, "set", "--name", "svc", "--icmp-echo=false")) + require.NotContains(t, readFile(t, nftPath), "@svc icmp type") + + // The reserved blocks render a fixed shape and reject both — including the + // proto value that happens to match what they already render. + require.Error(t, run(t, "set", "--name", "mgmt", "--proto", "udp")) + require.Error(t, run(t, "set", "--name", "mgmt", "--proto", "tcp")) + require.Error(t, run(t, "set", "--name", "in_cluster", "--proto", "tcp")) + require.Error(t, run(t, "set", "--name", "in_cluster", "--icmp-echo")) + + // --name with none of the value flags is still rejected. + require.ErrorContains(t, run(t, "set", "--name", "svc"), "at least one of") +} + +// TestUnknownRuleNameNeverDeclares is the AC that keeps a typo from creating a +// second rule alongside the intended one. +func TestUnknownRuleNameNeverDeclares(t *testing.T) { + nftPath, _ := stubManager(t) + require.NoError(t, run(t, "create", "--mgmt-cidrs", "10.0.0.0/8")) + require.NoError(t, run(t, "create-allow-rule", "--name", "rudder_server")) + + require.ErrorContains(t, run(t, "add", "--name", "rudder_sever", "--cidr", "10.0.0.1/32"), "no rule named") + require.ErrorContains(t, run(t, "remove", "--name", "rudder_sever", "--cidr", "10.0.0.1/32"), "no rule named") + require.ErrorContains(t, run(t, "set", "--name", "rudder_sever", "--cidrs", "10.0.0.1/32"), "no rule named") + require.NotContains(t, readFile(t, nftPath), "rudder_sever") +} + func TestCreateCmd_FromFile(t *testing.T) { nftPath, _ := stubManager(t) dir := t.TempDir() diff --git a/cmd/cli/commands/network/firewall/set.go b/cmd/cli/commands/network/firewall/set.go index e1e1195d..aa7622da 100644 --- a/cmd/cli/commands/network/firewall/set.go +++ b/cmd/cli/commands/network/firewall/set.go @@ -18,7 +18,9 @@ var setCmd = &cobra.Command{ "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.", + "block's addresses is how you disable it without deleting it.\n\n" + + "--proto and --icmp-echo change what an allow rule matches rather than who is in it; the reserved blocks " + + "reject both, since they render a fixed shape.", RunE: func(cmd *cobra.Command, _ []string) error { updates, err := resolveSetUpdates(cmd) if err != nil { @@ -34,7 +36,8 @@ var setCmd = &cobra.Command{ // 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") + general := f.Changed("name") || f.Changed("cidrs") || f.Changed("cidrs-file") || f.Changed("ports") || + f.Changed("proto") || f.Changed("icmp-echo") var legacy []fw.Update if f.Changed("mgmt-cidrs") { @@ -66,10 +69,25 @@ func resolveSetUpdates(cmd *cobra.Command) ([]fw.Update, error) { 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") + + // Pointers rather than values: "" is a real proto setting (meaning "back to + // the tcp default") and false is a real --icmp-echo setting, so only the + // flag having been given can distinguish them from "leave this alone". + var proto *fw.Proto + if f.Changed("proto") { + p := fw.Proto(flagProto) + proto = &p + } + var icmpEcho *bool + if f.Changed("icmp-echo") { + icmpEcho = &flagICMPEcho + } + + if cidrs == nil && ports == nil && proto == nil && icmpEcho == nil { + return nil, errorx.IllegalArgument.New( + "at least one of --cidrs, --cidrs-file, --ports, --proto or --icmp-echo is required") } - return []fw.Update{{Name: flagName, CIDRs: cidrs, Ports: ports}}, nil + return []fw.Update{{Name: flagName, CIDRs: cidrs, Ports: ports, Proto: proto, ICMPEcho: icmpEcho}}, nil } // resolveCIDRs returns the replacement address list from --cidrs or --cidrs-file @@ -130,6 +148,8 @@ func init() { 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().StringVar(&flagProto, "proto", "", "L4 protocol the rule's ports match: tcp or udp (allow rules only; empty restores the tcp default)") + setCmd.Flags().BoolVar(&flagICMPEcho, "icmp-echo", false, "Grant or revoke unmetered ICMP echo-request for this rule's sources (allow rules only)") 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)") diff --git a/docs/dev/traffic-shaper.md b/docs/dev/traffic-shaper.md index ce933207..25d986a0 100644 --- a/docs/dev/traffic-shaper.md +++ b/docs/dev/traffic-shaper.md @@ -440,7 +440,8 @@ asked first**, then traffic shaping: Configured by `--mgmt-cidrs`, `--blocked-cidrs`, `--ssh-port`, `--pod-cidr`, `--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. + `network firewall create-allow-rule` (or `create --from-file` for the whole + table), 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 @@ -460,14 +461,18 @@ The three `network` sub-scopes drive each plane directly; every mutation live- applies and then persists (see below), so they are safe to run by hand on a 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`, or `--from-file` for the - whole table; `add`/`remove`/`set`/`delete` take `--name` to address one rule — +- **`network firewall`** (`create`/`create-allow-rule`/`add`/`remove`/`set`/ + `show`/`delete`) — the host firewall. `create` takes `--mgmt-cidrs`, + `--blocked-cidrs`, `--in-cluster-ports`, `--ssh-port`, `--pod-cidr`, or + `--from-file` for the whole table; `create-allow-rule` declares one named allow + rule (`--name`, `--proto`, `--icmp-echo`); + `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. + their protocol) is declared by its own verb, so bringing a rule into existence + is always explicit; membership is moved by `add`/`remove`/`set`, which refuse + an unknown `--name` so a typo edits nothing. A declared rule may be empty and + renders nothing until it has a CIDR and either a port or `icmp_echo`. `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`) — diff --git a/docs/quickstart.md b/docs/quickstart.md index 8914a308..1f5394ee 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -692,7 +692,36 @@ ICMP is a fixed, safe ruleset: full ICMP from the management allowlist, and from #### Declare Named Allow Rules -`--from-file` is the only way to declare a named allow rule, and it renders the whole table: +`create-allow-rule` declares one named allow rule; `add` then supplies its addresses and ports. No file is involved, and both lists take comma-separated values, so one `add` finishes the rule in a single atomic apply: + +```bash +# Declare the rule, then populate it +sudo solo-provisioner network firewall create-allow-rule --name rudder_server --proto tcp --icmp-echo +sudo solo-provisioner network firewall add --name rudder_server \ + --cidr 200.201.203.205/32,10.1.0.0/16 --port 5309,8443,9000-9100 + +# Deletion needs no separate verb +sudo solo-provisioner network firewall delete --name rudder_server +``` + +**Flags**: + +| Flag | Description | Default | +|----------------|--------------------------------------------------------------------------------------|---------| +| `--name` | Name of the allow rule to declare (may not be a reserved block: `mgmt`, `blocked`, `in_cluster`) | (required) | +| `--proto` | L4 protocol the rule's ports match: `tcp` or `udp` | `tcp` | +| `--icmp-echo` | Grant this rule's sources unmetered ICMP echo-request, above the rate meter | `false` | +| `--force` | Replace an existing rule, **resetting the whole rule** — addresses, ports, `proto` and `icmp_echo` all return to their defaults unless supplied again (global flag) | `false` | + +A rule is declared before it has any members, and **renders nothing** until it has at least one CIDR and either a port or `--icmp-echo` — so running the declare and the populate as separate commands never opens access early. An incomplete rule is reported as a warning on every apply. + +Declaring is deliberately a separate verb from `add`: an unknown `--name` on `add`/`remove`/`set` keeps failing, so a typo edits nothing rather than quietly creating a second rule alongside the intended one. Re-declaring an existing name without `--force` warns and changes nothing, mirroring `network firewall create`. With `--force` the declaration **replaces** the rule outright, so `create-allow-rule --name x --force` on its own resets `proto` and `icmp_echo` as well as emptying the address and port lists — use `set` to change one field of a populated rule. + +`--proto` and `--icmp-echo` are also settable on `set` (see below), so a rule's protocol can be corrected without deleting and re-declaring it. The reserved blocks reject both — they render a fixed shape. + +##### Declaring the whole table from a file + +`create --from-file` states the whole table at once, as an alternative to the sequence above: ```yaml version: 1 @@ -757,7 +786,7 @@ sudo solo-provisioner network firewall create --from-file rules.yaml --force `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. +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`, or delete the file and re-run the `create` + `create-allow-rule` sequence, to repair one. #### Modify a Rule's Addresses / Ports @@ -770,6 +799,16 @@ sudo solo-provisioner network firewall add --name k8s-node --cidr 10.0.0.5/32 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 + +# Both lists are comma-separated or repeated, and one invocation is one atomic +# apply — so a rule can be populated in full without a command per element +sudo solo-provisioner network firewall add --name k8s-node \ + --cidr 10.0.0.5/32,10.0.0.6/32 --port 6443,2379-2380,10250 + +# --proto and --icmp-echo change what an allow rule matches, rather than who is in it +sudo solo-provisioner network firewall set --name cilium-vxlan --proto udp +sudo solo-provisioner network firewall set --name admin --icmp-echo +sudo solo-provisioner network firewall set --name admin --icmp-echo=false ``` **Flags**: @@ -782,6 +821,10 @@ sudo solo-provisioner network firewall set --name mgmt --cidrs-file /etc/ | `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) | +| `set` | `--proto` | L4 protocol the rule's ports match: `tcp` or `udp` (allow rules only; empty restores the `tcp` default) | +| `set` | `--icmp-echo` | Grant or revoke unmetered ICMP echo-request for this rule's sources (allow rules only) | + +> `add`/`remove` operate on membership only. To change an allow rule's `--proto` or `--icmp-echo` after it is declared, use `set` — `create-allow-rule --force` would reset the rest of the rule. The reserved blocks reject both flags outright, **including `--proto tcp`**: they render a fixed shape (TCP, with `mgmt` carrying its own broader ICMP type list), so accepting the value that happens to match would report a change the renderer ignores. The pre-existing per-block flags are retained as shorthands that name their reserved block implicitly, so every earlier invocation still works unchanged: diff --git a/internal/network/firewall/allow_test.go b/internal/network/firewall/allow_test.go index 5e22cac2..219db873 100644 --- a/internal/network/firewall/allow_test.go +++ b/internal/network/firewall/allow_test.go @@ -141,11 +141,21 @@ func TestTable_ValidateRejects(t *testing.T) { "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"}}) + "mgmt with tcp (matching value is still refused)": func(tbl *Table) error { + tbl.Mgmt.Proto = ProtoTCP + return tbl.Validate() }, - "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"}}) + "in_cluster with tcp (matching value is still refused)": func(tbl *Table) error { + tbl.InCluster.Proto = ProtoTCP + return tbl.Validate() + }, + "in_cluster with icmp_echo": func(tbl *Table) error { + tbl.InCluster.ICMPEcho = true + return tbl.Validate() + }, + "in_cluster with udp": func(tbl *Table) error { + tbl.InCluster.Proto = ProtoUDP + return tbl.Validate() }, "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"}}) @@ -158,6 +168,206 @@ func TestTable_ValidateRejects(t *testing.T) { } } +// TestTable_IncompleteAllowRulesAreLegalAndRenderNothing covers the state that +// makes `create-allow-rule` possible: a rule declared before it is populated. +// Every intermediate state of a declare-then-populate sequence has to validate, +// in whatever order the operator runs the element verbs, and none of them may +// grant anything until the rule is complete. +func TestTable_IncompleteAllowRulesAreLegalAndRenderNothing(t *testing.T) { + cases := map[string]Rule{ + "declared with nothing": {Name: "pending"}, + "cidrs but no ports": {Name: "pending", CIDRs: []string{"203.0.113.5/32"}}, + "ports but no cidrs": {Name: "pending", Ports: []string{"5309"}}, + "icmp_echo but no cidrs": {Name: "pending", ICMPEcho: true}, + } + for name, r := range cases { + t.Run(name, func(t *testing.T) { + tbl := sampleTable() + require.NoError(t, tbl.UpsertAllow(r)) + require.Equal(t, []string{"pending"}, tbl.IncompleteAllowRules()) + + doc, err := tbl.Render() + require.NoError(t, err) + // No transport rule and no echo accept in either family. + require.NotContains(t, doc, "@pending tcp dport") + require.NotContains(t, doc, "@pending udp dport") + require.NotContains(t, doc, "@pending icmp type") + require.NotContains(t, doc, "@pending6 icmp") + }) + } + + // Once complete it stops being reported and starts rendering. + tbl := sampleTable() + require.NoError(t, tbl.UpsertAllow(Rule{Name: "pending", CIDRs: []string{"203.0.113.5/32"}, Ports: []string{"5309"}})) + require.Empty(t, tbl.IncompleteAllowRules()) + doc, err := tbl.Render() + require.NoError(t, err) + require.Contains(t, doc, "ip saddr @pending tcp dport @pending_ports accept") +} + +// TestRender_CLIDeclaredMatchesFileDeclared is the acceptance criterion for +// #1009: a rule built up through the CLI verbs must render byte-identically to +// the same rule loaded from the persisted config. The two paths share +// UpsertAllow and Rule.Validate precisely so this holds — in particular the port +// sort, which is what would otherwise make the render depend on the order the +// operator happened to add ports in. +func TestRender_CLIDeclaredMatchesFileDeclared(t *testing.T) { + // Both sides start from the same reserved blocks, so the comparison isolates + // the allow rule rather than the surrounding table. + baseCfg, err := ParseConfig([]byte(allReservedYAML)) + require.NoError(t, err) + + // The CLI path: declare, then populate in an awkward order — ports before + // addresses, and each list unsorted and mixed-family. + viaCLI, err := baseCfg.Table() + require.NoError(t, err) + require.NoError(t, viaCLI.UpsertAllow(Rule{Name: "rudder-server", Proto: ProtoUDP, ICMPEcho: true})) + r, ok := viaCLI.Rule("rudder-server") + require.True(t, ok) + require.NoError(t, r.AddPorts([]string{"8443", "5309"})) + require.NoError(t, r.AddCIDRs([]string{"2001:db8:5e5::/64", "200.201.203.205/32"})) + + // The file path: the same rule stated declaratively. + cfg, err := ParseConfig([]byte(allReservedYAML + + "allow:\n" + + " - name: rudder-server\n" + + " cidrs: [\"200.201.203.205/32\", \"2001:db8:5e5::/64\"]\n" + + " ports: [\"5309\", \"8443\"]\n" + + " proto: udp\n" + + " icmp_echo: true\n")) + require.NoError(t, err) + viaFile, err := cfg.Table() + require.NoError(t, err) + + fromCLI, err := viaCLI.Render() + require.NoError(t, err) + fromFile, err := viaFile.Render() + require.NoError(t, err) + require.Equal(t, fromFile, fromCLI) + + // Guard against the assertion passing because neither rendered anything. + require.Contains(t, fromCLI, "ip saddr @rudder-server udp dport @rudder-server_ports accept") + require.Contains(t, fromCLI, "ip6 saddr @rudder-server6 udp dport @rudder-server_ports accept") + require.Contains(t, fromCLI, "ip saddr @rudder-server icmp type echo-request accept") +} + +// TestManager_CreateRule covers the declare verb's contract: create-if-missing, +// --force replaces, and the reserved names stay refused. +func TestManager_CreateRule(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + ctx := context.Background() + require.NoError(t, m.Apply(ctx, sampleTable())) + + // Declared empty, then populated by the element verbs. + changed, err := m.CreateRule(ctx, Rule{Name: "rudder", Proto: ProtoUDP}, false) + require.NoError(t, err) + require.True(t, changed) + require.NoError(t, m.Add(ctx, "rudder", []string{"200.201.203.205/32"}, []string{"5309", "8443"})) + require.Contains(t, readNft(t, nftPath), "ip saddr @rudder udp dport @rudder_ports accept") + + // Re-declaring without --force leaves the populated rule alone, and applies + // nothing at all rather than re-rendering an identical document. + before := applyCount + changed, err = m.CreateRule(ctx, Rule{Name: "rudder"}, false) + require.NoError(t, err) + require.False(t, changed) + require.Equal(t, before, applyCount, "an already-declared rule must not restart the nft unit") + tbl, err := m.Table(ctx) + require.NoError(t, err) + got, ok := tbl.Rule("rudder") + require.True(t, ok) + require.Equal(t, []string{"200.201.203.205/32"}, got.CIDRs) + require.Equal(t, ProtoUDP, got.Proto) + + // --force redeclares, which clears membership. + changed, err = m.CreateRule(ctx, Rule{Name: "rudder"}, true) + require.NoError(t, err) + require.True(t, changed) + tbl, err = m.Table(ctx) + require.NoError(t, err) + got, ok = tbl.Rule("rudder") + require.True(t, ok) + require.Empty(t, got.CIDRs) + require.Empty(t, got.Ports) + // --force replaces the whole rule, so proto and icmp_echo reset as well — + // the redeclare above supplied neither. + require.Equal(t, Proto(""), got.Proto) + require.False(t, got.ICMPEcho) + require.NotContains(t, readNft(t, nftPath), "@rudder udp dport") + + // Reserved names and set-name collisions are refused, by UpsertAllow and + // Table.Validate respectively. + _, err = m.CreateRule(ctx, Rule{Name: RuleMgmt}, false) + require.ErrorContains(t, err, "reserved name") + _, err = m.CreateRule(ctx, Rule{Name: "mgmt_addrs"}, false) + require.ErrorContains(t, err, "derive the nft set name") +} + +// TestManager_SetProtoAndICMPEcho covers editing the two fields that have no +// membership: they must be changeable after declaration, and refused on the +// reserved blocks, which render a fixed shape. +func TestManager_SetProtoAndICMPEcho(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, nftPath := newTestManager(t, r, &applyCount) + ctx := context.Background() + require.NoError(t, m.Apply(ctx, sampleTable())) + _, err := m.CreateRule(ctx, Rule{Name: "svc"}, false) + require.NoError(t, err) + require.NoError(t, m.Add(ctx, "svc", []string{"203.0.113.5/32"}, []string{"9000"})) + require.Contains(t, readNft(t, nftPath), "ip saddr @svc tcp dport @svc_ports accept") + + udp, echo := ProtoUDP, true + require.NoError(t, m.SetMany(ctx, []Update{{Name: "svc", Proto: &udp, ICMPEcho: &echo}})) + doc := readNft(t, nftPath) + require.Contains(t, doc, "ip saddr @svc udp dport @svc_ports accept") + require.Contains(t, doc, "ip saddr @svc icmp type echo-request accept") + + // A nil pointer leaves the field alone: setting only ports must not reset proto. + require.NoError(t, m.SetMany(ctx, []Update{{Name: "svc", Ports: []string{"9001"}}})) + require.Contains(t, readNft(t, nftPath), "ip saddr @svc udp dport @svc_ports accept") + + // Revoking echo is expressible, which a bare bool could not distinguish + // from "not supplied". + off := false + require.NoError(t, m.SetMany(ctx, []Update{{Name: "svc", ICMPEcho: &off}})) + require.NotContains(t, readNft(t, nftPath), "@svc icmp type") + + // Reserved blocks render a fixed shape, so both fields are refused — including + // proto=tcp, which would otherwise report a change the renderer ignores. + for _, name := range ReservedNames { + on := true + for _, proto := range []Proto{ProtoUDP, ProtoTCP} { + require.Error(t, m.SetMany(ctx, []Update{{Name: name, Proto: &proto}}), "%s proto=%s", name, proto) + } + require.Error(t, m.SetMany(ctx, []Update{{Name: name, ICMPEcho: &on}}), name) + } +} + +// TestManager_UnknownRuleNameStillFails pins the invariant that keeps a typo +// from silently creating a second rule: the element verbs never declare. +func TestManager_UnknownRuleNameStillFails(t *testing.T) { + r := &fakeRunner{} + applyCount := 0 + m, _ := newTestManager(t, r, &applyCount) + ctx := context.Background() + require.NoError(t, m.Apply(ctx, sampleTable())) + + require.ErrorContains(t, m.Add(ctx, "typo", []string{"10.0.0.0/8"}, nil), "no rule named") + require.ErrorContains(t, m.Remove(ctx, "typo", []string{"10.0.0.0/8"}, nil), "no rule named") + require.ErrorContains(t, m.Set(ctx, "typo", []string{"10.0.0.0/8"}, nil), "no rule named") + + // The message points at the verb that would have created it. + require.ErrorContains(t, m.Add(ctx, "typo", []string{"10.0.0.0/8"}, nil), "create-allow-rule") + + tbl, err := m.Table(ctx) + require.NoError(t, err) + _, ok := tbl.Rule("typo") + require.False(t, ok) +} + // 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. diff --git a/internal/network/firewall/manager.go b/internal/network/firewall/manager.go index 1f2f3f89..9df9ec2e 100644 --- a/internal/network/firewall/manager.go +++ b/internal/network/firewall/manager.go @@ -104,6 +104,54 @@ func (m *Manager) Apply(ctx context.Context, t *Table) error { return m.withLock(func() error { return m.applyAndPersist(ctx, t) }) } +// CreateRule declares a named allow rule, so a rule can be brought into +// existence without a config file. It is create-if-missing like Create: a name +// that already exists is left alone and reported as unchanged unless force is +// set, in which case the rule is replaced outright — the declaration states the +// whole rule, so every field not supplied on the redeclare returns to its +// default, membership and matching alike. +// +// The rule may be declared with no members; the element verbs populate it +// afterwards. It renders nothing until it has both sources and a destination, so +// running the declare and the populate as separate commands never opens access +// early. +// +// Deliberately not built on mutate: mutate always re-applies, and the +// already-exists path has nothing to apply — re-rendering an identical document +// would restart the nft unit for no reason. +func (m *Manager) CreateRule(ctx context.Context, r Rule, force bool) (bool, error) { + // Ahead of the exists check below, because the reserved blocks always exist: + // left to that branch, `create-allow-rule --name mgmt` would report "already + // exists" and succeed, when what the operator needs to be told is that mgmt + // is not an allow rule at all. + if IsReserved(r.Name) { + return false, 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) + } + + var changed bool + err := m.withLock(func() error { + t, err := m.load() + if err != nil { + return err + } + if _, exists := t.Rule(r.Name); exists && !force { + logx.As().Warn().Str("rule", r.Name).Msg( + "allow rule already exists — the supplied flags were not applied; pass --force to replace it, which resets the whole rule (addresses, ports, proto and icmp_echo)") + return nil + } + // UpsertAllow rejects the reserved names and runs Rule.Validate, so a + // CLI-declared rule is held to exactly the same rules as a file-declared + // one. Table.Validate then catches set-name collisions before render. + if err := t.UpsertAllow(r); err != nil { + return err + } + changed = true + return m.applyAndPersist(ctx, t) + }) + return changed, err +} + // 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 { @@ -126,11 +174,15 @@ func (m *Manager) Remove(ctx context.Context, name string, cidrs, ports []string } // Update is one rule's replacement membership for SetMany. A nil slice leaves -// that dimension unchanged; an empty (non-nil) slice clears it. +// that dimension unchanged; an empty (non-nil) slice clears it. Proto and +// ICMPEcho follow the same convention with pointers, since their zero values +// ("" and false) are both meaningful settings rather than "not supplied". type Update struct { - Name string - CIDRs []string - Ports []string + Name string + CIDRs []string + Ports []string + Proto *Proto + ICMPEcho *bool } // Set atomically replaces the named rule's address list and/or port list. @@ -148,8 +200,8 @@ func (m *Manager) SetMany(ctx context.Context, updates []Update) error { 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(), ", ")) + "no rule named %q; known rules are %s. Declare a new allow rule with `network firewall create-allow-rule --name %s`", + u.Name, strings.Join(t.Names(), ", "), u.Name) } if u.CIDRs != nil { if err := r.SetCIDRs(u.CIDRs); err != nil { @@ -161,6 +213,12 @@ func (m *Manager) SetMany(ctx context.Context, updates []Update) error { return err } } + if u.Proto != nil { + r.Proto = *u.Proto + } + if u.ICMPEcho != nil { + r.ICMPEcho = *u.ICMPEcho + } } return nil }) @@ -257,15 +315,15 @@ func (m *Manager) mutate(ctx context.Context, fn func(*Table) error) error { // 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. +// declaring a rule is its own verb, so a mistyped --name edits nothing instead +// of quietly creating a second rule alongside the one that was meant. 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(), ", ")) + "no rule named %q; known rules are %s. Declare a new allow rule with `network firewall create-allow-rule --name %s`", + name, strings.Join(t.Names(), ", "), name) } return fn(r) }) @@ -294,6 +352,15 @@ func (m *Manager) applyAndPersist(ctx context.Context, t *Table) error { return err } + // Declaring a rule before populating it is supported, so this is not an + // error — but a rule that grants nothing is indistinguishable from a + // finished one in `show`, and a half-run declare sequence is the likeliest + // way to end up here. + if names := t.IncompleteAllowRules(); len(names) > 0 { + logx.As().Warn().Strs("rules", names).Msg( + "allow rule(s) render nothing yet: each needs at least one CIDR and either a port or icmp_echo — populate with `network firewall add --name --cidr --port `") + } + cfg, err := FileConfigFromTable(t).Marshal() if err != nil { return err @@ -370,7 +437,7 @@ func (m *Manager) load() (*Table, error) { return nil, errorx.ExternalError.Wrap(err, "failed to read %s", m.nftPath) } 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)") + "no host firewall config file; recovering the reserved blocks from the rendered ruleset — named allow rules are not recoverable and must be re-declared with `network firewall create-allow-rule` and then re-populated with `network firewall add`") return Parse(string(nft)) } diff --git a/internal/network/firewall/parse.go b/internal/network/firewall/parse.go index d18ba605..2b7517b5 100644 --- a/internal/network/firewall/parse.go +++ b/internal/network/firewall/parse.go @@ -20,8 +20,10 @@ import ( // 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. +// costs the most. Recovering management access is the goal; the allow rules are +// not recovered at all — neither their existence nor their membership — so they +// must be re-declared with `network firewall create-allow-rule` and then +// re-populated with `network firewall add`. // // 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. diff --git a/internal/network/firewall/rule.go b/internal/network/firewall/rule.go index 6c958171..732fc26a 100644 --- a/internal/network/firewall/rule.go +++ b/internal/network/firewall/rule.go @@ -92,6 +92,19 @@ 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" } +// incomplete reports whether an allow rule is declared but not yet populated +// enough to emit anything. It mirrors the template's own gating: a rule needs at +// least one source address, and either a destination port or an echo accept, +// before any line is rendered for it. Only meaningful for allow rules — the +// reserved blocks render fixed positions and an empty one is a deliberate +// "disabled", not an unfinished declaration. +func (r *Rule) incomplete() bool { + if IsReserved(r.Name) { + return false + } + return len(r.CIDRs) == 0 || (len(r.Ports) == 0 && !r.ICMPEcho) +} + // proto returns the rule's effective protocol, applying the tcp default. func (r *Rule) proto() Proto { if r.Proto == "" { @@ -146,28 +159,36 @@ func (r *Rule) Validate() error { // 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) + // + // Even proto=tcp is refused rather than accepted as a no-op: the value + // would change nothing, cannot be expressed in the config schema (Block + // carries no proto), and accepting it tells an operator their `set + // --proto` landed when the renderer ignored it. + if r.Proto != "" { + return errorx.IllegalArgument.New("%q does not take proto: management access is TCP by construction", 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) + // A reserved block may be empty, which is how an operator disables it + // without deleting it. It does carry a fixed shape though: the template + // renders all three reserved blocks as TCP and gives them no echo accept, + // so accepting either field here would silently ignore what was asked. + if r.Proto != "" { + return errorx.IllegalArgument.New("%q does not take proto: the in-cluster host-service ports are TCP by construction", RuleInCluster) } - 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) + if r.ICMPEcho { + return errorx.IllegalArgument.New("%q does not take icmp_echo: pod-to-host ICMP is not part of the host-service allowance", RuleInCluster) } + default: + // An allow rule is allowed to be incomplete. `create-allow-rule` declares + // a rule before it has any members, and the element verbs populate it in + // whatever order the operator runs them, so every intermediate state has + // to be representable. An incomplete rule renders no nft rule at all — + // the template gates each emission on the address and port sets being + // non-empty — so it grants nothing rather than granting too much. + // applyAndPersist warns about them; see Table.IncompleteAllowRules. } // Order the port list numerically here rather than in each mutator, so a rule diff --git a/internal/network/firewall/table.go b/internal/network/firewall/table.go index 62326aca..7b77559b 100644 --- a/internal/network/firewall/table.go +++ b/internal/network/firewall/table.go @@ -154,6 +154,20 @@ func (t *Table) UpsertAllow(r Rule) error { return nil } +// IncompleteAllowRules returns the names of allow rules that are declared but do +// not yet render anything, in table order. Declaring a rule before populating it +// is supported (`network firewall create-allow-rule`), so this is a warning the +// manager surfaces on apply rather than a validation failure. +func (t *Table) IncompleteAllowRules() []string { + var out []string + for i := range t.Allow { + if t.Allow[i].incomplete() { + out = append(out, t.Allow[i].Name) + } + } + return out +} + // 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