Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cmd/cli/commands/network/firewall/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 " +
Expand Down Expand Up @@ -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.
Expand Down
71 changes: 71 additions & 0 deletions cmd/cli/commands/network/firewall/create_allow_rule.go
Original file line number Diff line number Diff line change
@@ -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 <rule>` " +
"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 <cidr> --port <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")
}
15 changes: 9 additions & 6 deletions cmd/cli/commands/network/firewall/firewall.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
115 changes: 113 additions & 2 deletions cmd/cli/commands/network/firewall/firewall_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"}},
} {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
30 changes: 25 additions & 5 deletions cmd/cli/commands/network/firewall/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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") {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)")
Expand Down
Loading