From 80744ee746d4cb146bcd47c89a2d1e83fcbdf7cd Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Fri, 19 Jun 2026 07:59:56 +0000 Subject: [PATCH] [caclmgrd]: Preserve externally-owned INPUT rules across CACL rebuild caclmgrd owns the host INPUT chain and flushes/rebuilds it from CONFIG_DB on every control-plane ACL change. Rules installed by OTHER services live only in the kernel and are silently wiped by that rebuild. The concrete case is the dhcp_server container docker0 syslog (UDP 514) ACCEPT (sonic-net/sonic-buildimage#27584): after any CACL update it is dropped until dhcp_server restarts, breaking syslog from the bridge-mode container. Make caclmgrd not clobber externally-owned INPUT rules. caclmgrd never comments its own rules, so an iptables --comment is an unambiguous marker of external ownership. Before the flush, snapshot any INPUT rule whose --comment is in an explicit ignore list (IGNORED_INPUT_RULE_COMMENTS, currently just "dhcp_server_syslog") and re-add it verbatim early in the rebuild -- before the ip2me and catch-all DROPs. The INPUT policy is ACCEPT throughout the rebuild and the catch-all DROP is appended last, so there is no window where the rule is missing while a DROP exists; no syslog is lost. The owning service keeps full lifecycle control: caclmgrd never creates or deletes the rule, it only mirrors it, and re-adds it byte-identically so the container own -C/-D keep matching. Host namespace / IPv4 only. No change to the dhcp_server container. Fixes sonic-net/sonic-buildimage#27584 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xichen96 --- scripts/caclmgrd | 54 +++++++ .../caclmgrd_preserved_input_rules_test.py | 153 ++++++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 tests/caclmgrd/caclmgrd_preserved_input_rules_test.py diff --git a/scripts/caclmgrd b/scripts/caclmgrd index ef31da1f..cb18e406 100755 --- a/scripts/caclmgrd +++ b/scripts/caclmgrd @@ -13,6 +13,7 @@ try: import ipaddress import os + import shlex import subprocess import sys import threading @@ -90,6 +91,12 @@ class ControlPlaneAclManager(logger.Logger): DPU_TABLE = "DPU" MID_PLANE_BRIDGE_TABLE = "MID_PLANE_BRIDGE" + # iptables --comment markers for INPUT rules owned by other services. caclmgrd + # never comments its own rules, so a matching comment marks an external rule to + # preserve across the flush-and-rebuild (the dhcp_server docker0 syslog rule, + # sonic-net/sonic-buildimage#27584). + IGNORED_INPUT_RULE_COMMENTS = ["dhcp_server_syslog"] + # To specify a port range instead of a single port, use iptables format: # separate start and end ports with a colon, e.g., "1000:2000" ACL_SERVICES = { @@ -642,6 +649,12 @@ class ControlPlaneAclManager(logger.Logger): iptables_cmds = [] service_to_source_ip_map = {} + # Snapshot externally-owned INPUT rules before the flush wipes them, to + # re-add below before any DROP. Host namespace only. + preserved_input_rules = [] + if namespace == DEFAULT_NAMESPACE: + preserved_input_rules = self.get_preserved_input_rules(namespace) + # First, add iptables commands to set default policies to accept all # traffic. In case we are connected remotely, the connection will not # drop when we flush the current rules @@ -668,6 +681,10 @@ class ControlPlaneAclManager(logger.Logger): iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-s', '127.0.0.1', '-i', 'lo', '-j', 'ACCEPT']) iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-s', '::1', '-i', 'lo', '-j', 'ACCEPT']) + # Re-add preserved external rules here -- before any DROP, while the + # policy is still ACCEPT -- so the rebuild drops no syslog. + iptables_cmds += preserved_input_rules + if self.bfdAllowed: iptables_cmds += self.get_bfd_iptable_commands(namespace) @@ -1071,6 +1088,43 @@ class ControlPlaneAclManager(logger.Logger): self.VxlanSrcIP = "" return True + def get_preserved_input_rules(self, namespace): + """Snapshot INPUT rules owned by other services so a rebuild won't drop them. + + caclmgrd never comments its own rules, so a --comment in + IGNORED_INPUT_RULE_COMMENTS marks an external rule. Each is captured + verbatim so the owner's own -C/-D keep matching after rebuild. + """ + preserved = [] + if not self.IGNORED_INPUT_RULE_COMMENTS: + return preserved + list_cmd = self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-S', 'INPUT'] + try: + output = subprocess.check_output(list_cmd, universal_newlines=True) + except (subprocess.CalledProcessError, OSError) as e: + self.log_warning("Unable to read INPUT chain to preserve externally-owned rules: {}".format(e)) + return preserved + for line in output.splitlines(): + # iptables -S quotes comments that contain spaces, so tokenize like a + # shell (shlex) rather than split() to keep argument boundaries and + # drop quotes -- otherwise the re-added rule would not be byte-identical + # and the owner's -C/-D could stop matching. + try: + tokens = shlex.split(line) + except ValueError: + continue + if len(tokens) < 2 or tokens[0] != '-A' or tokens[1] != 'INPUT': + continue + if '--comment' not in tokens: + continue + idx = tokens.index('--comment') + if idx + 1 >= len(tokens): + continue + if tokens[idx + 1] not in self.IGNORED_INPUT_RULE_COMMENTS: + continue + preserved.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables'] + tokens) + return preserved + def allow_redfish(self): self.RedfishAllowed = True self.log_info("Enabled REDFISH control plane ACL handling") diff --git a/tests/caclmgrd/caclmgrd_preserved_input_rules_test.py b/tests/caclmgrd/caclmgrd_preserved_input_rules_test.py new file mode 100644 index 00000000..bd11a124 --- /dev/null +++ b/tests/caclmgrd/caclmgrd_preserved_input_rules_test.py @@ -0,0 +1,153 @@ +import os +import sys + +from pyfakefs.fake_filesystem_unittest import patchfs +from sonic_py_common.general import load_module_from_source +from swsscommon import swsscommon +from unittest import TestCase, mock + +from tests.common.mock_configdb import MockConfigDb + + +DBCONFIG_PATH = '/var/run/redis/sonic-db/database_config.json' + +# A representative `iptables -S INPUT` snapshot: the dhcp_server container's +# commented docker0 syslog rule (externally owned), a rule with a different +# comment, an uncommented caclmgrd-style rule, and the policy line. +SAMPLE_INPUT_DUMP = "\n".join([ + "-P INPUT ACCEPT", + "-A INPUT -i docker0 -p udp -m udp --dport 514 -m comment --comment dhcp_server_syslog -j ACCEPT", + "-A INPUT -p tcp -m tcp --dport 22 -j ACCEPT", + "-A INPUT -i docker0 -p udp --dport 9999 -m comment --comment some_other_service -j ACCEPT", + "-A INPUT -j DROP", +]) + +DHCP_SYSLOG_RULE = ( + 'iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'udp', '-m', 'udp', + '--dport', '514', '-m', 'comment', '--comment', 'dhcp_server_syslog', '-j', 'ACCEPT' +) + + +class TestCaclmgrdPreservedInputRules(TestCase): + """ + caclmgrd owns the host INPUT chain and flushes/rebuilds it on every CACL + change. Rules owned by OTHER services (tagged with an iptables --comment in + IGNORED_INPUT_RULE_COMMENTS, e.g. the dhcp_server container's docker0 syslog + rule) must be preserved across that rebuild and re-added before any DROP. + """ + 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) + + def setup_daemon(self, config_db): + MockConfigDb.set_config_db(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.generate_allow_internal_docker_ip_traffic_commands = mock.MagicMock(return_value=[]) + self.caclmgrd.ControlPlaneAclManager.generate_allow_internal_chasis_midplane_traffic = 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='') + return self.caclmgrd.ControlPlaneAclManager("caclmgrd") + + @patchfs + def test_snapshot_keeps_only_ignore_listed_comments(self, fs): + """Only rules whose --comment is in IGNORED_INPUT_RULE_COMMENTS are kept; + a different comment and uncommented/policy lines are skipped. The kept + rule is reproduced verbatim as an 'iptables -A INPUT ...' command.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, "FEATURE": {}}) + daemon.iptables_cmd_ns_prefix[''] = [] + + with mock.patch.object(self.caclmgrd.subprocess, 'check_output', return_value=SAMPLE_INPUT_DUMP): + preserved = daemon.get_preserved_input_rules('') + + preserved = [tuple(r) for r in preserved] + self.assertEqual(preserved, [DHCP_SYSLOG_RULE]) + + @patchfs + def test_snapshot_handles_quoted_comment_with_spaces(self, fs): + """iptables -S quotes comments containing spaces; shlex parsing must keep + the comment as one unquoted token so the re-add stays byte-identical and + the owner's -C/-D still match (no literal quote chars).""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, "FEATURE": {}}) + daemon.iptables_cmd_ns_prefix[''] = [] + daemon.IGNORED_INPUT_RULE_COMMENTS = ['multi word comment'] + dump = '-A INPUT -i docker0 -p udp -m udp --dport 514 ' \ + '-m comment --comment "multi word comment" -j ACCEPT' + + with mock.patch.object(self.caclmgrd.subprocess, 'check_output', return_value=dump): + preserved = daemon.get_preserved_input_rules('') + + self.assertEqual(len(preserved), 1) + self.assertIn('multi word comment', preserved[0]) # single unquoted token + self.assertNotIn('"multi', preserved[0]) # no literal quote chars + + @patchfs + def test_snapshot_returns_empty_on_iptables_failure(self, fs): + """If the chain can't be read, preservation degrades gracefully to [].""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, "FEATURE": {}}) + daemon.iptables_cmd_ns_prefix[''] = [] + + with mock.patch.object(self.caclmgrd.subprocess, 'check_output', side_effect=OSError("no iptables")): + self.assertEqual(daemon.get_preserved_input_rules(''), []) + + @patchfs + def test_preserved_rule_reinserted_before_catch_all_drop(self, fs): + """End-to-end: with a CACL rule present (so the catch-all DROP exists), the + preserved rule appears in the rebuilt chain AND strictly before the DROP, + guaranteeing no flush-window where a DROP exists without it.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + config_db = { + "ACL_TABLE": { + "SSH_ONLY": {"stage": "INGRESS", "type": "CTRLPLANE", "services": ["SSH"]}, + }, + "ACL_RULE": { + "SSH_ONLY|RULE_1": {"PACKET_ACTION": "ACCEPT", "PRIORITY": "9999", "SRC_IP": "10.0.0.0/8"}, + }, + "DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {}, + } + daemon = self.setup_daemon(config_db) + daemon.iptables_cmd_ns_prefix[''] = [] + + with mock.patch.object(daemon, 'get_preserved_input_rules', return_value=[list(DHCP_SYSLOG_RULE)]): + cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb()) + + cmds = [tuple(c) for c in cmds] + catch_all_drop = ('iptables', '-A', 'INPUT', '-j', 'DROP') + self.assertIn(DHCP_SYSLOG_RULE, cmds, "preserved rule must be present in rebuild") + self.assertIn(catch_all_drop, cmds, "test setup should produce a catch-all DROP") + self.assertLess(cmds.index(DHCP_SYSLOG_RULE), cmds.index(catch_all_drop), + "preserved ACCEPT must come before the catch-all DROP") + + @patchfs + def test_preserve_skipped_for_non_host_namespace(self, fs): + """The docker0 rule is host/IPv4 only; ASIC namespaces must not even probe + the chain (snapshot not invoked).""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, "FEATURE": {}}) + daemon.iptables_cmd_ns_prefix['asic0'] = [] + + with mock.patch.object(daemon, 'get_preserved_input_rules', return_value=[list(DHCP_SYSLOG_RULE)]) as snap: + cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('asic0', MockConfigDb()) + + snap.assert_not_called() + self.assertNotIn(DHCP_SYSLOG_RULE, [tuple(c) for c in cmds])