Skip to content

[caclmgrd] Fix EXTERNAL_CLIENT ACL: duplicate rules, multi-port, and stale state #378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
littlespace wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[caclmgrd] Fix EXTERNAL_CLIENT ACL: duplicate rules, multi-port, and stale state #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 46 additions & 19 deletions scripts/caclmgrd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,9 @@ class ControlPlaneAclManager(logger.Logger):

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

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

if namespace:
# For namespace docker allow local communication on docker management ip for all proto
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'])

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'])
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'])

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'])
# Guard each IP version independently: skip if either IP is unavailable
# to prevent empty strings being passed as iptables -s/-d arguments.
if self.namespace_docker_mgmt_ip.get(namespace) and self.namespace_mgmt_ip:
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'])
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'])
else:
self.log_warning("Skipping IPv4 internal docker traffic rules for namespace '{}': IPv4 management IP unavailable".format(namespace))

if self.namespace_docker_mgmt_ipv6.get(namespace) and self.namespace_mgmt_ipv6:
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'])
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'])
else:
self.log_warning("Skipping IPv6 internal docker traffic rules for namespace '{}': IPv6 management IP unavailable".format(namespace))

else:

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

acl_services = table_data["services"]

# Shadow set for O(1) duplicate detection across all services in this table.
# Scoped per table (not per service) so cross-service duplicate commands are
# also caught. iptables_cmds stays a list to preserve rule ordering.
acl_cmds_seen = set()

for acl_service in acl_services:
if acl_service not in self.ACL_SERVICES:
self.log_warning("Ignoring control plane ACL '{}' with unrecognized service '{}'"
Expand Down Expand Up @@ -782,18 +795,26 @@ class ControlPlaneAclManager(logger.Logger):
elif self.is_rule_ipv4(rule_props):
table_ip_version = 4

# Read DST_PORT info from Config DB, insert it back to ACL_SERVICES
# Read DST_PORT info from Config DB into the local dst_ports list.
# Do NOT write back into ACL_SERVICES (class-level dict) — doing so
# would pollute subsequent reloads with stale ports from deleted rules.
if acl_service == 'EXTERNAL_CLIENT' and "L4_DST_PORT" in rule_props:
dst_ports = [rule_props["L4_DST_PORT"]]
self.ACL_SERVICES[acl_service]["dst_ports"] = dst_ports
if rule_props["L4_DST_PORT"] not in dst_ports:
dst_ports.append(rule_props["L4_DST_PORT"])
elif acl_service == 'EXTERNAL_CLIENT' and "L4_DST_PORT_RANGE" in rule_props:
dst_ports = []
port_ranges = rule_props["L4_DST_PORT_RANGE"].split("-")
port_start = int(port_ranges[0])
port_end = int(port_ranges[1])
try:
port_ranges = rule_props["L4_DST_PORT_RANGE"].split("-")
port_start = int(port_ranges[0])
port_end = int(port_ranges[1])
except (ValueError, IndexError):
self.log_error("Invalid L4_DST_PORT_RANGE value '{}' in ACL table '{}'. Skipping.".format(
rule_props["L4_DST_PORT_RANGE"], table_name))
continue
for port in range(port_start, port_end + 1):
dst_ports.append(port)
self.ACL_SERVICES[acl_service]["dst_ports"] = dst_ports
# Use str() to keep dst_ports type-consistent with L4_DST_PORT
port_str = str(port)
if port_str not in dst_ports:
dst_ports.append(port_str)

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

iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + rule_cmd)
num_ctrl_plane_acl_rules += 1
# Deduplicate using a shadow set for O(1) lookup.
# iptables_cmds stays a list to preserve rule ordering.
full_cmd = self.iptables_cmd_ns_prefix[namespace] + rule_cmd
full_cmd_key = tuple(full_cmd)
if full_cmd_key not in acl_cmds_seen:
acl_cmds_seen.add(full_cmd_key)
iptables_cmds.append(full_cmd)
num_ctrl_plane_acl_rules += 1


service_to_source_ip_map.update({ acl_service:{ "ipv4":ipv4_src_ip_set, "ipv6":ipv6_src_ip_set } })
Expand Down
180 changes: 162 additions & 18 deletions tests/caclmgrd/caclmgrd_external_client_acl_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,44 +11,188 @@
from tests.common.mock_configdb import MockConfigDb


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


class TestCaclmgrdExternalClientAcl(TestCase):
"""
Test caclmgrd EXTERNAL_CLIENT_ACL
Test caclmgrd EXTERNAL_CLIENT_ACL
"""

def setUp(self):
swsscommon.ConfigDBConnector = MockConfigDb
test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
modules_path = os.path.dirname(test_path)
scripts_path = os.path.join(modules_path, "scripts")
sys.path.insert(0, modules_path)
caclmgrd_path = os.path.join(scripts_path, 'caclmgrd')
self.caclmgrd = load_module_from_source('caclmgrd', caclmgrd_path)
caclmgrd_path = os.path.join(scripts_path, "caclmgrd")
self.caclmgrd = load_module_from_source("caclmgrd", caclmgrd_path)

@parameterized.expand(EXTERNAL_CLIENT_ACL_TEST_VECTOR)
@patchfs
def test_caclmgrd_external_client_acl(self, test_name, test_data, fs):
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH) # fake database_config.json
fs.create_file(DBCONFIG_PATH) # fake database_config.json

MockConfigDb.set_config_db(test_data["config_db"])
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(return_value=[])
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(return_value=["INPUT", "FORWARD", "OUTPUT"])
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = mock.MagicMock(return_value='')
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(
return_value=[]
)
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(
return_value=["INPUT", "FORWARD", "OUTPUT"]
)
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = (
mock.MagicMock(return_value="")
)
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")

