Skip to content

Commit e31cf38

Browse files
littlespacelittlespace
committed
[caclmgrd] Fix EXTERNAL_CLIENT ACL: duplicate rules, multi-port, and stale state
Fix duplicate iptables/ip6tables rules for EXTERNAL_CLIENT Control Plane ACL. The rule-emission loop could append identical full commands (ns-prefix + rule) multiple times. Fix builds the full command first and uses a per-table shadow set (acl_cmds_seen) for O(1) duplicate detection. iptables_cmds stays a list to preserve rule ordering. Fix single-port limitation for EXTERNAL_CLIENT ACL: previously each rule overwrote dst_ports with a single-element list, so only the last rule's port was used. Fix accumulates ports via append-if-not-present so all configured ports generate iptables rules. Fix ACL_SERVICES class dict pollution: dst_ports was written back into the class-level ACL_SERVICES dict on every call, causing stale ports from deleted rules to persist across reloads until process restart. dst_ports is now kept strictly local to each call. Fix int/str type mismatch in dst_ports: L4_DST_PORT_RANGE expansion now appends str(port) to stay type-consistent with L4_DST_PORT (which is a string from Redis). The mixed-type list silently broke the duplicate check producing duplicate --dport rules when a port appeared in both a range and an individual rule. Fix acl_cmds_seen scope: the dedup shadow set was reset per service, so duplicate commands generated by two services on the same ACL table were not caught. Moved to per-table scope. Fix ip2me block rules using network address instead of interface IP for non-/32 non-VLAN interfaces. ip_ntwrk.network_address returned the network base address (e.g. 10.0.1.0 for 10.0.1.2/24) instead of the interface's own IP. Fix uses ipaddress.ip_interface(iface_cidr).ip. Fix empty management IP string passed to iptables -s/-d arguments in generate_allow_internal_docker_ip_traffic_commands when get_namespace_mgmt_ip fails. Guard added to return early with a warning when the management IP is unavailable. Tests: - Add dedup assertion to existing parameterized test harness - Add vector: IPv4 multi-port + src-ip - Add vector: L4_DST_PORT_RANGE + overlapping L4_DST_PORT - Add vector: service listed twice on same table - Add standalone two-call reload test: port deleted between reloads must not leak stale rules - Add vector: non-/32 non-VLAN interfaces block interface IP not network address - Add standalone test: empty management IP returns empty list without crash Signed-off-by: littlespace <littlespace@users.noreply.github.com>
1 parent f24efde commit e31cf38

5 files changed

Lines changed: 1024 additions & 140 deletions

‎scripts/caclmgrd‎

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,9 @@ class ControlPlaneAclManager(logger.Logger):
306306

307307
# For VLAN interfaces, the IP address we want to block is the default gateway (i.e.,
308308
# the first available host IP address of the VLAN subnet)
309-
ip_addr = next(ip_ntwrk.hosts()) if iface_table_name == "VLAN_INTERFACE" else ip_ntwrk.network_address
309+
# Use ip_interface to get the actual interface IP (not the network address),
310+
# which would be wrong for non-/32 non-VLAN interfaces (e.g. /24 gives 10.0.0.0).
311+
ip_addr = next(ip_ntwrk.hosts()) if iface_table_name == "VLAN_INTERFACE" else ipaddress.ip_interface(iface_cidr).ip
310312

311313
if isinstance(ip_ntwrk, ipaddress.IPv4Network):
312314
block_ip2me_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-d', '{}/{}'.format(ip_addr, ip_ntwrk.max_prefixlen), '-j', 'DROP'])
@@ -373,13 +375,19 @@ class ControlPlaneAclManager(logger.Logger):
373375
allow_internal_docker_ip_cmds = []
374376

