diff --git a/client/firewall/iptables/manager_linux.go b/client/firewall/iptables/manager_linux.go index b229688fc..0897f831f 100644 --- a/client/firewall/iptables/manager_linux.go +++ b/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() diff --git a/client/firewall/manager/firewall.go b/client/firewall/manager/firewall.go index 084d19423..3b3164823 100644 --- a/client/firewall/manager/firewall.go +++ b/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, diff --git a/client/firewall/nftables/manager_linux.go b/client/firewall/nftables/manager_linux.go index e6b3a031b..2f8ee81a4 100644 --- a/client/firewall/nftables/manager_linux.go +++ b/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() diff --git a/client/firewall/uspfilter/uspfilter.go b/client/firewall/uspfilter/uspfilter.go index 11730dbb3..287e52773 100644 --- a/client/firewall/uspfilter/uspfilter.go +++ b/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 { - m.trackOutbound(d, srcIP, dstIP, size) - } + // for netflow we keep track even if the firewall is stateless + m.trackOutbound(d, srcIP, dstIP, size) return false } diff --git a/client/internal/acl/manager.go b/client/internal/acl/manager.go index a6316d7a2..5caf2b770 100644 --- a/client/internal/acl/manager.go +++ b/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. - // We only maintain this for return traffic (inbound dir) which is now handled by the stateful firewall already + if d.firewall.IsStateful() { + 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") diff --git a/client/internal/acl/manager_test.go b/client/internal/acl/manager_test.go index 3595ca600..532d70a24 100644 --- a/client/internal/acl/manager_test.go +++ b/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 { - t.Fatalf("failed to parse IP address: %v", err) - } + require.NoError(t, 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 { - t.Errorf("create firewall: %v", err) - return - } - defer func(fw manager.Manager) { - _ = fw.Close(nil) - }(fw) + require.NoError(t, err) + defer func() { + err = fw.Close(nil) + require.NoError(t, err) + }() + acl := NewDefaultManager(fw) t.Run("apply firewall rules", func(t *testing.T) { acl.ApplyFiltering(networkMap, false) - if len(acl.peerRulesPairs) != 2 { - t.Errorf("firewall rules not applied: %v", acl.peerRulesPairs) - return + if fw.IsStateful() { + assert.Equal(t, 0, len(acl.peerRulesPairs)) + } 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 - if len(acl.peerRulesPairs) != 2 { - t.Errorf("firewall rules not applied") - return + expectedRules := 2 + if fw.IsStateful() { + expectedRules = 1 // only the inbound rule } + 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 { - t.Errorf("rules should be empty if FirewallRulesIsEmpty is set, got: %v", len(acl.peerRulesPairs)) - return - } + acl.ApplyFiltering(networkMap, false) + assert.Equal(t, 0, len(acl.peerRulesPairs)) 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)) - return + + expectedRules := 1 + 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 { - t.Errorf("rules should contain 2, got: %v", rules) - return - } + assert.Equal(t, 2, len(rules)) r := rules[0] - switch { - case r.PeerIP != "0.0.0.0": - t.Errorf("IP should be 0.0.0.0, got: %v", r.PeerIP) - return - 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 - } + assert.Equal(t, "0.0.0.0", r.PeerIP) + assert.Equal(t, mgmProto.RuleDirection_IN, r.Direction) + assert.Equal(t, mgmProto.RuleProtocol_ALL, r.Protocol) + assert.Equal(t, mgmProto.RuleAction_ACCEPT, r.Action) r = rules[1] - switch { - case r.PeerIP != "0.0.0.0": - t.Errorf("IP should be 0.0.0.0, got: %v", r.PeerIP) - return - 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 - } + assert.Equal(t, "0.0.0.0", r.PeerIP) + assert.Equal(t, mgmProto.RuleDirection_OUT, r.Direction) + assert.Equal(t, mgmProto.RuleProtocol_ALL, r.Protocol) + assert.Equal(t, mgmProto.RuleAction_ACCEPT, r.Action) } 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) { - t.Errorf("we should get the same amount of rules as output, got %v", len(rules)) - } + rules, _ := manager.squashAcceptRules(networkMap) + 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 { - t.Fatalf("failed to parse IP address: %v", err) - } + require.NoError(t, 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 { - t.Errorf("create firewall: %v", err) - return - } - defer func(fw manager.Manager) { - _ = fw.Close(nil) - }(fw) + require.NoError(t, err) + defer func() { + err = fw.Close(nil) + require.NoError(t, err) + }() + acl := NewDefaultManager(fw) acl.ApplyFiltering(networkMap, false) - if len(acl.peerRulesPairs) != 3 { - t.Errorf("expect 3 rules (last must be SSH), got: %d", len(acl.peerRulesPairs)) - return + expectedRules := 3 + if fw.IsStateful() { + expectedRules = 3 // 2 inbound rules + SSH rule } + assert.Equal(t, expectedRules, len(acl.peerRulesPairs)) } diff --git a/client/internal/routemanager/static/route.go b/client/internal/routemanager/static/route.go index 98c34dbee..681c192fb 100644 --- a/client/internal/routemanager/static/route.go +++ b/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() }