Skip to content

Commit 42dd544

Browse files
deansheatherdeansheather
authored
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.
1 parent 7baaa16 commit 42dd544

11 files changed

Lines changed: 161 additions & 75 deletions

File tree

‎coderd/coderd.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -785,7 +785,7 @@ func New(options *Options) *API {
785785
options.WorkspaceAppsStatsCollectorOptions.Reporter = api.statsReporter
786786
}
787787

788-
api.workspaceAppServer = &workspaceapps.Server{
788+
api.workspaceAppServer = workspaceapps.NewServer(workspaceapps.ServerOptions{
789789
Logger: workspaceAppsLogger,
790790

791791
DashboardURL: api.AccessURL,
@@ -799,9 +799,9 @@ func New(options *Options) *API {
799799
StatsCollector: workspaceapps.NewStatsCollector(options.WorkspaceAppsStatsCollectorOptions),
800800

801801
DisablePathApps: options.DeploymentValues.DisablePathApps.Value(),
802-
Cookies: options.DeploymentValues.HTTPCookies,
802+
CookiesConfig: options.DeploymentValues.HTTPCookies,
803803
APIKeyEncryptionKeycache: options.AppEncryptionKeyCache,
804-
}
804+
})
805805

806806
apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
807807
DB: options.Database,

‎coderd/httpapi/cookie.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,11 @@ func StripCoderCookies(header string) string {
2424
name == codersdk.OAuth2StateCookie ||
2525
name == codersdk.OAuth2RedirectCookie ||
2626
name == codersdk.PathAppSessionTokenCookie ||
27-
name == codersdk.SubdomainAppSessionTokenCookie ||
27+
// This uses a prefix check because the subdomain cookie is unique
28+
// per workspace proxy and is based on a hash of the workspace proxy
29+
// subdomain hostname. See the workspaceapps package for more
30+
// details.
31+
strings.HasPrefix(name, codersdk.SubdomainAppSessionTokenCookie) ||
2832
name == codersdk.SignedAppTokenCookie {
2933
continue
3034
}

‎coderd/httpapi/cookie_test.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,15 @@ func TestStripCoderCookies(t *testing.T) {
2525
}, {
2626
"coder_session_token=ok; oauth_state=wow; oauth_redirect=/",
2727
"",
28+
}, {
29+
"coder_path_app_session_token=ok; wow=test",
30+
"wow=test",
31+
}, {
32+
"coder_subdomain_app_session_token=ok; coder_subdomain_app_session_token_1234567890=ok; wow=test",
33+
"wow=test",
34+
}, {
35+
"coder_signed_app_token=ok; wow=test",
36+
"wow=test",
2837
}} {
2938
t.Run(tc.Input, func(t *testing.T) {
3039
t.Parallel()

‎coderd/workspaceapps/apptest/apptest.go‎

Lines changed: 16 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -264,14 +264,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
264264
require.Equal(t, proxyTestAppBody, string(body))
265265
require.Equal(t, http.StatusOK, resp.StatusCode)
266266

267-
var appTokenCookie *http.Cookie
268-
for _, c := range resp.Cookies() {
269-
if c.Name == codersdk.SignedAppTokenCookie {
270-
appTokenCookie = c
271-
break
272-
}
273-
}
274-
require.NotNil(t, appTokenCookie, "no signed app token cookie in response")
267+
appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
275268
require.Equal(t, appTokenCookie.Path, u.Path, "incorrect path on app token cookie")
276269

277270
// Ensure the signed app token cookie is valid.
@@ -310,14 +303,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
310303
require.Equal(t, proxyTestAppBody, string(body))
311304
require.Equal(t, http.StatusOK, resp.StatusCode)
312305

313-
var appTokenCookie *http.Cookie
314-
for _, c := range resp.Cookies() {
315-
if c.Name == codersdk.SignedAppTokenCookie {
316-
appTokenCookie = c
317-
break
318-
}
319-
}
320-
require.NotNil(t, appTokenCookie, "no signed app token cookie in response")
306+
appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
321307
require.Equal(t, appTokenCookie.Path, u.Path, "incorrect path on app token cookie")
322308

323309
// Ensure the signed app token cookie is valid.
@@ -426,8 +412,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
426412
require.Equal(t, proxyTestAppBody, string(body))
427413
require.Equal(t, http.StatusOK, resp.StatusCode)
428414

429-
appTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie)
430-
require.NotNil(t, appTokenCookie, "no signed app token cookie in response")
415+
appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
431416
require.Equal(t, appTokenCookie.Path, u.Path, "incorrect path on app token cookie")
432417

433418
object, err := jose.ParseSigned(appTokenCookie.Value, []jose.SignatureAlgorithm{jwtutils.SigningAlgo})
@@ -467,7 +452,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
467452
assertWorkspaceLastUsedAtUpdated(ctx, t, appDetails)
468453

469454
// Since the old token is invalid, the signed app token cookie should have a new value.
470-
newTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie)
455+
newTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
471456
require.NotEqual(t, appTokenCookie.Value, newTokenCookie.Value)
472457
})
473458
})
@@ -978,15 +963,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
978963
resp.Body.Close()
979964
require.Equal(t, http.StatusSeeOther, resp.StatusCode)
980965

