-
Notifications
You must be signed in to change notification settings - Fork 336
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
Don't block the default route listener while calling REGISTER_IP_ADDRESSES
#4218
base: main
Are you sure you want to change the base?
Conversation
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.
Like we talked about, it might be better to have the daemon block if there is a problem with the split tunneling rather than it silently failing. The ideal would be some way to notify users about non-security critical problems without blocking the tunnel but since that would require UI changes for now blocking seems preferable.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)
talpid-core/src/split_tunnel/windows/mod.rs
line 891 at r1 (raw file):
) ); maybe_send(TunnelCommand::Block(ErrorStateCause::SplitTunnelError));
Like we discussed keeping this is probably a good idea
fbe6abb
to
08d3e10
Compare
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.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Jontified)
talpid-core/src/split_tunnel/windows/mod.rs
line 891 at r1 (raw file):
Previously, Jontified wrote…
Like we discussed keeping this is probably a good idea
Done.
08d3e10
to
81c18a5
Compare
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.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
fef72d2
to
0d8e73f
Compare
0cf2698
to
70c409d
Compare
70c409d
to
6358c5f
Compare
As above. The ioctl call can take some time to complete (mostly due to the transaction lock being blocked). If that occurs, the offline monitor will be affected as well. This PR simply queues up the calls on a different thread.
Another change that this PR makes is to simply log errors instead of forcing the tunnel state machine into the error state. There's no need to do so: failing to update the IPs cannot cause any leaks into the tunnel since the tunnel IP is static. From a security standpoint, moving to the error state is unnecessary.
Finally, also log any timeout that occurs on the split tunnel "request handling" thread.
This change is