ipn: plumb NetfilterMode all the way out to the CLI.

Signed-off-by: David Anderson <danderson@tailscale.com>

cmd/tailscale/tailscale.go

@@ -28,6 +28,7 @@ import (
 	"tailscale.com/paths"
 	"tailscale.com/safesocket"
 	"tailscale.com/tailcfg"
+	"tailscale.com/wgengine/router"
 )
 
 // globalStateKey is the ipn.StateKey that tailscaled loads on
@@ -59,8 +60,7 @@ func main() {
 	if runtime.GOOS == "linux" {
 		upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. 10.0.0.0/8,192.168.0.0/24)")
 		upf.BoolVar(&upArgs.noSNAT, "no-snat", false, "disable SNAT of traffic to local routes advertised with -advertise-routes")
-		upf.BoolVar(&upArgs.noNetfilterCalls, "no-netfilter-calls", false, "don't call Tailscale netfilter chains from the main netfilter chains")
-		upf.BoolVar(&upArgs.noNetfilter, "no-netfilter", false, "disable all netfilter rule management")
+		upf.StringVar(&upArgs.netfilterMode, "netfilter-mode", "on", "netfilter mode (one of on, nodivert, off)")
 	}
 	upCmd := &ffcli.Command{
 		Name:       "up",
@@ -111,8 +111,7 @@ var upArgs struct {
 	advertiseRoutes string
 	advertiseTags   string
 	noSNAT          bool
-	noNetfilterCalls bool
-	noNetfilter      bool
+	netfilterMode   string
 	authKey         string
 }
 
@@ -139,6 +138,10 @@ func parseIPOrCIDR(s string) (wgcfg.CIDR, bool) {
 	}
 }
 
