[management] remove ingress ports (#8062)

client/firewall/iptables/dnat_linux.go

@@ -8,177 +8,11 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/hashicorp/go-multierror"
 	log "github.com/sirupsen/logrus"
 
-	nberrors "github.com/netbirdio/netbird/client/errors"
 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
 )
 
-func (r *family) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) {
-	ruleID := rule.ID()
-	if _, exists := r.rules[ruleID+dnatSuffix]; exists {
-		return rule, nil
-	}
-
-	toDestination := rule.TranslatedAddress.String()
-	switch {
-	case len(rule.TranslatedPort.Values) == 0:
-		// no translated port, use original port
-	case len(rule.TranslatedPort.Values) == 1:
-		toDestination += fmt.Sprintf(":%d", rule.TranslatedPort.Values[0])
-	case rule.TranslatedPort.IsRange && len(rule.TranslatedPort.Values) == 2:
-		// need the "/originalport" suffix to avoid dnat port randomization
-		toDestination += fmt.Sprintf(":%d-%d/%d", rule.TranslatedPort.Values[0], rule.TranslatedPort.Values[1], rule.DestinationPort.Values[0])
-	default:
-		return nil, fmt.Errorf("invalid translated port: %v", rule.TranslatedPort)
-	}
-
-	proto := strings.ToLower(string(rule.Protocol))
-
-	rules := make(map[firewall.RuleID]ruleInfo, 3)
-
-	// DNAT rule
-	dnatRule := []string{
-		"!", "-i", r.wgIface.Name(),
-		"-p", proto,
-		"-j", "DNAT",
-		"--to-destination", toDestination,
-	}
-	dnatRule = append(dnatRule, applyPort("--dport", &rule.DestinationPort)...)
-	rules[ruleID+dnatSuffix] = ruleInfo{
-		table: tableNat,
-		chain: chainRTRdr,
-		rule:  dnatRule,
-	}
-
-	// SNAT rule
-	snatRule := []string{
-		"-o", r.wgIface.Name(),
-		"-p", proto,
-		"-d", rule.TranslatedAddress.String(),
-		"-j", "MASQUERADE",
-	}
-	snatRule = append(snatRule, applyPort("--dport", &rule.TranslatedPort)...)
-	rules[ruleID+snatSuffix] = ruleInfo{
-		table: tableNat,
-		chain: chainRTNAT,
-		rule:  snatRule,
-	}
-
-	// Forward filtering rule, if fwd policy is DROP
-	forwardRule := []string{
-		"-o", r.wgIface.Name(),
-		"-p", proto,
-		"-d", rule.TranslatedAddress.String(),
-		"-j", "ACCEPT",
-	}
-	forwardRule = append(forwardRule, applyPort("--dport", &rule.TranslatedPort)...)
-	rules[ruleID+fwdSuffix] = ruleInfo{
-		table: tableFilter,
-		chain: chainRTFwdOut,
-		rule:  forwardRule,
-	}
-
-	for key, ruleInfo := range rules {
-		if err := r.iptablesClient.Append(ruleInfo.table, ruleInfo.chain, ruleInfo.rule...); err != nil {
-			r.cleanupFailedDNATAdd(rules)
-			return nil, fmt.Errorf("add rule %s: %w", key, err)
-		}
-		r.rules[key] = ruleInfo.rule
-	}
-
-	if err := r.ipFwdState.RequestForwarding(r.v6); err != nil {
-		r.cleanupFailedDNATAdd(rules)
-		return nil, fmt.Errorf("enable forwarding: %w", err)
-	}
-
-	r.updateState()
-	return rule, nil
-}
-
-// cleanupFailedDNATAdd removes the bookkeeping written by a partially applied
-// AddDNATRule before rolling back the kernel rules, so no entries remain that
-// never got a forwarding refcount. rollbackRules re-adds entries it failed to
-// remove from the kernel.
-func (r *family) cleanupFailedDNATAdd(rules map[firewall.RuleID]ruleInfo) {
-	for key := range rules {
-		delete(r.rules, key)
-	}
-	if err := r.rollbackRules(rules); err != nil {
-		log.Errorf("rollback failed: %v", err)
-	}
-}
-
-func (r *family) rollbackRules(rules map[firewall.RuleID]ruleInfo) error {
-	var merr *multierror.Error
-	for key, ruleInfo := range rules {
-		if err := r.iptablesClient.DeleteIfExists(ruleInfo.table, ruleInfo.chain, ruleInfo.rule...); err != nil {
-			merr = multierror.Append(merr, fmt.Errorf("rollback rule %s: %w", key, err))
-			// On rollback error, add to rules map for next cleanup
-			r.rules[key] = ruleInfo.rule
-		}
-	}
-	if merr != nil {
-		r.updateState()
-	}
-	return nberrors.FormatErrorOrNil(merr)
-}
-
-func (r *family) DeleteDNATRule(rule firewall.Rule) error {
-	ruleID := rule.ID()
-
-	_, hadDNAT := r.rules[ruleID+dnatSuffix]
-	_, hadSNAT := r.rules[ruleID+snatSuffix]
-	_, hadFWD := r.rules[ruleID+fwdSuffix]
-	if !hadDNAT && !hadSNAT && !hadFWD {
-		return nil
-	}
-
-	var merr *multierror.Error
-	if dnatRule, exists := r.rules[ruleID+dnatSuffix]; exists {
-		if err := r.iptablesClient.Delete(tableNat, chainRTRdr, dnatRule...); err != nil {
-			merr = multierror.Append(merr, fmt.Errorf("delete DNAT rule: %w", err))
-		} else {
-			delete(r.rules, ruleID+dnatSuffix)
-		}
-	}
-
-	if snatRule, exists := r.rules[ruleID+snatSuffix]; exists {
-		if err := r.iptablesClient.Delete(tableNat, chainRTNAT, snatRule...); err != nil {
-			merr = multierror.Append(merr, fmt.Errorf("delete SNAT rule: %w", err))
-		} else {
-			delete(r.rules, ruleID+snatSuffix)
-		}
-	}
-
-	if fwdRule, exists := r.rules[ruleID+fwdSuffix]; exists {
-		if err := r.iptablesClient.Delete(tableFilter, chainRTFwdOut, fwdRule...); err != nil {
-			merr = multierror.Append(merr, fmt.Errorf("delete forward rule: %w", err))
-		} else {
-			delete(r.rules, ruleID+fwdSuffix)
-		}
-	}
-
-	// Release the refcount only once all rules are gone from the kernel. On
-	// partial failure the failed entries stay in r.rules so a retry can remove
-	// them and release then.
-	if merr == nil {
-		r.releaseForwarding()
-	}
-
-	r.updateState()
-
-	return nberrors.FormatErrorOrNil(merr)
-}
-
-// releaseForwarding drops one IP forwarding reference, logging any error.
-func (r *family) releaseForwarding() {
-	if err := r.ipFwdState.ReleaseForwarding(r.v6); err != nil {
-		log.Errorf("release IP forwarding: %v", err)
-	}
-}
-
 func (r *family) AddInboundDNAT(localAddr netip.Addr, protocol firewall.Protocol, originalPort, translatedPort uint16) error {
 	ruleID := firewall.RuleID(fmt.Sprintf("inbound-dnat-%s-%s-%d-%d", localAddr.String(), protocol, originalPort, translatedPort))
 

client/firewall/iptables/dnat_refcount_linux_test.go

@@ -1,240 +0,0 @@
-//go:build privileged
-
-package iptables
-
-import (
-	"net/netip"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"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}},
-	}
-}
-
-// TestIptablesRouting_RepeatedEnableSingleReference verifies that EnableRouting
-// (called on every network-map update) holds at most one reference per family
-// and a single DisableRouting drops both back to zero.
-func TestIptablesRouting_RepeatedEnableSingleReference(t *testing.T) {
-	m := newIptRefcountManager(t, true)
-	state := m.family4.ipFwdState
-
-	require.NoError(t, m.EnableRouting(), "first enable")
-	require.NoError(t, m.EnableRouting(), "second enable")
-	require.NoError(t, m.EnableRouting(), "third enable")
-	v4, v6 := state.Counts()
-	assert.Equal(t, 1, v4, "repeated enable holds a single v4 reference")
-	assert.Equal(t, 1, v6, "repeated enable holds a single v6 reference")
-
-	require.NoError(t, m.DisableRouting(), "disable")
-	v4, v6 = state.Counts()
-	assert.Equal(t, 0, v4, "single disable releases the v4 reference")
-	assert.Equal(t, 0, v6, "single disable releases the v6 reference")
-}
-
-// TestIptablesRouting_DisableKeepsDNATReference verifies that an unpaired
-// DisableRouting does not release references held by active DNAT rules.
-func TestIptablesRouting_DisableKeepsDNATReference(t *testing.T) {
-	m := newIptRefcountManager(t, true)
-	state := m.family4.ipFwdState
-
-	r1, err := m.AddDNATRule(iptDnatV6(9095))
-	require.NoError(t, err, "add v6 dnat")
-
-	require.NoError(t, m.DisableRouting(), "unpaired disable")
-	_, v6 := state.Counts()
-	assert.Equal(t, 1, v6, "DNAT-held reference survives unpaired DisableRouting")
-
-	require.NoError(t, m.DeleteDNATRule(r1), "delete v6 dnat")
-	_, v6 = state.Counts()
-	assert.Equal(t, 0, v6, "delete releases the DNAT reference")
-}
-
-// TestIptablesDNAT_RefcountBalancedV4 covers a Balanced Add/Delete pair on v4.
-func TestIptablesDNAT_RefcountBalancedV4(t *testing.T) {
-	m := newIptRefcountManager(t, false)
-	state := m.family4.ipFwdState
-
-	r1, err := m.AddDNATRule(iptDnatV4(7081))
-	require.NoError(t, err, "add v4 dnat 1")
-	v4, v6 := state.Counts()
-	assert.Equal(t, 1, v4, "v4 refcount after first add")
-	assert.Equal(t, 0, v6, "v6 refcount unchanged")
-
-	r2, err := m.AddDNATRule(iptDnatV4(7082))
-	require.NoError(t, err, "add v4 dnat 2")
-	v4, v6 = state.Counts()
-	assert.Equal(t, 2, v4, "v4 refcount after second add")
-	assert.Equal(t, 0, v6, "v6 refcount unchanged")
-
-	require.NoError(t, m.DeleteDNATRule(r1))
-	v4, v6 = state.Counts()
-	assert.Equal(t, 1, v4, "v4 refcount after first delete")
-	assert.Equal(t, 0, v6, "v6 refcount unchanged")
-
-	require.NoError(t, m.DeleteDNATRule(r2))
-	v4, v6 = state.Counts()
-	assert.Equal(t, 0, v4, "v4 refcount after second delete")
-	assert.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.family6, "v6 family")
-	require.Same(t, m.family4.ipFwdState, m.family6.ipFwdState, "shared state")
-	state := m.family4.ipFwdState
-
-	r1, err := m.AddDNATRule(iptDnatV6(9081))
-	require.NoError(t, err, "add v6 dnat 1")
-	v4, v6 := state.Counts()
-	assert.Equal(t, 0, v4)
-	assert.Equal(t, 1, v6, "v6 refcount after first add")
-
-	r2, err := m.AddDNATRule(iptDnatV6(9082))
-	require.NoError(t, err, "add v6 dnat 2")
-	v4, v6 = state.Counts()
-	assert.Equal(t, 0, v4, "v4 refcount unchanged")
-	assert.Equal(t, 2, v6, "v6 refcount after second add")
-
-	require.NoError(t, m.DeleteDNATRule(r1))
-	v4, v6 = state.Counts()
-	assert.Equal(t, 0, v4, "v4 refcount unchanged")
-	assert.Equal(t, 1, v6, "v6 refcount after first delete")
-
-	require.NoError(t, m.DeleteDNATRule(r2))
-	v4, v6 = state.Counts()
-	assert.Equal(t, 0, v4)
-	assert.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.family4.ipFwdState
-
-	rule := iptDnatV4(7083)
-	r1, err := m.AddDNATRule(rule)
-	require.NoError(t, err)
-	v4, _ := state.Counts()
-	assert.Equal(t, 1, v4)
-
-	_, err = m.AddDNATRule(rule)
-	require.NoError(t, err, "duplicate add")
-	v4, _ = state.Counts()
-	assert.Equal(t, 1, v4, "duplicate add must not increment")
-
-	require.NoError(t, m.DeleteDNATRule(r1))
-	v4, _ = state.Counts()
-	assert.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.family4.ipFwdState
-
-	phantom := iptDnatV4(7099)
-	require.NoError(t, m.DeleteDNATRule(&phantom), "delete missing v4")
-	v4, v6 := state.Counts()
-	assert.Equal(t, 0, v4)
-	assert.Equal(t, 0, v6)
-
-	phantom6 := iptDnatV6(9099)
-	require.NoError(t, m.DeleteDNATRule(&phantom6), "delete missing v6")
-	v4, v6 = state.Counts()
-	assert.Equal(t, 0, v4)
-	assert.Equal(t, 0, v6)
-
-	r1, err := m.AddDNATRule(iptDnatV4(7100))
-	require.NoError(t, err)
-	v4, _ = state.Counts()
-	assert.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.family4.ipFwdState
-
-	r1, err := m.AddDNATRule(iptDnatV6(9083))
-	require.NoError(t, err)
-	_, v6 := state.Counts()
-	assert.Equal(t, 1, v6)
-
-	require.NoError(t, m.DeleteDNATRule(r1), "first delete")
-	_, v6 = state.Counts()
-	assert.Equal(t, 0, v6)
-
-	require.NoError(t, m.DeleteDNATRule(r1), "second delete must be no-op")
-	_, v6 = state.Counts()
-	assert.Equal(t, 0, v6, "double delete must not underflow")
-}

client/firewall/iptables/family_linux.go

@@ -56,10 +56,6 @@ const (
 	markManglePost = "mark-mangle-post"
 	matchSet       = "--match-set"
 
-	dnatSuffix firewall.RuleID = "_dnat"
-	snatSuffix firewall.RuleID = "_snat"
-	fwdSuffix  firewall.RuleID = "_fwd"
-
 	// ipv4TCPHeaderSize is the minimum IPv4 (20) + TCP (20) header size for MSS calculation.
 	ipv4TCPHeaderSize = 40
 	// ipv6TCPHeaderSize is the minimum IPv6 (40) + TCP (20) header size for MSS calculation.

client/firewall/iptables/filter_linux.go

@@ -81,15 +81,6 @@ func (r *family) hasRule(id nbid.RuleID) bool {
 	return ok
 }
 
-// hasDNATRule reports whether this family owns the DNAT rule set for
-// the given user id. DNAT rules live in r.rules under the well-known
-// "<id>_dnat" key; the lookup here is used by Manager.DeleteDNATRule
-// to pick the right family.
-func (r *family) hasDNATRule(id firewall.RuleID) bool {
-	_, ok := r.rules[id+dnatSuffix]
-	return ok
-}
-
 // DeleteFilterRule removes a previously installed filter rule. The
 // rule's stored chain/table identify where to delete from; source set
 // references are recovered from the spec via findSets and dropped

client/firewall/iptables/manager_linux.go

@@ -323,31 +323,6 @@ func (m *Manager) DisableRouting() error {
 	return m.family4.ipFwdState.ReleaseRouting()
 }
 
-// AddDNATRule adds a DNAT rule
-func (m *Manager) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) {
-	m.mutex.Lock()
-	defer m.mutex.Unlock()
-
-	if rule.TranslatedAddress.Is6() {
-		if !m.hasIPv6() {
-			return nil, fmt.Errorf("add DNAT rule: %w", firewall.ErrIPv6NotInitialized)
-		}
-		return m.family6.AddDNATRule(rule)
-	}
-	return m.family4.AddDNATRule(rule)
-}
-
-// DeleteDNATRule deletes a DNAT rule
-func (m *Manager) DeleteDNATRule(rule firewall.Rule) error {
-	m.mutex.Lock()
-	defer m.mutex.Unlock()
-
-	if m.hasIPv6() && !m.family4.hasDNATRule(rule.ID()) {
-		return m.family6.DeleteDNATRule(rule)
-	}
-	return m.family4.DeleteDNATRule(rule)
-}
-
 // UpdateSet updates the set with the given prefixes
 func (m *Manager) UpdateSet(set firewall.Set, prefixes []netip.Prefix) error {
 	m.mutex.Lock()

client/firewall/iptables/manager_linux_test.go

@@ -497,16 +497,6 @@ func TestIptablesCloseRemovesAllState(t *testing.T) {
 	require.NoError(t, manager.AddNatRule(pair), "add nat rule")
 	require.NoError(t, manager.EnableRouting(), "enable routing")
 
-	// A DNAT redirect, which also holds a forwarding reference.
-	dnat := fw.ForwardRule{
-		Protocol:          fw.ProtocolTCP,
-		DestinationPort:   fw.Port{Values: []uint16{8080}},
-		TranslatedAddress: netip.MustParseAddr("10.20.0.44"),
-		TranslatedPort:    fw.Port{Values: []uint16{80}},
-	}
-	_, err = manager.AddDNATRule(dnat)
-	require.NoError(t, err, "add dnat rule")
-
 	require.NotEqual(t, before, snapshotIptables(t, ipv4Client), "the manager must have installed state")
 
 	// Everything above stays in place, so Close is what has to remove it.