Merge pull request #945 from netbirdio/feat/refactor_route_adding_in_client

Refactor check logic when adding routes

client/internal/routemanager/systemops_bsd.go

@@ -0,0 +1,82 @@
+//go:build darwin || dragonfly || freebsd || netbsd || openbsd
+// +build darwin dragonfly freebsd netbsd openbsd
+
+package routemanager
+
+import (
+	"fmt"
+	"net"
+	"net/netip"
+	"syscall"
+
+	"golang.org/x/net/route"
+)
+
+// selected BSD Route flags.
+const (
+	RTF_UP        = 0x1
+	RTF_GATEWAY   = 0x2
+	RTF_HOST      = 0x4
+	RTF_REJECT    = 0x8
+	RTF_DYNAMIC   = 0x10
+	RTF_MODIFIED  = 0x20
+	RTF_STATIC    = 0x800
+	RTF_BLACKHOLE = 0x1000
+	RTF_LOCAL     = 0x200000
+	RTF_BROADCAST = 0x400000
+	RTF_MULTICAST = 0x800000
+)
+
+func existsInRouteTable(prefix netip.Prefix) (bool, error) {
+	tab, err := route.FetchRIB(syscall.AF_UNSPEC, route.RIBTypeRoute, 0)
+	if err != nil {
+		return false, err
+	}
+	msgs, err := route.ParseRIB(route.RIBTypeRoute, tab)
+	if err != nil {
+		return false, err
+	}
+
+	for _, msg := range msgs {
+		m := msg.(*route.RouteMessage)
+
+		if m.Version < 3 || m.Version > 5 {
+			return false, fmt.Errorf("unexpected RIB message version: %d", m.Version)
+		}
+		if m.Type != 4 /* RTM_GET */ {
+			return true, fmt.Errorf("unexpected RIB message type: %d", m.Type)
+		}
+
+		if m.Flags&RTF_UP == 0 ||
+			m.Flags&(RTF_REJECT|RTF_BLACKHOLE) != 0 {
+			continue
+		}
+
+		dst, err := toIPAddr(m.Addrs[0])
+		if err != nil {
+			return true, fmt.Errorf("unexpected RIB destination: %v", err)
+		}
+
+		mask, _ := toIPAddr(m.Addrs[2])
+		cidr, _ := net.IPMask(mask.To4()).Size()
+		if dst.String() == prefix.Addr().String() && cidr == prefix.Bits() {
+			return true, nil
+		}
+	}
+
+	return false, nil
+}
+
+func toIPAddr(a route.Addr) (net.IP, error) {
+	switch t := a.(type) {
+	case *route.Inet4Addr:
+		ip := net.IPv4(t.IP[0], t.IP[1], t.IP[2], t.IP[3])
+		return ip, nil
+	case *route.Inet6Addr:
+		ip := make(net.IP, net.IPv6len)
+		copy(ip, t.IP[:])
+		return ip, nil
+	default:
+		return net.IP{}, fmt.Errorf("unknown family: %v", t)
+	}
+}

client/internal/routemanager/systemops_linux.go

@@ -6,10 +6,28 @@ import (
 	"net"
 	"net/netip"
 	"os"
+	"syscall"
+	"unsafe"
 
 	"github.com/vishvananda/netlink"
 )
 
