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) + } +}