Skip to content

wireguard trace reason encrypted #35183

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 6 commits into from
Oct 10, 2024

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Oct 2, 2024

This PR introduces the TRACE_REASON_ENCRYPT reason when tracing a WireGuard-encrypted packet, alongside the needed logic in the ingress path.
Each commit should contain a brief description of the changes applied.
The resulting output after applying this PR should look as follows:

# WireGuard with Overlay
1:<- endpoint 1756 flow 0x0 , identity 12670->unknown state unknown ifindex 0 orig-ip 0.0.0.0: 10.244.1.196 -> 10.244.2.22 EchoRequest
2:-> overlay flow 0x0 , identity 12670->7292 state new ifindex cilium_vxlan orig-ip 0.0.0.0: 10.244.1.196 -> 10.244.2.22 EchoRequest
3--> network encrypted  flow 0x9597ef3d , identity unknown->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.4:51871 -> 172.18.0.2:51871 udp
4-<- network encrypted  flow 0x9597ef3d , identity remote-node->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.2:51871 -> 172.18.0.4:51871 udp
5:<- overlay flow 0x0 , identity 7292->unknown state unknown ifindex cilium_vxlan orig-ip 0.0.0.0: 10.244.2.22 -> 10.244.1.196 EchoReply
6:-> endpoint 1756 flow 0x0 , identity 7292->12670 state reply ifindex lxcfe422d25f340 orig-ip 10.244.2.22: 10.244.2.22 -> 10.244.1.196 EchoReply

# WireGuard with Native Routing
1:<- endpoint 2379 flow 0x0 , identity 16125->unknown state unknown ifindex 0 orig-ip 0.0.0.0: 10.244.2.39 -> 10.244.1.72 EchoRequest
2:-> stack flow 0x0 , identity 16125->34030 state new ifindex 0 orig-ip 0.0.0.0: 10.244.2.39 -> 10.244.1.72 EchoRequest
3:-> 0: 10.244.2.39 -> 10.244.1.72 EchoRequest
4--> network encrypted  flow 0x0 , identity unknown->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.2:51871 -> 172.18.0.3:51871 udp
5-<- network encrypted  flow 0x0 , remote-node->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.3:51871 -> 172.18.0.2:51871 udp
6:<- host flow 0x0 , identity world->unknown state unknown ifindex cilium_wg0 orig-ip 0.0.0.0: 10.244.1.72 -> 10.244.2.39 EchoReply
7:-> endpoint 2379 flow 0x0 , identity 34030->16125 state reply ifindex lxc27ddb3574bfb orig-ip 10.244.1.72: 10.244.1.72 -> 10.244.2.39 EchoReply

Notice how lines 3-4 in Overlay and 4-5 in Native Routing have the encrypted flag.

This still does not solve all the Hubble/Cilium-dbg observability, as all the information concerning the inner packets (endpoint) are lost at the time of the tracing event. The purpose of this PR is to align the behavior with IPSec. See the linked issue below with the discussion.

Fixes: #34659

Add logic to detect and trace WireGuard encrypted ingress/egress packets.

@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 2, 2024
@smagnani96 smagnani96 changed the title Pr/wireguard reason encrypted wireguard trace reason encrypted Oct 2, 2024
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the pr/wireguard-reason-encrypted branch 2 times, most recently from 7876730 to ebb3c4b Compare October 3, 2024 17:52
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 force-pushed the pr/wireguard-reason-encrypted branch from ebb3c4b to 9f59dfd Compare October 7, 2024 15:43
@smagnani96
Copy link
Contributor Author

/ci-verifier

This commit adds to the config map, containing all the defined for the
datapath, the WG_PORT value where WireGuard is listening. This value can
then be later used to match incoming network packets destinated to WireGuard.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit introduces the logic to match WireGuard traffic, mainly for
tracing. A packet is classified as belonging to a WireGuard communication
in Cilium if all the following conditions hold true:

1. UDP packet           -- non UDP cannot be WireGuard.
2. L4 dport == WG_PORT  -- the destination is for WG_PORT, could be related
                           to a cilium-managed WireGuard communication.
3. L4 sport == dport    -- also the sender is using WG_PORT, could be
                           originated by a cilium node.