+// Pulled from http://man7.org/linux/man-pages/man7/rtnetlink.7.html
+// See the section on RTM_NEWROUTE, specifically 'struct rtmsg'.
+type routeInfoInMemory struct {
+	Family byte
+	DstLen byte
+	SrcLen byte
+	TOS    byte
+
+	Table    byte
+	Protocol byte
+	Scope    byte
+	Type     byte
+
+	Flags uint32
+}
+
 const ipv4ForwardingPath = "/proc/sys/net/ipv4/ip_forward"
 
 func addToRouteTable(prefix netip.Prefix, addr string) error {
@@ -61,6 +79,45 @@ func removeFromRouteTable(prefix netip.Prefix) error {
 	return nil
 }
 
+func existsInRouteTable(prefix netip.Prefix) (bool, error) {
+	tab, err := syscall.NetlinkRIB(syscall.RTM_GETROUTE, syscall.AF_UNSPEC)
+	if err != nil {
+		return true, err
+	}
+	msgs, err := syscall.ParseNetlinkMessage(tab)
+	if err != nil {
+		return true, err
+	}
+loop:
+	for _, m := range msgs {
+		switch m.Header.Type {
+		case syscall.NLMSG_DONE:
+			break loop
+		case syscall.RTM_NEWROUTE:
+			rt := (*routeInfoInMemory)(unsafe.Pointer(&m.Data[0]))
+			attrs, err := syscall.ParseNetlinkRouteAttr(&m)
+			if err != nil {
+				return true, err
+			}
+			if rt.Family != syscall.AF_INET {
+				continue loop
+			}
+
+			for _, attr := range attrs {
+				if attr.Attr.Type == syscall.RTA_DST {
+					ip := net.IP(attr.Value)
+					mask := net.CIDRMask(int(rt.DstLen), len(attr.Value)*8)
+					cidr, _ := mask.Size()
+					if ip.String() == prefix.Addr().String() && cidr == prefix.Bits() {
+						return true, nil
+					}
+				}
+			}
+		}
+	}
+	return false, nil
+}
+
 func enableIPForwarding() error {
 	bytes, err := os.ReadFile(ipv4ForwardingPath)
 	if err != nil {

client/internal/routemanager/systemops_nonandroid.go

@@ -14,19 +14,26 @@ import (
 var errRouteNotFound = fmt.Errorf("route not found")
 
 func addToRouteTableIfNoExists(prefix netip.Prefix, addr string) error {
-	gateway, err := getExistingRIBRouteGateway(netip.MustParsePrefix("0.0.0.0/0"))
+	defaultGateway, err := getExistingRIBRouteGateway(netip.MustParsePrefix("0.0.0.0/0"))
-	if err != nil && err != errRouteNotFound {
-		return err
-	}
-	prefixGateway, err := getExistingRIBRouteGateway(prefix)
 	if err != nil && err != errRouteNotFound {
 		return err
 	}
 
-	if prefixGateway != nil && !prefixGateway.Equal(gateway) {
+	gatewayIP := netip.MustParseAddr(defaultGateway.String())
-		log.Warnf("skipping adding a new route for network %s because it already exists and is pointing to the non default gateway: %s", prefix, prefixGateway)
+	if prefix.Contains(gatewayIP) {
+		log.Warnf("skipping adding a new route for network %s because it overlaps with the default gateway: %s", prefix, gatewayIP)
 		return nil
 	}
+
+	ok, err := existsInRouteTable(prefix)
+	if err != nil {
+		return err
+	}
+	if ok {
+		log.Warnf("skipping adding a new route for network %s because it already exists", prefix)
+		return nil
+	}
+
 	return addToRouteTable(prefix, addr)
 }
 
@@ -53,6 +60,7 @@ func getExistingRIBRouteGateway(prefix netip.Prefix) (net.IP, error) {
 		log.Errorf("getting routes returned an error: %v", err)
 		return nil, errRouteNotFound
 	}
+
 	if gateway == nil {
 		return preferredSrc, nil
 	}

client/internal/routemanager/systemops_nonandroid_test.go

@@ -1,13 +1,19 @@
 package routemanager
 
 import (
+	"bytes"
 	"fmt"
-	"github.com/netbirdio/netbird/iface"
-	"github.com/pion/transport/v2/stdnet"
-	"github.com/stretchr/testify/require"
 	"net"
 	"net/netip"
+	"os"
+	"strings"
 	"testing"
+
+	"github.com/pion/transport/v2/stdnet"
+	log "github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+
+	"github.com/netbirdio/netbird/iface"
 )
 
 func TestAddRemoveRoutes(t *testing.T) {
@@ -114,3 +120,98 @@ func TestGetExistingRIBRouteGateway(t *testing.T) {
 		t.Fatalf("local ip should match with testing IP: want %s got %s", testingIP, localIP.String())
 	}
 }
+
+func TestAddExistAndRemoveRouteNonAndroid(t *testing.T) {
+	defaultGateway, err := getExistingRIBRouteGateway(netip.MustParsePrefix("0.0.0.0/0"))
+	fmt.Println("defaultGateway: ", defaultGateway)
+	if err != nil {
+		t.Fatal("shouldn't return error when fetching the gateway: ", err)
+	}
+	testCases := []struct {
+		name              string
+		prefix            netip.Prefix
+		preExistingPrefix netip.Prefix
+		shouldAddRoute    bool
+	}{
+		{
+			name:           "Should Add And Remove random Route",
+			prefix:         netip.MustParsePrefix("99.99.99.99/32"),
+			shouldAddRoute: true,
+		},
+		{
+			name:           "Should Not Add Route if overlaps with default gateway",
+			prefix:         netip.MustParsePrefix(defaultGateway.String() + "/31"),
+			shouldAddRoute: false,
+		},
+		{
+			name:              "Should Add Route if bigger network exists",
+			prefix:            netip.MustParsePrefix("100.100.100.0/24"),
+			preExistingPrefix: netip.MustParsePrefix("100.100.0.0/16"),
+			shouldAddRoute:    true,
+		},
+		{
+			name:              "Should Add Route if smaller network exists",
+			prefix:            netip.MustParsePrefix("100.100.0.0/16"),
+			preExistingPrefix: netip.MustParsePrefix("100.100.100.0/24"),
+			shouldAddRoute:    true,
+		},
+		{
+			name:              "Should Not Add Route if same network exists",
+			prefix:            netip.MustParsePrefix("100.100.0.0/16"),
+			preExistingPrefix: netip.MustParsePrefix("100.100.0.0/16"),
+			shouldAddRoute:    false,
+		},
+	}
+
+	for n, testCase := range testCases {
+		var buf bytes.Buffer
+		log.SetOutput(&buf)
+		defer func() {
+			log.SetOutput(os.Stderr)
+		}()
+		t.Run(testCase.name, func(t *testing.T) {
+			newNet, err := stdnet.NewNet()
+			if err != nil {
+				t.Fatal(err)
+			}
+			wgInterface, err := iface.NewWGIFace(fmt.Sprintf("utun53%d", n), "100.65.75.2/24", iface.DefaultMTU, nil, newNet)
+			require.NoError(t, err, "should create testing WGIface interface")
+			defer wgInterface.Close()
+
+			err = wgInterface.Create()
+			require.NoError(t, err, "should create testing wireguard interface")
+
+			MockAddr := wgInterface.Address().IP.String()
+
+			// Prepare the environment
+			if testCase.preExistingPrefix.IsValid() {
+				err := addToRouteTableIfNoExists(testCase.preExistingPrefix, MockAddr)
+				require.NoError(t, err, "should not return err when adding pre-existing route")
+			}
+
+			// Add the route
+			err = addToRouteTableIfNoExists(testCase.prefix, MockAddr)
+			require.NoError(t, err, "should not return err when adding route")
+
+			if testCase.shouldAddRoute {
+				// test if route exists after adding
+				ok, err := existsInRouteTable(testCase.prefix)
+				require.NoError(t, err, "should not return err")
+				require.True(t, ok, "route should exist")
+
+				// remove route again if added
+				err = removeFromRouteTableIfNonSystem(testCase.prefix, MockAddr)
+				require.NoError(t, err, "should not return err")
+			}
+
+			// route should either not have been added or should have been removed
+			// In case of already existing route, it should not have been added (but still exist)
+			ok, err := existsInRouteTable(testCase.prefix)
+			fmt.Println("Buffer string: ", buf.String())
+			require.NoError(t, err, "should not return err")
+			if !strings.Contains(buf.String(), "because it already exists") {
+				require.False(t, ok, "route should not exist")
+			}
+		})
+	}
+}

client/internal/routemanager/systemops_windows.go

@@ -0,0 +1,37 @@
+//go:build windows
+// +build windows
+
+package routemanager
+
+import (
+	"net"
+	"net/netip"
+
+	"github.com/yusufpapurcu/wmi"
+)
+
+type Win32_IP4RouteTable struct {
+	Destination string
+	Mask        string
+}
+
+func existsInRouteTable(prefix netip.Prefix) (bool, error) {
+	var routes []Win32_IP4RouteTable
+	query := "SELECT Destination, Mask FROM Win32_IP4RouteTable"
+
+	err := wmi.Query(query, &routes)
+	if err != nil {
+		return true, err
+	}
+
+	for _, route := range routes {
+		ip := net.ParseIP(route.Mask)
+		ip = ip.To4()
+		mask := net.IPv4Mask(ip[0], ip[1], ip[2], ip[3])
+		cidr, _ := mask.Size()
+		if route.Destination == prefix.Addr().String() && cidr == prefix.Bits() {
+			return true, nil
+		}
+	}
+	return false, nil
+}

go.mod

@@ -57,6 +57,7 @@ require (
 	github.com/rs/xid v1.3.0
 	github.com/skratchdot/open-golang v0.0.0-20200116055534-eef842397966
 	github.com/stretchr/testify v1.8.1
+	github.com/yusufpapurcu/wmi v1.2.3
 	go.opentelemetry.io/otel v1.11.1
 	go.opentelemetry.io/otel/exporters/prometheus v0.33.0
 	go.opentelemetry.io/otel/metric v0.33.0
@@ -92,6 +93,7 @@ require (
 	github.com/go-gl/glfw/v3.3/glfw v0.0.0-20211024062804-40e447a793be // indirect
 	github.com/go-logr/logr v1.2.3 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
+	github.com/go-ole/go-ole v1.2.6 // indirect
 	github.com/go-redis/redis/v8 v8.11.5 // indirect
 	github.com/go-stack/stack v1.8.0 // indirect
 	github.com/goki/freetype v0.0.0-20181231101311-fa8a33aabaff // indirect

go.sum

@@ -219,6 +219,7 @@ github.com/go-logr/logr v1.2.3 h1:2DntVwHkVopvECVRSlL5PSo9eG+cAkDCuckLubN+rq0=
 github.com/go-logr/logr v1.2.3/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
 github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
+github.com/go-ole/go-ole v1.2.6 h1:/Fpf6oFPoeFik9ty7siob0G6Ke8QvQEuVcuChpwXzpY=
 github.com/go-ole/go-ole v1.2.6/go.mod h1:pprOEPIfldk/42T2oK7lQ4v4JSDwmV0As9GaiUsvbm0=
 github.com/go-openapi/jsonpointer v0.0.0-20160704185906-46af16f9f7b1/go.mod h1:+35s3my2LFTysnkMfxsJBAMHj/DoqoB9knIWoYG/Vk0=
 github.com/go-openapi/jsonpointer v0.19.3/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg=
@@ -661,6 +662,8 @@ github.com/yuin/goldmark v1.4.0/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1
 github.com/yuin/goldmark v1.4.1/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
 github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
+github.com/yusufpapurcu/wmi v1.2.3 h1:E1ctvB7uKFMOJw3fdOW32DwGE9I7t++CRUEMKvFoFiw=
+github.com/yusufpapurcu/wmi v1.2.3/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0=
 go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=
 go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU=
 go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8=