From a4feb96af9ac45aff41ded1744cf302b5c5a9e7e Mon Sep 17 00:00:00 2001 From: Christian Poessinger Date: Sat, 17 Sep 2022 20:36:22 +0200 Subject: wireguard: T4702: actively revoke peer if it gets disabled When any configured peer is set to `disable` while the Wireguard tunnel is up and running it does not get actively revoked and removed. This poses a security risk as connections keep beeing alive. Whenever any parameter of a peer changes we actively remove the peer and fully recreate it on the fly. --- python/vyos/ifconfig/wireguard.py | 85 ++++++++++++++++++----------------- src/conf_mode/interfaces-wireguard.py | 32 +++++++------ 2 files changed, 58 insertions(+), 59 deletions(-) diff --git a/python/vyos/ifconfig/wireguard.py b/python/vyos/ifconfig/wireguard.py index 28b5e2991..9a92c71b8 100644 --- a/python/vyos/ifconfig/wireguard.py +++ b/python/vyos/ifconfig/wireguard.py @@ -167,12 +167,8 @@ class WireGuardIf(Interface): # remove no longer associated peers first if 'peer_remove' in config: - for tmp in config['peer_remove']: - peer = config['peer_remove'][tmp] - peer['ifname'] = config['ifname'] - - cmd = 'wg set {ifname} peer {public_key} remove' - self._cmd(cmd.format(**peer)) + for peer, public_key in config['peer_remove'].items(): + self._cmd(f'wg set {self.ifname} peer {public_key} remove') config['private_key_file'] = '/tmp/tmp.wireguard.key' with open(config['private_key_file'], 'w') as f: @@ -187,44 +183,49 @@ class WireGuardIf(Interface): base_cmd = base_cmd.format(**config) - for tmp in config['peer']: - peer = config['peer'][tmp] - - # start of with a fresh 'wg' command - cmd = base_cmd + ' peer {public_key}' - - # 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' - psk_file = no_psk_file - if 'preshared_key' in peer: - psk_file = '/tmp/tmp.wireguard.psk' - with open(psk_file, 'w') as f: - f.write(peer['preshared_key']) - cmd += f' preshared-key {psk_file}' - - # Persistent keepalive is optional - if 'persistent_keepalive'in peer: - cmd += ' persistent-keepalive {persistent_keepalive}' - - # Multiple allowed-ip ranges can be defined - ensure we are always - # dealing with a list - if isinstance(peer['allowed_ips'], str): - peer['allowed_ips'] = [peer['allowed_ips']] - cmd += ' allowed-ips ' + ','.join(peer['allowed_ips']) - - # Endpoint configuration is optional - if {'address', 'port'} <= set(peer): - if is_ipv6(peer['address']): - cmd += ' endpoint [{address}]:{port}' - else: - cmd += ' endpoint {address}:{port}' + if 'peer' in config: + 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: + continue + + # start of with a fresh 'wg' command + cmd = base_cmd + ' peer {public_key}' + + # 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' + psk_file = no_psk_file + 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']) + + # Endpoint configuration is optional + if {'address', 'port'} <= set(peer_config): + if is_ipv6(peer_config['address']): + cmd += ' endpoint [{address}]:{port}' + else: + cmd += ' endpoint {address}:{port}' - self._cmd(cmd.format(**peer)) + self._cmd(cmd.format(**peer_config)) - # 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) + # 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) # call base class super().update(config) diff --git a/src/conf_mode/interfaces-wireguard.py b/src/conf_mode/interfaces-wireguard.py index 61bab2feb..8d738f55e 100755 --- a/src/conf_mode/interfaces-wireguard.py +++ b/src/conf_mode/interfaces-wireguard.py @@ -14,16 +14,12 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . -import os - from sys import exit -from copy import deepcopy from vyos.config import Config from vyos.configdict import dict_merge from vyos.configdict import get_interface_dict -from vyos.configdict import node_changed -from vyos.configdict import leaf_node_changed +from vyos.configdict import is_node_changed from vyos.configverify import verify_vrf from vyos.configverify import verify_address from vyos.configverify import verify_bridge_delete @@ -50,17 +46,20 @@ def get_config(config=None): ifname, wireguard = get_interface_dict(conf, base) # Check if a port was changed - wireguard['port_changed'] = leaf_node_changed(conf, base + [ifname, 'port']) + 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! - dict = {} - tmp = node_changed(conf, base + [ifname, 'peer'], key_mangling=('-', '_')) - for peer in (tmp or []): - public_key = leaf_node_changed(conf, base + [ifname, 'peer', peer, 'public_key']) - if public_key: - dict = dict_merge({'peer_remove' : {peer : {'public_key' : public_key[0]}}}, dict) - wireguard.update(dict) + 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) return wireguard @@ -81,12 +80,11 @@ def verify(wireguard): if 'peer' not in wireguard: raise ConfigError('At least one Wireguard peer is required!') - if 'port' in wireguard and wireguard['port_changed']: + if 'port' in wireguard and 'port_changed' in wireguard: listen_port = int(wireguard['port']) if check_port_availability('0.0.0.0', listen_port, 'udp') is not True: - raise ConfigError( - f'The UDP port {listen_port} is busy or unavailable and cannot be used for the interface' - ) + raise ConfigError(f'UDP port {listen_port} is busy or unavailable and ' + 'cannot be used for the interface!') # run checks on individual configured WireGuard peer for tmp in wireguard['peer']: -- cgit v1.2.3