From 42a675cc3b97f6e63675da1bda831c9dfba091ff Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 10:11:04 +0000 Subject: [PATCH 1/2] End ACL_OUTBOUND with RETURN so egress ACL rules stay in order CsNetfilters.compare() inserts each ACL rule ahead of the last rule in its ACL chain, which is right for ACL_INBOUND, whose last rule is its DROP. ACL_OUTBOUND (mangle) has no terminal rule, so whatever rule happens to be last is pushed behind every ACL rule: - on a tier, the 225.0.0.50 accept (conntrackd's sync multicast between redundant routers) ends up after the ACL's own deny, so with an egress deny the routers stop syncing connection state; - on a private gateway the chain is empty, so the ACL's first rule is the one pushed to the end, behind its deny. Close ACL_OUTBOUND with a RETURN the way ACL_INBOUND is closed with its DROP. RETURN is what falling off the end of the chain does already, so traffic is treated as before; the ACL rules just land ahead of it, in order. Fixes: #14354 Signed-off-by: Brad House --- systemvm/debian/opt/cloud/bin/cs/CsAddress.py | 5 ++ systemvm/test/TestCsAddress.py | 54 ++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/systemvm/debian/opt/cloud/bin/cs/CsAddress.py b/systemvm/debian/opt/cloud/bin/cs/CsAddress.py index 37ca8979edc7..6ab7c37185b4 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsAddress.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsAddress.py @@ -561,6 +561,10 @@ def fw_vpcrouter(self): ["mangle", "front", "-A ACL_OUTBOUND_%s -d 225.0.0.50/32 -j ACCEPT" % self.dev]) self.fw.append( ["mangle", "front", "-A ACL_OUTBOUND_%s -d 224.0.0.18/32 -j ACCEPT" % self.dev]) + # ACL rules are inserted ahead of the chain's last rule, so like ACL_INBOUND's DROP + # this has to stay last; RETURN is what falling off the end of the chain does anyway + self.fw.append( + ["mangle", "", "-A ACL_OUTBOUND_%s -j RETURN" % self.dev]) else: self.fw.append(["filter", "", "-A FORWARD -d %s -o %s -j ACCEPT" % (guestNetworkCidr, self.dev)]) @@ -621,6 +625,7 @@ def fw_vpcrouter(self): self.fw.append(["filter", "front", "-A FORWARD -d %s -o %s -m state --state RELATED,ESTABLISHED -j ACCEPT" % (self.address['network'], self.dev)]) self.fw.append(["filter", "", "-A ACL_INBOUND_%s -j DROP" % self.dev]) + self.fw.append(["mangle", "", "-A ACL_OUTBOUND_%s -j RETURN" % self.dev]) self.fw.append(["mangle", "", "-A PREROUTING -m state --state NEW -i %s -s %s ! -d %s/32 -j ACL_OUTBOUND_%s" % (self.dev, self.address['network'], self.address['gateway'], self.dev)]) diff --git a/systemvm/test/TestCsAddress.py b/systemvm/test/TestCsAddress.py index 0ad9ae861b91..e918e84f7bc4 100644 --- a/systemvm/test/TestCsAddress.py +++ b/systemvm/test/TestCsAddress.py @@ -16,10 +16,42 @@ # under the License. import unittest -from cs.CsAddress import CsAddress +from unittest import mock +from cs.CsAddress import CsAddress, CsIP import merge +class FakeConfig: + + def __init__(self): + self.fw = [] + self.nft_ipv4_fw = [] + self.nft_ipv4_acl = [] + + def get_fw(self): + return self.fw + + def get_nft_ipv4_fw(self): + return self.nft_ipv4_fw + + def get_nft_ipv4_acl(self): + return self.nft_ipv4_acl + + def cmdline(self): + cl = mock.Mock() + cl.get_vpccidr.return_value = "10.0.0.0/16" + return cl + + def is_vpc(self): + return True + + def is_routed(self): + return False + + def has_public_network(self): + return True + + class TestCsAddress(unittest.TestCase): def setUp(self): @@ -38,6 +70,26 @@ def test_get_guest_ip(self): def test_get_guest_netmask(self): self.assertTrue(self.csaddress.get_guest_netmask() == "255.255.255.0") + def acl_outbound_rules(self, address): + config = FakeConfig() + with mock.patch.object(CsIP, "list"): + ip = CsIP("eth3", config) + ip.setAddress(address) + ip.fw_vpcrouter() + return [fw for fw in config.fw if "-A ACL_OUTBOUND_eth3 " in fw[2]] + + def test_acl_outbound_ends_with_return_on_guest_tier(self): + # ACL rules are inserted ahead of the last rule of the chain, so that rule must be a + # terminal one, or the rule it is would end up behind the ACL rules + rules = self.acl_outbound_rules({"nw_type": "guest", "network": "10.0.1.0/24", "gateway": "10.0.1.1"}) + self.assertEqual(rules[-1], ["mangle", "", "-A ACL_OUTBOUND_eth3 -j RETURN"]) + self.assertTrue(all(fw[1] == "front" for fw in rules[:-1])) + + def test_acl_outbound_ends_with_return_on_private_gateway(self): + rules = self.acl_outbound_rules({"nw_type": "public", "is_private_gateway": True, "network": "172.16.0.0/24", + "gateway": "172.16.0.1", "public_ip": "172.16.0.10", "source_nat": False}) + self.assertEqual(rules, [["mangle", "", "-A ACL_OUTBOUND_eth3 -j RETURN"]]) + if __name__ == '__main__': unittest.main() From b621e41856ee746b594f3e9acd3ed318aa5d249e Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 11:00:16 +0000 Subject: [PATCH 2/2] Also close the ACL chains of a static route tier without public network On a VPC without a public network a tier's ACL chains get no rules of their own, ACL_INBOUND no DROP either, and are only jumped to for a static route whose gateway is in the tier. Both then had the same problem: the ACL's first rule ended up behind its deny. Close both chains with RETURN in that case; it does what falling off their end did, so traffic is treated as before. Signed-off-by: Brad House --- systemvm/debian/opt/cloud/bin/cs/CsAddress.py | 5 +++ systemvm/test/TestCsAddress.py | 31 ++++++++++++++++--- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/systemvm/debian/opt/cloud/bin/cs/CsAddress.py b/systemvm/debian/opt/cloud/bin/cs/CsAddress.py index 6ab7c37185b4..766eb14eb0d6 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsAddress.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsAddress.py @@ -618,6 +618,11 @@ def fw_vpcrouter(self): self.fw.append(["filter", "front", "-A FORWARD -d %s -o %s -m state --state RELATED,ESTABLISHED -j ACCEPT" % (static_route['network'], self.dev)]) + if self.get_type() in ["guest"] and not self.config.has_public_network(): + # Without a public network a tier's ACL chains get no last rule of their own, and are + # only jumped to from here; close them too, so the ACL rules stay in order + self.fw.append(["filter", "", "-A ACL_INBOUND_%s -j RETURN" % self.dev]) + self.fw.append(["mangle", "", "-A ACL_OUTBOUND_%s -j RETURN" % self.dev]) if self.is_private_gateway(): self.fw.append(["filter", "front", "-A FORWARD -d %s -o %s -j ACL_INBOUND_%s" % diff --git a/systemvm/test/TestCsAddress.py b/systemvm/test/TestCsAddress.py index e918e84f7bc4..695cc6d7ba4b 100644 --- a/systemvm/test/TestCsAddress.py +++ b/systemvm/test/TestCsAddress.py @@ -70,21 +70,44 @@ def test_get_guest_ip(self): def test_get_guest_netmask(self): self.assertTrue(self.csaddress.get_guest_netmask() == "255.255.255.0") - def acl_outbound_rules(self, address): + def acl_rules(self, address, chain="ACL_OUTBOUND_eth3", public_network=True, static_routes=None): config = FakeConfig() + config.has_public_network = lambda: public_network with mock.patch.object(CsIP, "list"): ip = CsIP("eth3", config) ip.setAddress(address) - ip.fw_vpcrouter() - return [fw for fw in config.fw if "-A ACL_OUTBOUND_eth3 " in fw[2]] + routes = mock.Mock() + routes.get_bag.return_value = static_routes or {} + with mock.patch("cs.CsAddress.CsStaticRoutes", return_value=routes): + ip.fw_vpcrouter() + return [fw for fw in config.fw if "-A %s " % chain in fw[2]] + + def acl_outbound_rules(self, address): + return self.acl_rules(address) def test_acl_outbound_ends_with_return_on_guest_tier(self): # ACL rules are inserted ahead of the last rule of the chain, so that rule must be a - # terminal one, or the rule it is would end up behind the ACL rules + # terminal one, or it would end up behind the ACL rules rules = self.acl_outbound_rules({"nw_type": "guest", "network": "10.0.1.0/24", "gateway": "10.0.1.1"}) self.assertEqual(rules[-1], ["mangle", "", "-A ACL_OUTBOUND_eth3 -j RETURN"]) self.assertTrue(all(fw[1] == "front" for fw in rules[:-1])) + def test_acl_chains_end_with_return_on_static_route_tier_without_public_network(self): + # such a tier's ACL chains are only jumped to for the static route, and get no last rule otherwise + address = {"nw_type": "guest", "network": "10.0.1.0/24", "gateway": "10.0.1.1", "public_ip": "10.0.1.1"} + routes = {"id": "staticroutes", "192.168.50.0/24": {"network": "192.168.50.0/24", "ip_address": "10.0.1.1", "revoke": False}} + for chain, table in (("ACL_INBOUND_eth3", "filter"), ("ACL_OUTBOUND_eth3", "mangle")): + rules = self.acl_rules(address, chain, public_network=False, static_routes=routes) + self.assertEqual(rules, [[table, "", "-A %s -j RETURN" % chain]]) + + def test_acl_chains_get_one_last_rule_on_static_route_tier_with_public_network(self): + address = {"nw_type": "guest", "network": "10.0.1.0/24", "gateway": "10.0.1.1", "public_ip": "10.0.1.1"} + routes = {"id": "staticroutes", "192.168.50.0/24": {"network": "192.168.50.0/24", "ip_address": "10.0.1.1", "revoke": False}} + inbound = self.acl_rules(address, "ACL_INBOUND_eth3", static_routes=routes) + self.assertEqual([fw[2] for fw in inbound if fw[1] == ""], ["-A ACL_INBOUND_eth3 -j DROP"]) + outbound = self.acl_rules(address, "ACL_OUTBOUND_eth3", static_routes=routes) + self.assertEqual([fw[2] for fw in outbound if fw[1] == ""], ["-A ACL_OUTBOUND_eth3 -j RETURN"]) + def test_acl_outbound_ends_with_return_on_private_gateway(self): rules = self.acl_outbound_rules({"nw_type": "public", "is_private_gateway": True, "network": "172.16.0.0/24", "gateway": "172.16.0.1", "public_ip": "172.16.0.10", "source_nat": False})