Merge pull request #1158 from sarthurdev/firewall

firewall: policy: T4131: T4144: T4159: T4164: Fix reported firewall issues, policy-route refactor

31 files changed, 378 insertions, 111 deletions

diff --git a/data/templates/firewall/nftables-defines.tmpl b/data/templates/firewall/nftables-defines.tmpl
new file mode 100644
index 000000000..3578a9dc5
--- /dev/null
+++ b/data/templates/firewall/nftables-defines.tmpl
@@ -0,0 +1,27 @@
+{% if group is defined %}
+{%   if group.address_group is defined %}
+{%     for group_name, group_conf in group.address_group.items() %}
+define A_{{ group_name }} = { {{ group_conf.address | join(",") }} }
+{%     endfor %}
+{%   endif %}
+{%   if group.ipv6_address_group is defined %}
+{%     for group_name, group_conf in group.ipv6_address_group.items() %}
+define A6_{{ group_name }} = { {{ group_conf.address | join(",") }} }
+{%     endfor %}
+{%   endif %}
+{%   if group.network_group is defined %}
+{%     for group_name, group_conf in group.network_group.items() %}
+define N_{{ group_name }} = { {{ group_conf.network | join(",") }} }
+{%     endfor %}
+{%   endif %}
+{%   if group.ipv6_network_group is defined %}
+{%     for group_name, group_conf in group.ipv6_network_group.items() %}
+define N6_{{ group_name }} = { {{ group_conf.network | join(",") }} }
+{%     endfor %}
+{%   endif %}
+{%   if group.port_group is defined %}
+{%     for group_name, group_conf in group.port_group.items() %}
+define P_{{ group_name }} = { {{ group_conf.port | join(",") }} }
+{%     endfor %}
+{%   endif %}
+{% endif %}
\ No newline at end of file
diff --git a/data/templates/firewall/nftables-policy.tmpl b/data/templates/firewall/nftables-policy.tmpl
index ecc7e0fbd..484b6f203 100644
--- a/data/templates/firewall/nftables-policy.tmpl
+++ b/data/templates/firewall/nftables-policy.tmpl
@@ -1,5 +1,13 @@
 #!/usr/sbin/nft -f
 
