bhouse-nexthop opened a new pull request, #14364:
URL: https://github.com/apache/cloudstack/pull/14364
### 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 `int`s, 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`, matching the nftables paths.
- 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 also arrives as
`0:0`, and can't be told apart from one without ports. Port 0 isn't usable for
TCP or UDP, so nothing real is lost.
#### 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
Existing tcp/udp ACL rules without ports start matching every port of that
protocol from the next ACL apply, as they always should have:
- an allow opens the protocol;
- a deny starts blocking it.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] 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
- [x] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] 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;
- a single port, a range, and a range starting at 0 (`0:1024`) are still
rendered as before.
- 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 (#14350, #14352,
#14355, #14357, #14359, #14360), and merges cleanly with each.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
