From 3359f0542334c1d7d5d34891a3bcc1512b84f4ae Mon Sep 17 00:00:00 2001 From: Nicolas Fort Date: Tue, 29 Oct 2024 19:05:52 +0000 Subject: [PATCH] T6841: firewall: improve config parsing for ZBF when using VRFs and interfaces attached to VRFs --- data/templates/firewall/nftables-zone.j2 | 57 +++++++++-- interface-definitions/firewall.xml.in | 45 ++++++--- .../include/version/firewall-version.xml.i | 2 +- python/vyos/utils/network.py | 4 +- smoketest/scripts/cli/test_firewall.py | 94 ++++++++++++++++++- src/conf_mode/firewall.py | 28 +++++- src/migration-scripts/firewall/16-to-17 | 0 src/migration-scripts/firewall/17-to-18 | 37 ++++++++ 8 files changed, 236 insertions(+), 31 deletions(-) mode change 100755 => 100644 interface-definitions/firewall.xml.in mode change 100755 => 100644 src/migration-scripts/firewall/16-to-17 create mode 100644 src/migration-scripts/firewall/17-to-18 diff --git a/data/templates/firewall/nftables-zone.j2 b/data/templates/firewall/nftables-zone.j2 index e787250794..1f1d8cf24b 100644 --- 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 old mode 100755 new mode 100644 index 07c88f799e..5bb269ae6d --- a/interface-definitions/firewall.xml.in +++ b/interface-definitions/firewall.xml.in @@ -464,24 +464,39 @@ - + Interface associated with zone - - txt - Interface associated with zone - - - vrf - VRF associated with zone - - - - vrf name - - - + + + + Interface associated with zone + + txt + Interface associated with zone + + + + + + + + + + VRF associated with zone + + vrf + VRF associated with zone + + + vrf name + + + + + + Intra-zone filtering diff --git a/interface-definitions/include/version/firewall-version.xml.i b/interface-definitions/include/version/firewall-version.xml.i index a15cf0eece..1a8098297e 100644 --- a/interface-definitions/include/version/firewall-version.xml.i +++ b/interface-definitions/include/version/firewall-version.xml.i @@ -1,3 +1,3 @@ - + diff --git a/python/vyos/utils/network.py b/python/vyos/utils/network.py index 8fce08de09..dc0c0a6d6a 100644 --- 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 index 2d18f04956..034bd1cc02 100755 --- 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 index ffbd915a2d..bdab6e9175 100755 --- 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 old mode 100755 new mode 100644 diff --git a/src/migration-scripts/firewall/17-to-18 b/src/migration-scripts/firewall/17-to-18 new file mode 100644 index 0000000000..ce69928af6 --- /dev/null +++ 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 . + +# From + # set firewall zone interface +# To + # set firewall zone interface name + # or + # set firewall zone interface 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) \ No newline at end of file