From c8d8211385d803eb441107d70b8fc22ca6bb15e5 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 14 Sep 2026 15:27:10 +0200 Subject: [PATCH] The firewall governs what is forwarded, and never closes ssh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two faults, opposite directions, both in issue 047. There was no forward chain, on the reasoning that dropping there stops every container the runtime allowed. The first half is true; the conclusion was not. A published port is redirected and then forwarded, so it never reaches the input chain — the firewall was silent about the ports most worth protecting. The way through is the one the system being replaced already used: deny by default, then allow the runtime's own networks explicitly. A forwarded rule matches what the client originally asked for, because the destination has been rewritten by the time the chain sees it. And ssh is now a floor nothing derives. Every other line comes from what is assigned, which is the point — but a mesh part-way through adopting a machine has been assigned almost nothing, so what it computed was a chain that shut the port used to fix it. From the mesh always; from outside on a machine that faces outward, because that is the way in when the private network is what broke. Rehearsed on three machines: a docker-published port declared mesh-only is now reachable from inside the mesh and refused from outside. Before, it was reachable from both. --- internal/catalogue/declaration.go | 2 +- internal/catalogue/filtering.go | 115 +++++++++++++++++++++++++-- internal/catalogue/filtering_test.go | 102 ++++++++++++++++++++---- 3 files changed, 193 insertions(+), 26 deletions(-) diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 3ead392..e91c3da 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -207,7 +207,7 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) { if err != nil { return nil, err } - filtering := AsNftables(rules, with.Mesh) + filtering := AsNftables(rules, with.Mesh, r.PublicDomain != "") var out []map[string]any for _, m := range r.Modules { diff --git a/internal/catalogue/filtering.go b/internal/catalogue/filtering.go index 090bd63..275a204 100644 --- a/internal/catalogue/filtering.go +++ b/internal/catalogue/filtering.go @@ -209,7 +209,14 @@ func widest(rules []Rule) []Rule { // `mesh` is rendered as the addresses of the nodes that are actually on the private network, not // as a subnet. The mesh knows every address; a subnet is a guess that stays wrong quietly, and // the set shrinks when a node leaves without anybody editing anything. -func AsNftables(rules []Rule, mesh []string) string { +// SSHPort is the one port a machine may never lose, whatever else it is told. +const SSHPort = 22 + +// AsNftables renders a rule set as a complete nftables configuration. +// +// `outward` says this machine is reachable from outside the mesh, which is the only thing that +// decides whether ssh is answered there as well as on the private network. +func AsNftables(rules []Rule, mesh []string, outward bool) string { var b strings.Builder b.WriteString("# Computed by the mesh from what is assigned to this node.\n") b.WriteString("# Edits are lost on the next declaration; change a module's listens instead.\n\n") @@ -231,6 +238,34 @@ func AsNftables(rules []Rule, mesh []string) string { b.WriteString("\t\ticmp type echo-request accept\n") b.WriteString("\t\ticmpv6 type { echo-request, nd-neighbor-solicit, nd-neighbor-advert, nd-router-advert } accept\n") + // **ssh, always, and not because a module asked.** + // + // Every other line in this chain is derived from what is assigned here, which is the whole + // point of the mechanism. This one is not, and the exception is deliberate: the rules are + // computed from what the mesh knows, a mesh part-way through adopting a machine knows almost + // nothing, and the first port to close is the one used to fix it. The session that loads these + // rules survives on conntrack until it drops, and then the machine is reached from a rescue + // console (novox/hq issue 047). + // + // From the private network always, because that is how machines reach each other. From + // everywhere when this machine faces outward, because that is the only way in on the day the + // private network is what broke. + if four, six := byFamily(mesh); len(four) > 0 || len(six) > 0 { + b.WriteString("\n\t\t# ssh, from the mesh — never derived, never closed\n") + if len(four) > 0 { + b.WriteString(fmt.Sprintf("\t\tip saddr { %s } tcp dport %d accept\n", + strings.Join(four, ", "), SSHPort)) + } + if len(six) > 0 { + b.WriteString(fmt.Sprintf("\t\tip6 saddr { %s } tcp dport %d accept\n", + strings.Join(six, ", "), SSHPort)) + } + } + if outward { + b.WriteString("\t\t# and from outside, because this machine faces it\n") + b.WriteString(fmt.Sprintf("\t\ttcp dport %d accept\n", SSHPort)) + } + if len(rules) > 0 { b.WriteString("\n") } @@ -277,18 +312,82 @@ func AsNftables(rules []Rule, mesh []string) string { // Nothing this node sends is filtered, and it is stated rather than left to nftables' default // so that reading this file answers the question instead of requiring the reader to know it. b.WriteString("\tchain output {\n\t\ttype filter hook output priority filter; policy accept;\n\t}\n") - // **No forward chain, deliberately.** What this machine forwards is the container runtime's - // business — docker writes its own rules for the bridges it creates, and a second table - // hooking forward is consulted as well as those, so a drop here drops container traffic that - // docker explicitly allowed. Every container on the node stops, including the control plane. + + // **A forward chain, because without one this firewall does not cover container ports.** // - // The mesh has no knowledge with which to compute a forwarding policy: nothing in a manifest - // says what a machine routes. Writing one anyway would be a rule nothing derives, which is the - // fault this whole mechanism exists to remove. + // It used to have none, on the reasoning that a drop here would drop container traffic the + // runtime explicitly allowed and stop every container on the node. The first half of that is + // true. The conclusion was not: a published port is redirected and then *forwarded*, so it + // never reaches the input chain above, and a firewall with no forward chain says nothing at all + // about the ports most worth protecting. Rehearsed on three machines: loading these rules + // refused a port on the host and left a published container port reachable (novox/hq issue 047). + // + // The way through is the one the system being replaced already used: deny by default here, and + // then explicitly allow the runtime's own networks, so containers keep working while everything + // else has to be asked for. + b.WriteString("\tchain forward {\n") + b.WriteString("\t\ttype filter hook forward priority filter; policy drop;\n") + b.WriteString("\t\tct state established,related accept\n") + b.WriteString("\t\tct state invalid drop\n") + b.WriteString("\n") + // What the container runtime created. Without these, denying by default stops every container + // on the machine — which is exactly the failure the absent chain was avoiding, avoided properly. + for _, network := range runtimeNetworks { + b.WriteString(fmt.Sprintf("\t\t# %s\n", network.why)) + b.WriteString(fmt.Sprintf("\t\tip saddr %s accept\n", network.cidr)) + } + + if len(rules) > 0 { + b.WriteString("\n") + } + for _, rule := range rules { + for i, module := range rule.Because { + why := "" + if i < len(rule.Why) && rule.Why[i] != "" { + why = " — " + rule.Why[i] + } + b.WriteString(fmt.Sprintf("\t\t# %s%s\n", module, why)) + } + // **Matched on where the packet was originally addressed, not where it is going now.** + // By the time a packet reaches this chain the runtime has already rewritten its + // destination to the container's own port, so matching the port a rule names would match + // nothing. Conntrack remembers what was asked for, which is the port the machine + // publishes and the only one a client ever knew. + switch rule.From { + case FromMachine: + b.WriteString(fmt.Sprintf("\t\t# %s/%d is this machine only, and is not forwarded\n", + rule.Protocol, rule.Port)) + case FromMesh: + four, six := byFamily(mesh) + if len(four) > 0 { + b.WriteString(fmt.Sprintf("\t\tip saddr { %s } ct original proto-dst %d accept\n", + strings.Join(four, ", "), rule.Port)) + } + if len(six) > 0 { + b.WriteString(fmt.Sprintf("\t\tip6 saddr { %s } ct original proto-dst %d accept\n", + strings.Join(six, ", "), rule.Port)) + } + case FromEverywhere: + b.WriteString(fmt.Sprintf("\t\tct original proto-dst %d accept\n", rule.Port)) + } + } + b.WriteString("\t}\n") b.WriteString("}\n") return b.String() } +// runtimeNetworks are the container runtime's own networks, which must keep working when the +// forward chain denies by default. +// +// Taken from what the system being replaced allows, which has been carrying this machine's traffic +// for months: the runtime's bridge range and the range its compose files are given. A machine whose +// runtime is configured with something else needs this to say so — which is a thing the mesh cannot +// derive and a reason this list is named here rather than computed. +var runtimeNetworks = []struct{ cidr, why string }{ + {"172.16.0.0/12", "the container runtime's bridge networks"}, + {"192.168.128.0/17", "the networks its compose files are given"}, +} + // byFamily splits addresses into the two nftables understands separately. // // `ip saddr` and `ip6 saddr` are different matches, and one set holding both families is a syntax diff --git a/internal/catalogue/filtering_test.go b/internal/catalogue/filtering_test.go index fb0a4fb..80f9d80 100644 --- a/internal/catalogue/filtering_test.go +++ b/internal/catalogue/filtering_test.go @@ -79,7 +79,7 @@ func TestTwoModulesWantingOnePortAreBothNamed(t *testing.T) { t.Fatalf("a module that wanted this port open is not named: %+v", rules[0]) } // The consequence, which is the reason this matters: removing web must not read as closing 443. - nft := AsNftables(rules, nil) + nft := AsNftables(rules, nil, false) if !strings.Contains(nft, "web") || !strings.Contains(nft, "board") { t.Fatalf("the rendered rule set does not name both sources:\n%s", nft) } @@ -107,13 +107,16 @@ func TestAPortOpenToEveryoneIsNotAlsoRestrictedToTheMesh(t *testing.T) { func TestWhatNoModuleDeclaredIsClosed(t *testing.T) { nft := AsNftables(mustFilter(t, Resolution{Modules: []Manifest{ {Module: "web", Listens: []Listening{{Port: 443, From: FromEverywhere}}}, - }}, nil), []string{"198.51.100.2"}) + }}, nil), []string{"198.51.100.2"}, false) // Naming the chain, not just the policy: the forward chain drops too, and an assertion on // "policy drop" alone passes while the input chain accepts everything. It did, once, here. if !strings.Contains(nft, "type filter hook input priority filter; policy drop;") { t.Fatalf("the input chain does not drop by default, so nothing is filtered:\n%s", nft) } - if strings.Contains(nft, "dport 22") { + // Some port nothing asked for. **Not 22**, which this used to use: ssh is now the one line in + // the chain that nothing derives, because a mesh that has been assigned almost nothing computes + // a firewall that shuts the port used to fix it (novox/hq issue 047). + if strings.Contains(nft, "dport 9999") { t.Fatalf("a port no module declared was opened:\n%s", nft) } // Dropping by default must not mean unreachable: the node's own outbound link to the broker @@ -131,7 +134,7 @@ func TestWhatNoModuleDeclaredIsClosed(t *testing.T) { // `flush ruleset` would do the first and not the second: it empties every table on the machine, // including the ones the container runtime writes for its bridges. func TestReloadingReplacesOnlyTheMeshsOwnRules(t *testing.T) { - nft := AsNftables(nil, nil) + nft := AsNftables(nil, nil, false) if strings.Contains(nft, "flush ruleset") { t.Fatalf("loading the rule set empties every table on the machine:\n%s", nft) } @@ -145,16 +148,54 @@ func TestReloadingReplacesOnlyTheMeshsOwnRules(t *testing.T) { } } -// The rule set governs what reaches this machine, and nothing else. +// The rule set governs what is forwarded too, or it does not govern container ports at all. // -// A forward policy would be consulted alongside the container runtime's own rules, so dropping -// there stops container traffic the runtime allowed -- every container on the node, control plane -// included. And nothing in a manifest says what a machine routes, so there is nothing to derive it -// from: a forwarding rule here would be exactly the undeclared rule this mechanism removes. -func TestTheRuleSetDoesNotDecideWhatTheMachineForwards(t *testing.T) { - nft := AsNftables(nil, []string{"198.51.100.2"}) - if strings.Contains(nft, "hook forward") { - t.Fatalf("the rule set filters forwarding, which stops every container on the node:\n%s", nft) +// **This reverses an earlier decision, and keeps the thing that decision was protecting.** There was +// no forward chain, on the reasoning that dropping there stops container traffic the runtime +// allowed — every container on the node, control plane included. That much is true. What it missed +// is that a published port is redirected and then forwarded, so it never reaches the input chain, +// and a firewall without a forward chain is silent about exactly the ports worth protecting +// (novox/hq issue 047). +// +// So the chain exists and denies by default, and the runtime's own networks are allowed explicitly +// — which is how the system being replaced has been doing it on these machines for months. +func TestWhatIsForwardedIsGovernedToo(t *testing.T) { + nft := AsNftables(nil, []string{"198.51.100.2"}, false) + if !strings.Contains(nft, "hook forward priority filter; policy drop") { + t.Fatalf("forwarded traffic is not governed, so container ports are open:\n%s", nft) + } +} + +// And containers keep working, which is the whole reason the chain was left out before. +func TestTheRuntimesOwnNetworksKeepWorking(t *testing.T) { + nft := AsNftables(nil, []string{"198.51.100.2"}, false) + for _, network := range []string{"172.16.0.0/12", "192.168.128.0/17"} { + if !strings.Contains(nft, "ip saddr "+network+" accept") { + t.Fatalf("%s is not allowed, so denying by default stops every container:\n%s", network, nft) + } + } +} + +// A published port is matched by what the client asked for, not by where the packet ends up. +// +// The runtime rewrites the destination before this chain sees it, so a rule naming the published +// port would match nothing and the port would stay shut while reporting itself open. +func TestAPublishedPortIsMatchedByWhatWasAskedFor(t *testing.T) { + nft := AsNftables(mustFilter(t, Resolution{Modules: []Manifest{ + {Module: "web", Listens: []Listening{{Port: 8080, From: FromEverywhere}}}, + }}, nil), []string{"198.51.100.2"}, false) + if !strings.Contains(nft, "ct original proto-dst 8080 accept") { + t.Fatalf("the forwarded rule does not match the port a client asked for:\n%s", nft) + } +} + +// And a mesh-scoped port stays mesh-scoped when it is reached through the runtime. +func TestAMeshScopedPortIsMeshScopedWhenForwarded(t *testing.T) { + nft := AsNftables(mustFilter(t, Resolution{Modules: []Manifest{ + {Module: "store", Listens: []Listening{{Port: 5432, From: FromMesh}}}, + }}, nil), []string{"198.51.100.2"}, false) + if !strings.Contains(nft, "ip saddr { 198.51.100.2 } ct original proto-dst 5432 accept") { + t.Fatalf("a mesh-only port is reachable from anywhere once forwarded:\n%s", nft) } } @@ -162,7 +203,7 @@ func TestTheRuleSetDoesNotDecideWhatTheMachineForwards(t *testing.T) { func TestFromTheMeshIsTheNodesTheMeshKnows(t *testing.T) { nft := AsNftables(mustFilter(t, Resolution{Modules: []Manifest{ {Module: "store", Listens: []Listening{{Port: 5432, From: FromMesh}}}, - }}, nil), []string{"198.51.100.2", "198.51.100.3"}) + }}, nil), []string{"198.51.100.2", "198.51.100.3"}, false) if !strings.Contains(nft, "ip saddr { 198.51.100.2, 198.51.100.3 } tcp dport 5432 accept") { t.Fatalf("a mesh-scoped port was not restricted to the mesh's addresses:\n%s", nft) } @@ -172,7 +213,7 @@ func TestFromTheMeshIsTheNodesTheMeshKnows(t *testing.T) { func TestAMeshPortOnANodeWithNoMeshIsClosedAndSaysSo(t *testing.T) { nft := AsNftables(mustFilter(t, Resolution{Modules: []Manifest{ {Module: "store", Listens: []Listening{{Port: 5432, From: FromMesh}}}, - }}, nil), nil) + }}, nil), nil, false) if strings.Contains(nft, "dport 5432 accept") { t.Fatalf("a port meant for the mesh was opened to everything:\n%s", nft) } @@ -185,7 +226,7 @@ func TestAMeshPortOnANodeWithNoMeshIsClosedAndSaysSo(t *testing.T) { func TestAMachineScopedPortIsNotOpened(t *testing.T) { nft := AsNftables(mustFilter(t, Resolution{Modules: []Manifest{ {Module: "cache", Listens: []Listening{{Port: 6379, From: FromMachine}}}, - }}, nil), []string{"198.51.100.2"}) + }}, nil), []string{"198.51.100.2"}, false) if strings.Contains(nft, "dport 6379 accept") { t.Fatalf("a port for this machine only was opened to the network:\n%s", nft) } @@ -227,7 +268,7 @@ func TestAskingForTheRuleSetWithNowhereToPutItIsRefused(t *testing.T) { func TestAMeshOnBothAddressFamiliesRendersBoth(t *testing.T) { nft := AsNftables(mustFilter(t, Resolution{Modules: []Manifest{ {Module: "store", Listens: []Listening{{Port: 5432, From: FromMesh}}}, - }}, nil), []string{"198.51.100.2", "2001:db8::2"}) + }}, nil), []string{"198.51.100.2", "2001:db8::2"}, false) if !strings.Contains(nft, "ip saddr { 198.51.100.2 } tcp dport 5432 accept") { t.Fatalf("the machines with v4 addresses were dropped:\n%s", nft) } @@ -622,3 +663,30 @@ func TestExposureRefusesAPortNotListenedOnAndABadSource(t *testing.T) { t.Error("a source that is not mesh/anywhere/machine was accepted") } } + +// ssh survives whatever the mesh does or does not know about this machine. +// +// **The one rule here that nothing derives.** Every other line comes from what is assigned, which +// is the point of the mechanism — and a mesh part-way through adopting a machine has been assigned +// almost nothing, so what it computes is a chain that drops the port used to fix it. The session +// loading the rules lives on conntrack until it drops, and then the machine is reached from a +// rescue console (novox/hq issue 047). +func TestSSHIsOpenFromTheMeshEvenWhenNothingIsAssigned(t *testing.T) { + nft := AsNftables(nil, []string{"198.51.100.2", "198.51.100.3"}, false) + if !strings.Contains(nft, "ip saddr { 198.51.100.2, 198.51.100.3 } tcp dport 22 accept") { + t.Fatalf("ssh is not open to the mesh, so a machine can lock everyone out:\n%s", nft) + } + // And not to the world, on a machine that does not face it. + if strings.Contains(nft, "\t\ttcp dport 22 accept") { + t.Fatalf("ssh is open to everywhere on a machine that faces inward only:\n%s", nft) + } +} + +// And from outside as well, on a machine that faces outward — because that is the way in when the +// private network is the thing that broke. +func TestSSHIsOpenFromOutsideOnAMachineThatFacesIt(t *testing.T) { + nft := AsNftables(nil, []string{"198.51.100.2"}, true) + if !strings.Contains(nft, "\t\ttcp dport 22 accept") { + t.Fatalf("a machine reachable from outside does not answer ssh there:\n%s", nft) + } +}