From becce82246e8a4929532ac5f13ddecd52170adc7 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Sun, 31 May 2020 14:35:30 -0700
Subject: [PATCH] net/netns, misc tests: remove TestOnlySkipPrivilegedOps, argv
 checks

The netns UID check is sufficient for now. We can do something else
later if/when needed.
---
 derp/derphttp/derphttp_test.go       |  5 ----
 net/netcheck/netcheck_test.go        |  5 ----
 net/netns/netns.go                   | 15 +-----------
 net/netns/netns_linux.go             | 36 +++++++---------------------
 wgengine/magicsock/magicsock_test.go |  5 ----
 wgengine/watchdog_test.go            |  5 ----
 6 files changed, 10 insertions(+), 61 deletions(-)

diff --git a/derp/derphttp/derphttp_test.go b/derp/derphttp/derphttp_test.go
index 2a700fba6..ffb5348ad 100644
--- a/derp/derphttp/derphttp_test.go
+++ b/derp/derphttp/derphttp_test.go
@@ -15,14 +15,9 @@ import (
 	"time"
 
 	"tailscale.com/derp"
-	"tailscale.com/net/netns"
 	"tailscale.com/types/key"
 )
 
-func init() {
-	netns.TestOnlySkipPrivilegedOps()
-}
-
 func TestSendRecv(t *testing.T) {
 	const numClients = 3
 	var serverPrivateKey key.Private
diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go
index 489697e5c..6cf0b8092 100644
--- a/net/netcheck/netcheck_test.go
+++ b/net/netcheck/netcheck_test.go
@@ -16,16 +16,11 @@ import (
 	"time"
 
 	"tailscale.com/net/interfaces"
-	"tailscale.com/net/netns"
 	"tailscale.com/net/stun"
 	"tailscale.com/net/stun/stuntest"
 	"tailscale.com/tailcfg"
 )
 
-func init() {
-	netns.TestOnlySkipPrivilegedOps()
-}
-
 func TestHairpinSTUN(t *testing.T) {
 	tx := stun.NewTxID()
 	c := &Client{
diff --git a/net/netns/netns.go b/net/netns/netns.go
index a48859a1d..e45680e45 100644
--- a/net/netns/netns.go
+++ b/net/netns/netns.go
@@ -11,13 +11,7 @@
 // operating system, and perhaps even by version of the OS.
 package netns
 
-import (
-	"net"
-
-	"tailscale.com/syncs"
-)
-
-var skipPrivileged syncs.AtomicBool
+import "net"
 
 // Listener returns a new net.Listener with its Control hook func
 // initialized as necessary to run in logical network namespace that
@@ -32,10 +26,3 @@ func Listener() *net.ListenConfig {
 func Dialer() *net.Dialer {
 	return &net.Dialer{Control: control}
 }
-
-// TestOnlySkipPrivilegedOps disables any behavior in this package
-// that requires root or other elevated privileges. It's used only in
-// tests, and using it definitely breaks some Tailscale functionality.
-func TestOnlySkipPrivilegedOps() {
-	skipPrivileged.Set(true)
-}
diff --git a/net/netns/netns_linux.go b/net/netns/netns_linux.go
index 09fc1982e..75d24eaff 100644
--- a/net/netns/netns_linux.go
+++ b/net/netns/netns_linux.go
@@ -6,11 +6,11 @@ package netns
 
 import (
 	"errors"
+	"flag"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 	"sync"
 	"syscall"
@@ -72,30 +72,18 @@ func defaultRouteInterface() (string, error) {
 // ignoreErrors returns true if we should ignore setsocketopt errors in
 // this instance.
 func ignoreErrors() bool {
+	// If we're in a test, ignore errors. Assume the test knows
+	// what it's doing and will do its own skips or permission
+	// checks if it's setting up a world that needs netns to work.
+	// But by default, assume that tests don't need netns and it's
+	// harmless to ignore the sockopts failing.
+	if flag.CommandLine.Lookup("test.v") != nil {
+		return true
+	}
 	if os.Getuid() != 0 {
 		// only root can manipulate these socket flags
 		return true
 	}
-
-	// TODO(apenwarr): this snooping around in the args is way too magic.
-	//  It would be better to explicitly activate, or not, this dialer
-	//  by passing it from the toplevel program.
-	v, _ := os.Executable()
-	switch filepath.Base(v) {
-	case "tailscale":
-		for _, arg := range os.Args {
-			if arg == "netcheck" {
-				return true
-			}
-		}
-	case "tailscaled":
-		for _, arg := range os.Args {
-			if arg == "-fake" || arg == "--fake" {
-				return true
-			}
-		}
-	}
-
 	return false
 }
 
@@ -104,12 +92,6 @@ func ignoreErrors() bool {
 // It's intentionally the same signature as net.Dialer.Control
 // and net.ListenConfig.Control.
 func control(network, address string, c syscall.RawConn) error {
-	if skipPrivileged.Get() {
-		// We can't set socket marks without CAP_NET_ADMIN on linux,
-		// skip as requested.
-		return nil
-	}
-
 	var sockErr error
 	err := c.Control(func(fd uintptr) {
 		if ipRuleAvailable() {
diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go
index 37a44e77e..859fb5b00 100644
--- a/wgengine/magicsock/magicsock_test.go
+++ b/wgengine/magicsock/magicsock_test.go
@@ -26,7 +26,6 @@ import (
 	"tailscale.com/derp"
 	"tailscale.com/derp/derphttp"
 	"tailscale.com/derp/derpmap"
-	"tailscale.com/net/netns"
 	"tailscale.com/net/stun/stuntest"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
@@ -36,10 +35,6 @@ import (
 	"tailscale.com/wgengine/tstun"
 )
 
-func init() {
-	netns.TestOnlySkipPrivilegedOps()
-}
-
 // WaitReady waits until the magicsock is entirely initialized and connected
 // to its home DERP server. This is normally not necessary, since magicsock
 // is intended to be entirely asynchronous, but it helps eliminate race
diff --git a/wgengine/watchdog_test.go b/wgengine/watchdog_test.go
index 7bfdc23e1..0e45dd641 100644
--- a/wgengine/watchdog_test.go
+++ b/wgengine/watchdog_test.go
@@ -11,15 +11,10 @@ import (
 	"testing"
 	"time"
 
-	"tailscale.com/net/netns"
 	"tailscale.com/wgengine/router"
 	"tailscale.com/wgengine/tstun"
 )
 
-func init() {
-	netns.TestOnlySkipPrivilegedOps()
-}
-
 func TestWatchdog(t *testing.T) {
 	t.Parallel()