From e12d52129618735d468c43f5bcc1246476978c92 Mon Sep 17 00:00:00 2001 From: aapostoliuk Date: Tue, 17 Dec 2024 13:39:49 +0200 Subject: [PATCH] T6841: firewall: Fixed issues in ZBF when using VRFs Improve config parsing for ZBF when using VRFs and interfaces attached to VRFs --- data/templates/firewall/nftables-zone.j2 | 59 ++++++++++---------- interface-definitions/firewall.xml.in | 4 +- smoketest/scripts/cli/test_firewall.py | 14 ++--- src/conf_mode/firewall.py | 69 ++++++++++++------------ src/migration-scripts/firewall/17-to-18 | 7 +-- src/op_mode/zone.py | 11 ++-- 6 files changed, 86 insertions(+), 78 deletions(-) diff --git a/data/templates/firewall/nftables-zone.j2 b/data/templates/firewall/nftables-zone.j2 index 1f1d8cf24bd..645a3870672 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 5bb269ae6d0..018e0dbdfbd 100644 --- a/interface-definitions/firewall.xml.in +++ b/interface-definitions/firewall.xml.in @@ -464,12 +464,12 @@ - + Interface associated with zone - + Interface associated with zone diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py index 034bd1cc022..b862ed1dcdd 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 @@ -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', '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']) @@ -970,12 +970,12 @@ def test_zone_with_vrf(self): 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 bdab6e91751..7d19d5c4708 100755 --- a/src/conf_mode/firewall.py +++ b/src/conf_mode/firewall.py @@ -18,7 +18,6 @@ import re from sys import exit - from vyos.base import Warning from vyos.config import Config from vyos.configdict import is_node_changed @@ -136,6 +135,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 @@ -448,22 +468,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') @@ -471,13 +490,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'] @@ -513,28 +538,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 af16ba8ec90..891f9f19507 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 interface # To - # set firewall zone interface name + # set firewall zone member interface # or - # set firewall zone interface vrf + # set firewall zone member 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 49fecdf2845..df39549d2f4 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"