Merge pull request #1134 from sarthurdev/firewall

firewall: zone-policy: T2199: T4130: Fixes for firewall, state-policy and zone-policy

7 files changed, 44 insertions, 41 deletions

diff --git a/data/templates/firewall/nftables.tmpl b/data/templates/firewall/nftables.tmpl
index bbb111b1f..68e83de64 100644
--- a/data/templates/firewall/nftables.tmpl
+++ b/data/templates/firewall/nftables.tmpl
@@ -46,11 +46,8 @@ define P_{{ group_name }} = {
 
 table ip filter {
 {% if first_install is defined %}
-    chain VYOS_FW_IN {
+    chain VYOS_FW_FORWARD {
         type filter hook forward priority 0; policy accept;
-    }
-    chain VYOS_FW_OUT {
-        type filter hook forward priority 1; policy accept;
         jump VYOS_POST_FW
     }
     chain VYOS_FW_LOCAL {
@@ -104,11 +101,8 @@ table ip filter {
 
 table ip6 filter {
 {% if first_install is defined %}
-    chain VYOS_FW6_IN {
+    chain VYOS_FW6_FORWARD {
         type filter hook forward priority 0; policy accept;
-    }
-    chain VYOS_FW6_OUT {
-        type filter hook forward priority 1; policy accept;
         jump VYOS_POST_FW6
     }
     chain VYOS_FW6_LOCAL {
diff --git a/data/templates/zone_policy/nftables.tmpl b/data/templates/zone_policy/nftables.tmpl
index 21230c688..fae6e8c4f 100644
--- a/data/templates/zone_policy/nftables.tmpl
+++ b/data/templates/zone_policy/nftables.tmpl
@@ -81,7 +81,7 @@ table ip6 filter {
 insert rule ip filter VYOS_FW_LOCAL counter jump VZONE_{{ zone_name }}_IN
 insert rule ip filter VYOS_FW_OUTPUT counter jump VZONE_{{ zone_name }}_OUT
 {%       else %}
-insert rule ip filter VYOS_FW_OUT oifname { {{ zone_conf.interface | join(',') }} } counter jump VZONE_{{ zone_name }}
+insert rule ip filter VYOS_FW_FORWARD oifname { {{ zone_conf.interface | join(',') }} } counter jump VZONE_{{ zone_name }}
 {%       endif %}
 {%     endif %}
 {%     if zone_conf.ipv6 %}
@@ -89,9 +89,19 @@ insert rule ip filter VYOS_FW_OUT oifname { {{ zone_conf.interface | join(',') }
 insert rule ip6 filter VYOS_FW6_LOCAL counter jump VZONE6_{{ zone_name }}_IN
 insert rule ip6 filter VYOS_FW6_OUTPUT counter jump VZONE6_{{ zone_name }}_OUT
 {%       else %}
-insert rule ip6 filter VYOS_FW6_OUT oifname { {{ zone_conf.interface | join(',') }} } counter jump VZONE6_{{ zone_name }}
+insert rule ip6 filter VYOS_FW6_FORWARD oifname { {{ zone_conf.interface | join(',') }} } counter jump VZONE6_{{ zone_name }}
 {%       endif %}
 {%     endif %}
 {%   endfor %}
 
+{# Ensure that state-policy rule is first in the chain #}
+{%   if firewall.state_policy is defined %}
+{%     for chain in ['VYOS_FW_FORWARD', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL'] %}
+insert rule ip filter {{ chain }} jump VYOS_STATE_POLICY
+{%     endfor %}
+{%     for chain in ['VYOS_FW6_FORWARD', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL'] %}
+insert rule ip6 filter {{ chain }} jump VYOS_STATE_POLICY6
+{%     endfor %}
+{%   endif %}
+
 {% endif %}
diff --git a/python/vyos/template.py b/python/vyos/template.py
index 7671bf377..6f65c6c98 100644
--- a/python/vyos/template.py
+++ b/python/vyos/template.py
@@ -526,11 +526,7 @@ def nft_state_policy(conf, state, ipv6=False):
     out.append('counter')
 
     if 'action' in conf:
-        if conf['action'] == 'accept':
-            jump_target = 'VYOS_POST_FW6' if ipv6 else 'VYOS_POST_FW'
-            out.append(f'jump {jump_target}')
-        else:
-            out.append(conf['action'])
+        out.append(conf['action'])
 
     return " ".join(out)
 
diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py
index 5f728f0cd..2b3b354ba 100755
--- a/smoketest/scripts/cli/test_firewall.py
+++ b/smoketest/scripts/cli/test_firewall.py
@@ -142,8 +142,8 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
 
         chains = {
-            'ip filter': ['VYOS_FW_IN', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL'],
-            'ip6 filter': ['VYOS_FW6_IN', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']
+            'ip filter': ['VYOS_FW_FORWARD', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL'],
+            'ip6 filter': ['VYOS_FW6_FORWARD', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']
         }
 
         for table in ['ip filter', 'ip6 filter']:
diff --git a/src/conf_mode/firewall-interface.py b/src/conf_mode/firewall-interface.py
index 516fa6c48..b0df9dff4 100755
--- a/src/conf_mode/firewall-interface.py
+++ b/src/conf_mode/firewall-interface.py
@@ -32,13 +32,13 @@ from vyos import airbag
 airbag.enable()
 
 NFT_CHAINS = {
-    'in': 'VYOS_FW_IN',
-    'out': 'VYOS_FW_OUT',
+    'in': 'VYOS_FW_FORWARD',
+    'out': 'VYOS_FW_FORWARD',
     'local': 'VYOS_FW_LOCAL'
 }
 NFT6_CHAINS = {
-    'in': 'VYOS_FW6_IN',
-    'out': 'VYOS_FW6_OUT',
+    'in': 'VYOS_FW6_FORWARD',
+    'out': 'VYOS_FW6_FORWARD',
     'local': 'VYOS_FW6_LOCAL'
 }
 
@@ -91,11 +91,11 @@ def verify(if_firewall):
 def generate(if_firewall):
     return None
 
-def cleanup_rule(table, chain, ifname, new_name=None):
+def cleanup_rule(table, chain, prefix, ifname, new_name=None):
     results = cmd(f'nft -a list chain {table} {chain}').split("\n")
     retval = None
     for line in results:
-        if f'ifname "{ifname}"' in line:
+        if f'{prefix}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
@@ -108,6 +108,7 @@ def cleanup_rule(table, chain, ifname, new_name=None):
     return retval
 
 def state_policy_handle(table, chain):
+    # Find any state-policy rule to ensure interface rules are only inserted afterwards
     results = cmd(f'nft -a list chain {table} {chain}').split("\n")
     for line in results:
         if 'jump VYOS_STATE_POLICY' in line:
@@ -126,11 +127,12 @@ def apply(if_firewall):
 
         name = dict_search_args(if_firewall, direction, 'name')
         if name:
-            rule_exists = cleanup_rule('ip filter', chain, ifname, name)
-            rule_action = 'insert'
-            rule_prefix = ''
+            rule_exists = cleanup_rule('ip filter', chain, if_prefix, ifname, name)
 
             if not rule_exists:
+                rule_action = 'insert'
+                rule_prefix = ''
+
                 handle = state_policy_handle('ip filter', chain)
                 if handle:
                     rule_action = 'add'
@@ -138,15 +140,16 @@ def apply(if_firewall):
 
                 run(f'nft {rule_action} rule ip filter {chain} {rule_prefix} {if_prefix}ifname {ifname} counter jump {name}')
         else:
-            cleanup_rule('ip filter', chain, ifname)
+            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, ifname, ipv6_name)
-            rule_action = 'insert'
-            rule_prefix = ''
+            rule_exists = cleanup_rule('ip6 filter', ipv6_chain, if_prefix, ifname, ipv6_name)
 
             if not rule_exists:
+                rule_action = 'insert'
+                rule_prefix = ''
+
                 handle = state_policy_handle('ip filter', chain)
                 if handle:
                     rule_action = 'add'
@@ -154,7 +157,7 @@ def apply(if_firewall):
 
                 run(f'nft {rule_action} rule ip6 filter {ipv6_chain} {rule_prefix} {if_prefix}ifname {ifname} counter jump {ipv6_name}')
         else:
-            cleanup_rule('ip6 filter', ipv6_chain, ifname)
+            cleanup_rule('ip6 filter', ipv6_chain, if_prefix, ifname)
 
     return None
 
diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index 8e037c679..6016d94fa 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -53,14 +53,12 @@ preserve_chains = [
     'INPUT',
     'FORWARD',
     'OUTPUT',
-    'VYOS_FW_IN',
-    'VYOS_FW_OUT',
+    'VYOS_FW_FORWARD',
     'VYOS_FW_LOCAL',
     'VYOS_FW_OUTPUT',
     'VYOS_POST_FW',
     'VYOS_FRAG_MARK',
-    'VYOS_FW6_IN',
-    'VYOS_FW6_OUT',
+    'VYOS_FW6_FORWARD',
     'VYOS_FW6_LOCAL',
     'VYOS_FW6_OUTPUT',
     'VYOS_POST_FW6',
@@ -228,7 +226,7 @@ def cleanup_commands(firewall):
                         commands.append(f'delete chain {table} {chain}')
             elif 'rule' in item:
                 rule = item['rule']
-                if rule['chain'] in ['VYOS_FW_IN', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL', 'VYOS_FW6_IN', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']:
+                if rule['chain'] in ['VYOS_FW_FORWARD', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL', 'VYOS_FW6_FORWARD', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']:
                     if 'expr' in rule and any([True for expr in rule['expr'] if dict_search_args(expr, 'jump', 'target') == state_chain]):
                         if 'state_policy' not in firewall:
                             chain = rule['chain']
@@ -303,7 +301,7 @@ def post_apply_trap(firewall):
 
 def state_policy_rule_exists():
     # Determine if state policy rules already exist in nft
-    search_str = cmd(f'nft list chain ip filter VYOS_FW_IN')
+    search_str = cmd(f'nft list chain ip filter VYOS_FW_FORWARD')
     return 'VYOS_STATE_POLICY' in search_str
 
 def apply(firewall):
@@ -317,10 +315,10 @@ def apply(firewall):
         raise ConfigError('Failed to apply firewall')
 
     if 'state_policy' in firewall and not state_policy_rule_exists():
-        for chain in ['VYOS_FW_IN', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL']:
+        for chain in ['VYOS_FW_FORWARD', 'VYOS_FW_OUTPUT', 'VYOS_FW_LOCAL']:
             cmd(f'nft insert rule ip filter {chain} jump VYOS_STATE_POLICY')
 
-        for chain in ['VYOS_FW6_IN', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']:
+        for chain in ['VYOS_FW6_FORWARD', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']:
             cmd(f'nft insert rule ip6 filter {chain} jump VYOS_STATE_POLICY6')
 
     apply_sysfs(firewall)
diff --git a/src/conf_mode/zone_policy.py b/src/conf_mode/zone_policy.py
index 2535ea33b..d605e9639 100755
--- a/src/conf_mode/zone_policy.py
+++ b/src/conf_mode/zone_policy.py
@@ -152,7 +152,9 @@ def cleanup_commands():
                     continue
                 for expr in item['rule']['expr']:
                     target = dict_search_args(expr, 'jump', 'target')
-                    if target and target.startswith("VZONE"):
+                    if not target:
+                        continue
+                    if target.startswith("VZONE") or target.startswith("VYOS_STATE_POLICY"):
                         commands.append(f'delete rule {table} {chain} handle {handle}')
         for item in obj['nftables']:
             if 'chain' in item:
@@ -180,7 +182,7 @@ def generate(zone_policy):
 
 def apply(zone_policy):
     install_result = run(f'nft -f {nftables_conf}')
-    if install_result == 1:
+    if install_result != 0:
         raise ConfigError('Failed to apply zone-policy')
 
     return None