firewall: T4694: Adding rt ipsec exists/missing match to firewall con…

…figs

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.

interface-definitions/include/firewall/common-rule-inet.xml.i

@@ -7,7 +7,6 @@
 #include <include/generic-disable-node.xml.i>
 #include <include/firewall/dscp.xml.i>
 #include <include/firewall/fragment.xml.i>
-#include <include/firewall/match-ipsec.xml.i>
 #include <include/firewall/limit.xml.i>
 #include <include/firewall/log.xml.i>
 #include <include/firewall/log-options.xml.i>

interface-definitions/include/firewall/common-rule-ipv4-raw.xml.i

@@ -9,7 +9,6 @@
 #include <include/firewall/limit.xml.i>
 #include <include/firewall/log.xml.i>
 #include <include/firewall/log-options.xml.i>
-#include <include/firewall/match-ipsec.xml.i>
 #include <include/firewall/protocol.xml.i>
 #include <include/firewall/nft-queue.xml.i>
 #include <include/firewall/recent.xml.i>

interface-definitions/include/firewall/common-rule-ipv6-raw.xml.i

@@ -9,7 +9,6 @@
 #include <include/firewall/limit.xml.i>
 #include <include/firewall/log.xml.i>
 #include <include/firewall/log-options.xml.i>
-#include <include/firewall/match-ipsec.xml.i>
 #include <include/firewall/protocol.xml.i>
 #include <include/firewall/nft-queue.xml.i>
 #include <include/firewall/recent.xml.i>

interface-definitions/include/firewall/ipv4-hook-input.xml.i

@@ -27,7 +27,7 @@
           <children>
             #include <include/firewall/common-rule-ipv4.xml.i>
             #include <include/firewall/inbound-interface.xml.i>
-            #include <include/firewall/match-ipsec.xml.i>
+            #include <include/firewall/match-ipsec-in.xml.i>
           </children>
         </tagNode>
       </children>

interface-definitions/include/firewall/ipv4-hook-output.xml.i

@@ -26,6 +26,7 @@
           </properties>
           <children>
             #include <include/firewall/common-rule-ipv4.xml.i>
+            #include <include/firewall/match-ipsec-out.xml.i>
             #include <include/firewall/outbound-interface.xml.i>
           </children>
         </tagNode>
@@ -53,6 +54,7 @@
           </properties>
           <children>
             #include <include/firewall/common-rule-ipv4-raw.xml.i>
+            #include <include/firewall/match-ipsec-out.xml.i>
             #include <include/firewall/outbound-interface.xml.i>
           </children>
         </tagNode>

interface-definitions/include/firewall/ipv4-hook-prerouting.xml.i

@@ -33,6 +33,7 @@
           </properties>
           <children>
             #include <include/firewall/common-rule-ipv4-raw.xml.i>
+            #include <include/firewall/match-ipsec-in.xml.i>
             #include <include/firewall/inbound-interface.xml.i>
             <leafNode name="jump-target">
               <properties>

interface-definitions/include/firewall/ipv6-hook-input.xml.i

@@ -27,7 +27,7 @@
           <children>
             #include <include/firewall/common-rule-ipv6.xml.i>
             #include <include/firewall/inbound-interface.xml.i>
-            #include <include/firewall/match-ipsec.xml.i>
+            #include <include/firewall/match-ipsec-in.xml.i>
           </children>
         </tagNode>
       </children>

interface-definitions/include/firewall/ipv6-hook-output.xml.i

@@ -26,6 +26,7 @@
           </properties>
           <children>
             #include <include/firewall/common-rule-ipv6.xml.i>
+            #include <include/firewall/match-ipsec-out.xml.i>
             #include <include/firewall/outbound-interface.xml.i>
           </children>
         </tagNode>
@@ -53,6 +54,7 @@
           </properties>
           <children>
             #include <include/firewall/common-rule-ipv6-raw.xml.i>
+            #include <include/firewall/match-ipsec-out.xml.i>
             #include <include/firewall/outbound-interface.xml.i>
           </children>
         </tagNode>

interface-definitions/include/firewall/ipv6-hook-prerouting.xml.i

@@ -33,6 +33,7 @@
           </properties>
           <children>
             #include <include/firewall/common-rule-ipv6-raw.xml.i>
+            #include <include/firewall/match-ipsec-in.xml.i>
             #include <include/firewall/inbound-interface.xml.i>
             <leafNode name="jump-target">
               <properties>

interface-definitions/include/firewall/match-ipsec-in.xml.i

@@ -0,0 +1,21 @@
+<!-- include start from firewall/match-ipsec-in.xml.i -->
+<node name="ipsec">
+  <properties>
+    <help>Inbound IPsec packets</help>
+  </properties>
+  <children>
+    <leafNode name="match-ipsec-in">
+      <properties>
+        <help>Inbound traffic that was IPsec encapsulated</help>
+        <valueless/>
+      </properties>
+    </leafNode>
+    <leafNode name="match-none-in">
+      <properties>
+        <help>Inbound traffic that was not IPsec encapsulated</help>
+        <valueless/>
+      </properties>
+    </leafNode>
+  </children>
+</node>
+<!-- include end -->

interface-definitions/include/firewall/match-ipsec-out.xml.i

@@ -0,0 +1,21 @@
+<!-- include start from firewall/match-ipsec-out.xml.i -->
+<node name="ipsec">
+  <properties>
+    <help>Outbound IPsec packets</help>
+  </properties>
+  <children>
+    <leafNode name="match-ipsec-out">
+      <properties>
+        <help>Outbound traffic to be IPsec encapsulated</help>
+        <valueless/>
+      </properties>
+    </leafNode>
+    <leafNode name="match-none-out">
+      <properties>
+        <help>Outbound traffic that will not be IPsec encapsulated</help>
+        <valueless/>
+      </properties>
+    </leafNode>
+  </children>
+</node>
+<!-- include end -->

interface-definitions/include/firewall/match-ipsec.xml.i

@@ -1,21 +1,33 @@
 <!-- include start from firewall/match-ipsec.xml.i -->
 <node name="ipsec">
   <properties>
-    <help>Inbound IPsec packets</help>
+    <help>IPsec encapsulated packets</help>
   </properties>
   <children>
-    <leafNode name="match-ipsec">
+    <leafNode name="match-ipsec-in">
       <properties>
-        <help>Inbound IPsec packets</help>
+        <help>Inbound traffic that was IPsec encapsulated</help>
         <valueless/>
       </properties>
     </leafNode>
-    <leafNode name="match-none">
+    <leafNode name="match-none-in">
       <properties>
-        <help>Inbound non-IPsec packets</help>
+        <help>Inbound traffic that was not IPsec encapsulated</help>
         <valueless/>
       </properties>
     </leafNode>
+    <leafNode name="match-ipsec-out">
+      <properties>
+        <help>Outbound traffic to be IPsec encapsulated</help>
+        <valueless/>
+      </properties>
+    </leafNode>
+    <leafNode name="match-none-out">
+      <properties>
+        <help>Outbound traffic that will not be IPsec encapsulated</help>
+        <valueless/>
+      </properties>
+    </leafNode>    
   </children>
 </node>
 <!-- include end -->

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='16'></syntaxVersion>
+<syntaxVersion component='firewall' version='17'></syntaxVersion>
 <!-- include end -->

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,

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

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 <http://www.gnu.org/licenses/>.
+
+#
+# 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 <family> <chainspec> rule <rule#> ipsec match-ipsec|match-none
+    #   to: set firewall <family> <chainspec> rule <rule#> ipsec match-ipsec-in|match-none-in
+#
+# The <chainspec> 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)