From de64fa2c05a53681516d5a42257f2c6623daca66 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 00:35:51 +0000 Subject: [PATCH 1/2] Apply every CIDR of a multi-CIDR VPC ACL rule in order A VPC network ACL rule may carry several CIDRs, which reach the VR as a comma-separated string. Both ways the VR renders them are broken. iptables (the normal VPC path): the rule went out as a single "-A ACL_INBOUND_ethX -s a,b ..." line. iptables expands that into one rule per address, but CsNetfilters.compare() counts the entry as one rule when it works out where to insert the next ACL rule ahead of the chain's DROP. Every later rule is therefore inserted one slot too high, between the expanded rules, and all but the last CIDR of each multi-CIDR rule drift down behind any explicit deny. An allow silently stops matching for those CIDRs; a multi-CIDR deny placed ahead of an allow stops denying. nftables (IPv6, and IPv4 in routed mode): the rule was built as "ip6 saddr a,b ...", which nft rejects with a syntax error. The failure is only logged, so the rule is simply missing. Emit one iptables rule per CIDR so the count stays right, and render a multi-CIDR nft match as a set, "{a,b}", the way the firewall rules in the same file already do. Fixes: #12668 Signed-off-by: Brad House --- systemvm/debian/opt/cloud/bin/configure.py | 31 +++++-- systemvm/test/TestCsAcl.py | 101 +++++++++++++++++++++ 2 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 systemvm/test/TestCsAcl.py diff --git a/systemvm/debian/opt/cloud/bin/configure.py b/systemvm/debian/opt/cloud/bin/configure.py index bf48be66694c..377cf62e5c45 100755 --- a/systemvm/debian/opt/cloud/bin/configure.py +++ b/systemvm/debian/opt/cloud/bin/configure.py @@ -71,6 +71,13 @@ def removeUndesiredCidrs(cidrs, version): return None +def nftAddrSet(cidrs): + """ nft rejects a bare comma-separated address list; several addresses must be a set """ + if "," in cidrs: + return "{" + cidrs + "}" + return cidrs + + def appendStringIfNotEmpty(s1, s2): if s2: if not isinstance(s2, str): @@ -440,9 +447,9 @@ def __process_routing_ip4(self, direction, rule_list): continue addr = "" if cidr: - addr = "ip daddr " + cidr + addr = "ip daddr " + nftAddrSet(cidr) if direction == "ingress": - addr = "ip saddr " + cidr + addr = "ip saddr " + nftAddrSet(cidr) proto = "" protocol = rule['type'] @@ -518,9 +525,9 @@ def __process_ip6(self, direction, rule_list): continue addr = "" if cidr: - addr = "ip6 daddr " + cidr + addr = "ip6 daddr " + nftAddrSet(cidr) if direction == "ingress": - addr = "ip6 saddr " + cidr + addr = "ip6 saddr " + nftAddrSet(cidr) proto = "" protocol = rule['type'] @@ -583,16 +590,20 @@ def process(self, direction, rule_list, base, is_routed): count = base for i in rule_list: - ruleData = copy.copy(i) - cidr = ruleData['cidr'] + cidr = i['cidr'] if cidr is not None and cidr != "": cidr = removeUndesiredCidrs(cidr, 6) if cidr is None or cidr == "": continue - ruleData['cidr'] = cidr - r = self.AclRule(direction, self, ruleData, self.config, count) - r.create() - count += 1 + # iptables expands "-s a,b" into one rule per address, but CsNetfilters + # counts each entry as a single rule when placing the next ACL rule ahead + # of the DROP. Emit one entry per CIDR so the two stay in step. + for c in cidr.split(",") if cidr else [cidr]: + ruleData = copy.copy(i) + ruleData['cidr'] = c + r = self.AclRule(direction, self, ruleData, self.config, count) + r.create() + count += 1 # Prepare IPv6 ACL rules self.__process_ip6(direction, rule_list) diff --git a/systemvm/test/TestCsAcl.py b/systemvm/test/TestCsAcl.py new file mode 100644 index 000000000000..e8a791f3312b --- /dev/null +++ b/systemvm/test/TestCsAcl.py @@ -0,0 +1,101 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import unittest +from configure import CsAcl + + +class FakeConfig: + + def __init__(self, routed=False): + self.routed = routed + self.fw = [] + self.ipv6_acl = [] + self.nft_ipv4_acl = [] + + def get_fw(self): + return self.fw + + def get_ipv6_acl(self): + return self.ipv6_acl + + def get_nft_ipv4_acl(self): + return self.nft_ipv4_acl + + def is_vpc(self): + return True + + def is_routed(self): + return self.routed + + def get_ingress_chain(self, device, ip): + return "ACL_INBOUND_%s" % device + + def get_egress_chain(self, device, ip): + return "ACL_OUTBOUND_%s" % device + + def get_egress_table(self): + return "mangle" + + +class TestCsAcl(unittest.TestCase): + + def acl_device(self, config, ingress): + obj = {"device": "eth3", "nic_ip": "10.1.1.1", "nic_netmask": "24", "nic_ip6_cidr": "fd00:1::/64", + "ingress_rules": ingress, "egress_rules": []} + return CsAcl.AclDevice(obj, config) + + def test_multiple_cidrs_emit_one_iptables_rule_each(self): + config = FakeConfig() + acl = self.acl_device(config, [ + {"type": "tcp", "cidr": "1.2.3.4/32,2.3.4.5/32", "first_port": 22, "last_port": 22, "allowed": True}, + {"type": "all", "cidr": "0.0.0.0/0", "allowed": False}]) + acl.process("ingress", acl.ingress, acl.FIXED_RULES_INGRESS, False) + + # iptables expands "-s a,b" into several rules, which CsNetfilters would count as one + # and so insert the following rules between them, i.e. behind a later deny + rules = [fw[2] for fw in config.fw] + self.assertEqual(rules, [ + "-A ACL_INBOUND_eth3 -p tcp -s 1.2.3.4/32 -m tcp --dport 22 -j ACCEPT", + "-A ACL_INBOUND_eth3 -p tcp -s 2.3.4.5/32 -m tcp --dport 22 -j ACCEPT", + "-A ACL_INBOUND_eth3 -p all -s 0.0.0.0/0 -j DROP"]) + self.assertEqual([fw[1] for fw in config.fw], [3, 4, 5]) + + def test_multiple_ipv6_cidrs_use_nft_set(self): + config = FakeConfig() + acl = self.acl_device(config, [ + {"type": "tcp", "cidr": "2001:db8:1::/64,2001:db8:2::/64,1.2.3.4/32", "first_port": 22, "last_port": 22, + "allowed": True}]) + acl.process("ingress", acl.ingress, acl.FIXED_RULES_INGRESS, False) + + rules = [r['rule'] for r in config.ipv6_acl if r.get('chain') == "eth3_ingress_policy" and 'rule' in r] + self.assertIn("ip6 saddr {2001:db8:1::/64,2001:db8:2::/64} tcp dport 22 accept", rules) + + def test_multiple_cidrs_use_nft_set_when_routed(self): + config = FakeConfig(routed=True) + acl = self.acl_device(config, [ + {"type": "tcp", "cidr": "1.2.3.4/32,2.3.4.5/32", "first_port": 22, "last_port": 22, "allowed": True}, + {"type": "tcp", "cidr": "3.4.5.6/32", "first_port": 443, "last_port": 443, "allowed": True}]) + acl.process("ingress", acl.ingress, acl.FIXED_RULES_INGRESS, True) + + rules = [r['rule'] for r in config.nft_ipv4_acl if r.get('chain') == "eth3_ingress_policy" and 'rule' in r] + self.assertIn("ip saddr {1.2.3.4/32,2.3.4.5/32} tcp dport 22 accept", rules) + self.assertIn("ip saddr 3.4.5.6/32 tcp dport 443 accept", rules) + + +if __name__ == '__main__': + unittest.main() From f22753fb95aa8e2fb2987b4e676bc5513146dae4 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 00:46:43 +0000 Subject: [PATCH 2/2] Skip empty CIDRs and space the nft address set An empty element in the cidr list ("a,,b") would now become its own iptables rule, fail, and still be counted by CsNetfilters.compare(), pushing every later ACL rule behind the DROP. The API never produces one, but skip it rather than rely on that. Render the nft set as "{ a, b }" rather than "{a,b}": the unspaced form is only safe because /bin/sh on the VR is dash, and bash would brace-expand it into a syntax error. Also cover the egress path and the IPv4 half of a mixed-family rule in the tests. Signed-off-by: Brad House --- systemvm/debian/opt/cloud/bin/configure.py | 11 ++++--- systemvm/test/TestCsAcl.py | 34 +++++++++++++++++++--- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/systemvm/debian/opt/cloud/bin/configure.py b/systemvm/debian/opt/cloud/bin/configure.py index 377cf62e5c45..a1dd8666a630 100755 --- a/systemvm/debian/opt/cloud/bin/configure.py +++ b/systemvm/debian/opt/cloud/bin/configure.py @@ -73,9 +73,11 @@ def removeUndesiredCidrs(cidrs, version): def nftAddrSet(cidrs): """ nft rejects a bare comma-separated address list; several addresses must be a set """ - if "," in cidrs: - return "{" + cidrs + "}" - return cidrs + cidrList = [c for c in cidrs.split(",") if c] + if len(cidrList) > 1: + # spaced so that no shell brace-expands it on the way to nft + return "{ %s }" % ", ".join(cidrList) + return ",".join(cidrList) def appendStringIfNotEmpty(s1, s2): @@ -598,7 +600,8 @@ def process(self, direction, rule_list, base, is_routed): # iptables expands "-s a,b" into one rule per address, but CsNetfilters # counts each entry as a single rule when placing the next ACL rule ahead # of the DROP. Emit one entry per CIDR so the two stay in step. - for c in cidr.split(",") if cidr else [cidr]: + cidrs = [c for c in cidr.split(",") if c] if cidr else [cidr] + for c in cidrs: ruleData = copy.copy(i) ruleData['cidr'] = c r = self.AclRule(direction, self, ruleData, self.config, count) diff --git a/systemvm/test/TestCsAcl.py b/systemvm/test/TestCsAcl.py index e8a791f3312b..81cdd890251f 100644 --- a/systemvm/test/TestCsAcl.py +++ b/systemvm/test/TestCsAcl.py @@ -54,9 +54,9 @@ def get_egress_table(self): class TestCsAcl(unittest.TestCase): - def acl_device(self, config, ingress): + def acl_device(self, config, ingress, egress=None): obj = {"device": "eth3", "nic_ip": "10.1.1.1", "nic_netmask": "24", "nic_ip6_cidr": "fd00:1::/64", - "ingress_rules": ingress, "egress_rules": []} + "ingress_rules": ingress, "egress_rules": egress or []} return CsAcl.AclDevice(obj, config) def test_multiple_cidrs_emit_one_iptables_rule_each(self): @@ -75,6 +75,30 @@ def test_multiple_cidrs_emit_one_iptables_rule_each(self): "-A ACL_INBOUND_eth3 -p all -s 0.0.0.0/0 -j DROP"]) self.assertEqual([fw[1] for fw in config.fw], [3, 4, 5]) + def test_multiple_egress_cidrs_emit_one_iptables_rule_each(self): + config = FakeConfig() + acl = self.acl_device(config, [], [ + {"type": "udp", "cidr": "8.8.8.8/32,8.8.4.4/32", "first_port": 53, "last_port": 53, "allowed": True}, + {"type": "all", "cidr": "0.0.0.0/0", "allowed": False}]) + acl.process("egress", acl.egress, acl.FIXED_RULES_EGRESS, False) + + self.assertEqual(config.fw, [ + ["mangle", 3, "-A ACL_OUTBOUND_eth3 -p udp -d 8.8.8.8/32 -m udp --dport 53 -j ACCEPT"], + ["mangle", 4, "-A ACL_OUTBOUND_eth3 -p udp -d 8.8.4.4/32 -m udp --dport 53 -j ACCEPT"], + ["mangle", 5, "-A ACL_OUTBOUND_eth3 -p all -d 0.0.0.0/0 -j DROP"]]) + + def test_empty_cidr_elements_are_skipped(self): + config = FakeConfig() + acl = self.acl_device(config, [ + {"type": "tcp", "cidr": "1.2.3.4/32,,2.3.4.5/32,", "first_port": 22, "last_port": 22, "allowed": True}]) + acl.process("ingress", acl.ingress, acl.FIXED_RULES_INGRESS, False) + + self.assertEqual([fw[2] for fw in config.fw], [ + "-A ACL_INBOUND_eth3 -p tcp -s 1.2.3.4/32 -m tcp --dport 22 -j ACCEPT", + "-A ACL_INBOUND_eth3 -p tcp -s 2.3.4.5/32 -m tcp --dport 22 -j ACCEPT"]) + rules = [r['rule'] for r in config.ipv6_acl if r.get('chain') == "eth3_ingress_policy" and 'rule' in r] + self.assertNotIn("{", " ".join(rules)) + def test_multiple_ipv6_cidrs_use_nft_set(self): config = FakeConfig() acl = self.acl_device(config, [ @@ -82,8 +106,10 @@ def test_multiple_ipv6_cidrs_use_nft_set(self): "allowed": True}]) acl.process("ingress", acl.ingress, acl.FIXED_RULES_INGRESS, False) + # the IPv4 CIDR goes to iptables on its own, the IPv6 ones to nft as a set + self.assertEqual([fw[2] for fw in config.fw], ["-A ACL_INBOUND_eth3 -p tcp -s 1.2.3.4/32 -m tcp --dport 22 -j ACCEPT"]) rules = [r['rule'] for r in config.ipv6_acl if r.get('chain') == "eth3_ingress_policy" and 'rule' in r] - self.assertIn("ip6 saddr {2001:db8:1::/64,2001:db8:2::/64} tcp dport 22 accept", rules) + self.assertIn("ip6 saddr { 2001:db8:1::/64, 2001:db8:2::/64 } tcp dport 22 accept", rules) def test_multiple_cidrs_use_nft_set_when_routed(self): config = FakeConfig(routed=True) @@ -93,7 +119,7 @@ def test_multiple_cidrs_use_nft_set_when_routed(self): acl.process("ingress", acl.ingress, acl.FIXED_RULES_INGRESS, True) rules = [r['rule'] for r in config.nft_ipv4_acl if r.get('chain') == "eth3_ingress_policy" and 'rule' in r] - self.assertIn("ip saddr {1.2.3.4/32,2.3.4.5/32} tcp dport 22 accept", rules) + self.assertIn("ip saddr { 1.2.3.4/32, 2.3.4.5/32 } tcp dport 22 accept", rules) self.assertIn("ip saddr 3.4.5.6/32 tcp dport 443 accept", rules)