diff --git a/tests/integration/test_private_ports.py b/tests/integration/test_private_ports.py index 263aa1c..5239397 100644 --- a/tests/integration/test_private_ports.py +++ b/tests/integration/test_private_ports.py @@ -2,16 +2,18 @@ Run: UVM_FIREWALL_INTEGRATION=1 python3 -B -m unittest discover -s tests/integration -v -Needs Linux, nft, ip, bridge, conntrack, sysctl and permission to create network +Needs Linux, nft, iptables, ip, bridge, conntrack, sysctl and permission to create network namespaces (root, or unprivileged user namespaces). No sudo, installation, /run/netns mounts, host firewall changes or br_netfilter sysctls are used. -Bridge-connected veths stand in for guest TAPs. A third, routed namespace tests -guest egress without NAT. Ordinary test runs skip without executing commands. +Bridge-connected veths stand in for guest TAPs. A third namespace with two +uplinks tests NAT, offline startup, reconnects and host route changes without +using the real internet. Ordinary test runs skip without executing commands. """ from __future__ import annotations import contextlib +import errno import json import os import re @@ -62,6 +64,13 @@ class SocketEndpoint: except TimeoutError: sock.close() return False + except OSError as error: + sock.close() + if operation == "connect" and error.errno in { + errno.ENETUNREACH, errno.EHOSTUNREACH, errno.ENETDOWN, errno.ECONNREFUSED, + }: + return False + raise except BaseException: sock.close() raise @@ -206,6 +215,7 @@ def _sandbox(parent_fd): from uvm.domain import PortRule, VmRecord from uvm.errors import UvmError from uvm.firewall import FirewallManager + from uvm.network import NetworkManager from uvm.system import CommandRunner runner = CommandRunner(emit=None) @@ -220,11 +230,12 @@ def _sandbox(parent_fd): manager = FirewallManager(settings, runner) manager.check_available() run("ip", "link", "set", "lo", "up") - run("ip", "link", "add", settings.bridge, "type", "bridge", "stp_state", "0") - run("ip", "addr", "add", f"{settings.gateway}/24", "dev", settings.bridge) - run("ip", "link", "set", settings.bridge, "up") - # This IPv4 sysctl belongs only to the freshly verified network namespace. - run("sysctl", "-w", "net.ipv4.ip_forward=1") + # All changes are inside the freshly verified namespace. A pre-existing + # terminal drop catches regressions that append ineffective forward rules. + run("iptables", "-P", "FORWARD", "DROP") + run("iptables", "-A", "FORWARD", "-j", "DROP") + assert not run("ip", "route", "show", "default").strip() + NetworkManager(settings, runner).ensure_bridge() metadata = dict(cpu=1, ram_mib=128, gateway=str(settings.gateway), socket="", config="", log="", network=str(settings.network), bridge=settings.bridge) @@ -263,6 +274,16 @@ def _sandbox(parent_fd): run("ip", "link", "set", tap, "type", "bridge_slave", "group_fwd_mask", "0x4000") endpoint.request("run", command=["ip", "route", "add", "default", "via", str(settings.gateway)]) + run("ip", "link", "add", "fw-up2", "type", "veth", "peer", "name", "fw-end2") + run("ip", "link", "set", "fw-end2", "netns", str(outside.process.pid)) + run("ip", "addr", "add", "172.30.74.1/24", "dev", "fw-up2") + run("ip", "link", "set", "fw-up2", "up") + outside.request("run", command=["ip", "link", "set", "fw-end2", "name", "eth1"]) + outside.request("run", command=["ip", "addr", "add", "172.30.74.2/24", "dev", "eth1"]) + outside.request("run", command=["ip", "link", "set", "eth1", "up"]) + internet_ip = "203.0.113.73" + outside.request("run", command=["ip", "addr", "add", f"{internet_ip}/32", "dev", "lo"]) + manager.apply(vm) manager.apply(peer_vm) for tap in (vm.tap, peer_vm.tap): @@ -328,6 +349,7 @@ def _sandbox(parent_fd): guest.request("udp_send", name="dns", ip="172.30.73.2", port=53, data="dns-query") query = outside.request("udp_recv", name="dns") assert query[0] == "dns-query" + assert query[1][0] == "172.30.73.1", "guest egress must use the host uplink's address" outside.request("udp_send", name="dns", ip=query[1][0], port=query[1][1], data="dns-reply") assert guest.request("udp_recv", name="dns")[0] == "dns-reply" guest.request("udp_bind", name="unsolicited", ip=vm.guest_ip, port=40054) @@ -338,6 +360,44 @@ def _sandbox(parent_fd): assert endpoint.request("ping", ip=target), (endpoint, target) print("PASS: independent peer policy, outbound UDP/DNS and ICMP; unsolicited UDP blocked", flush=True) + # No UVM reconfiguration after this point: the kernel must follow the + # host routes for both TCP and UDP/53, including after offline creation. + outside.request("listen", name="internet", ip=internet_ip, port=9443) + outside.request("udp_bind", name="internet-dns", ip=internet_ip, port=53) + + def offline(label): + assert not host.request("connect", name=f"host-{label}", ip=internet_ip, port=9443) + assert not guest.request("connect", name=f"guest-{label}", ip=internet_ip, port=9443) + assert guest.request("ping", ip=str(settings.gateway)), "local networking must still work offline" + + def online(label, source): + for endpoint, role in ((host, "host"), (guest, "guest")): + name = f"{role}-{label}" + assert endpoint.request("connect", name=name, ip=internet_ip, port=9443) + assert outside.request("accept", listener="internet", name=name) + transfer(endpoint, outside, name, "request") + transfer(outside, endpoint, name, "response") + guest.request("udp_bind", name=label, ip=vm.guest_ip, port=0) + guest.request("udp_send", name=label, ip=internet_ip, port=53, data=label) + query = outside.request("udp_recv", name="internet-dns") + assert query == [label, [source, query[1][1]]], query + outside.request("udp_send", name="internet-dns", ip=source, port=query[1][1], data="dns-reply") + assert guest.request("udp_recv", name=label)[0] == "dns-reply" + + offline("initially-offline") + run("ip", "route", "add", "default", "via", "172.30.73.2", "dev", "fw-up") + online("first-uplink", "172.30.73.1") + run("ip", "route", "replace", "default", "via", "172.30.74.2", "dev", "fw-up2") + online("changed-uplink", "172.30.74.1") + run("ip", "route", "del", "default") + run("ip", "link", "set", "fw-up", "down") + run("ip", "link", "set", "fw-up2", "down") + offline("disconnected") + run("ip", "link", "set", "fw-up", "up") + run("ip", "route", "add", "default", "via", "172.30.73.2", "dev", "fw-up") + online("reconnected", "172.30.73.1") + print("PASS: host/guest NAT egress follows offline/online, uplink changes and reconnects", flush=True) + # Spoofed source MACs must not reach FDB learning, including the local # link-local path which can skip bridge prerouting entirely. spoof = bytes.fromhex("02aabbccddee") @@ -393,7 +453,7 @@ class PrivatePortNamespaceTests(unittest.TestCase): probe = subprocess.run((*prefix, "true"), text=True, capture_output=True, timeout=5) if probe.returncode: self.skipTest(f"isolated namespace permission unavailable: {probe.stderr.strip()}") - missing = [name for name in ("ip", "bridge", "nft", "conntrack", "sysctl") if shutil.which(name) is None] + missing = [name for name in ("ip", "bridge", "nft", "iptables", "conntrack", "sysctl") if shutil.which(name) is None] if missing: self.skipTest(f"integration tools unavailable: {', '.join(missing)}") with open("/proc/self/ns/net", "rb") as parent: diff --git a/tests/test_lifecycle_commands.py b/tests/test_lifecycle_commands.py index 8ef7c55..6c936dd 100644 --- a/tests/test_lifecycle_commands.py +++ b/tests/test_lifecycle_commands.py @@ -385,7 +385,7 @@ class LifecyclePortTests(LifecycleTestCase): self.assertEqual(self.settings.state_path.read_bytes(), before) self.assertEqual(self.host.events, []) - def test_start_of_running_vm_repairs_policy_without_reboot_or_reprovision(self) -> None: + def test_start_of_running_vm_repairs_policy_and_egress_without_reboot_or_reprovision(self) -> None: vm = self.create_vm() del self.network.firewall.tables[vm.id] self.host.events.clear() @@ -395,9 +395,34 @@ class LifecyclePortTests(LifecycleTestCase): self.assertNotIn("process.start", self.host.actions(vm.id)) self.assertNotIn("process.terminate", self.host.actions(vm.id)) self.assertNotIn("client.configure", self.host.actions(vm.id)) + self.assertIn(("network.ensure_bridge", None), self.host.events) + self.assertLess(self.host.events.index(("firewall.apply", vm.id)), + self.host.events.index(("network.ensure_bridge", None))) self.assertEqual(self.provision.call_count, 1) self.assertEqual(self.copy_disk.call_count, 1) + def test_live_start_rejects_changed_network_settings_before_repairing_network(self) -> None: + vm = self.create_vm() + self.service._settings = replace(self.settings, bridge="another-bridge") + before = self.settings.state_path.read_bytes() + self.host.events.clear() + with self.assertRaisesRegex(ConflictError, "retained network"): + self.service.start(vm.id) + self.assertEqual(self.settings.state_path.read_bytes(), before) + self.assertNotIn(("network.ensure_bridge", None), self.host.events) + self.assertNotIn("firewall.apply", self.host.actions(vm.id)) + + def test_live_start_can_retry_an_egress_setup_failure_with_the_same_process(self) -> None: + vm = self.create_vm() + self.host.failures["network.ensure_bridge", None] = UvmError("egress setup failed") + with self.assertRaisesRegex(UvmError, "egress setup failed"): + self.service.start(vm.id) + self.assertEqual(self.process.observe(self.store.get(vm.id)), "alive") + self.assertEqual(self.network.firewall.status(vm), "enforced") + self.assertEqual(self.service.start(vm.id).pid, vm.pid) + self.assertEqual(len(self.process.started), 1) + self.assertEqual(self.provision.call_count, 1) + class LifecycleBulkTests(LifecycleTestCase): def test_all_bulk_actions_take_one_outer_lock_and_use_only_selected_ids(self) -> None: diff --git a/tests/test_network.py b/tests/test_network.py index fa583f2..7f4c931 100644 --- a/tests/test_network.py +++ b/tests/test_network.py @@ -5,6 +5,7 @@ import unittest from copy import deepcopy from dataclasses import replace from ipaddress import IPv4Address, IPv4Network +from unittest.mock import patch from uvm.config import Settings from uvm.domain import PortRule, VmRecord @@ -22,6 +23,10 @@ class FakeRunner: self.links: dict[str, dict] = {} self.inventory: str | None = None self.failures: dict[tuple[str, ...], BaseException] = {} + self.default_routes = "default via 192.0.2.1 dev eth0\n" + self.rules: dict[tuple[str, str], list[tuple[str, ...]]] = { + ("nat", "POSTROUTING"): [], ("filter", "FORWARD"): [], + } def run(self, command, *, check=True, capture=False, timeout=None) -> CommandResult: del capture, timeout @@ -68,11 +73,21 @@ class FakeRunner: elif args[:3] == ("ip", "addr", "replace") or args[:2] == ("sysctl", "-w"): pass elif args[0] == "iptables": - return CommandResult(args=args, returncode=1 if "-C" in args else 0) + if args[1:4] != ("--wait", "5", "-t"): + raise AssertionError(f"unexpected iptables options: {args}") + table, operation, chain, *rule = args[4:] + rules = self.rules[table, chain] + if operation == "-C": + return CommandResult(args=args, returncode=0 if tuple(rule) in rules else 1) + if operation == "-I": + position, *rule = rule + rules.insert(int(position) - 1, tuple(rule)) + else: + raise AssertionError(f"unexpected iptables mutation: {args}") elif args != ("ip", "route", "show", "default"): raise AssertionError(f"unexpected host command: {args}") if args == ("ip", "route", "show", "default"): - return CommandResult(args=args, returncode=0, stdout="default via 192.0.2.1 dev eth0\n") + return CommandResult(args=args, returncode=0, stdout=self.default_routes) return CommandResult(args=args, returncode=0) @@ -127,44 +142,61 @@ class NetworkManagerTests(unittest.TestCase): ("ip", "addr", "replace", "10.42.0.1/24", "dev", "uvm0"), self.runner.calls, ) - self.assertIn( - ( - "iptables", - "-A", - "FORWARD", - "-i", - "uvm0", - "-o", - "eth0", - "-s", - "10.42.0.0/24", - "-j", - "ACCEPT", - ), - self.runner.calls, - ) - checks = [ - call - for call in self.runner.calls - if call[:5] == ("iptables", "-t", "nat", "-C", "POSTROUTING") + self.assertIn(("sysctl", "-w", "net.ipv4.ip_forward=1"), self.runner.calls) + nat, = self.runner.rules["nat", "POSTROUTING"] + self.assertEqual(nat[:6], ("-s", "10.42.0.0/24", "!", "-o", "uvm0", "-m")) + self.assertEqual(nat[-2:], ("-j", "MASQUERADE")) + inbound, outbound = self.runner.rules["filter", "FORWARD"] + self.assertEqual(outbound[:4], ("-i", "uvm0", "-s", "10.42.0.0/24")) + self.assertEqual(inbound[:8], ( + "-o", "uvm0", "-d", "10.42.0.0/24", "-m", "conntrack", + "--ctstate", "ESTABLISHED,RELATED", + )) + self.assertEqual(inbound[-2:], ("-j", "ACCEPT")) + self.assertEqual(outbound[-2:], ("-j", "ACCEPT")) + + def test_egress_is_installed_offline_and_survives_uplink_changes_without_extra_rules(self) -> None: + self.runner.default_routes = "" + self.network.ensure_bridge() + offline_rules = deepcopy(self.runner.rules) + self.assertEqual([len(rules) for rules in offline_rules.values()], [1, 2]) + + for route in ( + "default via 192.0.2.1 dev eth0\n", + "default via 198.51.100.1 dev wlan0\n", + "default dev tun0\ndefault via 198.51.100.1 dev wlan0 metric 600\n", + "", + ): + with self.subTest(route=route): + self.runner.default_routes = route + self.runner.calls.clear() + self.network.ensure_bridge() + self.assertEqual(self.runner.rules, offline_rules) + self.assertFalse(any("-I" in call for call in self.runner.calls)) + self.assertFalse(any("eth0" in rule or "wlan0" in rule or "tun0" in rule + for rules in offline_rules.values() for rule in rules)) + + def test_egress_exceptions_precede_existing_drops_and_preserve_foreign_and_legacy_rules(self) -> None: + existing = [ + ("-j", "HOST-FIREWALL"), ("-j", "DROP"), + ("-i", "uvm0", "-o", "eth0", "-s", "10.42.0.0/24", "-j", "ACCEPT"), ] - self.assertEqual(len(checks), 1) - self.assertIn( - ( - "iptables", - "-t", - "nat", - "-A", - "POSTROUTING", - "-s", - "10.42.0.0/24", - "-o", - "eth0", - "-j", - "MASQUERADE", - ), - self.runner.calls, - ) + self.runner.rules["filter", "FORWARD"] = list(existing) + self.network.ensure_bridge() + rules = list(self.runner.rules["filter", "FORWARD"]) + self.assertEqual(rules[2:], existing) + self.assertTrue(all("uvm-egress" in rule and rule[-1] == "ACCEPT" for rule in rules[:2])) + self.network.ensure_bridge() + self.assertEqual(self.runner.rules["filter", "FORWARD"], rules) + + def test_iptables_inspection_errors_are_not_treated_as_missing_rules(self) -> None: + check = ("iptables", "--wait", "5", "-t", "nat", "-C", "POSTROUTING", + "-s", "10.42.0.0/24", "!", "-o", "uvm0", + "-m", "comment", "--comment", "uvm-egress", "-j", "MASQUERADE") + with patch.object(self.runner, "run", return_value=CommandResult(check, 4, stderr="permission denied")) as run: + with self.assertRaisesRegex(CommandError, "could not inspect.*permission denied"): + self.network._ensure_masquerade() + self.assertEqual(run.call_count, 1) def test_bridge_and_guest_network_support_non_default_prefixes(self) -> None: settings = Settings( @@ -178,6 +210,10 @@ class NetworkManagerTests(unittest.TestCase): ("ip", "addr", "replace", "10.50.0.1/16", "dev", "uvm0"), runner.calls, ) + for rules in runner.rules.values(): + for rule in rules: + self.assertIn("10.50.0.0/16", rule) + self.assertNotIn("10.42.0.0/24", rule) def test_refuses_to_adopt_an_existing_tap_device(self) -> None: for alias in (None, "foreign-owner", f"uvm:{self.vm.id}"): diff --git a/uvm/lifecycle.py b/uvm/lifecycle.py index b1231a4..6fc20da 100644 --- a/uvm/lifecycle.py +++ b/uvm/lifecycle.py @@ -177,7 +177,10 @@ class LifecycleService: raise ConflictError(f"resize is pending for {vm.id}; retry resize or stop it before starting") if vm.status != "running" or vm.operation not in {None, "policy"}: raise ConflictError(f"{vm.id} has an interrupted boot/stop; stop it before starting") - return self._sync_ports_locked(vm) + self._validate_retained_network(vm) + vm = self._sync_ports_locked(vm) + self._network.ensure_bridge() + return vm self._validate_resume(vm) if kernel is None: self._check_boot() @@ -276,6 +279,9 @@ class LifecycleService: disk = Path(vm.disk) if not vm.disk or not disk.is_file() or disk.is_symlink() or disk.stat().st_nlink != 1: raise UvmError(f"private VM disk is missing or unsafe: {disk}") + self._validate_retained_network(vm) + + def _validate_retained_network(self, vm: VmRecord) -> None: if (vm.gateway != str(self._settings.gateway) or vm.network not in {None, str(self._settings.network)} or vm.bridge not in {None, self._settings.bridge}): diff --git a/uvm/network.py b/uvm/network.py index 3d779d5..22e07c8 100644 --- a/uvm/network.py +++ b/uvm/network.py @@ -9,7 +9,7 @@ from ipaddress import IPv4Address from .config import Settings from .domain import VmRecord -from .errors import ConflictError, UvmError, ValidationError +from .errors import CommandError, ConflictError, UvmError, ValidationError from .firewall import FirewallManager from .system import CommandRunner @@ -178,111 +178,38 @@ class NetworkManager: raise UvmError(f"could not delete TAP device {name}{suffix}") def _ensure_masquerade(self) -> None: - route = self._runner.run(("ip", "route", "show", "default"), capture=True) - match = re.search(r"\bdev\s+(\S+)", route.stdout) - if match is None: - return - uplink = match.group(1) - rule = ( - "iptables", - "-t", - "nat", - "-C", - "POSTROUTING", - "-s", - str(self._settings.network), - "-o", - uplink, - "-j", - "MASQUERADE", - ) - present = self._runner.run(rule, check=False) - if present.returncode != 0: - self._runner.run( - ( - "iptables", - "-t", - "nat", - "-A", - "POSTROUTING", - "-s", - str(self._settings.network), - "-o", - uplink, - "-j", - "MASQUERADE", - ) - ) + # Match the guest network, not a snapshot of the host's default uplink. + # Routing selects the current interface for each destination, even after + # an offline boot, a reconnect, or a switch between Wi-Fi/Ethernet/VPN. + bridge = self._settings.bridge + network = str(self._settings.network) + owner = ("-m", "comment", "--comment", "uvm-egress") self._ensure_iptables_rule( - ( - "iptables", - "-C", - "FORWARD", - "-i", - self._settings.bridge, - "-o", - uplink, - "-s", - str(self._settings.network), - "-j", - "ACCEPT", - ), - ( - "iptables", - "-A", - "FORWARD", - "-i", - self._settings.bridge, - "-o", - uplink, - "-s", - str(self._settings.network), - "-j", - "ACCEPT", - ), + "nat", "POSTROUTING", + ("-s", network, "!", "-o", bridge, *owner, "-j", "MASQUERADE"), + ) + # Native bridge policy still checks guest identity and private ports. + # This also permits peer traffic when br_netfilter is already enabled. + self._ensure_iptables_rule( + "filter", "FORWARD", + ("-i", bridge, "-s", network, *owner, "-j", "ACCEPT"), ) self._ensure_iptables_rule( - ( - "iptables", - "-C", - "FORWARD", - "-i", - uplink, - "-o", - self._settings.bridge, - "-d", - str(self._settings.network), - "-m", - "conntrack", - "--ctstate", - "ESTABLISHED,RELATED", - "-j", - "ACCEPT", - ), - ( - "iptables", - "-A", - "FORWARD", - "-i", - uplink, - "-o", - self._settings.bridge, - "-d", - str(self._settings.network), - "-m", - "conntrack", - "--ctstate", - "ESTABLISHED,RELATED", - "-j", - "ACCEPT", - ), + "filter", "FORWARD", + ("-o", bridge, "-d", network, "-m", "conntrack", + "--ctstate", "ESTABLISHED,RELATED", *owner, "-j", "ACCEPT"), ) def _ensure_iptables_rule( - self, - check_rule: tuple[str, ...], - add_rule: tuple[str, ...], + self, table: str, chain: str, rule: tuple[str, ...], ) -> None: - present = self._runner.run(check_rule, check=False) - if present.returncode != 0: - self._runner.run(add_rule) + command = ("iptables", "--wait", "5", "-t", table) + present = self._runner.run((*command, "-C", chain, *rule), check=False, capture=True) + if present.returncode == 0: + return + if present.returncode != 1: + detail = present.stderr.strip() or present.stdout.strip() + raise CommandError(f"could not inspect UVM {table}/{chain} rule ({present.returncode}): {detail}") + # Scoped exceptions must precede terminal drops in the host's chains. + # Keep unrelated rules and the global forwarding policy intact. + self._runner.run((*command, "-I", chain, "1", *rule), capture=True)