From c2d215ee2e56c90e64d3619c0deaee71885275f5 Mon Sep 17 00:00:00 2001 From: Dmitrii Iurco Date: Mon, 4 May 2026 14:36:28 -0400 Subject: [PATCH] fix: cross-cell routing for split-tunnel peers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related fixes for split-tunnel peers that need to reach connected cells: 1. apply_peer_rules/apply_all_peer_rules now accept wg_subnet (actual local VPN subnet) and cell_subnets (connected cells' vpn_subnets) parameters instead of hardcoding 10.0.0.0/24. All callers (startup, add_peer, update_peer, apply-enforcement endpoint) pass the real values. 2. Explicit ACCEPT rules are inserted in FORWARD for each connected cell's subnet so split-tunnel peers (internet_access=False) can still reach connected cells via the wg0→wg0 path. 3. apply_ip_range in network_manager now loads cell_links.json and passes it to generate_corefile(), fixing a race where the bootstrap DNS thread could overwrite the Corefile and wipe cross-cell DNS forwarding zones on startup. Co-Authored-By: Claude Sonnet 4.6 --- api/app.py | 9 +++- api/firewall_manager.py | 39 ++++++++++---- api/network_manager.py | 11 +++- api/routes/peers.py | 23 +++++++-- api/routes/wireguard.py | 15 ++++-- tests/test_firewall_manager.py | 93 ++++++++++++++++++++++++++++++++++ 6 files changed, 171 insertions(+), 19 deletions(-) diff --git a/api/app.py b/api/app.py index 5886aa8..99b8fa8 100644 --- a/api/app.py +++ b/api/app.py @@ -308,7 +308,14 @@ def _apply_startup_enforcement(): peers = peer_registry.list_peers() cell_links = cell_link_manager.list_connections() firewall_manager.reconcile_stale_peer_rules(peers) - firewall_manager.apply_all_peer_rules(peers) + import ipaddress as _ipa + try: + _wg_addr = wireguard_manager._get_configured_address() + _wg_subnet = str(_ipa.ip_network(_wg_addr, strict=False)) if _wg_addr else '10.0.0.0/24' + except Exception: + _wg_subnet = '10.0.0.0/24' + _cell_subnets = [l['vpn_subnet'] for l in cell_links if l.get('vpn_subnet')] + firewall_manager.apply_all_peer_rules(peers, wg_subnet=_wg_subnet, cell_subnets=_cell_subnets) firewall_manager.apply_all_cell_rules(cell_links) firewall_manager.ensure_cell_api_dnat() # Embed DNAT rules in PostUp so they survive WireGuard interface restarts, diff --git a/api/firewall_manager.py b/api/firewall_manager.py index e79d2b6..77773fb 100644 --- a/api/firewall_manager.py +++ b/api/firewall_manager.py @@ -153,18 +153,27 @@ def clear_peer_rules(peer_ip: str) -> None: logger.error(f"clear_peer_rules({peer_ip}): {e}") -def apply_peer_rules(peer_ip: str, settings: Dict[str, Any]) -> bool: +def apply_peer_rules(peer_ip: str, settings: Dict[str, Any], + wg_subnet: str = '10.0.0.0/24', + cell_subnets: Optional[List[str]] = None) -> bool: """ Apply iptables FORWARD rules for a peer based on their access settings. + wg_subnet: the local cell's WireGuard VPN subnet (e.g. '10.0.2.0/24'). + Used for the peer-to-peer ACCEPT/DROP rule. Defaults to the + legacy hardcoded value so callers that don't yet pass it are safe. + cell_subnets: list of connected cells' vpn_subnet strings. When provided, + explicit ACCEPT rules are added so split-tunnel peers can reach + connected cells regardless of the internet_access setting. + Each rule is inserted at position 1 (-I), so the LAST call ends up at the TOP. We insert in reverse-priority order: lowest-priority rules first, highest last. Desired final chain order (top = highest priority): - 1. Per-service DROP/ACCEPT (most specific — must beat private-net ACCEPT) - 2. Peer-to-peer ACCEPT/DROP (10.0.0.0/24) - 3. Private-net ACCEPTs (for no-internet peers to reach local resources) - 4. Internet DROP or ACCEPT (lowest priority catch-all) + 1. Connected-cell subnet ACCEPTs (explicit cross-cell routing) + 2. Peer-to-peer ACCEPT/DROP (local VPN subnet) + 3. Private-net ACCEPTs (for no-internet peers to reach local resources) + 4. Internet DROP or ACCEPT (lowest priority catch-all) """ try: comment = _peer_comment(peer_ip) @@ -187,24 +196,34 @@ def apply_peer_rules(peer_ip: str, settings: Dict[str, Any]) -> bool: _iptables(['-I', 'FORWARD', '-s', peer_ip, '-d', net, '-m', 'comment', '--comment', comment, '-j', 'ACCEPT']) - # --- Step 2 --- Peer-to-peer (10.0.0.0/24) + # --- Step 2 --- Peer-to-peer: use the actual local VPN subnet target = 'ACCEPT' if peer_access else 'DROP' - _iptables(['-I', 'FORWARD', '-s', peer_ip, '-d', '10.0.0.0/24', + _iptables(['-I', 'FORWARD', '-s', peer_ip, '-d', wg_subnet, '-m', 'comment', '--comment', comment, '-j', target]) + # --- Step 3 --- Explicit ACCEPT for each connected cell's subnet so + # split-tunnel peers can route to connected cells (wg0 → wg0 forwarding). + if cell_subnets: + for subnet in reversed(cell_subnets): + _iptables(['-I', 'FORWARD', '-s', peer_ip, '-d', subnet, + '-m', 'comment', '--comment', comment, '-j', 'ACCEPT']) + # Service access restriction is done entirely by CoreDNS ACL. # No per-peer iptables rule for Caddy:80 — blocking it would also # prevent the peer from reaching the PIC web UI and API. logger.info(f"Applied rules for {peer_ip}: internet={internet_access} " - f"services={service_access} peers={peer_access}") + f"services={service_access} peers={peer_access} " + f"wg_subnet={wg_subnet} cell_subnets={cell_subnets}") return True except Exception as e: logger.error(f"apply_peer_rules({peer_ip}): {e}") return False -def apply_all_peer_rules(peers: List[Dict[str, Any]]) -> None: +def apply_all_peer_rules(peers: List[Dict[str, Any]], + wg_subnet: str = '10.0.0.0/24', + cell_subnets: Optional[List[str]] = None) -> None: """Re-apply rules for all peers (called on startup).""" ensure_caddy_virtual_ips() for peer in peers: @@ -215,7 +234,7 @@ def apply_all_peer_rules(peers: List[Dict[str, Any]]) -> None: 'internet_access': peer.get('internet_access', True), 'service_access': peer.get('service_access', list(SERVICE_IPS.keys())), 'peer_access': peer.get('peer_access', True), - }) + }, wg_subnet=wg_subnet, cell_subnets=cell_subnets) def reconcile_stale_peer_rules(peers: List[Dict[str, Any]]) -> int: diff --git a/api/network_manager.py b/api/network_manager.py index 48b5106..115a321 100644 --- a/api/network_manager.py +++ b/api/network_manager.py @@ -460,7 +460,8 @@ class NetworkManager(BaseServiceManager): except Exception as e: warnings.append(f"dnsmasq domain update failed: {e}") - # 2. Regenerate Corefile using generate_corefile so it always stays consistent + # 2. Regenerate Corefile — include cell-to-cell forwarding stanzas so a + # domain/ip_range change doesn't wipe cross-cell DNS forwarding zones. try: import firewall_manager as _fm corefile = os.path.join(self.config_dir, 'dns', 'Corefile') @@ -470,7 +471,13 @@ class NetworkManager(BaseServiceManager): peers = _json.loads(open(peers_file).read()) if os.path.exists(peers_file) else [] except Exception: peers = [] - _fm.generate_corefile(peers, corefile, domain) + cell_links_file = os.path.join(self.data_dir, 'cell_links.json') + try: + import json as _json2 + cell_links = _json2.loads(open(cell_links_file).read()) if os.path.exists(cell_links_file) else [] + except Exception: + cell_links = [] + _fm.generate_corefile(peers, corefile, domain, cell_links=cell_links) except Exception as e: warnings.append(f"Corefile domain update failed: {e}") diff --git a/api/routes/peers.py b/api/routes/peers.py index 129f7d1..b55f838 100644 --- a/api/routes/peers.py +++ b/api/routes/peers.py @@ -38,6 +38,13 @@ def add_peer(): from app import (peer_registry, wireguard_manager, firewall_manager, email_manager, calendar_manager, file_manager, auth_manager, cell_link_manager, _configured_domain, COREFILE_PATH) + try: + _wg_addr = wireguard_manager._get_configured_address() + _wg_subnet = str(ipaddress.ip_network(_wg_addr, strict=False)) if _wg_addr else '10.0.0.0/24' + except Exception: + _wg_subnet = '10.0.0.0/24' + _cell_links = cell_link_manager.list_connections() + _cell_subnets = [l['vpn_subnet'] for l in _cell_links if l.get('vpn_subnet')] data = request.get_json(silent=True) if data is None: return jsonify({"error": "No data provided"}), 400 @@ -118,7 +125,8 @@ def add_peer(): return jsonify({"error": f"Peer {peer_name} already exists"}), 400 peer_added_to_registry = True - firewall_manager.apply_peer_rules(peer_info['ip'], peer_info) + firewall_manager.apply_peer_rules(peer_info['ip'], peer_info, + wg_subnet=_wg_subnet, cell_subnets=_cell_subnets) firewall_applied = True wg_allowed = f"{assigned_ip}/32" if '/' not in assigned_ip else assigned_ip @@ -153,7 +161,15 @@ def add_peer(): @bp.route('/api/peers/', methods=['PUT']) def update_peer(peer_name): try: - from app import peer_registry, firewall_manager, cell_link_manager, _configured_domain, COREFILE_PATH + from app import (peer_registry, wireguard_manager, firewall_manager, + cell_link_manager, _configured_domain, COREFILE_PATH) + try: + _wg_addr = wireguard_manager._get_configured_address() + _wg_subnet = str(ipaddress.ip_network(_wg_addr, strict=False)) if _wg_addr else '10.0.0.0/24' + except Exception: + _wg_subnet = '10.0.0.0/24' + _cell_links = cell_link_manager.list_connections() + _cell_subnets = [l['vpn_subnet'] for l in _cell_links if l.get('vpn_subnet')] data = request.get_json(silent=True) or {} existing = peer_registry.get_peer(peer_name) if not existing: @@ -173,7 +189,8 @@ def update_peer(peer_name): if success: updated_peer = peer_registry.get_peer(peer_name) if updated_peer: - firewall_manager.apply_peer_rules(updated_peer['ip'], updated_peer) + firewall_manager.apply_peer_rules(updated_peer['ip'], updated_peer, + wg_subnet=_wg_subnet, cell_subnets=_cell_subnets) firewall_manager.apply_all_dns_rules(peer_registry.list_peers(), COREFILE_PATH, _configured_domain(), cell_links=cell_link_manager.list_connections()) return jsonify({"message": f"Peer {peer_name} updated", "config_changed": config_changed}) diff --git a/api/routes/wireguard.py b/api/routes/wireguard.py index 2346d92..820df4d 100644 --- a/api/routes/wireguard.py +++ b/api/routes/wireguard.py @@ -1,4 +1,5 @@ import logging +import ipaddress from flask import Blueprint, request, jsonify logger = logging.getLogger('picell') bp = Blueprint('wireguard', __name__) @@ -221,11 +222,19 @@ def refresh_external_ip(): @bp.route('/api/wireguard/apply-enforcement', methods=['POST']) def apply_wireguard_enforcement(): try: - from app import peer_registry, firewall_manager, cell_link_manager, _configured_domain, COREFILE_PATH + from app import (peer_registry, wireguard_manager, firewall_manager, + cell_link_manager, _configured_domain, COREFILE_PATH) peers = peer_registry.list_peers() - firewall_manager.apply_all_peer_rules(peers) + try: + _wg_addr = wireguard_manager._get_configured_address() + _wg_subnet = str(ipaddress.ip_network(_wg_addr, strict=False)) if _wg_addr else '10.0.0.0/24' + except Exception: + _wg_subnet = '10.0.0.0/24' + _cell_links = cell_link_manager.list_connections() + _cell_subnets = [l['vpn_subnet'] for l in _cell_links if l.get('vpn_subnet')] + firewall_manager.apply_all_peer_rules(peers, wg_subnet=_wg_subnet, cell_subnets=_cell_subnets) firewall_manager.apply_all_dns_rules(peers, COREFILE_PATH, _configured_domain(), - cell_links=cell_link_manager.list_connections()) + cell_links=_cell_links) return jsonify({'ok': True, 'peers': len(peers)}) except Exception as e: return jsonify({'error': str(e)}), 500 diff --git a/tests/test_firewall_manager.py b/tests/test_firewall_manager.py index 771c5ba..f61d030 100644 --- a/tests/test_firewall_manager.py +++ b/tests/test_firewall_manager.py @@ -308,6 +308,87 @@ class TestApplyPeerRules(unittest.TestCase): for c in vpn_rules: self.assertIn('DROP', c) + def test_custom_wg_subnet_replaces_default(self): + """wg_subnet parameter is used instead of hardcoded 10.0.0.0/24.""" + calls_made = [] + + def fake_wg_exec(args): + calls_made.append(args) + m = MagicMock() + m.returncode = 0 + m.stdout = '' + return m + + with patch.object(firewall_manager, '_wg_exec', side_effect=fake_wg_exec): + firewall_manager.apply_peer_rules( + '10.0.2.5', + {'internet_access': True, 'peer_access': True, + 'service_access': list(firewall_manager.SERVICE_IPS.keys())}, + wg_subnet='10.0.2.0/24', + ) + + iptables_calls = [c for c in calls_made if 'iptables' in c] + subnets_in_rules = [token for c in iptables_calls for token in c + if '/' in token and token.startswith('10.')] + self.assertIn('10.0.2.0/24', subnets_in_rules, + "Custom wg_subnet should appear in peer-to-peer FORWARD rule") + self.assertNotIn('10.0.0.0/24', subnets_in_rules, + "Default hardcoded subnet must not appear when custom wg_subnet given") + + def test_cell_subnets_get_explicit_accept_rules(self): + """Each cell subnet gets an explicit ACCEPT rule for cross-cell routing.""" + calls_made = [] + + def fake_wg_exec(args): + calls_made.append(args) + m = MagicMock() + m.returncode = 0 + m.stdout = '' + return m + + with patch.object(firewall_manager, '_wg_exec', side_effect=fake_wg_exec): + firewall_manager.apply_peer_rules( + '10.0.2.5', + {'internet_access': False, 'peer_access': True, + 'service_access': list(firewall_manager.SERVICE_IPS.keys())}, + wg_subnet='10.0.2.0/24', + cell_subnets=['10.0.0.0/24', '10.0.1.0/24'], + ) + + iptables_calls = [c for c in calls_made if 'iptables' in c] + for cell_net in ('10.0.0.0/24', '10.0.1.0/24'): + cell_rules = [c for c in iptables_calls if '-d' in c and cell_net in c] + self.assertTrue(cell_rules, + f"Expected FORWARD rule for cell subnet {cell_net}") + for c in cell_rules: + self.assertIn('ACCEPT', c, + f"Cell subnet rule for {cell_net} must be ACCEPT") + + def test_no_cell_subnets_no_extra_rules(self): + """When cell_subnets is empty/None, no extra FORWARD rules are added.""" + calls_made = [] + + def fake_wg_exec(args): + calls_made.append(args) + m = MagicMock() + m.returncode = 0 + m.stdout = '' + return m + + with patch.object(firewall_manager, '_wg_exec', side_effect=fake_wg_exec): + firewall_manager.apply_peer_rules( + '10.0.2.5', + {'internet_access': True, 'peer_access': True, + 'service_access': list(firewall_manager.SERVICE_IPS.keys())}, + wg_subnet='10.0.2.0/24', + cell_subnets=[], + ) + + iptables_calls = [c for c in calls_made if 'iptables' in c and '-I' in c] + # Only 2 rules expected: the catch-all ACCEPT + the peer-to-peer ACCEPT + self.assertEqual(len(iptables_calls), 2, + f"Expected exactly 2 INSERT rules, got: {iptables_calls}") + # --------------------------------------------------------------------------- # apply_all_peer_rules @@ -330,6 +411,18 @@ class TestApplyAllPeerRules(unittest.TestCase): firewall_manager.apply_all_peer_rules(peers) self.assertEqual(mock_apply.call_count, 1) + def test_wg_subnet_and_cell_subnets_forwarded(self): + """apply_all_peer_rules passes wg_subnet and cell_subnets to each apply_peer_rules call.""" + peers = [_make_peer('10.0.2.2')] + cell_subnets = ['10.0.0.0/24', '10.0.1.0/24'] + with patch.object(firewall_manager, 'ensure_caddy_virtual_ips', return_value=True), \ + patch.object(firewall_manager, 'apply_peer_rules', return_value=True) as mock_apply: + firewall_manager.apply_all_peer_rules(peers, wg_subnet='10.0.2.0/24', + cell_subnets=cell_subnets) + call_kwargs = mock_apply.call_args_list[0].kwargs + self.assertEqual(call_kwargs.get('wg_subnet'), '10.0.2.0/24') + self.assertEqual(call_kwargs.get('cell_subnets'), cell_subnets) + # --------------------------------------------------------------------------- # clear_peer_rules