From bd534b971a797c1fd7a876262d2a77de7b97db05 Mon Sep 17 00:00:00 2001 From: Sonia Appasamy Date: Thu, 16 Nov 2023 17:53:46 -0500 Subject: [PATCH] {client/web},{ipn/ipnlocal}: replace localapi debug-web-client endpoint This change removes the existing debug-web-client localapi endpoint and replaces it with functions passed directly to the web.ServerOpts when constructing a web.ManageServerMode client. The debug-web-client endpoint previously handled making noise requests to the control server via the /machine/webclient/ endpoints. The noise requests must be made from tailscaled, which has the noise connection open. But, now that the full client is served from tailscaled, we no longer need to proxy this request over the localapi. Updates tailscale/corp#14335 Signed-off-by: Sonia Appasamy --- client/web/auth.go | 55 ++++------------------------------ client/web/web.go | 45 +++++++++++++++++++++++----- client/web/web_test.go | 50 ++++++++++++++----------------- ipn/ipnlocal/web_client.go | 59 +++++++++++++++++++++++++++++++++++++ ipn/localapi/localapi.go | 60 -------------------------------------- 5 files changed, 125 insertions(+), 144 deletions(-) diff --git a/client/web/auth.go b/client/web/auth.go index 35312fde0..f4ed2aced 100644 --- a/client/web/auth.go +++ b/client/web/auth.go @@ -4,14 +4,10 @@ package web import ( - "bytes" "context" "crypto/rand" "encoding/base64" - "encoding/json" "errors" - "fmt" - "io" "net/http" "time" @@ -152,7 +148,7 @@ func (s *Server) getSession(r *http.Request) (*browserSession, *apitype.WhoIsRes // and stores it back to the session cache. Creating of a new session includes // generating a new auth URL from the control server. func (s *Server) newSession(ctx context.Context, src *apitype.WhoIsResponse) (*browserSession, error) { - d, err := s.getOrAwaitAuth(ctx, "", src.Node.ID) + a, err := s.newAuthURL(ctx, src.Node.ID) if err != nil { return nil, err } @@ -164,8 +160,8 @@ func (s *Server) newSession(ctx context.Context, src *apitype.WhoIsResponse) (*b ID: sid, SrcNode: src.Node.ID, SrcUser: src.UserProfile.ID, - AuthID: d.ID, - AuthURL: d.URL, + AuthID: a.ID, + AuthURL: a.URL, Created: s.timeNow(), } s.browserSessions.Store(sid, session) @@ -179,7 +175,7 @@ func (s *Server) awaitUserAuth(ctx context.Context, session *browserSession) err if session.isAuthorized(s.timeNow()) { return nil // already authorized } - d, err := s.getOrAwaitAuth(ctx, session.AuthID, session.SrcNode) + a, err := s.waitAuthURL(ctx, session.AuthID, session.SrcNode) if err != nil { // Clean up the session. Doing this on any error from control // server to avoid the user getting stuck with a bad session @@ -187,52 +183,13 @@ func (s *Server) awaitUserAuth(ctx context.Context, session *browserSession) err s.browserSessions.Delete(session.ID) return err } - if d.Complete { - session.Authenticated = d.Complete + if a.Complete { + session.Authenticated = a.Complete s.browserSessions.Store(session.ID, session) } return nil } -// getOrAwaitAuth connects to the control server for user auth, -// with the following behavior: -// -// 1. If authID is provided empty, a new auth URL is created on the control -// server and reported back here, which can then be used to redirect the -// user on the frontend. -// 2. If authID is provided non-empty, the connection to control blocks until -// the user has completed authenticating the associated auth URL, -// or until ctx is canceled. -func (s *Server) getOrAwaitAuth(ctx context.Context, authID string, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) { - type data struct { - ID string - Src tailcfg.NodeID - } - var b bytes.Buffer - if err := json.NewEncoder(&b).Encode(data{ID: authID, Src: src}); err != nil { - return nil, err - } - url := "http://" + apitype.LocalAPIHost + "/localapi/v0/debug-web-client" - req, err := http.NewRequestWithContext(ctx, "POST", url, &b) - if err != nil { - return nil, err - } - resp, err := s.lc.DoLocalRequest(req) - if err != nil { - return nil, err - } - body, _ := io.ReadAll(resp.Body) - resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed request: %s", body) - } - var authResp *tailcfg.WebClientAuthResponse - if err := json.Unmarshal(body, &authResp); err != nil { - return nil, err - } - return authResp, nil -} - func (s *Server) newSessionID() (string, error) { raw := make([]byte, 16) for i := 0; i < 5; i++ { diff --git a/client/web/web.go b/client/web/web.go index 3ecde836a..9457a564b 100644 --- a/client/web/web.go +++ b/client/web/web.go @@ -71,6 +71,13 @@ type Server struct { // The map provides a lookup of the session by cookie value // (browserSession.ID => browserSession). browserSessions sync.Map + + // newAuthURL creates a new auth URL that can be used to validate + // a browser session to manage this web client. + newAuthURL func(ctx context.Context, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) + // waitWebClientAuthURL blocks until the associated auth URL has + // been completed by its user, or until ctx is canceled. + waitAuthURL func(ctx context.Context, id string, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) } // ServerMode specifies the mode of a running web.Server. @@ -121,6 +128,20 @@ type ServerOpts struct { // Logf optionally provides a logger function. // log.Printf is used as default. Logf logger.Logf + + // The following two fields are required and used exclusively + // in ManageServerMode to facilitate the control server login + // check step for authorizing browser sessions. + + // NewAuthURL should be provided as a function that generates + // a new tailcfg.WebClientAuthResponse. + // This field is required for ManageServerMode mode. + NewAuthURL func(ctx context.Context, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) + // WaitAuthURL should be provided as a function that blocks until + // the associated tailcfg.WebClientAuthResponse has been marked + // as completed. + // This field is required for ManageServerMode mode. + WaitAuthURL func(ctx context.Context, id string, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) } // NewServer constructs a new Tailscale web client server. @@ -140,13 +161,23 @@ func NewServer(opts ServerOpts) (s *Server, err error) { opts.LocalClient = &tailscale.LocalClient{} } s = &Server{ - mode: opts.Mode, - logf: opts.Logf, - devMode: envknob.Bool("TS_DEBUG_WEB_CLIENT_DEV"), - lc: opts.LocalClient, - cgiMode: opts.CGIMode, - pathPrefix: opts.PathPrefix, - timeNow: opts.TimeNow, + mode: opts.Mode, + logf: opts.Logf, + devMode: envknob.Bool("TS_DEBUG_WEB_CLIENT_DEV"), + lc: opts.LocalClient, + cgiMode: opts.CGIMode, + pathPrefix: opts.PathPrefix, + timeNow: opts.TimeNow, + newAuthURL: opts.NewAuthURL, + waitAuthURL: opts.WaitAuthURL, + } + if s.mode == ManageServerMode { + if opts.NewAuthURL == nil { + return nil, fmt.Errorf("must provide a NewAuthURL implementation") + } + if opts.WaitAuthURL == nil { + return nil, fmt.Errorf("must provide WaitAuthURL implementation") + } } if s.timeNow == nil { s.timeNow = time.Now diff --git a/client/web/web_test.go b/client/web/web_test.go index 3a1e6b763..bd256bdac 100644 --- a/client/web/web_test.go +++ b/client/web/web_test.go @@ -4,6 +4,7 @@ package web import ( + "context" "encoding/json" "errors" "fmt" @@ -446,9 +447,11 @@ func TestServeAuth(t *testing.T) { sixtyDaysAgo := timeNow.Add(-sessionCookieExpiry * 2) s := &Server{ - mode: ManageServerMode, - lc: &tailscale.LocalClient{Dial: lal.Dial}, - timeNow: func() time.Time { return timeNow }, + mode: ManageServerMode, + lc: &tailscale.LocalClient{Dial: lal.Dial}, + timeNow: func() time.Time { return timeNow }, + newAuthURL: mockNewAuthURL, + waitAuthURL: mockWaitAuthURL, } successCookie := "ts-cookie-success" @@ -797,33 +800,24 @@ func mockLocalAPI(t *testing.T, whoIs map[string]*apitype.WhoIsResponse, self fu case "/localapi/v0/status": writeJSON(w, ipnstate.Status{Self: self()}) return - case "/localapi/v0/debug-web-client": // used by TestServeTailscaleAuth - type reqData struct { - ID string - Src tailcfg.NodeID - } - var data reqData - if err := json.NewDecoder(r.Body).Decode(&data); err != nil { - http.Error(w, "invalid JSON body", http.StatusBadRequest) - return - } - if data.Src == 0 { - http.Error(w, "missing Src node", http.StatusBadRequest) - return - } - var resp *tailcfg.WebClientAuthResponse - if data.ID == "" { - resp = &tailcfg.WebClientAuthResponse{ID: testAuthPath, URL: testControlURL + testAuthPath} - } else if data.ID == testAuthPathSuccess { - resp = &tailcfg.WebClientAuthResponse{Complete: true} - } else if data.ID == testAuthPathError { - http.Error(w, "authenticated as wrong user", http.StatusUnauthorized) - return - } - writeJSON(w, resp) - return default: t.Fatalf("unhandled localapi test endpoint %q, add to localapi handler func in test", r.URL.Path) } })} } + +func mockNewAuthURL(_ context.Context, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) { + // Create new dummy auth URL. + return &tailcfg.WebClientAuthResponse{ID: testAuthPath, URL: testControlURL + testAuthPath}, nil +} + +func mockWaitAuthURL(_ context.Context, id string, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) { + switch id { + case testAuthPathSuccess: // successful auth URL + return &tailcfg.WebClientAuthResponse{Complete: true}, nil + case testAuthPathError: // error auth URL + return nil, errors.New("authenticated as wrong user") + default: + return nil, errors.New("unknown id") + } +} diff --git a/ipn/ipnlocal/web_client.go b/ipn/ipnlocal/web_client.go index ba000d575..ee919d976 100644 --- a/ipn/ipnlocal/web_client.go +++ b/ipn/ipnlocal/web_client.go @@ -7,8 +7,10 @@ package ipnlocal import ( "context" + "encoding/json" "errors" "fmt" + "io" "net" "net/http" "net/netip" @@ -19,6 +21,7 @@ import ( "tailscale.com/client/web" "tailscale.com/logtail/backoff" "tailscale.com/net/netutil" + "tailscale.com/tailcfg" "tailscale.com/types/logger" "tailscale.com/util/mak" ) @@ -67,6 +70,8 @@ func (b *LocalBackend) webClientGetOrInit() (s *web.Server, err error) { Mode: web.ManageServerMode, LocalClient: b.webClient.lc, Logf: b.logf, + NewAuthURL: b.newWebClientAuthURL, + WaitAuthURL: b.waitWebClientAuthURL, }); err != nil { return nil, fmt.Errorf("web.NewServer: %w", err) } @@ -144,3 +149,57 @@ func (b *LocalBackend) newWebClientListener(ctx context.Context, ap netip.AddrPo bo: backoff.NewBackoff("webclient-listener", logf, 30*time.Second), } } + +// newWebClientAuthURL talks to the control server to create a new auth +// URL that can be used to validate a browser session to manage this +// tailscaled instance via the web client. +func (b *LocalBackend) newWebClientAuthURL(ctx context.Context, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) { + return b.doWebClientNoiseRequest(ctx, "", src) +} + +// waitWebClientAuthURL connects to the control server and blocks +// until the associated auth URL has been completed by its user, +// or until ctx is canceled. +func (b *LocalBackend) waitWebClientAuthURL(ctx context.Context, id string, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) { + return b.doWebClientNoiseRequest(ctx, id, src) +} + +// doWebClientNoiseRequest handles making the "/machine/webclient" +// noise requests to the control server for web client user auth. +// +// It either creates a new control auth URL or waits for an existing +// one to be completed, based on the presence or absence of the +// provided id value. +func (b *LocalBackend) doWebClientNoiseRequest(ctx context.Context, id string, src tailcfg.NodeID) (*tailcfg.WebClientAuthResponse, error) { + nm := b.NetMap() + if nm == nil || !nm.SelfNode.Valid() { + return nil, errors.New("[unexpected] no self node") + } + dst := nm.SelfNode.ID() + var noiseURL string + if id != "" { + noiseURL = fmt.Sprintf("https://unused/machine/webclient/wait/%d/to/%d/%s", src, dst, id) + } else { + noiseURL = fmt.Sprintf("https://unused/machine/webclient/init/%d/to/%d", src, dst) + } + + req, err := http.NewRequestWithContext(ctx, "POST", noiseURL, nil) + if err != nil { + return nil, err + } + resp, err := b.DoNoiseRequest(req) + if err != nil { + return nil, err + } + + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("failed request: %s", body) + } + var authResp *tailcfg.WebClientAuthResponse + if err := json.Unmarshal(body, &authResp); err != nil { + return nil, err + } + return authResp, nil +} diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index f4bcc4143..7d2a3c440 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -86,7 +86,6 @@ var handler = map[string]localAPIHandler{ "debug-peer-endpoint-changes": (*Handler).serveDebugPeerEndpointChanges, "debug-capture": (*Handler).serveDebugCapture, "debug-log": (*Handler).serveDebugLog, - "debug-web-client": (*Handler).serveDebugWebClient, "derpmap": (*Handler).serveDERPMap, "dev-set-state-store": (*Handler).serveDevSetStateStore, "set-push-device-token": (*Handler).serveSetPushDeviceToken, @@ -2300,65 +2299,6 @@ func (h *Handler) serveQueryFeature(w http.ResponseWriter, r *http.Request) { } } -// serveDebugWebClient is for use by the web client to communicate with -// the control server for browser auth sessions. -// -// This is an unsupported localapi endpoint and restricted to flagged -// domains on the control side. TODO(tailscale/#14335): Rename this handler. -func (h *Handler) serveDebugWebClient(w http.ResponseWriter, r *http.Request) { - if !h.PermitWrite { - http.Error(w, "access denied", http.StatusForbidden) - return - } - if r.Method != "POST" { - http.Error(w, "POST required", http.StatusMethodNotAllowed) - return - } - - type reqData struct { - ID string - Src tailcfg.NodeID - } - var data reqData - if err := json.NewDecoder(r.Body).Decode(&data); err != nil { - http.Error(w, "invalid JSON body", 400) - return - } - nm := h.b.NetMap() - if nm == nil || !nm.SelfNode.Valid() { - http.Error(w, "[unexpected] no self node", 400) - return - } - dst := nm.SelfNode.ID() - - var noiseURL string - if data.ID != "" { - noiseURL = fmt.Sprintf("https://unused/machine/webclient/wait/%d/to/%d/%s", data.Src, dst, data.ID) - } else { - noiseURL = fmt.Sprintf("https://unused/machine/webclient/init/%d/to/%d", data.Src, dst) - } - - req, err := http.NewRequestWithContext(r.Context(), "POST", noiseURL, nil) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - resp, err := h.b.DoNoiseRequest(req) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - body, _ := io.ReadAll(resp.Body) - resp.Body.Close() - if resp.StatusCode != http.StatusOK { - http.Error(w, string(body), resp.StatusCode) - return - } - w.Write(body) - w.Header().Set("Content-Type", "application/json") -} - func defBool(a string, def bool) bool { if a == "" { return def