overlay: Handle routes Windows already has, and parse quoted route numbers (#1924)

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 {

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")
+}

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()

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") })
+}