Merge pull request #5286 from c-po/ifconfig-refactoring

ifconfig: T9008: refactor vyos.ifconfig to use cmdl() for safer subprocess execution

5 files changed, 36 insertions, 32 deletions

diff --git a/python/vyos/ifconfig/bridge.py b/python/vyos/ifconfig/bridge.py
index 92596d07c..d54eb2470 100644
--- a/python/vyos/ifconfig/bridge.py
+++ b/python/vyos/ifconfig/bridge.py
@@ -208,8 +208,7 @@ class BridgeIf(Interface):
 
         # VLAN of bridge parent interface is always 1
         # VLAN 1 is the default VLAN for all unlabeled packets
-        cmd = f'bridge vlan add dev {self.ifname} vid 1 pvid untagged self'
-        self._cmd(cmd)
+        self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', '1', 'pvid', 'untagged', 'self'])
 
     def set_multicast_querier(self, enable):
         """
@@ -370,8 +369,7 @@ class BridgeIf(Interface):
         if 'enable_vlan' in config:
             for vlan in config.get('vif_remove', {}):
                 # Remove old VLANs from the bridge
-                cmd = f'bridge vlan del dev {self.ifname} vid {vlan} self'
-                self._cmd(cmd)
+                self._cmdl(['bridge', 'vlan', 'del', 'dev', self.ifname, 'vid', str(vlan), 'self'])
 
                 # After deleting vif, the FDB entry for that VLAN can linger.
                 # We should remove it manually:
@@ -384,13 +382,11 @@ class BridgeIf(Interface):
                         self._cmd(cmd)
 
             for vlan in config.get('vif', {}):
-                cmd = f'bridge vlan add dev {self.ifname} vid {vlan} self'
-                self._cmd(cmd)
+                self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', str(vlan), 'self'])
 
             # VLAN of bridge parent interface is always 1. VLAN 1 is the default
             # VLAN for all unlabeled packets
-            cmd = f'bridge vlan add dev {self.ifname} vid 1 pvid untagged self'
-            self._cmd(cmd)
+            self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', '1', 'pvid', 'untagged', 'self'])
 
         tmp = dict_search('member.interface', config)
         if tmp:
@@ -460,16 +456,13 @@ class BridgeIf(Interface):
 
                     # Remove redundant VLANs from the system
                     for vlan in list_diff(cur_vlan_ids, add_vlan):
-                        cmd = f'bridge vlan del dev {interface} vid {vlan} master'
-                        self._cmd(cmd)
+                        self._cmdl(['bridge', 'vlan', 'del', 'dev', interface, 'vid', str(vlan), 'master'])
 
                     for vlan in allowed_vlan_ids:
-                        cmd = f'bridge vlan add dev {interface} vid {vlan} master'
-                        self._cmd(cmd)
+                        self._cmdl(['bridge', 'vlan', 'add', 'dev', interface, 'vid', str(vlan), 'master'])
 
                     # Setting native VLAN to system
                     if native_vlan_id:
-                        cmd = f'bridge vlan add dev {interface} vid {native_vlan_id} pvid untagged master'
-                        self._cmd(cmd)
+                        self._cmdl(['bridge', 'vlan', 'add', 'dev', interface, 'vid', str(native_vlan_id), 'pvid', 'untagged', 'master'])
 
         super().update(config)
diff --git a/python/vyos/ifconfig/control.py b/python/vyos/ifconfig/control.py
index 03ab8959a..1f817b370 100644
--- a/python/vyos/ifconfig/control.py
+++ b/python/vyos/ifconfig/control.py
@@ -65,6 +65,20 @@ class Control(Section):
                 command = f'ip netns exec {self.config["netns"]} {command}'
         return cmd(command, self.debug, env=env)
 
+    def _cmdl(self, command, env=None):
+        from vyos.utils.process import cmdl
+        if not isinstance(command, list):
+            raise TypeError(f'_cmdl() requires a list, got {type(command).__name__}')
+        netns = self.config.get('netns')
+        vrf   = self.config.get('vrf')
+        # 'ip link set dev <ifname> netns <netns>' moves the interface into the
+        # namespace and must execute from the default netns — skip wrapping.
+        if (netns and command[:4] == ['ip', 'link', 'set', 'dev']
+                and 'netns' in command
+                and command[command.index('netns') + 1] == netns):
+            netns = None
+        return cmdl(command, self.debug, env=env, netns=netns, vrf=vrf)
+
     def _get_command(self, config, name):
         """
         Using the defined names, set data write to sysfs.
diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index 9d6b38fa9..4846ddfa7 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -372,10 +372,9 @@ class Interface(Control):
         if self.exists(f'{self.ifname}', netns=netns):
             return
 
