Skip to content

Conversation

pippolo84
Copy link
Member

@pippolo84 pippolo84 commented Mar 27, 2025

The strict-mode-test verifies that in Wireguard strict mode, no
unecrypted pod-to-pod traffic is leaked. To do that, it temporarily
removes all the ipcache entries related to the echo pods IP. Doing so,
the pod IP is assigned the "world" identity and since no encryption key
can be found, this leads to send unencrypted traffic is dropped as
expected.

Since v1.18, besides the pod IPs, the ipcache BPF map now stores the pod
CIDRs too. Thus, removing just the IP is not enough to have a pod
without an associated encryption key in the ipcache: the key can still
be found from the pod CIDR entry and all the traffic from the client
pods to the echo pods is encrypted.

Despite this, the test still passes (somehow accidentally), because the
reply traffic from the echo pod is now dropped. Since only pod CIDRs
from remote nodes are inserted into the ipcache, no entry is found for
the local endpoint on the remote agent, thus the egress reply traffic
from that endpoint is leaked.

To restore the test as was originally intended, the commit adds a new
version that removes the ipcache pod CIDR entries too. To do that it
relies on the cilium-dbg bpf ipcache match command, to find the pod
CIDRs with an exact match and restore them at the end of the test.

Example run: https://github.com/cilium/cilium/actions/runs/14173035345

Related: #38483
Blocked by: #38483 and #38579

@pippolo84 pippolo84 added dont-merge/preview-only Only for preview or testing, don't merge it. cilium-cli This PR contains changes related with cilium-cli labels Mar 27, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 27, 2025
@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch from b664334 to b14056f Compare March 27, 2025 21:58
@pippolo84 pippolo84 changed the title cilium-cli: Remove podCIDRs from ipcache in strict-mode-test cilium-cli: Temporarily remove podCIDRs from ipcache in strict-mode-test Mar 27, 2025
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch from b14056f to fbb5ffb Compare March 28, 2025 10:13
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch 2 times, most recently from 9ee4575 to 274c8dd Compare March 28, 2025 14:14
@pippolo84 pippolo84 added the release-note/misc This PR makes changes that have no direct user impact. label Mar 28, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 28, 2025
@pippolo84 pippolo84 added dont-merge/blocked Another PR must be merged before this one. and removed dont-merge/preview-only Only for preview or testing, don't merge it. labels Mar 28, 2025
@pippolo84 pippolo84 changed the title cilium-cli: Temporarily remove podCIDRs from ipcache in strict-mode-test cilium-cli: Add strict-mode-test v2 Mar 28, 2025
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch from 274c8dd to 494751b Compare March 29, 2025 14:52
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch from 494751b to c340d03 Compare March 31, 2025 09:03
@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch from c340d03 to 5f472a0 Compare March 31, 2025 13:41
@pippolo84
Copy link
Member Author

/ci-e2e-upgrade

@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch from 5f472a0 to 79cffd2 Compare April 1, 2025 13:20
@pippolo84 pippolo84 marked this pull request as ready for review April 1, 2025 13:21
@pippolo84 pippolo84 requested review from a team as code owners April 1, 2025 13:21
@pippolo84
Copy link
Member Author

/test

@smagnani96 smagnani96 self-requested a review April 1, 2025 13:45
Copy link
Contributor

@smagnani96 smagnani96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a couple of nits.
Thanks Fabio!

The `strict-mode-test` verifies that in Wireguard strict mode, no
unecrypted pod-to-pod traffic is leaked. To do that, it temporarily
removes all the ipcache entries related to the echo pods IP. Doing so,
the pod IP is assigned the "world" identity and since no encryption key
can be found, this leads to send unencrypted traffic is dropped as
expected.

Since v1.18, besides the pod IPs, the ipcache BPF map now stores the pod
CIDRs too.  Thus, removing just the IP is not enough to have a pod
without an associated encryption key in the ipcache: the key can still
be found from the pod CIDR entry and all the traffic from the client
pods to the echo pods is encrypted.

Despite this, the test still passes (somehow accidentally), because the
reply traffic from the echo pod is now dropped. Since only pod CIDRs
from remote nodes are inserted into the ipcache, no entry is found for
the local endpoint on the remote agent, thus the egress reply traffic
from that endpoint is leaked.

To restore the test as was originally intended, the commit adds a new
version that removes the ipcache pod CIDR entries too. To do that it
relies on the `cilium-dbg bpf ipcache match` command, to find the pod
CIDRs with an exact match and restore them at the end of the test.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/cilium-cli-strict-mode-test branch from 79cffd2 to 0cf892c Compare April 1, 2025 16:33
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 removed the dont-merge/blocked Another PR must be merged before this one. label Apr 2, 2025
@pippolo84 pippolo84 removed the request for review from nathanjsweet April 2, 2025 07:43
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 2, 2025
@julianwiedmann julianwiedmann added feature/wireguard Relates to Cilium's Wireguard feature release-note/ci This PR makes changes to the CI. and removed release-note/misc This PR makes changes that have no direct user impact. labels Apr 2, 2025
@julianwiedmann julianwiedmann added this pull request to the merge queue Apr 2, 2025
Merged via the queue into cilium:main with commit fc14c44 Apr 2, 2025
67 checks passed
@pippolo84 pippolo84 added the affects/v1.17 This issue affects v1.17 branch label Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.17 This issue affects v1.17 branch cilium-cli This PR contains changes related with cilium-cli feature/wireguard Relates to Cilium's Wireguard feature ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants