From e78ec2e985111dc3b1470c199f2093804a317abb Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Wed, 17 Jul 2024 19:50:06 +0200 Subject: [PATCH] Don't add exclusion routes for IPs that are part of connected networks (#2258) This prevents arp/ndp issues on macOS leading to unreachability of that IP. --- .../systemops/systemops_generic.go | 37 ++++++++- .../systemops/systemops_windows_test.go | 75 ++++++------------- 2 files changed, 59 insertions(+), 53 deletions(-) diff --git a/client/internal/routemanager/systemops/systemops_generic.go b/client/internal/routemanager/systemops/systemops_generic.go index 0d1c16ca1..615e1b528 100644 --- a/client/internal/routemanager/systemops/systemops_generic.go +++ b/client/internal/routemanager/systemops/systemops_generic.go @@ -50,7 +50,7 @@ func (r *SysOps) setupRefCounter(initAddresses []net.IP) (nbnet.AddHookFunc, nbn nexthop, err := r.addRouteToNonVPNIntf(prefix, r.wgInterface, initialNexthop) if errors.Is(err, vars.ErrRouteNotAllowed) || errors.Is(err, vars.ErrRouteNotFound) { log.Tracef("Adding for prefix %s: %v", prefix, err) - // These errors are not critical but also we should not track and try to remove the routes either. + // These errors are not critical, but also we should not track and try to remove the routes either. return nexthop, refcounter.ErrIgnore } return nexthop, err @@ -135,6 +135,11 @@ func (r *SysOps) addRouteToNonVPNIntf(prefix netip.Prefix, vpnIntf *iface.WGIfac return Nexthop{}, vars.ErrRouteNotAllowed } + // Check if the prefix is part of any local subnets + if isLocal, subnet := r.isPrefixInLocalSubnets(prefix); isLocal { + return Nexthop{}, fmt.Errorf("prefix %s is part of local subnet %s: %w", prefix, subnet, vars.ErrRouteNotAllowed) + } + // Determine the exit interface and next hop for the prefix, so we can add a specific route nexthop, err := GetNextHop(addr) if err != nil { @@ -167,6 +172,36 @@ func (r *SysOps) addRouteToNonVPNIntf(prefix netip.Prefix, vpnIntf *iface.WGIfac return exitNextHop, nil } +func (r *SysOps) isPrefixInLocalSubnets(prefix netip.Prefix) (bool, *net.IPNet) { + localInterfaces, err := net.Interfaces() + if err != nil { + log.Errorf("Failed to get local interfaces: %v", err) + return false, nil + } + + for _, intf := range localInterfaces { + addrs, err := intf.Addrs() + if err != nil { + log.Errorf("Failed to get addresses for interface %s: %v", intf.Name, err) + continue + } + + for _, addr := range addrs { + ipnet, ok := addr.(*net.IPNet) + if !ok { + log.Errorf("Failed to convert address to IPNet: %v", addr) + continue + } + + if ipnet.Contains(prefix.Addr().AsSlice()) { + return true, ipnet + } + } + } + + return false, nil +} + // genericAddVPNRoute adds a new route to the vpn interface, it splits the default prefix // in two /1 prefixes to avoid replacing the existing default route func (r *SysOps) genericAddVPNRoute(prefix netip.Prefix, intf *net.Interface) error { diff --git a/client/internal/routemanager/systemops/systemops_windows_test.go b/client/internal/routemanager/systemops/systemops_windows_test.go index 9180ed58c..19b006017 100644 --- a/client/internal/routemanager/systemops/systemops_windows_test.go +++ b/client/internal/routemanager/systemops/systemops_windows_test.go @@ -73,7 +73,7 @@ var testCases = []testCase{ { name: "To duplicate internal route without custom dialer via physical interface", // local route takes precedence destination: "10.0.0.2:53", - expectedSourceIP: "10.0.0.1", + expectedSourceIP: "127.0.0.1", expectedDestPrefix: "10.0.0.0/8", expectedNextHop: "0.0.0.0", expectedInterface: "Loopback Pseudo-Interface 1", @@ -110,7 +110,7 @@ var testCases = []testCase{ { name: "To more specific route (local) without custom dialer via physical interface", destination: "127.0.10.2:53", - expectedSourceIP: "10.0.0.1", + expectedSourceIP: "127.0.0.1", expectedDestPrefix: "127.0.0.0/8", expectedNextHop: "0.0.0.0", expectedInterface: "Loopback Pseudo-Interface 1", @@ -181,31 +181,6 @@ func testRoute(t *testing.T, destination string, dialer dialer) *FindNetRouteOut return combinedOutput } -func createAndSetupDummyInterface(t *testing.T, interfaceName, ipAddressCIDR string) string { - t.Helper() - - ip, ipNet, err := net.ParseCIDR(ipAddressCIDR) - require.NoError(t, err) - subnetMaskSize, _ := ipNet.Mask.Size() - script := fmt.Sprintf(`New-NetIPAddress -InterfaceAlias "%s" -IPAddress "%s" -PrefixLength %d -PolicyStore ActiveStore -Confirm:$False`, interfaceName, ip.String(), subnetMaskSize) - _, err = exec.Command("powershell", "-Command", script).CombinedOutput() - require.NoError(t, err, "Failed to assign IP address to loopback adapter") - - // Wait for the IP address to be applied - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) - defer cancel() - err = waitForIPAddress(ctx, interfaceName, ip.String()) - require.NoError(t, err, "IP address not applied within timeout") - - t.Cleanup(func() { - script = fmt.Sprintf(`Remove-NetIPAddress -InterfaceAlias "%s" -IPAddress "%s" -Confirm:$False`, interfaceName, ip.String()) - _, err = exec.Command("powershell", "-Command", script).CombinedOutput() - require.NoError(t, err, "Failed to remove IP address from loopback adapter") - }) - - return interfaceName -} - func fetchOriginalGateway() (*RouteInfo, error) { cmd := exec.Command("powershell", "-Command", "Get-NetRoute -DestinationPrefix 0.0.0.0/0 | Select-Object Nexthop, RouteMetric, InterfaceAlias | ConvertTo-Json") output, err := cmd.CombinedOutput() @@ -231,30 +206,6 @@ func verifyOutput(t *testing.T, output *FindNetRouteOutput, sourceIP, destPrefix assert.Equal(t, intf, output.InterfaceAlias, "Interface mismatch") } -func waitForIPAddress(ctx context.Context, interfaceAlias, expectedIPAddress string) error { - ticker := time.NewTicker(1 * time.Second) - defer ticker.Stop() - - for { - select { - case <-ctx.Done(): - return ctx.Err() - case <-ticker.C: - out, err := exec.Command("powershell", "-Command", fmt.Sprintf(`Get-NetIPAddress -InterfaceAlias "%s" | Select-Object -ExpandProperty IPAddress`, interfaceAlias)).CombinedOutput() - if err != nil { - return err - } - - ipAddresses := strings.Split(strings.TrimSpace(string(out)), "\n") - for _, ip := range ipAddresses { - if strings.TrimSpace(ip) == expectedIPAddress { - return nil - } - } - } - } -} - func combineOutputs(outputs []FindNetRouteOutput) *FindNetRouteOutput { var combined FindNetRouteOutput @@ -285,5 +236,25 @@ func combineOutputs(outputs []FindNetRouteOutput) *FindNetRouteOutput { func setupDummyInterfacesAndRoutes(t *testing.T) { t.Helper() - createAndSetupDummyInterface(t, "Loopback Pseudo-Interface 1", "10.0.0.1/8") + addDummyRoute(t, "10.0.0.0/8") +} + +func addDummyRoute(t *testing.T, dstCIDR string) { + t.Helper() + + script := fmt.Sprintf(`New-NetRoute -DestinationPrefix "%s" -InterfaceIndex 1 -PolicyStore ActiveStore`, dstCIDR) + + output, err := exec.Command("powershell", "-Command", script).CombinedOutput() + if err != nil { + t.Logf("Failed to add dummy route: %v\nOutput: %s", err, output) + t.FailNow() + } + + t.Cleanup(func() { + script = fmt.Sprintf(`Remove-NetRoute -DestinationPrefix "%s" -InterfaceIndex 1 -Confirm:$false`, dstCIDR) + output, err := exec.Command("powershell", "-Command", script).CombinedOutput() + if err != nil { + t.Logf("Failed to remove dummy route: %v\nOutput: %s", err, output) + } + }) }