From 309ddef85205c341c3eb0afdcb886e0c738d833e Mon Sep 17 00:00:00 2001 From: Maisem Ali <3953239+maisem@users.noreply.github.com> Date: Mon, 28 Mar 2022 10:24:11 -0700 Subject: [PATCH] net/netutil: add CheckIPForwardingLinux (#4301) Combine the code between `LocalBackend.CheckIPForwarding` and `controlclient.ipForwardingBroken`. Fixes #4300 Signed-off-by: Maisem Ali --- control/controlclient/direct.go | 90 ++----------- ipn/ipnlocal/local.go | 99 +------------- net/netutil/ip_forward.go | 221 ++++++++++++++++++++++++++++++++ net/tsaddr/tsaddr.go | 3 + 4 files changed, 238 insertions(+), 175 deletions(-) create mode 100644 net/netutil/ip_forward.go diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index d14910e9f..e84ee68d6 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -18,7 +18,6 @@ import ( "net/http" "net/url" "os" - "os/exec" "reflect" "runtime" "strings" @@ -39,6 +38,7 @@ import ( "tailscale.com/net/dnsfallback" "tailscale.com/net/interfaces" "tailscale.com/net/netns" + "tailscale.com/net/netutil" "tailscale.com/net/tlsdial" "tailscale.com/net/tshttpproxy" "tailscale.com/tailcfg" @@ -1151,89 +1151,17 @@ func TrimWGConfig() opt.Bool { // // It should not return false positives. // -// TODO(bradfitz): merge this code into LocalBackend.CheckIPForwarding -// and change controlclient.Options.SkipIPForwardingCheck into a -// func([]netaddr.IPPrefix) error signature instead. Then we only have -// one copy of this code. +// TODO(bradfitz): Change controlclient.Options.SkipIPForwardingCheck into a +// func([]netaddr.IPPrefix) error signature instead. func ipForwardingBroken(routes []netaddr.IPPrefix, state *interfaces.State) bool { - if len(routes) == 0 { - // Nothing to route, so no need to warn. - return false - } - - if runtime.GOOS != "linux" { - // We only do subnet routing on Linux for now. - // It might work on darwin/macOS when building from source, so - // don't return true for other OSes. We can OS-based warnings - // already in the admin panel. + warn, err := netutil.CheckIPForwarding(routes, state) + if err != nil { + // Oh well, we tried. This is just for debugging. + // We don't want false positives. + // TODO: maybe we want a different warning for inability to check? return false } - - localIPs := map[netaddr.IP]bool{} - for _, addrs := range state.InterfaceIPs { - for _, pfx := range addrs { - localIPs[pfx.IP()] = true - } - } - - v4Routes, v6Routes := false, false - for _, r := range routes { - // It's possible to advertise a route to one of the local - // machine's local IPs. IP forwarding isn't required for this - // to work, so we shouldn't warn for such exports. - if r.IsSingleIP() && localIPs[r.IP()] { - continue - } - if r.IP().Is4() { - v4Routes = true - } else { - v6Routes = true - } - } - - if v4Routes { - out, err := ioutil.ReadFile("/proc/sys/net/ipv4/ip_forward") - if err != nil { - // Try another way. - out, err = exec.Command("sysctl", "-n", "net.ipv4.ip_forward").Output() - } - if err != nil { - // Oh well, we tried. This is just for debugging. - // We don't want false positives. - // TODO: maybe we want a different warning for inability to check? - return false - } - if strings.TrimSpace(string(out)) == "0" { - return true - } - } - if v6Routes { - // Note: you might be wondering why we check only the state of - // conf.all.forwarding, rather than per-interface forwarding - // configuration. According to kernel documentation, it seems - // that to actually forward packets, you need to enable - // forwarding globally, and the per-interface forwarding - // setting only alters other things such as how router - // advertisements are handled. The kernel itself warns that - // enabling forwarding per-interface and not globally will - // probably not work, so I feel okay calling those configs - // broken until we have proof otherwise. - out, err := ioutil.ReadFile("/proc/sys/net/ipv6/conf/all/forwarding") - if err != nil { - out, err = exec.Command("sysctl", "-n", "net.ipv6.conf.all.forwarding").Output() - } - if err != nil { - // Oh well, we tried. This is just for debugging. - // We don't want false positives. - // TODO: maybe we want a different warning for inability to check? - return false - } - if strings.TrimSpace(string(out)) == "0" { - return true - } - } - - return false + return warn != nil } // isUniquePingRequest reports whether pr contains a new PingRequest.URL diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 339158c49..12b6fad11 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -5,7 +5,6 @@ package ipnlocal import ( - "bytes" "context" "errors" "fmt" @@ -13,7 +12,6 @@ import ( "net" "net/http" "os" - "os/exec" "os/user" "path/filepath" "runtime" @@ -35,6 +33,7 @@ import ( "tailscale.com/ipn/policy" "tailscale.com/net/dns" "tailscale.com/net/interfaces" + "tailscale.com/net/netutil" "tailscale.com/net/tsaddr" "tailscale.com/net/tsdial" "tailscale.com/paths" @@ -3038,105 +3037,17 @@ func nodeIP(n *tailcfg.Node, pred func(netaddr.IP) bool) netaddr.IP { return netaddr.IP{} } -func isBSD(s string) bool { - return s == "dragonfly" || s == "freebsd" || s == "netbsd" || s == "openbsd" -} - func (b *LocalBackend) CheckIPForwarding() error { if wgengine.IsNetstackRouter(b.e) { return nil } - switch { - case isBSD(runtime.GOOS): - return fmt.Errorf("Subnet routing and exit nodes only work with additional manual configuration on %v, and is not currently officially supported.", runtime.GOOS) - case runtime.GOOS == "linux": - return checkIPForwardingLinux() - default: - // TODO: subnet routing and exit nodes probably don't work - // correctly on non-linux, non-netstack OSes either. Warn - // instead of being silent? - return nil - } -} - -// checkIPForwardingLinux checks if IP forwarding is enabled correctly -// for subnet routing and exit node functionality. Returns an error -// describing configuration issues if the configuration is not -// definitely good. -func checkIPForwardingLinux() error { - const kbLink = "\nSee https://tailscale.com/kb/1104/enable-ip-forwarding/" - - disabled, err := disabledSysctls("net.ipv4.ip_forward", "net.ipv6.conf.all.forwarding") + // TODO: let the caller pass in the ranges. + warn, err := netutil.CheckIPForwarding(tsaddr.ExitRoutes(), nil) if err != nil { - return fmt.Errorf("Couldn't check system's IP forwarding configuration, subnet routing/exit nodes may not work: %w%s", err, kbLink) - } - - if len(disabled) == 0 { - // IP forwarding is enabled systemwide, all is well. - return nil - } - - // IP forwarding isn't enabled globally, but it might be enabled - // on a per-interface basis. Check if it's on for all interfaces, - // and warn appropriately if it's not. - ifaces, err := interfaces.GetList() - if err != nil { - return fmt.Errorf("Couldn't enumerate network interfaces, subnet routing/exit nodes may not work: %w%s", err, kbLink) - } - - var ( - warnings []string - anyEnabled bool - ) - for _, iface := range ifaces { - if iface.Name == "lo" { - continue - } - disabled, err = disabledSysctls(fmt.Sprintf("net.ipv4.conf.%s.forwarding", iface.Name), fmt.Sprintf("net.ipv6.conf.%s.forwarding", iface.Name)) - if err != nil { - return fmt.Errorf("Couldn't check system's IP forwarding configuration, subnet routing/exit nodes may not work: %w%s", err, kbLink) - } - if len(disabled) > 0 { - warnings = append(warnings, fmt.Sprintf("Traffic received on %s won't be forwarded (%s disabled)", iface.Name, strings.Join(disabled, ", "))) - } else { - anyEnabled = true - } - } - if !anyEnabled { - // IP forwarding is compeltely disabled, just say that rather - // than enumerate all the interfaces on the system. - return fmt.Errorf("IP forwarding is disabled, subnet routing/exit nodes will not work.%s", kbLink) - } - if len(warnings) > 0 { - // If partially enabled, enumerate the bits that won't work. - return fmt.Errorf("%s\nSubnet routes and exit nodes may not work correctly.%s", strings.Join(warnings, "\n"), kbLink) - } - - return nil -} - -// disabledSysctls checks if the given sysctl keys are off, according -// to strconv.ParseBool. Returns a list of keys that are disabled, or -// err if something went wrong which prevented the lookups from -// completing. -func disabledSysctls(sysctls ...string) (disabled []string, err error) { - for _, k := range sysctls { - // TODO: on linux, we can get at these values via /proc/sys, - // rather than fork subcommands that may not be installed. - bs, err := exec.Command("sysctl", "-n", k).Output() - if err != nil { - return nil, fmt.Errorf("couldn't check %s (%v)", k, err) - } - on, err := strconv.ParseBool(string(bytes.TrimSpace(bs))) - if err != nil { - return nil, fmt.Errorf("couldn't parse %s (%v)", k, err) - } - if !on { - disabled = append(disabled, k) - } + return err } - return disabled, nil + return warn } // DERPMap returns the current DERPMap in use, or nil if not connected. diff --git a/net/netutil/ip_forward.go b/net/netutil/ip_forward.go new file mode 100644 index 000000000..03f791a86 --- /dev/null +++ b/net/netutil/ip_forward.go @@ -0,0 +1,221 @@ +// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package netutil contains misc shared networking code & types. +package netutil + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strconv" + "strings" + + "inet.af/netaddr" + "tailscale.com/net/interfaces" +) + +// protocolsRequiredForForwarding reports whether IPv4 and/or IPv6 protocols are +// required to forward the specified routes. +// The state param must be specified. +func protocolsRequiredForForwarding(routes []netaddr.IPPrefix, state *interfaces.State) (v4, v6 bool) { + if len(routes) == 0 { + // Nothing to route, so no need to warn. + return false, false + } + + localIPs := make(map[netaddr.IP]bool) + for _, addrs := range state.InterfaceIPs { + for _, pfx := range addrs { + localIPs[pfx.IP()] = true + } + } + + for _, r := range routes { + // It's possible to advertise a route to one of the local + // machine's local IPs. IP forwarding isn't required for this + // to work, so we shouldn't warn for such exports. + if r.IsSingleIP() && localIPs[r.IP()] { + continue + } + if r.IP().Is4() { + v4 = true + } else { + v6 = true + } + } + return v4, v6 +} + +// CheckIPForwarding reports whether IP forwarding is enabled correctly +// for subnet routing and exit node functionality on any interface. +// The state param can be nil, in which case interfaces.GetState is used. +// The routes should only be advertised routes, and should not contain the +// nodes Tailscale IPs. +// It returns an error if it is unable to determine if IP forwarding is enabled. +// It returns a warning describing configuration issues if IP forwarding is +// non-functional or partly functional. +func CheckIPForwarding(routes []netaddr.IPPrefix, state *interfaces.State) (warn, err error) { + if runtime.GOOS != "linux" { + switch runtime.GOOS { + case "dragonfly", "freebsd", "netbsd", "openbsd": + return fmt.Errorf("Subnet routing and exit nodes only work with additional manual configuration on %v, and is not currently officially supported.", runtime.GOOS), nil + } + return nil, nil + } + const kbLink = "\nSee https://tailscale.com/kb/1104/enable-ip-forwarding/" + if state == nil { + var err error + state, err = interfaces.GetState() + if err != nil { + return nil, err + } + } + wantV4, wantV6 := protocolsRequiredForForwarding(routes, state) + if !wantV4 && !wantV6 { + return nil, nil + } + + v4e, err := ipForwardingEnabledLinux(ipv4, "") + if err != nil { + return nil, fmt.Errorf("Couldn't check system's IP forwarding configuration, subnet routing/exit nodes may not work: %w%s", err, kbLink) + } + v6e, err := ipForwardingEnabledLinux(ipv6, "") + if err != nil { + return nil, fmt.Errorf("Couldn't check system's IP forwarding configuration, subnet routing/exit nodes may not work: %w%s", err, kbLink) + } + + if v4e && v6e { + // IP forwarding is enabled systemwide, all is well. + return nil, nil + } + + if !wantV4 { + if !v6e { + return nil, fmt.Errorf("IPv6 forwarding is disabled, subnet routing/exit nodes may not work.%s", kbLink) + } + return nil, nil + } + // IP forwarding isn't enabled globally, but it might be enabled + // on a per-interface basis. Check if it's on for all interfaces, + // and warn appropriately if it's not. + // Note: you might be wondering why we check only the state of + // ipv6.conf.all.forwarding, rather than per-interface forwarding + // configuration. According to kernel documentation, it seems + // that to actually forward packets, you need to enable + // forwarding globally, and the per-interface forwarding + // setting only alters other things such as how router + // advertisements are handled. The kernel itself warns that + // enabling forwarding per-interface and not globally will + // probably not work, so I feel okay calling those configs + // broken until we have proof otherwise. + var ( + anyEnabled bool + warnings []string + ) + if wantV6 && !v6e { + warnings = append(warnings, "IPv6 forwarding is disabled.") + } + for _, iface := range state.Interface { + if iface.Name == "lo" { + continue + } + v4e, err := ipForwardingEnabledLinux(ipv4, iface.Name) + if err != nil { + return nil, fmt.Errorf("Couldn't check system's IP forwarding configuration, subnet routing/exit nodes may not work: %w%s", err, kbLink) + } else if !v4e { + warnings = append(warnings, fmt.Sprintf("Traffic received on %s won't be forwarded (%s disabled)", iface.Name, ipForwardSysctlKey(dotFormat, ipv4, iface.Name))) + } else { + anyEnabled = true + } + } + if !anyEnabled { + // IP forwarding is completely disabled, just say that rather + // than enumerate all the interfaces on the system. + return fmt.Errorf("IP forwarding is disabled, subnet routing/exit nodes will not work.%s", kbLink), nil + } + if len(warnings) > 0 { + // If partially enabled, enumerate the bits that won't work. + return fmt.Errorf("%s\nSubnet routes and exit nodes may not work correctly.%s", strings.Join(warnings, "\n"), kbLink), nil + } + + return nil, nil +} + +// ipForwardSysctlKey returns the sysctl key for the given protocol and iface. +// When the dotFormat parameter is true the output is formatted as `net.ipv4.ip_forward`, +// else it is `net/ipv4/ip_forward` +func ipForwardSysctlKey(format sysctlFormat, p protocol, iface string) string { + if iface == "" { + if format == dotFormat { + if p == ipv4 { + return "net.ipv4.ip_forward" + } + return "net.ipv6.conf.all.forwarding" + } + if p == ipv4 { + return "net/ipv4/ip_forward" + } + return "net/ipv6/conf/all/forwarding" + } + + var k string + if p == ipv4 { + k = "net/ipv4/conf/%s/forwarding" + } else { + k = "net/ipv6/conf/%s/forwarding" + } + if format == dotFormat { + // Swap the delimiters. + iface = strings.ReplaceAll(iface, ".", "/") + k = strings.ReplaceAll(k, "/", ".") + } + return fmt.Sprintf(k, iface) +} + +type sysctlFormat int + +const ( + dotFormat sysctlFormat = iota + slashFormat +) + +type protocol int + +const ( + ipv4 protocol = iota + ipv6 +) + +// ipForwardingEnabledLinux reports whether the IP Forwarding is enabled for the +// given interface. +// The iface param determines which interface to check against, "" means to check +// global config. +// It tries to lookup the value directly from `/proc/sys`, and fallsback to +// using `sysctl` on failure. +func ipForwardingEnabledLinux(p protocol, iface string) (bool, error) { + k := ipForwardSysctlKey(slashFormat, p, iface) + bs, err := os.ReadFile(filepath.Join("/proc/sys", k)) + if err != nil { + // Fallback to using sysctl. + // Sysctl accepts `/` as separator. + bs, err = exec.Command("sysctl", "-n", k).Output() + if err != nil { + // But in case it doesn't. + k := ipForwardSysctlKey(dotFormat, p, iface) + bs, err = exec.Command("sysctl", "-n", k).Output() + if err != nil { + return false, fmt.Errorf("couldn't check %s (%v)", k, err) + } + } + } + on, err := strconv.ParseBool(string(bytes.TrimSpace(bs))) + if err != nil { + return false, fmt.Errorf("couldn't parse %s (%v)", k, err) + } + return on, nil +} diff --git a/net/tsaddr/tsaddr.go b/net/tsaddr/tsaddr.go index 5d23d717b..c582163ce 100644 --- a/net/tsaddr/tsaddr.go +++ b/net/tsaddr/tsaddr.go @@ -238,3 +238,6 @@ func AllIPv4() netaddr.IPPrefix { return allIPv4 } // AllIPv6 returns ::/0. func AllIPv6() netaddr.IPPrefix { return allIPv6 } + +// ExitRoutes returns a slice containing AllIPv4 and AllIPv6. +func ExitRoutes() []netaddr.IPPrefix { return []netaddr.IPPrefix{allIPv4, allIPv6} }