From a779004a2357a3be5ec9757fa5b81a72d0086236 Mon Sep 17 00:00:00 2001 From: Andrew Topp Date: Wed, 3 Jul 2024 00:45:34 +1000 Subject: [PATCH] firewall: T4694: Adding rt ipsec exists/missing match to firewall configs Work in progress, containing several pieces so far: * Change ipsec match-ipsec/none to match-ipsec-in and match-none-in for fw rules * Updated commit now includes a migration script * Add ipsec match-ipsec-out and match-none-out * Change all the points where the match-ipsec.xml.i include was used before, making sure the new includes (match-ipsec-in/out.xml.i) are used appropriately. There were a handful of spots where match-ipsec.xml.i had snuck back in for output hooked chains already (the common-rule-* includes) * Add the -out generators to rendered templates * Heavy modification to firewall config validators: * I needed to check for ipsec-in matches no matter how deeply nested under an output-hook chain(via jump-target) - nftables will not tolerate this * Ended up retrofitting the jump-targets validator from root chains and for named custom chains. It checks for recursive loops and bad match-ipsec-*. However, it could be improved - it will not detect a cycle between 2 otherwise un-referenced named chains. * I still need to do a lot of manual testing for both fw config variations and the underlying issue with IPsec & DMVPN. --- .../include/firewall/common-rule-inet.xml.i | 1 - .../firewall/common-rule-ipv4-raw.xml.i | 1 - .../firewall/common-rule-ipv6-raw.xml.i | 1 - .../include/firewall/ipv4-hook-input.xml.i | 2 +- .../include/firewall/ipv4-hook-output.xml.i | 2 + .../firewall/ipv4-hook-prerouting.xml.i | 1 + .../include/firewall/ipv6-hook-input.xml.i | 2 +- .../include/firewall/ipv6-hook-output.xml.i | 2 + .../firewall/ipv6-hook-prerouting.xml.i | 1 + .../include/firewall/match-ipsec-in.xml.i | 21 ++++++ .../include/firewall/match-ipsec-out.xml.i | 21 ++++++ .../include/firewall/match-ipsec.xml.i | 22 ++++-- .../include/version/firewall-version.xml.i | 2 +- python/vyos/firewall.py | 8 ++- src/conf_mode/firewall.py | 70 ++++++++++++++----- src/migration-scripts/firewall/16-to-17 | 60 ++++++++++++++++ 16 files changed, 188 insertions(+), 29 deletions(-) create mode 100644 interface-definitions/include/firewall/match-ipsec-in.xml.i create mode 100644 interface-definitions/include/firewall/match-ipsec-out.xml.i create mode 100755 src/migration-scripts/firewall/16-to-17 diff --git a/interface-definitions/include/firewall/common-rule-inet.xml.i b/interface-definitions/include/firewall/common-rule-inet.xml.i index 55ffa3a8bb8..0acb08ec9b0 100644 --- a/interface-definitions/include/firewall/common-rule-inet.xml.i +++ b/interface-definitions/include/firewall/common-rule-inet.xml.i @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include diff --git a/interface-definitions/include/firewall/common-rule-ipv4-raw.xml.i b/interface-definitions/include/firewall/common-rule-ipv4-raw.xml.i index 960c960dbb5..e8da1a0e1f5 100644 --- a/interface-definitions/include/firewall/common-rule-ipv4-raw.xml.i +++ b/interface-definitions/include/firewall/common-rule-ipv4-raw.xml.i @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include diff --git a/interface-definitions/include/firewall/common-rule-ipv6-raw.xml.i b/interface-definitions/include/firewall/common-rule-ipv6-raw.xml.i index 958167b8915..3f7c5a0a36b 100644 --- a/interface-definitions/include/firewall/common-rule-ipv6-raw.xml.i +++ b/interface-definitions/include/firewall/common-rule-ipv6-raw.xml.i @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include diff --git a/interface-definitions/include/firewall/ipv4-hook-input.xml.i b/interface-definitions/include/firewall/ipv4-hook-input.xml.i index cefb1ffa786..491d1a9f360 100644 --- a/interface-definitions/include/firewall/ipv4-hook-input.xml.i +++ b/interface-definitions/include/firewall/ipv4-hook-input.xml.i @@ -27,7 +27,7 @@ #include #include - #include + #include diff --git a/interface-definitions/include/firewall/ipv4-hook-output.xml.i b/interface-definitions/include/firewall/ipv4-hook-output.xml.i index ca47ae09b1e..ee9157592e9 100644 --- a/interface-definitions/include/firewall/ipv4-hook-output.xml.i +++ b/interface-definitions/include/firewall/ipv4-hook-output.xml.i @@ -26,6 +26,7 @@ #include + #include #include @@ -53,6 +54,7 @@ #include + #include #include diff --git a/interface-definitions/include/firewall/ipv4-hook-prerouting.xml.i b/interface-definitions/include/firewall/ipv4-hook-prerouting.xml.i index 17ecfe82448..b431303ae69 100644 --- a/interface-definitions/include/firewall/ipv4-hook-prerouting.xml.i +++ b/interface-definitions/include/firewall/ipv4-hook-prerouting.xml.i @@ -33,6 +33,7 @@ #include + #include #include diff --git a/interface-definitions/include/firewall/ipv6-hook-input.xml.i b/interface-definitions/include/firewall/ipv6-hook-input.xml.i index e1f41e64c4f..154b102592c 100644 --- a/interface-definitions/include/firewall/ipv6-hook-input.xml.i +++ b/interface-definitions/include/firewall/ipv6-hook-input.xml.i @@ -27,7 +27,7 @@ #include #include - #include + #include diff --git a/interface-definitions/include/firewall/ipv6-hook-output.xml.i b/interface-definitions/include/firewall/ipv6-hook-output.xml.i index f877cfaafb6..d3c4c1eada7 100644 --- a/interface-definitions/include/firewall/ipv6-hook-output.xml.i +++ b/interface-definitions/include/firewall/ipv6-hook-output.xml.i @@ -26,6 +26,7 @@ #include + #include #include @@ -53,6 +54,7 @@ #include + #include #include diff --git a/interface-definitions/include/firewall/ipv6-hook-prerouting.xml.i b/interface-definitions/include/firewall/ipv6-hook-prerouting.xml.i index 3f384828da3..21f8de6f9e6 100644 --- a/interface-definitions/include/firewall/ipv6-hook-prerouting.xml.i +++ b/interface-definitions/include/firewall/ipv6-hook-prerouting.xml.i @@ -33,6 +33,7 @@ #include + #include #include diff --git a/interface-definitions/include/firewall/match-ipsec-in.xml.i b/interface-definitions/include/firewall/match-ipsec-in.xml.i new file mode 100644 index 00000000000..62ed6466bae --- /dev/null +++ b/interface-definitions/include/firewall/match-ipsec-in.xml.i @@ -0,0 +1,21 @@ + + + + Inbound IPsec packets + + + + + Inbound traffic that was IPsec encapsulated + + + + + + Inbound traffic that was not IPsec encapsulated + + + + + + \ No newline at end of file diff --git a/interface-definitions/include/firewall/match-ipsec-out.xml.i b/interface-definitions/include/firewall/match-ipsec-out.xml.i new file mode 100644 index 00000000000..880fdd4d87a --- /dev/null +++ b/interface-definitions/include/firewall/match-ipsec-out.xml.i @@ -0,0 +1,21 @@ + + + + Outbound IPsec packets + + + + + Outbound traffic to be IPsec encapsulated + + + + + + Outbound traffic that will not be IPsec encapsulated + + + + + + \ No newline at end of file diff --git a/interface-definitions/include/firewall/match-ipsec.xml.i b/interface-definitions/include/firewall/match-ipsec.xml.i index 82c2b324d54..d8d31ef1aa3 100644 --- a/interface-definitions/include/firewall/match-ipsec.xml.i +++ b/interface-definitions/include/firewall/match-ipsec.xml.i @@ -1,21 +1,33 @@ - Inbound IPsec packets + IPsec encapsulated packets - + - Inbound IPsec packets + Inbound traffic that was IPsec encapsulated - + - Inbound non-IPsec packets + Inbound traffic that was not IPsec encapsulated + + + Outbound traffic to be IPsec encapsulated + + + + + + Outbound traffic that will not be IPsec encapsulated + + + \ No newline at end of file diff --git a/interface-definitions/include/version/firewall-version.xml.i b/interface-definitions/include/version/firewall-version.xml.i index 560ed9e5f59..a15cf0eece7 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/firewall.py b/python/vyos/firewall.py index 664df28cc6d..40399f48114 100644 --- a/python/vyos/firewall.py +++ b/python/vyos/firewall.py @@ -366,10 +366,14 @@ def parse_rule(rule_conf, hook, fw_name, rule_id, ip_name): output.append(f'ip{def_suffix} dscp != {{{negated_dscp_str}}}') if 'ipsec' in rule_conf: - if 'match_ipsec' in rule_conf['ipsec']: + if 'match_ipsec_in' in rule_conf['ipsec']: output.append('meta ipsec == 1') - if 'match_none' in rule_conf['ipsec']: + if 'match_none_in' in rule_conf['ipsec']: output.append('meta ipsec == 0') + if 'match_ipsec_out' in rule_conf['ipsec']: + output.append('rt ipsec exists') + if 'match_none_out' in rule_conf['ipsec']: + output.append('rt ipsec missing') if 'fragment' in rule_conf: # Checking for fragmentation after priority -400 is not possible, diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py index ec6b86ef2f2..352d5cbb174 100755 --- a/src/conf_mode/firewall.py +++ b/src/conf_mode/firewall.py @@ -128,7 +128,49 @@ def get_config(config=None): return firewall -def verify_rule(firewall, rule_conf, ipv6): +def verify_jump_target(firewall, root_chain, jump_target, ipv6, recursive=False): + targets_seen = [] + targets_pending = [jump_target] + + while targets_pending: + target = targets_pending.pop() + + if not ipv6: + if target not in dict_search_args(firewall, 'ipv4', 'name'): + raise ConfigError(f'Invalid jump-target. Firewall name {target} does not exist on the system') + target_rules = dict_search_args(firewall, 'ipv4', 'name', target, 'rule') + else: + if target not in dict_search_args(firewall, 'ipv6', 'name'): + raise ConfigError(f'Invalid jump-target. Firewall ipv6 name {target} does not exist on the system') + target_rules = dict_search_args(firewall, 'ipv6', 'name', target, 'rule') + + no_ipsec_in = root_chain in ('output', ) + + if target_rules: + for target_rule_conf in target_rules.values(): + # Output hook types will not tolerate 'meta ipsec exists' matches even in jump targets: + if no_ipsec_in and (dict_search_args(target_rule_conf, 'ipsec', 'match_ipsec_in') is not None \ + or dict_search_args(target_rule_conf, 'ipsec', 'match_none_in') is not None): + if not ipv6: + raise ConfigError(f'Invalid jump-target for {root_chain}. Firewall name {target} rules contain incompatible ipsec inbound matches') + else: + raise ConfigError(f'Invalid jump-target for {root_chain}. Firewall ipv6 name {target} rules contain incompatible ipsec inbound matches') + # Make sure we're not looping back on ourselves somewhere: + if recursive and 'jump_target' in target_rule_conf: + child_target = target_rule_conf['jump_target'] + if child_target in targets_seen: + if not ipv6: + raise ConfigError(f'Loop detected in jump-targets, firewall name {target} refers to previously traversed name {child_target}') + else: + raise ConfigError(f'Loop detected in jump-targets, firewall ipv6 name {target} refers to previously traversed ipv6 name {child_target}') + targets_pending.append(child_target) + if len(targets_seen) == 7: + path_txt = ' -> '.join(targets_seen) + Warning(f'Deep nesting of jump targets has reached 8 levels deep, following the path {path_txt} -> {child_target}!') + + targets_seen.append(target) + +def verify_rule(firewall, chain_name, rule_conf, ipv6): if 'action' not in rule_conf: raise ConfigError('Rule action must be defined') @@ -139,12 +181,10 @@ def verify_rule(firewall, rule_conf, ipv6): if 'jump' not in rule_conf['action']: raise ConfigError('jump-target defined, but action jump needed and it is not defined') target = rule_conf['jump_target'] - if not ipv6: - if target not in dict_search_args(firewall, 'ipv4', 'name'): - raise ConfigError(f'Invalid jump-target. Firewall name {target} does not exist on the system') + if chain_name != 'name': # This is a bit clumsy, but consolidates a chunk of code. + verify_jump_target(firewall, chain_name, target, ipv6, recursive=True) else: - if target not in dict_search_args(firewall, 'ipv6', 'name'): - raise ConfigError(f'Invalid jump-target. Firewall ipv6 name {target} does not exist on the system') + verify_jump_target(firewall, chain_name, target, ipv6, recursive=False) if rule_conf['action'] == 'offload': if 'offload_target' not in rule_conf: @@ -185,8 +225,10 @@ def verify_rule(firewall, rule_conf, ipv6): raise ConfigError('Limit rate integer cannot be less than 1') if 'ipsec' in rule_conf: - if {'match_ipsec', 'match_non_ipsec'} <= set(rule_conf['ipsec']): - raise ConfigError('Cannot specify both "match-ipsec" and "match-non-ipsec"') + if {'match_ipsec_in', 'match_none_in'} <= set(rule_conf['ipsec']): + raise ConfigError('Cannot specify both "match-ipsec" and "match-none"') + if {'match_ipsec_out', 'match_none_out'} <= set(rule_conf['ipsec']): + raise ConfigError('Cannot specify both "match-ipsec" and "match-none"') if 'recent' in rule_conf: if not {'count', 'time'} <= set(rule_conf['recent']): @@ -349,13 +391,11 @@ def verify(firewall): raise ConfigError('default-jump-target defined, but default-action jump needed and it is not defined') if name_conf['default_jump_target'] == name_id: raise ConfigError(f'Loop detected on default-jump-target.') - ## Now need to check that default-jump-target exists (other firewall chain/name) - if target not in dict_search_args(firewall['ipv4'], 'name'): - raise ConfigError(f'Invalid jump-target. Firewall name {target} does not exist on the system') + verify_jump_target(firewall, name, target, False, recursive=True) if 'rule' in name_conf: for rule_id, rule_conf in name_conf['rule'].items(): - verify_rule(firewall, rule_conf, False) + verify_rule(firewall, name, rule_conf, False) if 'ipv6' in firewall: for name in ['name','forward','input','output', 'prerouting']: @@ -369,13 +409,11 @@ def verify(firewall): raise ConfigError('default-jump-target defined, but default-action jump needed and it is not defined') if name_conf['default_jump_target'] == name_id: raise ConfigError(f'Loop detected on default-jump-target.') - ## Now need to check that default-jump-target exists (other firewall chain/name) - if target not in dict_search_args(firewall['ipv6'], 'name'): - raise ConfigError(f'Invalid jump-target. Firewall name {target} does not exist on the system') + verify_jump_target(firewall, name, target, True, recursive=True) if 'rule' in name_conf: for rule_id, rule_conf in name_conf['rule'].items(): - verify_rule(firewall, rule_conf, True) + verify_rule(firewall, name, rule_conf, True) #### ZONESSSS local_zone = False diff --git a/src/migration-scripts/firewall/16-to-17 b/src/migration-scripts/firewall/16-to-17 new file mode 100755 index 00000000000..9ad7a30f822 --- /dev/null +++ b/src/migration-scripts/firewall/16-to-17 @@ -0,0 +1,60 @@ +# Copyright (C) 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 +# published by the Free Software Foundation. +# +# This program 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 General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# +# T4694: Adding rt ipsec exists/missing match to firewall configs. +# This involves a syntax change for IPsec matches, reflecting that different +# nftables expressions are required depending on whether we're matching a +# decrypted packet or a packet that will be encrypted - it's directional. +# The old rules only matched decrypted packets, those matches are now *-in: + # from: set firewall rule ipsec match-ipsec|match-none + # to: set firewall rule ipsec match-ipsec-in|match-none-in +# +# The positions this match allowed were: +# name (any custom chains), forward filter, input filter, prerouting raw. +# There are positions where it was possible to set, but it would never commit +# (nftables rejects 'meta ipsec' in output hooks), they are not considered here. +# + +import sys + +from vyos.configtree import ConfigTree + +firewall_base = ['firewall'] + +def migrate_chain(config: ConfigTree, path: list[str]) -> None: + for rule_num in config.list_nodes(path + ['rule']): + tmp_path = path + ['rule', rule_num, 'ipsec'] + if config.exists(tmp_path + ['match-ipsec']): + config.delete(tmp_path + ['match-ipsec']) + config.set(tmp_path + ['match-ipsec-in']) + elif config.exists(tmp_path + ['match-none']): + config.delete(tmp_path + ['match-none']) + config.set(tmp_path + ['match-none-in']) + +def migrate(config: ConfigTree) -> None: + if not config.exists(firewall_base): + # Nothing to do + return + + for family in ['ipv4', 'ipv6']: + tmp_path = firewall_base + [family, 'name'] + if config.exists(tmp_path): + for custom_fwname in config.list_nodes(tmp_path): + migrate_chain(config, tmp_path + [custom_fwname]) + + for base_hook in [['forward', 'filter'], ['input', 'filter'], ['prerouting', 'raw']]: + tmp_path = firewall_base + [family] + base_hook + if config.exists(tmp_path): + migrate_chain(config, tmp_path)