Skip to content

chore: add custom samesite options to auth cookies #16885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
GoogleTokenValidator: googleTokenValidator,
ExternalAuthConfigs: externalAuthConfigs,
RealIPConfig: realIPConfig,
SecureAuthCookie: vals.SecureAuthCookie.Value(),
SSHKeygenAlgorithm: sshKeygenAlgorithm,
TracerProvider: tracerProvider,
Telemetry: telemetry.NewNoop(),
Expand Down
3 changes: 3 additions & 0 deletions cli/testdata/coder_server_--help.golden
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,9 @@ NETWORKING OPTIONS:
Specifies whether to redirect requests that do not match the access
URL host.

--samesite-auth-cookie lax|none, $CODER_SAMESITE_AUTH_COOKIE (default: lax)
Controls the 'SameSite' property is set on browser session cookies.

--secure-auth-cookie bool, $CODER_SECURE_AUTH_COOKIE
Controls if the 'Secure' property is set on browser session cookies.

Expand Down
3 changes: 3 additions & 0 deletions cli/testdata/server-config.yaml.golden
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ networking:
# Controls if the 'Secure' property is set on browser session cookies.
# (default: <unset>, type: bool)
secureAuthCookie: false
# Controls the 'SameSite' property is set on browser session cookies.
# (default: lax, type: enum[lax\|none])
sameSiteAuthCookie: lax
# Whether Coder only allows connections to workspaces via the browser.
# (default: <unset>, type: bool)
browserOnly: false
Expand Down
17 changes: 14 additions & 3 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 14 additions & 3 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions coderd/apikey.go
Original file line number Diff line number Diff line change
Expand Up @@ -382,12 +382,10 @@ func (api *API) createAPIKey(ctx context.Context, params apikey.CreateParams) (*
APIKeys: []telemetry.APIKey{telemetry.ConvertAPIKey(newkey)},
})

return &http.Cookie{
return api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
Name: codersdk.SessionTokenCookie,
Value: sessionToken,
Path: "/",
HttpOnly: true,
SameSite: http.SameSiteLaxMode,
Secure: api.SecureAuthCookie,
}, &newkey, nil
}), &newkey, nil
}
11 changes: 5 additions & 6 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ type Options struct {
GithubOAuth2Config *GithubOAuth2Config
OIDCConfig *OIDCConfig
PrometheusRegistry *prometheus.Registry
SecureAuthCookie bool
StrictTransportSecurityCfg httpmw.HSTSConfig
SSHKeygenAlgorithm gitsshkey.Algorithm
Telemetry telemetry.Reporter
Expand Down Expand Up @@ -740,7 +739,7 @@ func New(options *Options) *API {
StatsCollector: workspaceapps.NewStatsCollector(options.WorkspaceAppsStatsCollectorOptions),

DisablePathApps: options.DeploymentValues.DisablePathApps.Value(),
SecureAuthCookie: options.DeploymentValues.SecureAuthCookie.Value(),
Cookies: options.DeploymentValues.HTTPCookies,
APIKeyEncryptionKeycache: options.AppEncryptionKeyCache,
}

Expand Down Expand Up @@ -828,7 +827,7 @@ func New(options *Options) *API {
next.ServeHTTP(w, r)
})
},
httpmw.CSRF(options.SecureAuthCookie),
httpmw.CSRF(options.DeploymentValues.HTTPCookies),
)

