From 255fcccbfc52d610324d7ca773b196f6adf673ae Mon Sep 17 00:00:00 2001 From: Nageswara Soma Date: Tue, 2 Jun 2026 15:02:19 -0700 Subject: [PATCH] [caclmgrd] Restrict FRR daemon loopback TCP ports to FRR user Lock down zebra (TCP/2601) and the fpmsyncd<->zebra socket (TCP/2620) on the loopback interface so only the FRR user (UID 300) can connect; every other local UID is dropped. Per port the ACCEPT (uid-owner 300) rule is appended before the DROP so iptables matches FRR-internal traffic first. Rules are emitted in every namespace caclmgrd manages (host on single-ASIC, per-ASIC bgp container namespaces on multi-ASIC) since FRR runs in each of them with the same UID and ports. The helper is invoked from get_acl_rules_and_translate_to_iptables_commands, so the rules are reinstalled on every full programming pass: at boot, on systemctl restart caclmgrd, and after any CONFIG_DB ACL_TABLE/ACL_RULE change. Adds caclmgrd_frr_loopback_acl_test.py covering rule presence, per-port ACCEPT-before-DROP ordering, helper rule count, and ASIC-namespace prefix, plus test_no_generic_output_drop_precedes_frr_accept which inspects the generated iptables command list and asserts that no IPv4 OUTPUT-chain DROP or REJECT rule appears before the FRR per-UID ACCEPT rules. This pins the current invariant so a future contributor can't silently shadow the FRR ACCEPT and break FRR daemon loopback communication. Signed-off-by: Nageswara Soma --- scripts/caclmgrd | 36 ++++ .../caclmgrd_frr_loopback_acl_test.py | 182 ++++++++++++++++++ .../caclmgrd/test_frr_loopback_acl_vectors.py | 23 +++ 3 files changed, 241 insertions(+) create mode 100644 tests/caclmgrd/caclmgrd_frr_loopback_acl_test.py create mode 100644 tests/caclmgrd/test_frr_loopback_acl_vectors.py diff --git a/scripts/caclmgrd b/scripts/caclmgrd index 258991e3..92b44a24 100755 --- a/scripts/caclmgrd +++ b/scripts/caclmgrd @@ -125,6 +125,17 @@ class ControlPlaneAclManager(logger.Logger): } smartswitch_midplane_bridge_ip = "169.254.200.254" + # Restrict access to FRR daemon TCP ports on the loopback interface. + # FRR processes run as a dedicated user (UID 300); only that UID is + # permitted to connect to these ports over `lo`, everything else is + # dropped. iptables match order requires the ACCEPT (uid-owner) rule to + # be appended before the DROP for the same port. + FRR_USER_UID = 300 + FRR_LOOPBACK_TCP_DROP_PORTS = [ + 2601, # zebra VTY + 2620, # fpmsyncd <-> zebra + ] + UPDATE_DELAY_SECS = 0.5 DualToR = False @@ -435,6 +446,28 @@ class ControlPlaneAclManager(logger.Logger): return drop_dulator_bgp_loopback1_cmds + def generate_frr_daemon_loopback_acl_commands(self, namespace): + """Restrict FRR daemon TCP ports on `lo` to FRR_USER_UID; emit ACCEPT + (uid-owner) before DROP per port. Applied in every managed namespace.""" + cmds = [] + prefix = self.iptables_cmd_ns_prefix[namespace] + uid = str(self.FRR_USER_UID) + for port in self.FRR_LOOPBACK_TCP_DROP_PORTS: + port_str = str(port) + cmds.append(prefix + [ + 'iptables', '-A', 'OUTPUT', '-o', 'lo', + '-p', 'tcp', '--dport', port_str, + '-m', 'owner', '--uid-owner', uid, + '-j', 'ACCEPT', + ]) + cmds.append(prefix + [ + 'iptables', '-A', 'OUTPUT', '-o', 'lo', + '-p', 'tcp', '--dport', port_str, + '-j', 'DROP', + ]) + + return cmds + def generate_fwd_traffic_from_host_to_soc(self, namespace, config_db_connector): fwd_dualtor_grpc_traffic_from_host_to_soc_cmds = [] @@ -742,6 +775,9 @@ class ControlPlaneAclManager(logger.Logger): iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'FORWARD', '-p', str(ip_protocol), '-s', self.namespace_mgmt_ip, '--sport', dst_port, '-j', 'ACCEPT']) + # Restrict FRR daemon loopback TCP ports to the FRR user only + iptables_cmds += self.generate_frr_daemon_loopback_acl_commands(namespace) + # Get current ACL tables and rules from Config DB self._tables_db_info = config_db_connector.get_table(self.ACL_TABLE) diff --git a/tests/caclmgrd/caclmgrd_frr_loopback_acl_test.py b/tests/caclmgrd/caclmgrd_frr_loopback_acl_test.py new file mode 100644 index 00000000..d0c4ae56 --- /dev/null +++ b/tests/caclmgrd/caclmgrd_frr_loopback_acl_test.py @@ -0,0 +1,182 @@ +import os +import sys + +from swsscommon import swsscommon +from parameterized import parameterized +from unittest import TestCase, mock +from pyfakefs.fake_filesystem_unittest import patchfs +from sonic_py_common.general import load_module_from_source + +from .test_frr_loopback_acl_vectors import CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR +from tests.common.mock_configdb import MockConfigDb + +DBCONFIG_PATH = '/var/run/redis/sonic-db/database_config.json' + + +def _accept_rule(port): + return ( + 'iptables', '-A', 'OUTPUT', '-o', 'lo', + '-p', 'tcp', '--dport', str(port), + '-m', 'owner', '--uid-owner', '300', + '-j', 'ACCEPT', + ) + + +def _drop_rule(port): + return ( + 'iptables', '-A', 'OUTPUT', '-o', 'lo', + '-p', 'tcp', '--dport', str(port), + '-j', 'DROP', + ) + + +class TestCaclmgrdFrrLoopbackAcl(TestCase): + """ + Verify caclmgrd installs OUTPUT-chain rules that restrict FRR daemon TCP + ports on the loopback interface to the FRR user (UID 300). + """ + + EXPECTED_PORTS = (2601, 2620) + + 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 _build_daemon(self): + 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='') + return self.caclmgrd.ControlPlaneAclManager("caclmgrd") + + @parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR) + @patchfs + def test_frr_loopback_rules_are_generated(self, _test_name, test_data, fs): + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + MockConfigDb.set_config_db(test_data["config_db"]) + caclmgrd_daemon = self._build_daemon() + + rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands( + '', MockConfigDb()) + rules_ret = [tuple(r) for r in rules_ret] + + for port in self.EXPECTED_PORTS: + self.assertIn(_accept_rule(port), rules_ret, + "Missing ACCEPT rule for FRR port {}".format(port)) + self.assertIn(_drop_rule(port), rules_ret, + "Missing DROP rule for FRR port {}".format(port)) + + @parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR) + @patchfs + def test_accept_precedes_drop_per_port(self, _test_name, test_data, fs): + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + MockConfigDb.set_config_db(test_data["config_db"]) + caclmgrd_daemon = self._build_daemon() + + rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands( + '', MockConfigDb()) + rules_ret = [tuple(r) for r in rules_ret] + + for port in self.EXPECTED_PORTS: + accept_idx = rules_ret.index(_accept_rule(port)) + drop_idx = rules_ret.index(_drop_rule(port)) + self.assertLess( + accept_idx, drop_idx, + "ACCEPT (uid-owner) must precede DROP for port {}".format(port)) + + @parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR) + @patchfs + def test_no_generic_output_drop_precedes_frr_accept(self, _test_name, test_data, fs): + """ + Guard against a regression where some other helper appends an + IPv4 OUTPUT-chain DROP/REJECT/jump above our ACCEPT rules. Such a + rule would silently shadow the per-UID ACCEPT and break FRR. + Only DROPs/REJECTs/jumps to bespoke chains targeting our specific + ports are an issue here, but we conservatively assert no such + terminating verdict appears in the OUTPUT chain anywhere before + our first ACCEPT. + """ + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + MockConfigDb.set_config_db(test_data["config_db"]) + caclmgrd_daemon = self._build_daemon() + + rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands( + '', MockConfigDb()) + rules_ret = [tuple(r) for r in rules_ret] + + first_frr_accept_idx = min( + rules_ret.index(_accept_rule(p)) for p in self.EXPECTED_PORTS) + + terminating_verdicts = {'DROP', 'REJECT'} + for idx, cmd in enumerate(rules_ret[:first_frr_accept_idx]): + if 'iptables' not in cmd or '-A' not in cmd: + continue + try: + chain_pos = cmd.index('-A') + 1 + except ValueError: + continue + if chain_pos >= len(cmd) or cmd[chain_pos] != 'OUTPUT': + continue + if '-j' in cmd: + target = cmd[cmd.index('-j') + 1] + self.assertNotIn( + target, terminating_verdicts, + "Found IPv4 OUTPUT-chain {} rule at index {} which precedes " + "the FRR ACCEPT rule and could shadow it: {}".format( + target, idx, ' '.join(cmd))) + + @parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR) + @patchfs + def test_helper_emits_rules_in_non_default_namespace(self, _test_name, test_data, fs): + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + MockConfigDb.set_config_db(test_data["config_db"]) + caclmgrd_daemon = self._build_daemon() + + ns = 'asic0' + ns_prefix = ['ip', 'netns', 'exec', ns] + caclmgrd_daemon.iptables_cmd_ns_prefix[ns] = ns_prefix + + cmds = caclmgrd_daemon.generate_frr_daemon_loopback_acl_commands(ns) + + self.assertEqual( + len(cmds), + 2 * len(caclmgrd_daemon.FRR_LOOPBACK_TCP_DROP_PORTS), + "Expected one ACCEPT+DROP pair per configured FRR port in ASIC namespace") + + for cmd in cmds: + self.assertEqual( + cmd[:len(ns_prefix)], ns_prefix, + "ASIC-namespace command must start with `ip netns exec ` prefix") + + for port in self.EXPECTED_PORTS: + self.assertIn(tuple(ns_prefix) + _accept_rule(port), [tuple(c) for c in cmds]) + self.assertIn(tuple(ns_prefix) + _drop_rule(port), [tuple(c) for c in cmds]) + + @parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR) + @patchfs + def test_helper_emits_one_pair_per_configured_port(self, _test_name, test_data, fs): + if not os.path.exists(DBCONFIG_PATH): + fs.create_file(DBCONFIG_PATH) + + MockConfigDb.set_config_db(test_data["config_db"]) + caclmgrd_daemon = self._build_daemon() + + cmds = caclmgrd_daemon.generate_frr_daemon_loopback_acl_commands('') + self.assertEqual( + len(cmds), + 2 * len(caclmgrd_daemon.FRR_LOOPBACK_TCP_DROP_PORTS), + "Expected exactly one ACCEPT+DROP pair per configured FRR port") diff --git a/tests/caclmgrd/test_frr_loopback_acl_vectors.py b/tests/caclmgrd/test_frr_loopback_acl_vectors.py new file mode 100644 index 00000000..66981ad9 --- /dev/null +++ b/tests/caclmgrd/test_frr_loopback_acl_vectors.py @@ -0,0 +1,23 @@ +""" +caclmgrd FRR daemon loopback ACL test vector. + +The test driver re-uses the standard CONFIG_DB skeleton; the FRR loopback +rules are static (independent of CONFIG_DB content), so a minimal config is +sufficient to exercise the generator. +""" + +CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR = [ + [ + "FRR daemon loopback ACL rules - default namespace", + { + "config_db": { + "DEVICE_METADATA": { + "localhost": { + "type": "ToRRouter", + } + }, + "FEATURE": {}, + }, + } + ] +]