From 5ea24ba56e219d08fa9a5e444e0c7dfb9dd4ec50 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Fri, 12 Apr 2024 17:53:07 +0200 Subject: [PATCH] Add sysctl opts to prevent reverse path filtering from dropping fwmark packets (#1839) --- .../internal/routemanager/systemops_linux.go | 131 +++++++++++++++--- .../internal/routemanager/systemops_test.go | 2 +- 2 files changed, 115 insertions(+), 18 deletions(-) diff --git a/client/internal/routemanager/systemops_linux.go b/client/internal/routemanager/systemops_linux.go index a0f55131df4..d1302b39cc6 100644 --- a/client/internal/routemanager/systemops_linux.go +++ b/client/internal/routemanager/systemops_linux.go @@ -9,6 +9,8 @@ import ( "net" "net/netip" "os" + "strconv" + "strings" "syscall" "github.com/hashicorp/go-multierror" @@ -30,14 +32,26 @@ const ( rtTablesPath = "/etc/iproute2/rt_tables" // ipv4ForwardingPath is the path to the file containing the IP forwarding setting. - ipv4ForwardingPath = "/proc/sys/net/ipv4/ip_forward" + ipv4ForwardingPath = "net.ipv4.ip_forward" + + rpFilterPath = "net.ipv4.conf.all.rp_filter" + rpFilterInterfacePath = "net.ipv4.conf.%s.rp_filter" + srcValidMarkPath = "net.ipv4.conf.all.src_valid_mark" ) var ErrTableIDExists = errors.New("ID exists with different name") var routeManager = &RouteManager{} + +// originalSysctl stores the original sysctl values before they are modified +var originalSysctl map[string]int + +// determines whether to use the legacy routing setup var isLegacy = os.Getenv("NB_USE_LEGACY_ROUTING") == "true" || nbnet.CustomRoutingDisabled() +// sysctlFailed is used as an indicator to emit a warning when default routes are configured +var sysctlFailed bool + type ruleParams struct { priority int fwmark int @@ -77,6 +91,13 @@ func setupRouting(initAddresses []net.IP, wgIface *iface.WGIface) (_ peer.Before log.Errorf("Error adding routing table name: %v", err) } + originalValues, err := setupSysctl(wgIface) + if err != nil { + log.Errorf("Error setting up sysctl: %v", err) + sysctlFailed = true + } + originalSysctl = originalValues + defer func() { if err != nil { if cleanErr := cleanupRouting(); cleanErr != nil { @@ -124,6 +145,12 @@ func cleanupRouting() error { } } + if err := cleanupSysctl(originalSysctl); err != nil { + result = multierror.Append(result, fmt.Errorf("cleanup sysctl: %w", err)) + } + originalSysctl = nil + sysctlFailed = false + return result.ErrorOrNil() } @@ -140,6 +167,10 @@ func addVPNRoute(prefix netip.Prefix, intf string) error { return genericAddVPNRoute(prefix, intf) } + if sysctlFailed && (prefix == defaultv4 || prefix == defaultv6) { + log.Warnf("Default route is configured but sysctl operations failed, VPN traffic may not be routed correctly, consider using NB_USE_LEGACY_ROUTING=true or setting net.ipv4.conf.*.rp_filter to 2 (loose) or 0 (off)") + } + // No need to check if routes exist as main table takes precedence over the VPN table via Rule 1 // TODO remove this once we have ipv6 support @@ -332,22 +363,8 @@ func flushRoutes(tableID, family int) error { } func enableIPForwarding() error { - bytes, err := os.ReadFile(ipv4ForwardingPath) - if err != nil { - return fmt.Errorf("read file %s: %w", ipv4ForwardingPath, err) - } - - // check if it is already enabled - // see more: https://github.com/netbirdio/netbird/issues/872 - if len(bytes) > 0 && bytes[0] == 49 { - return nil - } - - //nolint:gosec - if err := os.WriteFile(ipv4ForwardingPath, []byte("1"), 0644); err != nil { - return fmt.Errorf("write file %s: %w", ipv4ForwardingPath, err) - } - return nil + _, err := setSysctl(ipv4ForwardingPath, 1, false) + return err } // entryExists checks if the specified ID or name already exists in the rt_tables file @@ -475,3 +492,83 @@ func getAddressFamily(prefix netip.Prefix) int { } return netlink.FAMILY_V6 } + +// setupSysctl configures sysctl settings for RP filtering and source validation. +func setupSysctl(wgIface *iface.WGIface) (map[string]int, error) { + keys := map[string]int{} + var result *multierror.Error + + oldVal, err := setSysctl(srcValidMarkPath, 1, false) + if err != nil { + result = multierror.Append(result, err) + } else { + keys[srcValidMarkPath] = oldVal + } + + oldVal, err = setSysctl(rpFilterPath, 2, true) + if err != nil { + result = multierror.Append(result, err) + } else { + keys[rpFilterPath] = oldVal + } + + interfaces, err := net.Interfaces() + if err != nil { + result = multierror.Append(result, fmt.Errorf("list interfaces: %w", err)) + } + + for _, intf := range interfaces { + if intf.Name == "lo" || wgIface != nil && intf.Name == wgIface.Name() { + continue + } + + i := fmt.Sprintf(rpFilterInterfacePath, intf.Name) + oldVal, err := setSysctl(i, 2, true) + if err != nil { + result = multierror.Append(result, err) + } else { + keys[i] = oldVal + } + } + + return keys, result.ErrorOrNil() +} + +// setSysctl sets a sysctl configuration, if onlyIfOne is true it will only set the new value if it's set to 1 +func setSysctl(key string, desiredValue int, onlyIfOne bool) (int, error) { + path := fmt.Sprintf("/proc/sys/%s", strings.ReplaceAll(key, ".", "/")) + currentValue, err := os.ReadFile(path) + if err != nil { + return -1, fmt.Errorf("read sysctl %s: %w", key, err) + } + + currentV, err := strconv.Atoi(strings.TrimSpace(string(currentValue))) + if err != nil && len(currentValue) > 0 { + return -1, fmt.Errorf("convert current desiredValue to int: %w", err) + } + + if currentV == desiredValue || onlyIfOne && currentV != 1 { + return currentV, nil + } + + //nolint:gosec + if err := os.WriteFile(path, []byte(strconv.Itoa(desiredValue)), 0644); err != nil { + return currentV, fmt.Errorf("write sysctl %s: %w", key, err) + } + log.Debugf("Set sysctl %s from %d to %d", key, currentV, desiredValue) + + return currentV, nil +} + +func cleanupSysctl(originalSettings map[string]int) error { + var result *multierror.Error + + for key, value := range originalSettings { + _, err := setSysctl(key, value, false) + if err != nil { + result = multierror.Append(result, err) + } + } + + return result.ErrorOrNil() +} diff --git a/client/internal/routemanager/systemops_test.go b/client/internal/routemanager/systemops_test.go index 97386f19a1a..9f906c06fbe 100644 --- a/client/internal/routemanager/systemops_test.go +++ b/client/internal/routemanager/systemops_test.go @@ -61,7 +61,7 @@ func TestAddRemoveRoutes(t *testing.T) { err = wgInterface.Create() require.NoError(t, err, "should create testing wireguard interface") - _, _, err = setupRouting(nil, nil) + _, _, err = setupRouting(nil, wgInterface) require.NoError(t, err) t.Cleanup(func() { assert.NoError(t, cleanupRouting())