Merge pull request #4581 from talmakion/bugfix/T7544/escape-vrfif-nftables

vrf: T7544: Ensure correct quoting for VRF ifnames in nftables

Author: c-po

data/templates/firewall/nftables-defines.j2

@@ -111,7 +111,7 @@
         flags interval
         auto-merge
 {%             if group_conf.interface is vyos_defined or includes %}
-        elements = { {{ group_conf.interface | nft_nested_group(includes, group.interface_group, 'interface') | join(",") }} }
+        elements = { {{ group_conf.interface | nft_nested_group(includes, group.interface_group, 'interface') | quoted_join(",") }} }
 {%             endif %}
     }
 {%         endfor %}

data/templates/firewall/nftables-zone.j2

@@ -9,11 +9,11 @@
 {% for zone_name, zone_conf in zone.items() %}
 {%     if 'local_zone' not in zone_conf %}
 {%         if 'interface' in zone_conf.member %}
-        oifname { {{ zone_conf.member.interface | join(',') }} } counter jump VZONE_{{ zone_name }}
+        oifname { {{ zone_conf.member.interface | quoted_join(',') }} } counter jump VZONE_{{ zone_name }}
 {%         endif %}
 {%         if 'vrf' in zone_conf.member %}
 {%             for vrf_name in zone_conf.member.vrf %}
-        oifname { {{ zone_conf['vrf_interfaces'][vrf_name] }} } counter jump VZONE_{{ zone_name }}
+        oifname { "{{ zone_conf['vrf_interfaces'][vrf_name] }}" } counter jump VZONE_{{ zone_name }}
 {%             endfor %}
 {%         endif %}
 {%     endif %}
@@ -49,12 +49,12 @@
 {%             for from_zone, from_conf in zone_conf.from.items() if from_conf.firewall[fw_name] is vyos_defined %}
 
 {%                 if 'interface' in zone[from_zone].member %}
