Skip to content

Commit cbcf4b1

Browse files
nsoma-cisconsoma-cisco
committed
[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 <nsoma@cisco.com>
1 parent 5a370b6 commit cbcf4b1

3 files changed

Lines changed: 241 additions & 0 deletions

File tree

‎scripts/caclmgrd‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,17 @@ class ControlPlaneAclManager(logger.Logger):
120120
}
121121
smartswitch_midplane_bridge_ip = "169.254.200.254"
122122

123+
# Restrict access to FRR daemon TCP ports on the loopback interface.
124+
# FRR processes run as a dedicated user (UID 300); only that UID is
125+
# permitted to connect to these ports over `lo`, everything else is
126+
# dropped. iptables match order requires the ACCEPT (uid-owner) rule to
127+
# be appended before the DROP for the same port.
128+
FRR_USER_UID = 300
129+
FRR_LOOPBACK_TCP_DROP_PORTS = [
130+
2601, # zebra VTY
131+
2620, # fpmsyncd <-> zebra
132+
]
133+
123134
UPDATE_DELAY_SECS = 0.5
124135

125136
DualToR = False
@@ -426,6 +437,28 @@ class ControlPlaneAclManager(logger.Logger):
426437

427438
return drop_dulator_bgp_loopback1_cmds
428439

440+
def generate_frr_daemon_loopback_acl_commands(self, namespace):
441+
"""Restrict FRR daemon TCP ports on `lo` to FRR_USER_UID; emit ACCEPT
442+
(uid-owner) before DROP per port. Applied in every managed namespace."""
443+
cmds = []
444+
prefix = self.iptables_cmd_ns_prefix[namespace]
445+
uid = str(self.FRR_USER_UID)
446+
for port in self.FRR_LOOPBACK_TCP_DROP_PORTS:
447+
port_str = str(port)
448+
cmds.append(prefix + [
449+
'iptables', '-A', 'OUTPUT', '-o', 'lo',
450+
'-p', 'tcp', '--dport', port_str,
451+
'-m', 'owner', '--uid-owner', uid,
452+
'-j', 'ACCEPT',
453+
])
454+
cmds.append(prefix + [
455+
'iptables', '-A', 'OUTPUT', '-o', 'lo',
456+
'-p', 'tcp', '--dport', port_str,
457+
'-j', 'DROP',
458+
])
459+
460+
return cmds
461+
429462
def generate_fwd_traffic_from_host_to_soc(self, namespace, config_db_connector):
430463

431464
fwd_dualtor_grpc_traffic_from_host_to_soc_cmds = []
@@ -724,6 +757,9 @@ class ControlPlaneAclManager(logger.Logger):
724757
iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-t', 'raw', '-A', 'PREROUTING', '-p', 'ipv6-icmp', '-j', 'NOTRACK'])
725758
iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-t', 'raw', '-A', 'OUTPUT', '-p', 'ipv6-icmp', '-j', 'NOTRACK'])
726759

760+
# Restrict FRR daemon loopback TCP ports to the FRR user only
761+
iptables_cmds += self.generate_frr_daemon_loopback_acl_commands(namespace)
762+
727763
# Get current ACL tables and rules from Config DB
728764

729765
self._tables_db_info = config_db_connector.get_table(self.ACL_TABLE)

