wgengine/router{win}: ignore broadcast routes added by Windows when removing routes.

Signed-off-by: Maisem Ali <maisem@tailscale.com>

wgengine/router/ifconfig_windows.go

@@ -713,65 +713,80 @@ func getInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses, family winipcfg.Addres
 	return
 }
 
-// isSingleRouteInPrefixes reports whether r is a single-address
-// prefix that appears in pfxs.
-func isSingleRouteInPrefixes(r net.IPNet, pfxs []netaddr.IPPrefix) bool {
-	rr, ok := netaddr.FromStdIPNet(&r)
-	if !ok {
-		return false
-	}
-	if !rr.IsSingleIP() {
-		return false
-	}
-	for _, pfx := range pfxs {
-		if pfx == rr {
-			return true
-		}
-	}
-	return false
-}
-
-// syncRoutes incrementally sets multiples routes on an interface.
-// This avoids a full ifc.FlushRoutes call.
-// dontDelete is a list of interface address routes that the
-// synchronization logic should never delete.
-func syncRoutes(ifc *winipcfg.IPAdapterAddresses, want []*winipcfg.RouteData, dontDelete []netaddr.IPPrefix) error {
+func getAllInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses) ([]*winipcfg.RouteData, error) {
 	routes4, err := getInterfaceRoutes(ifc, windows.AF_INET)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	routes6, err := getInterfaceRoutes(ifc, windows.AF_INET6)
 	if err != nil {
 		// TODO: what if v6 unavailable?
-		return err
+		return nil, err
 	}
-
-	got := make([]*winipcfg.RouteData, 0, len(routes4))
+	rd := make([]*winipcfg.RouteData, 0, len(routes4)+len(routes6))
 	for _, r := range routes4 {
-		if isSingleRouteInPrefixes(r.DestinationPrefix.IPNet(), dontDelete) {
-			// See issue 1448: we don't want to touch the routes added
-			// by Windows for our interface addresses.
-			continue
-		}
-		got = append(got, &winipcfg.RouteData{
+		rd = append(rd, &winipcfg.RouteData{
 			Destination: r.DestinationPrefix.IPNet(),
 			NextHop:     r.NextHop.IP(),
 			Metric:      r.Metric,
 		})
 	}
 	for _, r := range routes6 {
-		if isSingleRouteInPrefixes(r.DestinationPrefix.IPNet(), dontDelete) {
-			// See issue 1448: we don't want to touch the routes added
-			// by Windows for our interface addresses.
-			continue
-		}
-		got = append(got, &winipcfg.RouteData{
+		rd = append(rd, &winipcfg.RouteData{
 			Destination: r.DestinationPrefix.IPNet(),
 			NextHop:     r.NextHop.IP(),
 			Metric:      r.Metric,
 		})
 	}
+	return rd, nil
+}
+
+// filterRoutes removes routes that have been added by Windows and should not
+// be managed by us.
+func filterRoutes(routes []*winipcfg.RouteData, dontDelete []netaddr.IPPrefix) []*winipcfg.RouteData {
+	ddm := make(map[netaddr.IPPrefix]bool)
+	for _, dd := range dontDelete {
+		// See issue 1448: we don't want to touch the routes added
+		// by Windows for our interface addresses.
+		ddm[dd] = true
+	}
+	for _, r := range routes {
+		// We don't want to touch broadcast routes that Windows adds.
+		nr, ok := netaddr.FromStdIPNet(&r.Destination)
+		if !ok {
+			continue
+		}
+		if nr.IsSingleIP() {
+			continue
+		}
+		lastIP := nr.Range().To
+		ddm[netaddr.IPPrefix{
+			IP:   lastIP,
+			Bits: lastIP.BitLen(),
+		}] = true
+	}
+	filtered := make([]*winipcfg.RouteData, 0, len(routes))
+	for _, r := range routes {
+		rr, ok := netaddr.FromStdIPNet(&r.Destination)
+		if ok && ddm[rr] {
+			continue
+		}
+		filtered = append(filtered, r)
+	}
+	return filtered
+}
+
+// syncRoutes incrementally sets multiples routes on an interface.
+// This avoids a full ifc.FlushRoutes call.
+// dontDelete is a list of interface address routes that the
+// synchronization logic should never delete.
+func syncRoutes(ifc *winipcfg.IPAdapterAddresses, want []*winipcfg.RouteData, dontDelete []netaddr.IPPrefix) error {
+	existingRoutes, err := getAllInterfaceRoutes(ifc)
+	if err != nil {
+		return err
+	}
+	got := filterRoutes(existingRoutes, dontDelete)
 
 	add, del := deltaRouteData(got, want)
 
@@ -783,6 +798,7 @@ func syncRoutes(ifc *winipcfg.IPAdapterAddresses, want []*winipcfg.RouteData, do
 			if dstStr == "169.254.255.255/32" {
 				// Issue 785. Ignore these routes
 				// failing to delete. Harmless.
+				// TODO(maisem): do we still need this?
 				continue
 			}
 			errs = append(errs, fmt.Errorf("deleting route %v: %w", dstStr, err))

wgengine/router/ifconfig_windows_test.go

@@ -193,6 +193,14 @@ func TestDeltaNets(t *testing.T) {
 	}
 }
 
+func formatRouteData(rds []*winipcfg.RouteData) string {
+	var b strings.Builder
+	for _, rd := range rds {
+		b.WriteString(fmt.Sprintf("%+v", rd))
+	}
+	return b.String()
+}
+
 func equalRouteDatas(a, b []*winipcfg.RouteData) bool {
 	if len(a) != len(b) {
 		return false
@@ -205,6 +213,43 @@ func equalRouteDatas(a, b []*winipcfg.RouteData) bool {
 	return true
 }
 
+func TestFilterRoutes(t *testing.T) {
+	var h0 net.IP
+	in := []*winipcfg.RouteData{
+		// LinkLocal and Loopback routes.
+		{*ipnet4("169.254.0.0", 16), h0, 1},
+		{*ipnet4("169.254.255.255", 32), h0, 1},
+		{*ipnet4("127.0.0.0", 8), h0, 1},
+		{*ipnet4("127.255.255.255", 32), h0, 1},
+		// Local LAN routes.
+		{*ipnet4("192.168.0.0", 24), h0, 1},
+		{*ipnet4("192.168.0.255", 32), h0, 1},
+		{*ipnet4("192.168.1.0", 25), h0, 1},
+		{*ipnet4("192.168.1.127", 32), h0, 1},
+		// Some random other route.
+		{*ipnet4("192.168.2.23", 32), h0, 1},
+		// Our own tailscale address.
+		{*ipnet4("100.100.100.100", 32), h0, 1},
+		// Other tailscale addresses.
+		{*ipnet4("100.100.100.101", 32), h0, 1},
+		{*ipnet4("100.100.100.102", 32), h0, 1},
+	}
+	want := []*winipcfg.RouteData{
+		{*ipnet4("169.254.0.0", 16), h0, 1},
+		{*ipnet4("127.0.0.0", 8), h0, 1},
+		{*ipnet4("192.168.0.0", 24), h0, 1},
+		{*ipnet4("192.168.1.0", 25), h0, 1},
+		{*ipnet4("192.168.2.23", 32), h0, 1},
+		{*ipnet4("100.100.100.101", 32), h0, 1},
+		{*ipnet4("100.100.100.102", 32), h0, 1},
+	}
+
+	got := filterRoutes(in, mustCIDRs("100.100.100.100/32"))
+	if !equalRouteDatas(got, want) {
+		t.Errorf("\ngot:  %v\n  want: %v\n", formatRouteData(got), formatRouteData(want))
+	}
+}
+
 func TestDeltaRouteData(t *testing.T) {
 	var h0 net.IP
 	h1 := net.ParseIP("99.99.99.99")
@@ -232,9 +277,9 @@ func TestDeltaRouteData(t *testing.T) {
 	}
 
 	if !equalRouteDatas(add, wantAdd) {
-		t.Errorf("add:\n   got: %v\n  want: %v\n", add, wantAdd)
+		t.Errorf("add:\n   got: %v\n  want: %v\n", formatRouteData(add), formatRouteData(wantAdd))
 	}
 	if !equalRouteDatas(del, wantDel) {
-		t.Errorf("del:\n   got: %v\n  want: %v\n", del, wantDel)
+		t.Errorf("del:\n   got: %v\n  want: %v\n", formatRouteData(del), formatRouteData(wantDel))
 	}
 }

wgengine/router/router_linux_test.go

@@ -20,22 +20,6 @@ import (
 	"inet.af/netaddr"
 )
 
-func mustCIDR(s string) netaddr.IPPrefix {
-	pfx, err := netaddr.ParseIPPrefix(s)
-	if err != nil {
-		panic(err)
-	}
-	return pfx
-}
-
-func mustCIDRs(ss ...string) []netaddr.IPPrefix {
-	var ret []netaddr.IPPrefix
-	for _, s := range ss {
-		ret = append(ret, mustCIDR(s))
-	}
-	return ret
-}
-
 func TestRouterStates(t *testing.T) {
 	basic := `
 ip rule add -4 pref 5210 fwmark 0x80000 table main

wgengine/router/router_test.go

@@ -0,0 +1,15 @@
+// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package router
+
+import "inet.af/netaddr"
+
+func mustCIDRs(ss ...string) []netaddr.IPPrefix {
+	var ret []netaddr.IPPrefix
+	for _, s := range ss {
+		ret = append(ret, netaddr.MustParseIPPrefix(s))
+	}
+	return ret
+}