VPC ACL: match all ports for a tcp/udp rule given without ports

[bhouse-nexthop]

Description

On a VPC virtual router, a network ACL rule for tcp or udp with no port range matched only port 0, instead of every port.

Fixes #14363

Why it breaks

• How the rule reaches the VR: a rule without ports is sent as the range 0:0 (NetworkACLTO.getStringPortRange()). SetNetworkAclConfigItem builds a TcpAclRule/UdpAclRule, whose ports are plain ints, so the VR always receives "first_port": 0, "last_port": 0.
• On the VR: configure.py's AclRule.init_vpc() only checked that first_port was present, and rendered -m tcp --dport 0.

What that did:

• An allow without ports admitted nothing.
• A deny without ports blocked nothing, so the traffic went on to the rules after it, and was bypassed by any broader allow later in the list.

This is the iptables path: IPv4 on VPC tiers and private gateways that are not in routed mode. The nftables paths (routed IPv4 and IPv6) already treat 0 as "no ports" and match every port. So the same rule allowed all TCP on IPv6 and none on IPv4.

How it is fixed

AclRule.init_vpc() treats exactly 0:0 as "no ports" and adds no --dport, the same as the nftables paths do for 0:0.

• A real range that starts at 0, such as 0:1024, is still applied as given.
• The API accepts port 0, so a rule given as port 0 alone (startport=0) is also stored as 0:0, and the VR can't tell it apart from one without ports. Such a rule is now applied to all ports, as the IPv6 and routed paths already do. See the upgrade note.

How to reproduce the old behaviour

1. In a VPC ACL list, add an ingress rule: protocol TCP, no start/end port, CIDR 10.0.0.0/8, Allow. Attach the list to a tier.
2. On the VR, run iptables -S ACL_INBOUND_ethX:
   • Before: -A ACL_INBOUND_ethX -s 10.0.0.0/8 -p tcp -m tcp --dport 0 -j ACCEPT
   • After: -A ACL_INBOUND_ethX -s 10.0.0.0/8 -p tcp -j ACCEPT

Upgrade note

Once routers run the updated scripts (after a management server upgrade, then recreating the router or restartVPC livepatch=true), existing tcp/udp ACL rules without ports match every port of that protocol, as they always should have:

• an allow opens the protocol;
• a deny starts blocking it.

This also applies to a rule created through the API with port 0 alone, for example a "deny tcp port 0" anti-scan rule. It becomes a deny of all TCP on IPv4, as it already is on IPv6. The UI can't create such a rule; it sends an empty port for 0. To find these rules before upgrading:

SELECT id, acl_id, protocol, start_port, end_port, action, traffic_type FROM network_acl_item
WHERE protocol IN ('tcp', 'udp', '6', '17') AND state = 'Active'
  AND (start_port IS NULL OR start_port = 0) AND (end_port IS NULL OR end_port = 0);

The rows with start_port = 0 are the ones set to port 0 on purpose; the rows with no ports are the ones this PR is about.

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/TestCsAclPorts.py:
  • tcp (ingress) and udp (egress) rules with 0:0 render without --dport; that test fails on the current 4.22 code;
  • the same rule renders as all ports on IPv6 too;
  • a single port, a range, and a range starting at 0 (0:1024) are still rendered as before;
  • icmp, protocol-number and all rules are unaffected.
• The before/after rules were checked to load in real iptables.
• pycodestyle and pylint (as in systemvm/test/runtests.sh) report nothing new.
• That the API and UI can create such a rule is from reading the code (validateSourceStartAndEndPorts returns early when both ports are null, and the UI doesn't require ports); I haven't reproduced it on a running VR.

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

• Rules with ports: a single port, a range, and a range starting at 0 render exactly as before.
• Other rule types: icmp, protocol-number and all rules have no first_port and are unaffected.
• Other PRs: independent of the other open ACL PRs (VPC ACL: apply every CIDR of a multi-CIDR rule, in order #14350, Routed network ACL: match protocol-number rules with ip protocol, not ip nexthdr #14352, VPC egress ACL: end ACL_OUTBOUND with RETURN so its rules stay in order #14355, VPC ACL: keep the ports and ICMP type of rules given by protocol number or as upper-case ICMP #14357, Validate ACL, firewall and routing rule protocols against the list, not as a substring #14359, IPv6 ACL: match protocol-number rules with meta l4proto #14360), and merges cleanly with each.
• Not covered here: the nftables paths (routed IPv4, IPv6) still treat a range that starts at 0, such as 0:1024, as all ports, because they test if first_port:. The iptables path applies it as given. That is an existing, separate difference.

[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 (437adc8) to head (c14a451).

Additional details and impacted files

@@            Coverage Diff            @@
##               4.22   #14364   +/-   ##
=========================================
  Coverage     18.02%   18.02%           
  Complexity    16247    16247           
=========================================
  Files          5936     5936           
  Lines        535823   535823           
  Branches      65612    65612           
=========================================
  Hits          96572    96572           
  Misses       428252   428252           
  Partials      10999    10999           

Flag	Coverage Δ
uitests	4.04% <ø> (ø)
unittests	19.09% <ø> (ø)

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.