Fix FORWARD rule ordering: embed API-sync ACCEPT inside apply_cell_rules
The per-cell catch-all DROP was reaching position 5 before our ACCEPT (position 6) because apply_all_cell_rules can re-run after ensure_cell_api_dnat, pushing the DNAT ACCEPT below the DROP. Fix: add the API-sync ACCEPT inside apply_cell_rules itself, tagged with the cell's own tag and inserted LAST (= position 1, above the DROP). Since it's part of the cell's rule block it is always in the right position relative to the catch-all DROP, regardless of call order. Also adds _get_cell_api_ip() helper (docker inspect cell-api) so the destination IP is always current, and two new tests that verify both the rule exists and that the insertion order guarantees it wins over DROP. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
+21
-2
@@ -254,16 +254,26 @@ def clear_cell_rules(cell_name: str) -> None:
|
|||||||
logger.error(f"clear_cell_rules({cell_name}): {e}")
|
logger.error(f"clear_cell_rules({cell_name}): {e}")
|
||||||
|
|
||||||
|
|
||||||
|
def _get_cell_api_ip() -> Optional[str]:
|
||||||
|
"""Return cell-api's Docker bridge IP. Returns empty string on failure."""
|
||||||
|
r = _run(['docker', 'inspect', '--format',
|
||||||
|
'{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}',
|
||||||
|
'cell-api'], check=False)
|
||||||
|
return r.stdout.strip()
|
||||||
|
|
||||||
|
|
||||||
def apply_cell_rules(cell_name: str, vpn_subnet: str, inbound_services: List[str]) -> bool:
|
def apply_cell_rules(cell_name: str, vpn_subnet: str, inbound_services: List[str]) -> bool:
|
||||||
"""Apply FORWARD rules for a cell-to-cell peer.
|
"""Apply FORWARD rules for a cell-to-cell peer.
|
||||||
|
|
||||||
Traffic from vpn_subnet is allowed only to service VIPs listed in
|
Traffic from vpn_subnet is allowed only to service VIPs listed in
|
||||||
inbound_services; all other cell traffic is DROPped. Cells get no
|
inbound_services; all other cell traffic is DROPped. Cells get no
|
||||||
internet or peer access — only explicit service VIPs.
|
internet or peer access — only explicit service VIPs, plus the
|
||||||
|
cell-api port (3000) for permission-sync pushes arriving via DNAT.
|
||||||
|
|
||||||
Rule insertion order (last inserted → top of chain):
|
Rule insertion order (last inserted → top of chain):
|
||||||
1. Catch-all DROP for the subnet (inserted first → bottom)
|
1. Catch-all DROP for the subnet (inserted first → bottom)
|
||||||
2. Per-service ACCEPT/DROP (inserted in reversed() order → top)
|
2. Per-service ACCEPT/DROP (inserted in reversed() order)
|
||||||
|
3. API-sync ACCEPT (inserted last → top, above catch-all)
|
||||||
"""
|
"""
|
||||||
try:
|
try:
|
||||||
tag = _cell_tag(cell_name)
|
tag = _cell_tag(cell_name)
|
||||||
@@ -279,6 +289,15 @@ def apply_cell_rules(cell_name: str, vpn_subnet: str, inbound_services: List[str
|
|||||||
_iptables(['-I', 'FORWARD', '-s', vpn_subnet, '-d', svc_ip,
|
_iptables(['-I', 'FORWARD', '-s', vpn_subnet, '-d', svc_ip,
|
||||||
'-m', 'comment', '--comment', tag, '-j', target])
|
'-m', 'comment', '--comment', tag, '-j', target])
|
||||||
|
|
||||||
|
# API permission-sync ACCEPT — inserted LAST so it goes to position 1 (above
|
||||||
|
# the catch-all DROP). Remote cells push permissions to our cell-api via the
|
||||||
|
# WG tunnel; iptables sees source=cell_subnet dst=api_ip after DNAT.
|
||||||
|
api_ip = _get_cell_api_ip()
|
||||||
|
if api_ip:
|
||||||
|
_iptables(['-I', 'FORWARD', '-s', vpn_subnet, '-d', api_ip,
|
||||||
|
'-p', 'tcp', '--dport', '3000',
|
||||||
|
'-m', 'comment', '--comment', tag, '-j', 'ACCEPT'])
|
||||||
|
|
||||||
logger.info(f"Applied cell rules for {cell_name} ({vpn_subnet}): inbound={inbound_services}")
|
logger.info(f"Applied cell rules for {cell_name} ({vpn_subnet}): inbound={inbound_services}")
|
||||||
return True
|
return True
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
|||||||
@@ -415,8 +415,10 @@ class TestCellRules(unittest.TestCase):
|
|||||||
|
|
||||||
# ── helpers ───────────────────────────────────────────────────────────────
|
# ── helpers ───────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
_FAKE_API_IP = '172.20.0.10'
|
||||||
|
|
||||||
def _capture_apply(self, cell_name, vpn_subnet, inbound_services):
|
def _capture_apply(self, cell_name, vpn_subnet, inbound_services):
|
||||||
"""Run apply_cell_rules with _wg_exec mocked; return list of captured iptables arg lists."""
|
"""Run apply_cell_rules with _wg_exec and _get_cell_api_ip mocked."""
|
||||||
calls_made = []
|
calls_made = []
|
||||||
|
|
||||||
def fake_wg_exec(args):
|
def fake_wg_exec(args):
|
||||||
@@ -426,7 +428,8 @@ class TestCellRules(unittest.TestCase):
|
|||||||
m.stdout = ''
|
m.stdout = ''
|
||||||
return m
|
return m
|
||||||
|
|
||||||
with patch.object(firewall_manager, '_wg_exec', side_effect=fake_wg_exec):
|
with patch.object(firewall_manager, '_wg_exec', side_effect=fake_wg_exec), \
|
||||||
|
patch.object(firewall_manager, '_get_cell_api_ip', return_value=self._FAKE_API_IP):
|
||||||
firewall_manager.apply_cell_rules(cell_name, vpn_subnet, inbound_services)
|
firewall_manager.apply_cell_rules(cell_name, vpn_subnet, inbound_services)
|
||||||
|
|
||||||
return [c for c in calls_made if 'iptables' in c]
|
return [c for c in calls_made if 'iptables' in c]
|
||||||
@@ -490,6 +493,38 @@ class TestCellRules(unittest.TestCase):
|
|||||||
files_targets = self._targets_for_dest(calls, files_ip)
|
files_targets = self._targets_for_dest(calls, files_ip)
|
||||||
self.assertIn('DROP', files_targets)
|
self.assertIn('DROP', files_targets)
|
||||||
|
|
||||||
|
def test_apply_cell_rules_accepts_api_sync_traffic(self):
|
||||||
|
"""apply_cell_rules inserts ACCEPT for cell-api:3000 so permission-sync pushes pass."""
|
||||||
|
calls = self._capture_apply('office', '10.0.1.0/24', [])
|
||||||
|
api_ip = self._FAKE_API_IP
|
||||||
|
api_accepts = [
|
||||||
|
c for c in calls
|
||||||
|
if '-s' in c and '10.0.1.0/24' in c
|
||||||
|
and '-d' in c and api_ip in c
|
||||||
|
and '--dport' in c and '3000' in c
|
||||||
|
and '-j' in c and c[c.index('-j') + 1] == 'ACCEPT'
|
||||||
|
]
|
||||||
|
self.assertTrue(api_accepts, 'Expected an ACCEPT rule for cell-api:3000')
|
||||||
|
|
||||||
|
def test_apply_cell_rules_api_sync_accept_before_catchall_drop(self):
|
||||||
|
"""The API-sync ACCEPT must be inserted after service rules so it ends up above DROP."""
|
||||||
|
insertion_order = []
|
||||||
|
|
||||||
|
def fake_wg_exec(args):
|
||||||
|
if '-I' in args and 'FORWARD' in args:
|
||||||
|
if '-j' in args:
|
||||||
|
insertion_order.append(args[args.index('-j') + 1])
|
||||||
|
m = MagicMock(); m.returncode = 0; m.stdout = ''; return m
|
||||||
|
|
||||||
|
with patch.object(firewall_manager, '_wg_exec', side_effect=fake_wg_exec), \
|
||||||
|
patch.object(firewall_manager, '_get_cell_api_ip', return_value='172.20.0.10'):
|
||||||
|
firewall_manager.apply_cell_rules('office', '10.0.1.0/24', [])
|
||||||
|
|
||||||
|
# The API-sync ACCEPT must be the LAST -I FORWARD insertion so it sits at position 1
|
||||||
|
self.assertTrue(insertion_order, 'Expected at least one FORWARD rule inserted')
|
||||||
|
self.assertEqual(insertion_order[-1], 'ACCEPT',
|
||||||
|
f'Last -I FORWARD insertion must be ACCEPT (got {insertion_order})')
|
||||||
|
|
||||||
# ── apply_cell_rules — empty inbound (all-deny) ───────────────────────────
|
# ── apply_cell_rules — empty inbound (all-deny) ───────────────────────────
|
||||||
|
|
||||||
def test_apply_cell_rules_empty_inbound_all_drop(self):
|
def test_apply_cell_rules_empty_inbound_all_drop(self):
|
||||||
|
|||||||
Reference in New Issue
Block a user