cmd/tailscale, ipn/localapi: move IP forwarding check to tailscaled, API

Instead of having the CLI check whether IP forwarding is enabled, ask
tailscaled. It has a better idea. If it's netstack, for instance, the
sysctl values don't matter. And it's possible that only the daemon has
permission to know.

Fixes #1626

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

client/tailscale/tailscale.go

@@ -8,6 +8,7 @@
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -198,7 +199,34 @@ func GetWaitingFile(ctx context.Context, baseName string) (rc io.ReadCloser, siz
 	if res.StatusCode != 200 {
 		body, _ := ioutil.ReadAll(res.Body)
 		res.Body.Close()
-		return nil, 0, fmt.Errorf("expected 204 No Content; got HTTP %s: %s", res.Status, body)
+		return nil, 0, fmt.Errorf("HTTP %s: %s", res.Status, body)
 	}
 	return res.Body, res.ContentLength, nil
 }
+
+func CheckIPForwarding(ctx context.Context) error {
+	req, err := http.NewRequestWithContext(ctx, "GET", "http://local-tailscaled.sock/localapi/v0/check-ip-forwarding", nil)
+	if err != nil {
+		return err
+	}
+	res, err := DoLocalRequest(req)
+	if err != nil {
+		return err
+	}
+	defer res.Body.Close()
+	if res.StatusCode != 200 {
+		body, _ := ioutil.ReadAll(res.Body)
+		res.Body.Close()
+		return fmt.Errorf("HTTP %s: %s", res.Status, body)
+	}
+	var jres struct {
+		Warning string
+	}
+	if err := json.NewDecoder(res.Body).Decode(&jres); err != nil {
+		return fmt.Errorf("invalid JSON from check-ip-forwarding: %w", err)
+	}
+	if jres.Warning != "" {
+		return errors.New(jres.Warning)
+	}
+	return nil
+}

cmd/tailscale/cli/up.go

@@ -5,17 +5,14 @@
 package cli
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"flag"
 	"fmt"
 	"log"
 	"os"
-	"os/exec"
 	"runtime"
 	"sort"
-	"strconv"
 	"strings"
 	"sync"
 
@@ -97,34 +94,6 @@ func warnf(format string, args ...interface{}) {
 	fmt.Printf("Warning: "+format+"\n", args...)
 }
 
