From 3606e68721d6e11e01b31acfc66e959635bf9926 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Thu, 26 Aug 2021 12:00:18 -0700
Subject: [PATCH] net/interfaces: fix default route lookup on Windows

It wasn't using the right metric. Apparently you're supposed to sum the route
metric and interface metric. Whoops.

While here, optimize a few little things too, not that this code
should be too hot.

Fixes #2707 (at least; probably dups but I'm failing to find)

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 net/interfaces/interfaces_windows.go | 74 ++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 10 deletions(-)

diff --git a/net/interfaces/interfaces_windows.go b/net/interfaces/interfaces_windows.go
index 61927b716..96d7fcc16 100644
--- a/net/interfaces/interfaces_windows.go
+++ b/net/interfaces/interfaces_windows.go
@@ -112,22 +112,36 @@ func NonTailscaleMTUs() (map[winipcfg.LUID]uint32, error) {
 	return mtus, err
 }
 
+func notTailscaleInterface(iface *winipcfg.IPAdapterAddresses) bool {
+	// TODO(bradfitz): do this without the Description method's
+	// utf16-to-string allocation. But at least we only do it for
+	// the virtual interfaces, for which there won't be many.
+	return !(iface.IfType == winipcfg.IfTypePropVirtual &&
+		iface.Description() == tsconst.WintunInterfaceDesc)
+}
+
 // NonTailscaleInterfaces returns a map of interface LUID to interface
 // for all interfaces except Tailscale tunnels.
 func NonTailscaleInterfaces() (map[winipcfg.LUID]*winipcfg.IPAdapterAddresses, error) {
-	ifs, err := winipcfg.GetAdaptersAddresses(windows.AF_UNSPEC, winipcfg.GAAFlagIncludeAllInterfaces)
+	return getInterfaces(windows.AF_UNSPEC, winipcfg.GAAFlagIncludeAllInterfaces, notTailscaleInterface)
+}
+
+// getInterfaces returns a map of interfaces keyed by their LUID for
+// all interfaces matching the provided match predicate.
+//
+// The family (AF_UNSPEC, AF_INET, or AF_INET6) and flags are passed
+// to winipcfg.GetAdaptersAddresses.
+func getInterfaces(family winipcfg.AddressFamily, flags winipcfg.GAAFlags, match func(*winipcfg.IPAdapterAddresses) bool) (map[winipcfg.LUID]*winipcfg.IPAdapterAddresses, error) {
+	ifs, err := winipcfg.GetAdaptersAddresses(family, flags)
 	if err != nil {
 		return nil, err
 	}
-
 	ret := map[winipcfg.LUID]*winipcfg.IPAdapterAddresses{}
 	for _, iface := range ifs {
-		if iface.Description() == tsconst.WintunInterfaceDesc {
-			continue
+		if match(iface) {
+			ret[iface.LUID] = iface
 		}
-		ret[iface.LUID] = iface
 	}
-
 	return ret, nil
 }
 
@@ -135,8 +149,26 @@ func NonTailscaleInterfaces() (map[winipcfg.LUID]*winipcfg.IPAdapterAddresses, e
 // default route for the given address family.
 //
 // It returns (nil, nil) if no interface is found.
+//
+// The family must be one of AF_INET or AF_INET6.
 func GetWindowsDefault(family winipcfg.AddressFamily) (*winipcfg.IPAdapterAddresses, error) {
-	ifs, err := NonTailscaleInterfaces()
+	ifs, err := getInterfaces(family, winipcfg.GAAFlagIncludeAllInterfaces, func(iface *winipcfg.IPAdapterAddresses) bool {
+		switch iface.IfType {
+		case winipcfg.IfTypeSoftwareLoopback:
+			return false
+		}
+		switch family {
+		case windows.AF_INET:
+			if iface.Flags&winipcfg.IPAAFlagIpv4Enabled == 0 {
+				return false
+			}
+		case windows.AF_INET6:
+			if iface.Flags&winipcfg.IPAAFlagIpv6Enabled == 0 {
+				return false
+			}
+		}
+		return iface.OperStatus == winipcfg.IfOperStatusUp && notTailscaleInterface(iface)
+	})
 	if err != nil {
 		return nil, err
 	}
@@ -149,12 +181,31 @@ func GetWindowsDefault(family winipcfg.AddressFamily) (*winipcfg.IPAdapterAddres
 	bestMetric := ^uint32(0)
 	var bestIface *winipcfg.IPAdapterAddresses
 	for _, route := range routes {
+		if route.DestinationPrefix.PrefixLength != 0 {
+			// Not a default route.
+			continue
+		}
 		iface := ifs[route.InterfaceLUID]
-		if route.DestinationPrefix.PrefixLength != 0 || iface == nil {
+		if iface == nil {
 			continue
 		}
-		if iface.OperStatus == winipcfg.IfOperStatusUp && route.Metric < bestMetric {
-			bestMetric = route.Metric
+
+		// Microsoft docs say:
+		//
+		// "The actual route metric used to compute the route
+		// preferences for IPv4 is the summation of the route
+		// metric offset specified in the Metric member of the
+		// MIB_IPFORWARD_ROW2 structure and the interface
+		// metric specified in this member for IPv4"
+		metric := route.Metric
+		switch family {
+		case windows.AF_INET:
+			metric += iface.Ipv4Metric
+		case windows.AF_INET6:
+			metric += iface.Ipv6Metric
+		}
+		if metric < bestMetric {
+			bestMetric = metric
 			bestIface = iface
 		}
 	}
@@ -163,6 +214,9 @@ func GetWindowsDefault(family winipcfg.AddressFamily) (*winipcfg.IPAdapterAddres
 }
 
 func DefaultRouteInterface() (string, error) {
+	// We always return the IPv4 default route.
+	// TODO(bradfitz): adjust API if/when anything cares. They could in theory differ, though,
+	// in which case we might send traffic to the wrong interface.
 	iface, err := GetWindowsDefault(windows.AF_INET)
 	if err != nil {
 		return "", err