ipn/localapi: only perform local-admin check in serveServeConfig (#10163)

On unix systems, the check involves executing sudo, which is slow.
Instead of doing it for every incoming request, move the logic into
localapi serveServeConfig handler and do it as needed.

Updates tailscale/corp#15405

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

cmd/tailscaled/depaware.txt

@@ -362,7 +362,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
      💣 tailscale.com/util/osdiag                                    from tailscale.com/cmd/tailscaled+
    W 💣 tailscale.com/util/osdiag/internal/wsc                       from tailscale.com/util/osdiag
         tailscale.com/util/osshare                                   from tailscale.com/ipn/ipnlocal+
-  LD    tailscale.com/util/osuser                                    from tailscale.com/ssh/tailssh
+        tailscale.com/util/osuser                                    from tailscale.com/ssh/tailssh+
         tailscale.com/util/race                                      from tailscale.com/net/dns/resolver
         tailscale.com/util/racebuild                                 from tailscale.com/logpolicy
         tailscale.com/util/rands                                     from tailscale.com/ipn/ipnlocal+

ipn/ipnserver/server.go

@@ -11,9 +11,7 @@ import (
 	"io"
 	"net"
 	"net/http"
-	"os/exec"
 	"os/user"
-	"runtime"
 	"strconv"
 	"strings"
 	"sync"
@@ -32,7 +30,6 @@ import (
 	"tailscale.com/util/mak"
 	"tailscale.com/util/set"
 	"tailscale.com/util/systemd"
-	"tailscale.com/version"
 )
 
 // Server is an IPN backend and its set of 0 or more active localhost
@@ -205,7 +202,7 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) {
 		lah := localapi.NewHandler(lb, s.logf, s.netMon, s.backendLogID)
 		lah.PermitRead, lah.PermitWrite = s.localAPIPermissions(ci)
 		lah.PermitCert = s.connCanFetchCerts(ci)
-		lah.CallerIsLocalAdmin = s.connIsLocalAdmin(ci)
+		lah.ConnIdentity = ci
 		lah.ServeHTTP(w, r)
 		return
 	}
@@ -367,61 +364,6 @@ func (s *Server) connCanFetchCerts(ci *ipnauth.ConnIdentity) bool {
 	return false
 }
 
-// connIsLocalAdmin reports whether ci has administrative access to the local
-// machine, for whatever that means with respect to the current OS.
-//
-// This returns true only on Windows machines when the client user is elevated.
-// This is useful because, on Windows, tailscaled itself always runs with
-// elevated rights: we want to avoid privilege escalation for certain mutative operations.
-func (s *Server) connIsLocalAdmin(ci *ipnauth.ConnIdentity) bool {
-	switch runtime.GOOS {
-	case "windows":
-		tok, err := ci.WindowsToken()
-		if err != nil {
-			if !errors.Is(err, ipnauth.ErrNotImplemented) {
-				s.logf("ipnauth.ConnIdentity.WindowsToken() error: %v", err)
-			}
-			return false
-		}
-		defer tok.Close()
-
-		return tok.IsElevated()
-
-	case "darwin":
-		if version.IsSandboxedMacOS() {
-			return false
-		}
-		// This is a standalone tailscaled setup, use the same logic as on
-		// Linux.
-		fallthrough
-	case "linux":
-		uid, ok := ci.Creds().UserID()
-		if !ok {
-			return false
-		}
-		// root is always admin.
-		if uid == "0" {
-			return true
-		}
-		// if non-root, must be operator AND able to execute "sudo tailscale".
-		operatorUID := s.mustBackend().OperatorUserID()
-		if operatorUID != "" && uid != operatorUID {
-			return false
-		}
-		u, err := user.LookupId(uid)
-		if err != nil {
-			return false
-		}
-		if err := exec.Command("sudo", "--other-user="+u.Name, "--list", "tailscale").Run(); err != nil {
-			return false
-		}
-		return true
-
-	default:
-		return false
-	}
-}
-
 // addActiveHTTPRequest adds c to the server's list of active HTTP requests.
 //
 // If the returned error may be of type inUseOtherUserError.

ipn/localapi/localapi.go

@@ -18,6 +18,7 @@ import (
 	"net/http/httputil"
 	"net/netip"
 	"net/url"
+	"os/exec"
 	"runtime"
 	"slices"
 	"strconv"
@@ -30,6 +31,7 @@ import (
 	"tailscale.com/health"
 	"tailscale.com/hostinfo"
 	"tailscale.com/ipn"
+	"tailscale.com/ipn/ipnauth"
 	"tailscale.com/ipn/ipnlocal"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/logtail"
@@ -50,6 +52,7 @@ import (
 	"tailscale.com/util/httpm"
 	"tailscale.com/util/mak"
 	"tailscale.com/util/osdiag"
+	"tailscale.com/util/osuser"
 	"tailscale.com/util/rands"
 	"tailscale.com/version"
 	"tailscale.com/wgengine/magicsock"
@@ -159,16 +162,12 @@ type Handler struct {
 	// cert fetching access.
 	PermitCert bool
 
-	// CallerIsLocalAdmin is whether the this handler is being invoked as a
+	// ConnIdentity is the identity of the client connected to the Handler.
-	// result of a LocalAPI call from a user who is a local admin of the current
+	ConnIdentity *ipnauth.ConnIdentity
-	// machine.
+
-	//
+	// Test-only override for connIsLocalAdmin method. If non-nil,
-	// As of 2023-10-26 it is only populated on Windows.
+	// connIsLocalAdmin returns this value.
-	//
+	testConnIsLocalAdmin *bool
-	// It can be used to to restrict some LocalAPI operations which should only
-	// be run by an admin and not unprivileged users in a computing environment
-	// managed by IT admins.
-	CallerIsLocalAdmin bool
 
 	b            *ipnlocal.LocalBackend
 	logf         logger.Logf
@@ -942,16 +941,22 @@ func (h *Handler) serveServeConfig(w http.ResponseWriter, r *http.Request) {
 }
 
 func authorizeServeConfigForGOOSAndUserContext(goos string, configIn *ipn.ServeConfig, h *Handler) error {
-	if !slices.Contains([]string{"windows", "linux", "darwin"}, goos) {
+	switch goos {
+	case "windows", "linux", "darwin":
+	default:
 		return nil
 	}
-	if goos == "darwin" && !version.IsSandboxedMacOS() {
+	// Only check for local admin on tailscaled-on-mac (based on "sudo"
+	// permissions). On sandboxed variants (MacSys and AppStore), tailscaled
+	// cannot serve files outside of the sandbox and this check is not
+	// relevant.
+	if goos == "darwin" && version.IsSandboxedMacOS() {
 		return nil
 	}
 	if !configIn.HasPathHandler() {
 		return nil
 	}
-	if h.CallerIsLocalAdmin {
+	if h.connIsLocalAdmin() {
 		return nil
 	}
 	switch goos {
@@ -967,6 +972,76 @@ func authorizeServeConfigForGOOSAndUserContext(goos string, configIn *ipn.ServeC
 
 }
 
+// connIsLocalAdmin reports whether the connected client has administrative
+// access to the local machine, for whatever that means with respect to the
+// current OS.
+//
+// This is useful because tailscaled itself always runs with elevated rights:
+// we want to avoid privilege escalation for certain mutative operations.
+func (h *Handler) connIsLocalAdmin() bool {
+	if h.testConnIsLocalAdmin != nil {
+		return *h.testConnIsLocalAdmin
+	}
+	if h.ConnIdentity == nil {
+		h.logf("[unexpected] missing ConnIdentity in LocalAPI Handler")
+		return false
+	}
+	switch runtime.GOOS {
+	case "windows":
+		tok, err := h.ConnIdentity.WindowsToken()
+		if err != nil {
+			if !errors.Is(err, ipnauth.ErrNotImplemented) {
+				h.logf("ipnauth.ConnIdentity.WindowsToken() error: %v", err)
+			}
+			return false
+		}
+		defer tok.Close()
+
+		return tok.IsElevated()
+
+	case "darwin":
+		// Unknown, or at least unchecked on sandboxed macOS variants. Err on
+		// the side of less permissions.
+		//
+		// authorizeServeConfigForGOOSAndUserContext should not call
+		// connIsLocalAdmin on sandboxed variants anyway.
+		if version.IsSandboxedMacOS() {
+			return false
+		}
+		// This is a standalone tailscaled setup, use the same logic as on
+		// Linux.
+		fallthrough
+	case "linux":
+		uid, ok := h.ConnIdentity.Creds().UserID()
+		if !ok {
+			return false
+		}
+		// root is always admin.
+		if uid == "0" {
+			return true
+		}
+		// if non-root, must be operator AND able to execute "sudo tailscale".
+		operatorUID := h.b.OperatorUserID()
+		if operatorUID != "" && uid != operatorUID {
+			return false
+		}
+		u, err := osuser.LookupByUID(uid)
+		if err != nil {
+			return false
+		}
+		// Short timeout just in case sudo hands for some reason.
+		ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
+		defer cancel()
+		if err := exec.CommandContext(ctx, "sudo", "--other-user="+u.Name, "--list", "tailscale").Run(); err != nil {
+			return false
+		}
+		return true
+
+	default:
+		return false
+	}
+}
+
 func (h *Handler) serveCheckIPForwarding(w http.ResponseWriter, r *http.Request) {
 	if !h.PermitRead {
 		http.Error(w, "IP forwarding check access denied", http.StatusForbidden)

ipn/localapi/localapi_test.go

@@ -156,49 +156,17 @@ func TestWhoIsJustIP(t *testing.T) {
 }
 
 func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) {
+	newHandler := func(connIsLocalAdmin bool) *Handler {
+		return &Handler{testConnIsLocalAdmin: &connIsLocalAdmin}
+	}
 	tests := []struct {
 		name     string
-		goos     string
 		configIn *ipn.ServeConfig
 		h        *Handler
 		wantErr  bool
 	}{
 		{
-			name:     "linux",
+			name: "not-path-handler",
-			goos:     "linux",
-			configIn: &ipn.ServeConfig{},
-			h:        &Handler{CallerIsLocalAdmin: false},
-			wantErr:  false,
-		},
-		{
-			name: "linux-path-handler-admin",
-			goos: "linux",
-			configIn: &ipn.ServeConfig{
-				Web: map[ipn.HostPort]*ipn.WebServerConfig{
-					"foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{
-						"/": {Path: "/tmp"},
-					}},
-				},
-			},
-			h:       &Handler{CallerIsLocalAdmin: true},
-			wantErr: false,
-		},
-		{
-			name: "linux-path-handler-not-admin",
-			goos: "linux",
-			configIn: &ipn.ServeConfig{
-				Web: map[ipn.HostPort]*ipn.WebServerConfig{
-					"foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{
-						"/": {Path: "/tmp"},
-					}},
-				},
-			},
-			h:       &Handler{CallerIsLocalAdmin: false},
-			wantErr: true,
-		},
-		{
-			name: "windows-not-path-handler",
-			goos: "windows",
 			configIn: &ipn.ServeConfig{
 				Web: map[ipn.HostPort]*ipn.WebServerConfig{
 					"foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{
@@ -206,12 +174,11 @@ func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) {
 					}},
 				},
 			},
-			h:       &Handler{CallerIsLocalAdmin: false},
+			h:       newHandler(false),
 			wantErr: false,
 		},
 		{
-			name: "windows-path-handler-admin",
+			name: "path-handler-admin",
-			goos: "windows",
 			configIn: &ipn.ServeConfig{
 				Web: map[ipn.HostPort]*ipn.WebServerConfig{
 					"foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{
@@ -219,12 +186,11 @@ func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) {
 					}},
 				},
 			},
-			h:       &Handler{CallerIsLocalAdmin: true},
+			h:       newHandler(true),
 			wantErr: false,
 		},
 		{
-			name: "windows-path-handler-not-admin",
+			name: "path-handler-not-admin",
-			goos: "windows",
 			configIn: &ipn.ServeConfig{
 				Web: map[ipn.HostPort]*ipn.WebServerConfig{
 					"foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{
@@ -232,20 +198,36 @@ func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) {
 					}},
 				},
 			},
-			h:       &Handler{CallerIsLocalAdmin: false},
+			h:       newHandler(false),
 			wantErr: true,
 		},
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+		for _, goos := range []string{"linux", "windows", "darwin"} {
-			err := authorizeServeConfigForGOOSAndUserContext(tt.goos, tt.configIn, tt.h)
+			t.Run(goos+"-"+tt.name, func(t *testing.T) {
-			gotErr := err != nil
+				err := authorizeServeConfigForGOOSAndUserContext(goos, tt.configIn, tt.h)
-			if gotErr != tt.wantErr {
+				gotErr := err != nil
-				t.Errorf("authorizeServeConfigForGOOSAndUserContext() got error = %v, want error %v", err, tt.wantErr)
+				if gotErr != tt.wantErr {
-			}
+					t.Errorf("authorizeServeConfigForGOOSAndUserContext() got error = %v, want error %v", err, tt.wantErr)
-		})
+				}
+			})
+		}
 	}
+	t.Run("other-goos", func(t *testing.T) {
+		configIn := &ipn.ServeConfig{
+			Web: map[ipn.HostPort]*ipn.WebServerConfig{
+				"foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{
+					"/": {Path: "/tmp"},
+				}},
+			},
+		}
+		h := newHandler(false)
+		err := authorizeServeConfigForGOOSAndUserContext("dos", configIn, h)
+		if err != nil {
+			t.Errorf("authorizeServeConfigForGOOSAndUserContext() got error = %v, want nil", err)
+		}
+	})
 }
 
 func TestServeWatchIPNBus(t *testing.T) {