‎tests/caclmgrd/caclmgrd_frr_loopback_acl_test.py‎

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
import os
2+
import sys
3+
4+
from swsscommon import swsscommon
5+
from parameterized import parameterized
6+
from unittest import TestCase, mock
7+
from pyfakefs.fake_filesystem_unittest import patchfs
8+
from sonic_py_common.general import load_module_from_source
9+
10+
from .test_frr_loopback_acl_vectors import CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR
11+
from tests.common.mock_configdb import MockConfigDb
12+
13+
DBCONFIG_PATH = '/var/run/redis/sonic-db/database_config.json'
14+
15+
16+
def _accept_rule(port):
17+
return (
18+
'iptables', '-A', 'OUTPUT', '-o', 'lo',
19+
'-p', 'tcp', '--dport', str(port),
20+
'-m', 'owner', '--uid-owner', '300',
21+
'-j', 'ACCEPT',
22+
)
23+
24+
25+
def _drop_rule(port):
26+
return (
27+
'iptables', '-A', 'OUTPUT', '-o', 'lo',
28+
'-p', 'tcp', '--dport', str(port),
29+
'-j', 'DROP',
30+
)
31+
32+
33+
class TestCaclmgrdFrrLoopbackAcl(TestCase):
34+
"""
35+
Verify caclmgrd installs OUTPUT-chain rules that restrict FRR daemon TCP
36+
ports on the loopback interface to the FRR user (UID 300).
37+
"""
38+
39+
EXPECTED_PORTS = (2601, 2620)
40+
41+
def setUp(self):
42+
swsscommon.ConfigDBConnector = MockConfigDb
43+
test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
44+
modules_path = os.path.dirname(test_path)
45+
scripts_path = os.path.join(modules_path, "scripts")
46+
sys.path.insert(0, modules_path)
47+
caclmgrd_path = os.path.join(scripts_path, 'caclmgrd')
48+
self.caclmgrd = load_module_from_source('caclmgrd', caclmgrd_path)
49+
50+
def _build_daemon(self):
51+
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
52+
self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()
53+
self.caclmgrd.ControlPlaneAclManager.generate_block_ip2me_traffic_iptables_commands = mock.MagicMock(return_value=[])
54+
self.caclmgrd.ControlPlaneAclManager.get_chain_list = mock.MagicMock(return_value=["INPUT", "FORWARD", "OUTPUT"])
55+
self.caclmgrd.ControlPlaneAclManager.get_chassis_midplane_interface_ip = mock.MagicMock(return_value='')
56+
return self.caclmgrd.ControlPlaneAclManager("caclmgrd")
57+
58+
@parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR)
59+
@patchfs
60+
def test_frr_loopback_rules_are_generated(self, _test_name, test_data, fs):
61+
if not os.path.exists(DBCONFIG_PATH):
62+
fs.create_file(DBCONFIG_PATH)
63+
64+
MockConfigDb.set_config_db(test_data["config_db"])
65+
caclmgrd_daemon = self._build_daemon()
66+
67+
rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
68+
'', MockConfigDb())
69+
rules_ret = [tuple(r) for r in rules_ret]
70+
71+
for port in self.EXPECTED_PORTS:
72+
self.assertIn(_accept_rule(port), rules_ret,
73+
"Missing ACCEPT rule for FRR port {}".format(port))
74+
self.assertIn(_drop_rule(port), rules_ret,
75+
"Missing DROP rule for FRR port {}".format(port))
76+
77+
@parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR)
78+
@patchfs
79+
def test_accept_precedes_drop_per_port(self, _test_name, test_data, fs):
80+
if not os.path.exists(DBCONFIG_PATH):
81+
fs.create_file(DBCONFIG_PATH)
82+
83+
MockConfigDb.set_config_db(test_data["config_db"])
84+
caclmgrd_daemon = self._build_daemon()
85+
86+
rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
87+
'', MockConfigDb())
88+
rules_ret = [tuple(r) for r in rules_ret]
89+
90+
for port in self.EXPECTED_PORTS:
91+
accept_idx = rules_ret.index(_accept_rule(port))
92+
drop_idx = rules_ret.index(_drop_rule(port))
93+
self.assertLess(
94+
accept_idx, drop_idx,
95+
"ACCEPT (uid-owner) must precede DROP for port {}".format(port))
96+
97+
@parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR)
98+
@patchfs
99+
def test_no_generic_output_drop_precedes_frr_accept(self, _test_name, test_data, fs):
100+
"""
101+
Guard against a regression where some other helper appends an
102+
IPv4 OUTPUT-chain DROP/REJECT/jump above our ACCEPT rules. Such a
103+
rule would silently shadow the per-UID ACCEPT and break FRR.
104+
Only DROPs/REJECTs/jumps to bespoke chains targeting our specific
105+
ports are an issue here, but we conservatively assert no such
106+
terminating verdict appears in the OUTPUT chain anywhere before
107+
our first ACCEPT.
108+
"""
109+
if not os.path.exists(DBCONFIG_PATH):
110+
fs.create_file(DBCONFIG_PATH)
111+
112+
MockConfigDb.set_config_db(test_data["config_db"])
113+
caclmgrd_daemon = self._build_daemon()
114+
115+
rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands(
116+
'', MockConfigDb())
117+
rules_ret = [tuple(r) for r in rules_ret]
118+
119+
first_frr_accept_idx = min(
120+
rules_ret.index(_accept_rule(p)) for p in self.EXPECTED_PORTS)
121+
122+
terminating_verdicts = {'DROP', 'REJECT'}
123+
for idx, cmd in enumerate(rules_ret[:first_frr_accept_idx]):
124+
if 'iptables' not in cmd or '-A' not in cmd:
125+
continue
126+
try:
127+
chain_pos = cmd.index('-A') + 1
128+
except ValueError:
129+
continue
130+
if chain_pos >= len(cmd) or cmd[chain_pos] != 'OUTPUT':
131+
continue
132+
if '-j' in cmd:
133+
target = cmd[cmd.index('-j') + 1]
134+
self.assertNotIn(
135+
target, terminating_verdicts,
136+
"Found IPv4 OUTPUT-chain {} rule at index {} which precedes "
137+
"the FRR ACCEPT rule and could shadow it: {}".format(
138+
target, idx, ' '.join(cmd)))
139+
140+
@parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR)
141+
@patchfs
142+
def test_helper_emits_rules_in_non_default_namespace(self, _test_name, test_data, fs):
143+
if not os.path.exists(DBCONFIG_PATH):
144+
fs.create_file(DBCONFIG_PATH)
145+
146+
MockConfigDb.set_config_db(test_data["config_db"])
147+
caclmgrd_daemon = self._build_daemon()
148+
149+
ns = 'asic0'
150+
ns_prefix = ['ip', 'netns', 'exec', ns]
151+
caclmgrd_daemon.iptables_cmd_ns_prefix[ns] = ns_prefix
152+
153+
cmds = caclmgrd_daemon.generate_frr_daemon_loopback_acl_commands(ns)
154+
155+
self.assertEqual(
156+
len(cmds),
157+
2 * len(caclmgrd_daemon.FRR_LOOPBACK_TCP_DROP_PORTS),
158+
"Expected one ACCEPT+DROP pair per configured FRR port in ASIC namespace")
159+
160+
for cmd in cmds:
161+
self.assertEqual(
162+
cmd[:len(ns_prefix)], ns_prefix,
163+
"ASIC-namespace command must start with `ip netns exec <ns>` prefix")
164+
165+
for port in self.EXPECTED_PORTS:
166+
self.assertIn(tuple(ns_prefix) + _accept_rule(port), [tuple(c) for c in cmds])
167+
self.assertIn(tuple(ns_prefix) + _drop_rule(port), [tuple(c) for c in cmds])
168+
169+
@parameterized.expand(CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR)
170+
@patchfs
171+
def test_helper_emits_one_pair_per_configured_port(self, _test_name, test_data, fs):
172+
if not os.path.exists(DBCONFIG_PATH):
173+
fs.create_file(DBCONFIG_PATH)
174+
175+
MockConfigDb.set_config_db(test_data["config_db"])
176+
caclmgrd_daemon = self._build_daemon()
177+
178+
cmds = caclmgrd_daemon.generate_frr_daemon_loopback_acl_commands('')
179+
self.assertEqual(
180+
len(cmds),
181+
2 * len(caclmgrd_daemon.FRR_LOOPBACK_TCP_DROP_PORTS),
182+
"Expected exactly one ACCEPT+DROP pair per configured FRR port")

‎tests/caclmgrd/test_frr_loopback_acl_vectors.py‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
"""
2+
caclmgrd FRR daemon loopback ACL test vector.
3+
4+
The test driver re-uses the standard CONFIG_DB skeleton; the FRR loopback
5+
rules are static (independent of CONFIG_DB content), so a minimal config is
6+
sufficient to exercise the generator.
7+
"""
8+
9+
CACLMGRD_FRR_LOOPBACK_ACL_TEST_VECTOR = [
10+
[
11+
"FRR daemon loopback ACL rules - default namespace",
12+
{
13+
"config_db": {
14+
"DEVICE_METADATA": {
15+
"localhost": {
16+
"type": "ToRRouter",
17+
}
18+
},
19+
"FEATURE": {},
20+
},
21+
}
22+
]
23+
]

0 commit comments

Comments
 (0)