T6841: firewall: Fixed issues in ZBF when using VRFs

Improve config parsing for ZBF when using VRFs and interfaces attached to VRFs

6 files changed, 86 insertions, 78 deletions

diff --git a/data/templates/firewall/nftables-zone.j2 b/data/templates/firewall/nftables-zone.j2
index 1f1d8cf24..645a38706 100644
--- a/data/templates/firewall/nftables-zone.j2
+++ b/data/templates/firewall/nftables-zone.j2
@@ -8,13 +8,12 @@
 {% endif %}
 {% for zone_name, zone_conf in zone.items() %}
 {%     if 'local_zone' not in zone_conf %}
-{%         if 'name' in zone_conf.interface %}
-        oifname { {{ zone_conf.interface.name | join(',') }} } counter jump VZONE_{{ zone_name }}
+{%         if 'interface' in zone_conf.member %}
+        oifname { {{ zone_conf.member.interface | join(',') }} } counter jump VZONE_{{ zone_name }}
 {%         endif %}
-{%         if 'vrf' in zone_conf.interface %}
-{%             for vrf_name in zone_conf.interface.vrf %}
+{%         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.interface.vrf | join(',') }} } counter jump VZONE_{{ zone_name }}
 {%             endfor %}
 {%         endif %}
 {%     endif %}
@@ -49,13 +48,13 @@
 {%         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 'name' in zone[from_zone].interface %}
