Skip to content

Commit 52d5558

Browse files
EmyrkEmyrk
authored
chore: add custom samesite options to auth cookies (coder#16885)
Allows controlling `samesite` cookie settings from the deployment config
1 parent 389e88e commit 52d5558

File tree

26 files changed

+240
-67
lines changed

26 files changed

+240
-67
lines changed

‎cli/server.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -641,7 +641,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
641641
GoogleTokenValidator: googleTokenValidator,
642642
ExternalAuthConfigs: externalAuthConfigs,
643643
RealIPConfig: realIPConfig,
644-
SecureAuthCookie: vals.SecureAuthCookie.Value(),
645644
SSHKeygenAlgorithm: sshKeygenAlgorithm,
646645
TracerProvider: tracerProvider,
647646
Telemetry: telemetry.NewNoop(),

‎cli/testdata/coder_server_--help.golden

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,9 @@ NETWORKING OPTIONS:
251251
Specifies whether to redirect requests that do not match the access
252252
URL host.
253253

254+
--samesite-auth-cookie lax|none, $CODER_SAMESITE_AUTH_COOKIE (default: lax)
255+
Controls the 'SameSite' property is set on browser session cookies.
256+
254257
--secure-auth-cookie bool, $CODER_SECURE_AUTH_COOKIE
255258
Controls if the 'Secure' property is set on browser session cookies.
256259

‎cli/testdata/server-config.yaml.golden

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,9 @@ networking:
174174
# Controls if the 'Secure' property is set on browser session cookies.
175175
# (default: <unset>, type: bool)
176176
secureAuthCookie: false
177+
# Controls the 'SameSite' property is set on browser session cookies.
178+
# (default: lax, type: enum[lax\|none])
179+
sameSiteAuthCookie: lax
177180
# Whether Coder only allows connections to workspaces via the browser.
178181
# (default: <unset>, type: bool)
179182
browserOnly: false

‎coderd/apidoc/docs.go

Lines changed: 14 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 14 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apikey.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -382,12 +382,10 @@ func (api *API) createAPIKey(ctx context.Context, params apikey.CreateParams) (*
382382
APIKeys: []telemetry.APIKey{telemetry.ConvertAPIKey(newkey)},
383383
})
384384

385-
return &http.Cookie{
385+
return api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
386386
Name: codersdk.SessionTokenCookie,
387387
Value: sessionToken,
388388
Path: "/",
389389
HttpOnly: true,
390-
SameSite: http.SameSiteLaxMode,
391-
Secure: api.SecureAuthCookie,
392-
}, &newkey, nil
390+
}), &newkey, nil
393391
}

‎coderd/coderd.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,6 @@ type Options struct {
155155
GithubOAuth2Config *GithubOAuth2Config
156156
OIDCConfig *OIDCConfig
157157
PrometheusRegistry *prometheus.Registry
158-
SecureAuthCookie bool
159158
StrictTransportSecurityCfg httpmw.HSTSConfig
160159
SSHKeygenAlgorithm gitsshkey.Algorithm
161160
Telemetry telemetry.Reporter
@@ -740,7 +739,7 @@ func New(options *Options) *API {
740739
StatsCollector: workspaceapps.NewStatsCollector(options.WorkspaceAppsStatsCollectorOptions),
741740

742741
DisablePathApps: options.DeploymentValues.DisablePathApps.Value(),
743-
SecureAuthCookie: options.DeploymentValues.SecureAuthCookie.Value(),
742+
Cookies: options.DeploymentValues.HTTPCookies,
744743
APIKeyEncryptionKeycache: options.AppEncryptionKeyCache,
745744
}
746745

@@ -828,7 +827,7 @@ func New(options *Options) *API {
828827
next.ServeHTTP(w, r)
829828
})
830829
},
831-
httpmw.CSRF(options.SecureAuthCookie),
830+
httpmw.CSRF(options.DeploymentValues.HTTPCookies),
832831
)
833832

