diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go index ab41ea0b7..d367eccb8 100644 --- a/wgengine/router/ifconfig_windows.go +++ b/wgengine/router/ifconfig_windows.go @@ -713,22 +713,68 @@ 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 +func getAllInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses) ([]*winipcfg.RouteData, error) { + routes4, err := getInterfaceRoutes(ifc, windows.AF_INET) + if err != nil { + return nil, err } - if !rr.IsSingleIP() { - return false + + routes6, err := getInterfaceRoutes(ifc, windows.AF_INET6) + if err != nil { + // TODO: what if v6 unavailable? + return nil, err } - for _, pfx := range pfxs { - if pfx == rr { - return true + rd := make([]*winipcfg.RouteData, 0, len(routes4)+len(routes6)) + for _, r := range routes4 { + rd = append(rd, &winipcfg.RouteData{ + Destination: r.DestinationPrefix.IPNet(), + NextHop: r.NextHop.IP(), + Metric: r.Metric, + }) + } + for _, r := range routes6 { + 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 } - return false + 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. @@ -736,42 +782,11 @@ func isSingleRouteInPrefixes(r net.IPNet, pfxs []netaddr.IPPrefix) bool { // 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 { - routes4, err := getInterfaceRoutes(ifc, windows.AF_INET) + existingRoutes, err := getAllInterfaceRoutes(ifc) if err != nil { return err } - - routes6, err := getInterfaceRoutes(ifc, windows.AF_INET6) - if err != nil { - // TODO: what if v6 unavailable? - return err - } - - got := make([]*winipcfg.RouteData, 0, len(routes4)) - 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{ - 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{ - Destination: r.DestinationPrefix.IPNet(), - NextHop: r.NextHop.IP(), - Metric: r.Metric, - }) - } + 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)) diff --git a/wgengine/router/ifconfig_windows_test.go b/wgengine/router/ifconfig_windows_test.go index 7819e9e8d..44b81c558 100644 --- a/wgengine/router/ifconfig_windows_test.go +++ b/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)) } } diff --git a/wgengine/router/router_linux_test.go b/wgengine/router/router_linux_test.go index d4a12ac3e..1aae98f1b 100644 --- a/wgengine/router/router_linux_test.go +++ b/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 diff --git a/wgengine/router/router_test.go b/wgengine/router/router_test.go new file mode 100644 index 000000000..4ce1a6e7b --- /dev/null +++ b/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 +}