[client] Fix rule order for deny rules in peer ACLs (#4147)

Author: lixmal

client/firewall/iptables/acl_linux.go

@@ -85,7 +85,7 @@ func (m *aclManager) AddPeerFiltering(
 ) ([]firewall.Rule, error) {
 	chain := chainNameInputRules
 
-	ipsetName = transformIPsetName(ipsetName, sPort, dPort)
+	ipsetName = transformIPsetName(ipsetName, sPort, dPort, action)
 	specs := filterRuleSpecs(ip, string(protocol), sPort, dPort, action, ipsetName)
 
 	mangleSpecs := slices.Clone(specs)
@@ -135,7 +135,14 @@ func (m *aclManager) AddPeerFiltering(
 		return nil, fmt.Errorf("rule already exists")
 	}
 
-	if err := m.iptablesClient.Append(tableFilter, chain, specs...); err != nil {
+	// Insert DROP rules at the beginning, append ACCEPT rules at the end
+	if action == firewall.ActionDrop {
+		// Insert at the beginning of the chain (position 1)
+		err = m.iptablesClient.Insert(tableFilter, chain, 1, specs...)
+	} else {
+		err = m.iptablesClient.Append(tableFilter, chain, specs...)
+	}
+	if err != nil {
 		return nil, err
 	}
 
@@ -388,17 +395,25 @@ func actionToStr(action firewall.Action) string {
 	return "DROP"
 }
 
-func transformIPsetName(ipsetName string, sPort, dPort *firewall.Port) string {
-	switch {
-	case ipsetName == "":
+func transformIPsetName(ipsetName string, sPort, dPort *firewall.Port, action firewall.Action) string {
+	if ipsetName == "" {
 		return ""
+	}
+
+	// Include action in the ipset name to prevent squashing rules with different actions
+	actionSuffix := ""
+	if action == firewall.ActionDrop {
+		actionSuffix = "-drop"
+	}
+
+	switch {
 	case sPort != nil && dPort != nil:
-		return ipsetName + "-sport-dport"
+		return ipsetName + "-sport-dport" + actionSuffix
 	case sPort != nil:
-		return ipsetName + "-sport"
+		return ipsetName + "-sport" + actionSuffix
 	case dPort != nil:
-		return ipsetName + "-dport"
+		return ipsetName + "-dport" + actionSuffix
 	default:
-		return ipsetName
+		return ipsetName + actionSuffix
 	}
 }

client/firewall/iptables/manager_linux_test.go

@@ -3,6 +3,7 @@ package iptables
 import (
 	"fmt"
 	"net/netip"
+	"strings"
 	"testing"
 	"time"
 
@@ -15,7 +16,7 @@ import (
 
 var ifaceMock = &iFaceMock{
 	NameFunc: func() string {
-		return "lo"
+		return "wg-test"
 	},
 	AddressFunc: func() wgaddr.Address {
 		return wgaddr.Address{
@@ -109,10 +110,84 @@ func TestIptablesManager(t *testing.T) {
 	})
 }
 
+func TestIptablesManagerDenyRules(t *testing.T) {
+	ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
+	require.NoError(t, err)
+
+	manager, err := Create(ifaceMock)
+	require.NoError(t, err)
+	require.NoError(t, manager.Init(nil))
+
+	defer func() {
+		err := manager.Close(nil)
+		require.NoError(t, err)
+	}()
+
+	t.Run("add deny rule", func(t *testing.T) {
+		ip := netip.MustParseAddr("10.20.0.3")
+		port := &fw.Port{Values: []uint16{22}}
+
+		rule, err := manager.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionDrop, "deny-ssh")
+		require.NoError(t, err, "failed to add deny rule")
+		require.NotEmpty(t, rule, "deny rule should not be empty")
+
+		// Verify the rule was added by checking iptables
+		for _, r := range rule {
+			rr := r.(*Rule)
+			checkRuleSpecs(t, ipv4Client, rr.chain, true, rr.specs...)
+		}
+	})
+
+	t.Run("deny rule precedence test", func(t *testing.T) {
+		ip := netip.MustParseAddr("10.20.0.4")
+		port := &fw.Port{Values: []uint16{80}}
+
+		// Add accept rule first
+		_, err := manager.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionAccept, "accept-http")
+		require.NoError(t, err, "failed to add accept rule")
+
+		// Add deny rule second for same IP/port - this should take precedence
+		_, err = manager.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionDrop, "deny-http")
+		require.NoError(t, err, "failed to add deny rule")
+
+		// Inspect the actual iptables rules to verify deny rule comes before accept rule
+		rules, err := ipv4Client.List("filter", chainNameInputRules)
+		require.NoError(t, err, "failed to list iptables rules")
+
+		// Debug: print all rules
+		t.Logf("All iptables rules in chain %s:", chainNameInputRules)
+		for i, rule := range rules {
+			t.Logf("  [%d] %s", i, rule)
+		}
+
+		var denyRuleIndex, acceptRuleIndex int = -1, -1
+		for i, rule := range rules {
+			if strings.Contains(rule, "DROP") {
+				t.Logf("Found DROP rule at index %d: %s", i, rule)
+				if strings.Contains(rule, "deny-http") && strings.Contains(rule, "80") {
+					denyRuleIndex = i
+				}
+			}
+			if strings.Contains(rule, "ACCEPT") {
+				t.Logf("Found ACCEPT rule at index %d: %s", i, rule)
+				if strings.Contains(rule, "accept-http") && strings.Contains(rule, "80") {
+					acceptRuleIndex = i
+				}
+			}
+		}
+
+		require.NotEqual(t, -1, denyRuleIndex, "deny rule should exist in iptables")
+		require.NotEqual(t, -1, acceptRuleIndex, "accept rule should exist in iptables")
+		require.Less(t, denyRuleIndex, acceptRuleIndex,
+			"deny rule should come before accept rule in iptables chain (deny at index %d, accept at index %d)",
+			denyRuleIndex, acceptRuleIndex)
+	})
+}
+
 func TestIptablesManagerIPSet(t *testing.T) {
 	mock := &iFaceMock{
 		NameFunc: func() string {
-			return "lo"
+			return "wg-test"
 		},
 		AddressFunc: func() wgaddr.Address {
 			return wgaddr.Address{
@@ -176,7 +251,7 @@ func checkRuleSpecs(t *testing.T, ipv4Client *iptables.IPTables, chainName strin
 func TestIptablesCreatePerformance(t *testing.T) {
 	mock := &iFaceMock{
 		NameFunc: func() string {
-			return "lo"
+			return "wg-test"
 		},
 		AddressFunc: func() wgaddr.Address {
 			return wgaddr.Address{

client/firewall/nftables/acl_linux.go

@@ -341,30 +341,38 @@ func (m *AclManager) addIOFiltering(
 	userData := []byte(ruleId)
 
 	chain := m.chainInputRules
-	nftRule := m.rConn.AddRule(&nftables.Rule{
+	rule := &nftables.Rule{
 		Table:    m.workTable,
 		Chain:    chain,
 		Exprs:    mainExpressions,
 		UserData: userData,
-	})
+	}
+
+	// Insert DROP rules at the beginning, append ACCEPT rules at the end
+	var nftRule *nftables.Rule
+	if action == firewall.ActionDrop {
+		nftRule = m.rConn.InsertRule(rule)
+	} else {
+		nftRule = m.rConn.AddRule(rule)
+	}
 
 	if err := m.rConn.Flush(); err != nil {
 		return nil, fmt.Errorf(flushError, err)
 	}
 
-	rule := &Rule{
+	ruleStruct := &Rule{
 		nftRule:    nftRule,
 		mangleRule: m.createPreroutingRule(expressions, userData),
 		nftSet:     ipset,
 		ruleID:     ruleId,
 		ip:         ip,
 	}
-	m.rules[ruleId] = rule
+	m.rules[ruleId] = ruleStruct
 	if ipset != nil {
 		m.ipsetStore.AddReferenceToIpset(ipset.Name)
 	}
 
-	return rule, nil
+	return ruleStruct, nil
 }
 
 func (m *AclManager) createPreroutingRule(expressions []expr.Any, userData []byte) *nftables.Rule {

client/firewall/nftables/manager_linux_test.go

@@ -2,6 +2,7 @@ package nftables
 
 import (
 	"bytes"
+	"encoding/binary"
 	"fmt"
 	"net/netip"
 	"os/exec"
@@ -20,7 +21,7 @@ import (
 
 var ifaceMock = &iFaceMock{
 	NameFunc: func() string {
-		return "lo"
+		return "wg-test"
 	},
 	AddressFunc: func() wgaddr.Address {
 		return wgaddr.Address{
@@ -103,9 +104,8 @@ func TestNftablesManager(t *testing.T) {
 			Kind: expr.VerdictAccept,
 		},
 	}
-	compareExprsIgnoringCounters(t, rules[0].Exprs, expectedExprs1)
-
-	expectedExprs2 := []expr.Any{
+	// Since DROP rules are inserted at position 0, the DROP rule comes first
+	expectedDropExprs := []expr.Any{
 		&expr.Payload{
 			DestRegister: 1,
 			Base:         expr.PayloadBaseNetworkHeader,
@@ -141,7 +141,12 @@ func TestNftablesManager(t *testing.T) {
 		},
 		&expr.Verdict{Kind: expr.VerdictDrop},
 	}
-	require.ElementsMatch(t, rules[1].Exprs, expectedExprs2, "expected the same expressions")
+
+	// Compare DROP rule at position 0 (inserted first due to InsertRule)
+	compareExprsIgnoringCounters(t, rules[0].Exprs, expectedDropExprs)
+
+	// Compare connection tracking rule at position 1 (pushed down by DROP rule insertion)
+	compareExprsIgnoringCounters(t, rules[1].Exprs, expectedExprs1)
 
 	for _, r := range rule {
 		err = manager.DeletePeerRule(r)
@@ -160,10 +165,90 @@ func TestNftablesManager(t *testing.T) {
 	require.NoError(t, err, "failed to reset")
 }
 
+func TestNftablesManagerRuleOrder(t *testing.T) {
+	// This test verifies rule insertion order in nftables peer ACLs
+	// We add accept rule first, then deny rule to test ordering behavior
+	manager, err := Create(ifaceMock)
+	require.NoError(t, err)
+	require.NoError(t, manager.Init(nil))
+
+	defer func() {
+		err = manager.Close(nil)
+		require.NoError(t, err)
+	}()
+
+	ip := netip.MustParseAddr("100.96.0.2").Unmap()
+	testClient := &nftables.Conn{}
+
+	// Add accept rule first
+	_, err = manager.AddPeerFiltering(nil, ip.AsSlice(), fw.ProtocolTCP, nil, &fw.Port{Values: []uint16{80}}, fw.ActionAccept, "accept-http")
+	require.NoError(t, err, "failed to add accept rule")
+
+	// Add deny rule second for the same traffic
+	_, err = manager.AddPeerFiltering(nil, ip.AsSlice(), fw.ProtocolTCP, nil, &fw.Port{Values: []uint16{80}}, fw.ActionDrop, "deny-http")
+	require.NoError(t, err, "failed to add deny rule")
+
+	err = manager.Flush()
+	require.NoError(t, err, "failed to flush")
+
+	rules, err := testClient.GetRules(manager.aclManager.workTable, manager.aclManager.chainInputRules)
+	require.NoError(t, err, "failed to get rules")
+
+	t.Logf("Found %d rules in nftables chain", len(rules))
+
+	// Find the accept and deny rules and verify deny comes before accept
+	var acceptRuleIndex, denyRuleIndex int = -1, -1
+	for i, rule := range rules {
+		hasAcceptHTTPSet := false
+		hasDenyHTTPSet := false
+		hasPort80 := false
+		var action string
+
+		for _, e := range rule.Exprs {
+			// Check for set lookup
+			if lookup, ok := e.(*expr.Lookup); ok {
+				if lookup.SetName == "accept-http" {
+					hasAcceptHTTPSet = true
+				} else if lookup.SetName == "deny-http" {
+					hasDenyHTTPSet = true
+				}
+			}
+			// Check for port 80
+			if cmp, ok := e.(*expr.Cmp); ok {
+				if cmp.Op == expr.CmpOpEq && len(cmp.Data) == 2 && binary.BigEndian.Uint16(cmp.Data) == 80 {
+					hasPort80 = true
+				}
+			}
+			// Check for verdict
+			if verdict, ok := e.(*expr.Verdict); ok {
+				if verdict.Kind == expr.VerdictAccept {
+					action = "ACCEPT"
+				} else if verdict.Kind == expr.VerdictDrop {
+					action = "DROP"
+				}
+			}
+		}
+
+		if hasAcceptHTTPSet && hasPort80 && action == "ACCEPT" {
+			t.Logf("Rule [%d]: accept-http set + Port 80 + ACCEPT", i)
+			acceptRuleIndex = i
+		} else if hasDenyHTTPSet && hasPort80 && action == "DROP" {
+			t.Logf("Rule [%d]: deny-http set + Port 80 + DROP", i)
+			denyRuleIndex = i
+		}
+	}
+
+	require.NotEqual(t, -1, acceptRuleIndex, "accept rule should exist in nftables")
+	require.NotEqual(t, -1, denyRuleIndex, "deny rule should exist in nftables")
+	require.Less(t, denyRuleIndex, acceptRuleIndex,
+		"deny rule should come before accept rule in nftables chain (deny at index %d, accept at index %d)",
+		denyRuleIndex, acceptRuleIndex)
+}
+
 func TestNFtablesCreatePerformance(t *testing.T) {
 	mock := &iFaceMock{
 		NameFunc: func() string {
-			return "lo"
+			return "wg-test"
 		},
 		AddressFunc: func() wgaddr.Address {
 			return wgaddr.Address{

client/firewall/uspfilter/allow_netbird.go

@@ -18,6 +18,7 @@ func (m *Manager) Close(stateManager *statemanager.Manager) error {
 	defer m.mutex.Unlock()
 
 	m.outgoingRules = make(map[netip.Addr]RuleSet)
+	m.incomingDenyRules = make(map[netip.Addr]RuleSet)
 	m.incomingRules = make(map[netip.Addr]RuleSet)
 
 	if m.udpTracker != nil {

client/firewall/uspfilter/allow_netbird_windows.go

@@ -27,6 +27,7 @@ func (m *Manager) Close(*statemanager.Manager) error {
 	defer m.mutex.Unlock()
 
 	m.outgoingRules = make(map[netip.Addr]RuleSet)
+	m.incomingDenyRules = make(map[netip.Addr]RuleSet)
 	m.incomingRules = make(map[netip.Addr]RuleSet)
 
 	if m.udpTracker != nil {