-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
wireguard trace reason encrypted #35183
Conversation
/test |
7876730
to
ebb3c4b
Compare
/test |
ebb3c4b
to
9f59dfd
Compare
/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>
9f59dfd
to
785a3c9
Compare
/test |
/ci-runtime |
/ci-eks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you!
There was a problem hiding this 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.
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>
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>
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>
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>
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>
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>
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>
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>
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:
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