[client] Apply return traffic rules only if firewall is stateless (#3895)

client/firewall/iptables/manager_linux.go

@@ -147,6 +147,10 @@ func (m *Manager) IsServerRouteSupported() bool {
 	return true
 }
 
+func (m *Manager) IsStateful() bool {
+	return true
+}
+
 func (m *Manager) AddNatRule(pair firewall.RouterPair) error {
 	m.mutex.Lock()
 	defer m.mutex.Unlock()

client/firewall/manager/firewall.go

@@ -116,6 +116,8 @@ type Manager interface {
 	// IsServerRouteSupported returns true if the firewall supports server side routing operations
 	IsServerRouteSupported() bool
 
+	IsStateful() bool
+
 	AddRouteFiltering(
 		id []byte,
 		sources []netip.Prefix,

client/firewall/nftables/manager_linux.go

@@ -170,6 +170,10 @@ func (m *Manager) IsServerRouteSupported() bool {
 	return true
 }
 
+func (m *Manager) IsStateful() bool {
+	return true
+}
+
 func (m *Manager) AddNatRule(pair firewall.RouterPair) error {
 	m.mutex.Lock()
 	defer m.mutex.Unlock()

client/firewall/uspfilter/uspfilter.go

@@ -326,6 +326,10 @@ func (m *Manager) IsServerRouteSupported() bool {
 	return true
 }
 
+func (m *Manager) IsStateful() bool {
+	return m.stateful
+}
+
 func (m *Manager) AddNatRule(pair firewall.RouterPair) error {
 	if m.nativeRouter.Load() && m.nativeFirewall != nil {
 		return m.nativeFirewall.AddNatRule(pair)
@@ -606,9 +610,8 @@ func (m *Manager) processOutgoingHooks(packetData []byte, size int) bool {
 		return true
 	}
 
-	if m.stateful {
+	// for netflow we keep track even if the firewall is stateless
-		m.trackOutbound(d, srcIP, dstIP, size)
+	m.trackOutbound(d, srcIP, dstIP, size)
-	}
 
 	return false
 }

client/internal/acl/manager.go

@@ -285,8 +285,10 @@ func (d *DefaultManager) protoRuleToFirewallRule(
 	case mgmProto.RuleDirection_IN:
 		rules, err = d.addInRules(r.PolicyID, ip, protocol, port, action, ipsetName)
 	case mgmProto.RuleDirection_OUT:
-		// TODO: Remove this soon. Outbound rules are obsolete.
+		if d.firewall.IsStateful() {
-		// We only maintain this for return traffic (inbound dir) which is now handled by the stateful firewall already
+			return "", nil, nil
+		}
+		// return traffic for outbound connections if firewall is stateless
 		rules, err = d.addOutRules(r.PolicyID, ip, protocol, port, action, ipsetName)
 	default:
 		return "", nil, fmt.Errorf("invalid direction, skipping firewall rule")

client/internal/acl/manager_test.go

@@ -5,9 +5,10 @@ import (
 	"testing"
 
 	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/netbirdio/netbird/client/firewall"
-	"github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/iface/wgaddr"
 	"github.com/netbirdio/netbird/client/internal/acl/mocks"
 	"github.com/netbirdio/netbird/client/internal/netflow"
@@ -43,9 +44,7 @@ func TestDefaultManager(t *testing.T) {
 	ifaceMock.EXPECT().IsUserspaceBind().Return(true).AnyTimes()
 	ifaceMock.EXPECT().SetFilter(gomock.Any())
 	ip, network, err := net.ParseCIDR("172.0.0.1/32")
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("failed to parse IP address: %v", err)
-	}
 
 	ifaceMock.EXPECT().Name().Return("lo").AnyTimes()
 	ifaceMock.EXPECT().Address().Return(wgaddr.Address{
@@ -54,23 +53,22 @@ func TestDefaultManager(t *testing.T) {
 	}).AnyTimes()
 	ifaceMock.EXPECT().GetWGDevice().Return(nil).AnyTimes()
 
-	// we receive one rule from the management so for testing purposes ignore it
 	fw, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false)
-	if err != nil {
+	require.NoError(t, err)
-		t.Errorf("create firewall: %v", err)
+	defer func() {
-		return
+		err = fw.Close(nil)
-	}
+		require.NoError(t, err)
-	defer func(fw manager.Manager) {
+	}()
-		_ = fw.Close(nil)
+
-	}(fw)
 	acl := NewDefaultManager(fw)
 
 	t.Run("apply firewall rules", func(t *testing.T) {
 		acl.ApplyFiltering(networkMap, false)
 
-		if len(acl.peerRulesPairs) != 2 {
+		if fw.IsStateful() {
-			t.Errorf("firewall rules not applied: %v", acl.peerRulesPairs)
+			assert.Equal(t, 0, len(acl.peerRulesPairs))
-			return
+		} else {
+			assert.Equal(t, 2, len(acl.peerRulesPairs))
 		}
 	})
 
@@ -94,12 +92,13 @@ func TestDefaultManager(t *testing.T) {
 
 		acl.ApplyFiltering(networkMap, false)
 
-		// we should have one old and one new rule in the existed rules
+		expectedRules := 2
-		if len(acl.peerRulesPairs) != 2 {
+		if fw.IsStateful() {
-			t.Errorf("firewall rules not applied")
+			expectedRules = 1 // only the inbound rule
-			return
 		}
 
+		assert.Equal(t, expectedRules, len(acl.peerRulesPairs))
+
 		// check that old rule was removed
 		previousCount := 0
 		for id := range acl.peerRulesPairs {
@@ -107,26 +106,87 @@ func TestDefaultManager(t *testing.T) {
 				previousCount++
 			}
 		}
-		if previousCount != 1 {
+
-			t.Errorf("old rule was not removed")
+		expectedPreviousCount := 0
+		if !fw.IsStateful() {
+			expectedPreviousCount = 1
 		}
+		assert.Equal(t, expectedPreviousCount, previousCount)
 	})
 
 	t.Run("handle default rules", func(t *testing.T) {
 		networkMap.FirewallRules = networkMap.FirewallRules[:0]
 
 		networkMap.FirewallRulesIsEmpty = true
-		if acl.ApplyFiltering(networkMap, false); len(acl.peerRulesPairs) != 0 {
+		acl.ApplyFiltering(networkMap, false)
-			t.Errorf("rules should be empty if FirewallRulesIsEmpty is set, got: %v", len(acl.peerRulesPairs))
+		assert.Equal(t, 0, len(acl.peerRulesPairs))
-			return
-		}
 
 		networkMap.FirewallRulesIsEmpty = false
 		acl.ApplyFiltering(networkMap, false)
-		if len(acl.peerRulesPairs) != 1 {
+
-			t.Errorf("rules should contain 1 rules if FirewallRulesIsEmpty is not set, got: %v", len(acl.peerRulesPairs))
+		expectedRules := 1
-			return
+		if fw.IsStateful() {
+			expectedRules = 1 // only inbound allow-all rule
 		}
+		assert.Equal(t, expectedRules, len(acl.peerRulesPairs))
+	})
+}
+
+func TestDefaultManagerStateless(t *testing.T) {
+	// stateless currently only in userspace, so we have to disable kernel
+	t.Setenv("NB_WG_KERNEL_DISABLED", "true")
+	t.Setenv("NB_DISABLE_CONNTRACK", "true")
+
+	networkMap := &mgmProto.NetworkMap{
+		FirewallRules: []*mgmProto.FirewallRule{
+			{
+				PeerIP:    "10.93.0.1",
+				Direction: mgmProto.RuleDirection_OUT,
+				Action:    mgmProto.RuleAction_ACCEPT,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "80",
+			},
+			{
+				PeerIP:    "10.93.0.2",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_ACCEPT,
+				Protocol:  mgmProto.RuleProtocol_UDP,
+				Port:      "53",
+			},
+		},
+	}
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	ifaceMock := mocks.NewMockIFaceMapper(ctrl)
+	ifaceMock.EXPECT().IsUserspaceBind().Return(true).AnyTimes()
+	ifaceMock.EXPECT().SetFilter(gomock.Any())
+	ip, network, err := net.ParseCIDR("172.0.0.1/32")
+	require.NoError(t, err)
+
+	ifaceMock.EXPECT().Name().Return("lo").AnyTimes()
+	ifaceMock.EXPECT().Address().Return(wgaddr.Address{
+		IP:      ip,
+		Network: network,
+	}).AnyTimes()
+	ifaceMock.EXPECT().GetWGDevice().Return(nil).AnyTimes()
+
+	fw, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false)
+	require.NoError(t, err)
+	defer func() {
+		err = fw.Close(nil)
+		require.NoError(t, err)
+	}()
+
+	acl := NewDefaultManager(fw)
+
+	t.Run("stateless firewall creates outbound rules", func(t *testing.T) {
+		acl.ApplyFiltering(networkMap, false)
+
+		// In stateless mode, we should have both inbound and outbound rules
+		assert.False(t, fw.IsStateful())
+		assert.Equal(t, 2, len(acl.peerRulesPairs))
 	})
 }
 
@@ -192,42 +252,19 @@ func TestDefaultManagerSquashRules(t *testing.T) {
 
 	manager := &DefaultManager{}
 	rules, _ := manager.squashAcceptRules(networkMap)
-	if len(rules) != 2 {
+	assert.Equal(t, 2, len(rules))
-		t.Errorf("rules should contain 2, got: %v", rules)
-		return
-	}
 
 	r := rules[0]
-	switch {
+	assert.Equal(t, "0.0.0.0", r.PeerIP)
-	case r.PeerIP != "0.0.0.0":
+	assert.Equal(t, mgmProto.RuleDirection_IN, r.Direction)
-		t.Errorf("IP should be 0.0.0.0, got: %v", r.PeerIP)
+	assert.Equal(t, mgmProto.RuleProtocol_ALL, r.Protocol)
-		return
+	assert.Equal(t, mgmProto.RuleAction_ACCEPT, r.Action)
-	case r.Direction != mgmProto.RuleDirection_IN:
-		t.Errorf("direction should be IN, got: %v", r.Direction)
-		return
-	case r.Protocol != mgmProto.RuleProtocol_ALL:
-		t.Errorf("protocol should be ALL, got: %v", r.Protocol)
-		return
-	case r.Action != mgmProto.RuleAction_ACCEPT:
-		t.Errorf("action should be ACCEPT, got: %v", r.Action)
-		return
-	}
 
 	r = rules[1]
-	switch {
+	assert.Equal(t, "0.0.0.0", r.PeerIP)
-	case r.PeerIP != "0.0.0.0":
+	assert.Equal(t, mgmProto.RuleDirection_OUT, r.Direction)
-		t.Errorf("IP should be 0.0.0.0, got: %v", r.PeerIP)
+	assert.Equal(t, mgmProto.RuleProtocol_ALL, r.Protocol)
-		return
+	assert.Equal(t, mgmProto.RuleAction_ACCEPT, r.Action)
-	case r.Direction != mgmProto.RuleDirection_OUT:
-		t.Errorf("direction should be OUT, got: %v", r.Direction)
-		return
-	case r.Protocol != mgmProto.RuleProtocol_ALL:
-		t.Errorf("protocol should be ALL, got: %v", r.Protocol)
-		return
-	case r.Action != mgmProto.RuleAction_ACCEPT:
-		t.Errorf("action should be ACCEPT, got: %v", r.Action)
-		return
-	}
 }
 
 func TestDefaultManagerSquashRulesNoAffect(t *testing.T) {
@@ -291,9 +328,8 @@ func TestDefaultManagerSquashRulesNoAffect(t *testing.T) {
 	}
 
 	manager := &DefaultManager{}
-	if rules, _ := manager.squashAcceptRules(networkMap); len(rules) != len(networkMap.FirewallRules) {
+	rules, _ := manager.squashAcceptRules(networkMap)
-		t.Errorf("we should get the same amount of rules as output, got %v", len(rules))
+	assert.Equal(t, len(networkMap.FirewallRules), len(rules))
-	}
 }
 
 func TestDefaultManagerEnableSSHRules(t *testing.T) {
@@ -337,9 +373,7 @@ func TestDefaultManagerEnableSSHRules(t *testing.T) {
 	ifaceMock.EXPECT().IsUserspaceBind().Return(true).AnyTimes()
 	ifaceMock.EXPECT().SetFilter(gomock.Any())
 	ip, network, err := net.ParseCIDR("172.0.0.1/32")
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("failed to parse IP address: %v", err)
-	}
 
 	ifaceMock.EXPECT().Name().Return("lo").AnyTimes()
 	ifaceMock.EXPECT().Address().Return(wgaddr.Address{
@@ -348,21 +382,20 @@ func TestDefaultManagerEnableSSHRules(t *testing.T) {
 	}).AnyTimes()
 	ifaceMock.EXPECT().GetWGDevice().Return(nil).AnyTimes()
 
-	// we receive one rule from the management so for testing purposes ignore it
 	fw, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false)
-	if err != nil {
+	require.NoError(t, err)
-		t.Errorf("create firewall: %v", err)
+	defer func() {
-		return
+		err = fw.Close(nil)
-	}
+		require.NoError(t, err)
-	defer func(fw manager.Manager) {
+	}()
-		_ = fw.Close(nil)
+
-	}(fw)
 	acl := NewDefaultManager(fw)
 
 	acl.ApplyFiltering(networkMap, false)
 
-	if len(acl.peerRulesPairs) != 3 {
+	expectedRules := 3
-		t.Errorf("expect 3 rules (last must be SSH), got: %d", len(acl.peerRulesPairs))
+	if fw.IsStateful() {
-		return
+		expectedRules = 3 // 2 inbound rules + SSH rule
 	}
+	assert.Equal(t, expectedRules, len(acl.peerRulesPairs))
 }

client/internal/routemanager/static/route.go

@@ -24,7 +24,6 @@ func NewRoute(rt *route.Route, routeRefCounter *refcounter.RouteRefCounter, allo
 	}
 }
 
-// Route route methods
 func (r *Route) String() string {
 	return r.route.Network.String()
 }