[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.
• A rule given as port 0 alone (startport=0) would also be stored as 0:0, and the VR can't tell it apart from one without ports. The API now refuses 0:0 (NetworkACLServiceImpl.validateSourceStartAndEndPorts, which covers create, update and import), and says to omit the ports to match all ports. Port 0 is not a usable tcp or udp port. A range starting at 0 is still accepted.

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.

The API now refuses port 0 alone (0:0), so no new such rules can be created. This only affects a rule stored as 0:0 before this change, 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. Updating such a rule now requires clearing its ports or setting a real range. 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.
• New tests in NetworkACLServiceImplTest: 0:0 is refused, and a range starting at 0 (0:1024) is accepted. The existing "both ports null" test now stubs null explicitly, since it was relying on Mockito's default of 0. NetworkACLServiceImplTest and the other ACL tests in server pass.
• 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 (20e1dc4).
⚠️ Report is 5 commits behind head on 4.22.

Additional details and impacted files

@@            Coverage Diff             @@
##               4.22   #14364    +/-   ##
==========================================
  Coverage     18.02%   18.02%            
- Complexity    16247    16288    +41     
==========================================
  Files          5936     5937     +1     
  Lines        535823   536773   +950     
  Branches      65612    65783   +171     
==========================================
+ Hits          96572    96777   +205     
- Misses       428252   428943   +691     
- Partials      10999    11053    +54     

Flag	Coverage Δ
uitests	4.04% <ø> (-0.01%)	⬇️
unittests	19.10% <100.00%> (+<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.

[DaanHoogland]

clgtm

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✖️ suse15. SL-JID 19645

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[Damans227]

thanks @bhouse-nexthop, CLGTM. can you get it tested on a vpc router and post the rules you see for a tcp rule with no ports and a 0:0 rule?

[blueorangutan]

[SF] Trillian test result (tid-17190)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58717 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14364-t17190-kvm-ol8.zip
Smoke tests completed. 148 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_redundant_vpc_site2site_vpn	Failure	451.73	test_vpc_vpn.py

[DaanHoogland]

[SF] Trillian test result (tid-17190) Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8 Total time taken: 58717 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14364-t17190-kvm-ol8.zip Smoke tests completed. 148 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_redundant_vpc_site2site_vpn Failure 451.73 test_vpc_vpn.py

could this be related to your changes, @bhouse-nexthop ? (looks like it could)

[bhouse-nexthop]

@DaanHoogland I went through the Marvin logs for tid-17190, and I don't think this one is caused by the PR.

• What failed: the assertion at test_vpc_vpn.py:937, Failed to connect between VPCs, see VPN state as Connected. Both createVpnConnection jobs succeeded. The passive side of the redundant pair just never reported Connected within the ~60 s wait_until window.
• ACLs in play at that point: only the built-in default_allow list on both tiers, i.e. protocol all with no ports. The test's port-22 rules come after line 937, so they never got created.
• The PR's effect on those rules: none. It only changes how a tcp/udp rule with no ports or 0:0 is rendered (--dport dropped). An all rule without ports renders to the same iptables line before and after. These chains filter tier traffic anyway; the IKE/ESP between the routers' public IPs doesn't go through them.
• Rest of the file passed in the same run: the other three tests in test_vpc_vpn.py passed, including the two non-redundant site-to-site VPN tests (test_01_vpc_site2site_vpn, test_01_vpc_site2site_vpn_multiple_options).
• Seen elsewhere: test_01_redundant_vpc_site2site_vpn has failed on unrelated PRs, e.g. Backup usage: bill each size for the time it applied, and publish the metric only on change #14310 (tid-17174, earlier today, 470s), config cleanup #13884, NAS backup: automated infrastructure backup (database, configs, certificates) #12900 and the 4.22 health check [HEALTH] 4.22 Health Check, please don't merge this! #12001.

A re-run should tell us more if you'd like to be sure.

[bhouse-nexthop]

@DaanHoogland this has your and @Damans227's approvals, and the only Trillian failure was the redundant VPC VPN test (details above; it also failed on #14310). Could it be merged for 4.22.2? I understand it's due out late this week or early next.

[weizhouapache]

code lgtm