fix(windows): prevent routing loops for TCP connections#6584
Merged
thomaseizinger merged 33 commits intomainfrom Sep 5, 2024
Merged
fix(windows): prevent routing loops for TCP connections#6584thomaseizinger merged 33 commits intomainfrom
thomaseizinger merged 33 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
4f208c4 to
2733b85
Compare
ad091a1 to
6923d70
Compare
|
| Report | Thu, September 5, 2024 at 06:02:33 UTC |
| Project | Firezone |
| Branch | test/windows-tcp-packet-loop |
| Testbed | github-actions |
🚨 1 ALERT: Threshold Boundary Limit exceeded!
| Benchmark | Measure (units) | View | Value | Lower Boundary | Upper Boundary |
|---|---|---|---|---|---|
| relayed-tcp-client2server | Throughput (bits/s) | 🚨 (view plot | view alert) | 235,923,773.80 (-5.45%) | 239,823,019.39 (101.65%) |
Click to view all benchmark results
| Benchmark | Throughput | Throughput Results bits/s | (Δ%) | Throughput Lower Boundary bits/s | (%) |
|---|---|---|---|
| direct-tcp-client2server | ✅ (view plot) | 246,138,522.82 (-0.24%) | 238,243,185.63 (96.79%) |
| direct-tcp-server2client | ✅ (view plot) | 262,640,763.25 (+4.43%) | 243,505,501.87 (92.71%) |
| direct-udp-client2server | ✅ (view plot) | 282,585,181.30 (-2.62%) | 271,288,650.24 (96.00%) |
| direct-udp-server2client | ✅ (view plot) | 416,871,445.28 (+3.74%) | 388,459,068.77 (93.18%) |
| relayed-tcp-client2server | 🚨 (view plot | view alert) | 235,923,773.80 (-5.45%) | 239,823,019.39 (101.65%) |
| relayed-tcp-server2client | ✅ (view plot) | 261,172,962.91 (+0.59%) | 249,955,441.77 (95.70%) |
| relayed-udp-client2server | ✅ (view plot) | 233,064,089.98 (+0.41%) | 220,658,728.83 (94.68%) |
| relayed-udp-server2client | ✅ (view plot) | 326,352,740.91 (-3.26%) | 316,328,576.73 (96.93%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
7d036b4 to
ad96735
Compare
0a549d4 to
f4be4bc
Compare
1cea021 to
2295fef
Compare
1cb868d to
3a3b9be
Compare
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
257fbeb to
5ba9394
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #6032, we attempted to fix routing loops for Windows and did so successfully for UDP packets. For TCP sockets, we believed that binding the socket to an interface is enough to prevent routing loops. This assumptions is wrong.
On most of our testing machines, this problem didn't surface but it turns out that on some machines, especially with WiFi cards there is a conflict between the routes added on the system. In particular, with the Internet resource active, we also add a catch-all route that we want to have the most priority, i.e. Windows SHOULD send all traffic to our TUN device. Except for traffic that we generate, like TCP connections to the portal or UDP packets sent to gateways, relays or DNS servers.
It appears that on some systems, mostly with Ethernet adapters, Windows picks the "correct" interface for our socket and sends traffic via that but on other systems, it doesn't. TCP sockets are only used for the WebSocket connection to the portal. Without that one, Firezone completely stops working because we can't send any control messages.
To reliably fix this issue, we need to add a dedicated route for the target IP of each TCP socket that is more specific than the Internet resource route (
0.0.0.0/0) but otherwise identical. We do this as part of creating a new TCP socket. This route is for the default interface and thus, doesn't get automatically removed when Firezone exits.We implement a RAII guard that attempts to drop the route on a best-effort basis. Despite this RAII guard, this route can linger around in case Firezone is being forced to exit or exits in otherwise unclean ways. To avoid lingering routes, we always delete all routing table entries matching the IP of the portal just before we are about to add one.
Fixes: #6591.