834833
// This incurs a performance hit from the middleware, but is required to make sure
@@ -868,7 +867,7 @@ func New(options *Options) *API {
868867
r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
869868
r.Use(
870869
apiKeyMiddlewareRedirect,
871-
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, nil),
870+
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
872871
)
873872
r.Get("/", api.externalAuthCallback(externalAuthConfig))
874873
})
@@ -1123,14 +1122,14 @@ func New(options *Options) *API {
11231122
r.Get("/github/device", api.userOAuth2GithubDevice)
11241123
r.Route("/github", func(r chi.Router) {
11251124
r.Use(
1126-
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, nil),
1125+
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
11271126
)
11281127
r.Get("/callback", api.userOAuth2Github)
11291128
})
11301129
})
11311130
r.Route("/oidc/callback", func(r chi.Router) {
11321131
r.Use(
1133-
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, oidcAuthURLParams),
1132+
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams),
11341133
)
11351134
r.Get("/", api.userOIDC)
11361135
})

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1320,7 +1320,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
13201320
// requests will fail.
13211321
func (f *FakeIDP) HTTPClient(rest *http.Client) *http.Client {
13221322
if f.serve {
1323-
if rest == nil || rest.Transport == nil {
1323+
if rest == nil {
13241324
return &http.Client{}
13251325
}
13261326
return rest

‎coderd/coderdtest/testjar/cookiejar.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package testjar
2+
3+
import (
4+
"net/http"
5+
"net/url"
6+
"sync"
7+
)
8+
9+
func New() *Jar {
10+
return &Jar{}
11+
}
12+
13+
// Jar exists because 'cookiejar.New()' strips many of the http.Cookie fields
14+
// that are needed to assert. Such as 'Secure' and 'SameSite'.
15+
type Jar struct {
16+
m sync.Mutex
17+
perURL map[string][]*http.Cookie
18+
}
19+
20+
func (j *Jar) SetCookies(u *url.URL, cookies []*http.Cookie) {
21+
j.m.Lock()
22+
defer j.m.Unlock()
23+
if j.perURL == nil {
24+
j.perURL = make(map[string][]*http.Cookie)
25+
}
26+
j.perURL[u.Host] = append(j.perURL[u.Host], cookies...)
27+
}
28+
29+
func (j *Jar) Cookies(u *url.URL) []*http.Cookie {
30+
j.m.Lock()
31+
defer j.m.Unlock()
32+
return j.perURL[u.Host]
33+
}

‎coderd/httpmw/csrf.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@ import (
1616
// for non-GET requests.
1717
// If enforce is false, then CSRF enforcement is disabled. We still want
1818
// to include the CSRF middleware because it will set the CSRF cookie.
19-
func CSRF(secureCookie bool) func(next http.Handler) http.Handler {
19+
func CSRF(cookieCfg codersdk.HTTPCookieConfig) func(next http.Handler) http.Handler {
2020
return func(next http.Handler) http.Handler {
2121
mw := nosurf.New(next)
22-
mw.SetBaseCookie(http.Cookie{Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: secureCookie})
22+
mw.SetBaseCookie(*cookieCfg.Apply(&http.Cookie{Path: "/", HttpOnly: true}))
2323
mw.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2424
sessCookie, err := r.Cookie(codersdk.SessionTokenCookie)
2525
if err == nil &&

‎coderd/httpmw/csrf_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ func TestCSRFExemptList(t *testing.T) {
5353
},
5454
}
5555

56-
mw := httpmw.CSRF(false)
56+
mw := httpmw.CSRF(codersdk.HTTPCookieConfig{})
5757
csrfmw := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})).(*nosurf.CSRFHandler)
5858

5959
for _, c := range cases {
@@ -87,7 +87,7 @@ func TestCSRFError(t *testing.T) {
8787
var handler http.Handler = http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
8888
writer.WriteHeader(http.StatusOK)
8989
})
90-
handler = httpmw.CSRF(false)(handler)
90+
handler = httpmw.CSRF(codersdk.HTTPCookieConfig{})(handler)
9191

9292
// Not testing the error case, just providing the example of things working
9393
// to base the failure tests off of.

‎coderd/httpmw/oauth2.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func OAuth2(r *http.Request) OAuth2State {
4040
// a "code" URL parameter will be redirected.
4141
// AuthURLOpts are passed to the AuthCodeURL function. If this is nil,
4242
// the default option oauth2.AccessTypeOffline will be used.
43-
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOpts map[string]string) func(http.Handler) http.Handler {
43+
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string) func(http.Handler) http.Handler {
4444
opts := make([]oauth2.AuthCodeOption, 0, len(authURLOpts)+1)
4545
opts = append(opts, oauth2.AccessTypeOffline)
4646
for k, v := range authURLOpts {
@@ -118,22 +118,20 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
118118
}
119119
}
120120

121-
http.SetCookie(rw, &http.Cookie{
121+
http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{
122122
Name: codersdk.OAuth2StateCookie,
123123
Value: state,
124124
Path: "/",
125125
HttpOnly: true,
126-
SameSite: http.SameSiteLaxMode,
127-
})
126+
}))
128127
// Redirect must always be specified, otherwise
129128
// an old redirect could apply!
130-
http.SetCookie(rw, &http.Cookie{
129+
http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{
131130
Name: codersdk.OAuth2RedirectCookie,
132131
Value: redirect,
133132
Path: "/",
134133
HttpOnly: true,
135-
SameSite: http.SameSiteLaxMode,
136-
})
134+
}))
137135

138136
http.Redirect(rw, r, config.AuthCodeURL(state, opts...), http.StatusTemporaryRedirect)
139137
return