981-
cookies := resp.Cookies()
982-
var cookie *http.Cookie
983-
for _, co := range cookies {
984-
if co.Name == c.sessionTokenCookieName {
985-
cookie = co
986-
break
987-
}
988-
}
989-
require.NotNil(t, cookie, "no app session token cookie was set")
966+
cookie := mustFindCookie(t, resp.Cookies(), c.sessionTokenCookieName)
990967
apiKey := cookie.Value
991968

992969
// Fetch the API key from the API.
@@ -1102,14 +1079,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
11021079

11031080
// Parse the returned signed token to verify that it contains the
11041081
// prefix.
1105-
var appTokenCookie *http.Cookie
1106-
for _, c := range resp.Cookies() {
1107-
if c.Name == codersdk.SignedAppTokenCookie {
1108-
appTokenCookie = c
1109-
break
1110-
}
1111-
}
1112-
require.NotNil(t, appTokenCookie, "no signed app token cookie in response")
1082+
appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
11131083

11141084
// Parse the JWT without verifying it (since we can't access the key
11151085
// from this test).
@@ -1334,14 +1304,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
13341304
require.Equal(t, proxyTestAppBody, string(body))
13351305
require.Equal(t, http.StatusOK, resp.StatusCode)
13361306

1337-
var appTokenCookie *http.Cookie
1338-
for _, c := range resp.Cookies() {
1339-
if c.Name == codersdk.SignedAppTokenCookie {
1340-
appTokenCookie = c
1341-
break
1342-
}
1343-
}
1344-
require.NotNil(t, appTokenCookie, "no signed token cookie in response")
1307+
appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
13451308
require.Equal(t, appTokenCookie.Path, "/", "incorrect path on signed token cookie")
13461309

13471310
// Ensure the signed app token cookie is valid.
@@ -1589,8 +1552,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
15891552
require.Equal(t, proxyTestAppBody, string(body))
15901553
require.Equal(t, http.StatusOK, resp.StatusCode)
15911554

1592-
appTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie)
1593-
require.NotNil(t, appTokenCookie, "no signed token cookie in response")
1555+
appTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
15941556
require.Equal(t, appTokenCookie.Path, "/", "incorrect path on signed token cookie")
15951557

15961558
object, err := jose.ParseSigned(appTokenCookie.Value, []jose.SignatureAlgorithm{jwtutils.SigningAlgo})
@@ -1614,7 +1576,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
16141576
[]*http.Cookie{
16151577
appTokenCookie,
16161578
{
1617-
Name: codersdk.SubdomainAppSessionTokenCookie,
1579+
Name: codersdk.SessionTokenCookie,
16181580
Value: apiKey,
16191581
},
16201582
},
@@ -1631,7 +1593,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
16311593
assertWorkspaceLastUsedAtUpdated(ctx, t, appDetails)
16321594

16331595
// Since the old token is invalid, the signed app token cookie should have a new value.
1634-
newTokenCookie := findCookie(resp.Cookies(), codersdk.SignedAppTokenCookie)
1596+
newTokenCookie := mustFindCookie(t, resp.Cookies(), codersdk.SignedAppTokenCookie)
16351597
require.NotEqual(t, appTokenCookie.Value, newTokenCookie.Value)
16361598
})
16371599
})
@@ -2542,11 +2504,15 @@ func generateBadJWT(t *testing.T, claims interface{}) string {
25422504
return compact
25432505
}
25442506

