From 4167c293796d33c16bbd85ee8aebe9f42b4c4e2a Mon Sep 17 00:00:00 2001 From: Will Bennett <77282969+willbennett-devops@users.noreply.github.com> Date: Sat, 31 Oct 2026 16:48:20 -0400 Subject: [PATCH] systemvm: route router replies to remote access VPN clients through ppp On a router with more than one public NIC, anything the router answers itself from an address on a non-source-NAT public NIC (an LB address, ICMP) matched "from lookup Table_ethN" and left through that NIC's gateway instead of the client's ppp interface, so VPN clients could not reach a load balancer on such an address. Add a throw route for each VPN client network to every Table_ethN, so the lookup falls through to main and its ppp host routes. The routes carry their own protocol number and are reconciled on every pass from the active VPN entries, so they follow NIC changes and VPN deletion. Client networks that overlap a router subnet, a static route or a site-to-site peer network are skipped. Packets from a client network that arrive on any interface other than ppp are dropped, so a spoofed source can't have the reply sent into a client's tunnel. --- systemvm/debian/opt/cloud/bin/configure.py | 86 ++++++++ .../test/TestCsRemoteAccessVpnClientRoutes.py | 204 ++++++++++++++++++ 2 files changed, 290 insertions(+) create mode 100644 systemvm/test/TestCsRemoteAccessVpnClientRoutes.py diff --git a/systemvm/debian/opt/cloud/bin/configure.py b/systemvm/debian/opt/cloud/bin/configure.py index 95cb73608468..d5b646af92c0 100755 --- a/systemvm/debian/opt/cloud/bin/configure.py +++ b/systemvm/debian/opt/cloud/bin/configure.py @@ -1216,6 +1216,75 @@ def del_l2tp_ipsec_user(self, user, obj): class CsRemoteAccessVpn(CsDataBag): VPNCONFDIR = "/etc/ipsec.d" + # Route protocol number that marks the client throw routes owned by client_routes() + CLIENT_ROUTE_PROTO = "147" + + def client_networks(self, obj, include_local_ip=False): + # The client range as CIDRs, plus the router's ppp address if asked. CloudStack only refuses + # a range that overlaps the VPC CIDR, so a network that overlaps a subnet on the router or a + # site-to-site peer network is left out and logged: routing it to the tunnels and dropping it + # off the tunnels would take that traffic over. A static route counts only when it equals or + # lies inside the network; a broader one (a supernet via a private gateway) leaves it alone. + start, end = [ipaddress.IPv4Address(a.strip()) for a in obj['ip_range'].split('-')] + nets = list(ipaddress.summarize_address_range(start, end)) + if include_local_ip: + nets.append(ipaddress.IPv4Network(obj['local_ip'] + '/32')) + + def ipv4(cidrs): + return [n for n in (ipaddress.ip_network(c.strip(), strict=False) for c in cidrs if c.strip() not in ("", "ERROR")) if n.version == 4] + + taken = [interface.get_network() for interface in self.config.address().get_interfaces()] + for peer in CsSite2SiteVpn("site2sitevpn", self.config).get_bag().values(): + if isinstance(peer, dict) and peer.get('create', True): + taken += peer.get('peer_guest_cidr_list', '').split(',') + taken = ipv4(taken) + static_routes = CsStaticRoutes("staticroutes", self.config).get_bag() + routes = ipv4([r.get('network', '') for k, r in static_routes.items() if k != "id" and not r.get('revoke')]) + usable = [] + for net in nets: + clash = [str(t) for t in taken if net.overlaps(t)] + [str(r) for r in routes if r.subnet_of(net)] + if clash: + logging.error("VPN client network %s overlaps %s on this router, not routing it to the VPN" % (net, ", ".join(clash))) + else: + usable.append(str(net)) + return usable + + def client_routes(self): + # Replies the router sends to VPN clients from an address on a public NIC (an LB address, + # ICMP) match "from lookup Table_ethX" and would leave by that NIC's gateway. + # A throw for each client network in every per-NIC table sends them on to main, which holds + # the per-session ppp routes, the same way tier networks are thrown. Reconciled on every pass: + # NIC changes rebuild the tables, and a deleted VPN is already gone from the data bag. + rules = CsHelper.execute("ip rule show") + if not rules: + logging.warning("ip rule show returned nothing, leaving VPN client routes alone") + return + tables = set() + for line in rules: + m = re.search(r"\blookup (Table_eth\d+)(?=\s|$)", line) + if m: + tables.add(m.group(1)) + nets = set() + for public_ip in self.dbag: + if public_ip == "id" or not self.dbag[public_ip].get('create'): + continue + # An entry that can't be read adds nothing, like a deleted one; its spoofing rules go too + try: + nets.update(self.client_networks(self.dbag[public_ip], include_local_ip=True)) + except Exception as e: + logging.error("Failed to read the VPN client networks of %s: %s" % (public_ip, e)) + desired = set((n, t) for n in nets for t in tables) + present = set() + for line in CsHelper.execute("ip -4 route show table all proto %s" % self.CLIENT_ROUTE_PROTO): + words = line.split() + if len(words) < 2 or words[0] != "throw": + continue + table = words[words.index("table") + 1] if "table" in words else "main" + present.add((str(ipaddress.IPv4Network(words[1])), table)) + for net, table in sorted(present - desired): + CsHelper.execute("ip route delete throw %s table %s proto %s" % (net, table, self.CLIENT_ROUTE_PROTO)) + for net, table in sorted(desired - present): + CsHelper.execute("ip route add throw %s table %s proto %s" % (net, table, self.CLIENT_ROUTE_PROTO)) def process(self): self.confips = [] @@ -1318,6 +1387,17 @@ def remoteaccessvpn_iptables(self, publicdev, publicip, obj): self.fw.append(["", "", "-A INPUT -i ppp+ -m tcp -p tcp --dport 53 -j ACCEPT"]) self.fw.append(["nat", "", "-I PREROUTING -i ppp+ -p tcp -m tcp --dport 53 -j DNAT --to-destination %s" % local_ip]) + # A packet that claims a client's address must have come through a tunnel: the client throw + # routes would send replies to a spoofed one into that client's tunnel. No lo exception is + # needed: the router never owns a pool address (local_ip is outside the range and not listed) + try: + client_nets = self.client_networks(obj) + except Exception as e: + logging.error("Failed to read the VPN client networks, no spoofing rules: %s" % e) + client_nets = [] + for net in client_nets: + self.fw.append(["mangle", "front", "-A PREROUTING -s %s ! -i ppp+ -j DROP" % net]) + if self.config.is_vpc(): return @@ -1670,6 +1750,12 @@ def main(argv): config.address().compare() config.address().process() + # Every pass, not only iptables passes: an ip_associations pass can rebuild a per-NIC table + try: + CsRemoteAccessVpn("remoteaccessvpn", config).client_routes() + except Exception as e: + logging.error("Failed to reconcile VPN client routes: %s" % e) + databag_map = OrderedDict([("guest_network", {"process_iptables": True, "executor": [CsVpcGuestNetwork("guestnetwork", config)]}), ("bgp_peers", {"process_iptables": False, "executor": [CsBgpPeers("bgppeers", config)]}), ("ip_aliases", {"process_iptables": True, "executor": []}), diff --git a/systemvm/test/TestCsRemoteAccessVpnClientRoutes.py b/systemvm/test/TestCsRemoteAccessVpnClientRoutes.py new file mode 100644 index 000000000000..fcdd3c932e5c --- /dev/null +++ b/systemvm/test/TestCsRemoteAccessVpnClientRoutes.py @@ -0,0 +1,204 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import ipaddress +import re +import unittest +from unittest import mock + +from configure import CsRemoteAccessVpn + +PROTO = CsRemoteAccessVpn.CLIENT_ROUTE_PROTO +# CloudStack's default client range: local_ip 10.1.2.1, pool 10.1.2.2-10.1.2.8 +CLIENT_NETS = ["10.1.2.1/32", "10.1.2.2/31", "10.1.2.4/30", "10.1.2.8/32"] +POOL_NETS = ["10.1.2.2/31", "10.1.2.4/30", "10.1.2.8/32"] + + +class FakeKernel: + """Policy rules and routes (type, dst, table, proto), with output shaped like iproute2's.""" + + def __init__(self): + self.rules = ["0:\tfrom all lookup local", "220:\tfrom all lookup 220", + "32759:\tfrom 198.51.100.128/25 lookup Table_eth2", "32760:\tfrom all fwmark 0x67 lookup Table_eth3", + "32761:\tfrom 203.0.113.0/28 lookup Table_eth1", "32762:\tfrom all fwmark 0x66 lookup Table_eth2", + "32763:\tfrom all fwmark 0x65 lookup Table_eth1", "32766:\tfrom all lookup main"] + self.routes = {("throw", "198.51.100.128/25", "Table_eth2", "static"), + ("throw", "192.0.2.0/24", "Table_eth2", "static"), + ("throw", "192.0.2.0/24", "Table_eth1", "static"), + # not ours, must never be touched + ("throw", "203.0.113.128/25", "Table_eth9", "static"), + ("10.1.2.2 dev ppp0", None, "main", "kernel")} + self.calls = [] + + def execute(self, cmd): + self.calls.append(cmd) + if cmd == "ip rule show": + return list(self.rules) + if cmd == "ip -4 route show table all proto %s" % PROTO: + out = [] + for typ, dst, table, proto in sorted(self.routes, key=str): + if proto == PROTO: + dst = dst[:-3] if dst.endswith("/32") else dst + out.append("%s %s%s" % (typ, dst, "" if table == "main" else " table " + table)) + return out + m = re.fullmatch(r"ip route (add|delete) throw (\S+) table (\S+) proto (\d+)", cmd) + if not m: + raise AssertionError("unexpected command: " + cmd) + route = ("throw", str(ipaddress.ip_network(m.group(2))), m.group(3), m.group(4)) + if m.group(1) == "add": + if any(r[:3] == route[:3] for r in self.routes): + raise AssertionError("route exists: " + cmd) + self.routes.add(route) + else: + self.routes.remove(route) + return [] + + def owned(self): + return {(dst, table) for typ, dst, table, proto in self.routes if proto == PROTO} + + def foreign(self): + return {r for r in self.routes if r[3] != PROTO} + + def changes(self): + return [c for c in self.calls if c.startswith("ip route ")] + + +class FakeConfig: + + def __init__(self, networks): + self.networks = networks + + def is_vpc(self): + return True + + def address(self): + interfaces = [mock.Mock(**{"get_network.return_value": n}) for n in self.networks] + return mock.Mock(**{"get_interfaces.return_value": interfaces}) + + +class TestCsRemoteAccessVpnClientRoutes(unittest.TestCase): + + def setUp(self): + self.kernel = FakeKernel() + self.static_routes = {"id": "staticroutes"} + self.peers = {"id": "site2sitevpn"} + self.vpn_entry = {"create": True, "local_ip": "10.1.2.1", "ip_range": "10.1.2.2-10.1.2.8", + "local_cidr": "192.0.2.0/24", "public_interface": "eth1"} + self.dbag = {"id": "remoteaccessvpn", "203.0.113.10": self.vpn_entry} + self.config = FakeConfig(["203.0.113.0/28", "198.51.100.128/25", "192.0.2.0/24"]) + for name, value in (("CsHelper.execute", self.kernel.execute), + ("CsStaticRoutes", lambda *a: mock.Mock(**{"get_bag.return_value": self.static_routes})), + ("CsSite2SiteVpn", lambda *a: mock.Mock(**{"get_bag.return_value": self.peers}))): + patcher = mock.patch("configure." + name, value) + patcher.start() + self.addCleanup(patcher.stop) + self.foreign = self.kernel.foreign() + + def vpn(self): + vpn = CsRemoteAccessVpn.__new__(CsRemoteAccessVpn) + vpn.dbag = self.dbag + vpn.config = self.config + vpn.fw = [] + return vpn + + def run_pass(self): + self.kernel.calls = [] + self.vpn().client_routes() + self.assertEqual(self.kernel.foreign(), self.foreign) + return self.kernel.changes() + + def expected(self, nets=CLIENT_NETS, tables=("Table_eth1", "Table_eth2", "Table_eth3")): + return {(n, t) for n in nets for t in tables} + + def test_throws_in_every_nic_table(self): + self.assertEqual(len(self.run_pass()), 12) + self.assertEqual(self.kernel.owned(), self.expected()) + self.assertEqual(self.run_pass(), []) + + def test_rebuilt_table_gets_its_throws_back(self): + self.run_pass() + self.kernel.routes = {r for r in self.kernel.routes if r[2] != "Table_eth2"} + self.foreign = self.kernel.foreign() + self.assertEqual(len(self.run_pass()), 4) + self.assertEqual(self.kernel.owned(), self.expected()) + + def test_new_nic_table(self): + self.run_pass() + self.kernel.rules.insert(2, "219:\tfrom 198.51.100.0/26 lookup Table_eth4") + self.assertEqual(len(self.run_pass()), 4) + self.assertEqual(self.kernel.owned(), self.expected(tables=("Table_eth1", "Table_eth2", "Table_eth3", "Table_eth4"))) + + def test_vpn_removed(self): + self.run_pass() + del self.dbag["203.0.113.10"] + self.assertEqual(len(self.run_pass()), 12) + self.assertEqual(self.kernel.owned(), set()) + + def test_vpn_disabled_entry_left_in_bag(self): + self.run_pass() + self.vpn_entry["create"] = False + self.run_pass() + self.assertEqual(self.kernel.owned(), set()) + + def test_range_changed(self): + self.run_pass() + self.vpn_entry["ip_range"] = "10.1.2.2-10.1.2.3" + self.run_pass() + self.assertEqual(self.kernel.owned(), self.expected(nets=["10.1.2.1/32", "10.1.2.2/31"])) + + def test_no_rules_read_changes_nothing(self): + self.run_pass() + self.kernel.rules = [] + self.assertEqual(self.run_pass(), []) + self.assertEqual(self.kernel.owned(), self.expected()) + + def test_unreadable_entry_adds_nothing(self): + self.vpn_entry["ip_range"] = "not-a-range" + self.run_pass() + self.assertEqual(self.kernel.owned(), set()) + + def test_overlapping_networks_are_skipped(self): + # a subnet on the router, a site-to-site peer network and a static route inside the pool + self.config.networks.append("10.1.2.8/32") + self.peers["198.51.100.1"] = {"create": True, "peer_guest_cidr_list": "192.0.2.128/25, 10.1.2.2/32"} + self.static_routes["10.1.2.4/31"] = {"network": "10.1.2.4/31", "revoke": False} + self.run_pass() + self.assertEqual(self.kernel.owned(), self.expected(nets=["10.1.2.1/32"])) + # a removed peer and a revoked route don't count + self.config.networks.pop() + self.peers["198.51.100.1"]["create"] = False + self.static_routes["10.1.2.4/31"]["revoke"] = True + self.run_pass() + self.assertEqual(self.kernel.owned(), self.expected()) + + def test_static_route_containing_the_pool_does_not_count(self): + self.vpn_entry["local_ip"] = "203.0.113.65" + self.vpn_entry["ip_range"] = "203.0.113.66-203.0.113.72" + self.static_routes["203.0.113.64/26"] = {"network": "203.0.113.64/26", "revoke": False} + self.run_pass() + nets = ["203.0.113.66/31", "203.0.113.68/30", "203.0.113.72/32", "203.0.113.65/32"] + self.assertEqual(self.kernel.owned(), self.expected(nets=nets)) + + def test_spoofed_client_sources_are_dropped(self): + vpn = self.vpn() + vpn.remoteaccessvpn_iptables("eth1", "203.0.113.10", self.vpn_entry) + drops = [fw for fw in vpn.fw if fw[0] == "mangle" and fw[2].endswith("! -i ppp+ -j DROP")] + self.assertEqual(drops, [["mangle", "front", "-A PREROUTING -s %s ! -i ppp+ -j DROP" % n] for n in POOL_NETS]) + + +if __name__ == '__main__': + unittest.main()