From 2fc8738bc9c2fb6364a22d86079e8635cee91949 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Thu, 2 Nov 2023 20:37:57 +0100 Subject: wireguard: T5707: remove previously deconfigured peer Changing the public key of a peer (updating the key material) left the old WireGuard peer in place, as the key removal command used the new key. 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. --- python/vyos/ifconfig/wireguard.py | 5 --- smoketest/scripts/cli/test_interfaces_wireguard.py | 48 ++++++++++++++++++++++ src/conf_mode/interfaces-wireguard.py | 33 +++++++-------- 3 files changed, 65 insertions(+), 21 deletions(-) diff --git a/python/vyos/ifconfig/wireguard.py b/python/vyos/ifconfig/wireguard.py index 4aac103ec..5704f8b64 100644 --- a/python/vyos/ifconfig/wireguard.py +++ b/python/vyos/ifconfig/wireguard.py @@ -167,11 +167,6 @@ class WireGuardIf(Interface): interface setup code and provide a single point of entry when workin on any interface. """ - # remove no longer associated peers first - if 'peer_remove' in config: - for peer, public_key in config['peer_remove'].items(): - self._cmd(f'wg set {self.ifname} peer {public_key} remove') - tmp_file = NamedTemporaryFile('w') tmp_file.write(config['private_key']) tmp_file.flush() diff --git a/smoketest/scripts/cli/test_interfaces_wireguard.py b/smoketest/scripts/cli/test_interfaces_wireguard.py index 48c7cb6a1..4b994a659 100755 --- a/smoketest/scripts/cli/test_interfaces_wireguard.py +++ b/smoketest/scripts/cli/test_interfaces_wireguard.py @@ -20,6 +20,7 @@ import unittest from base_vyostest_shim import VyOSUnitTestSHIM from vyos.configsession import ConfigSessionError from vyos.utils.file import read_file +from vyos.utils.process import cmd base_path = ['interfaces', 'wireguard'] @@ -152,5 +153,52 @@ class WireGuardInterfaceTest(VyOSUnitTestSHIM.TestCase): tmp = read_file(f'/sys/class/net/{interface}/threaded') self.assertTrue(tmp, "1") + def test_05_wireguard_peer_pubkey_change(self): + # T5707 changing WireGuard CLI public key of a peer - it's not removed + + def get_peers(interface) -> list: + tmp = cmd(f'sudo wg show {interface} dump') + first_line = True + peers = [] + for line in tmp.split('\n'): + if not line: + continue # Skip empty lines and last line + items = line.split('\t') + if first_line: + self.assertEqual(privkey, items[0]) + first_line = False + else: + peers.append(items[0]) + return peers + + + interface = 'wg1337' + port = '1337' + privkey = 'iJi4lb2HhkLx2KSAGOjji2alKkYsJjSPkHkrcpxgEVU=' + pubkey_1 = 'srQ8VF6z/LDjKCzpxBzFpmaNUOeuHYzIfc2dcmoc/h4=' + pubkey_2 = '8pbMHiQ7NECVP7F65Mb2W8+4ldGG2oaGvDSpSEsOBn8=' + + 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_commit() + + peers = get_peers(interface) + self.assertIn(pubkey_1, peers) + self.assertNotIn(pubkey_2, peers) + + # Now change the public key of our peer + self.cli_set(base_path + [interface, 'peer', 'VyOS', 'public-key', pubkey_2]) + self.cli_commit() + + # Verify config + peers = get_peers(interface) + self.assertNotIn(pubkey_1, peers) + self.assertIn(pubkey_2, peers) + if __name__ == '__main__': unittest.main(verbosity=2) diff --git a/src/conf_mode/interfaces-wireguard.py b/src/conf_mode/interfaces-wireguard.py index 122d9589a..79e5d3f44 100755 --- a/src/conf_mode/interfaces-wireguard.py +++ b/src/conf_mode/interfaces-wireguard.py @@ -51,17 +51,9 @@ def get_config(config=None): tmp = is_node_changed(conf, base + [ifname, 'port']) if tmp: wireguard['port_changed'] = {} - # Determine which Wireguard peer has been removed. - # Peers can only be removed with their public key! - if 'peer' in wireguard: - peer_remove = {} - for peer, peer_config in wireguard['peer'].items(): - # T4702: If anything on a peer changes we remove the peer first and re-add it - if is_node_changed(conf, base + [ifname, 'peer', peer]): - if 'public_key' in peer_config: - peer_remove = dict_merge({'peer_remove' : {peer : peer_config['public_key']}}, peer_remove) - if peer_remove: - wireguard.update(peer_remove) + # T4702: If anything on a peer changes we remove the peer first and re-add it + if is_node_changed(conf, base + [ifname, 'peer']): + wireguard.update({'rebuild_required': {}}) return wireguard @@ -113,12 +105,21 @@ def verify(wireguard): public_keys.append(peer['public_key']) def apply(wireguard): - tmp = WireGuardIf(wireguard['ifname']) - if 'deleted' in wireguard: - tmp.remove() - return None + 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() + + # Create the new interface if required + if 'deleted' not in wireguard: + wg = WireGuardIf(**wireguard) + wg.update(wireguard) - tmp.update(wireguard) return None if __name__ == '__main__': -- cgit v1.2.3