fix: use unique cookies for workspace proxies (#19930)

There is currently an issue with subdomain workspace apps on workspace
proxies, where if you have a workspace proxy wildcard nested beneath the
primary wildcard, cookies from the primary may be sent to the server
before cookies from the proxy specifically.

Currently:
1. Use a subdomain app via the primary proxy `*.coder.corp.com`
    a. Client sends no cookies
    a. Server does token smuggling flow
a. Server sets a cookie `coder_subdomain_app_session_token` on
`*.coder.corp.com`
    a. Server redirects client to reload the page
    a. Request should succeed as usual
1. Wait until the primary proxy's session token cookie has expired in
the database (or make it invalid yourself)
1. Use a subdomain app via a separate proxy `*.sydney.coder.corp.com`
a. Client sends `coder_subdomain_app_session_token` cookie from
`*.coder.corp.com`
    a. Server validates supplied cookie, it fails because it's expired
    a. Server does token smuggling flow
a. Server sets a cookie `coder_subdomain_app_session_token` on
`*.sydney.coder.corp.com`
    a. Server redirects client to reload page
    a. Client sends BOTH cookies.
a. The server will only process the first cookie it receives, so if the
expired cookie for the primary proxy is sent first the request will end
up in a permanent loop on step b.

The fix is to append `_{hash(wildcard_access_url)}` to the subdomain
cookies as we cannot control browser behavior further. This avoids the
conflict as each proxy will only read it's specific cookie.
This commit is contained in:
Dean Sheather
2025-09-25 00:30:02 +10:00
committed by GitHub
parent 7baaa16ac1
commit 42dd544d90
11 changed files with 163 additions and 77 deletions
+3 -3
View File
@@ -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,
+5 -1
View File
@@ -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
}
+9
View File
@@ -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()
+16 -50
View File
@@ -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
}
+52 -9
View File
@@ -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
}
return codersdk.PathAppSessionTokenCookie
// 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,
}
}
// 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
}
+34
View File
@@ -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)
}
+9
View File
@@ -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) {
+2 -1
View File
@@ -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,
}
+27 -10
View File
@@ -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,
+3
View File
@@ -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
+3 -3
View File
@@ -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)