-        iifname { {{ zone[from_zone].member.interface | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].member.interface | join(",") }} } counter return
+        iifname { {{ zone[from_zone].member.interface | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
+        iifname { {{ zone[from_zone].member.interface | quoted_join(",") }} } counter return
 {%                 endif %}
 {%                 if 'vrf' in zone[from_zone].member %}
-        iifname { {{ zone[from_zone].member.vrf | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].member.vrf | join(",") }} } counter return
+        iifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
+        iifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter return
 {%                 endif %}
 {%             endfor %}
 {%         endif %}
@@ -65,13 +65,13 @@
 {%         if zone_conf.from_local is vyos_defined %}
 {%             for from_zone, from_conf in zone_conf.from_local.items() if from_conf.firewall[fw_name] is vyos_defined %}
 {%                 if 'interface' in zone[from_zone].member %}
-        oifname { {{ zone[from_zone].member.interface | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        oifname { {{ zone[from_zone].member.interface | join(",") }} } counter return
+        oifname { {{ zone[from_zone].member.interface | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
+        oifname { {{ zone[from_zone].member.interface | quoted_join(",") }} } counter return
 {%                 endif %}
 {%                 if 'vrf' in zone[from_zone].member %}
 {%                     for vrf_name in zone[from_zone].member.vrf %}
-        oifname { {{ zone[from_zone]['vrf_interfaces'][vrf_name] }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        oifname { {{ zone[from_zone]['vrf_interfaces'][vrf_name] }} } counter return
+        oifname { "{{ zone[from_zone]['vrf_interfaces'][vrf_name] }}" } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
+        oifname { "{{ zone[from_zone]['vrf_interfaces'][vrf_name] }}" } counter return
 {%                     endfor %}
 {%                 endif %}
 {%             endfor %}
@@ -81,29 +81,29 @@
 {%     else %}
     chain VZONE_{{ zone_name }} {
 {%         if 'interface' in zone_conf.member %}
-        iifname { {{ zone_conf.member.interface | join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
+        iifname { {{ zone_conf.member.interface | quoted_join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
 {%         endif %}
 {%         if 'vrf' in zone_conf.member %}
-        iifname { {{ zone_conf.member.vrf | join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
+        iifname { {{ zone_conf.member.vrf | quoted_join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
 {%         endif %}
 {%         if zone_conf.intra_zone_filtering is vyos_defined %}
 {%             if 'interface' in zone_conf.member %}
-        iifname { {{ zone_conf.member.interface | join(",") }} } counter return
+        iifname { {{ zone_conf.member.interface | quoted_join(",") }} } counter return
 {%             endif %}
 {%             if 'vrf' in zone_conf.member %}
-        iifname { {{ zone_conf.member.vrf | join(",") }} } counter return
+        iifname { {{ zone_conf.member.vrf | quoted_join(",") }} } counter return
 {%             endif %}
 {%         endif %}
 {%         if zone_conf.from is vyos_defined %}
 {%             for from_zone, from_conf in zone_conf.from.items() if from_conf.firewall[fw_name] is vyos_defined %}
 {%                 if zone[from_zone].local_zone is not defined %}
 {%                     if 'interface' in zone[from_zone].member %}
-        iifname { {{ zone[from_zone].member.interface | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].member.interface | join(",") }} } counter return
+        iifname { {{ zone[from_zone].member.interface | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
+        iifname { {{ zone[from_zone].member.interface | quoted_join(",") }} } counter return
 {%                     endif %}
 {%                     if 'vrf' in zone[from_zone].member %}
-        iifname { {{ zone[from_zone].member.vrf | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].member.vrf | join(",") }} } counter return
+        iifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
+        iifname { {{ zone[from_zone].member.vrf | quoted_join(",") }} } counter return
 {%                     endif %}
 {%                 endif %}
 {%             endfor %}

python/vyos/firewall.py

@@ -361,7 +361,7 @@ def parse_rule(rule_conf, hook, fw_name, rule_id, ip_name):
             if iiface[0] == '!':
                 operator = '!='
                 iiface = iiface[1:]
-            output.append(f'iifname {operator} {{{iiface}}}')
+            output.append(f'iifname {operator} {{"{iiface}"}}')
         elif 'group' in rule_conf['inbound_interface']:
             iiface = rule_conf['inbound_interface']['group']
             if iiface[0] == '!':
@@ -376,7 +376,7 @@ def parse_rule(rule_conf, hook, fw_name, rule_id, ip_name):
             if oiface[0] == '!':
                 operator = '!='
                 oiface = oiface[1:]
-            output.append(f'oifname {operator} {{{oiface}}}')
+            output.append(f'oifname {operator} {{"{oiface}"}}')
         elif 'group' in rule_conf['outbound_interface']:
             oiface = rule_conf['outbound_interface']['group']
             if oiface[0] == '!':

python/vyos/ifconfig/interface.py

@@ -423,11 +423,11 @@ def _nft_check_and_run(self, nft_command):
             self._cmd(f'nft {nft_command}')
 
     def _del_interface_from_ct_iface_map(self):
-        nft_command = f'delete element inet vrf_zones ct_iface_map {{ "{self.ifname}" }}'
+        nft_command = f'delete element inet vrf_zones ct_iface_map {{ \'"{self.ifname}"\' }}'
         self._nft_check_and_run(nft_command)
 
     def _add_interface_to_ct_iface_map(self, vrf_table_id: int):
-        nft_command = f'add element inet vrf_zones ct_iface_map {{ "{self.ifname}" : {vrf_table_id} }}'
+        nft_command = f'add element inet vrf_zones ct_iface_map {{ \'"{self.ifname}"\' : {vrf_table_id} }}'
         self._nft_check_and_run(nft_command)
 
     def get_ifindex(self):

python/vyos/template.py

@@ -582,6 +582,10 @@ def snmp_auth_oid(type):
     }
     return OIDs[type]
 
+@register_filter('quoted_join')
+def quoted_join(input_list, join_str, quote='"'):
+    return str(join_str).join(f'{quote}{elem}{quote}' for elem in input_list)
+
 @register_filter('nft_action')
 def nft_action(vyos_action):
     if vyos_action == 'accept':

src/conf_mode/vrf.py

@@ -240,7 +240,7 @@ def apply(vrf):
                 vrf_iface.set_dhcpv6(False)
 
             # Remove nftables conntrack zone map item
-            nft_del_element = f'delete element inet vrf_zones ct_iface_map {{ "{tmp}" }}'
+            nft_del_element = f'delete element inet vrf_zones ct_iface_map {{ \'"{tmp}"\' }}'
             # Check if deleting is possible first to avoid raising errors
             _, err = popen(f'nft --check {nft_del_element}')
             if not err:
@@ -320,7 +320,7 @@ def apply(vrf):
             state = 'down' if 'disable' in config else 'up'
             vrf_if.set_admin_state(state)
             # Add nftables conntrack zone map item
-            nft_add_element = f'add element inet vrf_zones ct_iface_map {{ "{name}" : {table} }}'
+            nft_add_element = f'add element inet vrf_zones ct_iface_map {{ \'"{name}"\' : {table} }}'
             cmd(f'nft {nft_add_element}')
 
         # Only call into nftables as long as there is nothing setup to avoid wasting