Fix upgrade for IPsec with tunneling #26708
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pchaigno
added
kind/bug
pchaigno
force-pushed
the
fix-upgrade-ipsec-with-overlay
branch
2 times, most recently
from
0a4d608
to
24ae908
Compare
Since commit 3b3e8d0 ("node: Don't encrypt traffic to CiliumInternalIP"), traffic going to a CiliumInternalIP is not encrypted. In practice, that means we don't have an SPI (IPsec key reference) for the ipcache entries corresponding to CiliumInternalIPs. This commit therefore updates the IPsec BPF unit test to remove that SPI from the setup logic. Fixes: dee0683 ("bpf: test IPSec datapath on from-host") Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
On IPAM modes with one pod CIDR per node, the XFRM OUT policies look like below:
src 10.0.1.0/24 dst 10.0.0.0/24
dir out priority 0 ptype main
mark 0x66d11e00/0xffffff00
tmpl src 10.0.1.13 dst 10.0.0.214
proto esp spi 0x00000001 reqid 1 mode tunnel
When sending traffic from the hostns, however, it may not match the
source CIDR above. Traffic from the hostns may indeed leave the node
with the NodeInternalIP as the source IP (vs. CiliumInternalIP which
would match).
In such cases, we don't match the XFRM OUT policy and fall back to the
catch-all default-drop rule, ending up with XfrmOutPolBlock packet
drops.
Why wasn't this an issue before? It was. Traffic would simply go in
plain-text (which is okay given we never intended to encrypt hostns
traffic in the first place). What changes is that we now have a
catch-all default-drop XFRM OUT policy to avoid leaking plain-text
traffic. So it now results in XfrmOutPolBlock errors.
In commit 5fe2b2d ("bpf: Don't encrypt on path hostns -> remote pod")
we removed encryption for the path hostns -> remote pod. Unfortunately,
that doesn't mean the issue is completely gone. On a new Cilium install,
we won't see this issue of XfrmOutPolBlock drops for hostns traffic
anymore. But on existing clusters, we will still see those drops during
the upgrade, after the default-drop rule is installed but before hostns
traffic encryption is removed.
None of this is an issue on AKS and ENI IPAM modes because there, the
XFRM OUT policies look like:
src 0.0.0.0/0 dst 10.0.0.0/16
dir out priority 0 ptype main
mark 0x66d11e00/0xffffff00
tmpl src 10.0.1.13 dst 10.0.0.214
proto esp spi 0x00000001 reqid 1 mode tunnel
Thus, hostns -> remote pod traffic is matched regardless of the source
IP being selected and packets are not dropped by the default-drop rule.
We can therefore avoid the upgrade drops by changing the XFRM OUT
policies to never match on the source IPs, as on AKS and ENI IPAM modes.
Fixes: 7d44f37 ("ipsec: Catch-default default drop policy for encryption")
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
Commits 3b3e8d0 ("node: Don't encrypt traffic to CiliumInternalIP") and 5fe2b2d ("bpf: Don't encrypt on path hostns -> remote pod") removed a path asymmetry on the paths hostns <> remote pod. They however failed to remove workarounds that we have for this path asymmetry. In particular, we would encrypt packets on the path pod -> remote node (set SPI in the node manager) to try and avoid the path asymmetry, by also encrypting the replies. And, as a result of this first change, we would also need to handle a corner case in the datapath to appluy the correct reverse DNAT for reply traffic. All of that is unnecessary now that we fixed the path asymmetry. Fixes: 3b3e8d0 ("node: Don't encrypt traffic to CiliumInternalIP") Fixes: 5fe2b2d ("bpf: Don't encrypt on path hostns -> remote pod") Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
pchaigno
force-pushed
the
fix-upgrade-ipsec-with-overlay
branch
from
24ae908
to
09bd8c0
Compare
Commits 4c7cce1 ("bpf: Remove IP_POOLS IPsec code") and 19a62da ("bpf: Lookup tunnel endpoint for IPsec rewrite") changed the way we pass the tunnel endpoint. We used to pass it via skb->cb[4] and read it in bpf_host to build the encapsulation. We changed that in the above commits to pass the identity via skb->cb[4] instead. Therefore, on upgrades, for a short while, we may end up with bpf_lxc writing the identity into skb->cb[4] (new datapath version) and bpf_host interpreting it as the tunnel endpoint (old version). Reloading bpf_host before bpf_lxc is not enough to fix it because then, for a short while, bpf_lxc would write the tunnel endpoint in skb->cb[4] (old version) and bpf_host would interpret it as the security identity (new version). In addition to reloading bpf_host first, we also need to make sure that it can handle both cases (skb->cb[4] has tunnel endpoint or identity). To distinguish between those two cases and interpret skb->cb[4] correctly, bpf_host will rely on the first byte: in the case of the tunnel endpoint is can't be zero because that would mean the IP address is within the special purpose range 0.0.0.0/8; in the case of the identity, it has to be zero because identities are on 24-bits. This commit implements those changes. Commit ca9c056 ("daemon: Reload bpf_host first in case of IPsec upgrade") had already made the agent reload bpf_host first for ENI and Azure IPAM modes, so we just need to extend it to all IPAM modes. Note that the above bug on upgrades doesn't cause an immediate packet drop at the sender. Instead, it seems the packet is encrypted twice. The (unverified) assumption here is that the encapsulation is skipped because the tunnel endpoint IP address is invalid (being a security identity on 24-bits). The encrypted packet is then sent again to cilium_host where the encryption bit is reapplied (given the destination IP address is a CiliumInternalIP). And it goes through the XFRM encryption again. On the receiver's side, the packet is decrypted once as expected. It is then recirculated to bpf_overlay which removes the packet mark. Given it is still an ESP (encrypted) packet, it goes back through the XFRM decryption layer. But since the packet mark is now zero, it fails to match any XFRM IN state. The packet is dropped with XfrmInNoStates. This can be seen in the following trace: <- overlay encrypted flow 0x6fc46fc4 , identity unknown->unknown state new ifindex cilium_vxlan orig-ip 0.0.0.0: 10.0.9.91 -> 10.0.8.32 -> stack encrypted flow 0x6fc46fc4 , identity 134400->44 state new ifindex cilium_vxlan orig-ip 0.0.0.0: 10.0.9.91 -> 10.0.8.32 <- overlay encrypted flow 0x6fc46fc4 , identity unknown->unknown state new ifindex cilium_vxlan orig-ip 0.0.0.0: 10.0.9.91 -> 10.0.8.32 -> host from flow 0x6fc46fc4 , identity unknown->43 state unknown ifindex cilium_vxlan orig-ip 0.0.0.0: 10.0.9.91 -> 10.0.8.32 -> stack flow 0x6fc46fc4 , identity unknown->unknown state unknown ifindex cilium_host orig-ip 0.0.0.0: 10.0.9.91 -> 10.0.8.32 The packet comes from the overlay encrypted, is sent to the stack to be decrypted, and comes back still encrypted. Fixes: 4c7cce1 ("bpf: Remove IP_POOLS IPsec code") Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
pchaigno
force-pushed
the
fix-upgrade-ipsec-with-overlay
branch
3 times, most recently
from
e5a56ae
to
2895ea3
Compare
|
/test |
pchaigno
added
backport-done/1.13
maintainer-s-little-helper
bot
moved this from Backport pending to v1.13
to Backport done to v1.13
in 1.13.5
This was referenced Jul 17, 2023
maintainer-s-little-helper
bot
moved this from Needs backport from main
to Backport pending to v1.11
in 1.11.19
maintainer-s-little-helper
bot
moved this from Needs backport from main
to Backport pending to v1.11
in 1.11.19
pchaigno
removed
the
backport/author
gandro
added
backport-pending/1.14
maintainer-s-little-helper
bot
moved this from Needs backport from main
to Backport pending to v1.14
in 1.14.0
gandro
added
backport-done/1.14
maintainer-s-little-helper
bot
moved this from Backport pending to v1.14
to Backport done to v1.14
in 1.14.0
julianwiedmann
added
backport-done/1.11
maintainer-s-little-helper
bot
moved this from Backport pending to v1.12
to Backport done to v1.12
in 1.12.12
maintainer-s-little-helper
bot
moved this from Backport pending to v1.11
to Backport done to v1.11
in 1.11.19
This was referenced Jul 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix various packet drops impacting upgrades when running with IPsec and overlay mode. See commits for details.