bhouse-nexthop opened a new pull request, #14355:
URL: https://github.com/apache/cloudstack/pull/14355
### Description
On a VPC virtual router, rules in the egress ACL chain `ACL_OUTBOUND_ethX`
(mangle) are not kept in the order the ACL list gives them, so the chain's own
rules can end up behind the ACL's deny.
Fixes #14354
#### Why it breaks
`CsNetfilters.compare()` puts each rule of an
`ACL_INBOUND_*`/`ACL_OUTBOUND_*` chain at the chain's current rule count, i.e.
**just ahead of the last rule already in the chain**. That works for
`ACL_INBOUND_ethX`, because `CsAddress` always closes it with a `-j DROP`, so
every ACL rule lands ahead of that DROP, in order.
`ACL_OUTBOUND_ethX` has no terminal rule, so whichever rule happens to be
last is pushed behind every ACL rule:
- **Tier with a public network:** `CsAddress` adds two `front` accepts, for
224.0.0.18 (VRRP) and 225.0.0.50. The 225.0.0.50 accept ends up last, behind an
egress deny.
- 225.0.0.50 is the multicast group conntrackd uses to sync connection
state between redundant VPC routers, over the guest interface
(`CsRedundant.py`).
- The peer router's sync packets come from inside the tier's CIDR, so the
`PREROUTING ... -j ACL_OUTBOUND_ethX` jump sends them through this chain.
- With an egress deny on that tier (the `default_deny` list has one), they
are dropped, so state sync between the redundant routers breaks.
- **Private gateway:** `CsAddress` adds nothing to the chain, so it starts
empty, and the ACL's *first* rule is the one pushed to the end, behind the deny.
#### How it is fixed
`CsAddress` now closes `ACL_OUTBOUND_ethX` with `-j RETURN` on both paths
(tier and private gateway), the same way it closes `ACL_INBOUND_ethX` with `-j
DROP`. RETURN at the end of a user chain does exactly what falling off its end
did, so no traffic is treated differently. The ACL rules simply land ahead of
it, in order.
On a VR that already has a misordered chain, the next apply puts it back in
order. ACL rules are always re-inserted and the stale copies removed, and the
RETURN is appended ahead of them.
#### How to reproduce the old behaviour
1. In a VPC, create an ACL list with egress rules:
| # | Protocol | CIDR | Port | Action |
|---|---|---|---|---|
| 1 | TCP | `10.9.9.9/32` | 443 | Allow |
| 2 | UDP | `8.8.8.8/32` | 53 | Allow |
| 3 | All | `0.0.0.0/0` | – | Deny |
2. Attach it to a tier, and to a private gateway.
3. On the VR, run `iptables -t mangle -S ACL_OUTBOUND_ethX` for each
interface.
Tier, before (the 225.0.0.50 accept is behind the deny):
```
-A ACL_OUTBOUND_eth3 -d 224.0.0.18/32 -j ACCEPT
-A ACL_OUTBOUND_eth3 -d 10.9.9.9/32 -p tcp -m tcp --dport 443 -j ACCEPT
-A ACL_OUTBOUND_eth3 -d 8.8.8.8/32 -p udp -m udp --dport 53 -j ACCEPT
-A ACL_OUTBOUND_eth3 -j DROP
-A ACL_OUTBOUND_eth3 -d 225.0.0.50/32 -j ACCEPT
```
Tier, after:
```
-A ACL_OUTBOUND_eth3 -d 224.0.0.18/32 -j ACCEPT
-A ACL_OUTBOUND_eth3 -d 225.0.0.50/32 -j ACCEPT
-A ACL_OUTBOUND_eth3 -d 10.9.9.9/32 -p tcp -m tcp --dport 443 -j ACCEPT
-A ACL_OUTBOUND_eth3 -d 8.8.8.8/32 -p udp -m udp --dport 53 -j ACCEPT
-A ACL_OUTBOUND_eth3 -j DROP
-A ACL_OUTBOUND_eth3 -j RETURN
```
Private gateway, before (rule 1 is behind the deny and never matches):
```
-A ACL_OUTBOUND_eth3 -d 8.8.8.8/32 -p udp -m udp --dport 53 -j ACCEPT
-A ACL_OUTBOUND_eth3 -j DROP
-A ACL_OUTBOUND_eth3 -d 10.9.9.9/32 -p tcp -m tcp --dport 443 -j ACCEPT
```
Private gateway, after:
```
-A ACL_OUTBOUND_eth3 -d 10.9.9.9/32 -p tcp -m tcp --dport 443 -j ACCEPT
-A ACL_OUTBOUND_eth3 -d 8.8.8.8/32 -p udp -m udp --dport 53 -j ACCEPT
-A ACL_OUTBOUND_eth3 -j DROP
-A ACL_OUTBOUND_eth3 -j RETURN
```
### 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?
- The before/after chains above are real output, not hand-written:
- The ACL rules came from `AclDevice`, and were fed through the unmodified
`CsNetfilters().compare()` against real iptables in a network namespace.
- The `CsAddress` entries for the chain were reproduced as they stand
before and after this change.
- Covered for a tier and for a private gateway: a fresh apply, a second
apply (no change), and an upgrade from the misordered chain (put back in order).
- New tests in `systemvm/test/TestCsAddress.py` call `CsIP.fw_vpcrouter()`
and check that the last `ACL_OUTBOUND` entry is the RETURN, for a tier and for
a private gateway. Both fail on the current `4.22` code and pass with this
change. (Nothing in `.github/workflows` runs `systemvm/test`, so they run via
`systemvm/test/runtests.sh`.)
- `pycodestyle` and `pylint` (as in `runtests.sh`) report nothing new.
- The conntrackd impact is from reading `CsRedundant.py` and the
`PREROUTING` jump. I have not observed it on a redundant pair.
#### How did you try to break this feature and the system with this change?
- **Ingress:** `ACL_INBOUND` is unchanged and still ends with its DROP.
- **Traffic handling:** a RETURN at the end of the chain behaves exactly
like falling off its end, so traffic that matches no ACL rule is treated as
before.
- **Re-apply:** the RETURN is recognised as already present, so re-applying
doesn't add a second one.
--
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]