+{% if cleanup_commands is defined %}
+{%   for command in cleanup_commands %}
+{{ command }}
+{%   endfor %}
+{% endif %}
+
+include "/run/nftables_defines.conf"
+
 table ip mangle {
 {% if first_install is defined %}
     chain VYOS_PBR_PREROUTING {
diff --git a/data/templates/firewall/nftables.tmpl b/data/templates/firewall/nftables.tmpl
index 68e83de64..e8fa4e306 100644
--- a/data/templates/firewall/nftables.tmpl
+++ b/data/templates/firewall/nftables.tmpl
@@ -6,43 +6,7 @@
 {%   endfor %}
 {% endif %}
 
-{% if group is defined %}
-{%   if group.address_group is defined %}
-{%     for group_name, group_conf in group.address_group.items() %}
-define A_{{ group_name }} = {
-    {{ group_conf.address | join(",") }}
-}
-{%     endfor %}
-{%   endif %}
-{%   if group.ipv6_address_group is defined %}
-{%     for group_name, group_conf in group.ipv6_address_group.items() %}
-define A6_{{ group_name }} = {
-    {{ group_conf.address | join(",") }}
-}
-{%     endfor %}
-{%   endif %}
-{%   if group.network_group is defined %}
-{%     for group_name, group_conf in group.network_group.items() %}
-define N_{{ group_name }} = {
-    {{ group_conf.network | join(",") }}
-}
-{%     endfor %}
-{%   endif %}
-{%   if group.ipv6_network_group is defined %}
-{%     for group_name, group_conf in group.ipv6_network_group.items() %}
-define N6_{{ group_name }} = {
-    {{ group_conf.network | join(",") }}
-}
-{%     endfor %}
-{%   endif %}
-{%   if group.port_group is defined %}
-{%     for group_name, group_conf in group.port_group.items() %}
-define P_{{ group_name }} = {
-    {{ group_conf.port | join(",") }}
-}
-{%     endfor %}
-{%   endif %}
-{% endif %}
+include "/run/nftables_defines.conf"
 
 table ip filter {
 {% if first_install is defined %}
diff --git a/interface-definitions/firewall.xml.in b/interface-definitions/firewall.xml.in
index 07a36093f..f428efe58 100644
--- a/interface-definitions/firewall.xml.in
+++ b/interface-definitions/firewall.xml.in
@@ -109,8 +109,13 @@
                     <format>ipv6</format>
                     <description>IPv6 address to match</description>
                   </valueHelp>
+                  <valueHelp>
+                    <format>ipv6range</format>
+                    <description>IPv6 range to match (e.g. 2002::1-2002::ff)</description>
+                  </valueHelp>
                   <constraint>
                     <validator name="ipv6-address"/>
+                    <validator name="ipv6-range"/>
                   </constraint>
                   <multi/>
                 </properties>
diff --git a/interface-definitions/include/interface/interface-policy-vif-c.xml.i b/interface-definitions/include/interface/interface-policy-vif-c.xml.i
index 5dad6422b..866fcd5c0 100644
--- a/interface-definitions/include/interface/interface-policy-vif-c.xml.i
+++ b/interface-definitions/include/interface/interface-policy-vif-c.xml.i
@@ -13,11 +13,11 @@
         </completionHelp>
       </properties>
     </leafNode>
-    <leafNode name="ipv6-route">
+    <leafNode name="route6">
       <properties>
         <help>IPv6 policy route ruleset for interface</help>
         <completionHelp>
-          <path>policy ipv6-route</path>
+          <path>policy route6</path>
         </completionHelp>
       </properties>
     </leafNode>
diff --git a/interface-definitions/include/interface/interface-policy-vif.xml.i b/interface-definitions/include/interface/interface-policy-vif.xml.i
index 5ee80ae13..83510fe59 100644
--- a/interface-definitions/include/interface/interface-policy-vif.xml.i
+++ b/interface-definitions/include/interface/interface-policy-vif.xml.i
@@ -13,11 +13,11 @@
         </completionHelp>
       </properties>
     </leafNode>
-    <leafNode name="ipv6-route">
+    <leafNode name="route6">
       <properties>
         <help>IPv6 policy route ruleset for interface</help>
         <completionHelp>
-          <path>policy ipv6-route</path>
+          <path>policy route6</path>
         </completionHelp>
       </properties>
     </leafNode>
diff --git a/interface-definitions/include/interface/interface-policy.xml.i b/interface-definitions/include/interface/interface-policy.xml.i
index 06f025af1..42a8fd009 100644
--- a/interface-definitions/include/interface/interface-policy.xml.i
+++ b/interface-definitions/include/interface/interface-policy.xml.i
@@ -13,11 +13,11 @@
         </completionHelp>
       </properties>
     </leafNode>
-    <leafNode name="ipv6-route">
+    <leafNode name="route6">
       <properties>
         <help>IPv6 policy route ruleset for interface</help>
         <completionHelp>
-          <path>policy ipv6-route</path>
+          <path>policy route6</path>
         </completionHelp>
       </properties>
     </leafNode>
diff --git a/smoketest/configs/dialup-router-medium-vpn b/smoketest/configs/dialup-router-medium-vpn
index af7c075e4..7ca540b66 100644
--- a/smoketest/configs/dialup-router-medium-vpn
+++ b/smoketest/configs/dialup-router-medium-vpn
@@ -83,6 +83,7 @@ interfaces {
         }
         policy {
             route LAN-POLICY-BASED-ROUTING
+            ipv6-route LAN6-POLICY-BASED-ROUTING
         }
         smp-affinity auto
         speed auto
@@ -383,6 +384,29 @@ nat {
     }
 }
 policy {
+    ipv6-route LAN6-POLICY-BASED-ROUTING {
+        rule 10 {
+            destination {
+            }
+            disable
+            set {
+                table 10
+            }
+            source {
+                address 2002::1
+            }
+        }
+        rule 20 {
+            destination {
+            }
+            set {
+                table 100
+            }
+            source {
+                address 2008::f
+            }
+        }
+    }
     prefix-list user2-routes {
         rule 1 {
             action permit
diff --git a/smoketest/scripts/cli/test_policy_route.py b/smoketest/scripts/cli/test_policy_route.py
index 70a234187..4463a2255 100755
--- a/smoketest/scripts/cli/test_policy_route.py
+++ b/smoketest/scripts/cli/test_policy_route.py
@@ -31,8 +31,9 @@ class TestPolicyRoute(VyOSUnitTestSHIM.TestCase):
 
     def tearDown(self):
         self.cli_delete(['interfaces', 'ethernet', 'eth0'])
+        self.cli_delete(['protocols', 'static'])
         self.cli_delete(['policy', 'route'])
-        self.cli_delete(['policy', 'ipv6-route'])
+        self.cli_delete(['policy', 'route6'])
         self.cli_commit()
 
     def test_pbr_mark(self):
@@ -65,13 +66,19 @@ class TestPolicyRoute(VyOSUnitTestSHIM.TestCase):
         self.cli_set(['policy', 'route', 'smoketest', 'rule', '1', 'protocol', 'tcp_udp'])
         self.cli_set(['policy', 'route', 'smoketest', 'rule', '1', 'destination', 'port', '8888'])
         self.cli_set(['policy', 'route', 'smoketest', 'rule', '1', 'set', 'table', table_id])
+        self.cli_set(['policy', 'route6', 'smoketest6', 'rule', '1', 'protocol', 'tcp_udp'])
+        self.cli_set(['policy', 'route6', 'smoketest6', 'rule', '1', 'destination', 'port', '8888'])
+        self.cli_set(['policy', 'route6', 'smoketest6', 'rule', '1', 'set', 'table', table_id])
 
         self.cli_set(['interfaces', 'ethernet', 'eth0', 'policy', 'route', 'smoketest'])
+        self.cli_set(['interfaces', 'ethernet', 'eth0', 'policy', 'route6', 'smoketest6'])
 
         self.cli_commit()
 
         mark_hex = "{0:#010x}".format(table_mark_offset - int(table_id))
 
+        # IPv4
+
         nftables_search = [
             ['iifname "eth0"', 'jump VYOS_PBR_smoketest'],
             ['meta l4proto { tcp, udp }', 'th dport { 8888 }', 'meta mark set ' + mark_hex]
@@ -87,6 +94,25 @@ class TestPolicyRoute(VyOSUnitTestSHIM.TestCase):
                     break
             self.assertTrue(matched)
 
+        # IPv6
+
+        nftables6_search = [
+            ['iifname "eth0"', 'jump VYOS_PBR6_smoketest'],
+            ['meta l4proto { tcp, udp }', 'th dport { 8888 }', 'meta mark set ' + mark_hex]
+        ]
+
+        nftables6_output = cmd('sudo nft list table ip6 mangle')
+
+        for search in nftables6_search:
+            matched = False
+            for line in nftables6_output.split("\n"):
+                if all(item in line for item in search):
+                    matched = True
+                    break
+            self.assertTrue(matched)
+
+        # IP rule fwmark -> table
+
         ip_rule_search = [
             ['fwmark ' + hex(table_mark_offset - int(table_id)), 'lookup ' + table_id]
         ]
diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index 0b4c0854f..7b491a325 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -22,6 +22,7 @@ from sys import exit
 
 from vyos.config import Config
 from vyos.configdict import dict_merge
+from vyos.configdict import node_changed
 from vyos.configdiff import get_config_diff, Diff
 from vyos.template import render
 from vyos.util import cmd
@@ -33,7 +34,10 @@ from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
 
+policy_route_conf_script = '/usr/libexec/vyos/conf_mode/policy-route.py'
+
 nftables_conf = '/run/nftables.conf'
+nftables_defines_conf = '/run/nftables_defines.conf'
 
 sysfs_config = {
     'all_ping': {'sysfs': '/proc/sys/net/ipv4/icmp_echo_ignore_all', 'enable': '0', 'disable': '1'},
@@ -110,6 +114,7 @@ def get_config(config=None):
     default_values = defaults(base)
     firewall = dict_merge(default_values, firewall)
 
+    firewall['policy_resync'] = bool('group' in firewall or node_changed(conf, base + ['group']))
     firewall['interfaces'] = get_firewall_interfaces(conf)
 
     if 'config_trap' in firewall and firewall['config_trap'] == 'enable':
@@ -118,6 +123,7 @@ def get_config(config=None):
         firewall['trap_targets'] = conf.get_config_dict(['service', 'snmp', 'trap-target'],
                                         key_mangling=('-', '_'), get_first_key=True,
                                         no_tag_node_value_mangle=True)
+
     return firewall
 
 def verify_rule(firewall, rule_conf, ipv6):
@@ -147,17 +153,16 @@ def verify_rule(firewall, rule_conf, ipv6):
                 for group in valid_groups:
                     if group in side_conf['group']:
                         group_name = side_conf['group'][group]
-
                         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)
 
-                        if not dict_search_args(firewall, 'group', fw_group):
-                            error_group = fw_group.replace("_", "-")
-                            raise ConfigError(f'Group defined in rule but {error_group} is not configured')
-
-                        if group_name not in firewall['group'][fw_group]:
-                            error_group = group.replace("_", "-")
+                        if group_obj is None:
                             raise ConfigError(f'Invalid {error_group} "{group_name}" on firewall rule')
 
+                        if not group_obj:
+                            print(f'WARNING: {error_group} "{group_name}" has no members')
+
             if 'port' in side_conf or dict_search_args(side_conf, 'group', 'port_group'):
                 if 'protocol' not in rule_conf:
                     raise ConfigError('Protocol must be defined if specifying a port or port-group')
@@ -236,6 +241,7 @@ def generate(firewall):
         firewall['cleanup_commands'] = cleanup_commands(firewall)
 
     render(nftables_conf, 'firewall/nftables.tmpl', firewall)
+    render(nftables_defines_conf, 'firewall/nftables-defines.tmpl', firewall)
     return None
 
 def apply_sysfs(firewall):
@@ -299,6 +305,12 @@ def state_policy_rule_exists():
     search_str = cmd(f'nft list chain ip filter VYOS_FW_FORWARD')
     return 'VYOS_STATE_POLICY' in search_str
 
+def resync_policy_route():
+    # Update policy route as firewall groups were updated
+    tmp = run(policy_route_conf_script)
+    if tmp > 0:
+        print('Warning: Failed to re-apply policy route configuration')
+
 def apply(firewall):
     if 'first_install' in firewall:
         run('nfct helper add rpc inet tcp')
@@ -318,6 +330,9 @@ def apply(firewall):
 
     apply_sysfs(firewall)
 
+    if firewall['policy_resync']:
+        resync_policy_route()
+
     post_apply_trap(firewall)
 
     return None
diff --git a/src/conf_mode/policy-route-interface.py b/src/conf_mode/policy-route-interface.py
index e81135a74..1108aebe6 100755
--- a/src/conf_mode/policy-route-interface.py
+++ b/src/conf_mode/policy-route-interface.py
@@ -52,7 +52,7 @@ def verify(if_policy):
     if not if_policy:
         return None
 
-    for route in ['route', 'ipv6_route']:
+    for route in ['route', 'route6']:
         if route in if_policy:
             if route not in if_policy['policy']:
                 raise ConfigError('Policy route not configured')
@@ -71,7 +71,7 @@ def cleanup_rule(table, chain, ifname, new_name=None):
     results = cmd(f'nft -a list chain {table} {chain}').split("\n")
     retval = None
     for line in results:
-        if f'oifname "{ifname}"' in line:
+        if f'ifname "{ifname}"' in line:
             if new_name and f'jump {new_name}' in line:
                 # new_name is used to clear rules for any previously referenced chains
                 # returns true when rule exists and doesn't need to be created
@@ -98,8 +98,8 @@ def apply(if_policy):
     else:
         cleanup_rule('ip mangle', route_chain, ifname)
 
-    if 'ipv6_route' in if_policy:
-        name = 'VYOS_PBR6_' + if_policy['ipv6_route']
+    if 'route6' in if_policy:
+        name = 'VYOS_PBR6_' + if_policy['route6']
         rule_exists = cleanup_rule('ip6 mangle', ipv6_route_chain, ifname, name)
 
         if not rule_exists:
diff --git a/src/conf_mode/policy-route.py b/src/conf_mode/policy-route.py
index 9edab4b47..c5904309f 100755
--- a/src/conf_mode/policy-route.py
+++ b/src/conf_mode/policy-route.py
@@ -31,6 +31,35 @@ airbag.enable()
 mark_offset = 0x7FFFFFFF
 nftables_conf = '/run/nftables_policy.conf'
 
+preserve_chains = [
+    'VYOS_PBR_PREROUTING',
+    'VYOS_PBR_POSTROUTING',
+    'VYOS_PBR6_PREROUTING',
+    'VYOS_PBR6_POSTROUTING'
+]
+
+valid_groups = [
+    'address_group',
+    'network_group',
+    'port_group'
+]
+
+def get_policy_interfaces(conf):
+    out = {}
+    interfaces = conf.get_config_dict(['interfaces'], key_mangling=('-', '_'), get_first_key=True,
+                                    no_tag_node_value_mangle=True)
+    def find_interfaces(iftype_conf, output={}, prefix=''):
+        for ifname, if_conf in iftype_conf.items():
+            if 'policy' in if_conf:
+                output[prefix + ifname] = if_conf['policy']
+            for vif in ['vif', 'vif_s', 'vif_c']:
+                if vif in if_conf:
+                    output.update(find_interfaces(if_conf[vif], output, f'{prefix}{ifname}.'))
+        return output
+    for iftype, iftype_conf in interfaces.items():
+        out.update(find_interfaces(iftype_conf))
+    return out
+
 def get_config(config=None):
     if config:
         conf = config
@@ -38,61 +67,117 @@ def get_config(config=None):
         conf = Config()
     base = ['policy']
 
-    if not conf.exists(base + ['route']) and not conf.exists(base + ['ipv6-route']):
-        return None
-
     policy = conf.get_config_dict(base, key_mangling=('-', '_'), get_first_key=True,
                                     no_tag_node_value_mangle=True)
 
+    policy['firewall_group'] = conf.get_config_dict(['firewall', 'group'], key_mangling=('-', '_'), get_first_key=True,
+                                    no_tag_node_value_mangle=True)
+    policy['interfaces'] = get_policy_interfaces(conf)
+
     return policy
 
-def verify(policy):
-    # bail out early - looks like removal from running config
-    if not policy:
-        return None
+def verify_rule(policy, rule_conf, ipv6):
+    icmp = 'icmp' if not ipv6 else 'icmpv6'
+    if icmp in rule_conf:
+        icmp_defined = False
+        if 'type_name' in rule_conf[icmp]:
+            icmp_defined = True
+            if 'code' in rule_conf[icmp] or 'type' in rule_conf[icmp]:
+                raise ConfigError(f'{name} rule {rule_id}: Cannot use ICMP type/code with ICMP type-name')
+        if 'code' in rule_conf[icmp]:
+            icmp_defined = True
+            if 'type' not in rule_conf[icmp]:
+                raise ConfigError(f'{name} rule {rule_id}: ICMP code can only be defined if ICMP type is defined')
+        if 'type' in rule_conf[icmp]:
+            icmp_defined = True
+
+        if icmp_defined and 'protocol' not in rule_conf or rule_conf['protocol'] != icmp:
+            raise ConfigError(f'{name} rule {rule_id}: ICMP type/code or type-name can only be defined if protocol is ICMP')
+    if 'set' in rule_conf:
+        if 'tcp_mss' in rule_conf['set']:
+            tcp_flags = dict_search_args(rule_conf, 'tcp', 'flags')
+            if not tcp_flags or 'SYN' not in tcp_flags.split(","):
+                raise ConfigError(f'{name} rule {rule_id}: TCP SYN flag must be set to modify TCP-MSS')
+    if 'tcp' in rule_conf:
+        if 'flags' in rule_conf['tcp']:
+            if 'protocol' not in rule_conf or rule_conf['protocol'] != 'tcp':
+                raise ConfigError(f'{name} rule {rule_id}: TCP flags can only be set if protocol is set to TCP')
+
+    for side in ['destination', 'source']:
+        if side in rule_conf:
+            side_conf = rule_conf[side]
+
+            if 'group' in side_conf:
+                if {'address_group', 'network_group'} <= set(side_conf['group']):
+                    raise ConfigError('Only one address-group or network-group can be specified')
+
+                for group in valid_groups:
+                    if group in side_conf['group']:
+                        group_name = side_conf['group'][group]
+                        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(policy['firewall_group'], fw_group, group_name)
+
+                        if group_obj is None:
+                            raise ConfigError(f'Invalid {error_group} "{group_name}" on policy route rule')
+
+                        if not group_obj:
+                            print(f'WARNING: {error_group} "{group_name}" has no members')
+
+            if 'port' in side_conf or dict_search_args(side_conf, 'group', 'port_group'):
+                if 'protocol' not in rule_conf:
+                    raise ConfigError('Protocol must be defined if specifying a port or port-group')
+
+                if rule_conf['protocol'] not in ['tcp', 'udp', 'tcp_udp']:
+                    raise ConfigError('Protocol must be tcp, udp, or tcp_udp when specifying a port or port-group')
 
+def verify(policy):
     for route in ['route', 'route6']:
+        ipv6 = route == 'route6'
         if route in policy:
             for name, pol_conf in policy[route].items():
                 if 'rule' in pol_conf:
-                    for rule_id, rule_conf in pol_conf.items():
-                        icmp = 'icmp' if route == 'route' else 'icmpv6'
-                        if icmp in rule_conf:
-                            icmp_defined = False
-                            if 'type_name' in rule_conf[icmp]:
-                                icmp_defined = True
-                                if 'code' in rule_conf[icmp] or 'type' in rule_conf[icmp]:
-                                    raise ConfigError(f'{name} rule {rule_id}: Cannot use ICMP type/code with ICMP type-name')
-                            if 'code' in rule_conf[icmp]:
-                                icmp_defined = True
-                                if 'type' not in rule_conf[icmp]:
-                                    raise ConfigError(f'{name} rule {rule_id}: ICMP code can only be defined if ICMP type is defined')
-                            if 'type' in rule_conf[icmp]:
-                                icmp_defined = True
-
-                            if icmp_defined and 'protocol' not in rule_conf or rule_conf['protocol'] != icmp:
-                                raise ConfigError(f'{name} rule {rule_id}: ICMP type/code or type-name can only be defined if protocol is ICMP')
-                        if 'set' in rule_conf:
-                            if 'tcp_mss' in rule_conf['set']:
-                                tcp_flags = dict_search_args(rule_conf, 'tcp', 'flags')
-                                if not tcp_flags or 'SYN' not in tcp_flags.split(","):
-                                    raise ConfigError(f'{name} rule {rule_id}: TCP SYN flag must be set to modify TCP-MSS')
-                        if 'tcp' in rule_conf:
-                            if 'flags' in rule_conf['tcp']:
-                                if 'protocol' not in rule_conf or rule_conf['protocol'] != 'tcp':
-                                    raise ConfigError(f'{name} rule {rule_id}: TCP flags can only be set if protocol is set to TCP')
+                    for rule_id, rule_conf in pol_conf['rule'].items():
+                        verify_rule(policy, rule_conf, ipv6)
+
+    for ifname, if_policy in policy['interfaces'].items():
+        name = dict_search_args(if_policy, 'route')
+        ipv6_name = dict_search_args(if_policy, 'route6')
 
+        if name and not dict_search_args(policy, 'route', name):
+            raise ConfigError(f'Policy route "{name}" is still referenced on interface {ifname}')
+
+        if ipv6_name and not dict_search_args(policy, 'route6', ipv6_name):
+            raise ConfigError(f'Policy route6 "{ipv6_name}" is still referenced on interface {ifname}')
 
     return None
 
-def generate(policy):
-    if not policy:
-        if os.path.exists(nftables_conf):
-            os.unlink(nftables_conf)
-        return None
+def cleanup_commands(policy):
+    commands = []
+    for table in ['ip mangle', 'ip6 mangle']:
+        json_str = cmd(f'nft -j list table {table}')
+        obj = loads(json_str)
+        if 'nftables' not in obj:
+            continue
+        for item in obj['nftables']:
+            if 'chain' in item:
+                chain = item['chain']['name']
+                if not chain.startswith("VYOS_PBR"):
+                    continue
+                if chain not in preserve_chains:
+                    if table == 'ip mangle' and dict_search_args(policy, 'route', chain.replace("VYOS_PBR_", "", 1)):
+                        commands.append(f'flush chain {table} {chain}')
+                    elif table == 'ip6 mangle' and dict_search_args(policy, 'route6', chain.replace("VYOS_PBR6_", "", 1)):
+                        commands.append(f'flush chain {table} {chain}')
+                    else:
+                        commands.append(f'delete chain {table} {chain}')
+    return commands
 
+def generate(policy):
     if not os.path.exists(nftables_conf):
         policy['first_install'] = True
+    else:
+        policy['cleanup_commands'] = cleanup_commands(policy)
 
     render(nftables_conf, 'firewall/nftables-policy.tmpl', policy)
     return None
@@ -124,14 +209,6 @@ def cleanup_table_marks():
             cmd(f'ip rule del fwmark {fwmark} table {table}')
 
 def apply(policy):
-    if not policy or 'first_install' not in policy:
-        run(f'nft flush table ip mangle')
-        run(f'nft flush table ip6 mangle')
-
-    if not policy:
-        cleanup_table_marks()
-        return None
-
     install_result = run(f'nft -f {nftables_conf}')
     if install_result == 1:
         raise ConfigError('Failed to apply policy based routing')
diff --git a/src/migration-scripts/policy/1-to-2 b/src/migration-scripts/policy/1-to-2
index 3e46227de..7ffceef22 100755
--- a/src/migration-scripts/policy/1-to-2
+++ b/src/migration-scripts/policy/1-to-2
@@ -41,6 +41,24 @@ if not config.exists(base):
 config.rename(base, 'route6')
 config.set_tag(['policy', 'route6'])
 
+if config.exists(['interfaces']):
+    def if_policy_rename(config, path):
+        if config.exists(path + ['policy', 'ipv6-route']):
+            config.rename(path + ['policy', 'ipv6-route'], 'route6')
+
+    for if_type in config.list_nodes(['interfaces']):
+        for ifname in config.list_nodes(['interfaces', if_type]):
+            if_path = ['interfaces', if_type, ifname]
+            if_policy_rename(config, if_path)
+
+        for vif_type in ['vif', 'vif-s']:
+            if config.exists(if_path + [vif_type]):
+                for vifname in config.list_nodes(if_path + [vif_type]):
+                    if_policy_rename(config, if_path + [vif_type, vifname])
+
+                    if config.exists(if_path + [vif_type, vifname, 'vif-c']):
+                        for vifcname in config.list_nodes(if_path + [vif_type, vifname, 'vif-c']):
+                            if_policy_rename(config, if_path + [vif_type, vifname, 'vif-c', vifcname])
 try:
     with open(file_name, 'w') as f:
         f.write(config.to_string())
diff --git a/src/op_mode/firewall.py b/src/op_mode/firewall.py
index cf70890a6..030a9b19a 100755
--- a/src/op_mode/firewall.py
+++ b/src/op_mode/firewall.py
@@ -15,6 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import argparse
+import ipaddress
 import json
 import re
 import tabulate
@@ -266,13 +267,15 @@ def show_firewall_group(name=None):
                 continue
 
             references = find_references(group_type, group_name)
-            row = [group_name, group_type, ', '.join(references)]
+            row = [group_name, group_type, '\n'.join(references) or 'N/A']
             if 'address' in group_conf:
-                row.append(", ".join(group_conf['address']))
+                row.append("\n".join(sorted(group_conf['address'], key=ipaddress.ip_address)))
             elif 'network' in group_conf:
-                row.append(", ".join(group_conf['network']))
+                row.append("\n".join(sorted(group_conf['network'], key=ipaddress.ip_network)))
             elif 'port' in group_conf:
-                row.append(", ".join(group_conf['port']))
+                row.append("\n".join(sorted(group_conf['port'])))
+            else:
+                row.append('N/A')
             rows.append(row)
 
     if rows:
@@ -302,7 +305,7 @@ def show_summary():
         for name, name_conf in firewall['ipv6_name'].items():
             description = name_conf.get('description', '')
             interfaces = ", ".join(name_conf['interface'])
-            v6_out.append([name, description, interfaces])
+            v6_out.append([name, description, interfaces or 'N/A'])
 
     if v6_out:
         print('\nIPv6 name:\n')
diff --git a/src/op_mode/policy_route.py b/src/op_mode/policy_route.py
index e0b4ac514..95a7eadac 100755
--- a/src/op_mode/policy_route.py
+++ b/src/op_mode/policy_route.py
@@ -26,7 +26,7 @@ def get_policy_interfaces(conf, policy, name=None, ipv6=False):
     interfaces = conf.get_config_dict(['interfaces'], key_mangling=('-', '_'),
                                       get_first_key=True, no_tag_node_value_mangle=True)
 
-    routes = ['route', 'ipv6_route']
+    routes = ['route', 'route6']
 
     def parse_if(ifname, if_conf):
         if 'policy' in if_conf:
@@ -52,7 +52,7 @@ def get_policy_interfaces(conf, policy, name=None, ipv6=False):
 def get_config_policy(conf, name=None, ipv6=False, interfaces=True):
     config_path = ['policy']
     if name:
-        config_path += ['ipv6-route' if ipv6 else 'route', name]
+        config_path += ['route6' if ipv6 else 'route', name]
 
     policy = conf.get_config_dict(config_path, key_mangling=('-', '_'),
                                 get_first_key=True, no_tag_node_value_mangle=True)
@@ -64,7 +64,7 @@ def get_config_policy(conf, name=None, ipv6=False, interfaces=True):
                 for route_name, route_conf in policy['route'].items():
                     route_conf['interface'] = []
 
-            if 'ipv6_route' in policy:
+            if 'route6' in policy:
                 for route_name, route_conf in policy['ipv6_route'].items():
                     route_conf['interface'] = []
 
@@ -151,8 +151,8 @@ def show_policy(ipv6=False):
         for route, route_conf in policy['route'].items():
             output_policy_route(route, route_conf, ipv6=False)
 
-    if ipv6 and 'ipv6_route' in policy:
-        for route, route_conf in policy['ipv6_route'].items():
+    if ipv6 and 'route6' in policy:
+        for route, route_conf in policy['route6'].items():
             output_policy_route(route, route_conf, ipv6=True)
 
 def show_policy_name(name, ipv6=False):
diff --git a/src/validators/ip-address b/src/validators/ip-address
index 51fb72c85..11d6df09e 100755
--- a/src/validators/ip-address
+++ b/src/validators/ip-address
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-any-single $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IP address"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ip-cidr b/src/validators/ip-cidr
index 987bf84ca..60d2ac295 100755
--- a/src/validators/ip-cidr
+++ b/src/validators/ip-cidr
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-any-cidr $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IP CIDR"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ip-host b/src/validators/ip-host
index f2906e8cf..77c578fa2 100755
--- a/src/validators/ip-host
+++ b/src/validators/ip-host
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-any-host $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IP host"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ip-prefix b/src/validators/ip-prefix
index e58aad395..e5a64fea8 100755
--- a/src/validators/ip-prefix
+++ b/src/validators/ip-prefix
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-any-net $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IP prefix"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ip-protocol b/src/validators/ip-protocol
index 7898fa6d0..c4c882502 100755
--- a/src/validators/ip-protocol
+++ b/src/validators/ip-protocol
@@ -38,4 +38,5 @@ if __name__ == '__main__':
     if re.match(pattern, input):
         exit(0)
 
+    print(f'Error: {input} is not a valid IP protocol')
     exit(1)
diff --git a/src/validators/ipv4 b/src/validators/ipv4
index 53face090..8676d5800 100755
--- a/src/validators/ipv4
+++ b/src/validators/ipv4
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv4 $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not IPv4"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv4-address b/src/validators/ipv4-address
index 872a7645a..058db088b 100755
--- a/src/validators/ipv4-address
+++ b/src/validators/ipv4-address
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv4-single $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv4 address"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv4-host b/src/validators/ipv4-host
index f42feffa4..74b8c36a7 100755
--- a/src/validators/ipv4-host
+++ b/src/validators/ipv4-host
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv4-host $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv4 host"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv4-multicast b/src/validators/ipv4-multicast
index 5465c728d..3f28c51db 100755
--- a/src/validators/ipv4-multicast
+++ b/src/validators/ipv4-multicast
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv4-multicast $1 && ipaddrcheck --is-ipv4-single $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv4 multicast address"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv4-prefix b/src/validators/ipv4-prefix
index 8ec8a2c45..7e1e0e8dd 100755
--- a/src/validators/ipv4-prefix
+++ b/src/validators/ipv4-prefix
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv4-net $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv4 prefix"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv6 b/src/validators/ipv6
index f18d4a63e..4ae130eb5 100755
--- a/src/validators/ipv6
+++ b/src/validators/ipv6
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv6 $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not IPv6"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv6-address b/src/validators/ipv6-address
index e5d68d756..1fca77668 100755
--- a/src/validators/ipv6-address
+++ b/src/validators/ipv6-address
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv6-single $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv6 address"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv6-host b/src/validators/ipv6-host
index f7a745077..7085809a9 100755
--- a/src/validators/ipv6-host
+++ b/src/validators/ipv6-host
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv6-host $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv6 host"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv6-multicast b/src/validators/ipv6-multicast
index 5afc437e5..5aa7d734a 100755
--- a/src/validators/ipv6-multicast
+++ b/src/validators/ipv6-multicast
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv6-multicast $1 && ipaddrcheck --is-ipv6-single $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv6 multicast address"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv6-prefix b/src/validators/ipv6-prefix
index e43616350..890dda723 100755
--- a/src/validators/ipv6-prefix
+++ b/src/validators/ipv6-prefix
@@ -1,3 +1,10 @@
 #!/bin/sh
 
 ipaddrcheck --is-ipv6-net $1
+
+if [ $? -gt 0 ]; then
+    echo "Error: $1 is not a valid IPv6 prefix"
+    exit 1
+fi
+
+exit 0
\ No newline at end of file
diff --git a/src/validators/ipv6-range b/src/validators/ipv6-range
index 033b6461b..a3c401281 100755
--- a/src/validators/ipv6-range
+++ b/src/validators/ipv6-range
@@ -11,6 +11,7 @@ if __name__ == '__main__':
         if re.search('([a-f0-9:]+:+)+[a-f0-9]+-([a-f0-9:]+:+)+[a-f0-9]+', ipv6_range):
             for tmp in ipv6_range.split('-'):
                 if not is_ipv6(tmp):
+                    print(f'Error: {ipv6_range} is not a valid IPv6 range')
                     sys.exit(1)
 
     sys.exit(0)