Merge pull request #1199 from sarthurdev/T4218

firewall: T4218: T4216: Add prefix to user defined chains, support negated groups, fixes

13 files changed, 105 insertions, 34 deletions

diff --git a/data/templates/firewall/nftables.tmpl b/data/templates/firewall/nftables.tmpl
index 33c821e84..468a5a32f 100644
--- a/data/templates/firewall/nftables.tmpl
+++ b/data/templates/firewall/nftables.tmpl
@@ -32,7 +32,7 @@ table ip filter {
 {% endif %}
 {% if name is defined %}
 {%   for name_text, conf in name.items() %}
-    chain {{ name_text }} {
+    chain NAME_{{ name_text }} {
 {%     if conf.rule is defined %}
 {%       for rule_id, rule_conf in conf.rule.items() if rule_conf.disable is not defined %}
         {{ rule_conf | nft_rule(name_text, rule_id) }}
@@ -82,7 +82,7 @@ table ip6 filter {
 {% endif %}
 {% if ipv6_name is defined %}
 {%   for name_text, conf in ipv6_name.items() %}
-    chain {{ name_text }} {
+    chain NAME6_{{ name_text }} {
 {%     if conf.rule is defined %}
 {%       for rule_id, rule_conf in conf.rule.items() if rule_conf.disable is not defined %}
         {{ rule_conf | nft_rule(name_text, rule_id, 'ip6') }}
diff --git a/data/templates/zone_policy/nftables.tmpl b/data/templates/zone_policy/nftables.tmpl
index e59208a0d..093da6bd8 100644
--- a/data/templates/zone_policy/nftables.tmpl
+++ b/data/templates/zone_policy/nftables.tmpl
@@ -13,7 +13,7 @@ table ip filter {
     chain VZONE_{{ zone_name }}_IN {
         iifname lo counter return
 {%       for from_zone, from_conf in zone_conf.from.items() if from_conf.firewall.name is defined %}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump {{ from_conf.firewall.name }}
+        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME_{{ from_conf.firewall.name }}
         iifname { {{ zone[from_zone].interface | join(",") }} } counter return
 {%       endfor %}
         counter {{ zone_conf.default_action if zone_conf.default_action is defined else 'drop' }}
@@ -21,7 +21,7 @@ table ip filter {
     chain VZONE_{{ zone_name }}_OUT {
         oifname lo counter return
 {%         for from_zone, from_conf in zone_conf.from_local.items() if from_conf.firewall.name is defined %}
-        oifname { {{ zone[from_zone].interface | join(",") }} } counter jump {{ from_conf.firewall.name }}
+        oifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME_{{ from_conf.firewall.name }}
         oifname { {{ zone[from_zone].interface | join(",") }} } counter return
 {%         endfor %}
         counter {{ zone_conf.default_action if zone_conf.default_action is defined else 'drop' }}
@@ -34,7 +34,7 @@ table ip filter {
 {%       endif %}
 {%       for from_zone, from_conf in zone_conf.from.items() if from_conf.firewall.name is defined %}
 {%         if zone[from_zone].local_zone is not defined %}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump {{ from_conf.firewall.name }}
+        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME_{{ from_conf.firewall.name }}
         iifname { {{ zone[from_zone].interface | join(",") }} } counter return
 {%         endif %}
 {%       endfor %}
@@ -50,7 +50,7 @@ table ip6 filter {
     chain VZONE6_{{ zone_name }}_IN {
         iifname lo counter return
 {%       for from_zone, from_conf in zone_conf.from.items() if from_conf.firewall.ipv6_name is defined %}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump {{ from_conf.firewall.ipv6_name }}
+        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME6_{{ from_conf.firewall.ipv6_name }}
         iifname { {{ zone[from_zone].interface | join(",") }} } counter return
 {%       endfor %}
         counter {{ zone_conf.default_action if zone_conf.default_action is defined else 'drop' }}
@@ -58,7 +58,7 @@ table ip6 filter {
     chain VZONE6_{{ zone_name }}_OUT {
         oifname lo counter return
 {%         for from_zone, from_conf in zone_conf.from_local.items() if from_conf.firewall.ipv6_name is defined %}
-        oifname { {{ zone[from_zone].interface | join(",") }} } counter jump {{ from_conf.firewall.ipv6_name }}
+        oifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME6_{{ from_conf.firewall.ipv6_name }}
         oifname { {{ zone[from_zone].interface | join(",") }} } counter return
 {%         endfor %}
         counter {{ zone_conf.default_action if zone_conf.default_action is defined else 'drop' }}
@@ -71,7 +71,7 @@ table ip6 filter {
 {%       endif %}
 {%       for from_zone, from_conf in zone_conf.from.items() if from_conf.firewall.ipv6_name is defined %}
 {%         if zone[from_zone].local_zone is not defined %}
-        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump {{ from_conf.firewall.ipv6_name }}
+        iifname { {{ zone[from_zone].interface | join(",") }} } counter jump NAME6_{{ from_conf.firewall.ipv6_name }}
         iifname { {{ zone[from_zone].interface | join(",") }} } counter return
 {%         endif %}
 {%       endfor %}
diff --git a/interface-definitions/firewall.xml.in b/interface-definitions/firewall.xml.in
index f38bcfd9c..f2aca4b3a 100644
--- a/interface-definitions/firewall.xml.in
+++ b/interface-definitions/firewall.xml.in
@@ -74,6 +74,9 @@
           <tagNode name="address-group">
             <properties>
               <help>Firewall address-group</help>
+              <constraint>
+                <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+              </constraint>
             </properties>
             <children>
               <leafNode name="address">
@@ -100,6 +103,9 @@
           <tagNode name="ipv6-address-group">
             <properties>
               <help>Firewall ipv6-address-group</help>
+              <constraint>
+                <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+              </constraint>
             </properties>
             <children>
               <leafNode name="address">
@@ -126,6 +132,9 @@
           <tagNode name="ipv6-network-group">
             <properties>
               <help>Firewall ipv6-network-group</help>
+              <constraint>
+                <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+              </constraint>
             </properties>
             <children>
               #include <include/generic-description.xml.i>
@@ -147,6 +156,9 @@
           <tagNode name="mac-group">
             <properties>
               <help>Firewall mac-group</help>
+              <constraint>
+                <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+              </constraint>
             </properties>
             <children>
               #include <include/generic-description.xml.i>
@@ -168,6 +180,9 @@
           <tagNode name="network-group">
             <properties>
               <help>Firewall network-group</help>
+              <constraint>
+                <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+              </constraint>
             </properties>
             <children>
               #include <include/generic-description.xml.i>
@@ -189,6 +204,9 @@
           <tagNode name="port-group">
             <properties>
               <help>Firewall port-group</help>
+              <constraint>
+                <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+              </constraint>
             </properties>
             <children>
               #include <include/generic-description.xml.i>
@@ -240,6 +258,9 @@
       <tagNode name="ipv6-name">
         <properties>
           <help>IPv6 firewall rule-set name</help>
+          <constraint>
+            <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+          </constraint>
         </properties>
         <children>
           #include <include/firewall/name-default-action.xml.i>
@@ -423,6 +444,9 @@
       <tagNode name="name">
         <properties>
           <help>IPv4 firewall rule-set name</help>
+          <constraint>
+            <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+          </constraint>
         </properties>
         <children>
           #include <include/firewall/name-default-action.xml.i>
diff --git a/interface-definitions/policy-route.xml.in b/interface-definitions/policy-route.xml.in
index 4ce953b52..a1c3b50de 100644
--- a/interface-definitions/policy-route.xml.in
+++ b/interface-definitions/policy-route.xml.in
@@ -5,6 +5,9 @@
       <tagNode name="route6" owner="${vyos_conf_scripts_dir}/policy-route.py">
         <properties>
           <help>Policy route rule set name for IPv6</help>
+          <constraint>
+            <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+          </constraint>
           <priority>201</priority>
         </properties>
         <children>
@@ -51,6 +54,9 @@
       <tagNode name="route" owner="${vyos_conf_scripts_dir}/policy-route.py">
         <properties>
           <help>Policy route rule set name for IPv4</help>
+          <constraint>
+            <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+          </constraint>
           <priority>201</priority>
         </properties>
         <children>
diff --git a/interface-definitions/zone-policy.xml.in b/interface-definitions/zone-policy.xml.in
index dd64c7c16..69ee031c7 100644
--- a/interface-definitions/zone-policy.xml.in
+++ b/interface-definitions/zone-policy.xml.in
@@ -13,6 +13,9 @@
             <format>txt</format>
             <description>Zone name</description>
           </valueHelp>
+          <constraint>
+            <regex>^[a-zA-Z0-9][\w\-\.]*$</regex>
+          </constraint>
         </properties>
         <children>
           #include <include/generic-description.xml.i>
diff --git a/python/vyos/firewall.py b/python/vyos/firewall.py
index a2e133217..a74fd922a 100644
--- a/python/vyos/firewall.py
+++ b/python/vyos/firewall.py
@@ -104,13 +104,25 @@ def parse_rule(rule_conf, fw_name, rule_id, ip_name):
                 group = side_conf['group']
                 if 'address_group' in group:
                     group_name = group['address_group']
-                    output.append(f'{ip_name} {prefix}addr $A{def_suffix}_{group_name}')
+                    operator = ''
+                    if group_name[0] == '!':
+                        operator = '!='
+                        group_name = group_name[1:]
+                    output.append(f'{ip_name} {prefix}addr {operator} $A{def_suffix}_{group_name}')
                 elif 'network_group' in group:
                     group_name = group['network_group']
-                    output.append(f'{ip_name} {prefix}addr $N{def_suffix}_{group_name}')
+                    operator = ''
+                    if group_name[0] == '!':
+                        operator = '!='
+                        group_name = group_name[1:]
+                    output.append(f'{ip_name} {prefix}addr {operator} $N{def_suffix}_{group_name}')
                 if 'mac_group' in group:
                     group_name = group['mac_group']
-                    output.append(f'ether {prefix}addr $M_{group_name}')
+                    operator = ''
+                    if group_name[0] == '!':
+                        operator = '!='
+                        group_name = group_name[1:]
+                    output.append(f'ether {prefix}addr {operator} $M_{group_name}')
                 if 'port_group' in group:
                     proto = rule_conf['protocol']
                     group_name = group['port_group']
@@ -118,7 +130,12 @@ def parse_rule(rule_conf, fw_name, rule_id, ip_name):
                     if proto == 'tcp_udp':
                         proto = 'th'
 
-                    output.append(f'{proto} {prefix}port $P_{group_name}')
+                    operator = ''
+                    if group_name[0] == '!':
+                        operator = '!='
+                        group_name = group_name[1:]
+
+                    output.append(f'{proto} {prefix}port {operator} $P_{group_name}')
 
     if 'log' in rule_conf and rule_conf['log'] == 'enable':
         action = rule_conf['action'] if 'action' in rule_conf else 'accept'
diff --git a/python/vyos/template.py b/python/vyos/template.py
index 4d081b4c2..dabf53692 100644
--- a/python/vyos/template.py
+++ b/python/vyos/template.py
@@ -556,6 +556,7 @@ def nft_intra_zone_action(zone_conf, ipv6=False):
     if 'intra_zone_filtering' in zone_conf:
         intra_zone = zone_conf['intra_zone_filtering']
         fw_name = 'ipv6_name' if ipv6 else 'name'
+        name_prefix = 'NAME6_' if ipv6 else 'NAME_'
 
         if 'action' in intra_zone:
             if intra_zone['action'] == 'accept':
@@ -563,5 +564,5 @@ def nft_intra_zone_action(zone_conf, ipv6=False):
             return intra_zone['action']
         elif dict_search_args(intra_zone, 'firewall', fw_name):
             name = dict_search_args(intra_zone, 'firewall', fw_name)
-            return f'jump {name}'
+            return f'jump {name_prefix}{name}'
     return 'return'
diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py
index 6b74e6c92..ecc0c29a0 100755
--- a/smoketest/scripts/cli/test_firewall.py
+++ b/smoketest/scripts/cli/test_firewall.py
@@ -63,7 +63,7 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
 
         nftables_search = [
-            ['iifname "eth0"', 'jump smoketest'],
+            ['iifname "eth0"', 'jump NAME_smoketest'],
             ['ip saddr { 172.16.99.0/24 }', 'ip daddr 172.16.10.10', 'th dport { 53, 123 }', 'return'],
             ['ether saddr { 00:01:02:03:04:05 }', 'return']
         ]
@@ -94,7 +94,7 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
 
         nftables_search = [
-            ['iifname "eth0"', 'jump smoketest'],
+            ['iifname "eth0"', 'jump NAME_smoketest'],
             ['saddr 172.16.20.10', 'daddr 172.16.10.10', 'return'],
             ['tcp flags & (syn | ack) == syn', 'tcp dport { 8888 }', 'reject'],
             ['smoketest default-action', 'drop']
@@ -124,7 +124,7 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
 
         nftables_search = [
-            ['iifname "eth0"', 'jump v6-smoketest'],
+            ['iifname "eth0"', 'jump NAME6_v6-smoketest'],
             ['saddr 2002::1', 'daddr 2002::1:1', 'return'],
             ['meta l4proto { tcp, udp }', 'th dport { 8888 }', 'reject'],
             ['smoketest default-action', 'drop']
diff --git a/smoketest/scripts/cli/test_zone_policy.py b/smoketest/scripts/cli/test_zone_policy.py
index c0af6164b..00dfe0182 100755
--- a/smoketest/scripts/cli/test_zone_policy.py
+++ b/smoketest/scripts/cli/test_zone_policy.py
@@ -44,8 +44,8 @@ class TestZonePolicy(VyOSUnitTestSHIM.TestCase):
             ['oifname { "eth0" }', 'jump VZONE_smoketest-eth0'],
             ['jump VZONE_smoketest-local_IN'],
             ['jump VZONE_smoketest-local_OUT'],
-            ['iifname { "eth0" }', 'jump smoketest'],
-            ['oifname { "eth0" }', 'jump smoketest']
+            ['iifname { "eth0" }', 'jump NAME_smoketest'],
+            ['oifname { "eth0" }', 'jump NAME_smoketest']
         ]
 
         nftables_output = cmd('sudo nft list table ip filter')
diff --git a/src/conf_mode/firewall-interface.py b/src/conf_mode/firewall-interface.py
index a7442ecbd..9a5d278e9 100755
--- a/src/conf_mode/firewall-interface.py
+++ b/src/conf_mode/firewall-interface.py
@@ -31,6 +31,9 @@ from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
 
+NAME_PREFIX = 'NAME_'
+NAME6_PREFIX = 'NAME6_'
+
 NFT_CHAINS = {
     'in': 'VYOS_FW_FORWARD',
     'out': 'VYOS_FW_FORWARD',
@@ -127,7 +130,7 @@ def apply(if_firewall):
 
         name = dict_search_args(if_firewall, direction, 'name')
         if name:
-            rule_exists = cleanup_rule('ip filter', chain, if_prefix, ifname, name)
+            rule_exists = cleanup_rule('ip filter', chain, if_prefix, ifname, f'{NAME_PREFIX}{name}')
 
             if not rule_exists:
                 rule_action = 'insert'
@@ -138,13 +141,13 @@ def apply(if_firewall):
                     rule_action = 'add'
                     rule_prefix = f'position {handle}'
 
-                run(f'nft {rule_action} rule ip filter {chain} {rule_prefix} {if_prefix}ifname {ifname} counter jump {name}')
+                run(f'nft {rule_action} rule ip filter {chain} {rule_prefix} {if_prefix}ifname {ifname} counter jump {NAME_PREFIX}{name}')
         else:
             cleanup_rule('ip filter', chain, if_prefix, ifname)
 
         ipv6_name = dict_search_args(if_firewall, direction, 'ipv6_name')
         if ipv6_name:
-            rule_exists = cleanup_rule('ip6 filter', ipv6_chain, if_prefix, ifname, ipv6_name)
+            rule_exists = cleanup_rule('ip6 filter', ipv6_chain, if_prefix, ifname, f'{NAME6_PREFIX}{ipv6_name}')
 
             if not rule_exists:
                 rule_action = 'insert'
@@ -155,7 +158,7 @@ def apply(if_firewall):
                     rule_action = 'add'
                     rule_prefix = f'position {handle}'
 
-                run(f'nft {rule_action} rule ip6 filter {ipv6_chain} {rule_prefix} {if_prefix}ifname {ifname} counter jump {ipv6_name}')
+                run(f'nft {rule_action} rule ip6 filter {ipv6_chain} {rule_prefix} {if_prefix}ifname {ifname} counter jump {NAME6_PREFIX}{ipv6_name}')
         else:
             cleanup_rule('ip6 filter', ipv6_chain, if_prefix, ifname)
 
diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index 358b938e3..9dec2143e 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -54,6 +54,9 @@ sysfs_config = {
     'twa_hazards_protection': {'sysfs': '/proc/sys/net/ipv4/tcp_rfc1337'}
 }
 
+NAME_PREFIX = 'NAME_'
+NAME6_PREFIX = 'NAME6_'
+
 preserve_chains = [
     'INPUT',
     'FORWARD',
@@ -70,6 +73,9 @@ preserve_chains = [
     'VYOS_FRAG6_MARK'
 ]
 
+nft_iface_chains = ['VYOS_FW_FORWARD', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL']
+nft6_iface_chains = ['VYOS_FW6_FORWARD', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']
+
 valid_groups = [
     'address_group',
     'network_group',
@@ -201,6 +207,10 @@ def verify_rule(firewall, rule_conf, ipv6):
                 for group in valid_groups:
                     if group in side_conf['group']:
                         group_name = side_conf['group'][group]
+
+                        if group_name and group_name[0] == '!':
+                            group_name = group_name[1:]
+
                         fw_group = f'ipv6_{group}' if ipv6 and group in ['address_group', 'network_group'] else group
                         error_group = fw_group.replace("_", "-")
                         group_obj = dict_search_args(firewall, 'group', fw_group, group_name)
@@ -241,27 +251,29 @@ def verify(firewall):
             name = dict_search_args(if_firewall, direction, 'name')
             ipv6_name = dict_search_args(if_firewall, direction, 'ipv6_name')
 
-            if name and not dict_search_args(firewall, 'name', name):
+            if name and dict_search_args(firewall, 'name', name) == None:
                 raise ConfigError(f'Firewall name "{name}" is still referenced on interface {ifname}')
 
-            if ipv6_name and not dict_search_args(firewall, 'ipv6_name', ipv6_name):
+            if ipv6_name and dict_search_args(firewall, 'ipv6_name', ipv6_name) == None:
                 raise ConfigError(f'Firewall ipv6-name "{ipv6_name}" is still referenced on interface {ifname}')
 
     for fw_name, used_names in firewall['zone_policy'].items():
         for name in used_names:
-            if not dict_search_args(firewall, fw_name, name):
+            if dict_search_args(firewall, fw_name, name) == None:
                 raise ConfigError(f'Firewall {fw_name.replace("_", "-")} "{name}" is still referenced in zone-policy')
 
     return None
 
 def cleanup_rule(table, jump_chain):
     commands = []
-    results = cmd(f'nft -a list table {table}').split("\n")
-    for line in results:
-        if f'jump {jump_chain}' in line:
-            handle_search = re.search('handle (\d+)', line)
-            if handle_search:
-                commands.append(f'delete rule {table} {chain} handle {handle_search[1]}')
+    chains = nft_iface_chains if table == 'ip filter' else nft6_iface_chains
+    for chain in chains:
+        results = cmd(f'nft -a list chain {table} {chain}').split("\n")
+        for line in results:
+            if f'jump {jump_chain}' in line:
+                handle_search = re.search('handle (\d+)', line)
+                if handle_search:
+                    commands.append(f'delete rule {table} {chain} handle {handle_search[1]}')
     return commands
 
 def cleanup_commands(firewall):
@@ -281,9 +293,9 @@ def cleanup_commands(firewall):
                     else:
                         commands.append(f'flush chain {table} {chain}')
                 elif chain not in preserve_chains and not chain.startswith("VZONE"):
-                    if table == 'ip filter' and dict_search_args(firewall, 'name', chain):
+                    if table == 'ip filter' and dict_search_args(firewall, 'name', chain.replace(NAME_PREFIX, "", 1)) != None:
                         commands.append(f'flush chain {table} {chain}')
-                    elif table == 'ip6 filter' and dict_search_args(firewall, 'ipv6_name', chain):
+                    elif table == 'ip6 filter' and dict_search_args(firewall, 'ipv6_name', chain.replace(NAME6_PREFIX, "", 1)) != None:
                         commands.append(f'flush chain {table} {chain}')
                     else:
                         commands += cleanup_rule(table, chain)
diff --git a/src/conf_mode/policy-route.py b/src/conf_mode/policy-route.py
index 7dcab4b58..82f668acf 100755
--- a/src/conf_mode/policy-route.py
+++ b/src/conf_mode/policy-route.py
@@ -206,6 +206,7 @@ def apply_table_marks(policy):
     for route in ['route', 'route6']:
         if route in policy:
             cmd_str = 'ip' if route == 'route' else 'ip -6'
+            tables = []
             for name, pol_conf in policy[route].items():
                 if 'rule' in pol_conf:
                     for rule_id, rule_conf in pol_conf['rule'].items():
@@ -213,6 +214,9 @@ def apply_table_marks(policy):
                         if set_table:
                             if set_table == 'main':
                                 set_table = '254'
+                            if set_table in tables:
+                                continue
+                            tables.append(set_table)
                             table_mark = mark_offset - int(set_table)
                             cmd(f'{cmd_str} rule add pref {set_table} fwmark {table_mark} table {set_table}')
 
diff --git a/src/op_mode/firewall.py b/src/op_mode/firewall.py
index b6bb5b802..3146fc357 100755
--- a/src/op_mode/firewall.py
+++ b/src/op_mode/firewall.py
@@ -88,7 +88,8 @@ def get_config_firewall(conf, name=None, ipv6=False, interfaces=True):
 
 def get_nftables_details(name, ipv6=False):
     suffix = '6' if ipv6 else ''
-    command = f'sudo nft list chain ip{suffix} filter {name}'
+    name_prefix = 'NAME6_' if ipv6 else 'NAME_'
+    command = f'sudo nft list chain ip{suffix} filter {name_prefix}{name}'
     try:
         results = cmd(command)
     except: