diff --git a/client/firewall/iptables/manager_linux_test.go b/client/firewall/iptables/manager_linux_test.go index 3f9a0e7b9..9f53352e1 100644 --- a/client/firewall/iptables/manager_linux_test.go +++ b/client/firewall/iptables/manager_linux_test.go @@ -322,6 +322,49 @@ func TestIptablesFilterDestinationSetRequiresIPSet(t *testing.T) { require.ErrorContains(t, err, "requires ipset") } +// TestIptablesNatRuleDropsSourceSetOnDestinationFailure covers a marking rule +// whose source set is created but whose destination set is not: the source +// reference has to go back, or the set it created stays in the kernel with a +// count nothing will ever drop. +func TestIptablesNatRuleDropsSourceSetOnDestinationFailure(t *testing.T) { + manager, err := Create(ifaceMock, iface.DefaultMTU) + require.NoError(t, err) + require.NoError(t, manager.Init(nil)) + + defer func() { + require.NoError(t, manager.Close(nil)) + }() + + sourceSet := fw.NewPrefixSet([]netip.Prefix{ + netip.MustParsePrefix("100.0.0.0/16"), + netip.MustParsePrefix("10.10.0.0/16"), + }) + destSet := fw.NewDomainSet(domain.List{"example.org"}) + + // Poison the destination set's name so its hash:net creation fails after + // the source set has already been created. + poisoned := manager.family4.ipsetName(destSet.HashedName()) + require.NoError(t, ipset.Create(poisoned, ipset.TypeHashIP, ipset.CreateOptions{})) + t.Cleanup(func() { + if err := ipset.Destroy(poisoned); err != nil { + t.Logf("destroy poisoned set %s: %v", poisoned, err) + } + }) + + pair := fw.RouterPair{ + ID: "nat-source-set-test", + Source: fw.Network{Set: sourceSet}, + Destination: fw.Network{Set: destSet}, + Masquerade: true, + Dynamic: true, + } + + require.Error(t, manager.AddNatRule(pair), "the destination set must fail to be created") + + _, ok := manager.family4.ipsetCounter.Get(manager.family4.ipsetName(sourceSet.HashedName())) + require.False(t, ok, "the source set reference must be released") +} + // TestIptablesNatRuleReAddKeepsSetReferences re-adds the same NAT rule the way // a repeated network-map update does. The marking rule's set references must not // grow, or RemoveNatRule can never drop the count to zero and the set stays in @@ -461,13 +504,12 @@ func TestIptablesCloseRemovesAllState(t *testing.T) { TranslatedAddress: netip.MustParseAddr("10.20.0.44"), TranslatedPort: fw.Port{Values: []uint16{80}}, } - dnatRule, err := manager.AddDNATRule(dnat) + _, err = manager.AddDNATRule(dnat) require.NoError(t, err, "add dnat rule") require.NotEqual(t, before, snapshotIptables(t, ipv4Client), "the manager must have installed state") - require.NoError(t, manager.DeleteDNATRule(dnatRule), "delete dnat rule") - require.NoError(t, manager.DisableRouting(), "disable routing") + // Everything above stays in place, so Close is what has to remove it. require.NoError(t, manager.Close(nil), "close") after := snapshotIptables(t, ipv4Client) diff --git a/client/firewall/iptables/routing_linux.go b/client/firewall/iptables/routing_linux.go index 80403a9ca..c63be4e43 100644 --- a/client/firewall/iptables/routing_linux.go +++ b/client/firewall/iptables/routing_linux.go @@ -231,6 +231,7 @@ func (r *family) addNatRule(pair firewall.RouterPair) (err error) { } destExp, err := r.applyNetwork("-d", pair.Destination, nil) if err != nil { + r.dropSourceMatch(sourceExp) return fmt.Errorf("apply network -d: %w", err) } diff --git a/client/firewall/nftables/routing_linux.go b/client/firewall/nftables/routing_linux.go index fadb4edea..09dec76e9 100644 --- a/client/firewall/nftables/routing_linux.go +++ b/client/firewall/nftables/routing_linux.go @@ -158,7 +158,13 @@ func (r *family) queueNatRule(pair firewall.RouterPair, exprs []expr.Any) { if _, exists := r.rules[ruleID]; exists { if err := r.removeNatRule(pair); err != nil { + // The rule this replaces may still be in the kernel. Keep tracking + // it and skip the new one: overwriting the entry would leave the old + // rule installed with nothing that can find it again, while keeping + // it lets the next update retry the whole replacement. log.Errorf("replace prerouting rule %s: %v", ruleID, err) + r.dropNetworkMatch(exprs) + return } } @@ -360,7 +366,10 @@ func (r *family) queueLegacyRouteRule(pair firewall.RouterPair, exprs []expr.Any if _, exists := r.rules[ruleID]; exists { if err := r.removeLegacyRouteRule(pair); err != nil { + // Keep the old rule tracked instead of losing it, as in queueNatRule. log.Errorf("replace legacy forwarding rule %s: %v", ruleID, err) + r.dropNetworkMatch(exprs) + return } }