+func warning(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() {
@@ -147,16 +150,16 @@ func checkIPForwarding() {
 	}
 	bs, err := ioutil.ReadFile("/proc/sys/net/ipv4/ip_forward")
 	if err != nil {
-		fmt.Printf("Warning: couldn't check if IP forwarding is enabled (%v). IP forwarding must be enabled for subnet routes to work.", err)
+		warning("couldn't check if IP forwarding is enabled (%v). IP forwarding must be enabled for subnet routes to work.", err)
 		return
 	}
 	on, err := strconv.ParseBool(string(bytes.TrimSpace(bs)))
 	if err != nil {
-		fmt.Printf("Warning: couldn't check if IP forwarding is enabled (%v). IP forwarding must be enabled for subnet routes to work.", err)
+		warning("couldn't check if IP forwarding is enabled (%v). IP forwarding must be enabled for subnet routes to work.", err)
 		return
 	}
 	if !on {
-		fmt.Printf("Warning: IP forwarding is disabled, subnet routes will not work.")
+		warning("IP forwarding is disabled, subnet routes will not work.")
 	}
 }
 
@@ -200,8 +203,20 @@ func runUp(ctx context.Context, args []string) error {
 	prefs.AdvertiseRoutes = routes
 	prefs.AdvertiseTags = tags
 	prefs.NoSNAT = upArgs.noSNAT
-	prefs.NoNetfilter = upArgs.noNetfilter
-	prefs.NoNetfilterCalls = upArgs.noNetfilterCalls
+	if runtime.GOOS == "linux" {
+		switch upArgs.netfilterMode {
+		case "on":
+			prefs.NetfilterMode = router.NetfilterOn
+		case "nodivert":
+			prefs.NetfilterMode = router.NetfilterNoDivert
+			warning("netfilter in nodivert mode, you must add calls to Tailscale netfilter chains manually")
+		case "off":
+			prefs.NetfilterMode = router.NetfilterOff
+			warning("netfilter management disabled, you must write a secure packet filter yourself")
+		default:
+			log.Fatalf("invalid value --netfilter-mode: %q", upArgs.netfilterMode)
+		}
+	}
 
 	c, bc, ctx, cancel := connect(ctx)
 	defer cancel()

ipn/local.go

@@ -739,14 +739,7 @@ func routerConfig(cfg *wgcfg.Config, prefs *Prefs, dnsDomains []string) *router.
 		DNSDomains:       dnsDomains,
 		SubnetRoutes:     wgCIDRToNetaddr(prefs.AdvertiseRoutes),
 		SNATSubnetRoutes: !prefs.NoSNAT,
-	}
-	switch {
-	case prefs.NoNetfilter:
-		rs.NetfilterMode = router.NetfilterOff
-	case prefs.NoNetfilterCalls:
-		rs.NetfilterMode = router.NetfilterNoDivert
-	default:
-		rs.NetfilterMode = router.NetfilterOn
+		NetfilterMode:    prefs.NetfilterMode,
 	}
 
 	for _, peer := range cfg.Peers {

ipn/prefs.go

@@ -15,6 +15,7 @@ import (
 	"github.com/tailscale/wireguard-go/wgcfg"
 	"tailscale.com/atomicfile"
 	"tailscale.com/control/controlclient"
+	"tailscale.com/wgengine/router"
 )
 
 // Prefs are the user modifiable settings of the Tailscale node agent.
@@ -79,16 +80,9 @@ type Prefs struct {
 	//
 	// Linux-only.
 	NoSNAT bool
-	// NoNetfilter, if set, disables all management of firewall rules
-	// for Tailscale traffic. The resulting configuration is not
-	// secure, and it is the user's responsibility to correct that.
-	NoNetfilter bool
-	// NoNetfilterDivert, if set, disables calling Tailscale netfilter
-	// chains from the main netfilter chains, but still manages the
-	// contents of the Tailscale chains. The resulting configuration
-	// is not secure, and it is the user's responsibility to insert
-	// calls to Tailscale's chains at the right place.
-	NoNetfilterCalls bool
+	// NetfilterMode specifies how much to manage netfilter rules for
+	// Tailscale, if at all.
+	NetfilterMode router.NetfilterMode
 
 	// The Persist field is named 'Config' in the file for backward
 	// compatibility with earlier versions.
@@ -108,9 +102,9 @@ func (p *Prefs) Pretty() string {
 	} else {
 		pp = "Persist=nil"
 	}
-	return fmt.Sprintf("Prefs{ra=%v mesh=%v dns=%v want=%v notepad=%v derp=%v shields=%v routes=%v snat=%v nf=%v nfd=%v %v}",
+	return fmt.Sprintf("Prefs{ra=%v mesh=%v dns=%v want=%v notepad=%v derp=%v shields=%v routes=%v snat=%v nf=%v %v}",
 		p.RouteAll, p.AllowSingleHosts, p.CorpDNS, p.WantRunning,
-		p.NotepadURLs, !p.DisableDERP, p.ShieldsUp, p.AdvertiseRoutes, !p.NoSNAT, !p.NoNetfilter, !p.NoNetfilterCalls, pp)
+		p.NotepadURLs, !p.DisableDERP, p.ShieldsUp, p.AdvertiseRoutes, !p.NoSNAT, p.NetfilterMode, pp)
 }
 
 func (p *Prefs) ToBytes() []byte {
@@ -139,8 +133,7 @@ func (p *Prefs) Equals(p2 *Prefs) bool {
 		p.DisableDERP == p2.DisableDERP &&
 		p.ShieldsUp == p2.ShieldsUp &&
 		p.NoSNAT == p2.NoSNAT &&
-		p.NoNetfilter == p2.NoNetfilter &&
-		p.NoNetfilterCalls == p2.NoNetfilterCalls &&
+		p.NetfilterMode == p2.NetfilterMode &&
 		compareIPNets(p.AdvertiseRoutes, p2.AdvertiseRoutes) &&
 		compareStrings(p.AdvertiseTags, p2.AdvertiseTags) &&
 		p.Persist.Equals(p2.Persist)
@@ -180,6 +173,7 @@ func NewPrefs() *Prefs {
 		AllowSingleHosts: true,
 		CorpDNS:          true,
 		WantRunning:      true,
+		NetfilterMode:    router.NetfilterOn,
 	}
 }
 

ipn/prefs_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/tailscale/wireguard-go/wgcfg"
 	"tailscale.com/control/controlclient"
 	"tailscale.com/tstest"
+	"tailscale.com/wgengine/router"
 )
 
 func fieldsOf(t reflect.Type) (fields []string) {
@@ -23,7 +24,7 @@ func fieldsOf(t reflect.Type) (fields []string) {
 func TestPrefsEqual(t *testing.T) {
 	tstest.PanicOnLog()
 
-	prefsHandles := []string{"ControlURL", "RouteAll", "AllowSingleHosts", "CorpDNS", "WantRunning", "ShieldsUp", "AdvertiseTags", "NotepadURLs", "DisableDERP", "AdvertiseRoutes", "NoSNAT", "NoNetfilter", "NoNetfilterCalls", "Persist"}
+	prefsHandles := []string{"ControlURL", "RouteAll", "AllowSingleHosts", "CorpDNS", "WantRunning", "ShieldsUp", "AdvertiseTags", "NotepadURLs", "DisableDERP", "AdvertiseRoutes", "NoSNAT", "NetfilterMode", "Persist"}
 	if have := fieldsOf(reflect.TypeOf(Prefs{})); !reflect.DeepEqual(have, prefsHandles) {
 		t.Errorf("Prefs.Equal check might be out of sync\nfields: %q\nhandled: %q\n",
 			have, prefsHandles)
@@ -174,24 +175,13 @@ func TestPrefsEqual(t *testing.T) {
 		},
 
 		{
-			&Prefs{NoNetfilter: true},
-			&Prefs{NoNetfilter: false},
+			&Prefs{NetfilterMode: router.NetfilterOff},
+			&Prefs{NetfilterMode: router.NetfilterOn},
 			false,
 		},
 		{
-			&Prefs{NoNetfilter: true},
-			&Prefs{NoNetfilter: true},
-			true,
-		},
-
-		{
-			&Prefs{NoNetfilterCalls: true},
-			&Prefs{NoNetfilterCalls: false},
-			false,
-		},
-		{
-			&Prefs{NoNetfilterCalls: true},
-			&Prefs{NoNetfilterCalls: true},
+			&Prefs{NetfilterMode: router.NetfilterOn},
+			&Prefs{NetfilterMode: router.NetfilterOn},
 			true,
 		},
 

wgengine/router/router.go

@@ -35,6 +35,8 @@ func New(logf logger.Logf, wgdev *device.Device, tundev tun.Device) (Router, err
 	return newUserspaceRouter(logf, wgdev, tundev)
 }
 
+// NetfilterMode is the firewall management mode to use when
+// programming the Linux network stack.
 type NetfilterMode int
 
 const (
@@ -43,6 +45,19 @@ const (
 	NetfilterOn                            // manage tailscale chains and call them from main chains
 )
 
+func (m NetfilterMode) String() string {
+	switch m {
+	case NetfilterOff:
+		return "off"
+	case NetfilterNoDivert:
+		return "nodivert"
+	case NetfilterOn:
+		return "on"
+	default:
+		return "???"
+	}
+}
+
 // Config is the subset of Tailscale configuration that is relevant to
 // the OS's network stack.
 type Config struct {

wgengine/router/router_linux.go

@@ -718,6 +718,24 @@ func (r *linuxRouter) delNetfilterHooks() error {
 	del := func(table, chain string) error {
 		tsChain := tsChain(chain)
 
+		chains, err := r.ipt4.ListChains(table)
+		if err != nil {
+			return fmt.Errorf("listing iptables chains: %v", err)
+		}
+		found := false
+		for _, chain := range chains {
+			if chain == tsChain {
+				found = true
+				break
+			}
+		}
+		if !found {
+			// The divert rule can't exist if the chain doesn't exist,
+			// and querying for a jump to a non-existent chain errors
+			// out.
+			return nil
+		}
+
 		args := []string{"-j", tsChain}
 		exists, err := r.ipt4.Exists(table, chain, args...)
 		if err != nil {

wgengine/router/router_linux_test.go

@@ -50,6 +50,7 @@ ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10
 			name: "local addr only",
 			in: &Config{
 				LocalAddrs:    mustCIDRs("100.101.102.103/10"),
+				NetfilterMode: NetfilterOff,
 			},
 			want: `
 up
@@ -63,6 +64,7 @@ ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10
 			in: &Config{
 				LocalAddrs:    mustCIDRs("100.101.102.103/10"),
 				Routes:        mustCIDRs("100.100.100.100/32", "192.168.16.0/24"),
+				NetfilterMode: NetfilterOff,
 			},
 			want: `
 up
@@ -79,6 +81,7 @@ ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10
 				LocalAddrs:    mustCIDRs("100.101.102.103/10"),
 				Routes:        mustCIDRs("100.100.100.100/32", "192.168.16.0/24"),
 				SubnetRoutes:  mustCIDRs("200.0.0.0/8"),
+				NetfilterMode: NetfilterOff,
 			},
 			want: `
 up