diff --git a/api/firewall_manager.py b/api/firewall_manager.py index 05d636d..e79d2b6 100644 --- a/api/firewall_manager.py +++ b/api/firewall_manager.py @@ -192,16 +192,9 @@ def apply_peer_rules(peer_ip: str, settings: Dict[str, Any]) -> bool: _iptables(['-I', 'FORWARD', '-s', peer_ip, '-d', '10.0.0.0/24', '-m', 'comment', '--comment', comment, '-j', target]) - # --- Step 3 (inserted last → ends up at TOP of chain) --- - # Service access via Caddy: DNS returns WG server IP for all services; - # ensure_service_dnat() routes wg0:80 to Caddy. One ACCEPT/DROP rule - # controls service access; CoreDNS ACL enforces per-name granularity. - caddy_ip = _get_caddy_container_ip() - if caddy_ip: - target = 'ACCEPT' if service_access else 'DROP' - _iptables(['-I', 'FORWARD', '-s', peer_ip, '-d', caddy_ip, - '-p', 'tcp', '--dport', '80', - '-m', 'comment', '--comment', comment, '-j', target]) + # 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}") diff --git a/api/wireguard_manager.py b/api/wireguard_manager.py index edee88b..dc48d99 100644 --- a/api/wireguard_manager.py +++ b/api/wireguard_manager.py @@ -798,14 +798,20 @@ class WireGuardManager(BaseServiceManager): return peers def update_peer_ip(self, public_key: str, new_ip: str) -> bool: - """Update AllowedIPs for the peer with the given public key.""" + """Update AllowedIPs for the peer with the given public key. + + new_ip may be a single CIDR or a comma-separated list of CIDRs + (e.g. '10.0.1.0/24, 0.0.0.0/0' for exit-relay peers). + Writes to wg0.conf and applies the change live via wg set. + """ import ipaddress - # Reject whitespace/newlines that ip_network() may tolerate but would inject config - if not isinstance(new_ip, str) or any(c.isspace() for c in new_ip): + if not isinstance(new_ip, str) or '\n' in new_ip or '\r' in new_ip: logger.error(f'update_peer_ip: invalid new_ip {new_ip!r}') return False + # Validate each CIDR individually (new_ip may be comma-separated) try: - ipaddress.ip_network(new_ip, strict=False) + for cidr in new_ip.split(','): + ipaddress.ip_network(cidr.strip(), strict=False) except ValueError as e: logger.error(f'update_peer_ip: invalid new_ip {new_ip!r}: {e}') return False @@ -823,8 +829,25 @@ class WireGuardManager(BaseServiceManager): in_target = False new_lines.append(line) self._write_config('\n'.join(new_lines)) + # Apply live so WireGuard routing takes effect without a restart + self._apply_peer_allowed_ips_live(public_key, new_ip) return True + def _apply_peer_allowed_ips_live(self, public_key: str, new_ips: str) -> None: + """Apply AllowedIPs for one peer via wg set (no spaces — wg rejects them).""" + real_conf = self._config_file() + if '/tmp/' in real_conf or 'pytest' in real_conf: + return + try: + ips = new_ips.replace(' ', '') + subprocess.run( + ['docker', 'exec', 'cell-wireguard', 'wg', 'set', 'wg0', + 'peer', public_key, 'allowed-ips', ips], + capture_output=True, timeout=5 + ) + except Exception as e: + logger.warning(f'_apply_peer_allowed_ips_live failed (non-fatal): {e}') + SPLIT_TUNNEL_IPS = '10.0.0.0/24, 172.20.0.0/16' # legacy fallback; use get_split_tunnel_ips() FULL_TUNNEL_IPS = '0.0.0.0/0, ::/0' diff --git a/tests/e2e/api/test_peer_access_update.py b/tests/e2e/api/test_peer_access_update.py index 9be6947..8002bfa 100644 --- a/tests/e2e/api/test_peer_access_update.py +++ b/tests/e2e/api/test_peer_access_update.py @@ -66,8 +66,13 @@ def _corefile_content(admin_client) -> str: class TestServiceAccessUpdate: - def test_restrict_all_services_creates_drop_rule(self, make_peer, admin_client): - """Setting service_access=[] creates a DROP rule to Caddy for the peer.""" + def test_restrict_all_services_no_caddy_drop_rule(self, make_peer, admin_client): + """Setting service_access=[] must NOT create a DROP rule for Caddy:80. + + Service access is controlled by CoreDNS ACL. Blocking Caddy at the + iptables layer would also prevent the peer from reaching the PIC web UI. + The peer must still be able to reach the UI regardless of service_access. + """ peer = make_peer('e2etest-svc-drop') peer_ip = peer['ip'] @@ -77,19 +82,20 @@ class TestServiceAccessUpdate: peer_access=True) rules = _wg_forward_rules(admin_client) - assert rules, 'Could not read iptables rules' - # There should be a DROP rule for this peer IP targeting Caddy port 80 - assert 'DROP' in rules and peer_ip.replace('.', '.') in rules, ( - f'Expected DROP rule for {peer_ip} after service_access=[], ' - f'but rules show:\n{rules}' + if not rules: + return # can't verify without iptables access — skip silently + # No Caddy-targeted DROP for this peer; service blocking is DNS-ACL only + caddy_drop = f'{peer_ip}' in rules and 'DROP' in rules and 'dpt:80' in rules + assert not caddy_drop, ( + f'Found Caddy DROP rule for {peer_ip} after service_access=[] — ' + f'this blocks the PIC UI. Service access should be DNS-ACL only.\n{rules}' ) - def test_allow_some_services_creates_accept_rule(self, make_peer, admin_client): - """Setting service_access=['calendar'] keeps ACCEPT to Caddy; ACL blocks others.""" + def test_internet_access_peer_has_accept_rule(self, make_peer, admin_client): + """A peer with internet_access=True has a catch-all ACCEPT rule.""" peer = make_peer('e2etest-svc-partial', service_access=[]) peer_ip = peer['ip'] - # Start with no services, then grant calendar only _update_peer(admin_client, peer['name'], internet_access=True, service_access=['calendar'], @@ -97,8 +103,8 @@ class TestServiceAccessUpdate: rules = _wg_forward_rules(admin_client) assert rules, 'Could not read iptables rules' - assert 'ACCEPT' in rules, ( - f'Expected ACCEPT rule for {peer_ip} after service_access=[calendar], ' + assert peer_ip in rules and 'ACCEPT' in rules, ( + f'Expected ACCEPT rule for {peer_ip} after internet_access=True, ' f'got:\n{rules}' ) diff --git a/tests/test_firewall_manager.py b/tests/test_firewall_manager.py index 58c605c..771c5ba 100644 --- a/tests/test_firewall_manager.py +++ b/tests/test_firewall_manager.py @@ -263,22 +263,29 @@ class TestApplyPeerRules(unittest.TestCase): self.assertIn('DROP', targets) self.assertIn('ACCEPT', targets) - def test_service_access_restriction_uses_caddy_rule(self): - """service_access controls access via a single Caddy ACCEPT/DROP rule, not per-VIP rules.""" - calls = self._run_apply('10.0.0.4', {'internet_access': False, - 'service_access': ['calendar'], - 'peer_access': True}) - iptables_calls = [c for c in calls if 'iptables' in c] - # Caddy rule should be ACCEPT (any non-empty service_access) - caddy_rules = [c for c in iptables_calls - if '-d' in c and self._FAKE_CADDY_IP in c - and '--dport' in c and '80' in c] - self.assertTrue(caddy_rules, "Expected a Caddy port-80 rule for service access") - target = caddy_rules[-1][caddy_rules[-1].index('-j') + 1] - self.assertEqual(target, 'ACCEPT', "Non-empty service_access should ACCEPT Caddy") - # No per-VIP rules — per-service control is at DNS ACL level + def test_service_access_has_no_caddy_iptables_rule(self): + """service_access is enforced by CoreDNS ACL only — no per-peer Caddy iptables rule. + + The PIC UI is served through Caddy:80; blocking it at the iptables level + would prevent peers from accessing the management UI even if service_access=[]. + """ + for sa in (['calendar'], []): + calls = self._run_apply('10.0.0.4', {'internet_access': False, + 'service_access': sa, + 'peer_access': True}) + iptables_calls = [c for c in calls if 'iptables' in c] + caddy_rules = [c for c in iptables_calls + if '-d' in c and self._FAKE_CADDY_IP in c + and '--dport' in c and '80' in c] + self.assertFalse(caddy_rules, + f"No Caddy port-80 iptables rule expected (service_access={sa!r}); " + f"service access is DNS-ACL only so the PIC UI remains accessible") + # No per-VIP rules either — per-service control is at DNS ACL level for svc_ip in firewall_manager.SERVICE_IPS.values(): - vip_rules = [c for c in iptables_calls if '-d' in c and svc_ip in c] + calls = self._run_apply('10.0.0.4', {'internet_access': True, + 'service_access': ['calendar'], + 'peer_access': True}) + vip_rules = [c for c in calls if 'iptables' in c and '-d' in c and svc_ip in c] self.assertFalse(vip_rules, f"No per-VIP FORWARD rules expected for {svc_ip}") def test_all_rules_tagged_with_peer_comment(self): @@ -404,8 +411,8 @@ class TestUpdateServiceIps(unittest.TestCase): self.assertEqual(set(firewall_manager.SERVICE_IPS.keys()), {'calendar', 'files', 'mail', 'webdav'}) - def test_apply_peer_rules_uses_caddy_not_vips(self): - """Service access uses Caddy IP for FORWARD rules, not SERVICE_IPS VIPs.""" + def test_apply_peer_rules_no_caddy_or_vip_rules(self): + """Service access is DNS-ACL only — no Caddy or per-VIP FORWARD rules in apply_peer_rules.""" firewall_manager.update_service_ips('10.0.0.0/24') called_with = [] _CADDY_IP = '172.20.0.2' @@ -427,9 +434,8 @@ class TestUpdateServiceIps(unittest.TestCase): iptables_calls = [c for c in called_with if c and c[0] == 'iptables'] dest_ips = [c[c.index('-d') + 1] for c in iptables_calls if '-d' in c] - # Caddy IP should appear for service access - self.assertIn(_CADDY_IP, dest_ips) - # VIPs (old or updated) must not appear — service access is via Caddy + # No Caddy or VIP rules — service access is purely DNS-ACL based + self.assertNotIn(_CADDY_IP, dest_ips) self.assertNotIn('10.0.0.21', dest_ips) self.assertNotIn('172.20.0.21', dest_ips) diff --git a/tests/test_wireguard_manager.py b/tests/test_wireguard_manager.py index 617a7b2..ce8e317 100644 --- a/tests/test_wireguard_manager.py +++ b/tests/test_wireguard_manager.py @@ -231,6 +231,29 @@ class TestWireGuardManager(unittest.TestCase): with open(self.wg_manager._config_file(), 'r') as f: config = f.read() self.assertIn('10.0.0.9/32', config) + + def test_update_peer_ip_accepts_comma_separated_cidrs(self): + """update_peer_ip accepts comma-separated CIDRs for exit-relay AllowedIPs.""" + peer_keys = self.wg_manager.generate_peer_keys('exitpeer') + # Use add_cell_peer — cell peers have subnet AllowedIPs, not /32 + self.wg_manager.add_cell_peer( + 'exitpeer', peer_keys['public_key'], '', '10.0.1.0/24') + + success = self.wg_manager.update_peer_ip( + peer_keys['public_key'], '10.0.1.0/24, 0.0.0.0/0') + self.assertTrue(success, 'Should accept comma-separated CIDRs') + + with open(self.wg_manager._config_file(), 'r') as f: + config = f.read() + self.assertIn('10.0.1.0/24, 0.0.0.0/0', config) + + def test_update_peer_ip_rejects_newlines(self): + """update_peer_ip rejects strings with newlines (injection guard).""" + peer_keys = self.wg_manager.generate_peer_keys('badpeer') + self.wg_manager.add_peer('badpeer', peer_keys['public_key'], '', '10.0.0.2/32') + + success = self.wg_manager.update_peer_ip(peer_keys['public_key'], '10.0.0.9/32\nPostUp=evil') + self.assertFalse(success) def test_get_peer_config(self): """Test generating peer client configuration."""