375377
if namespace:
376-
# For namespace docker allow local communication on docker management ip for all proto
377-
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-s', self.namespace_docker_mgmt_ip[namespace], '-d', self.namespace_docker_mgmt_ip[namespace], '-j', 'ACCEPT'])
378-
379-
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-s', self.namespace_docker_mgmt_ipv6[namespace], '-d', self.namespace_docker_mgmt_ipv6[namespace], '-j', 'ACCEPT'])
380-
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-s', self.namespace_mgmt_ip, '-d', self.namespace_docker_mgmt_ip[namespace], '-j', 'ACCEPT'])
381-
382-
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-s', self.namespace_mgmt_ipv6, '-d', self.namespace_docker_mgmt_ipv6[namespace], '-j', 'ACCEPT'])
378+
# Guard each IP version independently: skip if either IP is unavailable
379+
# to prevent empty strings being passed as iptables -s/-d arguments.
380+
if self.namespace_docker_mgmt_ip.get(namespace) and self.namespace_mgmt_ip:
381+
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-s', self.namespace_docker_mgmt_ip[namespace], '-d', self.namespace_docker_mgmt_ip[namespace], '-j', 'ACCEPT'])
382+
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-s', self.namespace_mgmt_ip, '-d', self.namespace_docker_mgmt_ip[namespace], '-j', 'ACCEPT'])
383+
else:
384+
self.log_warning("Skipping IPv4 internal docker traffic rules for namespace '{}': IPv4 management IP unavailable".format(namespace))
385+
386+
if self.namespace_docker_mgmt_ipv6.get(namespace) and self.namespace_mgmt_ipv6:
387+
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-s', self.namespace_docker_mgmt_ipv6[namespace], '-d', self.namespace_docker_mgmt_ipv6[namespace], '-j', 'ACCEPT'])
388+
allow_internal_docker_ip_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-s', self.namespace_mgmt_ipv6, '-d', self.namespace_docker_mgmt_ipv6[namespace], '-j', 'ACCEPT'])
389+
else:
390+
self.log_warning("Skipping IPv6 internal docker traffic rules for namespace '{}': IPv6 management IP unavailable".format(namespace))
383391

384392
else:
385393

@@ -742,6 +750,11 @@ class ControlPlaneAclManager(logger.Logger):
742750

743751
acl_services = table_data["services"]
744752

