From 99b63a1eb5a4441aba4bd0c8908007450ceb7d1c 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. (cherry picked from commit a4feb96af9ac45aff41ded1744cf302b5c5a9e7e) --- python/vyos/ifconfig/wireguard.py | 85 ++++++++++++++++++----------------- src/conf_mode/interfaces-wireguard.py | 21 ++++----- 2 files changed, 54 insertions(+), 52 deletions(-) diff --git a/python/vyos/ifconfig/wireguard.py b/python/vyos/ifconfig/wireguard.py index de1b56ce5..200beb123 100644 --- a/python/vyos/ifconfig/wireguard.py +++ b/python/vyos/ifconfig/wireguard.py @@ -171,12 +171,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 {pubkey} 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') # Wireguard base command is identical for every peer base_cmd = 'wg set {ifname} private-key {private_key}' @@ -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 {pubkey}' - - # 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 {pubkey}' + + # 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 34e80cca3..b28aa9568 100755 --- a/src/conf_mode/interfaces-wireguard.py +++ b/src/conf_mode/interfaces-wireguard.py @@ -17,13 +17,11 @@ 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 @@ -52,13 +50,16 @@ def get_config(config=None): # Determine which Wireguard peer has been removed. # Peers can only be removed with their public key! - dict = {} - tmp = node_changed(conf, ['peer'], key_mangling=('-', '_')) - for peer in (tmp or []): - pubkey = leaf_node_changed(conf, ['peer', peer, 'pubkey']) - if pubkey: - dict = dict_merge({'peer_remove' : {peer : {'pubkey' : pubkey[0]}}}, dict) - wireguard.update(dict) + if 'peer' in wireguard: + ifname = wireguard['ifname'] + 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, ['peer', peer]): + if 'pubkey' in peer_config: + peer_remove = dict_merge({'peer_remove' : {peer : peer_config['pubkey']}}, peer_remove) + if peer_remove: + wireguard.update(peer_remove) return wireguard -- cgit v1.2.3