From 5993f85ec91b4ed6d33f2a87be7319c6dfbc135f Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Mon, 28 Aug 2023 18:34:52 -0700 Subject: [PATCH] fix: avoid redirect loop on workspace proxies (#9389) * fix: avoid redirect loop on workspace proxies --------- Co-authored-by: Steven Masley --- coderd/httpapi/cookie.go | 5 +- coderd/httpmw/apikey.go | 13 ++-- coderd/workspaceapps/apptest/apptest.go | 44 +++++-------- coderd/workspaceapps/cookies.go | 51 +++++++++++++++ coderd/workspaceapps/db_test.go | 66 ++++++++++---------- coderd/workspaceapps/provider.go | 16 +++-- coderd/workspaceapps/proxy.go | 6 +- coderd/workspaceapps/request.go | 4 ++ coderd/workspaceapps/request_test.go | 7 ++- coderd/workspaceapps/token.go | 51 +++++++++++---- coderd/workspaceapps/token_test.go | 83 +++++++++++++++++++++++++ codersdk/client.go | 18 +++--- 12 files changed, 265 insertions(+), 99 deletions(-) create mode 100644 coderd/workspaceapps/cookies.go diff --git a/coderd/httpapi/cookie.go b/coderd/httpapi/cookie.go index 4879478cb7..526dfb8207 100644 --- a/coderd/httpapi/cookie.go +++ b/coderd/httpapi/cookie.go @@ -23,8 +23,9 @@ func StripCoderCookies(header string) string { if name == codersdk.SessionTokenCookie || name == codersdk.OAuth2StateCookie || name == codersdk.OAuth2RedirectCookie || - name == codersdk.DevURLSessionTokenCookie || - name == codersdk.DevURLSignedAppTokenCookie { + name == codersdk.PathAppSessionTokenCookie || + name == codersdk.SubdomainAppSessionTokenCookie || + name == codersdk.SignedAppTokenCookie { continue } cookies = append(cookies, part) diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index 5335c29676..26b8258206 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -447,10 +447,10 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon // APITokenFromRequest returns the api token from the request. // Find the session token from: // 1: The cookie -// 1: The devurl cookie -// 3: The old cookie -// 4. The coder_session_token query parameter -// 5. The custom auth header +// 2. The coder_session_token query parameter +// 3. The custom auth header +// +// API tokens for apps are read from workspaceapps/cookies.go. func APITokenFromRequest(r *http.Request) string { cookie, err := r.Cookie(codersdk.SessionTokenCookie) if err == nil && cookie.Value != "" { @@ -467,11 +467,6 @@ func APITokenFromRequest(r *http.Request) string { return headerValue } - cookie, err = r.Cookie(codersdk.DevURLSessionTokenCookie) - if err == nil && cookie.Value != "" { - return cookie.Value - } - return "" } diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 7e15f188e7..8851cf5113 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -257,7 +257,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { var appTokenCookie *http.Cookie for _, c := range resp.Cookies() { - if c.Name == codersdk.DevURLSignedAppTokenCookie { + if c.Name == codersdk.SignedAppTokenCookie { appTokenCookie = c break } @@ -302,7 +302,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { var appTokenCookie *http.Cookie for _, c := range resp.Cookies() { - if c.Name == codersdk.DevURLSignedAppTokenCookie { + if c.Name == codersdk.SignedAppTokenCookie { appTokenCookie = c break } @@ -400,30 +400,19 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { appDetails := setupProxyTest(t, nil) cases := []struct { - name string - appURL *url.URL - verifyCookie func(t *testing.T, c *http.Cookie) + name string + appURL *url.URL + sessionTokenCookieName string }{ { - name: "Subdomain", - appURL: appDetails.SubdomainAppURL(appDetails.Apps.Owner), - verifyCookie: func(t *testing.T, c *http.Cookie) { - // TODO(@dean): fix these asserts, they don't seem to - // work. I wonder if Go strips the domain from the - // cookie object if it's invalid or something. - // domain := strings.SplitN(appDetails.Options.AppHost, ".", 2) - // require.Equal(t, "."+domain[1], c.Domain, "incorrect domain on app token cookie") - }, + name: "Subdomain", + appURL: appDetails.SubdomainAppURL(appDetails.Apps.Owner), + sessionTokenCookieName: codersdk.SubdomainAppSessionTokenCookie, }, { - name: "Path", - appURL: appDetails.PathAppURL(appDetails.Apps.Owner), - verifyCookie: func(t *testing.T, c *http.Cookie) { - // TODO(@dean): fix these asserts, they don't seem to - // work. I wonder if Go strips the domain from the - // cookie object if it's invalid or something. - // require.Equal(t, "", c.Domain, "incorrect domain on app token cookie") - }, + name: "Path", + appURL: appDetails.PathAppURL(appDetails.Apps.Owner), + sessionTokenCookieName: codersdk.PathAppSessionTokenCookie, }, } @@ -508,14 +497,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { cookies := resp.Cookies() var cookie *http.Cookie - for _, c := range cookies { - if c.Name == codersdk.DevURLSessionTokenCookie { - cookie = c + for _, co := range cookies { + if co.Name == c.sessionTokenCookieName { + cookie = co break } } require.NotNil(t, cookie, "no app session token cookie was set") - c.verifyCookie(t, cookie) apiKey := cookie.Value // Fetch the API key from the API. @@ -715,7 +703,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { var appTokenCookie *http.Cookie for _, c := range resp.Cookies() { - if c.Name == codersdk.DevURLSignedAppTokenCookie { + if c.Name == codersdk.SignedAppTokenCookie { appTokenCookie = c break } @@ -759,7 +747,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { var appTokenCookie *http.Cookie for _, c := range resp.Cookies() { - if c.Name == codersdk.DevURLSignedAppTokenCookie { + if c.Name == codersdk.SignedAppTokenCookie { appTokenCookie = c break } diff --git a/coderd/workspaceapps/cookies.go b/coderd/workspaceapps/cookies.go new file mode 100644 index 0000000000..7eee7fb9da --- /dev/null +++ b/coderd/workspaceapps/cookies.go @@ -0,0 +1,51 @@ +package workspaceapps + +import ( + "net/http" + + "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/codersdk" +) + +// AppConnectSessionTokenCookieName returns the cookie name for the session +// token for the given access method. +func AppConnectSessionTokenCookieName(accessMethod AccessMethod) string { + if accessMethod == AccessMethodSubdomain { + return codersdk.SubdomainAppSessionTokenCookie + } + return codersdk.PathAppSessionTokenCookie +} + +// AppConnectSessionTokenFromRequest returns the session token from the request +// if it exists. The access method is used to determine which cookie name to +// use. +// +// We use different cookie names for path apps and for subdomain apps to avoid +// both being set and sent to the server at the same time and the server using +// the wrong value. +// +// We use different cookie names for: +// - path apps on primary access URL: coder_session_token +// - path apps on proxies: coder_path_app_session_token +// - subdomain apps: coder_subdomain_app_session_token +// +// First we try the default function to get a token from request, which supports +// query parameters, the Coder-Session-Token header and the coder_session_token +// cookie. +// +// Then we try the specific cookie name for the access method. +func AppConnectSessionTokenFromRequest(r *http.Request, accessMethod AccessMethod) string { + // Try the default function first. + token := httpmw.APITokenFromRequest(r) + if token != "" { + return token + } + + // Then try the specific cookie name for the access method. + cookie, err := r.Cookie(AppConnectSessionTokenCookieName(accessMethod)) + if err == nil && cookie.Value != "" { + return cookie.Value + } + + return "" +} diff --git a/coderd/workspaceapps/db_test.go b/coderd/workspaceapps/db_test.go index 163247f6d4..7f1b9c1d45 100644 --- a/coderd/workspaceapps/db_test.go +++ b/coderd/workspaceapps/db_test.go @@ -222,14 +222,14 @@ func Test_ResolveRequest(t *testing.T) { // Try resolving a request for each app as the owner, without a // token, then use the token to resolve each app. for _, app := range allApps { - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceNameOrID: c.workspaceNameOrID, AgentNameOrID: c.agentNameOrID, AppSlugOrPort: app, - } + }).Normalize() t.Log("app", app) rw := httptest.NewRecorder() @@ -268,7 +268,7 @@ func Test_ResolveRequest(t *testing.T) { // Check that the token was set in the response and is valid. require.Len(t, w.Cookies(), 1) cookie := w.Cookies()[0] - require.Equal(t, codersdk.DevURLSignedAppTokenCookie, cookie.Name) + require.Equal(t, codersdk.SignedAppTokenCookie, cookie.Name) require.Equal(t, req.BasePath, cookie.Path) parsedToken, err := api.AppSecurityKey.VerifySignedToken(cookie.Value) @@ -305,14 +305,14 @@ func Test_ResolveRequest(t *testing.T) { t.Parallel() for _, app := range allApps { - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: app, - } + }).Normalize() t.Log("app", app) rw := httptest.NewRecorder() @@ -346,14 +346,14 @@ func Test_ResolveRequest(t *testing.T) { t.Parallel() for _, app := range allApps { - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: app, - } + }).Normalize() t.Log("app", app) rw := httptest.NewRecorder() @@ -391,9 +391,9 @@ func Test_ResolveRequest(t *testing.T) { t.Run("Invalid", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: "invalid", - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) token, ok := workspaceapps.ResolveRequest(rw, r, workspaceapps.ResolveRequestOptions{ @@ -465,13 +465,13 @@ func Test_ResolveRequest(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceAndAgent: c.workspaceAndAgent, AppSlugOrPort: appNamePublic, - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) @@ -510,7 +510,7 @@ func Test_ResolveRequest(t *testing.T) { t.Parallel() badToken := workspaceapps.SignedToken{ - Request: workspaceapps.Request{ + Request: (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, @@ -518,7 +518,7 @@ func Test_ResolveRequest(t *testing.T) { AgentNameOrID: agentName, // App name differs AppSlugOrPort: appNamePublic, - }, + }).Normalize(), Expiry: time.Now().Add(time.Minute), UserID: me.ID, WorkspaceID: workspace.ID, @@ -528,7 +528,7 @@ func Test_ResolveRequest(t *testing.T) { badTokenStr, err := api.AppSecurityKey.SignToken(badToken) require.NoError(t, err) - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, @@ -536,13 +536,13 @@ func Test_ResolveRequest(t *testing.T) { AgentNameOrID: agentName, // App name differs AppSlugOrPort: appNameOwner, - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) r.AddCookie(&http.Cookie{ - Name: codersdk.DevURLSignedAppTokenCookie, + Name: codersdk.SignedAppTokenCookie, Value: badTokenStr, }) @@ -566,7 +566,7 @@ func Test_ResolveRequest(t *testing.T) { _ = w.Body.Close() cookies := w.Cookies() require.Len(t, cookies, 1) - require.Equal(t, cookies[0].Name, codersdk.DevURLSignedAppTokenCookie) + require.Equal(t, cookies[0].Name, codersdk.SignedAppTokenCookie) require.NotEqual(t, cookies[0].Value, badTokenStr) parsedToken, err := api.AppSecurityKey.VerifySignedToken(cookies[0].Value) require.NoError(t, err) @@ -576,14 +576,14 @@ func Test_ResolveRequest(t *testing.T) { t.Run("PortPathBlocked", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: "8080", - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) @@ -604,14 +604,14 @@ func Test_ResolveRequest(t *testing.T) { t.Run("PortSubdomain", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodSubdomain, BasePath: "/", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: "9090", - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/", nil) @@ -633,11 +633,11 @@ func Test_ResolveRequest(t *testing.T) { t.Run("Terminal", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodTerminal, BasePath: "/app", AgentNameOrID: agentID.String(), - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) @@ -664,14 +664,14 @@ func Test_ResolveRequest(t *testing.T) { t.Run("InsufficientPermissions", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: appNameOwner, - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) @@ -691,14 +691,14 @@ func Test_ResolveRequest(t *testing.T) { t.Run("UserNotFound", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: "thisuserdoesnotexist", WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: appNameOwner, - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) @@ -719,14 +719,14 @@ func Test_ResolveRequest(t *testing.T) { t.Run("RedirectSubdomainAuth", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodSubdomain, BasePath: "/", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: appNameOwner, - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/some-path", nil) @@ -771,14 +771,14 @@ func Test_ResolveRequest(t *testing.T) { t.Run("UnhealthyAgent", func(t *testing.T) { t.Parallel() - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentNameUnhealthy, AppSlugOrPort: appNameAgentUnhealthy, - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) @@ -832,14 +832,14 @@ func Test_ResolveRequest(t *testing.T) { return false }, testutil.WaitLong, testutil.IntervalFast, "wait for app to become unhealthy") - req := workspaceapps.Request{ + req := (workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/app", UsernameOrID: me.Username, WorkspaceNameOrID: workspace.Name, AgentNameOrID: agentName, AppSlugOrPort: appNameUnhealthy, - } + }).Normalize() rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/app", nil) diff --git a/coderd/workspaceapps/provider.go b/coderd/workspaceapps/provider.go index 25c18effd5..8d4b7fd149 100644 --- a/coderd/workspaceapps/provider.go +++ b/coderd/workspaceapps/provider.go @@ -7,7 +7,6 @@ import ( "time" "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/codersdk" ) @@ -58,7 +57,7 @@ func ResolveRequest(rw http.ResponseWriter, r *http.Request, opts ResolveRequest AppRequest: appReq, PathAppBaseURL: opts.PathAppBaseURL.String(), AppHostname: opts.AppHostname, - SessionToken: httpmw.APITokenFromRequest(r), + SessionToken: AppConnectSessionTokenFromRequest(r, appReq.AccessMethod), AppPath: opts.AppPath, AppQuery: opts.AppQuery, } @@ -68,11 +67,16 @@ func ResolveRequest(rw http.ResponseWriter, r *http.Request, opts ResolveRequest return nil, false } - // Write the signed app token cookie. We always want this to apply to the - // current hostname (even for subdomain apps, without any wildcard - // shenanigans, because the token is only valid for a single app). + // Write the signed app token cookie. + // + // For path apps, this applies to only the path app base URL on the current + // domain, e.g. + // /@user/workspace[.agent]/apps/path-app/ + // + // For subdomain apps, this applies to the entire subdomain, e.g. + // app--agent--workspace--user.apps.example.com http.SetCookie(rw, &http.Cookie{ - Name: codersdk.DevURLSignedAppTokenCookie, + Name: codersdk.SignedAppTokenCookie, Value: tokenStr, Path: appReq.BasePath, Expires: token.Expiry, diff --git a/coderd/workspaceapps/proxy.go b/coderd/workspaceapps/proxy.go index 5e9babcb85..d252f2c15f 100644 --- a/coderd/workspaceapps/proxy.go +++ b/coderd/workspaceapps/proxy.go @@ -220,8 +220,12 @@ func (s *Server) handleAPIKeySmuggling(rw http.ResponseWriter, r *http.Request, // We don't set an expiration because the key in the database already has an // expiration, and expired tokens don't affect the user experience (they get // auto-redirected to re-smuggle the API key). + // + // We use different cookie names for path apps and for subdomain apps to + // avoid both being set and sent to the server at the same time and the + // server using the wrong value. http.SetCookie(rw, &http.Cookie{ - Name: codersdk.DevURLSessionTokenCookie, + Name: AppConnectSessionTokenCookieName(accessMethod), Value: token, Domain: domain, Path: "/", diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index fbfa010aed..949281d802 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -103,6 +103,10 @@ func (r Request) Normalize() Request { } } + if !strings.HasSuffix(req.BasePath, "/") { + req.BasePath += "/" + } + return req } diff --git a/coderd/workspaceapps/request_test.go b/coderd/workspaceapps/request_test.go index 03ecccd4d7..bc06c13e17 100644 --- a/coderd/workspaceapps/request_test.go +++ b/coderd/workspaceapps/request_test.go @@ -15,6 +15,7 @@ func Test_RequestValidate(t *testing.T) { cases := []struct { name string req workspaceapps.Request + noNormalize bool errContains string }{ { @@ -90,6 +91,7 @@ func Test_RequestValidate(t *testing.T) { AgentNameOrID: "baz", AppSlugOrPort: "qux", }, + noNormalize: true, errContains: "base path is required", }, { @@ -215,7 +217,10 @@ func Test_RequestValidate(t *testing.T) { c := c t.Run(c.name, func(t *testing.T) { t.Parallel() - req := c.req.Normalize() + req := c.req + if !c.noNormalize { + req = c.req.Normalize() + } err := req.Validate() if c.errContains == "" { require.NoError(t, err) diff --git a/coderd/workspaceapps/token.go b/coderd/workspaceapps/token.go index 96181340fc..fdd8575720 100644 --- a/coderd/workspaceapps/token.go +++ b/coderd/workspaceapps/token.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "encoding/json" "net/http" + "strings" "time" "github.com/go-jose/go-jose/v3" @@ -38,8 +39,17 @@ type SignedToken struct { // MatchesRequest returns true if the token matches the request. Any token that // does not match the request should be considered invalid. func (t SignedToken) MatchesRequest(req Request) bool { + tokenBasePath := t.Request.BasePath + if !strings.HasSuffix(tokenBasePath, "/") { + tokenBasePath += "/" + } + reqBasePath := req.BasePath + if !strings.HasSuffix(reqBasePath, "/") { + reqBasePath += "/" + } + return t.AccessMethod == req.AccessMethod && - t.BasePath == req.BasePath && + tokenBasePath == reqBasePath && t.UsernameOrID == req.UsernameOrID && t.WorkspaceNameOrID == req.WorkspaceNameOrID && t.AgentNameOrID == req.AgentNameOrID && @@ -227,22 +237,39 @@ func (k SecurityKey) DecryptAPIKey(encryptedAPIKey string) (string, error) { // FromRequest returns the signed token from the request, if it exists and is // valid. The caller must check that the token matches the request. func FromRequest(r *http.Request, key SecurityKey) (*SignedToken, bool) { - // Get the token string from the request. We usually use a cookie for this, - // but for web terminal we also support a query parameter to support - // cross-domain terminal access. - tokenStr := "" - tokenCookie, cookieErr := r.Cookie(codersdk.DevURLSignedAppTokenCookie) - if cookieErr == nil { - tokenStr = tokenCookie.Value - } else { - tokenStr = r.URL.Query().Get(codersdk.SignedAppTokenQueryParameter) + // Get all signed app tokens from the request. This includes the query + // parameter and all matching cookies sent with the request. If there are + // somehow multiple signed app token cookies, we want to try all of them + // (up to 4). The first one that is valid is used. + // + // Browsers will send all cookies in the request, even if there are multiple + // with the same name on different paths. + // + // If using a query parameter the request MUST be a terminal request. We use + // this to support cross-domain terminal access for the web terminal. + var ( + tokens = []string{} + hasQueryParam = false + ) + if q := r.URL.Query().Get(codersdk.SignedAppTokenQueryParameter); q != "" { + hasQueryParam = true + tokens = append(tokens, q) + } + for _, cookie := range r.Cookies() { + if cookie.Name == codersdk.SignedAppTokenCookie { + tokens = append(tokens, cookie.Value) + } } - if tokenStr != "" { + if len(tokens) > 4 { + tokens = tokens[:4] + } + + for _, tokenStr := range tokens { token, err := key.VerifySignedToken(tokenStr) if err == nil { req := token.Request.Normalize() - if cookieErr != nil && req.AccessMethod != AccessMethodTerminal { + if hasQueryParam && req.AccessMethod != AccessMethodTerminal { // The request must be a terminal request if we're using a // query parameter. return nil, false diff --git a/coderd/workspaceapps/token_test.go b/coderd/workspaceapps/token_test.go index e674fedb9c..8535c61dc2 100644 --- a/coderd/workspaceapps/token_test.go +++ b/coderd/workspaceapps/token_test.go @@ -2,9 +2,13 @@ package workspaceapps_test import ( "fmt" + "net/http" + "net/http/httptest" "testing" "time" + "github.com/coder/coder/v2/codersdk" + "github.com/go-jose/go-jose/v3" "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -46,6 +50,29 @@ func Test_TokenMatchesRequest(t *testing.T) { }, want: true, }, + { + name: "NormalizePath", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "qux", + }, + token: workspaceapps.SignedToken{ + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + // With trailing slash + BasePath: "/app/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "qux", + }, + }, + want: true, + }, { name: "DifferentAccessMethod", req: workspaceapps.Request{ @@ -283,6 +310,62 @@ func Test_GenerateToken(t *testing.T) { } } +func Test_FromRequest(t *testing.T) { + t.Parallel() + + t.Run("MultipleTokens", func(t *testing.T) { + t.Parallel() + r := httptest.NewRequest("GET", "/", nil) + + // Add an invalid token + r.AddCookie(&http.Cookie{ + Name: codersdk.SignedAppTokenCookie, + Value: "invalid", + }) + + token := workspaceapps.SignedToken{ + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: "user", + WorkspaceAndAgent: "workspace/agent", + WorkspaceNameOrID: "workspace", + AgentNameOrID: "agent", + AppSlugOrPort: "app", + }, + Expiry: time.Now().Add(time.Hour), + UserID: uuid.New(), + WorkspaceID: uuid.New(), + AgentID: uuid.New(), + AppURL: "/", + } + + // Add an expired cookie + expired := token + expired.Expiry = time.Now().Add(time.Hour * -1) + expiredStr, err := coderdtest.AppSecurityKey.SignToken(token) + require.NoError(t, err) + r.AddCookie(&http.Cookie{ + Name: codersdk.SignedAppTokenCookie, + Value: expiredStr, + }) + + // Add a valid token + validStr, err := coderdtest.AppSecurityKey.SignToken(token) + require.NoError(t, err) + + r.AddCookie(&http.Cookie{ + Name: codersdk.SignedAppTokenCookie, + Value: validStr, + }) + + signed, ok := workspaceapps.FromRequest(r, coderdtest.AppSecurityKey) + require.True(t, ok, "expected a token to be found") + // Confirm it is the correct token. + require.Equal(t, signed.UserID, token.UserID) + }) +} + // The ParseToken fn is tested quite thoroughly in the GenerateToken test as // well. func Test_ParseToken(t *testing.T) { diff --git a/codersdk/client.go b/codersdk/client.go index 9c34245bcb..aa2f95bcc3 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -38,15 +38,19 @@ const ( // OAuth2RedirectCookie is the name of the cookie that stores the oauth2 redirect. OAuth2RedirectCookie = "oauth_redirect" - // DevURLSessionTokenCookie is the name of the cookie that stores a devurl - // token on app domains. + // PathAppSessionTokenCookie is the name of the cookie that stores an + // application-scoped API token on workspace proxy path app domains. //nolint:gosec - DevURLSessionTokenCookie = "coder_devurl_session_token" - // DevURLSignedAppTokenCookie is the name of the cookie that stores a - // temporary JWT that can be used to authenticate instead of the session - // token. + PathAppSessionTokenCookie = "coder_path_app_session_token" + // SubdomainAppSessionTokenCookie is the name of the cookie that stores an + // application-scoped API token on subdomain app domains (both the primary + // and proxies). //nolint:gosec - DevURLSignedAppTokenCookie = "coder_devurl_signed_app_token" + SubdomainAppSessionTokenCookie = "coder_subdomain_app_session_token" + // SignedAppTokenCookie is the name of the cookie that stores a temporary + // JWT that can be used to authenticate instead of the app session token. + //nolint:gosec + SignedAppTokenCookie = "coder_signed_app_token" // SignedAppTokenQueryParameter is the name of the query parameter that // stores a temporary JWT that can be used to authenticate instead of the // session token. This is only acceptable on reconnecting-pty requests, not