753+
# Shadow set for O(1) duplicate detection across all services in this table.
754+
# Scoped per table (not per service) so cross-service duplicate commands are
755+
# also caught. iptables_cmds stays a list to preserve rule ordering.
756+
acl_cmds_seen = set()
757+
745758
for acl_service in acl_services:
746759
if acl_service not in self.ACL_SERVICES:
747760
self.log_warning("Ignoring control plane ACL '{}' with unrecognized service '{}'"
@@ -782,18 +795,26 @@ class ControlPlaneAclManager(logger.Logger):
782795
elif self.is_rule_ipv4(rule_props):
783796
table_ip_version = 4
784797

785-
# Read DST_PORT info from Config DB, insert it back to ACL_SERVICES
798+
# Read DST_PORT info from Config DB into the local dst_ports list.
799+
# Do NOT write back into ACL_SERVICES (class-level dict) — doing so
800+
# would pollute subsequent reloads with stale ports from deleted rules.
786801
if acl_service == 'EXTERNAL_CLIENT' and "L4_DST_PORT" in rule_props:
787-
dst_ports = [rule_props["L4_DST_PORT"]]
788-
self.ACL_SERVICES[acl_service]["dst_ports"] = dst_ports
802+
if rule_props["L4_DST_PORT"] not in dst_ports:
803+
dst_ports.append(rule_props["L4_DST_PORT"])
789804
elif acl_service == 'EXTERNAL_CLIENT' and "L4_DST_PORT_RANGE" in rule_props:
790-
dst_ports = []
791-
port_ranges = rule_props["L4_DST_PORT_RANGE"].split("-")
792-
port_start = int(port_ranges[0])
793-
port_end = int(port_ranges[1])
805+
try:
806+
port_ranges = rule_props["L4_DST_PORT_RANGE"].split("-")
807+
port_start = int(port_ranges[0])
808+
port_end = int(port_ranges[1])
809+
except (ValueError, IndexError):
810+
self.log_error("Invalid L4_DST_PORT_RANGE value '{}' in ACL table '{}'. Skipping.".format(
811+
rule_props["L4_DST_PORT_RANGE"], table_name))
812+
continue
794813
for port in range(port_start, port_end + 1):
795-
dst_ports.append(port)
796-
self.ACL_SERVICES[acl_service]["dst_ports"] = dst_ports
814+
# Use str() to keep dst_ports type-consistent with L4_DST_PORT
815+
port_str = str(port)
816+
if port_str not in dst_ports:
817+
dst_ports.append(port_str)
797818

798819
if (self.is_rule_ipv6(rule_props) and (table_ip_version == 4)):
799820
self.log_error("CtrlPlane ACL table {} is a IPv4 based table and rule {} is a IPV6 rule! Ignoring rule."
@@ -869,8 +890,14 @@ class ControlPlaneAclManager(logger.Logger):
869890
# Append the packet action as the jump target
870891
rule_cmd += ["-j", "{}".format(rule_props["PACKET_ACTION"])]
871892

872-
iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + rule_cmd)
873-
num_ctrl_plane_acl_rules += 1
893+
# Deduplicate using a shadow set for O(1) lookup.
894+
# iptables_cmds stays a list to preserve rule ordering.
895+
full_cmd = self.iptables_cmd_ns_prefix[namespace] + rule_cmd
896+
full_cmd_key = tuple(full_cmd)
897+
if full_cmd_key not in acl_cmds_seen:
898+
acl_cmds_seen.add(full_cmd_key)
899+
iptables_cmds.append(full_cmd)
900+
num_ctrl_plane_acl_rules += 1
874901

875902

876903
service_to_source_ip_map.update({ acl_service:{ "ipv4":ipv4_src_ip_set, "ipv6":ipv6_src_ip_set } })

‎tests/caclmgrd/caclmgrd_external_client_acl_test.py‎

Lines changed: 162 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,44 +11,188 @@
1111
from tests.common.mock_configdb import MockConfigDb
1212

1313

14-
DBCONFIG_PATH = '/var/run/redis/sonic-db/database_config.json'
14+
DBCONFIG_PATH = "/var/run/redis/sonic-db/database_config.json"
1515

1616

1717
class TestCaclmgrdExternalClientAcl(TestCase):
1818
"""
19-
Test caclmgrd EXTERNAL_CLIENT_ACL
19+
Test caclmgrd EXTERNAL_CLIENT_ACL
2020
"""
21+
2122
def setUp(self):
2223
swsscommon.ConfigDBConnector = MockConfigDb
2324
test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
2425
modules_path = os.path.dirname(test_path)
2526
scripts_path = os.path.join(modules_path, "scripts")
2627
sys.path.insert(0, modules_path)
27-
caclmgrd_path = os.path.join(scripts_path, 'caclmgrd')
28-
self.caclmgrd = load_module_from_source('caclmgrd', caclmgrd_path)
28+
caclmgrd_path = os.path.join(scripts_path, "caclmgrd")
29+
self.caclmgrd = load_module_from_source("caclmgrd", caclmgrd_path)
2930

3031
@parameterized.expand(EXTERNAL_CLIENT_ACL_TEST_VECTOR)
3132
@patchfs
3233
def test_caclmgrd_external_client_acl(self, test_name, test_data, fs):
3334
if not os.path.exists(DBCONFIG_PATH):
34-
fs.create_file(DBCONFIG_PATH) # fake database_config.json
35+
fs.create_file(DBCONFIG_PATH) # fake database_config.json
3536

3637
MockConfigDb.set_config_db(test_data["config_db"])
3738
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
3839
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()
39-
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(return_value=[])
40-
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(return_value=["INPUT", "FORWARD", "OUTPUT"])
41-
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = mock.MagicMock(return_value='')
40+
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(
41+
return_value=[]
42+
)
43+
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(
44+
return_value=["INPUT", "FORWARD", "OUTPUT"]
45+
)
46+
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = (
47+
mock.MagicMock(return_value="")
48+
)
4249
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
4350

44-
iptables_rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb())
45-
test_data['return'] = [tuple(i) for i in test_data['return']]
51+
iptables_rules_ret, _ = (
52+
caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
53+
"", MockConfigDb()
54+
)
55+
)
56+
test_data["return"] = [tuple(i) for i in test_data["return"]]
4657
iptables_rules_ret = [tuple(i) for i in iptables_rules_ret]
47-
self.assertEqual(set(test_data["return"]).issubset(set(iptables_rules_ret)), True)
48-
caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = ['ip', 'netns', 'exec', 'asic0']
49-
caclmgrd_daemon.namespace_docker_mgmt_ip['asic0'] = '1.1.1.1'
50-
caclmgrd_daemon.namespace_mgmt_ip = '2.2.2.2'
51-
caclmgrd_daemon.namespace_docker_mgmt_ipv6['asic0'] = 'fd::01'
52-
caclmgrd_daemon.namespace_mgmt_ipv6 = 'fd::02'
53-
54-
_ = caclmgrd_daemon.generate_fwd_traffic_from_namespace_to_host_commands('asic0', None)
58+
self.assertEqual(
59+
set(test_data["return"]).issubset(set(iptables_rules_ret)), True
60+
)
61+
# Assert no duplicate iptables rules are emitted (SONIC-103)
62+
self.assertEqual(len(iptables_rules_ret), len(set(iptables_rules_ret)))
63+
caclmgrd_daemon.iptables_cmd_ns_prefix["asic0"] = [
64+
"ip",
65+
"netns",
66+
"exec",
67+
"asic0",
68+
]
69+
caclmgrd_daemon.namespace_docker_mgmt_ip["asic0"] = "1.1.1.1"
70+
caclmgrd_daemon.namespace_mgmt_ip = "2.2.2.2"
71+
caclmgrd_daemon.namespace_docker_mgmt_ipv6["asic0"] = "fd::01"
72+
caclmgrd_daemon.namespace_mgmt_ipv6 = "fd::02"
73+
74+
_ = caclmgrd_daemon.generate_fwd_traffic_from_namespace_to_host_commands(
75+
"asic0", None
76+
)
77+
78+
@patchfs
79+
def test_acl_services_not_polluted_across_reloads(self, fs):
80+
"""
81+
BUG 2 regression test: ACL_SERVICES class-level dict must not be mutated
82+
with dst_ports from a previous call. If it were, a port deleted from
83+
Config DB between two reloads would still appear in the second call's
84+
iptables rules (stale port leak).
85+
86+
Scenario:
87+
- First call: EXTERNAL_CLIENT table has rules for ports 8081 and 8082.
88+
- Second call (simulated reload): only port 8081 remains.
89+
- Expected: second call produces no rules for port 8082.
90+
"""
91+
if not os.path.exists(DBCONFIG_PATH):
92+
fs.create_file(DBCONFIG_PATH)
93+
94+
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
95+
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()
96+
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(
97+
return_value=[]
98+
)
99+
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(
100+
return_value=["INPUT", "FORWARD", "OUTPUT"]
101+
)
102+
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = (
103+
mock.MagicMock(return_value="")
104+
)
105+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
106+
107+
# First reload: ports 8081 and 8082 both present
108+
config_first = {
109+
"ACL_TABLE": {
110+
"EXTERNAL_CLIENT_ACL": {
111+
"stage": "INGRESS",
112+
"type": "CTRLPLANE",
113+
"services": ["EXTERNAL_CLIENT"],
114+
}
115+
},
116+
"ACL_RULE": {
117+
"EXTERNAL_CLIENT_ACL|DEFAULT_RULE": {
118+
"ETHER_TYPE": "2048",
119+
"PACKET_ACTION": "DROP",
120+
"PRIORITY": "1",
121+
},
122+
"EXTERNAL_CLIENT_ACL|RULE_1": {
123+
"L4_DST_PORT": "8081",
124+
"PACKET_ACTION": "ACCEPT",
125+
"PRIORITY": "9998",
126+
"SRC_IP": "10.0.0.1/32",
127+
},
128+
"EXTERNAL_CLIENT_ACL|RULE_2": {
129+
"L4_DST_PORT": "8082",
130+
"PACKET_ACTION": "ACCEPT",
131+
"PRIORITY": "9997",
132+
"SRC_IP": "10.0.0.1/32",
133+
},
134+
},
135+
"DEVICE_METADATA": {"localhost": {}},
136+
"FEATURE": {},
137+
}
138+
MockConfigDb.set_config_db(config_first)
139+
rules_first, _ = (
140+
caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
141+
"", MockConfigDb()
142+
)
143+
)
144+
rules_first = [tuple(r) for r in rules_first]
145+
self.assertIn(
146+
("iptables", "-A", "INPUT", "-p", "tcp", "--dport", "8081", "-j", "DROP"),
147+
rules_first,
148+
)
149+
self.assertIn(
150+
("iptables", "-A", "INPUT", "-p", "tcp", "--dport", "8082", "-j", "DROP"),
151+
rules_first,
152+
)
153+
154+
# Second reload: RULE_2 (port 8082) deleted from Config DB
155+
config_second = {
156+
"ACL_TABLE": {
157+
"EXTERNAL_CLIENT_ACL": {
158+
"stage": "INGRESS",
159+
"type": "CTRLPLANE",
160+
"services": ["EXTERNAL_CLIENT"],
161+
}
162+
},
163+
"ACL_RULE": {
164+
"EXTERNAL_CLIENT_ACL|DEFAULT_RULE": {
165+
"ETHER_TYPE": "2048",
166+
"PACKET_ACTION": "DROP",
167+
"PRIORITY": "1",
168+
},
169+
"EXTERNAL_CLIENT_ACL|RULE_1": {
170+
"L4_DST_PORT": "8081",
171+
"PACKET_ACTION": "ACCEPT",
172+
"PRIORITY": "9998",
173+
"SRC_IP": "10.0.0.1/32",
174+
},
175+
},
176+
"DEVICE_METADATA": {"localhost": {}},
177+
"FEATURE": {},
178+
}
179+
MockConfigDb.set_config_db(config_second)
180+
rules_second, _ = (
181+
caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
182+
"", MockConfigDb()
183+
)
184+
)
185+
rules_second = [tuple(r) for r in rules_second]
186+
187+
# Port 8081 must still be present
188+
self.assertIn(
189+
("iptables", "-A", "INPUT", "-p", "tcp", "--dport", "8081", "-j", "DROP"),
190+
rules_second,
191+
)
192+
# Port 8082 must NOT appear — it was deleted and ACL_SERVICES must not carry stale state
193+
port_8082_rules = [r for r in rules_second if "--dport" in r and "8082" in r]
194+
self.assertEqual(
195+
port_8082_rules,
196+
[],
197+
"Stale port 8082 rules found after reload — ACL_SERVICES class dict was polluted",
198+
)

0 commit comments

Comments
 (0)