Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cilium: encryption, fixes for ENI & Azure mode with shared podIPs and networkIPs #15048

Merged
merged 9 commits into from
Feb 24, 2021

Conversation

jrfastab
Copy link
Contributor

@jrfastab jrfastab commented Feb 21, 2021

Subnet mode came with some baked in assumptions that are not met in all ENI use cases. First we assumed the networkIPs would not overlap with the podIPs. This was important because we use the same routing table for both encrypt and decrypt with the above assumption this works because encrypted packets and decrypted packets will match different routes and never collide. But, once this assumption is broke depending on how subnets and IPs are chosen its possible they collide. The result is a encrypted packet may be routed as if it has already been decrypted or vice versa. We've seen some cases where the overlap happens in EKS and AKS.

Next we made an assumption that cilium_host interface would always understand how to handle packets. In ENI mode with endpoint routes enabled we optimized cilium_host recently so it would not handle ingress packets. However, we missed the subnet encryption case combination of features. Fix it here by also enabling subnet encryption to work with endpoint routes. We took this opportunity to make a small optimization and cut out an extra pass through bpf_network as it was no longer needed with above fixes.

Finally, feature detection incorrectly detected the ingress interface in some cases.

Quick note, I broke the patches down into small chunks. Other than the refactor patches they are hopefully small and straightforward. Patch List

patches 1- 2 refactor code so we can push optional flag down to policy rules and program fwd policy directly
patch3: set optional flag on all fwd policies, we don't use fwd polices for policy (to drop packets not encrypted for example) because we can enforce these policies in the BPF programs and drop any non-compliant packets early in the stack at XDP or TC layers. Without this endpoint routes + one CIDR for network and podIPs can cause a drop.
patch4: subnetEncryption was always enabling nodeEncrypt even if it wasn't configured this is wrong.
patch5: ENI needs to use linkAddr for IPSec tunnel IPs
patch6: ENI, redirecting to cilium_host breaks endpoint routes
patch7: simplify stack to avoid extra loop by having xfrm stack clear mark bits.
patch8: remove mark from fwd rule to make it more permissive again we don't use it for policy
patch9: clear mark from xfrm states so we can skip extra pass through eth0
patch10: ENI auto iface detection doesn't in my deployment it picks the wrong interface, this makes it so we use the user supplied interface if its given.

Couple pictures to help with understanding ingress datapath.

Picture of possible failing case if subnet IPs overlap and then collide with interface IPs.

encryption notes(2)

Picture of fixed datapath to support overlapping subnet IPs.

encryption notes(1)

@jrfastab jrfastab requested a review from a team February 21, 2021 22:03
@jrfastab jrfastab requested review from a team as code owners February 21, 2021 22:03
@maintainer-s-little-helper

This comment has been minimized.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 21, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 21, 2021
@jrfastab jrfastab requested a review from jibi February 21, 2021 22:03
@jrfastab jrfastab self-assigned this Feb 21, 2021
@jrfastab jrfastab added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. needs-backport/1.9 labels Feb 21, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.5 Feb 21, 2021
@jrfastab jrfastab marked this pull request as draft February 21, 2021 22:04
@jrfastab
Copy link
Contributor Author

I'll pop patch1 off in a bit it fixes a build env issue on my system

@maintainer-s-little-helper

This comment has been minimized.

bpf/lib/encrypt.h Outdated Show resolved Hide resolved
@jrfastab
Copy link
Contributor Author

@joestringer We need to do this in two steps. First get the extra option in the netlink code. Here is the PR cilium/netlink#1

@jrfastab jrfastab force-pushed the pr/jrfastab/fix-encrypt-subnets-v2 branch from 0cfaf5a to e3734eb Compare February 23, 2021 01:12
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 23, 2021
@jrfastab jrfastab force-pushed the pr/jrfastab/fix-encrypt-subnets-v2 branch from e3734eb to 011c9b9 Compare February 23, 2021 01:15
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.9.5 Feb 25, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Needs backport from master in 1.8.8 Feb 25, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.5 Feb 25, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.5 Mar 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.5 Mar 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.8 Mar 4, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.8 Mar 4, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.5 Mar 8, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.5 Mar 8, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.8 Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.8.8
Backport done to v1.8
1.9.5
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

8 participants