From 55cd5c575bce2c7557c9063c813040dc196b6847 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Thu, 9 Nov 2023 13:55:46 -0700 Subject: [PATCH] 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 --- cmd/tailscaled/depaware.txt | 2 +- ipn/ipnserver/server.go | 60 +------------------- ipn/localapi/localapi.go | 101 +++++++++++++++++++++++++++++----- ipn/localapi/localapi_test.go | 82 +++++++++++---------------- 4 files changed, 122 insertions(+), 123 deletions(-) diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 29174fd26..b24eeeb4f 100644 --- a/cmd/tailscaled/depaware.txt +++ b/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+ diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 5a6507090..90b5cfd65 100644 --- a/ipn/ipnserver/server.go +++ b/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. diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index a0066f17c..aeb5cd02c 100644 --- a/ipn/localapi/localapi.go +++ b/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 - // result of a LocalAPI call from a user who is a local admin of the current - // machine. - // - // As of 2023-10-26 it is only populated on Windows. - // - // 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 + // ConnIdentity is the identity of the client connected to the Handler. + ConnIdentity *ipnauth.ConnIdentity + + // Test-only override for connIsLocalAdmin method. If non-nil, + // connIsLocalAdmin returns this value. + testConnIsLocalAdmin *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) diff --git a/ipn/localapi/localapi_test.go b/ipn/localapi/localapi_test.go index 2988e9604..cbaf8aa63 100644 --- a/ipn/localapi/localapi_test.go +++ b/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", - 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", + name: "not-path-handler", 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", - goos: "windows", + name: "path-handler-admin", 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", - goos: "windows", + name: "path-handler-not-admin", 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) { - err := authorizeServeConfigForGOOSAndUserContext(tt.goos, tt.configIn, tt.h) - gotErr := err != nil - if gotErr != tt.wantErr { - t.Errorf("authorizeServeConfigForGOOSAndUserContext() got error = %v, want error %v", err, tt.wantErr) - } - }) + for _, goos := range []string{"linux", "windows", "darwin"} { + t.Run(goos+"-"+tt.name, func(t *testing.T) { + err := authorizeServeConfigForGOOSAndUserContext(goos, tt.configIn, tt.h) + gotErr := err != nil + 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) {