diff --git a/client/firewall/iptables/dnat_refcount_linux_test.go b/client/firewall/iptables/dnat_refcount_linux_test.go new file mode 100644 index 000000000..26df653ae --- /dev/null +++ b/client/firewall/iptables/dnat_refcount_linux_test.go @@ -0,0 +1,195 @@ +package iptables + +import ( + "net/netip" + "testing" + + "github.com/stretchr/testify/require" + + fw "github.com/netbirdio/netbird/client/firewall/manager" + "github.com/netbirdio/netbird/client/iface" + "github.com/netbirdio/netbird/client/iface/wgaddr" +) + +func iptRefcountIfaceV4() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("10.20.0.1"), + Network: netip.MustParsePrefix("10.20.0.0/24"), + } + }, + } +} + +func iptRefcountIfaceDual() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("10.20.0.1"), + Network: netip.MustParsePrefix("10.20.0.0/24"), + IPv6: netip.MustParseAddr("fd00::1"), + IPv6Net: netip.MustParsePrefix("fd00::/64"), + } + }, + } +} + +func newIptRefcountManager(t *testing.T, dual bool) *Manager { + t.Helper() + var ifMock *iFaceMock + if dual { + ifMock = iptRefcountIfaceDual() + } else { + ifMock = iptRefcountIfaceV4() + } + m, err := Create(ifMock, iface.DefaultMTU) + require.NoError(t, err, "create manager") + require.NoError(t, m.Init(nil), "init manager") + t.Cleanup(func() { + require.NoError(t, m.Close(nil), "close manager") + }) + return m +} + +func iptDnatV4(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("10.20.0.2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +func iptDnatV6(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("fd00::2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +// TestIptablesDNAT_RefcountBalancedV4 covers a Balanced Add/Delete pair on v4. +func TestIptablesDNAT_RefcountBalancedV4(t *testing.T) { + m := newIptRefcountManager(t, false) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(iptDnatV4(7081)) + require.NoError(t, err, "add v4 dnat 1") + v4, v6 := state.Counts() + require.Equal(t, 1, v4, "v4 refcount after first add") + require.Equal(t, 0, v6, "v6 refcount unchanged") + + r2, err := m.AddDNATRule(iptDnatV4(7082)) + require.NoError(t, err, "add v4 dnat 2") + v4, _ = state.Counts() + require.Equal(t, 2, v4, "v4 refcount after second add") + + require.NoError(t, m.DeleteDNATRule(r1)) + v4, _ = state.Counts() + require.Equal(t, 1, v4, "v4 refcount after first delete") + + require.NoError(t, m.DeleteDNATRule(r2)) + v4, v6 = state.Counts() + require.Equal(t, 0, v4, "v4 refcount after second delete") + require.Equal(t, 0, v6, "v6 refcount unchanged") +} + +// TestIptablesDNAT_RefcountBalancedV6 checks the v6 path increments v6 only and +// decrements back to zero. +func TestIptablesDNAT_RefcountBalancedV6(t *testing.T) { + m := newIptRefcountManager(t, true) + require.NotNil(t, m.router6, "v6 router") + require.Same(t, m.router.ipFwdState, m.router6.ipFwdState, "shared state") + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(iptDnatV6(9081)) + require.NoError(t, err, "add v6 dnat 1") + v4, v6 := state.Counts() + require.Equal(t, 0, v4) + require.Equal(t, 1, v6, "v6 refcount after first add") + + r2, err := m.AddDNATRule(iptDnatV6(9082)) + require.NoError(t, err, "add v6 dnat 2") + _, v6 = state.Counts() + require.Equal(t, 2, v6, "v6 refcount after second add") + + require.NoError(t, m.DeleteDNATRule(r1)) + _, v6 = state.Counts() + require.Equal(t, 1, v6, "v6 refcount after first delete") + + require.NoError(t, m.DeleteDNATRule(r2)) + v4, v6 = state.Counts() + require.Equal(t, 0, v4) + require.Equal(t, 0, v6, "v6 refcount after second delete") +} + +// TestIptablesDNAT_DuplicateAddNoLeak verifies the duplicate-rule path returns +// without bumping the refcount. +func TestIptablesDNAT_DuplicateAddNoLeak(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + rule := iptDnatV4(7083) + r1, err := m.AddDNATRule(rule) + require.NoError(t, err) + v4, _ := state.Counts() + require.Equal(t, 1, v4) + + _, err = m.AddDNATRule(rule) + require.NoError(t, err, "duplicate add") + v4, _ = state.Counts() + require.Equal(t, 1, v4, "duplicate add must not increment") + + require.NoError(t, m.DeleteDNATRule(r1)) + v4, _ = state.Counts() + require.Equal(t, 0, v4, "single delete must drop to zero") +} + +// TestIptablesDNAT_DeleteMissingNoUnderflow verifies Delete on an unknown rule +// neither errors nor releases the refcount. +func TestIptablesDNAT_DeleteMissingNoUnderflow(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + phantom := iptDnatV4(7099) + require.NoError(t, m.DeleteDNATRule(&phantom), "delete missing v4") + v4, v6 := state.Counts() + require.Equal(t, 0, v4) + require.Equal(t, 0, v6) + + phantom6 := iptDnatV6(9099) + require.NoError(t, m.DeleteDNATRule(&phantom6), "delete missing v6") + v4, v6 = state.Counts() + require.Equal(t, 0, v4) + require.Equal(t, 0, v6) + + r1, err := m.AddDNATRule(iptDnatV4(7100)) + require.NoError(t, err) + v4, _ = state.Counts() + require.Equal(t, 1, v4, "real add still increments after phantom delete") + require.NoError(t, m.DeleteDNATRule(r1)) +} + +// TestIptablesDNAT_DoubleDeleteNoUnderflow verifies a second Delete on the same +// rule is a no-op. +func TestIptablesDNAT_DoubleDeleteNoUnderflow(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(iptDnatV6(9083)) + require.NoError(t, err) + _, v6 := state.Counts() + require.Equal(t, 1, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "first delete") + _, v6 = state.Counts() + require.Equal(t, 0, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "second delete must be no-op") + _, v6 = state.Counts() + require.Equal(t, 0, v6, "double delete must not underflow") +} diff --git a/client/firewall/iptables/router_linux.go b/client/firewall/iptables/router_linux.go index 592d34f5c..eeb86ca0d 100644 --- a/client/firewall/iptables/router_linux.go +++ b/client/firewall/iptables/router_linux.go @@ -763,10 +763,6 @@ func (r *router) updateState() { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { - if err := r.ipFwdState.RequestForwarding(r.v6); err != nil { - return nil, err - } - ruleKey := rule.ID() if _, exists := r.rules[ruleKey+dnatSuffix]; exists { return rule, nil @@ -841,6 +837,16 @@ func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { r.rules[key] = ruleInfo.rule } + if err := r.ipFwdState.RequestForwarding(r.v6); err != nil { + if rollbackErr := r.rollbackRules(rules); rollbackErr != nil { + log.Errorf("rollback failed: %v", rollbackErr) + } + for key := range rules { + delete(r.rules, key) + } + return nil, fmt.Errorf("enable forwarding: %w", err) + } + r.updateState() return rule, nil } @@ -861,12 +867,15 @@ func (r *router) rollbackRules(rules map[string]ruleInfo) error { } func (r *router) DeleteDNATRule(rule firewall.Rule) error { - if err := r.ipFwdState.ReleaseForwarding(r.v6); err != nil { - log.Errorf("%v", err) - } - ruleKey := rule.ID() + _, hadDNAT := r.rules[ruleKey+dnatSuffix] + _, hadSNAT := r.rules[ruleKey+snatSuffix] + _, hadFWD := r.rules[ruleKey+fwdSuffix] + if !hadDNAT && !hadSNAT && !hadFWD { + return nil + } + var merr *multierror.Error if dnatRule, exists := r.rules[ruleKey+dnatSuffix]; exists { if err := r.iptablesClient.Delete(tableNat, chainRTRDR, dnatRule...); err != nil { @@ -889,6 +898,10 @@ func (r *router) DeleteDNATRule(rule firewall.Rule) error { delete(r.rules, ruleKey+fwdSuffix) } + if err := r.ipFwdState.ReleaseForwarding(r.v6); err != nil { + log.Errorf("%v", err) + } + r.updateState() return nberrors.FormatErrorOrNil(merr) } diff --git a/client/firewall/nftables/dnat_refcount_linux_test.go b/client/firewall/nftables/dnat_refcount_linux_test.go new file mode 100644 index 000000000..e4d1449d3 --- /dev/null +++ b/client/firewall/nftables/dnat_refcount_linux_test.go @@ -0,0 +1,206 @@ +package nftables + +import ( + "net/netip" + "testing" + + "github.com/stretchr/testify/require" + + fw "github.com/netbirdio/netbird/client/firewall/manager" + "github.com/netbirdio/netbird/client/iface" + "github.com/netbirdio/netbird/client/iface/wgaddr" +) + +func nftRefcountIfaceV4() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.96.0.1"), + Network: netip.MustParsePrefix("100.96.0.0/16"), + } + }, + } +} + +func nftRefcountIfaceDual() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.96.0.1"), + Network: netip.MustParsePrefix("100.96.0.0/16"), + IPv6: netip.MustParseAddr("fd00::1"), + IPv6Net: netip.MustParsePrefix("fd00::/64"), + } + }, + } +} + +func newNftRefcountManager(t *testing.T, dual bool) *Manager { + t.Helper() + if check() != NFTABLES { + t.Skip("nftables not supported on this system") + } + var ifMock *iFaceMock + if dual { + ifMock = nftRefcountIfaceDual() + } else { + ifMock = nftRefcountIfaceV4() + } + m, err := Create(ifMock, iface.DefaultMTU) + require.NoError(t, err, "create manager") + require.NoError(t, m.Init(nil), "init manager") + t.Cleanup(func() { + require.NoError(t, m.Close(nil), "close manager") + }) + return m +} + +func dnatV4(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("100.96.0.2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +func dnatV6(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("fd00::2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +// TestNftablesDNAT_RefcountBalancedV4 verifies that Add/Delete pairs leave the +// v4 refcount at zero. +func TestNftablesDNAT_RefcountBalancedV4(t *testing.T) { + m := newNftRefcountManager(t, false) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(dnatV4(8081)) + require.NoError(t, err, "add v4 dnat 1") + v4, v6 := state.Counts() + require.Equal(t, 1, v4, "v4 refcount after first add") + require.Equal(t, 0, v6, "v6 refcount unchanged") + + r2, err := m.AddDNATRule(dnatV4(8082)) + require.NoError(t, err, "add v4 dnat 2") + v4, v6 = state.Counts() + require.Equal(t, 2, v4, "v4 refcount after second add") + require.Equal(t, 0, v6, "v6 refcount unchanged") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v4 dnat 1") + v4, _ = state.Counts() + require.Equal(t, 1, v4, "v4 refcount after first delete") + + require.NoError(t, m.DeleteDNATRule(r2), "delete v4 dnat 2") + v4, v6 = state.Counts() + require.Equal(t, 0, v4, "v4 refcount after second delete") + require.Equal(t, 0, v6, "v6 refcount unchanged") +} + +// TestNftablesDNAT_RefcountBalancedV6 verifies the v6 path increments v6 only +// and decrements back to zero on Delete. +func TestNftablesDNAT_RefcountBalancedV6(t *testing.T) { + m := newNftRefcountManager(t, true) + require.NotNil(t, m.router6, "v6 router") + require.Same(t, m.router.ipFwdState, m.router6.ipFwdState, "shared state") + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(dnatV6(9091)) + require.NoError(t, err, "add v6 dnat 1") + v4, v6 := state.Counts() + require.Equal(t, 0, v4, "v4 refcount unchanged") + require.Equal(t, 1, v6, "v6 refcount after first add") + + r2, err := m.AddDNATRule(dnatV6(9092)) + require.NoError(t, err, "add v6 dnat 2") + v4, v6 = state.Counts() + require.Equal(t, 0, v4) + require.Equal(t, 2, v6, "v6 refcount after second add") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v6 dnat 1") + _, v6 = state.Counts() + require.Equal(t, 1, v6, "v6 refcount after first delete") + + require.NoError(t, m.DeleteDNATRule(r2), "delete v6 dnat 2") + v4, v6 = state.Counts() + require.Equal(t, 0, v4) + require.Equal(t, 0, v6, "v6 refcount after second delete") +} + +// TestNftablesDNAT_DuplicateAddNoLeak verifies that a duplicate Add (same +// ForwardRule) does not double-increment the refcount. +func TestNftablesDNAT_DuplicateAddNoLeak(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + rule := dnatV4(8083) + r1, err := m.AddDNATRule(rule) + require.NoError(t, err, "add v4 dnat") + v4, _ := state.Counts() + require.Equal(t, 1, v4) + + // duplicate add: same rule ID, must be a no-op for the refcount. + _, err = m.AddDNATRule(rule) + require.NoError(t, err, "duplicate add") + v4, _ = state.Counts() + require.Equal(t, 1, v4, "duplicate add must not increment") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v4 dnat") + v4, _ = state.Counts() + require.Equal(t, 0, v4, "single delete must drop to zero") +} + +// TestNftablesDNAT_DeleteMissingNoUnderflow verifies deleting a rule that was +// never added does not underflow the refcount. +func TestNftablesDNAT_DeleteMissingNoUnderflow(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + // Construct a Rule reference for something never added. The router stores + // rules by ID(), and DeleteDNATRule looks them up in r.rules; a missing + // entry must be a no-op rather than calling Release. + phantom := dnatV4(8099) + require.NoError(t, m.DeleteDNATRule(&phantom), "delete missing v4 dnat") + v4, v6 := state.Counts() + require.Equal(t, 0, v4, "v4 refcount unaffected by missing delete") + require.Equal(t, 0, v6, "v6 refcount unaffected") + + phantom6 := dnatV6(9099) + require.NoError(t, m.DeleteDNATRule(&phantom6), "delete missing v6 dnat") + v4, v6 = state.Counts() + require.Equal(t, 0, v4) + require.Equal(t, 0, v6, "v6 refcount unaffected by missing delete") + + // And after a phantom delete, a real add still results in count=1. + r1, err := m.AddDNATRule(dnatV4(8100)) + require.NoError(t, err, "add v4 dnat after phantom delete") + v4, _ = state.Counts() + require.Equal(t, 1, v4, "real add still increments after phantom delete") + require.NoError(t, m.DeleteDNATRule(r1)) +} + +// TestNftablesDNAT_DoubleDeleteNoUnderflow verifies that deleting the same rule +// twice does not underflow the refcount (the second delete is a no-op). +func TestNftablesDNAT_DoubleDeleteNoUnderflow(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(dnatV6(9093)) + require.NoError(t, err) + _, v6 := state.Counts() + require.Equal(t, 1, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "first delete") + _, v6 = state.Counts() + require.Equal(t, 0, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "second delete must be no-op") + _, v6 = state.Counts() + require.Equal(t, 0, v6, "double delete must not underflow") +} diff --git a/client/firewall/nftables/router_linux.go b/client/firewall/nftables/router_linux.go index 701f6ddcd..9e5fb431d 100644 --- a/client/firewall/nftables/router_linux.go +++ b/client/firewall/nftables/router_linux.go @@ -1550,10 +1550,6 @@ func (r *router) refreshRulesMap() error { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { - if err := r.ipFwdState.RequestForwarding(r.af.tableFamily == nftables.TableFamilyIPv6); err != nil { - return nil, err - } - ruleKey := rule.ID() if _, exists := r.rules[ruleKey+dnatSuffix]; exists { return rule, nil @@ -1575,7 +1571,19 @@ func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { // We also cannot just add "oif accept" there and filter in our own table as we don't know what is supposed to be allowed. // TODO: find chains with drop policies and add rules there + v6 := r.af.tableFamily == nftables.TableFamilyIPv6 + if err := r.ipFwdState.RequestForwarding(v6); err != nil { + delete(r.rules, ruleKey+dnatSuffix) + delete(r.rules, ruleKey+snatSuffix) + return nil, fmt.Errorf("enable forwarding: %w", err) + } + if err := r.conn.Flush(); err != nil { + if rerr := r.ipFwdState.ReleaseForwarding(v6); rerr != nil { + log.Warnf("rollback forwarding refcount: %v", rerr) + } + delete(r.rules, ruleKey+dnatSuffix) + delete(r.rules, ruleKey+snatSuffix) return nil, fmt.Errorf("flush rules: %w", err) } @@ -1778,16 +1786,18 @@ func (r *router) addDnatMasq(rule firewall.ForwardRule, protoNum uint8, ruleKey } func (r *router) DeleteDNATRule(rule firewall.Rule) error { - if err := r.ipFwdState.ReleaseForwarding(r.af.tableFamily == nftables.TableFamilyIPv6); err != nil { - log.Errorf("%v", err) - } - ruleKey := rule.ID() if err := r.refreshRulesMap(); err != nil { return fmt.Errorf(refreshRulesMapError, err) } + _, hadDNAT := r.rules[ruleKey+dnatSuffix] + _, hadSNAT := r.rules[ruleKey+snatSuffix] + if !hadDNAT && !hadSNAT { + return nil + } + var merr *multierror.Error var needsFlush bool @@ -1822,6 +1832,9 @@ func (r *router) DeleteDNATRule(rule firewall.Rule) error { if merr == nil { delete(r.rules, ruleKey+dnatSuffix) delete(r.rules, ruleKey+snatSuffix) + if err := r.ipFwdState.ReleaseForwarding(r.af.tableFamily == nftables.TableFamilyIPv6); err != nil { + log.Errorf("%v", err) + } } return nberrors.FormatErrorOrNil(merr) diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate.go b/client/internal/routemanager/ipfwdstate/ipfwdstate.go index 7838979a4..5a1bef031 100644 --- a/client/internal/routemanager/ipfwdstate/ipfwdstate.go +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate.go @@ -25,6 +25,14 @@ func NewIPForwardingState(wgIfaceName string) *IPForwardingState { return &IPForwardingState{wgIfaceName: wgIfaceName} } +// Counts returns the current v4 and v6 refcounts. Intended for diagnostics +// and tests. +func (f *IPForwardingState) Counts() (v4, v6 int) { + f.mu.Lock() + defer f.mu.Unlock() + return f.v4Count, f.v6Count +} + // RequestForwarding enables the family's forwarding sysctl on first request. func (f *IPForwardingState) RequestForwarding(v6 bool) error { f.mu.Lock()