bhouse-nexthop opened a new pull request, #14350:
URL: https://github.com/apache/cloudstack/pull/14350
### 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. 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`.)
- **nftables:** a multi-CIDR match is rendered as a set, `ip saddr {a,b}`,
via a small `nftAddrSet()` helper. This is the same syntax the VPC/IPv6
*firewall* code in the same file already uses for `source_cidr_list`. It is
passed through `/bin/sh` (dash), which does not brace-expand.
#### 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` syntax
error. 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)
- [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/TestCsAcl.py` covers the iptables, IPv6 and routed-IPv4
paths. All three tests fail on the current `4.22` code and pass with this
change.
- 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`, and 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.
--
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]
