T6841: firewall: improve config parsing for ZBF when using VRFs and i…

…nterfaces attached to VRFs
vyos · Oct 29, 2024 · 3359f05 · 3359f05
1 parent ceb64f3
commit 3359f05
Show file tree

Hide file tree

Showing 8 changed files with 236 additions and 31 deletions.
diff --git a/data/templates/firewall/nftables-zone.j2 b/data/templates/firewall/nftables-zone.j2
@@ -8,7 +8,15 @@
 {% endif %}
 {% for zone_name, zone_conf in zone.items() %}
 {%     if 'local_zone' not in zone_conf %}
-        oifname { {{ zone_conf.interface | join(',') }} } counter jump VZONE_{{ zone_name }}
+{%         if 'name' in zone_conf.interface %}
+        oifname { {{ zone_conf.interface.name | join(',') }} } counter jump VZONE_{{ zone_name }}
+{%         endif %}
+{%         if 'vrf' in zone_conf.interface %}
+{%             for vrf_name in zone_conf.interface.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 %}
 {% endfor %}
     }
@@ -40,8 +48,15 @@
         iifname lo counter return
 {%         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 %}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter return
+
+{%                 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
+{%                 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
+{%                 endif %}
 {%             endfor %}
 {%         endif %}
         {{ zone_conf | nft_default_rule('zone_' + zone_name, family) }}
@@ -50,23 +65,47 @@
         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 %}
-        oifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        oifname { {{ zone[from_zone].interface | join(",") }} } counter return
+{%                 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
+{%                 endif %}
+{%                 if 'vrf' in zone[from_zone].interface %}
+{%                     for vrf_name in zone[from_zone].interface.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 %}
+{%                 endif %}
 {%             endfor %}
 {%         endif %}
         {{ zone_conf | nft_default_rule('zone_' + zone_name, family) }}
     }
 {%     else %}
     chain VZONE_{{ zone_name }} {
-        iifname { {{ zone_conf.interface | join(",") }} } counter {{ zone_conf | nft_intra_zone_action(ipv6) }}
+{%         if 'name' in zone_conf.interface %}
+        iifname { {{ zone_conf.interface.name | 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) }}
+{%         endif %}
 {%         if zone_conf.intra_zone_filtering is vyos_defined %}
-        iifname { {{ zone_conf.interface | join(",") }} } counter return
+{%             if 'name' in zone_conf.interface %}
+        iifname { {{ zone_conf.interface.name | join(",") }} } counter return
+{%             endif %}
+{%             if 'vrf' in zone_conf.interface %}
+        iifname { {{ zone_conf.interface.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 %}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME{{ suffix }}_{{ from_conf.firewall[fw_name] }}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter return
+{%                     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
+{%                     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
+{%                     endif %}
 {%                 endif %}
 {%             endfor %}
 {%         endif %}

diff --git a/interface-definitions/firewall.xml.in b/interface-definitions/firewall.xml.in
@@ -464,24 +464,39 @@
               </node>
             </children>
           </tagNode>
-          <leafNode name="interface">
+          <node name="interface">
             <properties>
               <help>Interface associated with zone</help>
-              <valueHelp>
-                <format>txt</format>
-                <description>Interface associated with zone</description>
-              </valueHelp>
-              <valueHelp>
-                <format>vrf</format>
-                <description>VRF associated with zone</description>
-              </valueHelp>
-              <completionHelp>
-                <script>${vyos_completion_dir}/list_interfaces</script>
-                <path>vrf name</path>
-              </completionHelp>
-              <multi/>
             </properties>
-          </leafNode>
+            <children>
+              <leafNode name="name">
+                <properties>
+                  <help>Interface associated with zone</help>
+                  <valueHelp>
+                    <format>txt</format>
+                    <description>Interface associated with zone</description>
+                  </valueHelp>
+                  <completionHelp>
+                    <script>${vyos_completion_dir}/list_interfaces</script>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
+              <leafNode name="vrf">
+                <properties>
+                  <help>VRF associated with zone</help>
+                  <valueHelp>
+                    <format>vrf</format>
+                    <description>VRF associated with zone</description>
+                  </valueHelp>
+                  <completionHelp>
+                    <path>vrf name</path>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
+            </children>
+          </node>
           <node name="intra-zone-filtering">
             <properties>
               <help>Intra-zone filtering</help>

diff --git a/interface-definitions/include/version/firewall-version.xml.i b/interface-definitions/include/version/firewall-version.xml.i
@@ -1,3 +1,3 @@
 <!-- include start from include/version/firewall-version.xml.i -->
-<syntaxVersion component='firewall' version='17'></syntaxVersion>
+<syntaxVersion component='firewall' version='18'></syntaxVersion>
 <!-- include end -->
diff --git a/python/vyos/utils/network.py b/python/vyos/utils/network.py
@@ -69,7 +69,9 @@ def get_vrf_members(vrf: str) -> list:
         answer = json.loads(output)
         for data in answer:
             if 'ifname' in data:
-                interfaces.append(data.get('ifname'))
+                # Skip PIM interfaces which appears in VRF
+                if 'pim' not in data.get('ifname'):
+                    interfaces.append(data.get('ifname'))
     except:
         pass
     return interfaces

diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py
@@ -905,7 +905,7 @@ def test_timeout_sysctl(self):
     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', 'eth0'])
+        self.cli_set(['firewall', 'zone', 'smoketest-eth0', 'interface', 'name', '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'])
@@ -963,6 +963,98 @@ def test_zone_basic(self):
         self.verify_nftables(nftables_search, 'ip vyos_filter')
         self.verify_nftables(nftables_search_v6, 'ip6 vyos_filter')
 
+    def test_zone_with_vrf(self):
+        self.cli_set(['firewall', 'ipv4', 'name', 'ZONE1-to-LOCAL', 'default-action', 'accept'])
+        self.cli_set(['firewall', 'ipv4', 'name', 'ZONE2_to_ZONE1', 'default-action', 'continue'])
+        self.cli_set(['firewall', 'ipv6', 'name', 'LOCAL_to_ZONE2_v6', 'default-action', 'drop'])
+        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', '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(['vrf', 'name', 'VRF-1', 'table', '101'])
+        self.cli_set(['vrf', 'name', 'VRF-2', 'table', '102'])
+        self.cli_set(['interfaces', 'ethernet', 'eth0', 'vrf', 'VRF-1'])
+        self.cli_set(['interfaces', 'vti', 'vti1', 'vrf', 'VRF-2'])
+
+        self.cli_commit()
+
+        nftables_search = [
+            ['chain NAME_ZONE1-to-LOCAL'],
+            ['counter', 'accept', 'comment "NAM-ZONE1-to-LOCAL default-action accept"'],
+            ['chain NAME_ZONE2_to_ZONE1'],
+            ['counter', 'continue', 'comment "NAM-ZONE2_to_ZONE1 default-action continue"'],
+            ['chain VYOS_ZONE_FORWARD'],
+            ['type filter hook forward priority filter + 1'],
+            ['oifname { "eth1", "eth2" }', 'counter packets', 'jump VZONE_ZONE1'],
+            ['oifname "eth0"', 'counter packets', 'jump VZONE_ZONE1'],
+            ['oifname "vtun66"', 'counter packets', 'jump VZONE_ZONE2'],
+            ['oifname "vti1"', 'counter packets', 'jump VZONE_ZONE2'],
+            ['chain VYOS_ZONE_LOCAL'],
+            ['type filter hook input priority filter + 1'],
+            ['counter packets', 'jump VZONE_LOCAL_IN'],
+            ['chain VYOS_ZONE_OUTPUT'],
+            ['type filter hook output priority filter + 1'],
+            ['counter packets', 'jump VZONE_LOCAL_OUT'],
+            ['chain VZONE_LOCAL_IN'],
+            ['iifname { "eth1", "eth2" }', 'counter packets', 'jump NAME_ZONE1-to-LOCAL'],
+            ['iifname "VRF-1"', 'counter packets', 'jump NAME_ZONE1-to-LOCAL'],
+            ['counter packets', 'drop', 'comment "zone_LOCAL default-action drop"'],
+            ['chain VZONE_LOCAL_OUT'],
+            ['counter packets', 'drop', 'comment "zone_LOCAL default-action drop"'],
+            ['chain VZONE_ZONE1'],
+            ['iifname { "eth1", "eth2" }', 'counter packets', 'return'],
+            ['iifname "VRF-1"', 'counter packets', 'return'],
+            ['iifname "vtun66"', 'counter packets', 'jump NAME_ZONE2_to_ZONE1'],
+            ['iifname "vtun66"', 'counter packets', 'return'],
+            ['iifname "VRF-2"', 'counter packets', 'jump NAME_ZONE2_to_ZONE1'],
+            ['iifname "VRF-2"', 'counter packets', 'return'],
+            ['counter packets', 'drop', 'comment "zone_ZONE1 default-action drop"'],
+            ['chain VZONE_ZONE2'],
+            ['iifname "vtun66"', 'counter packets', 'return'],
+            ['iifname "VRF-2"', 'counter packets', 'return'],
+            ['counter packets', 'drop', 'comment "zone_ZONE2 default-action drop"']
+        ]
+
+        nftables_search_v6 = [
+            ['chain NAME6_LOCAL_to_ZONE2_v6'],
+            ['counter', 'drop', 'comment "NAM-LOCAL_to_ZONE2_v6 default-action drop"'],
+            ['chain VYOS_ZONE_FORWARD'],
+            ['type filter hook forward priority filter + 1'],
+            ['oifname { "eth1", "eth2" }', 'counter packets', 'jump VZONE_ZONE1'],
+            ['oifname "eth0"', 'counter packets', 'jump VZONE_ZONE1'],
+            ['oifname "vtun66"', 'counter packets', 'jump VZONE_ZONE2'],
+            ['oifname "vti1"', 'counter packets', 'jump VZONE_ZONE2'],
+            ['chain VYOS_ZONE_LOCAL'],
+            ['type filter hook input priority filter + 1'],
+            ['counter packets', 'jump VZONE_LOCAL_IN'],
+            ['chain VYOS_ZONE_OUTPUT'],
+            ['type filter hook output priority filter + 1'],
+            ['counter packets', 'jump VZONE_LOCAL_OUT'],
+            ['chain VZONE_LOCAL_IN'],
+            ['counter packets', 'drop', 'comment "zone_LOCAL default-action drop"'],
+            ['chain VZONE_LOCAL_OUT'],
+            ['oifname "vtun66"', 'counter packets', 'jump NAME6_LOCAL_to_ZONE2_v6'],
+            ['oifname "vti1"', 'counter packets', 'jump NAME6_LOCAL_to_ZONE2_v6'],
+            ['counter packets', 'drop', 'comment "zone_LOCAL default-action drop"'],
+            ['chain VZONE_ZONE1'],
+            ['iifname { "eth1", "eth2" }', 'counter packets', 'return'],
+            ['iifname "VRF-1"', 'counter packets', 'return'],
+            ['counter packets', 'drop', 'comment "zone_ZONE1 default-action drop"'],
+            ['chain VZONE_ZONE2'],
+            ['iifname "vtun66"', 'counter packets', 'return'],
+            ['iifname "VRF-2"', 'counter packets', 'return'],
+            ['counter packets', 'drop', 'comment "zone_ZONE2 default-action drop"']
+        ]
+
+        self.verify_nftables(nftables_search, 'ip vyos_filter')
+        self.verify_nftables(nftables_search_v6, 'ip6 vyos_filter')
+
     def test_flow_offload(self):
         self.cli_set(['interfaces', 'ethernet', 'eth0', 'vif', '10'])
         self.cli_set(['firewall', 'flowtable', 'smoketest', 'interface', 'eth0.10'])

diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
@@ -34,6 +34,8 @@
 from vyos.utils.process import call
 from vyos.utils.process import cmd
 from vyos.utils.process import rc_cmd
+from vyos.utils.network import get_vrf_members
+from vyos.utils.network import get_interface_vrf
 from vyos import ConfigError
 from vyos import airbag
 from pathlib import Path
@@ -442,6 +444,7 @@ def verify(firewall):
 
     local_zone = False
     zone_interfaces = []
+    zone_vrf = []
 
     if 'zone' in firewall:
         for zone, zone_conf in firewall['zone'].items():
@@ -458,12 +461,23 @@ def verify(firewall):
                 local_zone = True
 
             if 'interface' in zone_conf:
-                found_duplicates = [intf for intf in zone_conf['interface'] if intf in zone_interfaces]
+                if 'name'in zone_conf['interface']:
 
-                if found_duplicates:
-                    raise ConfigError(f'Interfaces cannot be assigned to multiple zones')
+                    for iface in zone_conf['interface']['name']:
 
-                zone_interfaces += zone_conf['interface']
+                        if iface in zone_interfaces:
+                            raise ConfigError(f'Interfaces cannot be assigned to multiple zones')
+
+                        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
+
+                if 'vrf' in zone_conf['interface']:
+                    for vrf in zone_conf['interface']['vrf']:
+                        if vrf in zone_vrf:
+                            raise ConfigError(f'VRF cannot be assigned to multiple zones')
+                        zone_vrf += vrf
 
             if 'intra_zone_filtering' in zone_conf:
                 intra_zone = zone_conf['intra_zone_filtering']
@@ -505,6 +519,12 @@ def generate(firewall):
     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'] = {}

diff --git a/src/migration-scripts/firewall/16-to-17 b/src/migration-scripts/firewall/16-to-17
diff --git a/src/migration-scripts/firewall/17-to-18 b/src/migration-scripts/firewall/17-to-18
@@ -0,0 +1,37 @@
+# Copyright (C) 2024 VyOS maintainers and contributors
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this library.  If not, see <http://www.gnu.org/licenses/>.
+
+# From
+    # set firewall zone <zone> interface <iface>
+# To
+    # set firewall zone <zone> interface name <iface>
+    # or
+    # set firewall zone <zone> interface vrf <vrf>
+
+
+from vyos.configtree import ConfigTree
+from vyos.utils.network import vrf_list
+
+base = ['firewall', 'zone']
+
+def migrate(config: ConfigTree) -> None:
+    if not config.exists(base):
+        # Nothing to do
+        return
+
+    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)