From 23b5d45b68c4caeb8bb493f58aed08aad665aa4d Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Wed, 18 Jun 2025 18:56:48 +0200 Subject: [PATCH] [client] Fix port range squashing (#4007) --- client/internal/acl/manager.go | 8 +- client/internal/acl/manager_test.go | 428 ++++++++++++++++++++++++++++ 2 files changed, 434 insertions(+), 2 deletions(-) diff --git a/client/internal/acl/manager.go b/client/internal/acl/manager.go index c8bc9123b..32dc7fbb8 100644 --- a/client/internal/acl/manager.go +++ b/client/internal/acl/manager.go @@ -398,11 +398,15 @@ func (d *DefaultManager) squashAcceptRules( // // We zeroed this to notify squash function that this protocol can't be squashed. addRuleToCalculationMap := func(i int, r *mgmProto.FirewallRule, protocols map[mgmProto.RuleProtocol]*protoMatch) { - drop := r.Action == mgmProto.RuleAction_DROP || r.Port != "" - if drop { + hasPortRestrictions := r.Action == mgmProto.RuleAction_DROP || + r.Port != "" || !portInfoEmpty(r.PortInfo) + + if hasPortRestrictions { + // Don't squash rules with port restrictions protocols[r.Protocol] = &protoMatch{ips: map[string]int{}} return } + if _, ok := protocols[r.Protocol]; !ok { protocols[r.Protocol] = &protoMatch{ ips: map[string]int{}, diff --git a/client/internal/acl/manager_test.go b/client/internal/acl/manager_test.go index 16620033e..b378de8c8 100644 --- a/client/internal/acl/manager_test.go +++ b/client/internal/acl/manager_test.go @@ -330,6 +330,434 @@ func TestDefaultManagerSquashRulesNoAffect(t *testing.T) { assert.Equal(t, len(networkMap.FirewallRules), len(rules)) } +func TestDefaultManagerSquashRulesWithPortRestrictions(t *testing.T) { + tests := []struct { + name string + rules []*mgmProto.FirewallRule + expectedCount int + description string + }{ + { + name: "should not squash rules with port ranges", + rules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: &mgmProto.PortInfo_Range{ + Start: 8080, + End: 8090, + }, + }, + }, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: &mgmProto.PortInfo_Range{ + Start: 8080, + End: 8090, + }, + }, + }, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: &mgmProto.PortInfo_Range{ + Start: 8080, + End: 8090, + }, + }, + }, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: &mgmProto.PortInfo_Range{ + Start: 8080, + End: 8090, + }, + }, + }, + }, + }, + expectedCount: 4, + description: "Rules with port ranges should not be squashed even if they cover all peers", + }, + { + name: "should not squash rules with specific ports", + rules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Port{ + Port: 80, + }, + }, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Port{ + Port: 80, + }, + }, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Port{ + Port: 80, + }, + }, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Port{ + Port: 80, + }, + }, + }, + }, + expectedCount: 4, + description: "Rules with specific ports should not be squashed even if they cover all peers", + }, + { + name: "should not squash rules with legacy port field", + rules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + }, + expectedCount: 4, + description: "Rules with legacy port field should not be squashed", + }, + { + name: "should not squash rules with DROP action", + rules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_DROP, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_DROP, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_DROP, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_DROP, + Protocol: mgmProto.RuleProtocol_TCP, + }, + }, + expectedCount: 4, + description: "Rules with DROP action should not be squashed", + }, + { + name: "should squash rules without port restrictions", + rules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + }, + }, + expectedCount: 1, + description: "Rules without port restrictions should be squashed into a single 0.0.0.0 rule", + }, + { + name: "mixed rules should not squash protocol with port restrictions", + rules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + PortInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Port{ + Port: 80, + }, + }, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + }, + }, + expectedCount: 4, + description: "TCP should not be squashed because one rule has port restrictions", + }, + { + name: "should squash UDP but not TCP when TCP has port restrictions", + rules: []*mgmProto.FirewallRule{ + // TCP rules with port restrictions - should NOT be squashed + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_TCP, + Port: "443", + }, + // UDP rules without port restrictions - SHOULD be squashed + { + PeerIP: "10.93.0.1", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_UDP, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_UDP, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_UDP, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.RuleDirection_IN, + Action: mgmProto.RuleAction_ACCEPT, + Protocol: mgmProto.RuleProtocol_UDP, + }, + }, + expectedCount: 5, // 4 TCP rules + 1 squashed UDP rule (0.0.0.0) + description: "UDP should be squashed to 0.0.0.0 rule, but TCP should remain as individual rules due to port restrictions", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + networkMap := &mgmProto.NetworkMap{ + RemotePeers: []*mgmProto.RemotePeerConfig{ + {AllowedIps: []string{"10.93.0.1"}}, + {AllowedIps: []string{"10.93.0.2"}}, + {AllowedIps: []string{"10.93.0.3"}}, + {AllowedIps: []string{"10.93.0.4"}}, + }, + FirewallRules: tt.rules, + } + + manager := &DefaultManager{} + rules, _ := manager.squashAcceptRules(networkMap) + + assert.Equal(t, tt.expectedCount, len(rules), tt.description) + + // For squashed rules, verify we get the expected 0.0.0.0 rule + if tt.expectedCount == 1 { + assert.Equal(t, "0.0.0.0", rules[0].PeerIP) + assert.Equal(t, mgmProto.RuleDirection_IN, rules[0].Direction) + assert.Equal(t, mgmProto.RuleAction_ACCEPT, rules[0].Action) + } + }) + } +} + +func TestPortInfoEmpty(t *testing.T) { + tests := []struct { + name string + portInfo *mgmProto.PortInfo + expected bool + }{ + { + name: "nil PortInfo should be empty", + portInfo: nil, + expected: true, + }, + { + name: "PortInfo with zero port should be empty", + portInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Port{ + Port: 0, + }, + }, + expected: true, + }, + { + name: "PortInfo with valid port should not be empty", + portInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Port{ + Port: 80, + }, + }, + expected: false, + }, + { + name: "PortInfo with nil range should be empty", + portInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: nil, + }, + }, + expected: true, + }, + { + name: "PortInfo with zero start range should be empty", + portInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: &mgmProto.PortInfo_Range{ + Start: 0, + End: 100, + }, + }, + }, + expected: true, + }, + { + name: "PortInfo with zero end range should be empty", + portInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: &mgmProto.PortInfo_Range{ + Start: 80, + End: 0, + }, + }, + }, + expected: true, + }, + { + name: "PortInfo with valid range should not be empty", + portInfo: &mgmProto.PortInfo{ + PortSelection: &mgmProto.PortInfo_Range_{ + Range: &mgmProto.PortInfo_Range{ + Start: 8080, + End: 8090, + }, + }, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := portInfoEmpty(tt.portInfo) + assert.Equal(t, tt.expected, result) + }) + } +} + func TestDefaultManagerEnableSSHRules(t *testing.T) { networkMap := &mgmProto.NetworkMap{ PeerConfig: &mgmProto.PeerConfig{