From fd0bcaf120bc4ad5f3e9add93f0fa2c2c60e984f Mon Sep 17 00:00:00 2001
From: sarthurdev <965089+sarthurdev@users.noreply.github.com>
Date: Thu, 21 Sep 2023 12:05:20 +0200
Subject: conntrack: T5376: T5598: Fix for kernel conntrack helpers

`nf_conntrack_helper` that auto-assigned helpers is removed from the kernel
---
 data/templates/conntrack/nftables-ct.j2            | 51 +---------------
 data/templates/conntrack/nftables-helpers.j2       | 70 ++++++++++++++++++++++
 data/templates/conntrack/vyos_nf_conntrack.conf.j2 |  3 +-
 smoketest/scripts/cli/test_system_conntrack.py     | 37 +++++++-----
 src/conf_mode/conntrack.py                         | 63 ++++++++++++-------
 5 files changed, 137 insertions(+), 87 deletions(-)
 create mode 100644 data/templates/conntrack/nftables-helpers.j2

diff --git a/data/templates/conntrack/nftables-ct.j2 b/data/templates/conntrack/nftables-ct.j2
index 895f61a55..1e0fc8065 100644
--- a/data/templates/conntrack/nftables-ct.j2
+++ b/data/templates/conntrack/nftables-ct.j2
@@ -1,5 +1,6 @@
 #!/usr/sbin/nft -f
 
+{% import 'conntrack/nftables-helpers.j2' as helper_tmpl %}
 {% import 'firewall/nftables-defines.j2' as group_tmpl %}
 
 {% if first_install is not vyos_defined %}
@@ -52,30 +53,7 @@ table ip vyos_conntrack {
         notrack
     }
 