2545-
func findCookie(cookies []*http.Cookie, name string) *http.Cookie {
2507+
func mustFindCookie(t *testing.T, cookies []*http.Cookie, prefix string) *http.Cookie {
2508+
t.Helper()
25462509
for _, cookie := range cookies {
2547-
if cookie.Name == name {
2510+
t.Logf("testing cookie against prefix %q: %q", prefix, cookie.Name)
2511+
if strings.HasPrefix(cookie.Name, prefix) {
2512+
t.Logf("cookie %q found", cookie.Name)
25482513
return cookie
25492514
}
25502515
}
2516+
t.Fatalf("cookie with prefix %q not found", prefix)
25512517
return nil
25522518
}

‎coderd/workspaceapps/cookies.go‎

Lines changed: 50 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,62 @@
11
package workspaceapps
22

33
import (
4+
"crypto/sha256"
5+
"encoding/hex"
46
"net/http"
57

68
"github.com/coder/coder/v2/coderd/httpmw"
79
"github.com/coder/coder/v2/codersdk"
810
)
911

10-
// AppConnectSessionTokenCookieName returns the cookie name for the session
12+
type AppCookies struct {
13+
PathAppSessionToken string
14+
SubdomainAppSessionToken string
15+
SignedAppToken string
16+
}
17+
18+
// NewAppCookies returns the cookie names for the app session token for the
19+
// given hostname. The subdomain cookie is unique per workspace proxy and is
20+
// based on a hash of the workspace proxy subdomain hostname. See
21+
// SubdomainAppSessionTokenCookie for more details.
22+
func NewAppCookies(hostname string) AppCookies {
23+
return AppCookies{
24+
PathAppSessionToken: codersdk.PathAppSessionTokenCookie,
25+
SubdomainAppSessionToken: SubdomainAppSessionTokenCookie(hostname),
26+
SignedAppToken: codersdk.SignedAppTokenCookie,
27+
}
28+
}
29+
30+
// CookieNameForAccessMethod returns the cookie name for the long-lived session
1131
// token for the given access method.
12-
func AppConnectSessionTokenCookieName(accessMethod AccessMethod) string {
32+
func (c AppCookies) CookieNameForAccessMethod(accessMethod AccessMethod) string {
1333
if accessMethod == AccessMethodSubdomain {
14-
return codersdk.SubdomainAppSessionTokenCookie
34+
return c.SubdomainAppSessionToken
1535
}
16-
return codersdk.PathAppSessionTokenCookie
36+
// Path-based and terminal apps are on the same domain:
37+
return c.PathAppSessionToken
38+
}
39+
40+
// SubdomainAppSessionTokenCookie returns the cookie name for the subdomain app
41+
// session token. This is unique per workspace proxy and is based on a hash of
42+
// the workspace proxy subdomain hostname.
43+
//
44+
// The reason the cookie needs to be unique per workspace proxy is to avoid
45+
// cookies from one proxy (e.g. the primary) being sent on requests to a
46+
// different proxy underneath the wildcard.
47+
//
48+
// E.g. `*.dev.coder.com` and `*.sydney.dev.coder.com`
49+
//
50+
// If you have an expired cookie on the primary proxy (valid for
51+
// `*.dev.coder.com`), your browser will send it on all requests to the Sydney
52+
// proxy as it's underneath the wildcard.
53+
//
54+
// By using a unique cookie name per workspace proxy, we can avoid this issue.
55+
func SubdomainAppSessionTokenCookie(hostname string) string {
56+
hash := sha256.Sum256([]byte(hostname))
57+
// 16 bytes of uniqueness is probably enough.
58+
str := hex.EncodeToString(hash[:16])
59+
return codersdk.SubdomainAppSessionTokenCookie + "_" + str
1760
}
1861

1962
// AppConnectSessionTokenFromRequest returns the session token from the request
@@ -27,22 +70,22 @@ func AppConnectSessionTokenCookieName(accessMethod AccessMethod) string {
2770
// We use different cookie names for:
2871
// - path apps on primary access URL: coder_session_token
2972
// - path apps on proxies: coder_path_app_session_token
30-
// - subdomain apps: coder_subdomain_app_session_token
73+
// - subdomain apps: coder_subdomain_app_session_token_{unique_hash}
3174
//
3275
// First we try the default function to get a token from request, which supports
3376
// query parameters, the Coder-Session-Token header and the coder_session_token
3477
// cookie.
3578
//
3679
// Then we try the specific cookie name for the access method.
37-
func AppConnectSessionTokenFromRequest(r *http.Request, accessMethod AccessMethod) string {
80+
func (c AppCookies) TokenFromRequest(r *http.Request, accessMethod AccessMethod) string {
3881
// Try the default function first.
3982
token := httpmw.APITokenFromRequest(r)
4083
if token != "" {
4184
return token
4285
}
4386

4487
// Then try the specific cookie name for the access method.
45-
cookie, err := r.Cookie(AppConnectSessionTokenCookieName(accessMethod))
88+
cookie, err := r.Cookie(c.CookieNameForAccessMethod(accessMethod))
4689
if err == nil && cookie.Value != "" {
4790
return cookie.Value
4891
}

‎coderd/workspaceapps/cookies_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package workspaceapps_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/require"
7+
8+
"github.com/coder/coder/v2/coderd/workspaceapps"
9+
"github.com/coder/coder/v2/codersdk"
10+
)
11+
12+
func TestAppCookies(t *testing.T) {
13+
t.Parallel()
14+
15+
const (
16+
domain = "example.com"
17+
hash = "a379a6f6eeafb9a55e378c118034e275"
18+
expectedSubdomainCookie = codersdk.SubdomainAppSessionTokenCookie + "_" + hash
19+
)
20+
21+
cookies := workspaceapps.NewAppCookies(domain)
22+
require.Equal(t, codersdk.PathAppSessionTokenCookie, cookies.PathAppSessionToken)
23+
require.Equal(t, expectedSubdomainCookie, cookies.SubdomainAppSessionToken)
24+
require.Equal(t, codersdk.SignedAppTokenCookie, cookies.SignedAppToken)
25+
26+
require.Equal(t, cookies.PathAppSessionToken, cookies.CookieNameForAccessMethod(workspaceapps.AccessMethodPath))
27+
require.Equal(t, cookies.PathAppSessionToken, cookies.CookieNameForAccessMethod(workspaceapps.AccessMethodTerminal))
28+
require.Equal(t, cookies.SubdomainAppSessionToken, cookies.CookieNameForAccessMethod(workspaceapps.AccessMethodSubdomain))
29+
30+
// A new cookies object with a different domain should have a different
31+
// subdomain cookie.
32+
newCookies := workspaceapps.NewAppCookies("different.com")
33+
require.NotEqual(t, cookies.SubdomainAppSessionToken, newCookies.SubdomainAppSessionToken)
34+
}

‎coderd/workspaceapps/db_test.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,6 +1236,15 @@ func workspaceappsResolveRequest(t testing.TB, connLogger connectionlog.Connecti
12361236
if opts.SignedTokenProvider != nil && connLogger != nil {
12371237
opts.SignedTokenProvider = signedTokenProviderWithConnLogger(t, opts.SignedTokenProvider, connLogger, time.Hour)
12381238
}
1239+
if opts.Cookies.PathAppSessionToken == "" {
1240+
opts.Cookies.PathAppSessionToken = codersdk.PathAppSessionTokenCookie
1241+
}
1242+
if opts.Cookies.SubdomainAppSessionToken == "" {
1243+
opts.Cookies.SubdomainAppSessionToken = codersdk.SubdomainAppSessionTokenCookie + "_test"
1244+
}
1245+
if opts.Cookies.SignedAppToken == "" {
1246+
opts.Cookies.SignedAppToken = codersdk.SignedAppTokenCookie
1247+
}
12391248

12401249
tracing.StatusWriterMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
12411250
httpmw.AttachRequestID(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

‎coderd/workspaceapps/provider.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ const (
2222
type ResolveRequestOptions struct {
2323
Logger slog.Logger
2424
SignedTokenProvider SignedTokenProvider
25+
Cookies AppCookies
2526
CookieCfg codersdk.HTTPCookieConfig
2627

2728
DashboardURL *url.URL
@@ -58,7 +59,7 @@ func ResolveRequest(rw http.ResponseWriter, r *http.Request, opts ResolveRequest
5859
AppRequest: appReq,
5960
PathAppBaseURL: opts.PathAppBaseURL.String(),
6061
AppHostname: opts.AppHostname,
61-
SessionToken: AppConnectSessionTokenFromRequest(r, appReq.AccessMethod),
62+
SessionToken: opts.Cookies.TokenFromRequest(r, appReq.AccessMethod),
6263
AppPath: opts.AppPath,
6364
AppQuery: opts.AppQuery,
6465
}

0 commit comments

Comments
 (0)