// This incurs a performance hit from the middleware, but is required to make sure
Expand Down Expand Up @@ -868,7 +867,7 @@ func New(options *Options) *API {
r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
r.Use(
apiKeyMiddlewareRedirect,
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, nil),
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
)
r.Get("/", api.externalAuthCallback(externalAuthConfig))
})
Expand Down Expand Up @@ -1123,14 +1122,14 @@ func New(options *Options) *API {
r.Get("/github/device", api.userOAuth2GithubDevice)
r.Route("/github", func(r chi.Router) {
r.Use(
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, nil),
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
)
r.Get("/callback", api.userOAuth2Github)
})
})
r.Route("/oidc/callback", func(r chi.Router) {
r.Use(
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, oidcAuthURLParams),
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams),
)
r.Get("/", api.userOIDC)
})
Expand Down
2 changes: 1 addition & 1 deletion coderd/coderdtest/oidctest/idp.go
Original file line number Diff line number Diff line change
Expand Up @@ -1320,7 +1320,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
// requests will fail.
func (f *FakeIDP) HTTPClient(rest *http.Client) *http.Client {
if f.serve {
if rest == nil || rest.Transport == nil {
if rest == nil {
return &http.Client{}
}
return rest
Expand Down
33 changes: 33 additions & 0 deletions coderd/coderdtest/testjar/cookiejar.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package testjar

import (
"net/http"
"net/url"
"sync"
)

func New() *Jar {
return &Jar{}
}

// Jar exists because 'cookiejar.New()' strips many of the http.Cookie fields
// that are needed to assert. Such as 'Secure' and 'SameSite'.
type Jar struct {
m sync.Mutex
perURL map[string][]*http.Cookie
}

func (j *Jar) SetCookies(u *url.URL, cookies []*http.Cookie) {
j.m.Lock()
defer j.m.Unlock()
if j.perURL == nil {
j.perURL = make(map[string][]*http.Cookie)
}
j.perURL[u.Host] = append(j.perURL[u.Host], cookies...)
}

func (j *Jar) Cookies(u *url.URL) []*http.Cookie {
j.m.Lock()
defer j.m.Unlock()
return j.perURL[u.Host]
}
4 changes: 2 additions & 2 deletions coderd/httpmw/csrf.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ import (
// for non-GET requests.
// If enforce is false, then CSRF enforcement is disabled. We still want
// to include the CSRF middleware because it will set the CSRF cookie.
func CSRF(secureCookie bool) func(next http.Handler) http.Handler {
func CSRF(cookieCfg codersdk.HTTPCookieConfig) func(next http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
mw := nosurf.New(next)
mw.SetBaseCookie(http.Cookie{Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: secureCookie})
mw.SetBaseCookie(*cookieCfg.Apply(&http.Cookie{Path: "/", HttpOnly: true}))
mw.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
sessCookie, err := r.Cookie(codersdk.SessionTokenCookie)
if err == nil &&
Expand Down
4 changes: 2 additions & 2 deletions coderd/httpmw/csrf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func TestCSRFExemptList(t *testing.T) {
},
}

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

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

// Not testing the error case, just providing the example of things working
// to base the failure tests off of.
Expand Down
12 changes: 5 additions & 7 deletions coderd/httpmw/oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func OAuth2(r *http.Request) OAuth2State {
// a "code" URL parameter will be redirected.
// AuthURLOpts are passed to the AuthCodeURL function. If this is nil,
// the default option oauth2.AccessTypeOffline will be used.
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOpts map[string]string) func(http.Handler) http.Handler {
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string) func(http.Handler) http.Handler {
opts := make([]oauth2.AuthCodeOption, 0, len(authURLOpts)+1)
opts = append(opts, oauth2.AccessTypeOffline)
for k, v := range authURLOpts {
Expand Down Expand Up @@ -118,22 +118,20 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
}
}

http.SetCookie(rw, &http.Cookie{
http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{
Name: codersdk.OAuth2StateCookie,
Value: state,
Path: "/",
HttpOnly: true,
SameSite: http.SameSiteLaxMode,
})
}))
// Redirect must always be specified, otherwise
// an old redirect could apply!
http.SetCookie(rw, &http.Cookie{
http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{
Name: codersdk.OAuth2RedirectCookie,
Value: redirect,
Path: "/",
HttpOnly: true,
SameSite: http.SameSiteLaxMode,
})
}))

http.Redirect(rw, r, config.AuthCodeURL(state, opts...), http.StatusTemporaryRedirect)
return
Expand Down
23 changes: 14 additions & 9 deletions coderd/httpmw/oauth2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ func TestOAuth2(t *testing.T) {
t.Parallel()
req := httptest.NewRequest("GET", "/", nil)
res := httptest.NewRecorder()
httpmw.ExtractOAuth2(nil, nil, nil)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
})
t.Run("RedirectWithoutCode", func(t *testing.T) {
t.Parallel()
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape("/dashboard"), nil)
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
location := res.Header().Get("Location")
if !assert.NotEmpty(t, location) {
return
Expand All @@ -82,7 +82,7 @@ func TestOAuth2(t *testing.T) {
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape(uri.String()), nil)
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
location := res.Header().Get("Location")
if !assert.NotEmpty(t, location) {
return
Expand All @@ -97,15 +97,15 @@ func TestOAuth2(t *testing.T) {
req := httptest.NewRequest("GET", "/?code=something", nil)
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
})
t.Run("NoStateCookie", func(t *testing.T) {
t.Parallel()
req := httptest.NewRequest("GET", "/?code=something&state=test", nil)
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
})
t.Run("MismatchedState", func(t *testing.T) {
Expand All @@ -117,7 +117,7 @@ func TestOAuth2(t *testing.T) {
})
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(nil).ServeHTTP(res, req)
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
})
t.Run("ExchangeCodeAndState", func(t *testing.T) {
Expand All @@ -133,7 +133,7 @@ func TestOAuth2(t *testing.T) {
})
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
httpmw.ExtractOAuth2(tp, nil, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
state := httpmw.OAuth2(r)
require.Equal(t, "/dashboard", state.Redirect)
})).ServeHTTP(res, req)
Expand All @@ -144,7 +144,7 @@ func TestOAuth2(t *testing.T) {
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("foo", "bar"))
authOpts := map[string]string{"foo": "bar"}
httpmw.ExtractOAuth2(tp, nil, authOpts)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts)(nil).ServeHTTP(res, req)
location := res.Header().Get("Location")
// Ideally we would also assert that the location contains the query params
// we set in the auth URL but this would essentially be testing the oauth2 package.
Expand All @@ -157,12 +157,17 @@ func TestOAuth2(t *testing.T) {
req := httptest.NewRequest("GET", "/?oidc_merge_state="+customState+"&redirect="+url.QueryEscape("/dashboard"), nil)
res := httptest.NewRecorder()
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{
Secure: true,
SameSite: "none",
}, nil)(nil).ServeHTTP(res, req)

found := false
for _, cookie := range res.Result().Cookies() {
if cookie.Name == codersdk.OAuth2StateCookie {
require.Equal(t, cookie.Value, customState, "expected state")
require.Equal(t, true, cookie.Secure, "cookie set to secure")
require.Equal(t, http.SameSiteNoneMode, cookie.SameSite, "same-site = none")
found = true
}
}
Expand Down
7 changes: 3 additions & 4 deletions coderd/userauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
Path: "/",
Value: token,
Expires: claims.Expiry.Time(),
Secure: api.SecureAuthCookie,
Secure: api.DeploymentValues.HTTPCookies.Secure.Value(),
HttpOnly: true,
// Must be SameSite to work on the redirected auth flow from the
// oauth provider.
Expand Down Expand Up @@ -1913,13 +1913,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
slog.F("user_id", user.ID),
)
}
cookies = append(cookies, &http.Cookie{
cookies = append(cookies, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
Name: codersdk.SessionTokenCookie,
Path: "/",
MaxAge: -1,
Secure: api.SecureAuthCookie,
HttpOnly: true,
})
}))
// This is intentional setting the key to the deleted old key,
// as the user needs to be forced to log back in.
key = *oldKey
Expand Down
Loading
Loading