4. Identity in cluster  -- the sender is a valid node within the cluster
                           now we now for sure that is a WG-valid packet.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit refactors the tracing of the packets in the `do_netdev` function.
Before this commit, we always trace the packet with an unknown src identity.
Since few lines of code later we retrieve this value in case of an IPv4/6
packet, it makes sense to use it also while tracing the packet
so that we provide a bit more context. An example of output after applying
this patch would be as follows (no encryption, native routing):

```bash
1:<- endpoint 2223 flow 0x0 , identity 2173->unknown state unknown ifindex 0 orig-ip 0.0.0.0: 10.244.3.213 -> 10.244.1.98 EchoRequest
2:-> stack flow 0x0 , identity 2173->13606 state new ifindex 0 orig-ip 0.0.0.0: 10.244.3.213 -> 10.244.1.98 EchoRequest
3:-> 0: 10.244.3.213 -> 10.244.1.98 EchoRequest
4:<- network flow 0x0 , identity 13606->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.98 -> 10.244.3.213 EchoReply
5:<- host flow 0x0 , identity 13606->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.98 -> 10.244.3.213 EchoReply
6:-> endpoint 2223 flow 0x0 , identity 13606->2173 state reply ifindex lxcf91eb0f2f0ab orig-ip 10.244.1.98: 10.244.1.98 -> 10.244.3.213 EchoReply
```

We can now see in line 4 the source identity while logging the packet.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit moves the check `from_host` inside the ifdef that would use it,
allowing us to save unneeded complexity when using
`ENABLE_HOST_FIREWALL && !ENABLE_MASQUERADE_IPV4`.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit adds logic to trace ingress WireGuard packets with the
TRACE_REASON_ENCRYPT. The reason is to have an aligned behavior similarly
to IPSec, for which we inspect and check whether the packet is ESP and mark
it for decryption. Within WireGuard, we just need to check that it is a
valid WireGuard packet for and from a cilium-managed endpoint and trace it
accordingly without having to use ctx->mark. An example output would look
as follows:

```bash
1:<- endpoint 321 flow 0x0 , identity 41978->unknown state unknown ifindex 0 orig-ip 0.0.0.0: 10.244.2.90 -> 10.244.1.17 EchoRequest
2:-> stack flow 0x0 , identity 41978->26105 state new ifindex 0 orig-ip 0.0.0.0: 10.244.2.90 -> 10.244.1.17 EchoRequest
3:-> 0: 10.244.2.90 -> 10.244.1.17 EchoRequest
4--> network flow 0x0 , identity unknown->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.4:51871 -> 172.18.0.2:51871 udp
5-<- network encrypted  flow 0x0 , identity remote-node->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.2:51871 -> 172.18.0.4:51871 udp
6:<- host flow 0x0 , identity 26105->unknown state unknown ifindex cilium_wg0 orig-ip 0.0.0.0: 10.244.1.17 -> 10.244.2.90 EchoReply
7:-> endpoint 321 flow 0x0 , identity 26105->41978 state reply ifindex lxceef65ccc7619 orig-ip 10.244.1.17: 10.244.1.17 -> 10.244.2.90 EchoReply
```

Note how in line 5 the FROM_NETWORK packet is marked as `encrypted`.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit modifies the `trace.reason` in the egress path of a WireGuard
encrypted packet, to trace it with TRACE_REASON_ENCRYPT.
An example output would look as follows:

```bash
1:<- endpoint 321 flow 0x0 , identity 41978->unknown state unknown ifindex 0 orig-ip 0.0.0.0: 10.244.2.90 -> 10.244.1.17 EchoRequest
2:-> stack flow 0x0 , identity 41978->26105 state new ifindex 0 orig-ip 0.0.0.0: 10.244.2.90 -> 10.244.1.17 EchoRequest
3:-> 0: 10.244.2.90 -> 10.244.1.17 EchoRequest
4--> network encrypted flow 0x0 , identity unknown->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.4:51871 -> 172.18.0.2:51871 udp
5-<- network encrypted  flow 0x0 , identity remote-node->unknown state new ifindex eth0 orig-ip 0.0.0.0: 172.18.0.2:51871 -> 172.18.0.4:51871 udp
6:<- host flow 0x0 , identity 26105->unknown state unknown ifindex cilium_wg0 orig-ip 0.0.0.0: 10.244.1.17 -> 10.244.2.90 EchoReply
7:-> endpoint 321 flow 0x0 , identity 26105->41978 state reply ifindex lxceef65ccc7619 orig-ip 10.244.1.17: 10.244.1.17 -> 10.244.2.90 EchoReply
```

