VPC ACL: apply every CIDR of a multi-CIDR rule, in order

[bhouse-nexthop]

Description

A VPC network ACL rule can carry several CIDRs. On the VR those rules are either applied in the wrong order or not applied at all, and nothing is reported back to the UI or API.

Fixes #12668

Why it breaks

IPv4, iptables (normal VPC): AclDevice.process() turns each ACL rule into one fw entry, e.g. -A ACL_INBOUND_eth3 -s 1.2.3.4/32,2.3.4.5/32 -p tcp --dport 22 -j ACCEPT. iptables expands a comma-separated address into one rule per address, so that line creates two rules. CsNetfilters.compare() inserts each ACL rule at the chain's current rule count (just ahead of the trailing DROP), and bumps that count by one per fw entry, not by the number of rules iptables actually created. After a multi-CIDR rule the count is short by n-1, so every later rule is inserted between the expanded rules. All but the last CIDR of each multi-CIDR rule drift down behind every rule that follows, including an explicit deny.

The effect: an allow silently stops working for every CIDR but one, and a multi-CIDR deny placed ahead of an allow (e.g. "block RFC1918, then allow 0.0.0.0/0") stops denying. Single-CIDR rules are unaffected. A lone multi-CIDR rule also looks fine, since there is nothing after it to land in the gap, which is why this can be hard to reproduce. Both ingress (filter) and egress (mangle) chains are affected.

IPv6, and IPv4 in routed mode (nftables): __process_ip6() and __process_routing_ip4() build ip6 saddr a,b ... / ip saddr a,b .... nft rejects a bare comma list:

$ nft add rule ip6 t c ip6 saddr 2001:db8:1::/64,2001:db8:2::/64 tcp dport 22 accept
Error: syntax error, unexpected /, expecting end of file or newline or semicolon

The rules are added one nft add rule at a time and the failure is only logged, so each multi-CIDR rule is silently missing while the rest of the list loads.

How it is fixed

• iptables: AclDevice.process() emits one fw entry per CIDR, in list order, skipping any empty element. Each entry is then exactly one iptables rule, so the insert position compare() tracks stays correct. (This is simpler and safer than teaching compare() how iptables expands -s/-d. It also lifts a limit: a single -s with several hundred CIDRs fails outright with iptables: Message too long.)
• nftables: a multi-CIDR match is rendered as a set, ip saddr { a, b }, via a small nftAddrSet() helper. The routed-network firewall rules (add_routing_rules) and CsIpv6Firewall in the same file already use nft sets for source_cidr_list. The set is written with spaces so that no shell can brace-expand it on its way to nft. nft also accepts overlapping or duplicate elements in it and merges them.

How to reproduce the old behaviour

1. In a VPC, create a network ACL list with these ingress rules and attach it to a tier:

   #	Protocol	CIDR list	Port	Action
   1	TCP	1.2.3.4/32,2.3.4.5/32	22	Allow
   2	TCP	3.4.5.6/32	443	Allow
   3	All	0.0.0.0/0	–	Deny

2. On the VR, run iptables -S ACL_INBOUND_ethX for that tier's interface.

Before (the first CIDR of rule 1 sits behind the explicit deny and never matches):

-A ACL_INBOUND_eth3 -d 225.0.0.50/32 -j ACCEPT
-A ACL_INBOUND_eth3 -d 224.0.0.18/32 -j ACCEPT
-A ACL_INBOUND_eth3 -s 2.3.4.5/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -s 3.4.5.6/32 -p tcp -m tcp --dport 443 -j ACCEPT
-A ACL_INBOUND_eth3 -j DROP
-A ACL_INBOUND_eth3 -s 1.2.3.4/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -j DROP

After:

-A ACL_INBOUND_eth3 -d 225.0.0.50/32 -j ACCEPT
-A ACL_INBOUND_eth3 -d 224.0.0.18/32 -j ACCEPT
-A ACL_INBOUND_eth3 -s 1.2.3.4/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -s 2.3.4.5/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -s 3.4.5.6/32 -p tcp -m tcp --dport 443 -j ACCEPT
-A ACL_INBOUND_eth3 -j DROP
-A ACL_INBOUND_eth3 -j DROP

For the IPv6 side, give the tier an IPv6 CIDR and add a rule with two IPv6 CIDRs. Before the fix it is absent from nft list chain ip6 ip6_acl ethX_ingress_policy, and /var/log/cloud.log shows the nft add rule failing with a non-zero exit status (nft's syntax error itself goes to stderr). After the fix it is listed as ip6 saddr { a, b } ....

Until this lands, the workaround is one CIDR per ACL rule.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

How Has This Been Tested?

• New systemvm/test/TestCsAcl.py covers iptables ingress and egress, the IPv4 and IPv6 halves of a mixed-family rule, routed IPv4, and empty list elements. The multi-CIDR tests fail on the current 4.22 code and all pass with this change. (Nothing in .github/workflows runs systemvm/test, so these run via systemvm/test/runtests.sh.)
• The before/after chains above are real output, not hand-written. They come from feeding the rules AclDevice generates through the unmodified CsNetfilters().compare() against real iptables (nf_tables backend) in a network namespace. Re-applying the same ACL a second time leaves the chain unchanged.
• The nft set syntax was checked with real nft add rule for both ip and ip6 on nft 1.0.6 (the version in the Debian 12 systemvm template) and 1.0.9, including overlapping, duplicate and 0.0.0.0/0 / ::/0 elements. The old bare-list form was confirmed to fail.
• pycodestyle and pylint (as in systemvm/test/runtests.sh) report nothing new.

How did you try to break this feature and the system with this change?

• Mixed IPv4/IPv6 CIDR lists still split the same way as before (removeUndesiredCidrs). Each family only gets its own CIDRs, and a single-CIDR rule produces exactly the same output as before.
• Rules with no CIDR (all) are unchanged.
• Duplicate CIDRs, within one rule or across rules, are applied and cleaned up correctly on re-apply.
• Upgrade case: a VR that already has the old scrambled chain is put back in order on the next apply.

[bhouse-nexthop]

@vladimirpetrov @sureshanaparti could you take a look at this one? We'd like this fix to make it into the upcoming 4.22.2 release.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 18.02%. Comparing base (2974af8) to head (f22753f).

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #14350      +/-   ##
============================================
- Coverage     18.02%   18.02%   -0.01%     
+ Complexity    16250    16245       -5     
============================================
  Files          5936     5936              
  Lines        535823   535823              
  Branches      65612    65612              
============================================
- Hits          96582    96556      -26     
- Misses       428242   428270      +28     
+ Partials      10999    10997       -2     

Flag	Coverage Δ
uitests	4.04% <ø> (ø)
unittests	19.09% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fermosan]

Hello, this seems to do the work. Is there any chance for this to be applied to systemvm for 4.23 ? As far as i know there is no 4.23 systemvm. Maybe a 4.22 image with only this patch could make it to a 4.23?