-// checkIPForwarding prints warnings on linux if IP forwarding is not
-// enabled, or if we were unable to verify the state of IP forwarding.
-func checkIPForwarding() {
-	var key string
-
-	if runtime.GOOS == "linux" {
-		key = "net.ipv4.ip_forward"
-	} else if isBSD(runtime.GOOS) {
-		key = "net.inet.ip.forwarding"
-	} else {
-		return
-	}
-
-	bs, err := exec.Command("sysctl", "-n", key).Output()
-	if err != nil {
-		warnf("couldn't check %s (%v).\nSubnet routes won't work without IP forwarding.", key, err)
-		return
-	}
-	on, err := strconv.ParseBool(string(bytes.TrimSpace(bs)))
-	if err != nil {
-		warnf("couldn't parse %s (%v).\nSubnet routes won't work without IP forwarding.", key, err)
-		return
-	}
-	if !on {
-		warnf("%s is disabled. Subnet routes won't work.", key)
-	}
-}
-
 var (
 	ipv4default = netaddr.MustParseIPPrefix("0.0.0.0/0")
 	ipv6default = netaddr.MustParseIPPrefix("::/0")
@@ -181,9 +150,8 @@ func runUp(ctx context.Context, args []string) error {
 		routeMap[netaddr.MustParseIPPrefix("::/0")] = true
 	}
 	if len(routeMap) > 0 {
-		checkIPForwarding()
+		if err := tailscale.CheckIPForwarding(context.Background()); err != nil {
-		if isBSD(runtime.GOOS) {
+			warnf("%v", err)
-			warnf("Subnet routing and exit nodes only work with additional manual configuration on %v, and is not currently officially supported.", runtime.GOOS)
 		}
 	}
 	routes := make([]netaddr.IPPrefix, 0, len(routeMap))

control/controlclient/direct.go

@@ -66,6 +66,7 @@ type Direct struct {
 	getMachinePrivKey      func() (wgkey.Private, error)
 	debugFlags             []string
 	keepSharerAndUserSplit bool
+	skipIPForwardingCheck  bool
 
 	mu           sync.Mutex // mutex guards the following fields
 	serverKey    wgkey.Key
@@ -98,6 +99,11 @@ type Options struct {
 	// KeepSharerAndUserSplit controls whether the client
 	// understands Node.Sharer. If false, the Sharer is mapped to the User.
 	KeepSharerAndUserSplit bool
+
+	// SkipIPForwardingCheck declares that the host's IP
+	// forwarding works and should not be double-checked by the
+	// controlclient package.
+	SkipIPForwardingCheck bool
 }
 
 type Decompressor interface {
@@ -159,6 +165,7 @@ func NewDirect(opts Options) (*Direct, error) {
 		debugFlags:             opts.DebugFlags,
 		keepSharerAndUserSplit: opts.KeepSharerAndUserSplit,
 		linkMon:                opts.LinkMonitor,
+		skipIPForwardingCheck:  opts.SkipIPForwardingCheck,
 	}
 	if opts.Hostinfo == nil {
 		c.SetHostinfo(NewHostinfo())
@@ -577,7 +584,8 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*netm
 		OmitPeers:  cb == nil,
 	}
 	var extraDebugFlags []string
-	if hostinfo != nil && c.linkMon != nil && ipForwardingBroken(hostinfo.RoutableIPs, c.linkMon.InterfaceState()) {
+	if hostinfo != nil && c.linkMon != nil && !c.skipIPForwardingCheck &&
+		ipForwardingBroken(hostinfo.RoutableIPs, c.linkMon.InterfaceState()) {
 		extraDebugFlags = append(extraDebugFlags, "warn-ip-forwarding-off")
 	}
 	if health.RouterHealth() != nil {
@@ -1181,6 +1189,11 @@ func TrimWGConfig() opt.Bool {
 // and will definitely not work for the routes provided.
 //
 // 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.
 func ipForwardingBroken(routes []netaddr.IPPrefix, state *interfaces.State) bool {
 	if len(routes) == 0 {
 		// Nothing to route, so no need to warn.

ipn/ipnlocal/local.go

@@ -12,6 +12,7 @@
 	"io"
 	"net"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"runtime"
 	"strconv"
@@ -640,6 +641,10 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		DiscoPublicKey:  discoPublic,
 		DebugFlags:      controlDebugFlags,
 		LinkMonitor:     b.e.GetLinkMonitor(),
+
+		// Don't warn about broken Linux IP forwading when
+		// netstack is being used.
+		SkipIPForwardingCheck: wgengine.IsNetstack(b.e),
 	})
 	if err != nil {
 		return err
@@ -2018,3 +2023,45 @@ func (b *LocalBackend) OpenFile(name string) (rc io.ReadCloser, size int64, err
 	}
 	return apiSrv.OpenFile(name)
 }
+
+func isBSD(s string) bool {
+	return s == "dragonfly" || s == "freebsd" || s == "netbsd" || s == "openbsd"
+}
+
+func (b *LocalBackend) CheckIPForwarding() error {
+	if wgengine.IsNetstack(b.e) {
+		return nil
+	}
+	if isBSD(runtime.GOOS) {
+		//lint:ignore ST1005 output to users as is
+		return fmt.Errorf("Subnet routing and exit nodes only work with additional manual configuration on %v, and is not currently officially supported.", runtime.GOOS)
+	}
+
+	var keys []string
+
+	if runtime.GOOS == "linux" {
+		keys = append(keys, "net.ipv4.ip_forward", "net.ipv6.conf.all.forwarding")
+	} else if isBSD(runtime.GOOS) {
+		keys = append(keys, "net.inet.ip.forwarding")
+	} else {
+		return nil
+	}
+
+	for _, key := range keys {
+		bs, err := exec.Command("sysctl", "-n", key).Output()
+		if err != nil {
+			//lint:ignore ST1005 output to users as is
+			return fmt.Errorf("couldn't check %s (%v).\nSubnet routes won't work without IP forwarding.", key, err)
+		}
+		on, err := strconv.ParseBool(string(bytes.TrimSpace(bs)))
+		if err != nil {
+			//lint:ignore ST1005 output to users as is
+			return fmt.Errorf("couldn't parse %s (%v).\nSubnet routes won't work without IP forwarding.", key, err)
+		}
+		if !on {
+			//lint:ignore ST1005 output to users as is
+			return fmt.Errorf("%s is disabled. Subnet routes won't work.", key)
+		}
+	}
+	return nil
+}

ipn/localapi/localapi.go

@@ -67,6 +67,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		h.serveGoroutines(w, r)
 	case "/localapi/v0/status":
 		h.serveStatus(w, r)
+	case "/localapi/v0/check-ip-forwarding":
+		h.serveCheckIPForwarding(w, r)
 	default:
 		io.WriteString(w, "tailscaled\n")
 	}
@@ -121,6 +123,23 @@ func (h *Handler) serveGoroutines(w http.ResponseWriter, r *http.Request) {
 	w.Write(buf)
 }
 
+func (h *Handler) serveCheckIPForwarding(w http.ResponseWriter, r *http.Request) {
+	if !h.PermitRead {
+		http.Error(w, "IP forwarding check access denied", http.StatusForbidden)
+		return
+	}
+	var warning string
+	if err := h.b.CheckIPForwarding(); err != nil {
+		warning = err.Error()
+	}
+	w.Header().Set("Content-Type", "application/json")
+	json.NewEncoder(w).Encode(struct {
+		Warning string
+	}{
+		Warning: warning,
+	})
+}
+
 func (h *Handler) serveStatus(w http.ResponseWriter, r *http.Request) {
 	if !h.PermitRead {
 		http.Error(w, "status access denied", http.StatusForbidden)