-        cmd = f'ip link add dev {self.ifname}'
-        if type: cmd += f' type {type}'
-        if 'netns' in self.config: cmd = f'ip netns exec {netns} {cmd}'
-        self._cmd(cmd)
+        cmd = ['ip', 'link', 'add', 'dev', self.ifname]
+        if type: cmd += ['type', type]
+        self._cmdl(cmd)
 
     def remove(self, skip_delete=False):
         """
@@ -602,7 +601,7 @@ class Interface(Control):
         >>> from vyos.ifconfig import Interface
         >>> Interface('dum0').set_netns('foo')
         """
-        self._cmd(f'ip link set dev {self.ifname} netns {netns}')
+        self._cmdl(['ip', 'link', 'set', 'dev', self.ifname, 'netns', netns])
         return True
 
     def get_vrf(self):
@@ -1310,11 +1309,10 @@ class Interface(Control):
         elif addr == 'dhcpv6':
             self.set_dhcpv6(True, vrf_changed=vrf_changed)
         elif not is_intf_addr_assigned(self.ifname, addr, netns=netns):
-            netns_cmd  = f'ip netns exec {netns}' if netns else ''
-            tmp = f'{netns_cmd} ip addr add {addr} dev {self.ifname}'
+            tmp = ['ip', 'addr', 'add', addr, 'dev', self.ifname]
             # Add broadcast address for IPv4
-            if is_ipv4(addr): tmp += ' brd +'
-            self._cmd(tmp)
+            if is_ipv4(addr): tmp += ['brd', '+']
+            self._cmdl(tmp)
         else:
             return False
 
@@ -1359,8 +1357,7 @@ class Interface(Control):
         elif addr == 'dhcpv6':
             self.set_dhcpv6(False)
         elif is_intf_addr_assigned(self.ifname, addr, netns=netns):
-            netns_cmd  = f'ip netns exec {netns}' if netns else ''
-            self._cmd(f'{netns_cmd} ip addr del {addr} dev {self.ifname}')
+            self._cmdl(['ip', 'addr', 'del', addr, 'dev', self.ifname])
         else:
             return False
 
diff --git a/python/vyos/ifconfig/vxlan.py b/python/vyos/ifconfig/vxlan.py
index a72cb6201..996a5c8f8 100644
--- a/python/vyos/ifconfig/vxlan.py
+++ b/python/vyos/ifconfig/vxlan.py
@@ -158,8 +158,8 @@ class VXLANIf(Interface):
                 if cur_vni_filter != None:
                     vni = vlan_config['vni']
                     if vni in cur_vni_filter:
-                        self._cmd(f'bridge vni delete dev {self.ifname} vni {vni}')
-                self._cmd(f'bridge vlan del dev {self.ifname} vid {vlan}')
+                        self._cmdl(['bridge', 'vni', 'delete', 'dev', self.ifname, 'vni', str(vni)])
+                self._cmdl(['bridge', 'vlan', 'del', 'dev', self.ifname, 'vid', str(vlan)])
 
         # Determine current OS Kernel vlan_tunnel setting - only adjust when needed
         tmp = get_interface_config(self.ifname)
@@ -182,12 +182,12 @@ class VXLANIf(Interface):
                 vni = vlan_config['vni']
                 # The following commands must be run one after another,
                 # they cannot be combined with linux 6.1 and iproute2 6.1
-                self._cmd(f'bridge vlan add dev {self.ifname} vid {vlan}')
-                self._cmd(f'bridge vlan add dev {self.ifname} vid {vlan} tunnel_info id {vni}')
+                self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', str(vlan)])
+                self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', str(vlan), 'tunnel_info', 'id', str(vni)])
 
                 # If VNI filtering is enabled, install matching VNI filter
                 if dict_search('parameters.vni_filter', self.config) != None:
-                    self._cmd(f'bridge vni add dev {self.ifname} vni {vni}')
+                    self._cmdl(['bridge', 'vni', 'add', 'dev', self.ifname, 'vni', str(vni)])
 
     def update(self, config):
         """ General helper function which works on a dictionary retrieved by
diff --git a/python/vyos/ifconfig/wireguard.py b/python/vyos/ifconfig/wireguard.py
index fb576bac0..94de0bb6b 100644
--- a/python/vyos/ifconfig/wireguard.py
+++ b/python/vyos/ifconfig/wireguard.py
@@ -36,7 +36,7 @@ class WireGuardOperational(Operational):
         output = {}
 
         # Dump wireguard connection data
-        _f = self._cmd('wg show all dump')
+        _f = self._cmdl(['wg', 'show', 'all', 'dump'])
         for line in _f.split('\n'):
             if not line:
                 # Skip empty lines and last line
@@ -166,7 +166,7 @@ class WireGuardOperational(Operational):
         output = {}
 
         # Dump wireguard last handshake
-        tmp = self._cmd(f'wg show {self.ifname} latest-handshakes')
+        tmp = self._cmdl(['wg', 'show', self.ifname, 'latest-handshakes'])
         # Output:
         # PUBLIC-KEY=    1732812147
         for line in tmp.split('\n'):