Skip to content

connectivity: encryption tests: filter when icmpv6.type == 136 #38798

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 10, 2025
Merged

connectivity: encryption tests: filter when icmpv6.type == 136 #38798

merged 1 commit into from
Apr 10, 2025

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented Apr 7, 2025 •
edited
Loading

Addresses: #38688

Update: The expression not icmp6[1] = 136 does not appear to work. I tested ip6[40] = 136 and that does appear to filter by the NA msg.

Filter expression can be tested in scapy via:

send(IPv6(src="fe80::5055:55ff:fe24:430")/ICMPv6ND_NA(), loop=1, inter=1

[nix-shell:~/Code/cilium]$ sudo tcpdump -i eth0 'ip6[40] = 136'
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
listening on eth0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
09:21:25.619892 IP6 lima-vm > ip6-allnodes: ICMP6, neighbor advertisement, tgt is ::, length 24
09:21:26.620501 IP6 lima-vm > ip6-allnodes: ICMP6, neighbor advertisement, tgt is ::, length 24
09:21:27.621135 IP6 lima-vm > ip6-allnodes: ICMP6, neighbor advertisement, tgt is ::, length 24
09:21:28.621907 IP6 lima-vm > ip6-allnodes: ICMP6, neighbor advertisement, tgt is ::, length 24

@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 Apr 7, 2025
@tommyp1ckles
Copy link
Contributor Author

/test

@github-actions github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary labels Apr 7, 2025
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/skip-icmpv6-on-wg branch from ee1779a to cf4264d Compare April 8, 2025 02:44
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/skip-icmpv6-on-wg branch from cf4264d to a82ca43 Compare April 8, 2025 22:22
@tommyp1ckles tommyp1ckles changed the title [DEBUG/WIP] try avoiding icmpv6 [DEBUG/WIP] [CI] try avoiding icmpv6 msg = 136 Apr 8, 2025
@tommyp1ckles
Copy link
Contributor Author

/ci-e2e-upgrade

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/skip-icmpv6-on-wg branch from a82ca43 to 2a8e57d Compare April 8, 2025 22:41
@tommyp1ckles
Copy link
Contributor Author

/ci-e2e-upgrade

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/skip-icmpv6-on-wg branch from 2a8e57d to 9c629e9 Compare April 8, 2025 23:01
@tommyp1ckles
Copy link
Contributor Author

/ci-e2e-upgrade

@tommyp1ckles tommyp1ckles reopened this Apr 9, 2025
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles changed the title [DEBUG/WIP] [CI] try avoiding icmpv6 msg = 136 connectivity: encryption tests: filter when icmpv6 msg = 136 Apr 9, 2025
@tommyp1ckles tommyp1ckles marked this pull request as ready for review April 9, 2025 05:50
@tommyp1ckles tommyp1ckles requested a review from a team as a code owner April 9, 2025 05:50
@tommyp1ckles tommyp1ckles changed the title connectivity: encryption tests: filter when icmpv6 msg = 136 connectivity: encryption tests: filter when icmpv6.type == 136 Apr 9, 2025
@tommyp1ckles tommyp1ckles marked this pull request as draft April 9, 2025 14:56
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles marked this pull request as ready for review April 9, 2025 16:22
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/skip-icmpv6-on-wg branch from 9c629e9 to e87b149 Compare April 9, 2025 20:30
@tommyp1ckles tommyp1ckles requested review from a team as code owners April 9, 2025 20:30
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/ci This PR makes changes to the CI. labels Apr 9, 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 Apr 10, 2025
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/skip-icmpv6-on-wg branch from e87b149 to 2282626 Compare April 10, 2025 04:35
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles
cilium-cli: connectivity: filter icmpv6 neigh advertise msg.
f471bfc 
icmpv6 neighbor advertise messages are not sent to wg device [1].
These cause failures in tcpdump leak sniffing. To fix this we will
attempt to filter these out when:
* node encryption = true
* encryption = wireguard
* ipv6 = true

[1]: https://github.com/cilium/cilium/blob/d116b166d595e13e3e8772acb37a88088723d340/bpf/lib/wireguard.h#L95

Fixes: #38688

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/skip-icmpv6-on-wg branch from 2282626 to f471bfc Compare April 10, 2025 04:37
@tommyp1ckles
Copy link
Contributor Author

/test

jschwinger233
jschwinger233 approved these changes Apr 10, 2025
View reviewed changes
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

yeah this matches https://github.com/cilium/cilium/blob/v1.18.0-pre.1/bpf/lib/wireguard.h#L95

@tommyp1ckles tommyp1ckles enabled auto-merge April 10, 2025 14:17
@tommyp1ckles tommyp1ckles added this pull request to the merge queue Apr 10, 2025
Merged via the queue into main with commit f3649ae Apr 10, 2025
303 checks passed
@tommyp1ckles tommyp1ckles deleted the pr/tp/skip-icmpv6-on-wg branch April 10, 2025 14:56
@julianwiedmann
Copy link
Member

@tommyp1ckles thank you! 🚀

Do you know whether this was triggered by any specific CLI change? Wondering if we're already seeing this on stable branches and need to push an out-of-schedule CLI release, or got lucky and caught it in time.

@julianwiedmann
Copy link
Member

Seems like this still hits config 12 in e2e-upgrade, which uses native routing mode and thus builds its filter here.

@julianwiedmann
Copy link
Member

Seems like this still hits config 12 in e2e-upgrade, which uses native routing mode and thus builds its filter here.

Started to hack about in #39160.

@julianwiedmann julianwiedmann mentioned this pull request Apr 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jschwinger233 jschwinger233 jschwinger233 approved these changes

@ldelossa ldelossa Awaiting requested review from ldelossa

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tommyp1ckles @julianwiedmann @jschwinger233