From f15a4f441621e537340234d63271d3cf4dffe4b5 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Fri, 1 May 2026 09:01:53 -0700 Subject: [PATCH] client/web: move API permission checks into handlers (#19576) There are only a couple endpoints that check peer capabilities. Keeping permission checks with the code that assumes they were performed, rather than with the routing layer, feels easier to reason about. Check that the caller is actually a peer and pass their capabilities via a context value for handlers that want to check them. Along with this, simplify the helper handler wrappers that are not needed for most of the endpoints. Updates #40851 Signed-off-by: Andrew Lytvynov --- client/web/web.go | 189 +++++++++++++--------------------- client/web/web_test.go | 30 +++++- cmd/k8s-operator/depaware.txt | 2 +- cmd/tailscale/depaware.txt | 2 +- cmd/tailscaled/depaware.txt | 2 +- cmd/tsidp/depaware.txt | 2 +- tsnet/depaware.txt | 2 +- 7 files changed, 104 insertions(+), 125 deletions(-) diff --git a/client/web/web.go b/client/web/web.go index cebea60fc..95259ef1a 100644 --- a/client/web/web.go +++ b/client/web/web.go @@ -35,8 +35,10 @@ import ( "tailscale.com/net/netutil" "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" + "tailscale.com/tsweb" "tailscale.com/types/logger" "tailscale.com/types/views" + "tailscale.com/util/ctxkey" "tailscale.com/util/httpm" "tailscale.com/util/syspolicy/policyclient" "tailscale.com/version" @@ -527,45 +529,40 @@ func (s *Server) serveLoginAPI(w http.ResponseWriter, r *http.Request) { } } -type apiHandler[data any] struct { - s *Server - w http.ResponseWriter - r *http.Request - - // permissionCheck allows for defining whether a requesting peer's - // capabilities grant them access to make the given data update. - // If permissionCheck reports false, the request fails as unauthorized. - permissionCheck func(data data, peer peerCapabilities) bool -} - -// newHandler constructs a new api handler which restricts the given request -// to the specified permission check. If the permission check fails for -// the peer associated with the request, an unauthorized error is returned -// to the client. -func newHandler[data any](s *Server, w http.ResponseWriter, r *http.Request, permissionCheck func(data data, peer peerCapabilities) bool) *apiHandler[data] { - return &apiHandler[data]{ - s: s, - w: w, - r: r, - permissionCheck: permissionCheck, +// handleJSON manages decoding the request's body JSON as data and passing it +// on to the provided handler function. +func handleJSON[data any](h func(ctx context.Context, data data) error) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + var body data + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + if err := h(r.Context(), body); err != nil { + if httpErr, ok := errors.AsType[tsweb.HTTPError](err); ok { + tsweb.WriteHTTPError(w, r, httpErr) + } else { + http.Error(w, err.Error(), http.StatusInternalServerError) + } + return + } + w.WriteHeader(http.StatusOK) } } -// alwaysAllowed can be passed as the permissionCheck argument to newHandler -// for requests that are always allowed to complete regardless of a peer's -// capabilities. -func alwaysAllowed[data any](_ data, _ peerCapabilities) bool { return true } +var contextKeyPeer = ctxkey.New("peer-capabilities", peerCapabilities{}) -func (a *apiHandler[data]) getPeer() (peerCapabilities, error) { +func (s *Server) setPeer(r *http.Request) (*http.Request, error) { // TODO(tailscale/corp#16695,sonia): We also call StatusWithoutPeers and // WhoIs when originally checking for a session from authorizeRequest. // Would be nice if we could pipe those through to here so we don't end // up having to re-call them to grab the peer capabilities. - status, err := a.s.lc.StatusWithoutPeers(a.r.Context()) + status, err := s.lc.StatusWithoutPeers(r.Context()) if err != nil { return nil, err } - whois, err := a.s.lc.WhoIs(a.r.Context(), a.r.RemoteAddr) + whois, err := s.lc.WhoIs(r.Context(), r.RemoteAddr) if err != nil { return nil, err } @@ -573,56 +570,11 @@ func (a *apiHandler[data]) getPeer() (peerCapabilities, error) { if err != nil { return nil, err } - return peer, nil + return r.WithContext(contextKeyPeer.WithValue(r.Context(), peer)), nil } -type noBodyData any // empty type, for use from serveAPI for endpoints with empty body - -// handle runs the given handler if the source peer satisfies the -// constraints for running this request. -// -// handle is expected for use when `data` type is empty, or set to -// `noBodyData` in practice. For requests that expect JSON body data -// to be attached, use handleJSON instead. -func (a *apiHandler[data]) handle(h http.HandlerFunc) { - peer, err := a.getPeer() - if err != nil { - http.Error(a.w, err.Error(), http.StatusInternalServerError) - return - } - var body data // not used - if !a.permissionCheck(body, peer) { - http.Error(a.w, "not allowed", http.StatusUnauthorized) - return - } - h(a.w, a.r) -} - -// handleJSON manages decoding the request's body JSON and passing -// it on to the provided function if the source peer satisfies the -// constraints for running this request. -func (a *apiHandler[data]) handleJSON(h func(ctx context.Context, data data) error) { - defer a.r.Body.Close() - var body data - if err := json.NewDecoder(a.r.Body).Decode(&body); err != nil { - http.Error(a.w, err.Error(), http.StatusInternalServerError) - return - } - peer, err := a.getPeer() - if err != nil { - http.Error(a.w, err.Error(), http.StatusInternalServerError) - return - } - if !a.permissionCheck(body, peer) { - http.Error(a.w, "not allowed", http.StatusUnauthorized) - return - } - - if err := h(a.r.Context(), body); err != nil { - http.Error(a.w, err.Error(), http.StatusInternalServerError) - return - } - a.w.WriteHeader(http.StatusOK) +func (s *Server) getPeer(ctx context.Context) peerCapabilities { + return contextKeyPeer.Value(ctx) } // serveAPI serves requests for the web client api. @@ -637,67 +589,44 @@ func (s *Server) serveAPI(w http.ResponseWriter, r *http.Request) { } } + var err error + r, err = s.setPeer(r) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + path := strings.TrimPrefix(r.URL.Path, "/api") switch { case path == "/data" && r.Method == httpm.GET: - newHandler[noBodyData](s, w, r, alwaysAllowed). - handle(s.serveGetNodeData) + s.serveGetNodeData(w, r) return case path == "/exit-nodes" && r.Method == httpm.GET: - newHandler[noBodyData](s, w, r, alwaysAllowed). - handle(s.serveGetExitNodes) + s.serveGetExitNodes(w, r) return case path == "/routes" && r.Method == httpm.POST: - peerAllowed := func(d postRoutesRequest, p peerCapabilities) bool { - if d.SetExitNode && !p.canEdit(capFeatureExitNodes) { - return false - } else if d.SetRoutes && !p.canEdit(capFeatureSubnets) { - return false - } - return true - } - newHandler[postRoutesRequest](s, w, r, peerAllowed). - handleJSON(s.servePostRoutes) + handleJSON[postRoutesRequest](s.servePostRoutes)(w, r) return case path == "/device-details-click" && r.Method == httpm.POST: - newHandler[noBodyData](s, w, r, alwaysAllowed). - handle(s.serveDeviceDetailsClick) + s.serveDeviceDetailsClick(w, r) return case path == "/local/v0/logout" && r.Method == httpm.POST: - peerAllowed := func(_ noBodyData, peer peerCapabilities) bool { - return peer.canEdit(capFeatureAccount) - } - newHandler[noBodyData](s, w, r, peerAllowed). - handle(s.proxyRequestToLocalAPI) + s.proxyRequestToLocalAPI(w, r) return case path == "/local/v0/prefs" && r.Method == httpm.PATCH: - peerAllowed := func(data maskedPrefs, peer peerCapabilities) bool { - if data.RunSSHSet && !peer.canEdit(capFeatureSSH) { - return false - } - return true - } - newHandler[maskedPrefs](s, w, r, peerAllowed). - handleJSON(s.serveUpdatePrefs) + handleJSON[maskedPrefs](s.serveUpdatePrefs)(w, r) return case path == "/local/v0/update/check" && r.Method == httpm.GET: - newHandler[noBodyData](s, w, r, alwaysAllowed). - handle(s.proxyRequestToLocalAPI) + s.proxyRequestToLocalAPI(w, r) return case path == "/local/v0/update/check" && r.Method == httpm.POST: - peerAllowed := func(_ noBodyData, peer peerCapabilities) bool { - return peer.canEdit(capFeatureAccount) - } - newHandler[noBodyData](s, w, r, peerAllowed). - handle(s.proxyRequestToLocalAPI) + s.proxyRequestToLocalAPI(w, r) return case path == "/local/v0/update/progress" && r.Method == httpm.POST: - newHandler[noBodyData](s, w, r, alwaysAllowed). - handle(s.proxyRequestToLocalAPI) + s.proxyRequestToLocalAPI(w, r) return case path == "/local/v0/upload-client-metrics" && r.Method == httpm.POST: - newHandler[noBodyData](s, w, r, alwaysAllowed). - handle(s.proxyRequestToLocalAPI) + s.proxyRequestToLocalAPI(w, r) return } http.Error(w, "invalid endpoint", http.StatusNotFound) @@ -1122,6 +1051,11 @@ type maskedPrefs struct { } func (s *Server) serveUpdatePrefs(ctx context.Context, prefs maskedPrefs) error { + peer := s.getPeer(ctx) + if prefs.RunSSHSet && !peer.canEdit(capFeatureSSH) { + return tsweb.Error(http.StatusUnauthorized, "RunSSHSet not allowed", nil) + } + _, err := s.lc.EditPrefs(ctx, &ipn.MaskedPrefs{ RunSSHSet: prefs.RunSSHSet, Prefs: ipn.Prefs{ @@ -1141,8 +1075,16 @@ type postRoutesRequest struct { func (s *Server) servePostRoutes(ctx context.Context, data postRoutesRequest) error { if !data.SetExitNode && !data.SetRoutes { - return errors.New("must specify SetExitNode or SetRoutes") + return tsweb.Error(http.StatusBadRequest, "must specify SetExitNode or SetRoutes", nil) } + peer := s.getPeer(ctx) + if data.SetExitNode && !peer.canEdit(capFeatureExitNodes) { + return tsweb.Error(http.StatusUnauthorized, "SetExitNode not allowed", nil) + } + if data.SetRoutes && !peer.canEdit(capFeatureSubnets) { + return tsweb.Error(http.StatusUnauthorized, "SetRoutes not allowed", nil) + } + prefs, err := s.lc.GetPrefs(ctx) if err != nil { return err @@ -1340,6 +1282,19 @@ func (s *Server) proxyRequestToLocalAPI(w http.ResponseWriter, r *http.Request) return } + switch path { + case "/v0/logout": + if !s.getPeer(r.Context()).canEdit(capFeatureAccount) { + http.Error(w, "not allowed", http.StatusUnauthorized) + return + } + case "/v0/update/check": + if r.Method == httpm.POST && !s.getPeer(r.Context()).canEdit(capFeatureAccount) { + http.Error(w, "not allowed", http.StatusUnauthorized) + return + } + } + localAPIURL := "http://" + apitype.LocalAPIHost + "/localapi" + path req, err := http.NewRequestWithContext(r.Context(), r.Method, localAPIURL, r.Body) if err != nil { diff --git a/client/web/web_test.go b/client/web/web_test.go index fbf459545..51b6a8ac5 100644 --- a/client/web/web_test.go +++ b/client/web/web_test.go @@ -191,7 +191,7 @@ func TestServeAPI(t *testing.T) { reqBody: "{\"setExitNode\":true}", tests: []requestTest{{ remoteIP: remoteIPWithNoCapabilities, - wantResponse: "not allowed", + wantResponse: "SetExitNode not allowed", wantStatus: http.StatusUnauthorized, }, { remoteIP: remoteIPWithAllCapabilities, @@ -204,7 +204,7 @@ func TestServeAPI(t *testing.T) { reqContentType: "application/json", tests: []requestTest{{ remoteIP: remoteIPWithNoCapabilities, - wantResponse: "not allowed", + wantResponse: "RunSSHSet not allowed", wantStatus: http.StatusUnauthorized, }, { remoteIP: remoteIPWithAllCapabilities, @@ -1617,6 +1617,7 @@ func TestServePostRoutes(t *testing.T) { tests := []struct { name string data postRoutesRequest + peerCaps peerCapabilities wantErr bool wantEditPrefs bool // whether EditPrefs (PATCH /prefs) should be called wantExitNodeID tailcfg.StableNodeID @@ -1625,6 +1626,7 @@ func TestServePostRoutes(t *testing.T) { { name: "empty-request", data: postRoutesRequest{}, + peerCaps: peerCapabilities{capFeatureExitNodes: true, capFeatureSubnets: true}, wantErr: true, wantEditPrefs: false, }, @@ -1634,20 +1636,40 @@ func TestServePostRoutes(t *testing.T) { SetExitNode: true, UseExitNode: "new-exit-node", }, + peerCaps: peerCapabilities{capFeatureExitNodes: true, capFeatureSubnets: true}, wantEditPrefs: true, wantExitNodeID: "new-exit-node", wantRoutes: []netip.Prefix{existingRoute}, }, + { + name: "SetExitNode-not-allowed", + data: postRoutesRequest{ + SetExitNode: true, + UseExitNode: "new-exit-node", + }, + peerCaps: peerCapabilities{capFeatureSubnets: true}, + wantErr: true, + }, { name: "SetRoutes-only", data: postRoutesRequest{ SetRoutes: true, AdvertiseRoutes: []string{"10.0.0.0/8"}, }, + peerCaps: peerCapabilities{capFeatureExitNodes: true, capFeatureSubnets: true}, wantEditPrefs: true, wantExitNodeID: existingExitNodeID, wantRoutes: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, }, + { + name: "SetRoutes-not-allowed", + data: postRoutesRequest{ + SetRoutes: true, + AdvertiseRoutes: []string{"10.0.0.0/8"}, + }, + peerCaps: peerCapabilities{capFeatureExitNodes: true}, + wantErr: true, + }, { name: "SetExitNode-and-SetRoutes", data: postRoutesRequest{ @@ -1656,6 +1678,7 @@ func TestServePostRoutes(t *testing.T) { UseExitNode: "new-exit-node", AdvertiseRoutes: []string{"10.0.0.0/8"}, }, + peerCaps: peerCapabilities{capFeatureExitNodes: true, capFeatureSubnets: true}, wantEditPrefs: true, wantExitNodeID: "new-exit-node", wantRoutes: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, @@ -1699,7 +1722,8 @@ func TestServePostRoutes(t *testing.T) { lc: &local.Client{Dial: lal.Dial}, } - err := s.servePostRoutes(context.Background(), tt.data) + ctx := contextKeyPeer.WithValue(t.Context(), tt.peerCaps) + err := s.servePostRoutes(ctx, tt.data) if tt.wantErr { if err == nil { diff --git a/cmd/k8s-operator/depaware.txt b/cmd/k8s-operator/depaware.txt index 30f955e7b..478e761ed 100644 --- a/cmd/k8s-operator/depaware.txt +++ b/cmd/k8s-operator/depaware.txt @@ -910,7 +910,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ tailscale.com/tstime from tailscale.com/cmd/k8s-operator+ tailscale.com/tstime/mono from tailscale.com/net/tstun+ tailscale.com/tstime/rate from tailscale.com/wgengine/filter - tailscale.com/tsweb from tailscale.com/util/eventbus + tailscale.com/tsweb from tailscale.com/util/eventbus+ tailscale.com/tsweb/varz from tailscale.com/util/usermetric+ tailscale.com/types/appctype from tailscale.com/ipn/ipnlocal+ tailscale.com/types/bools from tailscale.com/tsnet+ diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 2fc8d0c79..d23ab1f4f 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -239,7 +239,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/tstime from tailscale.com/control/controlhttp+ tailscale.com/tstime/mono from tailscale.com/tstime/rate tailscale.com/tstime/rate from tailscale.com/cmd/tailscale/cli - tailscale.com/tsweb from tailscale.com/util/eventbus + tailscale.com/tsweb from tailscale.com/util/eventbus+ tailscale.com/tsweb/varz from tailscale.com/util/usermetric+ tailscale.com/types/appctype from tailscale.com/client/local+ tailscale.com/types/dnstype from tailscale.com/tailcfg+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 4e9c89d57..fa1f4e7d2 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -404,7 +404,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/tstime from tailscale.com/control/controlclient+ tailscale.com/tstime/mono from tailscale.com/net/tstun+ tailscale.com/tstime/rate from tailscale.com/wgengine/filter - tailscale.com/tsweb from tailscale.com/util/eventbus + tailscale.com/tsweb from tailscale.com/util/eventbus+ tailscale.com/tsweb/varz from tailscale.com/cmd/tailscaled+ tailscale.com/types/appctype from tailscale.com/ipn/ipnlocal+ tailscale.com/types/bools from tailscale.com/wgengine/netlog diff --git a/cmd/tsidp/depaware.txt b/cmd/tsidp/depaware.txt index cedde0f42..169fffe0d 100644 --- a/cmd/tsidp/depaware.txt +++ b/cmd/tsidp/depaware.txt @@ -309,7 +309,7 @@ tailscale.com/cmd/tsidp dependencies: (generated by github.com/tailscale/depawar tailscale.com/tstime from tailscale.com/control/controlclient+ tailscale.com/tstime/mono from tailscale.com/net/tstun+ tailscale.com/tstime/rate from tailscale.com/wgengine/filter - tailscale.com/tsweb from tailscale.com/util/eventbus + tailscale.com/tsweb from tailscale.com/util/eventbus+ tailscale.com/tsweb/varz from tailscale.com/tsweb+ tailscale.com/types/appctype from tailscale.com/ipn/ipnlocal+ tailscale.com/types/bools from tailscale.com/tsnet+ diff --git a/tsnet/depaware.txt b/tsnet/depaware.txt index 71a6f40e8..b28233de6 100644 --- a/tsnet/depaware.txt +++ b/tsnet/depaware.txt @@ -304,7 +304,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware) tailscale.com/tstime from tailscale.com/control/controlclient+ tailscale.com/tstime/mono from tailscale.com/net/tstun+ tailscale.com/tstime/rate from tailscale.com/wgengine/filter - LDW tailscale.com/tsweb from tailscale.com/util/eventbus + LDW tailscale.com/tsweb from tailscale.com/util/eventbus+ tailscale.com/tsweb/varz from tailscale.com/tsweb+ tailscale.com/types/appctype from tailscale.com/ipn/ipnlocal+ tailscale.com/types/bools from tailscale.com/tsnet+