iptables_rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb())
test_data['return'] = [tuple(i) for i in test_data['return']]
iptables_rules_ret, _ = (
caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
"", MockConfigDb()
)
)
test_data["return"] = [tuple(i) for i in test_data["return"]]
iptables_rules_ret = [tuple(i) for i in iptables_rules_ret]
self.assertEqual(set(test_data["return"]).issubset(set(iptables_rules_ret)), True)
caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = ['ip', 'netns', 'exec', 'asic0']
caclmgrd_daemon.namespace_docker_mgmt_ip['asic0'] = '1.1.1.1'
caclmgrd_daemon.namespace_mgmt_ip = '2.2.2.2'
caclmgrd_daemon.namespace_docker_mgmt_ipv6['asic0'] = 'fd::01'
caclmgrd_daemon.namespace_mgmt_ipv6 = 'fd::02'

_ = caclmgrd_daemon.generate_fwd_traffic_from_namespace_to_host_commands('asic0', None)
self.assertEqual(
set(test_data["return"]).issubset(set(iptables_rules_ret)), True
)
# Assert no duplicate iptables rules are emitted (SONIC-103)
self.assertEqual(len(iptables_rules_ret), len(set(iptables_rules_ret)))
caclmgrd_daemon.iptables_cmd_ns_prefix["asic0"] = [
"ip",
"netns",
"exec",
"asic0",
]
caclmgrd_daemon.namespace_docker_mgmt_ip["asic0"] = "1.1.1.1"
caclmgrd_daemon.namespace_mgmt_ip = "2.2.2.2"
caclmgrd_daemon.namespace_docker_mgmt_ipv6["asic0"] = "fd::01"
caclmgrd_daemon.namespace_mgmt_ipv6 = "fd::02"

_ = caclmgrd_daemon.generate_fwd_traffic_from_namespace_to_host_commands(
"asic0", None
)

@patchfs
def test_acl_services_not_polluted_across_reloads(self, fs):
"""
BUG 2 regression test: ACL_SERVICES class-level dict must not be mutated
with dst_ports from a previous call. If it were, a port deleted from
Config DB between two reloads would still appear in the second call's
iptables rules (stale port leak).

Scenario:
- First call: EXTERNAL_CLIENT table has rules for ports 8081 and 8082.
- Second call (simulated reload): only port 8081 remains.
- Expected: second call produces no rules for port 8082.
"""
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH)

self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(
return_value=[]
)
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(
return_value=["INPUT", "FORWARD", "OUTPUT"]
)
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = (
mock.MagicMock(return_value="")
)
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")

# First reload: ports 8081 and 8082 both present
config_first = {
"ACL_TABLE": {
"EXTERNAL_CLIENT_ACL": {
"stage": "INGRESS",
"type": "CTRLPLANE",
"services": ["EXTERNAL_CLIENT"],
}
},
"ACL_RULE": {
"EXTERNAL_CLIENT_ACL|DEFAULT_RULE": {
"ETHER_TYPE": "2048",
"PACKET_ACTION": "DROP",
"PRIORITY": "1",
},
"EXTERNAL_CLIENT_ACL|RULE_1": {
"L4_DST_PORT": "8081",
"PACKET_ACTION": "ACCEPT",
"PRIORITY": "9998",
"SRC_IP": "10.0.0.1/32",
},
"EXTERNAL_CLIENT_ACL|RULE_2": {
"L4_DST_PORT": "8082",
"PACKET_ACTION": "ACCEPT",
"PRIORITY": "9997",
"SRC_IP": "10.0.0.1/32",
},
},
"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {},
}
MockConfigDb.set_config_db(config_first)
rules_first, _ = (
caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
"", MockConfigDb()
)
)
rules_first = [tuple(r) for r in rules_first]
self.assertIn(
("iptables", "-A", "INPUT", "-p", "tcp", "--dport", "8081", "-j", "DROP"),
rules_first,
)
self.assertIn(
("iptables", "-A", "INPUT", "-p", "tcp", "--dport", "8082", "-j", "DROP"),
rules_first,
)

# Second reload: RULE_2 (port 8082) deleted from Config DB
config_second = {
"ACL_TABLE": {
"EXTERNAL_CLIENT_ACL": {
"stage": "INGRESS",
"type": "CTRLPLANE",
"services": ["EXTERNAL_CLIENT"],
}
},
"ACL_RULE": {
"EXTERNAL_CLIENT_ACL|DEFAULT_RULE": {
"ETHER_TYPE": "2048",
"PACKET_ACTION": "DROP",
"PRIORITY": "1",
},
"EXTERNAL_CLIENT_ACL|RULE_1": {
"L4_DST_PORT": "8081",
"PACKET_ACTION": "ACCEPT",
"PRIORITY": "9998",
"SRC_IP": "10.0.0.1/32",
},
},
"DEVICE_METADATA": {"localhost": {}},
"FEATURE": {},
}
MockConfigDb.set_config_db(config_second)
rules_second, _ = (
caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
"", MockConfigDb()
)
)
rules_second = [tuple(r) for r in rules_second]

# Port 8081 must still be present
self.assertIn(
("iptables", "-A", "INPUT", "-p", "tcp", "--dport", "8081", "-j", "DROP"),
rules_second,
)
# Port 8082 must NOT appear — it was deleted and ACL_SERVICES must not carry stale state
port_8082_rules = [r for r in rules_second if "--dport" in r and "8082" in r]
self.assertEqual(
port_8082_rules,
[],
"Stale port 8082 rules found after reload — ACL_SERVICES class dict was polluted",
)
Loading
Loading