diff --git a/overlay/route.go b/overlay/route.go index c6403f91..6d32c54b 100644 --- a/overlay/route.go +++ b/overlay/route.go @@ -50,7 +50,13 @@ func (r Route) String() string { func makeRouteTree(l *slog.Logger, routes []Route, allowMTU bool) (*bart.Table[routing.Gateways], error) { routeTree := new(bart.Table[routing.Gateways]) + seen := make(map[netip.Prefix]bool, len(routes)) for _, r := range routes { + if seen[r.Cidr] { + l.Warn("route is listed more than once, only the last entry's via is used", "route", r) + } + seen[r.Cidr] = true + if !allowMTU && r.MTU > 0 { l.Warn("route MTU is not supported on this platform", "goos", runtime.GOOS, @@ -98,7 +104,11 @@ func parseRoutes(c *config.C, networks []netip.Prefix) ([]Route, error) { mtu, ok := rMtu.(int) if !ok { - mtu, err = strconv.Atoi(rMtu.(string)) + s, ok := rMtu.(string) + if !ok { + return nil, fmt.Errorf("entry %v.mtu in tun.routes is not an integer: %v", i+1, rMtu) + } + mtu, err = strconv.Atoi(s) if err != nil { return nil, fmt.Errorf("entry %v.mtu in tun.routes is not an integer: %v", i+1, err) } @@ -174,7 +184,11 @@ func parseUnsafeRoutes(c *config.C, networks []netip.Prefix) ([]Route, error) { if rMtu, ok := m["mtu"]; ok { mtu, ok = rMtu.(int) if !ok { - mtu, err = strconv.Atoi(rMtu.(string)) + s, ok := rMtu.(string) + if !ok { + return nil, fmt.Errorf("entry %v.mtu in tun.unsafe_routes is not an integer: %v", i+1, rMtu) + } + mtu, err = strconv.Atoi(s) if err != nil { return nil, fmt.Errorf("entry %v.mtu in tun.unsafe_routes is not an integer: %v", i+1, err) } @@ -192,10 +206,15 @@ func parseUnsafeRoutes(c *config.C, networks []netip.Prefix) ([]Route, error) { metric, ok := rMetric.(int) if !ok { - _, err = strconv.ParseInt(rMetric.(string), 10, 32) + s, ok := rMetric.(string) + if !ok { + return nil, fmt.Errorf("entry %v.metric in tun.unsafe_routes is not an integer: %v", i+1, rMetric) + } + n, err := strconv.ParseInt(s, 10, 32) if err != nil { return nil, fmt.Errorf("entry %v.metric in tun.unsafe_routes is not an integer: %v", i+1, err) } + metric = int(n) } if metric < 0 || metric > math.MaxInt32 { @@ -248,10 +267,15 @@ func parseUnsafeRoutes(c *config.C, networks []netip.Prefix) ([]Route, error) { gatewayWeight, ok := rGatewayWeight.(int) if !ok { - _, err = strconv.ParseInt(rGatewayWeight.(string), 10, 32) + s, ok := rGatewayWeight.(string) + if !ok { + return nil, fmt.Errorf("entry .weight in tun.unsafe_routes[%v].via[%v] is not an integer", i+1, ig+1) + } + w, err := strconv.ParseInt(s, 10, 32) if err != nil { return nil, fmt.Errorf("entry .weight in tun.unsafe_routes[%v].via[%v] is not an integer", i+1, ig+1) } + gatewayWeight = int(w) } if gatewayWeight < 1 || gatewayWeight > math.MaxInt32 { diff --git a/overlay/route_test.go b/overlay/route_test.go index 8d62f0b0..8a8cb4cf 100644 --- a/overlay/route_test.go +++ b/overlay/route_test.go @@ -1,8 +1,10 @@ package overlay import ( + "bytes" "fmt" "net/netip" + "strings" "testing" "github.com/slackhq/nebula/config" @@ -445,3 +447,74 @@ func Test_getAllRoutesFromConfigCapsMTU(t *testing.T) { } assert.Equal(t, map[string]int{"10.0.0.0/29": MaxMTU, "10.0.0.8/29": 1300, "1.0.0.0/29": MaxMTU}, mtus) } + +// The route tree keeps one entry per CIDR, so a CIDR listed twice only routes by the last one. Say so instead of +// letting the earlier via, or on Windows the earlier metric, quietly lose +func Test_makeRouteTree_WarnsOnDuplicate(t *testing.T) { + buf := &bytes.Buffer{} + l := test.NewLoggerWithOutput(buf) + via := func(s string) routing.Gateways { + return routing.Gateways{routing.NewGateway(netip.MustParseAddr(s), 1)} + } + cidr := netip.MustParsePrefix("10.251.1.0/24") + + _, err := makeRouteTree(l, []Route{ + {Cidr: cidr, Via: via("10.250.0.2"), Install: true, Metric: 100}, + {Cidr: netip.MustParsePrefix("10.251.2.0/24"), Via: via("10.250.0.2"), Install: true}, + }, false) + require.NoError(t, err) + assert.Empty(t, buf.String()) + + tree, err := makeRouteTree(l, []Route{ + {Cidr: cidr, Via: via("10.250.0.2"), Install: true, Metric: 100}, + {Cidr: cidr, Via: via("10.250.0.3"), Install: true, Metric: 200}, + }, false) + require.NoError(t, err) + assert.Equal(t, 1, strings.Count(buf.String(), "route is listed more than once, only the last entry's via is used")) + gw, ok := tree.Lookup(netip.MustParseAddr("10.251.1.1")) + require.True(t, ok) + assert.Equal(t, netip.MustParseAddr("10.250.0.3"), gw[0].Addr(), "the last entry wins") +} + +// A quoted metric or weight is a number written as a string, it means that number. Anything else is an error, not a +// panic +func Test_parseUnsafeRoutes_QuotedNumbers(t *testing.T) { + c := config.NewC(test.NewLogger()) + n := netip.MustParsePrefix("10.0.0.1/24") + route := func(metric, weight any) { + c.Settings["tun"] = map[string]any{"unsafe_routes": []any{map[string]any{ + "route": "1.0.0.0/8", + "metric": metric, + "via": []any{map[string]any{"gateway": "10.0.0.2", "weight": weight}}, + }}} + } + + route("100", "5") + routes, err := parseUnsafeRoutes(c, []netip.Prefix{n}) + require.NoError(t, err) + require.Len(t, routes, 1) + assert.Equal(t, 100, routes[0].Metric) + assert.Equal(t, "{addr: 10.0.0.2, weight: 5}", routes[0].Via[0].String()) + + route(1.5, 1) + _, err = parseUnsafeRoutes(c, []netip.Prefix{n}) + require.EqualError(t, err, "entry 1.metric in tun.unsafe_routes is not an integer: 1.5") + + route(1, true) + _, err = parseUnsafeRoutes(c, []netip.Prefix{n}) + require.EqualError(t, err, "entry .weight in tun.unsafe_routes[1].via[1] is not an integer") +} + +// An mtu that is neither a number nor a quoted one is an error, not a panic +func Test_parseRoutes_MTUNotANumber(t *testing.T) { + c := config.NewC(test.NewLogger()) + n := netip.MustParsePrefix("10.0.0.1/24") + + c.Settings["tun"] = map[string]any{"routes": []any{map[string]any{"route": "1.0.0.0/8", "mtu": 1500.5}}} + _, err := parseRoutes(c, []netip.Prefix{n}) + require.EqualError(t, err, "entry 1.mtu in tun.routes is not an integer: 1500.5") + + c.Settings["tun"] = map[string]any{"unsafe_routes": []any{map[string]any{"route": "1.0.0.0/8", "via": "10.0.0.2", "mtu": 1500.5}}} + _, err = parseUnsafeRoutes(c, []netip.Prefix{n}) + require.EqualError(t, err, "entry 1.mtu in tun.unsafe_routes is not an integer: 1500.5") +} diff --git a/overlay/tun_windows.go b/overlay/tun_windows.go index 982b8fa0..18680f2b 100644 --- a/overlay/tun_windows.go +++ b/overlay/tun_windows.go @@ -4,6 +4,7 @@ package overlay import ( "crypto" + "errors" "fmt" "log/slog" "net/netip" @@ -138,7 +139,7 @@ func (t *winTun) reload(c *config.C, initial bool) error { if !initial { // Remove first, if the system removes a wanted route hopefully it will be re-added next - err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes)) + err := t.removeRoutes(findRemovedSystemRoutes(routes, *oldRoutes)) if err != nil { util.LogWithContextIfNeeded("Failed to remove routes", err, t.l) } @@ -187,17 +188,27 @@ func (t *winTun) addRoutes(logErrors bool) error { foundDefault4 := false carriesV6 := slices.ContainsFunc(t.vpnNetworks, func(p netip.Prefix) bool { return p.Addr().Is6() }) - for _, r := range routes { + // A CIDR listed twice routes by its last entry, install only that one or each reload would flip between them. The + // same rule as systemRoutes + last := make(map[netip.Prefix]int, len(routes)) + for i, r := range routes { + last[r.Cidr] = i + } + + for i, r := range routes { if len(r.Via) == 0 || !r.Install { // We don't allow route MTUs so only install routes with a via continue } + if last[r.Cidr] != i { + continue + } // A v6 unsafe_route is legal under a v4-only cert; uninstalled ones put nothing on the adapter. carriesV6 = carriesV6 || r.Cidr.Addr().Is6() // Add our unsafe route as an on-link route to the nebula tun device. - err := luid.AddRoute(r.Cidr, unspecifiedNextHop(r.Cidr), uint32(r.Metric)) + result, err := addRoute(luid, r) if err != nil { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { @@ -207,7 +218,12 @@ func (t *winTun) addRoutes(logErrors bool) error { return retErr } } else { - t.l.Info("Added route", "route", r) + switch result { + case routeAdded: + t.l.Info("Added route", "route", r) + case routeMetricUpdated: + t.l.Info("Updated route metric", "route", r) + } } if !foundDefault4 { @@ -319,6 +335,61 @@ func (t *winTun) Close() error { return t.tun.Close() } +// systemRoutes maps each CIDR to the route that puts it in the system table. A CIDR's last entry decides, including +// one that says not to install it +func systemRoutes(routes []Route) map[netip.Prefix]Route { + out := make(map[netip.Prefix]Route, len(routes)) + for _, r := range routes { + if len(r.Via) > 0 && r.Install { + out[r.Cidr] = r + } else { + delete(out, r.Cidr) + } + } + return out +} + +// findRemovedSystemRoutes is the routes to delete from the system table. Windows deletes by destination and next hop, +// so only a CIDR nothing installs anymore goes, a changed metric is set on the route in place by addRoute +func findRemovedSystemRoutes(newRoutes, oldRoutes []Route) []Route { + keep := systemRoutes(newRoutes) + var removed []Route + for _, r := range systemRoutes(oldRoutes) { + if _, ok := keep[r.Cidr]; !ok { + removed = append(removed, r) + } + } + return removed +} + +type routeResult int + +const ( + routeAdded routeResult = iota + routeMetricUpdated + routeAlreadyInstalled +) + +// addRoute installs r on the interface. A reload re-adds every route to catch any the system dropped, and Windows +// refuses one it has with the same destination and next hop whatever its metric. One that is there with another +// metric, ours from before a metric change or someone else's, is brought to ours in place +func addRoute(luid winipcfg.LUID, r Route) (routeResult, error) { + nextHop := unspecifiedNextHop(r.Cidr) + err := luid.AddRoute(r.Cidr, nextHop, uint32(r.Metric)) + if !errors.Is(err, windows.ERROR_OBJECT_ALREADY_EXISTS) { + return routeAdded, err + } + row, err := luid.Route(r.Cidr, nextHop) + if err != nil { + return routeAlreadyInstalled, err + } + if row.Metric != uint32(r.Metric) { + row.Metric = uint32(r.Metric) + return routeMetricUpdated, row.Set() + } + return routeAlreadyInstalled, nil +} + func unspecifiedNextHop(p netip.Prefix) netip.Addr { if p.Addr().Is4() { return netip.IPv4Unspecified() diff --git a/overlay/tun_windows_route_test.go b/overlay/tun_windows_route_test.go new file mode 100644 index 00000000..71c5c232 --- /dev/null +++ b/overlay/tun_windows_route_test.go @@ -0,0 +1,168 @@ +//go:build windows && !e2e_testing + +package overlay + +import ( + "bytes" + "fmt" + "net/netip" + "strings" + "testing" + + "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" + "golang.zx2c4.com/wireguard/windows/tunnel/winipcfg" +) + +type routeTest struct { + c *config.C + tun *winTun + luid winipcfg.LUID + log *bytes.Buffer +} + +// newRouteTest makes a wintun device of its own, before installing tun.unsafe_routes from cfg +func newRouteTest(t *testing.T, cfg string) *routeTest { + t.Helper() + if !windows.GetCurrentProcessToken().IsElevated() { + t.Skip("creating a wintun device needs an elevated process") + } + if err := checkWinTunExists(); err != nil { + t.Skipf("wintun.dll is not where nebula looks for it: %v", err) + } + rt := &routeTest{log: &bytes.Buffer{}} + l := test.NewLoggerWithOutput(rt.log) + rt.c = config.NewC(l) + require.NoError(t, rt.c.LoadString(cfg)) + var err error + rt.tun, err = newTun(rt.c, l, []netip.Prefix{netip.MustParsePrefix("10.250.0.1/24")}, false) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.tun.Close() }) + rt.luid = winipcfg.LUID(rt.tun.tun.LUID()) + return rt +} + +func (rt *routeTest) metric(t *testing.T, cidr string) uint32 { + t.Helper() + p := netip.MustParsePrefix(cidr) + row, err := rt.luid.Route(p, unspecifiedNextHop(p)) + require.NoError(t, err) + return row.Metric +} + +func unsafeRoutes(routes ...string) string { + var b strings.Builder + b.WriteString("tun:\n dev: nebroutetest\n unsafe_routes:\n") + // cidr[@metric][!], the ! for install: false + for _, r := range routes { + r, noInstall := strings.CutSuffix(r, "!") + cidr, metric, _ := strings.Cut(r, "@") + fmt.Fprintf(&b, " - route: %s\n via: 10.250.0.2\n", cidr) + if metric != "" { + fmt.Fprintf(&b, " metric: %s\n", metric) + } + if noInstall { + b.WriteString(" install: false\n") + } + } + return b.String() +} + +// A reload that adds a route re-adds the ones already installed, Windows refusing those as already there isn't a failure +func TestWinTun_ReloadKeepsInstalledRoutes(t *testing.T) { + forFamilies(t, func(t *testing.T, a, b string) { + rt := newRouteTest(t, unsafeRoutes(a)) + require.NoError(t, rt.tun.Activate()) + + require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a, b))) + assert.NotContains(t, rt.log.String(), "Failed to add route") + rt.metric(t, a) + rt.metric(t, b) + }) +} + +// Windows calls a route the same by destination and next hop, so a changed metric is set on the installed route in +// place rather than deleted and added, which would leave a moment with no route +func TestWinTun_ReloadMetricChangeApplies(t *testing.T) { + forFamilies(t, func(t *testing.T, a, b string) { + rt := newRouteTest(t, unsafeRoutes(a+"@100")) + require.NoError(t, rt.tun.Activate()) + require.Equal(t, uint32(100), rt.metric(t, a)) + + require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a+"@200"))) + assert.NotContains(t, rt.log.String(), "Failed to add route") + assert.NotContains(t, rt.log.String(), "Removed route") + assert.Equal(t, uint32(200), rt.metric(t, a)) + }) +} + +// A route someone else put on the device with another metric is brought to ours, at startup too +func TestWinTun_ForeignRouteGetsOurMetric(t *testing.T) { + forFamilies(t, func(t *testing.T, a, b string) { + rt := newRouteTest(t, unsafeRoutes(a+"@100")) + p := netip.MustParsePrefix(a) + require.NoError(t, rt.luid.AddRoute(p, unspecifiedNextHop(p), 50)) + + require.NoError(t, rt.tun.Activate()) + assert.Equal(t, uint32(100), rt.metric(t, a)) + assert.Contains(t, rt.log.String(), "level=INFO msg=\"Updated route metric\"") + }) +} + +// A CIDR listed twice is installed once, by its last entry, and reloads leave it there instead of flipping between the +// two metrics +func TestWinTun_DuplicateCidrInstallsTheLastEntry(t *testing.T) { + forFamilies(t, func(t *testing.T, a, b string) { + rt := newRouteTest(t, unsafeRoutes(a+"@100", a+"@200")) + require.NoError(t, rt.tun.Activate()) + assert.Equal(t, uint32(200), rt.metric(t, a)) + + require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a+"@100", a+"@200", b))) + assert.Equal(t, uint32(200), rt.metric(t, a)) + assert.NotContains(t, rt.log.String(), "Updated route metric") + assert.NotContains(t, rt.log.String(), "Failed to add route") + }) +} + +// Dropping the duplicate that wasn't installed leaves the installed route alone. Windows deletes by destination and +// next hop, deleting the dropped one would take the live route with it +func TestWinTun_DroppingAnUninstalledDuplicateKeepsTheRoute(t *testing.T) { + forFamilies(t, func(t *testing.T, a, b string) { + rt := newRouteTest(t, unsafeRoutes(a+"@100", a+"@200")) + require.NoError(t, rt.tun.Activate()) + + require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a+"@200"))) + assert.NotContains(t, rt.log.String(), "Removed route") + assert.Equal(t, uint32(200), rt.metric(t, a)) + }) +} + +// A reload ends where a fresh start with the same config would, a CIDR whose last entry says not to install it is +// removed +func TestWinTun_ReloadMatchesAFreshStart(t *testing.T) { + forFamilies(t, func(t *testing.T, a, b string) { + cidr := netip.MustParsePrefix(a) + routes := unsafeRoutes(a+"@100", a+"@100!") + + fresh := newRouteTest(t, routes) + require.NoError(t, fresh.tun.Activate()) + _, err := fresh.luid.Route(cidr, unspecifiedNextHop(cidr)) + require.Error(t, err, "a fresh start installed it") + require.NoError(t, fresh.tun.Close()) + + rt := newRouteTest(t, unsafeRoutes(a+"@100")) + require.NoError(t, rt.tun.Activate()) + require.NoError(t, rt.c.ReloadConfigString(routes)) + _, err = rt.luid.Route(cidr, unspecifiedNextHop(cidr)) + assert.Error(t, err, "the reload left it installed") + }) +} + +// forFamilies runs f with two unsafe_routes CIDRs of each address family, the tun's own network stays IPv4 +func forFamilies(t *testing.T, f func(t *testing.T, a, b string)) { + t.Run("ipv4", func(t *testing.T) { f(t, "10.251.1.0/24", "10.251.2.0/24") }) + t.Run("ipv6", func(t *testing.T) { f(t, "fd00:251:1::/64", "fd00:251:2::/64") }) +}