diff --git a/cmd/cli/commands/network/firewall/add.go b/cmd/cli/commands/network/firewall/add.go index 0c692f8d..4d5856d8 100644 --- a/cmd/cli/commands/network/firewall/add.go +++ b/cmd/cli/commands/network/firewall/add.go @@ -27,7 +27,7 @@ var addCmd = &cobra.Command{ 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") + 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") 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 e4e2b39f..e6c59325 100644 --- a/cmd/cli/commands/network/firewall/create.go +++ b/cmd/cli/commands/network/firewall/create.go @@ -102,7 +102,7 @@ var createCmd = &cobra.Command{ 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 before any other rule (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().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") diff --git a/cmd/cli/commands/network/policy/delete.go b/cmd/cli/commands/network/policy/delete.go index 4f407e7c..824afe2f 100644 --- a/cmd/cli/commands/network/policy/delete.go +++ b/cmd/cli/commands/network/policy/delete.go @@ -13,7 +13,7 @@ var deleteCmd = &cobra.Command{ Long: "Remove a named policy from the `inet weaver-workload-policy` table: re-renders the chain without it, " + "applies the result to the live kernel, restores remaining policies' live membership, " + "removes the registry file, and atomically rewrites network-weaver-workload-policy.nft. " + - "If this is the last policy, an empty chain (policy drop, no rules) is applied and the " + + "If this is the last policy, the table is torn down entirely (live and on disk) and the " + "boot oneshot is left enabled.", RunE: func(cmd *cobra.Command, args []string) error { if err := newManager().Delete(cmd.Context(), flagName); err != nil { diff --git a/docs/dev/traffic-shaper.md b/docs/dev/traffic-shaper.md index f6cc90e7..69212e39 100644 --- a/docs/dev/traffic-shaper.md +++ b/docs/dev/traffic-shaper.md @@ -30,6 +30,37 @@ and its traffic categories and daemon reconciler are block-node-specific (below) but the table itself holds whatever `network policy` writes, block-node-related or not. +### Which plane sees which traffic + +The two tables register on different hooks, so they see **disjoint traffic**. That is why +neither carries a rule for the other's ports, and why no block-node service port appears +anywhere in the host firewall's rules or templates. + +| Traffic | Outcome | Decided by | +|---|---|---| +| External → node address, **non**-service port | Dropped | Host firewall `input` (`policy drop`) | +| External → node address, service port | Translated, then forwarded. Classified when the port is in a managed `_ports` set, otherwise forwarded unclassified | Workload policy `forward` | +| In-cluster → pod address directly, any port | Not constrained here — forwarded under `policy accept` | Cilium | +| Either endpoint in `@bn-restricted` | Dropped, both directions and both families | Workload policy `forward` | + +The first row misleads, because the mechanism is not the one the rule layout suggests. A packet +addressed to a port with no service behind it gets **no load-balancer translation** — only +exposed service ports have translation entries. Untranslated, its destination is still the +node's own address, so the routing decision delivers it locally, it arrives at `input`, and the +default drop catches it. It never becomes pod-bound traffic, so the classifier never sees it. + +Service traffic takes the opposite path: translation happens *before* the routing decision +(Cilium's eBPF at the tc ingress hook, or `prerouting` when kube-proxy performs the DNAT), so +the packet is forwarded and bypasses `input` entirely. That is why a block node serves traffic +on its service ports while the host firewall opens none of them. + +One consequence worth knowing, because it is silent: **an exposed port absent from the managed +`_ports` sets is forwarded and unshaped.** It matches no classification rule, carries no +`meta priority`, and lands in the HTB default class — `reserve-ingress` inbound, a 10% +guarantee. Since those sets are reconciled from statusz, a listener the block node does not +report gets no shaping rather than an error. For what each hook does and does not enforce, see +[Coexistence with the host's existing network stack](#coexistence-with-the-hosts-existing-network-stack). + ## How classify-and-shape fits together The policy plane and the shaper are decoupled and meet through exactly one thing: @@ -287,22 +318,36 @@ weaver tables simply register alongside the others. What that pattern does **not** give you is additive permissiveness. Within a base chain, `accept` ends evaluation *of that chain only* — the packet still traverses every other base chain registered on the same hook. A `drop` (or `reject`) is final for the packet across all -of them. Both weaver chains are `policy drop`, so anything they do not explicitly accept is -dropped; the `forward` chain also ends in an explicit `drop`, while `input` falls through to -its chain policy. That makes **weaver the binding filter on the node**: nothing Cilium or -kube-proxy accepts can rescue traffic weaver does not match. - -Concretely, the only broad escapes are: - -| Hook | Escapes | -|---|---| -| `input` (host firewall) | mgmt allowlist on the SSH port, `in_cluster_ports` from the pod CIDR, ICMP path-health, `ct state established,related` | -| `forward` (workload policy) | `ip saddr accept` (unclassified pod egress), `ct state established,related accept` | - -Everything else forwarded or delivered on 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. +of them. So on any hook where a weaver chain is `policy drop`, weaver is the binding filter: +nothing Cilium or kube-proxy accepts can rescue traffic weaver does not match. + +The two tables sit on opposite sides of that line, and the distinction matters: + +| Hook | Table | Chain policy | Role | +|---|---|---|---| +| `prerouting` (priority `raw`, −300) | host firewall | `accept` | Drops the operator block list ahead of conntrack. Covers the forward path too, so a blocked CIDR is blocked for pod-bound traffic as well. | +| `input` (priority `filter`, 0) | host firewall | `drop` | **Enforcing.** Anything not explicitly accepted is dropped. | +| `output` (priority `filter`, 0) | host firewall | `accept` | Block-list symmetry only — drops traffic *to* a blocked CIDR. Deliberately not an egress allowlist. | +| `forward` (priority `filter`, 0) | workload policy | `accept` | **Classifying.** Stamps `meta priority` for the HTB hierarchy; the only drops are the explicit `bn-restricted` quarantine rules. | + +The block list is spelled on three hooks because one is not enough. Dropping a peer inbound +does not stop the host from dialing it, and once the host initiates, the replies come back in +under `ct state established` — so an inbound-only block list does not block the connection at +all. The `input` copy is redundant with `prerouting` for anything arriving on a wire; it is +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 `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 +rests entirely on Cilium — which also means a host whose Cilium datapath is degraded or not yet +up has no weaver-side backstop for forwarded traffic. ### tc: why the HTB hierarchies do not fight Cilium diff --git a/docs/quickstart.md b/docs/quickstart.md index 6b2e4f36..19ac2223 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -222,7 +222,7 @@ sudo solo-provisioner block node install \ | `--load-balancer-enabled` | Inject MetalLB address-pool annotation into the block node service; set to `false` for environments without MetalLB (default: `true`). See [Block-node service exposure](./block-node-service-exposure.md) for how this interacts with `service.type` and the chart's split topology. | | `--firewall-enabled` | Apply the node-level host firewall (`inet weaver-host-firewall` table: SSH/mgmt allowlist, ICMP policy, in-cluster ports). Opt-in (default: `false`); set to `true` to have this tool manage the host firewall | | `--mgmt-cidrs` | Host firewall SSH/management allowlist CIDRs (IPv4 and/or IPv6 — each entry is routed to the matching `ipv4_addr`/`ipv6_addr` set). Empty skips the host firewall. | -| `--blocked-cidrs` | Host firewall operator-curated block list CIDRs (IPv4 and/or IPv6), dropped before any other rule including established connections. Distinct from the BN workload plane's `bn-restricted` set, which the traffic-shaper daemon manages automatically. | +| `--blocked-cidrs` | Host firewall operator-curated block list CIDRs (IPv4 and/or IPv6), dropped inbound, outbound, and forwarded — including established connections, and including pod-bound traffic. Distinct from the BN workload plane's `bn-restricted` set, which the traffic-shaper daemon manages automatically. | | `--ssh-port` | Host firewall SSH/management TCP port (default `22`) | | `--pod-cidr` | Host firewall pod CIDR for the in-cluster host-service ports rule (defaults to the cluster pod subnet). May be IPv4 and/or IPv6 (repeat or comma-separate for dual-stack). | | `--in-cluster-ports` | Host firewall in-cluster host-service ports (defaults to `6443,4244,7472,10250`) | @@ -841,7 +841,7 @@ Remove a policy's rules, set, and registry file, and re-render the `inet weaver- sudo solo-provisioner network policy delete --name bn-restricted ``` -`delete` re-renders the full chain without the removed policy, snapshots and restores remaining policies' live membership (so the destructive `delete table; add table` does not wipe their sets), removes the registry file, and atomically overwrites `network-weaver-workload-policy.nft`. If this is the last policy, an empty chain (`policy drop`, no rules) is applied; the boot oneshot stays enabled. +`delete` re-renders the full chain without the removed policy, snapshots and restores remaining policies' live membership (so the destructive `delete table; add table` does not wipe their sets), removes the registry file, and atomically overwrites `network-weaver-workload-policy.nft`. If this is the last policy, the table is torn down entirely (live and on disk); the boot oneshot stays enabled. | Flag | Description | Required | |----------|-----------------|----------| diff --git a/internal/network/firewall/firewall_test.go b/internal/network/firewall/firewall_test.go index a069bb53..ac110e95 100644 --- a/internal/network/firewall/firewall_test.go +++ b/internal/network/firewall/firewall_test.go @@ -151,6 +151,42 @@ func TestRender_SecurityInvariants(t *testing.T) { require.Less(t, blockedIdx, ctIdx, "blocked-CIDR drop must precede the conntrack fast-path") } +// TestRender_BlockListReachesEveryPath pins the block list's scope. A CIDR in +// @blocked_addrs means "blocked on this node": dropped ahead of conntrack on the +// way in (which also covers pod-bound forwarded traffic), and dropped as a +// destination on the way out. Inbound-only is not enough — blocking a peer 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. +func TestRender_BlockListReachesEveryPath(t *testing.T) { + doc, err := dualStackTable().Render() + require.NoError(t, err) + + // Priority must be below conntrack's -200, or the early drop buys nothing. + pre := chainBody(t, doc, "prerouting_blocklist") + require.Contains(t, pre, "type filter hook prerouting priority -300; policy accept;") + require.Contains(t, pre, "ip saddr @blocked_addrs drop") + require.Contains(t, pre, "ip6 saddr @blocked_addrs6 drop") + + // The output chain is block-list symmetry, NOT an egress allowlist: it must + // stay `policy accept` and must never grow a rule that gates normal traffic. + out := chainBody(t, doc, "output") + require.Contains(t, out, "type filter hook output priority 0; policy accept;") + require.Contains(t, out, "ip daddr @blocked_addrs drop") + require.Contains(t, out, "ip6 daddr @blocked_addrs6 drop") + for _, line := range strings.Split(out, "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") || strings.HasPrefix(line, "type filter") { + continue + } + require.Contains(t, line, "@blocked_addrs", "output chain must carry block-list rules only, got %q", line) + } + + // The input copy is redundant with prerouting for wire traffic but is kept + // deliberately, so the block list's position relative to the conntrack + // fast-path remains a property of the input chain itself. + require.Contains(t, chainBody(t, doc, "input"), "ip saddr @blocked_addrs drop") +} + // TestRender_FamilySplit pins the point of the per-family chains: a packet must // never be evaluated against a rule belonging to the other address family. func TestRender_FamilySplit(t *testing.T) { diff --git a/internal/network/firewall/table.go b/internal/network/firewall/table.go index 0fc637b4..b2ddcb26 100644 --- a/internal/network/firewall/table.go +++ b/internal/network/firewall/table.go @@ -35,6 +35,13 @@ type Table struct { // 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 + // forwarded traffic and runs ahead of conntrack), again on input, and as a + // 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: 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 63b09446..4bbf82aa 100644 --- a/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft +++ b/internal/network/firewall/testdata/network-weaver-host-firewall.golden.nft @@ -8,6 +8,19 @@ table inet weaver-host-firewall { set blocked_addrs6 { type ipv6_addr; flags interval; } set in_cluster_ports { type inet_service; 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 + # 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 @@ -21,6 +34,12 @@ table inet weaver-host-firewall { # 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 @@ -111,4 +130,19 @@ table inet weaver-host-firewall { # SSH / management access from the allowlist only. ip6 saddr @mgmt_addrs6 tcp dport 22 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/policy/manager.go b/internal/network/policy/manager.go index 1f813a84..cd82f6c8 100644 --- a/internal/network/policy/manager.go +++ b/internal/network/policy/manager.go @@ -697,8 +697,9 @@ func (m *Manager) showOne(ctx context.Context, p *Policy) (string, error) { // live membership (which the destructive re-render wipes), removes the // registry file, and atomically rewrites network-weaver-workload-policy.nft. // -// If this is the last policy, an empty chain (policy drop, no rules) is -// applied and the boot oneshot is left enabled. +// If this is the last policy, the table is torn down entirely — live and on +// disk — rather than left registered with nothing to classify. The boot oneshot +// is left enabled. func (m *Manager) Delete(ctx context.Context, name string) error { return m.withLock(func() error { policies, err := loadAll(m.registryDir) @@ -725,11 +726,10 @@ func (m *Manager) Delete(ctx context.Context, name string) error { if len(remaining) == 0 { // Deleting the last policy: tear the whole table down rather than - // render an empty chain. Render([]) emits `policy drop` with no - // accept rule for new connections — a blackhole that Apply() would - // load into the kernel and atomicWrite would persist for replay at - // boot. Remove the live table (if present) and the persisted file so - // an empty registry means "no inet weaver-workload-policy table", live or on disk. + // render a chain with no rules, which Apply() would load into the + // kernel and atomicWrite would persist for replay at boot. Remove the + // live table (if present) and the persisted file so an empty registry + // means "no inet weaver-workload-policy table", live or on disk. if exists, err := m.runner.Exists(ctx); err != nil { return errorx.Decorate(err, "failed to check the inet weaver-workload-policy table while removing the last policy") } else if exists { diff --git a/internal/network/policy/manager_ops_test.go b/internal/network/policy/manager_ops_test.go index 3291ec6e..194a3583 100644 --- a/internal/network/policy/manager_ops_test.go +++ b/internal/network/policy/manager_ops_test.go @@ -331,8 +331,8 @@ func TestDelete_LastPolicy_TearsDownTable(t *testing.T) { require.NoError(t, m.Delete(context.Background(), "bn-restricted")) - // Deleting the last policy tears the whole table down rather than applying - // an empty policy-drop chain that would blackhole all forwarded traffic. + // Deleting the last policy tears the whole table down rather than leaving an + // empty table registered on the forward hook. require.False(t, r.exists, "inet weaver-workload-policy table must be deleted after the last policy is removed") // The persisted file is removed so the boot oneshot replays nothing. require.NoFileExists(t, nftPath) diff --git a/internal/network/policy/policy_test.go b/internal/network/policy/policy_test.go index 91e1560a..4ac218c2 100644 --- a/internal/network/policy/policy_test.go +++ b/internal/network/policy/policy_test.go @@ -170,7 +170,67 @@ func TestRender_DualStackGolden(t *testing.T) { require.Contains(t, doc, "set bn-backfill6 { type ipv6_addr . inet_service; }") require.Contains(t, doc, "ip6 saddr @bn-restricted6 drop") require.Contains(t, doc, "ip6 daddr 2001:db8:c0de::/64 ip6 saddr @bn-publisher6") - require.Contains(t, doc, "ip6 saddr 2001:db8:c0de::/64 accept") +} + +// chainBody returns the body of the named chain, so an ordering assertion can be +// scoped to one chain. Document order stopped being evaluation order when the +// forward chain was split by family: the per-family chains are defined below the +// hooked chain but run before it falls through, via the nfproto dispatch. +func chainBody(t *testing.T, doc, name string) string { + t.Helper() + // An empty chain renders on one line as `chain { }`; matching only the + // multi-line form would fail here with "not found" for a chain that is present + // and legitimately empty. + if strings.Contains(doc, "\tchain "+name+" { }\n") { + return "" + } + header := "\tchain " + name + " {\n" + start := strings.Index(doc, header) + require.Greater(t, start, -1, "chain %s not found in rendered document", name) + rest := doc[start+len(header):] + end := strings.Index(rest, "\n\t}\n") + require.Greater(t, end, -1, "chain %s is not terminated", name) + return rest[:end] +} + +// TestRender_SingleStackEmitsAnEmptyChainForTheAbsentFamily covers the one input +// that produces a chain with no rules at all: stamp-only policies (no deny tier, +// the only tier a family without a pod CIDR renders) on a single-stack +// deployment. The chain must still exist, because the hooked chain's vmap jumps +// to it unconditionally and an unresolved jump fails the whole load. +func TestRender_SingleStackEmitsAnEmptyChainForTheAbsentFamily(t *testing.T) { + stampOnly := []*Policy{ + {Name: "bn-publisher", Action: ActionStamp, Stamp: "publisher", Direction: DirectionIngress, Ports: []string{"40840"}, CreatedAt: fixedTime()}, + } + doc, err := Render(stampOnly, "10.4.0.0/24") + require.NoError(t, err) + + require.Contains(t, doc, "jump "+chainV6, "the dispatch always jumps to both families") + require.Contains(t, doc, "\tchain "+chainV6+" { }\n", "the absent family's chain must still be declared") + require.Contains(t, chainBody(t, doc, chainV4), "ip daddr 10.4.0.0/24 ip saddr @bn-publisher") +} + +// TestRender_AbsentFamilyOmitsTheReplyRestore pins the reply-restore tier to the +// same pod-CIDR gate as the stamp tiers. The ct mark it matches is only written +// by a stamp rule, which renders solely for a family that has a pod CIDR, so +// emitting the restore for the absent family would add a rule no packet can ever +// match. +func TestRender_AbsentFamilyOmitsTheReplyRestore(t *testing.T) { + replyStamp := []*Policy{ + {Name: "bn-backfill", Action: ActionStamp, Stamp: "reserve-egress", ReplyStamp: "backfill-response", Direction: DirectionEgress, CreatedAt: fixedTime()}, + {Name: "bn-restricted", Action: ActionDeny, CreatedAt: fixedTime()}, + } + + single, err := Render(replyStamp, "10.4.0.0/24") + require.NoError(t, err) + require.Contains(t, chainBody(t, single, chainV4), "ct direction reply") + require.NotContains(t, chainBody(t, single, chainV6), "ct direction reply", + "a family with no pod CIDR carries the deny tier only") + + dual, err := Render(replyStamp, "10.4.0.0/24", "2001:db8:c0de::/64") + require.NoError(t, err) + require.Contains(t, chainBody(t, dual, chainV6), "ct direction reply", + "both families keep the restore once both have a pod CIDR") } func TestRender_DeterministicRegardlessOfInputOrder(t *testing.T) { @@ -188,25 +248,90 @@ func TestRender_DeterministicRegardlessOfInputOrder(t *testing.T) { require.Equal(t, want, got, "Render must sort internally, not rely on the caller's order") } +// TestRender_TierOrderInvariants pins tier order *within* a family chain. Every +// assertion is scoped to one chain body: a whole-document strings.Index compares +// positions across chains that never evaluate the same packet, which would make +// the assertion meaningless. func TestRender_TierOrderInvariants(t *testing.T) { - doc, err := Render(sampleBNPolicies(), "10.4.0.0/24") + doc, err := Render(sampleBNPolicies(), "10.4.0.0/24", "2001:db8:c0de::/64") + require.NoError(t, err) + + for _, tc := range []struct { + chain, deny, specific, fallthr string + }{ + {chainV4, "ip saddr @bn-restricted drop", "@bn-partner-out ", "@bn-public-out_ports"}, + {chainV6, "ip6 saddr @bn-restricted6 drop", "@bn-partner-out6 ", "@bn-public-out_ports"}, + } { + t.Run(tc.chain, func(t *testing.T) { + body := chainBody(t, doc, tc.chain) + + // The quarantine drops must lead the chain: every rule below them + // ends in `accept`, so a deny that sorted lower would let a + // restricted peer's traffic be stamped and accepted instead. + denyIdx := strings.Index(body, tc.deny) + restoreIdx := strings.Index(body, "ct direction reply ct mark 0x20") + specificIdx := strings.Index(body, tc.specific) + require.Positive(t, denyIdx) + require.Positive(t, restoreIdx) + require.Positive(t, specificIdx) + require.Less(t, denyIdx, restoreIdx, "deny must precede the reply restore") + require.Less(t, restoreIdx, specificIdx, "reply restore must precede classification") + + // Specific (partner) must precede the fallthrough (public) so + // partner-bound replies hit 1:40 and everyone else 1:50. + require.Less(t, specificIdx, strings.Index(body, tc.fallthr)) + }) + } +} + +// TestRender_FamilySplit pins the structural change: a minimal hooked chain that +// only dispatches, per-family chains that carry no rule from the other family, +// and no conntrack-state rule or terminal drop anywhere. +func TestRender_FamilySplit(t *testing.T) { + doc, err := Render(sampleBNPolicies(), "10.4.0.0/24", "2001:db8:c0de::/64") require.NoError(t, err) - // Quarantine drops and the reply restore must both precede `ct state - // established,related accept`, otherwise open connections survive a - // quarantine and reply packets are misclassified. - estIdx := strings.Index(doc, "ct state established,related accept") - require.Positive(t, estIdx) - require.Less(t, strings.Index(doc, "ip saddr @bn-restricted drop"), estIdx, "deny must precede est,rel accept") - require.Less(t, strings.Index(doc, "ct direction reply ct mark 0x20"), estIdx, "reply restore must precede est,rel accept") + base := chainBody(t, doc, chainBase) + require.Contains(t, base, "type filter hook forward priority 0; policy accept;") + require.Contains(t, base, "meta nfproto vmap { ipv4 : jump forward_ipv4, ipv6 : jump forward_ipv6 }") + + // The hooked chain must carry nothing but the policy line and the dispatch — + // a rule that creeps in here is evaluated for every forwarded packet. + var baseRules []string + for _, line := range strings.Split(base, "\n") { + line = strings.TrimSpace(line) + if line != "" && !strings.HasPrefix(line, "#") { + baseRules = append(baseRules, line) + } + } + require.Len(t, baseRules, 2, "hooked chain must hold only the policy line and the dispatch, got %v", baseRules) + + // No rule may match a family it cannot belong to. + for _, line := range strings.Split(chainBody(t, doc, chainV4), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + require.NotContains(t, line, "ip6 ", "IPv6 match in the IPv4 chain: %s", line) + } + for _, line := range strings.Split(chainBody(t, doc, chainV6), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + require.False(t, strings.HasPrefix(line, "ip "), "IPv4 match in the IPv6 chain: %s", line) + } - // Specific (partner) must precede the fallthrough (public) so partner-bound - // replies hit 1:40 and everyone else 1:50. - require.Less(t, strings.Index(doc, "@bn-partner-out"), strings.Index(doc, "@bn-public-out_ports")) + // The reply restore matches conntrack only, so it belongs in both chains — + // it cannot be hoisted into the hooked chain above the dispatch, where its + // `accept` would let a quarantined peer's replies escape the deny tier. + require.Contains(t, chainBody(t, doc, chainV4), "ct direction reply ct mark 0x20") + require.Contains(t, chainBody(t, doc, chainV6), "ct direction reply ct mark 0x20") - // Chain must default-drop and end with a trailing drop. - require.Contains(t, doc, "policy drop;") - require.True(t, strings.HasSuffix(strings.TrimSpace(doc), "}\n}") || strings.Contains(doc, "\n\t\tdrop\n")) + // The chain classifies; it no longer enforces. + require.NotContains(t, doc, "policy drop;") + require.NotContains(t, doc, "ct state") + require.NotContains(t, doc, "\n\t\tdrop\n") } func TestRender_WorkedExamples(t *testing.T) { diff --git a/internal/network/policy/render.go b/internal/network/policy/render.go index 24bba410..cc5094e9 100644 --- a/internal/network/policy/render.go +++ b/internal/network/policy/render.go @@ -22,16 +22,20 @@ import ( // (`_ports` for a ManagedPorts policy) is likewise declared empty here and // filled from statusz at runtime, exactly like the CIDR membership set. // -// Rule position is determined by action type and match specificity, never by -// creation order: +// The hooked `forward` chain holds no rules of its own beyond a `meta nfproto` +// dispatch into forward_ipv4 / forward_ipv6, so a packet never evaluates rules +// belonging to the other address family. Its policy is `accept`: this table +// classifies traffic for the HTB hierarchy, it does not enforce workload +// isolation (Cilium does). Traffic no rule matches therefore falls through +// carrying no `meta priority` and lands in the HTB default class. +// +// Rule position *within a family chain* is determined by action type and match +// specificity, never by creation order: // // 1. deny drops (both directions) // 2. asymmetric reply-stamp restore // 3. stamp classification — specific (has an IP-set match) // 4. stamp classification — fallthrough (--from-entity world) -// 5. unclassified pod egress (structural; only when podCIDR is set) -// 6. ct state established,related accept (structural) -// 7. drop (structural) func Render(policies []*Policy, podCIDRs ...string) (string, error) { podV4, podV6 := partitionPodCIDRs(podCIDRs) if podV4 == "" && podV6 == "" && needsPodCIDR(policies) { @@ -50,7 +54,11 @@ func Render(policies []*Policy, podCIDRs ...string) (string, error) { if err != nil { return "", err } - chainLines, err := renderChain(policies, podV4, podV6) + v4Lines, err := renderFamilyChain(policies, familyV4(podV4)) + if err != nil { + return "", err + } + v6Lines, err := renderFamilyChain(policies, familyV6(podV6)) if err != nil { return "", err } @@ -67,13 +75,79 @@ func Render(policies []*Policy, podCIDRs ...string) (string, error) { b.WriteString(strings.Join(setLines, "\n")) b.WriteString("\n\n") } - b.WriteString("\tchain forward {\n") - b.WriteString(strings.Join(chainLines, "\n")) - b.WriteString("\n\t}\n") + b.WriteString(strings.Join(baseChainLines(), "\n")) + b.WriteString("\n\n") + writeChain(&b, chainV4, v4Lines) + b.WriteString("\n") + writeChain(&b, chainV6, v6Lines) b.WriteString("}\n") return b.String(), nil } +// writeChain emits one regular chain. A chain with no rules renders as a bare +// `chain { }` — reachable on a single-stack deployment with no deny +// policies, since the deny tier is the only one a family without a pod CIDR +// renders at all. +func writeChain(b *strings.Builder, name string, lines []string) { + if len(lines) == 0 { + b.WriteString("\tchain " + name + " { }\n") + return + } + b.WriteString("\tchain " + name + " {\n") + b.WriteString(strings.Join(lines, "\n")) + b.WriteString("\n\t}\n") +} + +// Chain names. The hooked chain keeps its original name so `nft list chain inet +// weaver-workload-policy forward` and the operator docs still resolve. +const ( + chainBase = "forward" + chainV4 = "forward_ipv4" + chainV6 = "forward_ipv6" +) + +// baseChainLines renders the hooked chain: a policy declaration and the family +// dispatch, nothing else. +// +// `meta nfproto` rather than `meta protocol`: in an `inet` table nfproto reads +// the netfilter protocol family straight off the hook, whereas `meta protocol` +// depends on the skb's ethertype being populated — not guaranteed for locally +// generated traffic or non-Ethernet interfaces. +func baseChainLines() []string { + return []string{ + "\t# The hooked chain carries no rules of its own — it only dispatches into", + "\t# the per-family chains below, so an IPv4 packet never evaluates an IPv6", + "\t# rule and vice versa.", + "\t#", + "\t# Policy is `accept` because this table classifies traffic for the HTB", + "\t# hierarchy rather than enforcing workload isolation, which is Cilium's", + "\t# job. A packet no rule matches falls through carrying no `meta priority`", + "\t# and therefore lands in the HTB default class.", + "\tchain " + chainBase + " {", + "\t\ttype filter hook forward priority 0; policy accept;", + "\t\tmeta nfproto vmap { ipv4 : jump " + chainV4 + ", ipv6 : jump " + chainV6 + " }", + "\t}", + } +} + +// family carries the per-address-family spelling differences between the two +// generated chains: the nft L3 match keyword, that family's pod CIDR (empty on a +// single-stack deployment's absent family, which suppresses its stamp rules), +// and the selector for a policy's membership set. +type family struct { + proto string + podCIDR string + setName func(string) string +} + +func familyV4(podCIDR string) family { + return family{proto: "ip", podCIDR: podCIDR, setName: func(name string) string { return name }} +} + +func familyV6(podCIDR string) family { + return family{proto: "ip6", podCIDR: podCIDR, setName: V6SetName} +} + // needsPodCIDR reports whether any policy in the set is a --stamp policy. // POD_CIDR is only ever read by renderStampRule; a deny-only chain never // references it, so it shouldn't be required to render one. @@ -164,47 +238,54 @@ func renderSetDecls(policies []*Policy) ([]string, error) { return lines, nil } -// renderChain builds the forward chain body lines (indented two tabs), grouped -// into the seven tiers above. -func renderChain(policies []*Policy, podV4, podV6 string) ([]string, error) { - lines := []string{ - "\t\ttype filter hook forward priority 0; policy drop;", - "\t\tct state invalid drop", - } +// renderFamilyChain builds one address family's chain body (indented two tabs), +// grouped into the four tiers above. Both chains are rendered from the same +// policy set; f decides how each rule is spelled and which membership set it +// references, so no rule can match a family it cannot belong to. +func renderFamilyChain(policies []*Policy, f family) ([]string, error) { + var lines []string - // Tier 1: quarantine drops, both families. + // Tier 1: quarantine drops, both directions. var deny []string for _, p := range policies { if p.Action == ActionDeny { deny = append(deny, - fmt.Sprintf("\t\tip saddr @%s drop", p.Name), - fmt.Sprintf("\t\tip daddr @%s drop", p.Name), - fmt.Sprintf("\t\tip6 saddr @%s drop", V6SetName(p.Name)), - fmt.Sprintf("\t\tip6 daddr @%s drop", V6SetName(p.Name))) + fmt.Sprintf("\t\t%s saddr @%s drop", f.proto, f.setName(p.Name)), + fmt.Sprintf("\t\t%s daddr @%s drop", f.proto, f.setName(p.Name))) } } if len(deny) > 0 { - lines = append(lines, "", - "\t\t# Quarantine (deny), both directions. Runs before est,rel accept", - "\t\t# so already-open connections are also killed.") + lines = appendSection(lines, + "\t\t# Quarantine (deny), both directions. Runs ahead of the classification", + "\t\t# accepts below so a quarantined peer's traffic is dropped rather than", + "\t\t# stamped. There is no conntrack fast-path in this chain, so packets on", + "\t\t# already-open connections are evaluated against these drops too.") lines = append(lines, deny...) } - // Tier 2: asymmetric reply-stamp restore. + // Tier 2: asymmetric reply-stamp restore. Gated on this family's pod CIDR for + // the same reason as the stamp tiers below: the ct mark it matches is only ever + // written by renderStampRule, which is itself pod-CIDR-gated, so in a family + // with no pod CIDR no packet can carry the mark and the rule is unreachable. var restore []string - for _, p := range policies { - if p.Action == ActionStamp && p.ReplyStamp != "" { - rule, err := renderReplyRestoreRule(p) - if err != nil { - return nil, err + if f.podCIDR != "" { + for _, p := range policies { + if p.Action == ActionStamp && p.ReplyStamp != "" { + rule, err := renderReplyRestoreRule(p) + if err != nil { + return nil, err + } + restore = append(restore, rule) } - restore = append(restore, rule) } } if len(restore) > 0 { - lines = append(lines, "", - "\t\t# Asymmetric reply restore. Must precede est,rel accept so every", - "\t\t# reply packet is reclassified, not just the SYN.") + lines = appendSection(lines, + "\t\t# Asymmetric reply restore. Matches conntrack only, so it is spelled", + "\t\t# identically in both family chains that have a pod CIDR. Must precede", + "\t\t# the classification tiers: on the reply the addresses are reversed, so a", + "\t\t# broad fallthrough rule below would otherwise claim the packet and stamp", + "\t\t# it with the forward class instead of the reply class.") lines = append(lines, restore...) } @@ -230,53 +311,45 @@ func renderChain(policies []*Policy, podV4, podV6 string) ([]string, error) { var specific, fallthr []string for _, p := range specificPolicies { - rules, err := renderStampRule(p, podV4, podV6) + rule, err := renderStampRule(p, f) if err != nil { return nil, err } - specific = append(specific, rules...) + if rule != "" { + specific = append(specific, rule) + } } for _, p := range fallthrPolicies { - rules, err := renderStampRule(p, podV4, podV6) + rule, err := renderStampRule(p, f) if err != nil { return nil, err } - fallthr = append(fallthr, rules...) + if rule != "" { + fallthr = append(fallthr, rule) + } } if len(specific) > 0 { - lines = append(lines, "", "\t\t# Classification — specific matches.") + lines = appendSection(lines, "\t\t# Classification — specific matches.") lines = append(lines, specific...) } if len(fallthr) > 0 { - lines = append(lines, "", "\t\t# Classification — fallthrough (any source/dest).") + lines = appendSection(lines, "\t\t# Classification — fallthrough (any source/dest).") lines = append(lines, fallthr...) } - // Tier 5: unclassified pod egress. Only rendered when a pod CIDR is known - // (the canonical BN install set always supplies one; a deny-only chain - // built without one just skips this tier rather than erroring, since - // nothing above requires it either). Deliberately no meta priority: this - // is a default-allow escape hatch for outbound traffic this registry - // doesn't otherwise classify (e.g. the chart's Maven-based plugin - // resolution), so it falls to the HTB default class instead of one of - // the classified priority bands. - if podV4 != "" || podV6 != "" { - lines = append(lines, "", - "\t\t# Unclassified pod egress (no meta priority; HTB default class).") - if podV4 != "" { - lines = append(lines, fmt.Sprintf("\t\tip saddr %s accept", podV4)) - } - if podV6 != "" { - lines = append(lines, fmt.Sprintf("\t\tip6 saddr %s accept", podV6)) - } - } - - lines = append(lines, "", - "\t\tct state established,related accept", - "\t\tdrop") return lines, nil } +// appendSection appends a tier's comment block, separated from any preceding +// tier by a blank line. A chain whose first tier is empty (a stamp-only registry +// has no deny rules) therefore does not open with a stray blank line. +func appendSection(lines []string, comment ...string) []string { + if len(lines) > 0 { + lines = append(lines, "") + } + return append(lines, comment...) +} + // orderByGroupThenCreatedAt returns policies reordered so that members of the // same (Direction, Ports) group (see groupKey) are contiguous and sorted by // CreatedAt ascending, while groups themselves keep the relative order of @@ -322,76 +395,53 @@ func renderReplyRestoreRule(p *Policy) (string, error) { hex(reply.Mark), hex(reply.Priority)), nil } -// renderStampRule renders a stamp policy's classification rule(s) for its -// direction, honoring --from-entity world (no IP-set clause) and --reply-stamp -// (compound-key egress forward rule with a ct mark write). It emits one rule per -// pod-CIDR family supplied: an `ip` rule (against @) when podV4 is set and -// an `ip6` rule (against @6) when podV6 is set, so a dual-stack deployment -// classifies both families and a single-stack one renders only its family. -func renderStampRule(p *Policy, podV4, podV6 string) ([]string, error) { +// renderStampRule renders one address family's classification rule for a stamp +// policy, honoring --from-entity world (no IP-set clause) and --reply-stamp +// (compound-key egress forward rule with a ct mark write). It returns "" when +// this family has no pod CIDR, so a single-stack deployment renders only its own +// family's rules while the other family's chain carries the deny tier alone. +func renderStampRule(p *Policy, f family) (string, error) { + if f.podCIDR == "" { + return "", nil + } fwd, err := lookupClass(p.Stamp) if err != nil { - return nil, err + return "", err } - var rules []string - if p.isCompoundSet() { // --reply-stamp forward rule: egress, compound ip:port destination key, - // ct mark write for the reply restore to read back. One per pod family. + // ct mark write for the reply restore to read back. reply, err := lookupClass(p.ReplyStamp) if err != nil { - return nil, err - } - if podV4 != "" { - rules = append(rules, fmt.Sprintf("\t\tip saddr %s ip daddr . tcp dport @%s ct mark set %s meta priority set %s accept", - podV4, p.Name, hex(reply.Mark), hex(fwd.Priority))) - } - if podV6 != "" { - rules = append(rules, fmt.Sprintf("\t\tip6 saddr %s ip6 daddr . tcp dport @%s ct mark set %s meta priority set %s accept", - podV6, V6SetName(p.Name), hex(reply.Mark), hex(fwd.Priority))) + return "", err } - return rules, nil + return fmt.Sprintf("\t\t%s saddr %s %s daddr . tcp dport @%s ct mark set %s meta priority set %s accept", + f.proto, f.podCIDR, f.proto, f.setName(p.Name), hex(reply.Mark), hex(fwd.Priority)), nil } - if podV4 != "" { - rule, err := renderPlainStampRule(p, podV4, "ip", p.Name, fwd) - if err != nil { - return nil, err - } - rules = append(rules, rule) - } - if podV6 != "" { - rule, err := renderPlainStampRule(p, podV6, "ip6", V6SetName(p.Name), fwd) - if err != nil { - return nil, err - } - rules = append(rules, rule) - } - return rules, nil + return renderPlainStampRule(p, f, fwd) } // renderPlainStampRule renders one address family's plain stamp classification -// rule. proto is the nft L3 keyword ("ip" or "ip6"); setName is that family's -// membership set (@ or @6). The listener-ports set is a shared -// family-agnostic inet_service set, so it is referenced by the same @_ports -// name in both families. -func renderPlainStampRule(p *Policy, podCIDR, proto, setName string, fwd class) (string, error) { +// rule. The listener-ports set is a shared family-agnostic inet_service set, so +// it is referenced by the same @_ports name in both families. +func renderPlainStampRule(p *Policy, f family, fwd class) (string, error) { var b strings.Builder b.WriteString("\t\t") switch p.Direction { case DirectionIngress: - b.WriteString(proto + " daddr " + podCIDR) + b.WriteString(f.proto + " daddr " + f.podCIDR) if p.hasCIDRSet() { - b.WriteString(" " + proto + " saddr @" + setName) + b.WriteString(" " + f.proto + " saddr @" + f.setName(p.Name)) } if p.hasPortsSet() { b.WriteString(" tcp dport @" + PortsSetName(p.Name)) } case DirectionEgress: - b.WriteString(proto + " saddr " + podCIDR) + b.WriteString(f.proto + " saddr " + f.podCIDR) if p.hasCIDRSet() { - b.WriteString(" " + proto + " daddr @" + setName) + b.WriteString(" " + f.proto + " daddr @" + f.setName(p.Name)) } if p.hasPortsSet() { b.WriteString(" tcp sport @" + PortsSetName(p.Name)) diff --git a/internal/network/policy/render_weaver.go b/internal/network/policy/render_weaver.go index cb3b7ac8..5422622d 100644 --- a/internal/network/policy/render_weaver.go +++ b/internal/network/policy/render_weaver.go @@ -25,12 +25,14 @@ func RenderWeaverNft(registryDir, weaverNftPath string, podCIDRs ...string) erro return err } if len(policies) == 0 { - // An empty registry means no policies to enforce. A rendered empty chain - // would be `policy drop` with no accept rule for new connections — - // blackholing all forwarded traffic (pod startup, image pulls, inter-pod - // DNS). Remove any stale persisted file so the boot oneshot's `test -e` - // guard skips it and never replays a harmful or out-of-date inet weaver-workload-policy - // table. "Empty registry" thus means "no file", not "an empty table". + // An empty registry means there is nothing to classify. Remove any stale + // persisted file so the boot oneshot's `test -e` guard skips it and never + // replays an out-of-date inet weaver-workload-policy table. "Empty + // registry" thus means "no file", not "an empty table". + // + // The chain is `policy accept`, so an empty table would be inert rather + // than harmful — this is hygiene (never replay stale classification), not + // the blackhole guard it was when the chain still defaulted to drop. if err := os.Remove(weaverNftPath); err != nil && !os.IsNotExist(err) { return errorx.ExternalError.Wrap(err, "failed to remove stale %s for an empty policy registry", weaverNftPath) } diff --git a/internal/network/policy/render_weaver_test.go b/internal/network/policy/render_weaver_test.go index 886c49ee..a0f0965d 100644 --- a/internal/network/policy/render_weaver_test.go +++ b/internal/network/policy/render_weaver_test.go @@ -18,9 +18,9 @@ func TestRenderWeaverNft_EmptyRegistry(t *testing.T) { dir := t.TempDir() out := filepath.Join(dir, "network-weaver-workload-policy.nft") - // Missing registry dir is treated as an empty registry: must be a no-op. - // Writing a forward chain with policy drop for zero policies would silently - // block all new forwarded traffic (pod startup, image pulls, DNS). + // Missing registry dir is treated as an empty registry: must be a no-op, so + // the boot oneshot has nothing to replay rather than a table that classifies + // nothing. require.NoError(t, RenderWeaverNft(filepath.Join(dir, "policies"), out, "")) _, err := os.Stat(out) diff --git a/internal/network/policy/testdata/network-weaver-workload-policy-dualstack.golden.nft b/internal/network/policy/testdata/network-weaver-workload-policy-dualstack.golden.nft index 72b2e08e..ff15b7d6 100644 --- a/internal/network/policy/testdata/network-weaver-workload-policy-dualstack.golden.nft +++ b/internal/network/policy/testdata/network-weaver-workload-policy-dualstack.golden.nft @@ -15,40 +15,66 @@ table inet weaver-workload-policy { set bn-restricted6 { type ipv6_addr; flags interval; } set bn-subscriber-in_ports { type inet_service; elements = { 40980, 40981 }; } + # The hooked chain carries no rules of its own — it only dispatches into + # the per-family chains below, so an IPv4 packet never evaluates an IPv6 + # rule and vice versa. + # + # Policy is `accept` because this table classifies traffic for the HTB + # hierarchy rather than enforcing workload isolation, which is Cilium's + # job. A packet no rule matches falls through carrying no `meta priority` + # and therefore lands in the HTB default class. chain forward { - type filter hook forward priority 0; policy drop; - ct state invalid drop + type filter hook forward priority 0; policy accept; + meta nfproto vmap { ipv4 : jump forward_ipv4, ipv6 : jump forward_ipv6 } + } - # Quarantine (deny), both directions. Runs before est,rel accept - # so already-open connections are also killed. + chain forward_ipv4 { + # Quarantine (deny), both directions. Runs ahead of the classification + # accepts below so a quarantined peer's traffic is dropped rather than + # stamped. There is no conntrack fast-path in this chain, so packets on + # already-open connections are evaluated against these drops too. ip saddr @bn-restricted drop ip daddr @bn-restricted drop - ip6 saddr @bn-restricted6 drop - ip6 daddr @bn-restricted6 drop - # Asymmetric reply restore. Must precede est,rel accept so every - # reply packet is reclassified, not just the SYN. + # Asymmetric reply restore. Matches conntrack only, so it is spelled + # identically in both family chains that have a pod CIDR. Must precede + # the classification tiers: on the reply the addresses are reversed, so a + # broad fallthrough rule below would otherwise claim the packet and stamp + # it with the forward class instead of the reply class. ct direction reply ct mark 0x20 meta priority set 0x10020 accept # Classification — specific matches. ip saddr 10.4.0.0/24 ip daddr . tcp dport @bn-backfill ct mark set 0x20 meta priority set 0x10060 accept - ip6 saddr 2001:db8:c0de::/64 ip6 daddr . tcp dport @bn-backfill6 ct mark set 0x20 meta priority set 0x10060 accept ip saddr 10.4.0.0/24 ip daddr @bn-partner-out tcp sport @bn-partner-out_ports meta priority set 0x10040 accept - ip6 saddr 2001:db8:c0de::/64 ip6 daddr @bn-partner-out6 tcp sport @bn-partner-out_ports meta priority set 0x10040 accept ip daddr 10.4.0.0/24 ip saddr @bn-publisher tcp dport @bn-publisher_ports meta priority set 0x10010 accept - ip6 daddr 2001:db8:c0de::/64 ip6 saddr @bn-publisher6 tcp dport @bn-publisher_ports meta priority set 0x10010 accept # Classification — fallthrough (any source/dest). ip saddr 10.4.0.0/24 tcp sport @bn-public-out_ports meta priority set 0x10050 accept - ip6 saddr 2001:db8:c0de::/64 tcp sport @bn-public-out_ports meta priority set 0x10050 accept ip daddr 10.4.0.0/24 tcp dport @bn-subscriber-in_ports meta priority set 0x10030 accept - ip6 daddr 2001:db8:c0de::/64 tcp dport @bn-subscriber-in_ports meta priority set 0x10030 accept + } - # Unclassified pod egress (no meta priority; HTB default class). - ip saddr 10.4.0.0/24 accept - ip6 saddr 2001:db8:c0de::/64 accept + chain forward_ipv6 { + # Quarantine (deny), both directions. Runs ahead of the classification + # accepts below so a quarantined peer's traffic is dropped rather than + # stamped. There is no conntrack fast-path in this chain, so packets on + # already-open connections are evaluated against these drops too. + ip6 saddr @bn-restricted6 drop + ip6 daddr @bn-restricted6 drop - ct state established,related accept - drop + # Asymmetric reply restore. Matches conntrack only, so it is spelled + # identically in both family chains that have a pod CIDR. Must precede + # the classification tiers: on the reply the addresses are reversed, so a + # broad fallthrough rule below would otherwise claim the packet and stamp + # it with the forward class instead of the reply class. + ct direction reply ct mark 0x20 meta priority set 0x10020 accept + + # Classification — specific matches. + ip6 saddr 2001:db8:c0de::/64 ip6 daddr . tcp dport @bn-backfill6 ct mark set 0x20 meta priority set 0x10060 accept + ip6 saddr 2001:db8:c0de::/64 ip6 daddr @bn-partner-out6 tcp sport @bn-partner-out_ports meta priority set 0x10040 accept + ip6 daddr 2001:db8:c0de::/64 ip6 saddr @bn-publisher6 tcp dport @bn-publisher_ports meta priority set 0x10010 accept + + # Classification — fallthrough (any source/dest). + ip6 saddr 2001:db8:c0de::/64 tcp sport @bn-public-out_ports meta priority set 0x10050 accept + ip6 daddr 2001:db8:c0de::/64 tcp dport @bn-subscriber-in_ports meta priority set 0x10030 accept } } diff --git a/internal/network/policy/testdata/network-weaver-workload-policy.golden.nft b/internal/network/policy/testdata/network-weaver-workload-policy.golden.nft index 5485a34a..87bad0c9 100644 --- a/internal/network/policy/testdata/network-weaver-workload-policy.golden.nft +++ b/internal/network/policy/testdata/network-weaver-workload-policy.golden.nft @@ -15,19 +15,32 @@ table inet weaver-workload-policy { set bn-restricted6 { type ipv6_addr; flags interval; } set bn-subscriber-in_ports { type inet_service; elements = { 40980, 40981 }; } + # The hooked chain carries no rules of its own — it only dispatches into + # the per-family chains below, so an IPv4 packet never evaluates an IPv6 + # rule and vice versa. + # + # Policy is `accept` because this table classifies traffic for the HTB + # hierarchy rather than enforcing workload isolation, which is Cilium's + # job. A packet no rule matches falls through carrying no `meta priority` + # and therefore lands in the HTB default class. chain forward { - type filter hook forward priority 0; policy drop; - ct state invalid drop + type filter hook forward priority 0; policy accept; + meta nfproto vmap { ipv4 : jump forward_ipv4, ipv6 : jump forward_ipv6 } + } - # Quarantine (deny), both directions. Runs before est,rel accept - # so already-open connections are also killed. + chain forward_ipv4 { + # Quarantine (deny), both directions. Runs ahead of the classification + # accepts below so a quarantined peer's traffic is dropped rather than + # stamped. There is no conntrack fast-path in this chain, so packets on + # already-open connections are evaluated against these drops too. ip saddr @bn-restricted drop ip daddr @bn-restricted drop - ip6 saddr @bn-restricted6 drop - ip6 daddr @bn-restricted6 drop - # Asymmetric reply restore. Must precede est,rel accept so every - # reply packet is reclassified, not just the SYN. + # Asymmetric reply restore. Matches conntrack only, so it is spelled + # identically in both family chains that have a pod CIDR. Must precede + # the classification tiers: on the reply the addresses are reversed, so a + # broad fallthrough rule below would otherwise claim the packet and stamp + # it with the forward class instead of the reply class. ct direction reply ct mark 0x20 meta priority set 0x10020 accept # Classification — specific matches. @@ -38,11 +51,14 @@ table inet weaver-workload-policy { # Classification — fallthrough (any source/dest). ip saddr 10.4.0.0/24 tcp sport @bn-public-out_ports meta priority set 0x10050 accept ip daddr 10.4.0.0/24 tcp dport @bn-subscriber-in_ports meta priority set 0x10030 accept + } - # Unclassified pod egress (no meta priority; HTB default class). - ip saddr 10.4.0.0/24 accept - - ct state established,related accept - drop + chain forward_ipv6 { + # Quarantine (deny), both directions. Runs ahead of the classification + # accepts below so a quarantined peer's traffic is dropped rather than + # stamped. There is no conntrack fast-path in this chain, so packets on + # already-open connections are evaluated against these drops too. + ip6 saddr @bn-restricted6 drop + ip6 daddr @bn-restricted6 drop } } 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 19450a87..e9aa9c3c 100644 --- a/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl +++ b/internal/templates/files/network/network-weaver-host-firewall.nft.tmpl @@ -8,6 +8,19 @@ table inet weaver-host-firewall { 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}} } + # 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 @@ -21,6 +34,12 @@ table inet weaver-host-firewall { # 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 @@ -118,4 +137,19 @@ table inet weaver-host-firewall { ip6 saddr {{.PodCIDR6}} tcp dport @in_cluster_ports accept {{- end}} } + + # 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/workflows/steps/step_network_nft_weaver.go b/internal/workflows/steps/step_network_nft_weaver.go index 7c314088..2d037267 100644 --- a/internal/workflows/steps/step_network_nft_weaver.go +++ b/internal/workflows/steps/step_network_nft_weaver.go @@ -48,8 +48,7 @@ func NftWeaverPersist() *automa.StepBuilder { } // Reconcile the persisted file to the registry. On an empty registry // this removes any stale network-weaver-workload-policy.nft so the boot oneshot never - // replays a harmful/old inet weaver-workload-policy table (an empty chain renders as - // policy drop with no accept for new connections). + // replays an out-of-date inet weaver-workload-policy table. if err := policy.RenderWeaverNft(policy.RegistryDir, policy.WeaverNftPath, ""); err != nil { return automa.FailureReport(stp, automa.WithError( errorx.Decorate(err, "failed to render %s", policy.WeaverNftPath). diff --git a/internal/workflows/steps/step_network_policy.go b/internal/workflows/steps/step_network_policy.go index ae1668c7..47e32287 100644 --- a/internal/workflows/steps/step_network_policy.go +++ b/internal/workflows/steps/step_network_policy.go @@ -138,7 +138,7 @@ func (c canonicalPolicy) toPolicy(healthPort string) *policy.Policy { // weaver` table) by running the create-if-missing equivalent of `network policy // create` for each canonical BN category. It must run before NftWeaverPersist so // the policy registry is populated when that step re-renders and persists -// network-weaver-workload-policy.nft (an empty registry would render a policy-drop chain). +// network-weaver-workload-policy.nft (an empty registry persists no file at all). // // Every create is idempotent: a re-run leaves existing policies and their // operator-mutated set membership untouched. When force is set, each policy's diff --git a/scripts/network/gen-appstate.sh b/scripts/network/gen-appstate.sh index 1050b783..53599e36 100755 --- a/scripts/network/gen-appstate.sh +++ b/scripts/network/gen-appstate.sh @@ -17,8 +17,16 @@ # gen-appstate.sh \ # # -# Ports are the current per-deployment BN listener assignments: -# publisher 40984 | subscriber 40980 | block-access 40981 | server-status 40982 +# Ports come from the caller, which reads them out of the chart values the harness +# deploys with (test/config/network_uat_values.yaml) so the fixture cannot claim a +# listener the block node does not have. A wrong port here is invisible: the daemon +# programs it into the `_ports` set, real traffic on the true port then +# matches no rule and lands in the default class, and the run reads as a +# classification regression rather than a seeding bug. +# +# On the single-service topology every facility shares one port, so the fixture's +# four ports collapse to the same value and only the source-IP sets separate +# publisher/partner from public. Override individually for a split-topology run. set -eu if [ "$#" -ne 7 ]; then @@ -34,11 +42,12 @@ PUBLIC=$5 BACKFILL=$6 RESTRICTED=$7 -PORT_PUBLISHER=40984 -PORT_SUBSCRIBER=40980 -PORT_BLOCKACCESS=40981 -PORT_STATUS=40982 -BACKFILL_PEER_PORT=50980 # the peer-BN API port for the outbound backfill entry +BN_PORT=${BN_PORT:?BN_PORT is required (read from the harness chart values)} +PORT_PUBLISHER=${PORT_PUBLISHER:-$BN_PORT} +PORT_SUBSCRIBER=${PORT_SUBSCRIBER:-$BN_PORT} +PORT_BLOCKACCESS=${PORT_BLOCKACCESS:-$BN_PORT} +PORT_STATUS=${PORT_STATUS:-$BN_PORT} +BACKFILL_PEER_PORT=${BACKFILL_PEER_PORT:-50980} # the peer-BN API port for the outbound backfill entry mkdir -p "$OUT_DIR" diff --git a/scripts/network/provision-peers.sh b/scripts/network/provision-peers.sh index bc01aaba..c6594b1b 100755 --- a/scripts/network/provision-peers.sh +++ b/scripts/network/provision-peers.sh @@ -4,7 +4,9 @@ # Provision a freshly cloned "peers" UTM VM's guest OS for the network harness: # 1. de-conflict its DHCP identity and stop the dhcpcd<->macvlan ARP churn, # 2. wait for a stable bridged LAN IP, -# 3. install the SSH key and verify it with a real login. +# 3. install the SSH key and verify it with a real login, +# 4. grant passwordless sudo and verify it with `sudo -n` over that login, +# 5. pin the ARP sysctls the macvlan children need. # # Prints the resolved peers LAN IP as the LAST line of STDOUT; all progress goes # to STDERR, so the caller can capture the IP with `$(... )`. @@ -102,5 +104,33 @@ done if [ -z "$SSH_OK" ]; then log "❌ SSH key never took effect on the peers VM (login still failing)"; exit 1; fi log "✓ SSH key working on $PEERS_IP" +# 4. Grant passwordless sudo, then VERIFY with `sudo -n` over a real SSH login. +# The caller sets up the macvlan children over non-interactive SSH, where a +# sudo password prompt cannot be answered: every `sudo` fails, and because +# those failures are non-fatal the harness goes on to report peer IPs that +# were never created. The golden image carries no such drop-in — vm.yaml +# installs one only on the BN VM, which is why that VM works and this one +# does not. +log "Configuring passwordless sudo..." +SUDO_OK="" +for _ in $(seq 1 5); do + gexec "echo '$USER_ ALL=(ALL) NOPASSWD: ALL' > /etc/sudoers.d/$USER_ && chmod 440 /etc/sudoers.d/$USER_" || true + if ssh -i "$PRIV" $SSH_OPTS -o BatchMode=yes -o ConnectTimeout=5 "$USER_@$PEERS_IP" "sudo -n true" 2>/dev/null; then + SUDO_OK=1; break + fi + sleep 2 +done +if [ -z "$SUDO_OK" ]; then log "❌ passwordless sudo never took effect on the peers VM"; exit 1; fi +log "✓ passwordless sudo working on $PEERS_IP" + +# 5. Stop ARP flux before any macvlan child exists. All six addresses share one +# /24, and with the default arp_ignore=0 every interface answers ARP for every +# local address — the upstream switch can then bind the parent's management IP +# to a child's MAC and the peers VM drops off mid-run. arp_announce=2 keeps the +# parent from sourcing ARP with a child's address. Written to /etc/sysctl.d so +# it survives the VM restarts this harness does between runs. +log "Hardening ARP for the multi-homed peers VM..." +gexec "printf 'net.ipv4.conf.all.arp_ignore=1\nnet.ipv4.conf.all.arp_announce=2\n' > /etc/sysctl.d/99-solo-weaver-peers.conf && sysctl -p /etc/sysctl.d/99-solo-weaver-peers.conf >/dev/null" || true + # Emit the resolved IP as the last stdout line for the caller to capture. echo "$PEERS_IP" diff --git a/taskfiles/network.yaml b/taskfiles/network.yaml index 7a6a9204..89b77216 100644 --- a/taskfiles/network.yaml +++ b/taskfiles/network.yaml @@ -31,6 +31,9 @@ vars: NET_EGRESS_IFACE: '{{.NET_EGRESS_IFACE | default "enp0s1"}}' # Host arch selects the daemon binary the BN install consumes (built into bin/). NET_DAEMON_ARCH: '{{.NET_DAEMON_ARCH | default "arm64"}}' + # Chart values the harness deploys with, and the single source of truth for the + # port network:seed-statusz writes into the statusz fixtures. + NET_BN_VALUES: '{{.NET_BN_VALUES | default "test/config/blocknode_values_multi_ports.yaml"}}' tasks: network:all: @@ -140,7 +143,16 @@ tasks: # classification. ASSUMPTION: the UTM network forwards these child MACs # (true on a bridged segment; validate on 'shared' — this is the macvlan risk). NIC="$(ssh -i {{.SSH_PRIVATE_KEY}} {{.SSH_OPTS}} {{.VM_USER}}@$PEERS_IP "ip route get 1.1.1.1 | sed -n 's/.* dev \([^ ]*\).*/\1/p' | head -1")" - echo "peers primary NIC: ${NIC:-}" + [ -z "$NIC" ] && { echo "❌ could not resolve the peers VM's primary NIC (no default route yet? retry once it has finished booting)"; exit 1; } + echo "peers primary NIC: $NIC" + + # iperf3 is the traffic generator for every role, so install it once here + # rather than re-running apt inside the per-role loop below. + ssh -i {{.SSH_PRIVATE_KEY}} {{.SSH_OPTS}} {{.VM_USER}}@$PEERS_IP \ + "command -v iperf3 >/dev/null 2>&1 || (sudo apt-get update -qq && sudo apt-get install -y iperf3 >/dev/null)" || true + ssh -i {{.SSH_PRIVATE_KEY}} {{.SSH_OPTS}} {{.VM_USER}}@$PEERS_IP "command -v iperf3 >/dev/null" \ + || { echo "❌ iperf3 is missing on the peers VM (apt failed — is the guest online?)"; exit 1; } + PREFIX="$(echo "$PEERS_IP" | cut -d. -f1-3)" : > "{{.NET_HOSTS_FILE}}" echo "PEERS_VM_IP=$PEERS_IP" >> "{{.NET_HOSTS_FILE}}" @@ -149,15 +161,19 @@ tasks: i=$((i + 1)) off="$(echo '{{.NET_PEER_OFFSETS}}' | cut -d' ' -f$i)" ip="$PREFIX.$off" + # Verify the address by effect and fail hard. sudo that cannot prompt, or + # a segment that will not forward the child MAC, both leave the address + # absent — and a hosts.env listing peer IPs that do not exist sends every + # downstream task chasing a classification bug that is really a setup bug. ssh -i {{.SSH_PRIVATE_KEY}} {{.SSH_OPTS}} {{.VM_USER}}@$PEERS_IP " sudo ip link add ${role}0 link $NIC type macvlan mode bridge 2>/dev/null || true sudo ip addr replace $ip/24 dev ${role}0 sudo ip link set ${role}0 up - command -v iperf3 >/dev/null 2>&1 || (sudo apt-get update -qq && sudo apt-get install -y iperf3) - " 2>&1 || echo " ⚠️ macvlan/iperf3 setup for $role may have failed (validate on-host)" + ip -4 -br addr show ${role}0 | grep -q '$ip/24' + " || { echo "❌ could not bring up $ip on ${role}0 — check passwordless sudo on the peers VM and macvlan forwarding on this network segment"; exit 1; } upper="$(echo "$role" | tr '[:lower:]' '[:upper:]')" echo "${upper}_IP=$ip" >> "{{.NET_HOSTS_FILE}}" - echo " $role -> $ip (${role}0)" + echo " ✓ $role -> $ip (${role}0)" done echo "✓ peer source IPs captured:" cat "{{.NET_HOSTS_FILE}}" @@ -176,7 +192,34 @@ tasks: [ -z "$BN_IP" ] && { echo "❌ BN VM has no IP (is it running?)"; exit 1; } echo "BN local address: $BN_IP" - bash scripts/network/gen-appstate.sh "{{.NET_STATE_DIR}}" \ + # Read the facility ports out of the same values file network:bn-deploy + # installs with, so the fixture and the deployed chart cannot disagree. + yamlport() { sed -n "s/^[[:space:]]*$1:[[:space:]]*\([0-9]\{1,\}\).*/\1/p" "{{.NET_BN_VALUES}}" | head -1; } + # Spelled as if/fi rather than `[ -z ... ] && { ...; }`. Under go-task's + # mvdan/sh with `set -e`, an && list that ends false is a failure even on the + # path where the guard is simply not tripped, so the idiom aborts the task with + # a bare "exit status 1" and no message. + needport() { + if [ -z "$2" ]; then + echo "❌ $1 not found in {{.NET_BN_VALUES}}" + exit 1 + fi + } + BN_PORT="$(yamlport port)" + PORT_PUBLISHER="$(yamlport publisher)" + PORT_SUBSCRIBER="$(yamlport subscriber)" + PORT_BLOCKACCESS="$(yamlport blockAccess)" + PORT_STATUS="$(yamlport serverStatus)" + needport "service.port" "$BN_PORT" + needport "blockNode.ports.publisher" "$PORT_PUBLISHER" + needport "blockNode.ports.subscriber" "$PORT_SUBSCRIBER" + needport "blockNode.ports.blockAccess" "$PORT_BLOCKACCESS" + needport "blockNode.ports.serverStatus" "$PORT_STATUS" + echo "BN facility ports (from {{.NET_BN_VALUES}}): publisher=$PORT_PUBLISHER subscriber=$PORT_SUBSCRIBER blockAccess=$PORT_BLOCKACCESS serverStatus=$PORT_STATUS" + + BN_PORT="$BN_PORT" PORT_PUBLISHER="$PORT_PUBLISHER" PORT_SUBSCRIBER="$PORT_SUBSCRIBER" \ + PORT_BLOCKACCESS="$PORT_BLOCKACCESS" PORT_STATUS="$PORT_STATUS" \ + bash scripts/network/gen-appstate.sh "{{.NET_STATE_DIR}}" \ "$BN_IP" "$PUBLISHER_IP" "$PARTNER_IP" "$PUBLIC_IP" "$BACKFILL_IP" "$RESTRICTED_IP" echo "Copying application-state files -> $BN_IP:{{.NET_APPSTATE_DIR}} ..." @@ -219,6 +262,7 @@ tasks: ssh -i {{.SSH_PRIVATE_KEY}} {{.SSH_OPTS}} {{.VM_USER}}@$BN_IP " cd /mnt/solo-weaver && sudo solo-provisioner block node install -p local \ + --values {{.NET_BN_VALUES}} \ --traffic-shaping-enabled --egress-interface {{.NET_EGRESS_IFACE}} --link-rate 1gbit \ --firewall-enabled --mgmt-cidrs '$MGMT_CIDRS' \ --daemon-bin bin/solo-provisioner-daemon-linux-{{.NET_DAEMON_ARCH}} -VV diff --git a/test/config/blocknode_values_multi_ports.yaml b/test/config/blocknode_values_multi_ports.yaml new file mode 100644 index 00000000..7579c8d4 --- /dev/null +++ b/test/config/blocknode_values_multi_ports.yaml @@ -0,0 +1,57 @@ +# SPDX-License-Identifier: Apache-2.0 +# +# Block node chart values for the multi-VM network harness (taskfiles/network.yaml). +# +# Splits the block node's facilities onto distinct listener ports so the harness can +# exercise port-based classification. Without this the chart binds every facility to +# the single blockNode.config.SERVER_PORT, all the `_ports` sets collapse to +# one value, and the only thing separating publisher from partner from public is the +# source-address set — leaving the port half of every rule untested. +# +# Deep-merged over the profile's base template by ComputeValuesFile, so this carries +# only the deltas; initContainers, persistence wiring and the rest come from the base. +# +# health is deliberately NOT set here. The base template pins it from +# DefaultBlockNodeHealthPort so the port weaver opens for bn-mgmt is the same port it +# dials for statusz; overriding it here would win the merge and break statusz +# discovery, which is what bootstraps every other port in this file. +# +# This file is also the single source of truth for the ports network:seed-statusz +# writes into the application-state fixtures, so what the block node reports through +# statusz — and therefore what the daemon programs into the nft sets — cannot drift +# from what the chart actually exposes. + +blockNode: + ports: + publisher: 40984 + subscriber: 40980 + blockAccess: 40981 + serverStatus: 40982 + config: + # Fallback listener for any facility not split out above. + SERVER_PORT: "40840" + +plugins: + names: "facility-messaging,health,server-status,block-access-service,stream-publisher,stream-subscriber,verification,blocks-file-historic,blocks-file-recent,backfill" + +service: + type: LoadBalancer + port: 40840 + +# Sized for the UTM VM, which is smaller than a real node. +resources: + requests: + cpu: "500m" + memory: "1Gi" + limits: + cpu: "1" + memory: "1.5Gi" + +kubepromstack: + enabled: false + +loki: + enabled: false + +promtail: + enabled: false