-        iifname { {{ zone[from_zone].interface.name | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].interface.name | join(",") }} } counter return
+{%                 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
 {%                 endif %}
-{%                 if 'vrf' in zone[from_zone].interface %}
-        iifname { {{ zone[from_zone].interface.vrf | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].interface.vrf | join(",") }} } counter return
+{%                 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
 {%                 endif %}
 {%             endfor %}
 {%         endif %}
@@ -65,12 +64,12 @@
         oifname lo counter return
 {%         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 'name' in zone[from_zone].interface %}
-        oifname { {{ zone[from_zone].interface.name | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        oifname { {{ zone[from_zone].interface.name | join(",") }} } counter return
+{%                 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
 {%                 endif %}
-{%                 if 'vrf' in zone[from_zone].interface %}
-{%                     for vrf_name in zone[from_zone].interface.vrf %}
+{%                 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
 {%                     endfor %}
@@ -81,30 +80,30 @@
     }
 {%     else %}
     chain VZONE_{{ zone_name }} {
-{%         if 'name' in zone_conf.interface %}
-        iifname { {{ zone_conf.interface.name | join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
+{%         if 'interface' in zone_conf.member %}
+        iifname { {{ zone_conf.member.interface | join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
 {%         endif %}
-{%         if 'vrf' in zone_conf.interface %}
-        iifname { {{ zone_conf.interface.vrf | join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
+{%         if 'vrf' in zone_conf.member %}
+        iifname { {{ zone_conf.member.vrf | join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
 {%         endif %}
 {%         if zone_conf.intra_zone_filtering is vyos_defined %}
-{%             if 'name' in zone_conf.interface %}
-        iifname { {{ zone_conf.interface.name | join(",") }} } counter return
+{%             if 'interface' in zone_conf.member %}
+        iifname { {{ zone_conf.member.interface | join(",") }} } counter return
 {%             endif %}
-{%             if 'vrf' in zone_conf.interface %}
-        iifname { {{ zone_conf.interface.vrf | join(",") }} } counter return
+{%             if 'vrf' in zone_conf.member %}
+        iifname { {{ zone_conf.member.vrf | 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 'name' in zone[from_zone].interface %}
-        iifname { {{ zone[from_zone].interface.name | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].interface.name | join(",") }} } counter return
+{%                     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
 {%                     endif %}
-{%                     if 'vrf' in zone[from_zone].interface %}
-        iifname { {{ zone[from_zone].interface.vrf | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].interface.vrf | join(",") }} } counter return
+{%                     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
 {%                     endif %}
 {%                 endif %}
 {%             endfor %}
diff --git a/interface-definitions/firewall.xml.in b/interface-definitions/firewall.xml.in
index 5bb269ae6..018e0dbdf 100644
--- a/interface-definitions/firewall.xml.in
+++ b/interface-definitions/firewall.xml.in
@@ -464,12 +464,12 @@
               </node>
             </children>
           </tagNode>
-          <node name="interface">
+          <node name="member">
             <properties>
               <help>Interface associated with zone</help>
             </properties>
             <children>
-              <leafNode name="name">
+              <leafNode name="interface">
                 <properties>
                   <help>Interface associated with zone</help>
                   <valueHelp>
diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py
index b71c062cc..10301831e 100755
--- a/smoketest/scripts/cli/test_firewall.py
+++ b/smoketest/scripts/cli/test_firewall.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2021-2023 VyOS maintainers and contributors
+# Copyright (C) 2021-2024 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -906,7 +906,7 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
     def test_zone_basic(self):
         self.cli_set(['firewall', 'ipv4', 'name', 'smoketest', 'default-action', 'drop'])
         self.cli_set(['firewall', 'ipv6', 'name', 'smoketestv6', 'default-action', 'drop'])
-        self.cli_set(['firewall', 'zone', 'smoketest-eth0', 'interface', 'name', 'eth0'])
+        self.cli_set(['firewall', 'zone', 'smoketest-eth0', 'member', 'interface', 'eth0'])
         self.cli_set(['firewall', 'zone', 'smoketest-eth0', 'from', 'smoketest-local', 'firewall', 'name', 'smoketest'])
         self.cli_set(['firewall', 'zone', 'smoketest-eth0', 'intra-zone-filtering', 'firewall', 'ipv6-name', 'smoketestv6'])
         self.cli_set(['firewall', 'zone', 'smoketest-local', 'local-zone'])
@@ -971,12 +971,12 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
         self.cli_set(['firewall', 'zone', 'LOCAL', 'from', 'ZONE1', 'firewall', 'name', 'ZONE1-to-LOCAL'])
         self.cli_set(['firewall', 'zone', 'LOCAL', 'local-zone'])
         self.cli_set(['firewall', 'zone', 'ZONE1', 'from', 'ZONE2', 'firewall', 'name', 'ZONE2_to_ZONE1'])
-        self.cli_set(['firewall', 'zone', 'ZONE1', 'interface', 'name', 'eth1'])
-        self.cli_set(['firewall', 'zone', 'ZONE1', 'interface', 'name', 'eth2'])
-        self.cli_set(['firewall', 'zone', 'ZONE1', 'interface', 'vrf', 'VRF-1'])
+        self.cli_set(['firewall', 'zone', 'ZONE1', 'member', 'interface', 'eth1'])
+        self.cli_set(['firewall', 'zone', 'ZONE1', 'member', 'interface', 'eth2'])
+        self.cli_set(['firewall', 'zone', 'ZONE1', 'member', 'vrf', 'VRF-1'])
         self.cli_set(['firewall', 'zone', 'ZONE2', 'from', 'LOCAL', 'firewall', 'ipv6-name', 'LOCAL_to_ZONE2_v6'])
-        self.cli_set(['firewall', 'zone', 'ZONE2', 'interface', 'name', 'vtun66'])
-        self.cli_set(['firewall', 'zone', 'ZONE2', 'interface', 'vrf', 'VRF-2'])
+        self.cli_set(['firewall', 'zone', 'ZONE2', 'member', 'interface', 'vtun66'])
+        self.cli_set(['firewall', 'zone', 'ZONE2', 'member', 'vrf', 'VRF-2'])
 
         self.cli_set(['vrf', 'name', 'VRF-1', 'table', '101'])
         self.cli_set(['vrf', 'name', 'VRF-2', 'table', '102'])
diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index c09f934aa..768bb127d 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -18,7 +18,6 @@ import os
 import re
 
 from sys import exit
-
 from vyos.base import Warning
 from vyos.config import Config
 from vyos.configdict import is_node_changed
@@ -135,6 +134,27 @@ def get_config(config=None):
 
     fqdn_config_parse(firewall, 'firewall')
 
+    if not os.path.exists(nftables_conf):
+        firewall['first_install'] = True
+
+    if 'zone' in firewall:
+        for local_zone, local_zone_conf in firewall['zone'].items():
+            if 'local_zone' not in local_zone_conf:
+                # Get physical interfaces assigned to the zone if vrf is used:
+                if 'vrf' in local_zone_conf['member']:
+                    local_zone_conf['vrf_interfaces'] = {}
+                    for vrf_name in local_zone_conf['member']['vrf']:
+                        local_zone_conf['vrf_interfaces'][vrf_name] = ','.join(get_vrf_members(vrf_name))
+                continue
+
+            local_zone_conf['from_local'] = {}
+
+            for zone, zone_conf in firewall['zone'].items():
+                if zone == local_zone or 'from' not in zone_conf:
+                    continue
+                if local_zone in zone_conf['from']:
+                    local_zone_conf['from_local'][zone] = zone_conf['from'][local_zone]
+
     set_dependents('conntrack', conf)
 
     return firewall
@@ -447,22 +467,21 @@ def verify(firewall):
 
     if 'zone' in firewall:
         for zone, zone_conf in firewall['zone'].items():
-            if 'local_zone' not in zone_conf and 'interface' not in zone_conf:
+            if 'local_zone' not in zone_conf and 'member' not in zone_conf:
                 raise ConfigError(f'Zone "{zone}" has no interfaces and is not the local zone')
 
             if 'local_zone' in zone_conf:
                 if local_zone:
                     raise ConfigError('There cannot be multiple local zones')
-                if 'interface' in zone_conf:
+                if 'member' in zone_conf:
                     raise ConfigError('Local zone cannot have interfaces assigned')
                 if 'intra_zone_filtering' in zone_conf:
                     raise ConfigError('Local zone cannot use intra-zone-filtering')
                 local_zone = True
 
-            if 'interface' in zone_conf:
-                if 'name'in zone_conf['interface']:
-
-                    for iface in zone_conf['interface']['name']:
+            if 'member' in zone_conf:
+                if 'interface' in zone_conf['member']:
+                    for iface in zone_conf['member']['interface']:
 
                         if iface in zone_interfaces:
                             raise ConfigError(f'Interfaces cannot be assigned to multiple zones')
@@ -470,13 +489,19 @@ def verify(firewall):
                         iface_vrf = get_interface_vrf(iface)
                         if iface_vrf != 'default':
                             Warning(f"Interface {iface} assigned to zone {zone} is in VRF {iface_vrf}. This might not work as expected.")
-                        zone_interfaces += iface
+                        zone_interfaces.append(iface)
 
-                if 'vrf' in zone_conf['interface']:
-                    for vrf in zone_conf['interface']['vrf']:
+                if 'vrf' in zone_conf['member']:
+                    for vrf in zone_conf['member']['vrf']:
                         if vrf in zone_vrf:
                             raise ConfigError(f'VRF cannot be assigned to multiple zones')
-                        zone_vrf += vrf
+                        zone_vrf.append(vrf)
+
+            if 'vrf_interfaces' in zone_conf:
+                for vrf_name, vrf_interfaces in zone_conf['vrf_interfaces'].items():
+                    if not vrf_interfaces:
+                        raise ConfigError(
+                            f'VRF "{vrf_name}" cannot be a member of any zone. It does not contain any interfaces.')
 
             if 'intra_zone_filtering' in zone_conf:
                 intra_zone = zone_conf['intra_zone_filtering']
@@ -512,28 +537,6 @@ def verify(firewall):
     return None
 
 def generate(firewall):
-    if not os.path.exists(nftables_conf):
-        firewall['first_install'] = True
-
-    if 'zone' in firewall:
-        for local_zone, local_zone_conf in firewall['zone'].items():
-            if 'local_zone' not in local_zone_conf:
-                # Get physical interfaces assigned to the zone if vrf is used:
-                if 'vrf' in local_zone_conf['interface']:
-                    local_zone_conf['vrf_interfaces'] = {}
-                    for vrf_name in local_zone_conf['interface']['vrf']:
-                        local_zone_conf['vrf_interfaces'][vrf_name] = ','.join(get_vrf_members(vrf_name))
-                        #local_zone_conf['interface']['vrf'][vrf_name] = ''.join(get_vrf_members(vrf_name))
-                continue
-
-            local_zone_conf['from_local'] = {}
-
-            for zone, zone_conf in firewall['zone'].items():
-                if zone == local_zone or 'from' not in zone_conf:
-                    continue
-                if local_zone in zone_conf['from']:
-                    local_zone_conf['from_local'][zone] = zone_conf['from'][local_zone]
-
     render(nftables_conf, 'firewall/nftables.j2', firewall)
     render(sysctl_file, 'firewall/sysctl-firewall.conf.j2', firewall)
     return None
diff --git a/src/migration-scripts/firewall/17-to-18 b/src/migration-scripts/firewall/17-to-18
index af16ba8ec..891f9f195 100755
--- a/src/migration-scripts/firewall/17-to-18
+++ b/src/migration-scripts/firewall/17-to-18
@@ -16,9 +16,9 @@
 # From
     # set firewall zone <zone> interface <iface>
 # To
-    # set firewall zone <zone> interface name <iface>
+    # set firewall zone <zone> member interface <iface>
     # or
-    # set firewall zone <zone> interface vrf <vrf>
+    # set firewall zone <zone> member vrf <vrf>
 
 
 from vyos.configtree import ConfigTree
@@ -33,4 +33,5 @@ def migrate(config: ConfigTree) -> None:
     for zone in config.list_nodes(base):
         if config.exists(base + [zone, 'interface']):
             for iface in config.return_values(base + [zone, 'interface']):
-                config.set(base + [zone, 'interface', 'name'], value=iface, replace=False)
\ No newline at end of file
+                config.set(base + [zone, 'member', 'interface'], value=iface, replace=False)
+            config.delete(base + [zone, 'interface'])
\ No newline at end of file
diff --git a/src/op_mode/zone.py b/src/op_mode/zone.py
index 49fecdf28..df39549d2 100644
--- a/src/op_mode/zone.py
+++ b/src/op_mode/zone.py
@@ -56,10 +56,15 @@ def _convert_one_zone_data(zone: str, zone_config: dict) -> dict:
                 from_zone_dict['firewall_v6'] = dict_search(
                     'firewall.ipv6_name', from_zone_config)
             list_of_rules.append(from_zone_dict)
+    zone_members =[]
+    interface_members = dict_search('member.interface', zone_config)
+    vrf_members = dict_search('member.vrf', zone_config)
+    zone_members += interface_members if interface_members is not None else []
+    zone_members += vrf_members if vrf_members is not None else []
 
     zone_dict = {
         'name': zone,
-        'interface': dict_search('interface', zone_config),
+        'members': zone_members,
         'type': 'LOCAL' if dict_search('local_zone',
                                        zone_config) is not None else None,
     }
@@ -126,7 +131,7 @@ def output_zone_list(zone_conf: dict) -> list:
     if zone_conf['type'] == 'LOCAL':
         zone_info.append('LOCAL')
     else:
-        zone_info.append("\n".join(zone_conf['interface']))
+        zone_info.append("\n".join(zone_conf['members']))
 
     from_zone = []
     firewall = []
@@ -175,7 +180,7 @@ def get_formatted_output(zone_policy: list) -> str:
     :rtype: str
     """
     headers = ["Zone",
-               "Interfaces",
+               "Members",
                "From Zone",
                "Firewall IPv4",
                "Firewall IPv6"