diff --git a/coderd/coderd.go b/coderd/coderd.go index eb4436cb16..3d14ea345c 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -785,7 +785,7 @@ func New(options *Options) *API { options.WorkspaceAppsStatsCollectorOptions.Reporter = api.statsReporter } - api.workspaceAppServer = &workspaceapps.Server{ + api.workspaceAppServer = workspaceapps.NewServer(workspaceapps.ServerOptions{ Logger: workspaceAppsLogger, DashboardURL: api.AccessURL, @@ -799,9 +799,9 @@ func New(options *Options) *API { StatsCollector: workspaceapps.NewStatsCollector(options.WorkspaceAppsStatsCollectorOptions), DisablePathApps: options.DeploymentValues.DisablePathApps.Value(), - Cookies: options.DeploymentValues.HTTPCookies, + CookiesConfig: options.DeploymentValues.HTTPCookies, APIKeyEncryptionKeycache: options.AppEncryptionKeyCache, - } + }) apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: options.Database, diff --git a/coderd/httpapi/cookie.go b/coderd/httpapi/cookie.go index 526dfb8207..e950865244 100644 --- a/coderd/httpapi/cookie.go +++ b/coderd/httpapi/cookie.go @@ -24,7 +24,11 @@ func StripCoderCookies(header string) string { name == codersdk.OAuth2StateCookie || name == codersdk.OAuth2RedirectCookie || name == codersdk.PathAppSessionTokenCookie || - name == codersdk.SubdomainAppSessionTokenCookie || + // This uses a prefix check because the subdomain cookie is unique + // per workspace proxy and is based on a hash of the workspace proxy + // subdomain hostname. See the workspaceapps package for more + // details. + strings.HasPrefix(name, codersdk.SubdomainAppSessionTokenCookie) || name == codersdk.SignedAppTokenCookie { continue } diff --git a/coderd/httpapi/cookie_test.go b/coderd/httpapi/cookie_test.go index e653e3653b..c92a5ff3ae 100644 --- a/coderd/httpapi/cookie_test.go +++ b/coderd/httpapi/cookie_test.go @@ -25,6 +25,15 @@ func TestStripCoderCookies(t *testing.T) { }, { "coder_session_token=ok; oauth_state=wow; oauth_redirect=/", "", + }, { + "coder_path_app_session_token=ok; wow=test", + "wow=test", + }, { + "coder_subdomain_app_session_token=ok; coder_subdomain_app_session_token_1234567890=ok; wow=test", + "wow=test", + }, { + "coder_signed_app_token=ok; wow=test", + "wow=test", }} { t.Run(tc.Input, func(t *testing.T) { t.Parallel() diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 1d15ecda08..07b54b7b3f 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -264,14 +264,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) - var appTokenCookie *http.Cookie - for _, c := range resp.Cookies() { - if c.Name == codersdk.SignedAppTokenCookie { - appTokenCookie = c - break - } - } - require.NotNil(t, appTokenCookie, "no signed app token cookie in response") + appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) require.Equal(t, appTokenCookie.Path, u.Path, "incorrect path on app token cookie") // Ensure the signed app token cookie is valid. @@ -310,14 +303,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) - var appTokenCookie *http.Cookie - for _, c := range resp.Cookies() { - if c.Name == codersdk.SignedAppTokenCookie { - appTokenCookie = c - break - } - } - require.NotNil(t, appTokenCookie, "no signed app token cookie in response") + appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) require.Equal(t, appTokenCookie.Path, u.Path, "incorrect path on app token cookie") // Ensure the signed app token cookie is valid. @@ -426,8 +412,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) - appTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie) - require.NotNil(t, appTokenCookie, "no signed app token cookie in response") + appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) require.Equal(t, appTokenCookie.Path, u.Path, "incorrect path on app token cookie") object, err := jose.ParseSigned(appTokenCookie.Value, []jose.SignatureAlgorithm{jwtutils.SigningAlgo}) @@ -467,7 +452,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { assertWorkspaceLastUsedAtUpdated(ctx, t, appDetails) // Since the old token is invalid, the signed app token cookie should have a new value. - newTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie) + newTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) require.NotEqual(t, appTokenCookie.Value, newTokenCookie.Value) }) }) @@ -978,15 +963,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { resp.Body.Close() require.Equal(t, http.StatusSeeOther, resp.StatusCode) - cookies := resp.Cookies() - var cookie *http.Cookie - for _, co := range cookies { - if co.Name == c.sessionTokenCookieName { - cookie = co - break - } - } - require.NotNil(t, cookie, "no app session token cookie was set") + cookie := mustFindCookie(t, resp.Cookies(), c.sessionTokenCookieName) apiKey := cookie.Value // Fetch the API key from the API. @@ -1102,14 +1079,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // Parse the returned signed token to verify that it contains the // prefix. - var appTokenCookie *http.Cookie - for _, c := range resp.Cookies() { - if c.Name == codersdk.SignedAppTokenCookie { - appTokenCookie = c - break - } - } - require.NotNil(t, appTokenCookie, "no signed app token cookie in response") + appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) // Parse the JWT without verifying it (since we can't access the key // from this test). @@ -1334,14 +1304,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) - var appTokenCookie *http.Cookie - for _, c := range resp.Cookies() { - if c.Name == codersdk.SignedAppTokenCookie { - appTokenCookie = c - break - } - } - require.NotNil(t, appTokenCookie, "no signed token cookie in response") + appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) require.Equal(t, appTokenCookie.Path, "/", "incorrect path on signed token cookie") // Ensure the signed app token cookie is valid. @@ -1589,8 +1552,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) - appTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie) - require.NotNil(t, appTokenCookie, "no signed token cookie in response") + appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) require.Equal(t, appTokenCookie.Path, "/", "incorrect path on signed token cookie") object, err := jose.ParseSigned(appTokenCookie.Value, []jose.SignatureAlgorithm{jwtutils.SigningAlgo}) @@ -1614,7 +1576,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { []*http.Cookie{ appTokenCookie, { - Name: codersdk.SubdomainAppSessionTokenCookie, + Name: codersdk.SessionTokenCookie, Value: apiKey, }, }, @@ -1631,7 +1593,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { assertWorkspaceLastUsedAtUpdated(ctx, t, appDetails) // Since the old token is invalid, the signed app token cookie should have a new value. - newTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie) + newTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie) require.NotEqual(t, appTokenCookie.Value, newTokenCookie.Value) }) }) @@ -2542,11 +2504,15 @@ func generateBadJWT(t *testing.T, claims interface{}) string { return compact } -func findCookie(cookies []*http.Cookie, name string) *http.Cookie { +func mustFindCookie(t *testing.T, cookies []*http.Cookie, prefix string) *http.Cookie { + t.Helper() for _, cookie := range cookies { - if cookie.Name == name { + t.Logf("testing cookie against prefix %q: %q", prefix, cookie.Name) + if strings.HasPrefix(cookie.Name, prefix) { + t.Logf("cookie %q found", cookie.Name) return cookie } } + t.Fatalf("cookie with prefix %q not found", prefix) return nil } diff --git a/coderd/workspaceapps/cookies.go b/coderd/workspaceapps/cookies.go index 7eee7fb9da..28169fe18c 100644 --- a/coderd/workspaceapps/cookies.go +++ b/coderd/workspaceapps/cookies.go @@ -1,19 +1,62 @@ package workspaceapps import ( + "crypto/sha256" + "encoding/hex" "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 +type AppCookies struct { + PathAppSessionToken string + SubdomainAppSessionToken string + SignedAppToken string +} + +// NewAppCookies returns the cookie names for the app session token for the +// given hostname. The subdomain cookie is unique per workspace proxy and is +// based on a hash of the workspace proxy subdomain hostname. See +// SubdomainAppSessionTokenCookie for more details. +func NewAppCookies(hostname string) AppCookies { + return AppCookies{ + PathAppSessionToken: codersdk.PathAppSessionTokenCookie, + SubdomainAppSessionToken: SubdomainAppSessionTokenCookie(hostname), + SignedAppToken: codersdk.SignedAppTokenCookie, } - return codersdk.PathAppSessionTokenCookie +} + +// CookieNameForAccessMethod returns the cookie name for the long-lived session +// token for the given access method. +func (c AppCookies) CookieNameForAccessMethod(accessMethod AccessMethod) string { + if accessMethod == AccessMethodSubdomain { + return c.SubdomainAppSessionToken + } + // Path-based and terminal apps are on the same domain: + return c.PathAppSessionToken +} + +// SubdomainAppSessionTokenCookie returns the cookie name for the subdomain app +// session token. This is unique per workspace proxy and is based on a hash of +// the workspace proxy subdomain hostname. +// +// The reason the cookie needs to be unique per workspace proxy is to avoid +// cookies from one proxy (e.g. the primary) being sent on requests to a +// different proxy underneath the wildcard. +// +// E.g. `*.dev.coder.com` and `*.sydney.dev.coder.com` +// +// If you have an expired cookie on the primary proxy (valid for +// `*.dev.coder.com`), your browser will send it on all requests to the Sydney +// proxy as it's underneath the wildcard. +// +// By using a unique cookie name per workspace proxy, we can avoid this issue. +func SubdomainAppSessionTokenCookie(hostname string) string { + hash := sha256.Sum256([]byte(hostname)) + // 16 bytes of uniqueness is probably enough. + str := hex.EncodeToString(hash[:16]) + return codersdk.SubdomainAppSessionTokenCookie + "_" + str } // AppConnectSessionTokenFromRequest returns the session token from the request @@ -27,14 +70,14 @@ func AppConnectSessionTokenCookieName(accessMethod AccessMethod) string { // 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 +// - subdomain apps: coder_subdomain_app_session_token_{unique_hash} // // 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 { +func (c AppCookies) TokenFromRequest(r *http.Request, accessMethod AccessMethod) string { // Try the default function first. token := httpmw.APITokenFromRequest(r) if token != "" { @@ -42,7 +85,7 @@ func AppConnectSessionTokenFromRequest(r *http.Request, accessMethod AccessMetho } // Then try the specific cookie name for the access method. - cookie, err := r.Cookie(AppConnectSessionTokenCookieName(accessMethod)) + cookie, err := r.Cookie(c.CookieNameForAccessMethod(accessMethod)) if err == nil && cookie.Value != "" { return cookie.Value } diff --git a/coderd/workspaceapps/cookies_test.go b/coderd/workspaceapps/cookies_test.go new file mode 100644 index 0000000000..898c35c995 --- /dev/null +++ b/coderd/workspaceapps/cookies_test.go @@ -0,0 +1,34 @@ +package workspaceapps_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/workspaceapps" + "github.com/coder/coder/v2/codersdk" +) + +func TestAppCookies(t *testing.T) { + t.Parallel() + + const ( + domain = "example.com" + hash = "a379a6f6eeafb9a55e378c118034e275" + expectedSubdomainCookie = codersdk.SubdomainAppSessionTokenCookie + "_" + hash + ) + + cookies := workspaceapps.NewAppCookies(domain) + require.Equal(t, codersdk.PathAppSessionTokenCookie, cookies.PathAppSessionToken) + require.Equal(t, expectedSubdomainCookie, cookies.SubdomainAppSessionToken) + require.Equal(t, codersdk.SignedAppTokenCookie, cookies.SignedAppToken) + + require.Equal(t, cookies.PathAppSessionToken, cookies.CookieNameForAccessMethod(workspaceapps.AccessMethodPath)) + require.Equal(t, cookies.PathAppSessionToken, cookies.CookieNameForAccessMethod(workspaceapps.AccessMethodTerminal)) + require.Equal(t, cookies.SubdomainAppSessionToken, cookies.CookieNameForAccessMethod(workspaceapps.AccessMethodSubdomain)) + + // A new cookies object with a different domain should have a different + // subdomain cookie. + newCookies := workspaceapps.NewAppCookies("different.com") + require.NotEqual(t, cookies.SubdomainAppSessionToken, newCookies.SubdomainAppSessionToken) +} diff --git a/coderd/workspaceapps/db_test.go b/coderd/workspaceapps/db_test.go index 22669d568b..a7ad1a85e5 100644 --- a/coderd/workspaceapps/db_test.go +++ b/coderd/workspaceapps/db_test.go @@ -1236,6 +1236,15 @@ func workspaceappsResolveRequest(t testing.TB, connLogger connectionlog.Connecti if opts.SignedTokenProvider != nil && connLogger != nil { opts.SignedTokenProvider = signedTokenProviderWithConnLogger(t, opts.SignedTokenProvider, connLogger, time.Hour) } + if opts.Cookies.PathAppSessionToken == "" { + opts.Cookies.PathAppSessionToken = codersdk.PathAppSessionTokenCookie + } + if opts.Cookies.SubdomainAppSessionToken == "" { + opts.Cookies.SubdomainAppSessionToken = codersdk.SubdomainAppSessionTokenCookie + "_test" + } + if opts.Cookies.SignedAppToken == "" { + opts.Cookies.SignedAppToken = codersdk.SignedAppTokenCookie + } tracing.StatusWriterMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { httpmw.AttachRequestID(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/coderd/workspaceapps/provider.go b/coderd/workspaceapps/provider.go index 227ced5563..f18153aecc 100644 --- a/coderd/workspaceapps/provider.go +++ b/coderd/workspaceapps/provider.go @@ -22,6 +22,7 @@ const ( type ResolveRequestOptions struct { Logger slog.Logger SignedTokenProvider SignedTokenProvider + Cookies AppCookies CookieCfg codersdk.HTTPCookieConfig DashboardURL *url.URL @@ -58,7 +59,7 @@ func ResolveRequest(rw http.ResponseWriter, r *http.Request, opts ResolveRequest AppRequest: appReq, PathAppBaseURL: opts.PathAppBaseURL.String(), AppHostname: opts.AppHostname, - SessionToken: AppConnectSessionTokenFromRequest(r, appReq.AccessMethod), + SessionToken: opts.Cookies.TokenFromRequest(r, appReq.AccessMethod), AppPath: opts.AppPath, AppQuery: opts.AppQuery, } diff --git a/coderd/workspaceapps/proxy.go b/coderd/workspaceapps/proxy.go index 002bb1ea05..981bba4584 100644 --- a/coderd/workspaceapps/proxy.go +++ b/coderd/workspaceapps/proxy.go @@ -81,11 +81,7 @@ type AgentProvider interface { Close() error } -// Server serves workspace apps endpoints, including: -// - Path-based apps -// - Subdomain app middleware -// - Workspace reconnecting-pty (aka. web terminal) -type Server struct { +type ServerOptions struct { Logger slog.Logger // DashboardURL should be a url to the coderd dashboard. This can be the @@ -112,15 +108,32 @@ type Server struct { // Subdomain apps are safer with their cookies scoped to the subdomain, and XSS // calls to the dashboard are not possible due to CORs. DisablePathApps bool - Cookies codersdk.HTTPCookieConfig + CookiesConfig codersdk.HTTPCookieConfig AgentProvider AgentProvider StatsCollector *StatsCollector +} + +// Server serves workspace apps endpoints, including: +// - Path-based apps +// - Subdomain app middleware +// - Workspace reconnecting-pty (aka. web terminal) +type Server struct { + ServerOptions + + cookies AppCookies websocketWaitMutex sync.Mutex websocketWaitGroup sync.WaitGroup } +func NewServer(options ServerOptions) *Server { + return &Server{ + ServerOptions: options, + cookies: NewAppCookies(options.Hostname), + } +} + // Close waits for all reconnecting-pty WebSocket connections to drain before // returning. func (s *Server) Close() error { @@ -231,8 +244,8 @@ func (s *Server) handleAPIKeySmuggling(rw http.ResponseWriter, r *http.Request, // 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, s.Cookies.Apply(&http.Cookie{ - Name: AppConnectSessionTokenCookieName(accessMethod), + http.SetCookie(rw, s.CookiesConfig.Apply(&http.Cookie{ + Name: s.cookies.CookieNameForAccessMethod(accessMethod), Value: payload.APIKey, Domain: domain, Path: "/", @@ -299,7 +312,8 @@ func (s *Server) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) // permissions to connect to a workspace. token, ok := ResolveRequest(rw, r, ResolveRequestOptions{ Logger: s.Logger, - CookieCfg: s.Cookies, + Cookies: s.cookies, + CookieCfg: s.CookiesConfig, SignedTokenProvider: s.SignedTokenProvider, DashboardURL: s.DashboardURL, PathAppBaseURL: s.AccessURL, @@ -433,6 +447,8 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler) // Generate a signed token for the request. token, ok := ResolveRequest(rw, r, ResolveRequestOptions{ Logger: s.Logger, + Cookies: s.cookies, + CookieCfg: s.CookiesConfig, SignedTokenProvider: s.SignedTokenProvider, DashboardURL: s.DashboardURL, PathAppBaseURL: s.AccessURL, @@ -666,7 +682,8 @@ func (s *Server) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { appToken, ok := ResolveRequest(rw, r, ResolveRequestOptions{ Logger: s.Logger, - CookieCfg: s.Cookies, + Cookies: s.cookies, + CookieCfg: s.CookiesConfig, SignedTokenProvider: s.SignedTokenProvider, DashboardURL: s.DashboardURL, PathAppBaseURL: s.AccessURL, diff --git a/codersdk/client.go b/codersdk/client.go index 6c1003d0c9..e71703c751 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -48,6 +48,9 @@ const ( // SubdomainAppSessionTokenCookie is the name of the cookie that stores an // application-scoped API token on subdomain app domains (both the primary // and proxies). + // + // To avoid conflicts between multiple proxies, we append an underscore and + // a hash suffix to the cookie name. //nolint:gosec SubdomainAppSessionTokenCookie = "coder_subdomain_app_session_token" // SignedAppTokenCookie is the name of the cookie that stores a temporary diff --git a/enterprise/wsproxy/wsproxy.go b/enterprise/wsproxy/wsproxy.go index 6e1da2f258..94b1802959 100644 --- a/enterprise/wsproxy/wsproxy.go +++ b/enterprise/wsproxy/wsproxy.go @@ -290,7 +290,7 @@ func New(ctx context.Context, opts *Options) (*Server, error) { opts.StatsCollectorOptions.Reporter = &appStatsReporter{Client: client} } - s.AppServer = &workspaceapps.Server{ + s.AppServer = workspaceapps.NewServer(workspaceapps.ServerOptions{ Logger: workspaceAppsLogger, DashboardURL: opts.DashboardURL, AccessURL: opts.AccessURL, @@ -308,12 +308,12 @@ func New(ctx context.Context, opts *Options) (*Server, error) { }, DisablePathApps: opts.DisablePathApps, - Cookies: opts.CookieConfig, + CookiesConfig: opts.CookieConfig, AgentProvider: agentProvider, StatsCollector: workspaceapps.NewStatsCollector(opts.StatsCollectorOptions), APIKeyEncryptionKeycache: encryptionCache, - } + }) derpHandler := derphttp.Handler(derpServer) derpHandler, s.derpCloseFunc = tailnet.WithWebsocketSupport(derpServer, derpHandler)