-    ct helper rpc_tcp {
-        type "rpc" protocol tcp;
-    }
-
-    ct helper rpc_udp {
-        type "rpc" protocol udp;
-    }
-
-    ct helper tns_tcp {
-        type "tns" protocol tcp;
-    }
-
-    chain VYOS_CT_HELPER {
-{% for module, module_conf in module_map.items() %}
-{%     if modules[module] is vyos_defined %}
-{%         if 'nftables' in module_conf %}
-{%             for rule in module_conf.nftables %}
-        {{ rule }}
-{%             endfor %}
-{%         endif %}
-{%     endif %}
-{% endfor %}
-        return
-    }
+{{ helper_tmpl.conntrack_helpers(module_map, modules, ipv4=True) }}
 
     chain FW_CONNTRACK {
         {{ ipv4_firewall_action }}
@@ -140,30 +118,7 @@ table ip6 vyos_conntrack {
         notrack
     }
 
-    ct helper rpc_tcp {
-        type "rpc" protocol tcp;
-    }
-
-    ct helper rpc_udp {
-        type "rpc" protocol udp;
-    }
-
-    ct helper tns_tcp {
-        type "tns" protocol tcp;
-    }
-
-    chain VYOS_CT_HELPER {
-{% for module, module_conf in module_map.items() %}
-{%     if modules[module] is vyos_defined %}
-{%         if 'nftables' in module_conf %}
-{%             for rule in module_conf.nftables %}
-        {{ rule }}
-{%             endfor %}
-{%         endif %}
-{%     endif %}
-{% endfor %}
-        return
-    }
+{{ helper_tmpl.conntrack_helpers(module_map, modules, ipv4=False) }}
 
     chain FW_CONNTRACK {
         {{ ipv6_firewall_action }}
diff --git a/data/templates/conntrack/nftables-helpers.j2 b/data/templates/conntrack/nftables-helpers.j2
new file mode 100644
index 000000000..433931162
--- /dev/null
+++ b/data/templates/conntrack/nftables-helpers.j2
@@ -0,0 +1,70 @@
+{% macro conntrack_helpers(module_map, modules, ipv4=True) %}
+{% if modules.ftp is vyos_defined %}
+    ct helper ftp_tcp {
+        type "ftp" protocol tcp;
+    }
+{% endif %}
+
+{% if modules.h323 is vyos_defined %}
+    ct helper ras_udp {
+        type "RAS" protocol udp;
+    }
+
+    ct helper q931_tcp {
+        type "Q.931" protocol tcp;
+    }
+{% endif %}
+
+{% if modules.pptp is vyos_defined and ipv4 %}
+    ct helper pptp_tcp {
+        type "pptp" protocol tcp;
+    }
+{% endif %}
+
+{% if modules.nfs is vyos_defined %}
+    ct helper rpc_tcp {
+        type "rpc" protocol tcp;
+    }
+
+    ct helper rpc_udp {
+        type "rpc" protocol udp;
+    }
+{% endif %}
+
+{% if modules.sip is vyos_defined %}
+    ct helper sip_tcp {
+        type "sip" protocol tcp;
+    }
+
+    ct helper sip_udp {
+        type "sip" protocol udp;
+    }
+{% endif %}
+
+{% if modules.tftp is vyos_defined %}
+    ct helper tftp_udp {
+        type "tftp" protocol udp;
+    }
+{% endif %}
+
+{% if modules.sqlnet is vyos_defined %}
+    ct helper tns_tcp {
+        type "tns" protocol tcp;
+    }
+{% endif %}
+
+    chain VYOS_CT_HELPER {
+{% for module, module_conf in module_map.items() %}
+{%     if modules[module] is vyos_defined %}
+{%         if 'nftables' in module_conf %}
+{%             if module_conf.ipv4 is not vyos_defined or module_conf.ipv4 == ipv4 %}
+{%                 for rule in module_conf.nftables %}
+        {{ rule }}
+{%                 endfor %}
+{%             endif %}
+{%         endif %}
+{%     endif %}
+{% endfor %}
+        return
+    }
+{% endmacro %}
diff --git a/data/templates/conntrack/vyos_nf_conntrack.conf.j2 b/data/templates/conntrack/vyos_nf_conntrack.conf.j2
index 111459485..197155d96 100644
--- a/data/templates/conntrack/vyos_nf_conntrack.conf.j2
+++ b/data/templates/conntrack/vyos_nf_conntrack.conf.j2
@@ -1,3 +1,2 @@
 # Autogenerated by conntrack.py
-options nf_conntrack hashsize={{ hash_size }} nf_conntrack_helper=1
-
+options nf_conntrack hashsize={{ hash_size }}
diff --git a/smoketest/scripts/cli/test_system_conntrack.py b/smoketest/scripts/cli/test_system_conntrack.py
index c9f184558..7657ab724 100755
--- a/smoketest/scripts/cli/test_system_conntrack.py
+++ b/smoketest/scripts/cli/test_system_conntrack.py
@@ -162,27 +162,34 @@ class TestSystemConntrack(VyOSUnitTestSHIM.TestCase):
     def test_conntrack_module_enable(self):
         # conntrack helper modules are disabled by default
         modules = {
-            'ftp' : {
-                'driver' : ['nf_nat_ftp', 'nf_conntrack_ftp'],
+            'ftp': {
+                'driver': ['nf_nat_ftp', 'nf_conntrack_ftp'],
+                'nftables': ['ct helper set "ftp_tcp"']
             },
-            'h323' : {
-                'driver' : ['nf_nat_h323', 'nf_conntrack_h323'],
+            'h323': {
+                'driver': ['nf_nat_h323', 'nf_conntrack_h323'],
+                'nftables': ['ct helper set "ras_udp"',
+                             'ct helper set "q931_tcp"']
             },
-            'nfs' : {
-                'nftables' : ['ct helper set "rpc_tcp"',
-                              'ct helper set "rpc_udp"']
+            'nfs': {
+                'nftables': ['ct helper set "rpc_tcp"',
+                             'ct helper set "rpc_udp"']
             },
-            'pptp' : {
-                'driver' : ['nf_nat_pptp', 'nf_conntrack_pptp'],
+            'pptp': {
+                'driver': ['nf_nat_pptp', 'nf_conntrack_pptp'],
+                'nftables': ['ct helper set "pptp_tcp"']
              },
-            'sip' : {
-                'driver' : ['nf_nat_sip', 'nf_conntrack_sip'],
+            'sip': {
+                'driver': ['nf_nat_sip', 'nf_conntrack_sip'],
+                'nftables': ['ct helper set "sip_tcp"',
+                             'ct helper set "sip_udp"']
              },
-            'sqlnet' : {
-                'nftables' : ['ct helper set "tns_tcp"']
+            'sqlnet': {
+                'nftables': ['ct helper set "tns_tcp"']
             },
-            'tftp' : {
-                'driver' : ['nf_nat_tftp', 'nf_conntrack_tftp'],
+            'tftp': {
+                'driver': ['nf_nat_tftp', 'nf_conntrack_tftp'],
+                'nftables': ['ct helper set "tftp_udp"']
              },
         }
 
diff --git a/src/conf_mode/conntrack.py b/src/conf_mode/conntrack.py
index 50089508a..75fd26588 100755
--- a/src/conf_mode/conntrack.py
+++ b/src/conf_mode/conntrack.py
@@ -39,27 +39,35 @@ nftables_ct_file = r'/run/nftables-ct.conf'
 # Every ALG (Application Layer Gateway) consists of either a Kernel Object
 # also called a Kernel Module/Driver or some rules present in iptables
 module_map = {
-    'ftp' : {
-        'ko' : ['nf_nat_ftp', 'nf_conntrack_ftp'],
+    'ftp': {
+        'ko': ['nf_nat_ftp', 'nf_conntrack_ftp'],
+        'nftables': ['ct helper set "ftp_tcp" tcp dport {21} return']
     },
-    'h323' : {
-        'ko' : ['nf_nat_h323', 'nf_conntrack_h323'],
+    'h323': {
+        'ko': ['nf_nat_h323', 'nf_conntrack_h323'],
+        'nftables': ['ct helper set "ras_udp" udp dport {1719} return',
+                     'ct helper set "q931_tcp" tcp dport {1720} return']
     },
-    'nfs' : {
-        'nftables' : ['ct helper set "rpc_tcp" tcp dport {111} return',
-                      'ct helper set "rpc_udp" udp dport {111} return']
+    'nfs': {
+        'nftables': ['ct helper set "rpc_tcp" tcp dport {111} return',
+                     'ct helper set "rpc_udp" udp dport {111} return']
     },
-    'pptp' : {
-        'ko' : ['nf_nat_pptp', 'nf_conntrack_pptp'],
+    'pptp': {
+        'ko': ['nf_nat_pptp', 'nf_conntrack_pptp'],
+        'nftables': ['ct helper set "pptp_tcp" tcp dport {1723} return'],
+        'ipv4': True
      },
-    'sip' : {
-        'ko' : ['nf_nat_sip', 'nf_conntrack_sip'],
+    'sip': {
+        'ko': ['nf_nat_sip', 'nf_conntrack_sip'],
+        'nftables': ['ct helper set "sip_tcp" tcp dport {5060,5061} return',
+                     'ct helper set "sip_udp" udp dport {5060,5061} return']
      },
-    'sqlnet' : {
-        'nftables' : ['ct helper set "tns_tcp" tcp dport {1521,1525,1536} return']
+    'sqlnet': {
+        'nftables': ['ct helper set "tns_tcp" tcp dport {1521,1525,1536} return']
     },
-    'tftp' : {
-        'ko' : ['nf_nat_tftp', 'nf_conntrack_tftp'],
+    'tftp': {
+        'ko': ['nf_nat_tftp', 'nf_conntrack_tftp'],
+        'nftables': ['ct helper set "tftp_udp" udp dport {69} return']
      },
 }
 
@@ -177,23 +185,34 @@ def generate(conntrack):
 def apply(conntrack):
     # Depending on the enable/disable state of the ALG (Application Layer Gateway)
     # modules we need to either insmod or rmmod the helpers.
+    
+    add_modules = []
+    rm_modules = []
+
     for module, module_config in module_map.items():
-        if dict_search(f'modules.{module}', conntrack) is None:
+        if dict_search_args(conntrack, 'modules', module) is None:
             if 'ko' in module_config:
-                for mod in module_config['ko']:
-                    # Only remove the module if it's loaded
-                    if os.path.exists(f'/sys/module/{mod}'):
-                        cmd(f'rmmod {mod}')
+                unloaded = [mod for mod in module_config['ko'] if os.path.exists(f'/sys/module/{mod}')]
+                rm_modules.extend(unloaded)
         else:
             if 'ko' in module_config:
-                for mod in module_config['ko']:
-                    cmd(f'modprobe {mod}')
+                add_modules.extend(module_config['ko'])
+
+    # Add modules before nftables uses them
+    if add_modules:
+        module_str = ' '.join(add_modules)
+        cmd(f'modprobe -a {module_str}')
 
     # Load new nftables ruleset
     install_result, output = rc_cmd(f'nft -f {nftables_ct_file}')
     if install_result == 1:
         raise ConfigError(f'Failed to apply configuration: {output}')
 
+    # Remove modules after nftables stops using them
+    if rm_modules:
+        module_str = ' '.join(rm_modules)
+        cmd(f'rmmod {module_str}')
+
     if process_named_running('conntrackd'):
         # Reload conntrack-sync daemon to fetch new sysctl values
         resync_conntrackd()
-- 
cgit v1.2.3