Revert "ipsec: set interface ID different from 0" by pchaigno · Pull Request #19019 · cilium/cilium

pchaigno · 2022-03-03T14:12:07Z

This reverts pull request #18789.

Linux commit 68ac0f3810e7 ("xfrm: state and policy should fail if XFRMA_IF_ID 0") broke userspace applications by refusing xfrm states and policies with a zero XFRMA_IF_ID. Commit 735ac6a ("ipsec: set interface ID different from 0") attempted to work around this by defining a non-zero XFRMA_IF_ID.

Unfortunately, this workaround breaks IPsec connectivity between nodes. Once the XFRMA_IF_ID is set to the placeholder value (1), traffic that should be encrypted leave the node without any encryption. On GKE and self-managed clusters, that's the only noticeable impact. However, on AKS and EKS, we also have BPF logic to rewrite the outer IP address to the proper IP. This still happens despite the failure to encrypt traffic, leading to packet drops.

The traffic leaves the node unencrypted because packets don't match the xfrm policies anymore, due to the non-zero XFRMA_IF_ID.

Thus, we didn't notice this regression in the pull request introducing the workaround because 1) GKE and Jenkins tests didn't fail and 2) the EKS and AKS IPsec tests are currently disabled. This was noticed while attempting to reenable the AKS IPsec test.

This revert was tested in CI by running the whole AKS+IPsec workflow three times successfully (at https://github.com/cilium/cilium/runs/5407467510, the two failures are because I restarted too soon and the previous cluster was not deleted yet) and locally by running the connectivity tests 10 times on the same AKS cluster.

/cc @tormath1

This reverts commit 735ac6a. Linux commit 68ac0f3810e7 ("xfrm: state and policy should fail if XFRMA_IF_ID 0") broke userspace applications by refusing xfrm states and policies with a zero XFRMA_IF_ID. Commit 735ac6a ("ipsec: set interface ID different from 0") attempted to work around this by defining a non-zero XFRMA_IF_ID. Unfortunately, this workaround breaks IPsec connectivity between nodes. Once the XFRMA_IF_ID is set to the placeholder value (1), traffic that should be encrypted leave the node without any encryption. On GKE and self-managed clusters, that's the only noticeable impact. However, on AKS and EKS, we also have BPF logic to rewrite the outer IP address to the proper IP. This still happens despite the failure to encrypt traffic, leading to packet drops. The traffic leaves the node unencrypted because packets don't match the xfrm policies anymore, due to the non-zero XFRMA_IF_ID. Thus, we didn't notice this regression in the pull request introducing the workaround because 1) GKE and Jenkins tests didn't fail and 2) the EKS and AKS IPsec tests are currently disabled. This was noticed while attempting to reenable the AKS IPsec test. This revert was tested in CI by running the whole workflow three times successfully and locally by running the connectivity tests 10 times on the same cluster. Signed-off-by: Paul Chaignon <paul@cilium.io>

pchaigno · 2022-03-03T14:23:42Z

/test

Job 'Cilium-PR-K8s-1.23-kernel-net-next' hit: #18895 (92.75% similarity)

tklauser

FWIW, I think with vishvananda/netlink#727 in place the netlink library should no longer set zero XFRMA_IF_ID values in any case. The above PR was vendored into master by #18506 That PR was backported to the v1.11 branch already and is in release v1.11.2. I just noticed that you also marked the PR to be backported to v1.10, thanks!

tormath1 · 2022-03-03T16:46:45Z

Oh right, a fix was already in 1.11.2 - good to know. Sorry for the noise then !

pchaigno · 2022-03-03T17:16:15Z

k8s-1.23-kernel-net-next failed with known flake CI: K8sVerifier Runs the kernel verifier against Cilium's BPF datapath: terminating containers are not deleted after timeout #18895. This pull request doesn't affect the datapath code so it's fine to ignore that test anyway.
checkpatch is complaining because I referenced a Linux kernel commit and it can't find it in the Cilium git tree. No comment.

Other than that, the revert was extensively tested in another PR and locally as said above. The cilium/bpf review is covered by Tobias. Marking ready to merge.

