From 2b009d420f28a61353cc373abdea957df0555d18 Mon Sep 17 00:00:00 2001 From: sarthurdev <965089+sarthurdev@users.noreply.github.com> Date: Wed, 23 Apr 2025 16:32:43 +0200 Subject: wireguard: T7387: Optimise wireguard peer handling --- python/vyos/ifconfig/wireguard.py | 160 +++++++++++++-------- python/vyos/utils/network.py | 15 ++ smoketest/scripts/cli/test_interfaces_wireguard.py | 27 +++- src/conf_mode/interfaces_wireguard.py | 16 +-- 4 files changed, 140 insertions(+), 78 deletions(-) diff --git a/python/vyos/ifconfig/wireguard.py b/python/vyos/ifconfig/wireguard.py index f5217aecb..3a28723b3 100644 --- a/python/vyos/ifconfig/wireguard.py +++ b/python/vyos/ifconfig/wireguard.py @@ -22,12 +22,13 @@ from tempfile import NamedTemporaryFile from hurry.filesize import size from hurry.filesize import alternative +from vyos.base import Warning from vyos.configquery import ConfigTreeQuery from vyos.ifconfig import Interface from vyos.ifconfig import Operational from vyos.template import is_ipv6 from vyos.template import is_ipv4 - +from vyos.utils.network import get_wireguard_peers class WireGuardOperational(Operational): def _dump(self): """Dump wireguard data in a python friendly way.""" @@ -251,92 +252,131 @@ class WireGuardIf(Interface): """Get a synthetic MAC address.""" return self.get_mac_synthetic() + def get_peer_public_keys(self, config, disabled=False): + """Get list of configured peer public keys""" + if 'peer' not in config: + return [] + + public_keys = [] + + for _, peer_config in config['peer'].items(): + if disabled == ('disable' in peer_config): + public_keys.append(peer_config['public_key']) + + return public_keys + def update(self, config): """General helper function which works on a dictionary retrived by get_config_dict(). It's main intention is to consolidate the scattered interface setup code and provide a single point of entry when workin on any interface.""" - tmp_file = NamedTemporaryFile('w') - tmp_file.write(config['private_key']) - tmp_file.flush() # Wireguard base command is identical for every peer base_cmd = f'wg set {self.ifname}' + interface_cmd = base_cmd if 'port' in config: interface_cmd += ' listen-port {port}' if 'fwmark' in config: interface_cmd += ' fwmark {fwmark}' - interface_cmd += f' private-key {tmp_file.name}' - interface_cmd = interface_cmd.format(**config) - # T6490: execute command to ensure interface configured - self._cmd(interface_cmd) + with NamedTemporaryFile('w') as tmp_file: + tmp_file.write(config['private_key']) + tmp_file.flush() - # If no PSK is given remove it by using /dev/null - passing keys via - # the shell (usually bash) is considered insecure, thus we use a file - no_psk_file = '/dev/null' + interface_cmd += f' private-key {tmp_file.name}' + interface_cmd = interface_cmd.format(**config) + # T6490: execute command to ensure interface configured + self._cmd(interface_cmd) + + current_peer_public_keys = get_wireguard_peers(self.ifname) + + if 'rebuild_required' in config: + # Remove all existing peers that no longer exist in config + current_public_keys = self.get_peer_public_keys(config) + cmd_remove_peers = [f' peer {public_key} remove' + for public_key in current_peer_public_keys + if public_key not in current_public_keys] + if cmd_remove_peers: + self._cmd(base_cmd + ''.join(cmd_remove_peers)) if 'peer' in config: + # Group removal of disabled peers in one command + current_disabled_peers = self.get_peer_public_keys(config, disabled=True) + cmd_disabled_peers = [f' peer {public_key} remove' + for public_key in current_disabled_peers] + if cmd_disabled_peers: + self._cmd(base_cmd + ''.join(cmd_disabled_peers)) + + peer_cmds = [] + peer_domain_cmds = [] + peer_psk_files = [] + for peer, peer_config in config['peer'].items(): # T4702: No need to configure this peer when it was explicitly # marked as disabled - also active sessions are terminated as # the public key was already removed when entering this method! if 'disable' in peer_config: - # remove peer if disabled, no error report even if peer not exists - cmd = base_cmd + ' peer {public_key} remove' - self._cmd(cmd.format(**peer_config)) continue - psk_file = no_psk_file - # start of with a fresh 'wg' command - peer_cmd = base_cmd + ' peer {public_key}' + peer_cmd = ' peer {public_key}' - try: - cmd = peer_cmd - - if 'preshared_key' in peer_config: - psk_file = '/tmp/tmp.wireguard.psk' - with open(psk_file, 'w') as f: - f.write(peer_config['preshared_key']) - cmd += f' preshared-key {psk_file}' - - # Persistent keepalive is optional - if 'persistent_keepalive' in peer_config: - cmd += ' persistent-keepalive {persistent_keepalive}' - - # Multiple allowed-ip ranges can be defined - ensure we are always - # dealing with a list - if isinstance(peer_config['allowed_ips'], str): - peer_config['allowed_ips'] = [peer_config['allowed_ips']] - cmd += ' allowed-ips ' + ','.join(peer_config['allowed_ips']) - - self._cmd(cmd.format(**peer_config)) - - cmd = peer_cmd - - # Ensure peer is created even if dns not working - if {'address', 'port'} <= set(peer_config): - if is_ipv6(peer_config['address']): - cmd += ' endpoint [{address}]:{port}' - elif is_ipv4(peer_config['address']): - cmd += ' endpoint {address}:{port}' - else: - # don't set endpoint if address uses domain name - continue - elif {'host_name', 'port'} <= set(peer_config): - cmd += ' endpoint {host_name}:{port}' - - self._cmd(cmd.format(**peer_config), env={ + cmd = peer_cmd + + if 'preshared_key' in peer_config: + with NamedTemporaryFile(mode='w', delete=False) as tmp_file: + tmp_file.write(peer_config['preshared_key']) + tmp_file.flush() + cmd += f' preshared-key {tmp_file.name}' + peer_psk_files.append(tmp_file.name) + else: + # If no PSK is given remove it by using /dev/null - passing keys via + # the shell (usually bash) is considered insecure, thus we use a file + cmd += f' preshared-key /dev/null' + + # Persistent keepalive is optional + if 'persistent_keepalive' in peer_config: + cmd += ' persistent-keepalive {persistent_keepalive}' + + # Multiple allowed-ip ranges can be defined - ensure we are always + # dealing with a list + if isinstance(peer_config['allowed_ips'], str): + peer_config['allowed_ips'] = [peer_config['allowed_ips']] + cmd += ' allowed-ips ' + ','.join(peer_config['allowed_ips']) + + peer_cmds.append(cmd.format(**peer_config)) + + cmd = peer_cmd + + # Ensure peer is created even if dns not working + if {'address', 'port'} <= set(peer_config): + if is_ipv6(peer_config['address']): + cmd += ' endpoint [{address}]:{port}' + elif is_ipv4(peer_config['address']): + cmd += ' endpoint {address}:{port}' + else: + # don't set endpoint if address uses domain name + continue + elif {'host_name', 'port'} <= set(peer_config): + cmd += ' endpoint {host_name}:{port}' + else: + continue + + peer_domain_cmds.append(cmd.format(**peer_config)) + + try: + if peer_cmds: + self._cmd(base_cmd + ''.join(peer_cmds)) + + if peer_domain_cmds: + self._cmd(base_cmd + ''.join(peer_domain_cmds), env={ 'WG_ENDPOINT_RESOLUTION_RETRIES': config['max_dns_retry']}) - except: - # todo: logging - pass - finally: - # PSK key file is not required to be stored persistently as its backed by CLI - if psk_file != no_psk_file and os.path.exists(psk_file): - os.remove(psk_file) + except Exception as e: + Warning(f'Failed to apply Wireguard peers on {self.ifname}: {e}') + finally: + for tmp in peer_psk_files: + os.unlink(tmp) # call base class super().update(config) diff --git a/python/vyos/utils/network.py b/python/vyos/utils/network.py index 2f666f0ee..7332bb63d 100644 --- a/python/vyos/utils/network.py +++ b/python/vyos/utils/network.py @@ -396,6 +396,21 @@ def is_wireguard_key_pair(private_key: str, public_key:str) -> bool: else: return False +def get_wireguard_peers(ifname: str) -> list: + """ + Return list of configured Wireguard peers for interface + :param ifname: Interface name + :type ifname: str + :return: list of public keys + :rtype: list + """ + if not interface_exists(ifname): + return [] + + from vyos.utils.process import cmd + peers = cmd(f'wg show {ifname} peers') + return peers.splitlines() + def is_subnet_connected(subnet, primary=False): """ Verify is the given IPv4/IPv6 subnet is connected to any interface on this diff --git a/smoketest/scripts/cli/test_interfaces_wireguard.py b/smoketest/scripts/cli/test_interfaces_wireguard.py index f8cd18cf2..7bc82c187 100755 --- a/smoketest/scripts/cli/test_interfaces_wireguard.py +++ b/smoketest/scripts/cli/test_interfaces_wireguard.py @@ -154,13 +154,15 @@ class WireGuardInterfaceTest(BasicInterfaceTest.TestCase): tmp = read_file(f'/sys/class/net/{intf}/threaded') self.assertTrue(tmp, "1") - def test_wireguard_peer_pubkey_change(self): + def test_wireguard_peer_change(self): # T5707 changing WireGuard CLI public key of a peer - it's not removed + # Also check if allowed-ips update - def get_peers(interface) -> list: + def get_peers(interface) -> list[tuple]: tmp = cmd(f'sudo wg show {interface} dump') first_line = True peers = [] + allowed_ips = [] for line in tmp.split('\n'): if not line: continue # Skip empty lines and last line @@ -170,24 +172,27 @@ class WireGuardInterfaceTest(BasicInterfaceTest.TestCase): first_line = False else: peers.append(items[0]) - return peers + allowed_ips.append(items[3]) + return peers, allowed_ips interface = 'wg1337' port = '1337' privkey = 'iJi4lb2HhkLx2KSAGOjji2alKkYsJjSPkHkrcpxgEVU=' pubkey_1 = 'srQ8VF6z/LDjKCzpxBzFpmaNUOeuHYzIfc2dcmoc/h4=' pubkey_2 = '8pbMHiQ7NECVP7F65Mb2W8+4ldGG2oaGvDSpSEsOBn8=' + allowed_ips_1 = '10.205.212.10/32' + allowed_ips_2 = '10.205.212.11/32' self.cli_set(base_path + [interface, 'address', '172.16.0.1/24']) self.cli_set(base_path + [interface, 'port', port]) self.cli_set(base_path + [interface, 'private-key', privkey]) self.cli_set(base_path + [interface, 'peer', 'VyOS', 'public-key', pubkey_1]) - self.cli_set(base_path + [interface, 'peer', 'VyOS', 'allowed-ips', '10.205.212.10/32']) + self.cli_set(base_path + [interface, 'peer', 'VyOS', 'allowed-ips', allowed_ips_1]) self.cli_commit() - peers = get_peers(interface) + peers, _ = get_peers(interface) self.assertIn(pubkey_1, peers) self.assertNotIn(pubkey_2, peers) @@ -196,10 +201,20 @@ class WireGuardInterfaceTest(BasicInterfaceTest.TestCase): self.cli_commit() # Verify config - peers = get_peers(interface) + peers, _ = get_peers(interface) self.assertNotIn(pubkey_1, peers) self.assertIn(pubkey_2, peers) + # Update allowed-ips + self.cli_delete(base_path + [interface, 'peer', 'VyOS', 'allowed-ips', allowed_ips_1]) + self.cli_set(base_path + [interface, 'peer', 'VyOS', 'allowed-ips', allowed_ips_2]) + self.cli_commit() + + # Verify config + _, allowed_ips = get_peers(interface) + self.assertNotIn(allowed_ips_1, allowed_ips) + self.assertIn(allowed_ips_2, allowed_ips) + def test_wireguard_hostname(self): # T4930: Test dynamic endpoint support interface = 'wg1234' diff --git a/src/conf_mode/interfaces_wireguard.py b/src/conf_mode/interfaces_wireguard.py index 192937dba..289414c83 100755 --- a/src/conf_mode/interfaces_wireguard.py +++ b/src/conf_mode/interfaces_wireguard.py @@ -145,19 +145,11 @@ def generate(wireguard): def apply(wireguard): check_kmod('wireguard') - if 'rebuild_required' in wireguard or 'deleted' in wireguard: - wg = WireGuardIf(**wireguard) - # WireGuard only supports peer removal based on the configured public-key, - # by deleting the entire interface this is the shortcut instead of parsing - # out all peers and removing them one by one. - # - # Peer reconfiguration will always come with a short downtime while the - # WireGuard interface is recreated (see below) - wg.remove() + wg = WireGuardIf(**wireguard) - # Create the new interface if required - if 'deleted' not in wireguard: - wg = WireGuardIf(**wireguard) + if 'deleted' in wireguard: + wg.remove() + else: wg.update(wireguard) domain_resolver_usage = '/run/use-vyos-domain-resolver-interfaces-wireguard-' + wireguard['ifname'] -- cgit v1.2.3 From ab602253d57c1fb4a01a9c84f75bbbc480a66189 Mon Sep 17 00:00:00 2001 From: sarthurdev <965089+sarthurdev@users.noreply.github.com> Date: Wed, 23 Apr 2025 20:05:18 +0200 Subject: resolver: T4930: Fix always True on glob check --- src/conf_mode/firewall.py | 25 ++++++++++++++----------- src/conf_mode/interfaces_wireguard.py | 12 +++++++----- src/conf_mode/nat.py | 6 +++--- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py index 72f2d39f4..14438dae7 100755 --- a/src/conf_mode/firewall.py +++ b/src/conf_mode/firewall.py @@ -17,6 +17,8 @@ import os import re +from glob import glob + from sys import exit from vyos.base import Warning from vyos.config import Config @@ -30,6 +32,7 @@ from vyos.firewall import geoip_update from vyos.template import render from vyos.utils.dict import dict_search_args from vyos.utils.dict import dict_search_recursive +from vyos.utils.file import write_file from vyos.utils.process import call from vyos.utils.process import cmd from vyos.utils.process import rc_cmd @@ -37,7 +40,6 @@ from vyos.utils.network import get_vrf_members from vyos.utils.network import get_interface_vrf from vyos import ConfigError from vyos import airbag -from pathlib import Path from subprocess import run as subp_run airbag.enable() @@ -205,7 +207,7 @@ def verify_rule(firewall, family, hook, priority, rule_id, rule_conf): if 'jump' not in rule_conf['action']: raise ConfigError('jump-target defined, but action jump needed and it is not defined') target = rule_conf['jump_target'] - if hook != 'name': # This is a bit clumsy, but consolidates a chunk of code. + if hook != 'name': # This is a bit clumsy, but consolidates a chunk of code. verify_jump_target(firewall, hook, target, family, recursive=True) else: verify_jump_target(firewall, hook, target, family, recursive=False) @@ -268,12 +270,12 @@ def verify_rule(firewall, family, hook, priority, rule_id, rule_conf): if dict_search_args(rule_conf, 'gre', 'flags', 'checksum') is None: # There is no builtin match in nftables for the GRE key, so we need to do a raw lookup. - # The offset of the key within the packet shifts depending on the C-flag. - # 99% of the time, nobody will have checksums enabled - it's usually a manual config option. - # We can either assume it is unset unless otherwise directed + # The offset of the key within the packet shifts depending on the C-flag. + # 99% of the time, nobody will have checksums enabled - it's usually a manual config option. + # We can either assume it is unset unless otherwise directed # (confusing, requires doco to explain why it doesn't work sometimes) - # or, demand an explicit selection to be made for this specific match rule. - # This check enforces the latter. The user is free to create rules for both cases. + # or, demand an explicit selection to be made for this specific match rule. + # This check enforces the latter. The user is free to create rules for both cases. raise ConfigError('Matching GRE tunnel key requires an explicit checksum flag match. For most cases, use "gre flags checksum unset"') if dict_search_args(rule_conf, 'gre', 'flags', 'key', 'unset') is not None: @@ -286,7 +288,7 @@ def verify_rule(firewall, family, hook, priority, rule_id, rule_conf): if gre_inner_value < 0 or gre_inner_value > 65535: raise ConfigError('inner-proto outside valid ethertype range 0-65535') except ValueError: - pass # Symbolic constant, pre-validated before reaching here. + pass # Symbolic constant, pre-validated before reaching here. tcp_flags = dict_search_args(rule_conf, 'tcp', 'flags') if tcp_flags: @@ -616,10 +618,11 @@ def apply(firewall): domain_action = 'restart' if dict_search_args(firewall, 'group', 'remote_group') or dict_search_args(firewall, 'group', 'domain_group') or firewall['ip_fqdn'].items() or firewall['ip6_fqdn'].items(): text = f'# Automatically generated by firewall.py\nThis file indicates that vyos-domain-resolver service is used by the firewall.\n' - Path(domain_resolver_usage).write_text(text) + write_file(domain_resolver_usage, text) else: - Path(domain_resolver_usage).unlink(missing_ok=True) - if not Path('/run').glob('use-vyos-domain-resolver*'): + if os.path.exists(domain_resolver_usage): + os.unlink(domain_resolver_usage) + if not glob('/run/use-vyos-domain-resolver*'): domain_action = 'stop' call(f'systemctl {domain_action} vyos-domain-resolver.service') diff --git a/src/conf_mode/interfaces_wireguard.py b/src/conf_mode/interfaces_wireguard.py index 289414c83..f2f29da01 100755 --- a/src/conf_mode/interfaces_wireguard.py +++ b/src/conf_mode/interfaces_wireguard.py @@ -14,6 +14,9 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . +import os + +from glob import glob from sys import exit from vyos.config import Config @@ -35,7 +38,6 @@ from vyos.utils.network import is_wireguard_key_pair from vyos.utils.process import call from vyos import ConfigError from vyos import airbag -from pathlib import Path airbag.enable() @@ -160,12 +162,12 @@ def apply(wireguard): from vyos.utils.file import write_file text = f'# Automatically generated by interfaces_wireguard.py\nThis file indicates that vyos-domain-resolver service is used by the interfaces_wireguard.\n' - text += "intefaces:\n" + "".join([f" - {peer}\n" for peer in wireguard['peers_need_resolve']]) - Path(domain_resolver_usage).write_text(text) + text += "interfaces:\n" + "".join([f" - {peer}\n" for peer in wireguard['peers_need_resolve']]) write_file(domain_resolver_usage, text) else: - Path(domain_resolver_usage).unlink(missing_ok=True) - if not Path('/run').glob('use-vyos-domain-resolver*'): + if os.path.exists(domain_resolver_usage): + os.unlink(domain_resolver_usage) + if not glob('/run/use-vyos-domain-resolver*'): domain_action = 'stop' call(f'systemctl {domain_action} vyos-domain-resolver.service') diff --git a/src/conf_mode/nat.py b/src/conf_mode/nat.py index 504b3e82a..6c88e5cfd 100755 --- a/src/conf_mode/nat.py +++ b/src/conf_mode/nat.py @@ -16,8 +16,8 @@ import os +from glob import glob from sys import exit -from pathlib import Path from vyos.base import Warning from vyos.config import Config @@ -265,9 +265,9 @@ def apply(nat): text = f'# Automatically generated by nat.py\nThis file indicates that vyos-domain-resolver service is used by nat.\n' write_file(domain_resolver_usage, text) elif os.path.exists(domain_resolver_usage): - Path(domain_resolver_usage).unlink(missing_ok=True) + os.unlink(domain_resolver_usage) - if not Path('/run').glob('use-vyos-domain-resolver*'): + if not glob('/run/use-vyos-domain-resolver*'): domain_action = 'stop' call(f'systemctl {domain_action} vyos-domain-resolver.service') -- cgit v1.2.3