diff --git a/systemvm/debian/opt/cloud/bin/cs/CsAddress.py b/systemvm/debian/opt/cloud/bin/cs/CsAddress.py index 37ca8979edc7..766eb14eb0d6 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)]) @@ -614,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" % @@ -621,6 +630,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..695cc6d7ba4b 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,49 @@ 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_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) + 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 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}) + self.assertEqual(rules, [["mangle", "", "-A ACL_OUTBOUND_eth3 -j RETURN"]]) + if __name__ == '__main__': unittest.main()