From 0c3d343ea3b62e8a96eed003154497ece1ad71a4 Mon Sep 17 00:00:00 2001 From: Will Norris Date: Thu, 24 Aug 2023 12:46:51 -0700 Subject: [PATCH] client/web: invert auth logic for synology and qnap Add separate server methods for synology and qnap, and enforce authentication and authorization checks before calling into the actual serving handlers. This allows us to remove all of the auth logic from those handlers, since all requests will already be authenticated by that point. Also simplify the Synology token redirect handler by using fetch. Remove the SynologyUser from nodeData, since it was never used in the frontend anyway. Updates tailscale/corp#13775 Signed-off-by: Will Norris --- client/web/api.go | 6 +-- client/web/qnap.go | 17 +++++++++ client/web/synology.go | 73 ++++++++++++++++++----------------- client/web/web.go | 86 +++++++++++++----------------------------- 4 files changed, 83 insertions(+), 99 deletions(-) diff --git a/client/web/api.go b/client/web/api.go index be220580f..1ee37980e 100644 --- a/client/web/api.go +++ b/client/web/api.go @@ -20,16 +20,12 @@ type api struct { // which protects the handler using gorilla csrf. func (a *api) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Set("X-CSRF-Token", csrf.Token(r)) - user, err := authorize(w, r) - if err != nil { - return - } path := strings.TrimPrefix(r.URL.Path, "/api") switch path { case "/data": switch r.Method { case httpm.GET: - a.s.serveGetNodeDataJSON(w, r, user) + a.s.serveGetNodeDataJSON(w, r) case httpm.POST: a.s.servePostNodeUpdate(w, r) default: diff --git a/client/web/qnap.go b/client/web/qnap.go index 682c4d952..d3b1d8dd7 100644 --- a/client/web/qnap.go +++ b/client/web/qnap.go @@ -16,6 +16,23 @@ import ( "net/url" ) +// authorizeQNAP authenticates the logged-in QNAP user and verifies +// that they are authorized to use the web client. It returns true if the +// request was handled and no further processing is required. +func authorizeQNAP(w http.ResponseWriter, r *http.Request) (handled bool) { + _, resp, err := qnapAuthn(r) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return true + } + if resp.IsAdmin == 0 { + http.Error(w, "user is not an admin", http.StatusForbidden) + return true + } + + return false +} + type qnapAuthResponse struct { AuthPassed int `xml:"authPassed"` IsAdmin int `xml:"isAdmin"` diff --git a/client/web/synology.go b/client/web/synology.go index 1d8f1096e..7c3f82c11 100644 --- a/client/web/synology.go +++ b/client/web/synology.go @@ -15,6 +15,37 @@ import ( "tailscale.com/util/groupmember" ) +// authorizeSynology authenticates the logged-in Synology user and verifies +// that they are authorized to use the web client. It returns true if the +// request was handled and no further processing is required. +func authorizeSynology(w http.ResponseWriter, r *http.Request) (handled bool) { + if synoTokenRedirect(w, r) { + return true + } + + // authenticate the Synology user + cmd := exec.Command("/usr/syno/synoman/webman/modules/authenticate.cgi") + out, err := cmd.CombinedOutput() + if err != nil { + http.Error(w, fmt.Sprintf("auth: %v: %s", err, out), http.StatusUnauthorized) + return true + } + user := strings.TrimSpace(string(out)) + + // check if the user is in the administrators group + isAdmin, err := groupmember.IsMemberOfGroup("administrators", user) + if err != nil { + http.Error(w, err.Error(), http.StatusForbidden) + return true + } + if !isAdmin { + http.Error(w, "not a member of administrators group", http.StatusForbidden) + return true + } + + return false +} + func synoTokenRedirect(w http.ResponseWriter, r *http.Request) bool { if r.Header.Get("X-Syno-Token") != "" { return false @@ -31,41 +62,15 @@ func synoTokenRedirect(w http.ResponseWriter, r *http.Request) bool { return true } -const synoTokenRedirectHTML = ` +const synoTokenRedirectHTML = ` Redirecting with session token... - ` - -func synoAuthn() (string, error) { - cmd := exec.Command("/usr/syno/synoman/webman/modules/authenticate.cgi") - out, err := cmd.CombinedOutput() - if err != nil { - return "", fmt.Errorf("auth: %v: %s", err, out) - } - return strings.TrimSpace(string(out)), nil -} - -// authorizeSynology checks whether the provided user has access to the web UI -// by consulting the membership of the "administrators" group. -func authorizeSynology(name string) error { - yes, err := groupmember.IsMemberOfGroup("administrators", name) - if err != nil { - return err - } - if !yes { - return fmt.Errorf("not a member of administrators group") - } - return nil -} diff --git a/client/web/web.go b/client/web/web.go index 01dc78f3b..a6e3ef589 100644 --- a/client/web/web.go +++ b/client/web/web.go @@ -100,48 +100,25 @@ func init() { tmpls = template.Must(template.New("").ParseFS(embeddedFS, "*")) } -// authorize returns the name of the user accessing the web UI after verifying -// whether the user has access to the web UI. The function will write the -// error to the provided http.ResponseWriter. -// Note: This is different from a tailscale user, and is typically the local -// user on the node. -func authorize(w http.ResponseWriter, r *http.Request) (string, error) { +// ServeHTTP processes all requests for the Tailscale web client. +func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { + // some platforms where the client runs have their own authentication + // and authorization mechanisms we need to work with. Do those checks first. switch distro.Get() { case distro.Synology: - user, err := synoAuthn() - if err != nil { - http.Error(w, err.Error(), http.StatusUnauthorized) - return "", err - } - if err := authorizeSynology(user); err != nil { - http.Error(w, err.Error(), http.StatusForbidden) - return "", err + if authorizeSynology(w, r) { + return } - return user, nil case distro.QNAP: - user, resp, err := qnapAuthn(r) - if err != nil { - http.Error(w, err.Error(), http.StatusUnauthorized) - return "", err - } - if resp.IsAdmin == 0 { - http.Error(w, err.Error(), http.StatusForbidden) - return "", err + if authorizeQNAP(w, r) { + return } - return user, nil } - return "", nil -} -func authRedirect(w http.ResponseWriter, r *http.Request) bool { - if distro.Get() == distro.Synology { - return synoTokenRedirect(w, r) - } - return false + s.serve(w, r) } -// ServeHTTP processes all requests for the Tailscale web client. -func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { +func (s *Server) serve(w http.ResponseWriter, r *http.Request) { if s.devMode { if strings.HasPrefix(r.URL.Path, "/api/") { // Pass through to other handlers via CSRF protection. @@ -153,29 +130,19 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if authRedirect(w, r) { - return - } - - user, err := authorize(w, r) - if err != nil { - return - } - switch { case r.Method == "POST": s.servePostNodeUpdate(w, r) return default: s.lc.IncrementCounter(context.Background(), "web_client_page_load", 1) - s.serveGetNodeData(w, r, user) + s.serveGetNodeData(w, r) return } } type nodeData struct { Profile tailcfg.UserProfile - SynologyUser string Status string DeviceName string IP string @@ -190,7 +157,7 @@ type nodeData struct { IPNVersion string } -func (s *Server) getNodeData(ctx context.Context, user string) (*nodeData, error) { +func (s *Server) getNodeData(ctx context.Context) (*nodeData, error) { st, err := s.lc.Status(ctx) if err != nil { return nil, err @@ -203,17 +170,16 @@ func (s *Server) getNodeData(ctx context.Context, user string) (*nodeData, error deviceName := strings.Split(st.Self.DNSName, ".")[0] versionShort := strings.Split(st.Version, "-")[0] data := &nodeData{ - SynologyUser: user, - Profile: profile, - Status: st.BackendState, - DeviceName: deviceName, - LicensesURL: licenses.LicensesURL(), - TUNMode: st.TUN, - IsSynology: distro.Get() == distro.Synology || envknob.Bool("TS_FAKE_SYNOLOGY"), - DSMVersion: distro.DSMVersion(), - IsUnraid: distro.Get() == distro.Unraid, - UnraidToken: os.Getenv("UNRAID_CSRF_TOKEN"), - IPNVersion: versionShort, + Profile: profile, + Status: st.BackendState, + DeviceName: deviceName, + LicensesURL: licenses.LicensesURL(), + TUNMode: st.TUN, + IsSynology: distro.Get() == distro.Synology || envknob.Bool("TS_FAKE_SYNOLOGY"), + DSMVersion: distro.DSMVersion(), + IsUnraid: distro.Get() == distro.Unraid, + UnraidToken: os.Getenv("UNRAID_CSRF_TOKEN"), + IPNVersion: versionShort, } exitNodeRouteV4 := netip.MustParsePrefix("0.0.0.0/0") exitNodeRouteV6 := netip.MustParsePrefix("::/0") @@ -233,8 +199,8 @@ func (s *Server) getNodeData(ctx context.Context, user string) (*nodeData, error return data, nil } -func (s *Server) serveGetNodeData(w http.ResponseWriter, r *http.Request, user string) { - data, err := s.getNodeData(r.Context(), user) +func (s *Server) serveGetNodeData(w http.ResponseWriter, r *http.Request) { + data, err := s.getNodeData(r.Context()) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -247,8 +213,8 @@ func (s *Server) serveGetNodeData(w http.ResponseWriter, r *http.Request, user s w.Write(buf.Bytes()) } -func (s *Server) serveGetNodeDataJSON(w http.ResponseWriter, r *http.Request, user string) { - data, err := s.getNodeData(r.Context(), user) +func (s *Server) serveGetNodeDataJSON(w http.ResponseWriter, r *http.Request) { + data, err := s.getNodeData(r.Context()) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return