From 493ddb4fe3466d2e73fce2e5fc3e2e4f761f9e64 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Fri, 9 Jun 2023 17:59:06 +0200 Subject: [PATCH] Revert "hacky all-operating-systems solution" This reverts commit 75fac258e743f95f9f2187d1cf7670be18012ec7. --- .../routemanager/systemops_nonandroid.go | 102 +++++++++++------- .../routemanager/systemops_nonandroid_test.go | 62 ++++------- 2 files changed, 84 insertions(+), 80 deletions(-) diff --git a/client/internal/routemanager/systemops_nonandroid.go b/client/internal/routemanager/systemops_nonandroid.go index dd2d18ad8..c9efd7882 100644 --- a/client/internal/routemanager/systemops_nonandroid.go +++ b/client/internal/routemanager/systemops_nonandroid.go @@ -6,9 +6,26 @@ import ( "fmt" "net" "net/netip" + "syscall" "github.com/libp2p/go-netroute" log "github.com/sirupsen/logrus" + "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 ) var errRouteNotFound = fmt.Errorf("route not found") @@ -25,16 +42,7 @@ func addToRouteTableIfNoExists(prefix netip.Prefix, addr string) error { return nil } - ok, err := routeExistsWithDifferentGateway(prefix, addr) - if err != nil { - return err - } - if ok { - log.Warnf("skipping adding a new route for network %s because it already exists", prefix) - return nil - } - - ok, err = routeExistsWithSameGateway(prefix, addr) + ok, err := existsInRouteTable(prefix) if err != nil { return err } @@ -64,53 +72,65 @@ func getExistingRIBRouteGateway(prefix netip.Prefix) (net.IP, error) { if err != nil { return nil, err } - _, gateway, preferredSrc, err := r.Route(prefix.Addr().AsSlice()) + _, gateway, _, err := r.Route(prefix.Addr().AsSlice()) if err != nil { log.Errorf("getting routes returned an error: %v", err) return nil, errRouteNotFound } - if gateway == nil { - return preferredSrc, nil - } - return gateway, nil } -func routeExistsWithSameGateway(prefix netip.Prefix, addr string) (bool, error) { - r, err := netroute.New() +func existsInRouteTable(prefix netip.Prefix) (bool, error) { + tab, err := route.FetchRIB(syscall.AF_UNSPEC, route.RIBTypeRoute, 0) if err != nil { - return true, err + return false, err + } + msgs, err := route.ParseRIB(route.RIBTypeRoute, tab) + if err != nil { + return false, err } - _, gateway, preferredSrc, err := r.Route(prefix.Addr().AsSlice()) - if err != nil { - log.Errorf("getting routes returned an error: %v", err) - return true, errRouteNotFound - } + for _, msg := range msgs { + m := msg.(*route.RouteMessage) - if gateway.String() == addr || preferredSrc.String() == addr { - return true, nil + 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 routeExistsWithDifferentGateway(prefix netip.Prefix, addr string) (bool, error) { - r, err := netroute.New() - if err != nil { - return true, err +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) } - - _, gateway, preferredSrc, err := r.Route(prefix.Addr().AsSlice()) - if err != nil { - log.Errorf("getting routes returned an error: %v", err) - return true, errRouteNotFound - } - - if gateway.String() != addr && preferredSrc.String() != addr { - return false, nil - } - - return true, nil } diff --git a/client/internal/routemanager/systemops_nonandroid_test.go b/client/internal/routemanager/systemops_nonandroid_test.go index 1f086fe62..2020aa8d4 100644 --- a/client/internal/routemanager/systemops_nonandroid_test.go +++ b/client/internal/routemanager/systemops_nonandroid_test.go @@ -86,59 +86,43 @@ func TestAddExistAndRemoveRoute(t *testing.T) { t.Fatal("shouldn't return error when fetching the gateway: ", err) } testCases := []struct { - name string - prefix netip.Prefix - gateway string - preExistingPrefix netip.Prefix - preExistingGateway string - shouldAddRoute bool + 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"), - gateway: "127.0.0.1", shouldAddRoute: true, }, { name: "Should Not Add Route if overlaps with default gateway", prefix: netip.MustParsePrefix(defaultGateway.String() + "/31"), - gateway: "127.0.0.1", shouldAddRoute: false, }, { - name: "Should Add Route if bigger network exists", - prefix: netip.MustParsePrefix("100.100.100.0/24"), - gateway: "127.0.0.1", - preExistingPrefix: netip.MustParsePrefix("100.100.0.0/16"), - preExistingGateway: "127.0.0.1", - shouldAddRoute: true, + 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"), - gateway: "127.0.0.1", - preExistingPrefix: netip.MustParsePrefix("100.100.100.0/24"), - preExistingGateway: "127.0.0.1", - 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 with same gateway exists", - prefix: netip.MustParsePrefix("100.100.0.0/16"), - gateway: "127.0.0.1", - preExistingPrefix: netip.MustParsePrefix("100.100.0.0/16"), - preExistingGateway: "127.0.0.1", - shouldAddRoute: false, - }, - { - name: "Should Not Add Route if same network with different gateway exists", - prefix: netip.MustParsePrefix("100.100.0.0/16"), - gateway: "127.0.0.1", - preExistingPrefix: netip.MustParsePrefix("100.100.0.0/16"), - preExistingGateway: "123.0.0.1", - shouldAddRoute: false, + 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, }, } + MOCK_ADDR := "127.0.0.1" + for _, testCase := range testCases { var buf bytes.Buffer log.SetOutput(&buf) @@ -148,28 +132,28 @@ func TestAddExistAndRemoveRoute(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { // Prepare the environment if testCase.preExistingPrefix.IsValid() { - err := addToRouteTableIfNoExists(testCase.preExistingPrefix, testCase.preExistingGateway) + err := addToRouteTableIfNoExists(testCase.preExistingPrefix, MOCK_ADDR) require.NoError(t, err, "should not return err when adding pre-existing route") } // Add the route - err = addToRouteTableIfNoExists(testCase.prefix, testCase.gateway) + err = addToRouteTableIfNoExists(testCase.prefix, MOCK_ADDR) require.NoError(t, err, "should not return err when adding pre-existing route") if testCase.shouldAddRoute { // test if route exists after adding - ok, err := routeExistsWithSameGateway(testCase.prefix, testCase.gateway) + 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, testCase.gateway) + err = removeFromRouteTableIfNonSystem(testCase.prefix, MOCK_ADDR) 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 := routeExistsWithSameGateway(testCase.prefix, testCase.gateway) + 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") {