VPN-6363: Better LAN exclusions #9674
Conversation
oskirby
force-pushed
the
vpn-6363-better-lan-exclusions
branch
from
78348ea
to
ed9ee1b
Compare
oskirby
force-pushed
the
vpn-6363-better-lan-exclusions
branch
from
cb06521
to
8de0b4f
Compare
oskirby
force-pushed
the
vpn-6363-better-lan-exclusions
branch
from
99a7a82
to
07f5dc9
Compare
There was a problem hiding this comment.
I didn't review the Linux and Windows portions, as I have no context on how we do routing on those platforms. If nobody else with expertise in that area can review this, I can dig in and learn (and review those portions of the PR).
Everything else looks good - just a few questions to make sure I understand it. Thanks - this seems great!
There was a problem hiding this comment.
This approval should be considered a partial approval. I didn't review the Linux and Windows portions, as I have no context on how we do routing on those platforms. (If nobody else with expertise in that area can review this, I can dig in and learn and review those portions of the PR.)
Thanks for answering all my questions!
|
|
||
| // Permit LAN traffic through the firewall. | ||
| if (!WindowsFirewall::instance()->enableLanBypass(addresses)) { | ||
| result = false; |
There was a problem hiding this comment.
Should we be removing the exclusion route here, given if we end up here that route exist but might be blocked by the firewall?
Once the default-route-suppression rule was removed, it turns out that we really do need to setup the wireguard routing table to exclude LAN addresses. This can easily be done using the RTN_THROW route type. But then once the throw routes are in place, we can't actually reach the internal Mulvad address space, so we need to add unicast routes back too. There is a bunch of duplicated work between the throw and unicast routes though. So it still results in an ugly looking routing table.
This attempts to make up for the lack of policy-based routing on Windows by interactively updating the routing table to force non-local traffic into the VPN tunnel. The gist of the algorithm being deployed here is to dump the routing table, and then dynamically duplicate any routes which might try to leak traffic but with a lower metric to force it back into the VPN anyways.
We should flush the cloned routes in the destructor, and tidy up a couple bits of lint while we're at it
oskirby
force-pushed
the
vpn-6363-better-lan-exclusions
branch
from
07f5dc9
to
10ca2d6
Compare
Description
The way we set up LAN exclusions has begun to diverge between platforms, and I think we have some bugs in Linux. Specifically I have found that the IP routing policy of
ip rule add table main suppress_prefixlength 0has basically meant that we defer to themainrouting table for all destinations, except the default route. This kindof works, except what we really intended was to send all traffic to the wireguard interface first and then punt traffic back to the main table if it wasn't really supposed to go to the wireguard interface.To fix this, I think it's about time that we move the routing exclusion logic out of the client and make it a part of the Daemon setup - it has always kinda been this way for iOS anyways so we might as well make it the standard procedure.
Fun fact, it turns out that Android actually has an API for this that would be much better suited to setting up address-range exclusions: VpnService.Builder.excludeRoute()
Reference
JIRA Issue VPN-6363
JIRA Issue VPN-6482
JIRA Issue VPN-5865
Previous PR #9257
Checklist