This reverts commit 68ac0f3 because ID 0 was meant to be used for configuring the policy/state without matching for a specific interface (e.g., Cilium is affected, see cilium/cilium#18789 and cilium/cilium#19019). Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

This reverts commit 68ac0f3 because ID 0 was meant to be used for configuring the policy/state without matching for a specific interface (e.g., Cilium is affected, see cilium/cilium#18789 and cilium/cilium#19019). Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> Link: https://lore.kernel.org/netdev/20220309130839.3263912-3-steffen.klassert@secunet.com/ (cherry picked from commit a3d9001 https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=a3d9001b4e287fc043e5539d03d71a32ab114bcb) Bug: 221187056 Test: run_net_test.sh Change-Id: Iccaf42f776cad9166f808c490bfce586f850554c

This reverts commit 68ac0f3810e76a853b5f7b90601a05c3048b8b54 because ID 0 was meant to be used for configuring the policy/state without matching for a specific interface (e.g., Cilium is affected, see cilium/cilium#18789 and cilium/cilium#19019). Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> Link: https://lore.kernel.org/netdev/20220309130839.3263912-3-steffen.klassert@secunet.com/ (cherry picked from commit a3d9001b4e287fc043e5539d03d71a32ab114bcb https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=a3d9001b4e287fc043e5539d03d71a32ab114bcb) Bug: 221187056 Test: run_net_test.sh Change-Id: Iccaf42f776cad9166f808c490bfce586f850554c Signed-off-by: Tashfin Shakeer Rhythm <tashfinshakeerrhythm@gmail.com>

This reverts commit 68ac0f3810e76a853b5f7b90601a05c3048b8b54 because ID 0 was meant to be used for configuring the policy/state without matching for a specific interface (e.g., Cilium is affected, see cilium/cilium#18789 and cilium/cilium#19019). Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> Link: https://lore.kernel.org/netdev/20220309130839.3263912-3-steffen.klassert@secunet.com/ (cherry picked from commit a3d9001b4e287fc043e5539d03d71a32ab114bcb https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=a3d9001b4e287fc043e5539d03d71a32ab114bcb) Bug: 221187056 Test: run_net_test.sh Change-Id: Iccaf42f776cad9166f808c490bfce586f850554c

commit a3d9001 upstream. This reverts commit 68ac0f3 because ID 0 was meant to be used for configuring the policy/state without matching for a specific interface (e.g., Cilium is affected, see cilium/cilium#18789 and cilium/cilium#19019). Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

pchaigno added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/misc This PR makes changes that have no direct user impact. labels Mar 3, 2022

pchaigno requested review from a team and brb March 3, 2022 14:12

pchaigno mentioned this pull request Mar 3, 2022

ipsec: set interface ID different from 0 #18789

Merged

pchaigno force-pushed the revert-if_id-fix branch from c3b1773 to f97f112 Compare March 3, 2022 14:16

tklauser approved these changes Mar 3, 2022

View reviewed changes

pothos mentioned this pull request Mar 3, 2022

Cilium with IPSec tunneling fails to start on 3033.2.2 flatcar/Flatcar#626

Closed

maintainer-s-little-helper bot mentioned this pull request Mar 3, 2022

CI: K8sVerifier Runs the kernel verifier against Cilium's BPF datapath: terminating containers are not deleted after timeout #18895

Closed

pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 3, 2022

gandro merged commit c3d27f4 into cilium:master Mar 3, 2022

pchaigno deleted the revert-if_id-fix branch March 3, 2022 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "ipsec: set interface ID different from 0"#19019

Revert "ipsec: set interface ID different from 0"#19019
gandro merged 1 commit intocilium:masterfrom
pchaigno:revert-if_id-fix

pchaigno commented Mar 3, 2022 •

edited

Loading

Uh oh!

pchaigno commented Mar 3, 2022 •

edited by maintainer-s-little-helper bot

Loading

Uh oh!

tklauser left a comment

Uh oh!

tormath1 commented Mar 3, 2022

Uh oh!

pchaigno commented Mar 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

pchaigno commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchaigno commented Mar 3, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tklauser left a comment

Choose a reason for hiding this comment

Uh oh!

tormath1 commented Mar 3, 2022

Uh oh!

pchaigno commented Mar 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pchaigno commented Mar 3, 2022 •

edited

Loading

pchaigno commented Mar 3, 2022 •

edited by maintainer-s-little-helper bot

Loading