‎coderd/httpmw/oauth2_test.go

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,15 @@ func TestOAuth2(t *testing.T) {
5050
t.Parallel()
5151
req := httptest.NewRequest("GET", "/", nil)
5252
res := httptest.NewRecorder()
53-
httpmw.ExtractOAuth2(nil, nil, nil)(nil).ServeHTTP(res, req)
53+
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
5454
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
5555
})
5656
t.Run("RedirectWithoutCode", func(t *testing.T) {
5757
t.Parallel()
5858
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape("/dashboard"), nil)
5959
res := httptest.NewRecorder()
6060
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
61-
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
61+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
6262
location := res.Header().Get("Location")
6363
if !assert.NotEmpty(t, location) {
6464
return
@@ -82,7 +82,7 @@ func TestOAuth2(t *testing.T) {
8282
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape(uri.String()), nil)
8383
res := httptest.NewRecorder()
8484
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
85-
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
85+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
8686
location := res.Header().Get("Location")
8787
if !assert.NotEmpty(t, location) {
8888
return
@@ -97,15 +97,15 @@ func TestOAuth2(t *testing.T) {
9797
req := httptest.NewRequest("GET", "/?code=something", nil)
9898
res := httptest.NewRecorder()
9999
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
100-
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
100+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
101101
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
102102
})
103103
t.Run("NoStateCookie", func(t *testing.T) {
104104
t.Parallel()
105105
req := httptest.NewRequest("GET", "/?code=something&state=test", nil)
106106
res := httptest.NewRecorder()
107107
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
108-
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
108+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
109109
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
110110
})
111111
t.Run("MismatchedState", func(t *testing.T) {
@@ -117,7 +117,7 @@ func TestOAuth2(t *testing.T) {
117117
})
118118
res := httptest.NewRecorder()
119119
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
120-
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
120+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
121121
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
122122
})
123123
t.Run("ExchangeCodeAndState", func(t *testing.T) {
@@ -133,7 +133,7 @@ func TestOAuth2(t *testing.T) {
133133
})
134134
res := httptest.NewRecorder()
135135
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
136-
httpmw.ExtractOAuth2(tp, nil, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
136+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
137137
state := httpmw.OAuth2(r)
138138
require.Equal(t, "/dashboard", state.Redirect)
139139
})).ServeHTTP(res, req)
@@ -144,7 +144,7 @@ func TestOAuth2(t *testing.T) {
144144
res := httptest.NewRecorder()
145145
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("foo", "bar"))
146146
authOpts := map[string]string{"foo": "bar"}
147-
httpmw.ExtractOAuth2(tp, nil, authOpts)(nil).ServeHTTP(res, req)
147+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts)(nil).ServeHTTP(res, req)
148148
location := res.Header().Get("Location")
149149
// Ideally we would also assert that the location contains the query params
150150
// we set in the auth URL but this would essentially be testing the oauth2 package.
@@ -157,12 +157,17 @@ func TestOAuth2(t *testing.T) {
157157
req := httptest.NewRequest("GET", "/?oidc_merge_state="+customState+"&redirect="+url.QueryEscape("/dashboard"), nil)
158158
res := httptest.NewRecorder()
159159
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
160-
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
160+
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{
161+
Secure: true,
162+
SameSite: "none",
163+
}, nil)(nil).ServeHTTP(res, req)
161164

162165
found := false
163166
for _, cookie := range res.Result().Cookies() {
164167
if cookie.Name == codersdk.OAuth2StateCookie {
165168
require.Equal(t, cookie.Value, customState, "expected state")
169+
require.Equal(t, true, cookie.Secure, "cookie set to secure")
170+
require.Equal(t, http.SameSiteNoneMode, cookie.SameSite, "same-site = none")
166171
found = true
167172
}
168173
}

‎coderd/userauth.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
204204
Path: "/",
205205
Value: token,
206206
Expires: claims.Expiry.Time(),
207-
Secure: api.SecureAuthCookie,
207+
Secure: api.DeploymentValues.HTTPCookies.Secure.Value(),
208208
HttpOnly: true,
209209
// Must be SameSite to work on the redirected auth flow from the
210210
// oauth provider.
@@ -1913,13 +1913,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
19131913
slog.F("user_id", user.ID),
19141914
)
19151915
}
1916-
cookies = append(cookies, &http.Cookie{
1916+
cookies = append(cookies, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
19171917
Name: codersdk.SessionTokenCookie,
19181918
Path: "/",
19191919
MaxAge: -1,
1920-
Secure: api.SecureAuthCookie,
19211920
HttpOnly: true,
1922-
})
1921+
}))
19231922
// This is intentional setting the key to the deleted old key,
19241923
// as the user needs to be forced to log back in.
19251924
key = *oldKey

0 commit comments

Comments
 (0)