From ea6731d62c7d9e68a279345dcbb7ab79947f84d4 Mon Sep 17 00:00:00 2001 From: Dmitrii Iurco Date: Fri, 1 May 2026 14:05:49 -0400 Subject: [PATCH] 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 --- api/firewall_manager.py | 23 ++++++++++++++++++-- tests/test_firewall_manager.py | 39 ++++++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/api/firewall_manager.py b/api/firewall_manager.py index 9f4aca4..2dd5f64 100644 --- a/api/firewall_manager.py +++ b/api/firewall_manager.py @@ -254,16 +254,26 @@ def clear_cell_rules(cell_name: str) -> None: 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: """Apply FORWARD rules for a cell-to-cell peer. Traffic from vpn_subnet is allowed only to service VIPs listed in 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): 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: 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, '-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}") return True except Exception as e: diff --git a/tests/test_firewall_manager.py b/tests/test_firewall_manager.py index 209ddd0..b386449 100644 --- a/tests/test_firewall_manager.py +++ b/tests/test_firewall_manager.py @@ -415,8 +415,10 @@ class TestCellRules(unittest.TestCase): # ── helpers ─────────────────────────────────────────────────────────────── + _FAKE_API_IP = '172.20.0.10' + 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 = [] def fake_wg_exec(args): @@ -426,7 +428,8 @@ class TestCellRules(unittest.TestCase): m.stdout = '' 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) 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) 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) ─────────────────────────── def test_apply_cell_rules_empty_inbound_all_drop(self):