From 6764a4818c4691fe346d600600e3776175160a50 Mon Sep 17 00:00:00 2001 From: shreyansh-nexthop Date: Mon, 31 Aug 2026 16:23:21 +0530 Subject: [PATCH 1/2] [caclmgrd] Own the redfish docker0 syslog INPUT exception Signed-off-by: shreyansh-nexthop --- scripts/caclmgrd | 8 +- .../caclmgrd/caclmgrd_redfish_syslog_test.py | 192 ++++++++++++++++++ 2 files changed, 197 insertions(+), 3 deletions(-) create mode 100644 tests/caclmgrd/caclmgrd_redfish_syslog_test.py diff --git a/scripts/caclmgrd b/scripts/caclmgrd index a52d5324..4d174922 100755 --- a/scripts/caclmgrd +++ b/scripts/caclmgrd @@ -703,9 +703,11 @@ 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']) - # dhcp_server docker0 syslog (RELP tcp/2514) exception; host namespace / IPv4 only - if namespace == DEFAULT_NAMESPACE and self.DhcpServerSyslogAllowed: - iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514', '-j', 'ACCEPT', '-m', 'comment', '--comment', 'dhcp_server_syslog']) + # bridged-container docker0 syslog (RELP tcp/2514) exceptions; host namespace / IPv4 only + if namespace == DEFAULT_NAMESPACE: + for feature, allowed in (("redfish", self.RedfishAllowed), ("dhcp_server", self.DhcpServerSyslogAllowed)): + if allowed: + iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514', '-j', 'ACCEPT', '-m', 'comment', '--comment', feature + '_syslog']) if self.bfdAllowed: diff --git a/tests/caclmgrd/caclmgrd_redfish_syslog_test.py b/tests/caclmgrd/caclmgrd_redfish_syslog_test.py new file mode 100644 index 00000000..73c98007 --- /dev/null +++ b/tests/caclmgrd/caclmgrd_redfish_syslog_test.py @@ -0,0 +1,192 @@ +import os +import sys + +from swsscommon import swsscommon +from sonic_py_common.general import load_module_from_source +from unittest import TestCase, mock +from pyfakefs.fake_filesystem_unittest import patchfs + +from tests.common.mock_configdb import MockConfigDb + + +DBCONFIG_PATH = '/var/run/redis/sonic-db/database_config.json' + +# Must stay byte-identical to the container-side -C check in docker_image_ctl.j2. +REDFISH_SYSLOG_RULE = ( + 'iptables', '-A', 'INPUT', '-i', 'docker0', '-p', 'tcp', '--dport', '2514', + '-j', 'ACCEPT', '-m', 'comment', '--comment', 'redfish_syslog', +) + + +class TestCaclmgrdRedfishSyslog(TestCase): + """ + Verifies caclmgrd owns the redfish docker0 syslog (RELP tcp/2514) INPUT + exception, gated on FEATURE.redfish, and re-emits it before the + catch-all DROP on every rebuild. + + redfish is bridge-networked, so its rsyslog forwards over RELP to the + docker0 gateway instead of 127.0.0.1. That traffic arrives on docker0 + (not lo), so it is not covered by the loopback ACCEPT and would be + swept into the control-plane catch-all DROP without this exception. + """ + 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) + self.maxDiff = None + + 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_init_seeds_flag_from_feature_state(self, fs): + """RedfishAllowed is seeded from the persisted FEATURE state at __init__.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + enabled = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {"redfish": {"state": "enabled"}}}) + disabled = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {"redfish": {"state": "disabled"}}}) + absent = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {}}) + self.assertTrue(enabled.RedfishAllowed) + self.assertFalse(disabled.RedfishAllowed) + self.assertFalse(absent.RedfishAllowed) + + @patchfs + def test_rule_emitted_when_feature_enabled(self, fs): + """When enabled, the docker0/2514 ACCEPT rule is present in the host rebuild.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {"redfish": {"state": "enabled"}}}) + self.assertTrue(daemon.RedfishAllowed) + + cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb()) + self.assertIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds]) + + @patchfs + def test_rule_absent_when_feature_disabled(self, fs): + """When disabled, no exception is programmed.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {"redfish": {"state": "disabled"}}}) + self.assertFalse(daemon.RedfishAllowed) + + cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb()) + self.assertNotIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds]) + + @patchfs + def test_rule_absent_when_feature_entry_missing(self, fs): + """Images built without redfish have no FEATURE entry at all -- no exception.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {}}) + self.assertFalse(daemon.RedfishAllowed) + + cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('', MockConfigDb()) + self.assertNotIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds]) + + @patchfs + def test_rule_reinserted_before_catch_all_drop(self, fs): + """With a CACL rule present (so the catch-all DROP exists), the exception appears + AND strictly before the DROP -- there is never a rebuild window where a DROP + exists without it.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({ + "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": {"redfish": {"state": "enabled"}}, + }) + + 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(REDFISH_SYSLOG_RULE, cmds, "exception must be present in rebuild") + self.assertIn(catch_all_drop, cmds, "test setup should produce a catch-all DROP") + self.assertLess(cmds.index(REDFISH_SYSLOG_RULE), cmds.index(catch_all_drop), + "exception must come before the catch-all DROP") + + @patchfs + def test_rule_host_namespace_only(self, fs): + """docker0 lives in the host namespace, so asic namespaces get no exception.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {"redfish": {"state": "enabled"}}}) + self.assertTrue(daemon.RedfishAllowed) + + daemon.iptables_cmd_ns_prefix['asic0'] = [] + cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('asic0', MockConfigDb()) + self.assertNotIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds]) + + @patchfs + def test_handle_redfish_feature_events(self, fs): + """Drive the FEATURE-table handler: enable transition, disable transition, + unrelated key ignored, and same-state no-op. The exception is only correct + across a runtime feature toggle if this flag tracks the state.""" + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, + "FEATURE": {"redfish": {"state": "disabled"}}}) + self.assertFalse(daemon.RedfishAllowed) + + def sub_with(events): + sub = mock.MagicMock() + sub.pop.side_effect = events + [("", None, None)] + return sub + + # enable: flag flips True and namespace queued for re-walk + notif = set() + daemon.handle_redfish_feature_events( + sub_with([("redfish", "SET", (("state", "enabled"),))]), "", notif) + self.assertTrue(daemon.RedfishAllowed) + self.assertIn("", notif) + + # disable: flag flips False and namespace queued + notif = set() + daemon.handle_redfish_feature_events( + sub_with([("redfish", "SET", (("state", "disabled"),))]), "", notif) + self.assertFalse(daemon.RedfishAllowed) + self.assertIn("", notif) + + # unrelated FEATURE event: redfish flag untouched, nothing queued + notif = set() + daemon.handle_redfish_feature_events( + sub_with([("bgp", "SET", (("state", "enabled"),))]), "", notif) + self.assertFalse(daemon.RedfishAllowed) + self.assertEqual(notif, set()) + + # same-state event (already disabled): no-op, nothing queued + notif = set() + daemon.handle_redfish_feature_events( + sub_with([("redfish", "SET", (("state", "disabled"),))]), "", notif) + self.assertFalse(daemon.RedfishAllowed) + self.assertEqual(notif, set()) From 48324e6bcf9d6237e4ea5f5be863c77a5032d373 Mon Sep 17 00:00:00 2001 From: Shreyansh Jain Date: Tue, 1 Sep 2026 03:14:38 +0000 Subject: [PATCH 2/2] [caclmgrd] Drop the duplicate feature-event test from the redfish syslog tests caclmgrd_redfish_acl_test.py already drives handle_feature_state_events through the same four cases: enable transition, disable transition, unrelated key ignored, and same-state no-op. The copy added alongside the syslog tests called the pre-rename name and duplicated coverage that already exists. Signed-off-by: Shreyansh Jain --- .../caclmgrd/caclmgrd_redfish_syslog_test.py | 45 ------------------- 1 file changed, 45 deletions(-) diff --git a/tests/caclmgrd/caclmgrd_redfish_syslog_test.py b/tests/caclmgrd/caclmgrd_redfish_syslog_test.py index 73c98007..528b1b6a 100644 --- a/tests/caclmgrd/caclmgrd_redfish_syslog_test.py +++ b/tests/caclmgrd/caclmgrd_redfish_syslog_test.py @@ -145,48 +145,3 @@ def test_rule_host_namespace_only(self, fs): daemon.iptables_cmd_ns_prefix['asic0'] = [] cmds, _ = daemon.get_acl_rules_and_translate_to_iptables_commands('asic0', MockConfigDb()) self.assertNotIn(REDFISH_SYSLOG_RULE, [tuple(c) for c in cmds]) - - @patchfs - def test_handle_redfish_feature_events(self, fs): - """Drive the FEATURE-table handler: enable transition, disable transition, - unrelated key ignored, and same-state no-op. The exception is only correct - across a runtime feature toggle if this flag tracks the state.""" - if not os.path.exists(DBCONFIG_PATH): - fs.create_file(DBCONFIG_PATH) - - daemon = self.setup_daemon({"DEVICE_METADATA": {"localhost": {}}, - "FEATURE": {"redfish": {"state": "disabled"}}}) - self.assertFalse(daemon.RedfishAllowed) - - def sub_with(events): - sub = mock.MagicMock() - sub.pop.side_effect = events + [("", None, None)] - return sub - - # enable: flag flips True and namespace queued for re-walk - notif = set() - daemon.handle_redfish_feature_events( - sub_with([("redfish", "SET", (("state", "enabled"),))]), "", notif) - self.assertTrue(daemon.RedfishAllowed) - self.assertIn("", notif) - - # disable: flag flips False and namespace queued - notif = set() - daemon.handle_redfish_feature_events( - sub_with([("redfish", "SET", (("state", "disabled"),))]), "", notif) - self.assertFalse(daemon.RedfishAllowed) - self.assertIn("", notif) - - # unrelated FEATURE event: redfish flag untouched, nothing queued - notif = set() - daemon.handle_redfish_feature_events( - sub_with([("bgp", "SET", (("state", "enabled"),))]), "", notif) - self.assertFalse(daemon.RedfishAllowed) - self.assertEqual(notif, set()) - - # same-state event (already disabled): no-op, nothing queued - notif = set() - daemon.handle_redfish_feature_events( - sub_with([("redfish", "SET", (("state", "disabled"),))]), "", notif) - self.assertFalse(daemon.RedfishAllowed) - self.assertEqual(notif, set())