Skip to content

adjust encryption bit while tracing outgoing wireguard-encrypted packets #35354

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

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Oct 11, 2024

With this PR we just set the tracing encryption bit rather than overwriting trace reasons, causing potential loss of information. Offending commit hash 8a67387, PR #35183.

NOTE: backport shouldn't be needed since the offending commit made it to pre-release only https://github.com/cilium/cilium/releases/tag/v1.17.0-pre.2.

Fix incorrect trace reason for egress packets when WireGuard is used with Host Firewall.

@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 Oct 11, 2024
@smagnani96 smagnani96 force-pushed the pr/wg-trace-restore-reason branch 2 times, most recently from 8488357 to 467b26b Compare October 11, 2024 10:34
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the pr/wg-trace-restore-reason branch from 467b26b to 606136b Compare October 23, 2024 14:46
@smagnani96 smagnani96 added the dont-merge/blocked Another PR must be merged before this one. label Oct 23, 2024
@smagnani96 smagnani96 force-pushed the pr/wg-trace-restore-reason branch from 606136b to 906365a Compare October 28, 2024 18:04
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the pr/wg-trace-restore-reason branch from 906365a to 4722e91 Compare October 30, 2024 11:27
@smagnani96 smagnani96 removed the dont-merge/blocked Another PR must be merged before this one. label Oct 30, 2024
@smagnani96 smagnani96 force-pushed the pr/wg-trace-restore-reason branch from 4722e91 to 3fd8fdd Compare October 30, 2024 11:41
@smagnani96 smagnani96 changed the title wireguard restoring trace reason when not fwd to cilium_wg0 adjust encryption bit while tracing outgoing wireguard-encrypted packets Oct 30, 2024
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 added feature/wireguard Relates to Cilium's Wireguard feature release-note/bug This PR fixes an issue in a previous release of Cilium. labels Nov 4, 2024
@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 Nov 4, 2024
@smagnani96 smagnani96 added the area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Nov 4, 2024
@smagnani96 smagnani96 marked this pull request as ready for review November 4, 2024 10:16
@smagnani96 smagnani96 requested a review from a team as a code owner November 4, 2024 10:16
This commit adjusts the tracing reason on the egress path when WireGuard
is active. In cilium#35183 we erroneously potentially override the trace reason
with Wireguard and HostFirewall enabled: while the latter could modify the
trace structure after a successful ct lookup, the WireGuard code would
simply ignore that value with either just TRACE_REASON_ENCRYPTED or
TRACE_REASON_UNKNOWN. With this commit, the WireGuard path set at
most just the TRACE_REASON_ENCRYPTED bit only after checking that the packet
is WireGuard-encrypted, preserving the previous tracing reason.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/wg-trace-restore-reason branch from 3fd8fdd to d0c3307 Compare November 6, 2024 10:20
@smagnani96
Copy link
Contributor Author

Before we start adding this check to all the deep-down NAT flows, I think it makes sense to reconsider whether our WG traffic should even go down that path. Or be excluded here.
We clearly don't care about the RevDNAT case for service replies. So it comes down to understanding the SNAT case a bit better.

As discussed offline, I just dropped the respective commits and left only the first fix.
The concern with SNAT will be tackled in a separate Issue/PR.

@smagnani96
Copy link
Contributor Author

/test

@julianwiedmann julianwiedmann self-requested a review November 7, 2024 13:27
@julianwiedmann julianwiedmann added the area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. label Nov 7, 2024
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Thank you!

@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 Nov 7, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Nov 7, 2024
Merged via the queue into cilium:main with commit fd4cbf5 Nov 7, 2024
66 checks passed
@smagnani96 smagnani96 deleted the pr/wg-trace-restore-reason branch March 18, 2025 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. 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/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants