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

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{