Note how also in line 4 the TO_NETWORK packet is marked as `encrypted`.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/wireguard-reason-encrypted branch from 9f59dfd to 785a3c9 Compare October 8, 2024 11:19
@smagnani96
Copy link
Contributor Author

/test

@smagnani96
Copy link
Contributor Author

/ci-runtime

@smagnani96
Copy link
Contributor Author

/ci-eks

@smagnani96 smagnani96 marked this pull request as ready for review October 8, 2024 14:24
@smagnani96 smagnani96 requested review from a team as code owners October 8, 2024 14:24
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Awesome! Thank you!

@jschwinger233 jschwinger233 added 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. labels Oct 10, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 10, 2024
@jschwinger233 jschwinger233 added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Oct 10, 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.

Nice work @smagnani96!

There's a tiny bit we'll need to fix-up, but I'll resolve the comment so we can merge this and fix with a follow-up PR.

@julianwiedmann julianwiedmann added this pull request to the merge queue Oct 10, 2024
Merged via the queue into cilium:main with commit 8a67387 Oct 10, 2024
73 checks passed
smagnani96 added a commit to smagnani96/cilium that referenced this pull request Oct 10, 2024
This commit introduces the control to check whether the `ipv6_hdrlen`
function returned an error or not. If positive, we stop the processing and
drop the packet with the related error.

Offending commit hash: a732cd7
Offending PR: cilium#35183

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
smagnani96 added a commit to smagnani96/cilium that referenced this pull request Oct 11, 2024
This commit introduces the control to check whether the `ipv6_hdrlen`
function returned an error or not. If positive, we stop the processing and
drop the packet with the related error.

Offending commit hash: a732cd7
Offending PR: cilium#35183

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
smagnani96 added a commit to smagnani96/cilium that referenced this pull request Oct 21, 2024
This commit introduces the control to check whether the `ipv6_hdrlen`
function returned an error or not. If positive, we stop the processing and
drop the packet with the related error.

Offending commit hash: a732cd7
Offending PR: cilium#35183

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
smagnani96 added a commit to smagnani96/cilium that referenced this pull request Oct 21, 2024
This commit introduces the control to check whether the `ipv6_hdrlen`
function returned an error or not. If positive, we stop the processing and
drop the packet with the related error.

Offending commit hash: a732cd7
Offending PR: cilium#35183

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
smagnani96 added a commit to smagnani96/cilium that referenced this pull request Oct 23, 2024
This commit introduces a check to the output of `ipv6_hdrlen`
before calling the ctx_is_wireguard` function for setting the tracing
encryption bit to WireGuard packets. In case of no errors, execute
`ctx_is_wireguard` and set the encryption bit in case of WireGuard
packets. Otherwise, continue without trying to set the encryption bit to
the trace event. We do not directly drop the packet here since tracing
does not necessarily rely on `ipv6_hdrlen`. The subsequent tail call
(`handle_ipv6`) will handle such error case.

Offending commit hash: a732cd7
Offending PR: cilium#35183

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 24, 2024
This commit introduces a check to the output of `ipv6_hdrlen`
before calling the ctx_is_wireguard` function for setting the tracing
encryption bit to WireGuard packets. In case of no errors, execute
`ctx_is_wireguard` and set the encryption bit in case of WireGuard
packets. Otherwise, continue without trying to set the encryption bit to
the trace event. We do not directly drop the packet here since tracing
does not necessarily rely on `ipv6_hdrlen`. The subsequent tail call
(`handle_ipv6`) will handle such error case.

Offending commit hash: a732cd7
Offending PR: #35183

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
smagnani96 added a commit to smagnani96/cilium that referenced this pull request Nov 6, 2024
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>
github-merge-queue bot pushed a commit that referenced this pull request Nov 7, 2024
This commit adjusts the tracing reason on the egress path when WireGuard
is active. In #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 deleted the pr/wireguard-reason-encrypted 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/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. 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/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wireguard: use TRACE_REASON_ENCRYPTED
6 participants