bhouse-nexthop opened a new pull request, #14357:
URL: https://github.com/apache/cloudstack/pull/14357
### Description
A VPC network ACL rule given by protocol number (1, 6, 17), or with an
upper-case protocol name, can lose its port range or ICMP type on the virtual
router. It is then applied to the whole protocol, and nothing is reported. A
rule with ports on any other protocol number is accepted, and its ports are
ignored.
Fixes #14356
#### Why it breaks
The VR applies ports only to a rule it receives as `tcp` or `udp`, and an
ICMP type/code only to one it receives as `icmp`.
`SetNetworkACLCommand.generateFwRules()` sent the protocol exactly as stored.
`SetNetworkAclConfigItem` turns anything else into a `ProtocolAclRule`: a bare
protocol number with no port or ICMP fields. So:
1. **Protocol 6 or 17 with ports** (the UI offers ports for "Protocol
number", and the API accepts them) is applied to the whole protocol. "Protocol
6, port 22, allow" becomes `-p tcp -j ACCEPT`, i.e. all TCP.
2. **Protocol 1 set by `updateNetworkACLItem`.** `createNetworkACL` maps
protocol `1` to `icmp`, but `updateNetworkACLItem` did not. The ICMP type is
then cleared, so the VR allows all ICMP.
3. **An upper-case `ICMP`** (stored as given) fails `generateFwRules()`'s
case-sensitive `"icmp"` check. The rule is sent with the empty port range,
`0:0`, where the ICMP type and code belong, so the VR applies `--icmp-type 0/0`
(echo-reply only), whatever type was asked for.
4. **Ports with any other protocol number** (e.g. 132) are accepted by the
UI and API, and silently ignored by the VR.
#### How it is fixed
- **`SetNetworkACLCommand.generateFwRules()`** sends protocol 1, 6 and 17,
and any case, to the VR as `icmp`, `tcp` and `udp`. Their ports or ICMP type
then reach the rule the VR builds. A missing ICMP type or code is sent as `-1`
(any) instead of `null`; a rule updated to protocol 1 may have neither, and
`null` would make the VR side's `parseInt` fail. Normalising where the rule is
encoded also fixes rules already stored this way, with no DB migration.
- **`UpdateNetworkACLItemCmd.getProtocol()`** maps protocol 1 to `icmp`, the
same as `CreateNetworkACLCmd`.
- **`NetworkACLServiceImpl`** rejects ports given with a protocol number
other than 6 or 17, since they can't be applied. On update, only ports given in
that request are checked, so existing rules that already carry such ports stay
editable.
- **UI (`AclRulesTab.vue`)** only shows and sends Start/End port for TCP,
UDP, and protocol number 6 or 17.
#### Upgrade note
Existing rules affected by 1–3 will be applied as configured from the next
ACL apply:
- **Narrower:** "protocol 6, port 22" stops allowing every other TCP port.
- **Wider or narrower:** an `ICMP` rule with type "any" stops being
echo-reply only and matches all ICMP.
That is what those rules always asked for, but traffic that only got through
(or was only blocked) because of the bug will be treated differently.
#### How to reproduce the old behaviour
1. In a VPC ACL list, add an ingress rule: Protocol number `6`, start/end
port `22`, CIDR `0.0.0.0/0`, Allow. Attach the list to a tier.
2. On the VR, run `iptables -S ACL_INBOUND_ethX`:
- Before: `-A ACL_INBOUND_ethX -p tcp -j ACCEPT` (every TCP port open)
- After: `-A ACL_INBOUND_ethX -p tcp -m tcp --dport 22 -j ACCEPT`
3. `createNetworkACL protocol=ICMP icmptype=8 icmpcode=0 ...`:
- Before: `-p icmp -m icmp --icmp-type 0/0`
- After: `--icmp-type 8/0`
### 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?
- `SetNetworkACLCommandTest` checks the encoded rules:
- 6/17 with ports keep them;
- 1 keeps its ICMP type and code, or gets `-1;-1` when it has none;
- `ICMP`/`TCP` in upper case;
- other numbers and `all` pass through unchanged.
- `UpdateNetworkACLItemCmdTest`: protocol 1 becomes `icmp`, and anything
else is left as is.
- `NetworkACLServiceImplTest`: ports are accepted with tcp/udp/6/17, and
rejected with another protocol number when given (start port alone included).
- The `api`, `core` and `server` modules build with checkstyle, and the
three test classes pass (`NetworkACLServiceImplTest`: 108 tests).
- ESLint (the UI's own config) is clean for `AclRulesTab.vue`.
- I checked the before/after iptables text with real iptables; I have not
run it on a VR.
#### How did you try to break this feature and the system with this change?
- **Revoked rules** keep their existing encoding.
- **Other protocols:** `all`, other protocol numbers and lower-case names
are encoded exactly as before.
- **Existing rules with ports on another protocol number** can still be
updated (e.g. renumbered) through the UI and the API.
--
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]
