only attach the ftp helper on static nat and port forwarded public ips

Author: Damans227

systemvm/debian/opt/cloud/bin/configure.py

@@ -1415,8 +1415,7 @@ def processForwardRule(self, rule):
 
     def forward_ftp_helper(self, rule):
         # Linux 6.0 no longer attaches the ftp helper after DNAT, so attach it
-        # to the public port that is forwarded to port 21. Public port 21 is
-        # already covered by the rule from CsAddress.py
+        # to the public port that is forwarded to port 21
         if rule["protocol"] != "tcp" or "any" in [rule["public_ports"], rule["internal_ports"]]:
             return
         public_start = int(rule["public_ports"].split(":")[0])
@@ -1426,8 +1425,6 @@ def forward_ftp_helper(self, rule):
         if not internal_start <= 21 <= internal_end:
             return
         public_port = public_start + 21 - internal_start
-        if public_port == 21:
-            return
         self.fw.append(["raw", "", "-A PREROUTING -d %s/32 -p tcp -m tcp --dport %s -j CT --helper ftp" %
                         (rule["public_ip"], public_port)])
 
@@ -1574,6 +1571,7 @@ def processStaticNatRule(self, rule):
             self.fw.append(["mangle", "front", "-A %s -d %s -j RETURN" %
                             (chain_name, static_route['network'])])
 
+        self.fw.append(["raw", "", "-A PREROUTING -d %s/32 -p tcp -m tcp --dport 21 -j CT --helper ftp" % rule["public_ip"]])
         self.fw.append(["nat", "front",
                         "-A PREROUTING -d %s/32 -j DNAT --to-destination %s" % (rule["public_ip"], rule["internal_ip"])])
         self.fw.append(["nat", "front",

systemvm/debian/opt/cloud/bin/cs/CsAddress.py

@@ -485,8 +485,6 @@ def fw_router(self):
                         "-A POSTROUTING " +
                         "-p udp -m udp --dport 68 -j CHECKSUM --checksum-fill"])
 
-        self.fw.append(["raw", "", "-A PREROUTING -p tcp -m tcp --dport 21 -j CT --helper ftp"])
-
         if self.get_type() in ["public"]:
             self.fw.append(["mangle", "front",
                             "-A PREROUTING " +
@@ -568,7 +566,6 @@ def fw_vpcrouter(self):
             return
 
         self.fw.append(["filter", "", "-A FORWARD -m state --state RELATED,ESTABLISHED -j ACCEPT"])
-        self.fw.append(["raw", "", "-A PREROUTING -p tcp -m tcp --dport 21 -j CT --helper ftp"])
 
         if self.get_type() in ["guest"]:
             self.fw.append(["mangle", "front", "-A PREROUTING " +

systemvm/debian/opt/cloud/bin/setup/common.sh

@@ -165,7 +165,7 @@ enable_fwding() {
 enable_passive_ftp() {
   log_it "cloud: enabling passive FTP for guest VMs"
   # Linux 6.0 removed the nf_conntrack_helper sysctl, the ftp helper is
-  # attached to port 21 traffic by a raw table rule from CsAddress.py
+  # attached by raw table rules for static nat and port forwarding
   modprobe nf_nat_ftp
 }
 

systemvm/test/TestCsAddress.py

@@ -16,8 +16,7 @@
 # under the License.
 
 import unittest
-import mock
-from cs.CsAddress import CsAddress, CsIP
+from cs.CsAddress import CsAddress
 import merge
 
 
@@ -39,21 +38,6 @@ def test_get_guest_ip(self):
     def test_get_guest_netmask(self):
         self.assertTrue(self.csaddress.get_guest_netmask() == "255.255.255.0")
 
-    @mock.patch.object(CsIP, 'list')
-    def test_ftp_helper_rule(self, _):
-        ftp_rule = ["raw", "", "-A PREROUTING -p tcp -m tcp --dport 21 -j CT --helper ftp"]
-        for is_vpc in [False, True]:
-            config = mock.MagicMock()
-            config.get_fw.return_value = []
-            config.is_vpc.return_value = is_vpc
-            config.is_routed.return_value = False
-            config.is_dhcp.return_value = False
-            ip = CsIP("eth2", config)
-            ip.setAddress({"nw_type": "public", "public_ip": "10.0.0.2"})
-            ip.fw_router()
-            ip.fw_vpcrouter()
-            self.assertIn(ftp_rule, ip.fw)
-
 
 if __name__ == '__main__':
     unittest.main()

systemvm/test/TestCsForwardingRules.py

@@ -16,6 +16,7 @@
 # under the License.
 
 import unittest
+import mock
 from configure import CsForwardingRules
 
 
@@ -36,12 +37,27 @@ def test_ftp_helper_on_port_range(self):
         self.assertEqual(self.ftp_helper_rules("tcp", "1020:1030", "20:30"),
                          [["raw", "", "-A PREROUTING -d 10.0.0.2/32 -p tcp -m tcp --dport 1021 -j CT --helper ftp"]])
 
+    def test_ftp_helper_on_port_21(self):
+        self.assertEqual(self.ftp_helper_rules("tcp", "21:21", "21:21"),
+                         [["raw", "", "-A PREROUTING -d 10.0.0.2/32 -p tcp -m tcp --dport 21 -j CT --helper ftp"]])
+
     def test_no_ftp_helper(self):
-        self.assertEqual(self.ftp_helper_rules("tcp", "21:21", "21:21"), [])
         self.assertEqual(self.ftp_helper_rules("tcp", "2121:2121", "2121:2121"), [])
         self.assertEqual(self.ftp_helper_rules("udp", "2121:2121", "21:21"), [])
         self.assertEqual(self.ftp_helper_rules("tcp", "any", "any"), [])
 
+    @mock.patch.object(CsForwardingRules, 'getStaticRoutes', return_value=[])
+    @mock.patch.object(CsForwardingRules, 'getPrivateGatewayNetworks', return_value=[])
+    @mock.patch.object(CsForwardingRules, 'getGuestIpByIp', return_value="10.1.1.1")
+    @mock.patch.object(CsForwardingRules, 'getNetworkByIp', return_value="10.1.1.0/24")
+    @mock.patch.object(CsForwardingRules, 'getDeviceByIp', return_value="eth2")
+    def test_ftp_helper_on_static_nat(self, *_):
+        forwarding = CsForwardingRules.__new__(CsForwardingRules)
+        forwarding.fw = []
+        forwarding.processStaticNatRule({"public_ip": "10.0.0.2", "internal_ip": "10.1.1.10"})
+        self.assertIn(["raw", "", "-A PREROUTING -d 10.0.0.2/32 -p tcp -m tcp --dport 21 -j CT --helper ftp"],
+                      forwarding.fw)
+
 
 if __name__ == '__main__':
     unittest.main()