Skip to content

feat: Implement (but not enforce) CSRF for FE requests #3786

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.

Sign up for GitHub

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 29 commits into from
Sep 13, 2022
Merged

feat: Implement (but not enforce) CSRF for FE requests #3786

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
52c7575
feat: Implement CSRF in cli client and FE api
Emyrk Aug 31, 2022
642a29f
Make fmt
Emyrk Aug 31, 2022
37c0ba8
const vs let
Emyrk Aug 31, 2022
c774fcc
Fix lint error
presleyp Aug 31, 2022
3c75967
remove bad console log
Emyrk Sep 1, 2022
ab48b4a
Add CSRF token in header
Emyrk Sep 1, 2022
51d856e
Log error if token content is null
presleyp Sep 1, 2022
c8c8be0
Merge branch 'stevenmasley/csrf' of github.com:coder/coder into steve…
presleyp Sep 1, 2022
b03610b
Fix dev server csrf with hardcoded value
Emyrk Sep 1, 2022
e798e11
Do not error log in JS unit test
Emyrk Sep 1, 2022
1c4810a
Make fmt on js files
Emyrk Sep 1, 2022
a6fdac8
Fix agent token checking
Emyrk Sep 1, 2022
a343da9
Fix unit test
Emyrk Sep 1, 2022
dd80cc9
Check auth cookie exists
Emyrk Sep 1, 2022
08e76d4
Fix test auth
Emyrk Sep 1, 2022
0aae08a
Fix logout test
Emyrk Sep 1, 2022
3116964
Merge remote-tracking branch 'origin/main' into stevenmasley/csrf
Emyrk Sep 13, 2022
10b4296
Fix merge issues
Emyrk Sep 13, 2022
7177909
fixup! Fix merge issues
Emyrk Sep 13, 2022
633118e
Make unit test use correct session value
Emyrk Sep 13, 2022
5662a55
puppeteer does not have document defined
Emyrk Sep 13, 2022
86b9ecf
Make fmt
Emyrk Sep 13, 2022
ecaf61f
Update wireguard dep
Emyrk Sep 13, 2022
484fe2b
Add comment about BE cookie
Emyrk Sep 13, 2022
b18ea2e
chore: Ensure multiple version compatibility
Emyrk Sep 13, 2022
b97225f
Merge remote-tracking branch 'origin/main' into stevenmasley/csrf
Emyrk Sep 13, 2022
3f1eedf
Do not enforce CSRF
Emyrk Sep 13, 2022
8f367d2
Add nolint
Emyrk Sep 13, 2022
85dcbfd
Account for devurl cookie
Emyrk Sep 13, 2022
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: 1 addition & 0 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ func New(options *Options) *API {
httpmw.Recover(api.Logger),
httpmw.Logger(api.Logger),
httpmw.Prometheus(options.PrometheusRegistry),
httpmw.CSRF(options.SecureAuthCookie),
)

apps := func(r chi.Router) {
Expand Down
19 changes: 18 additions & 1 deletion coderd/httpmw/apikey.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,29 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs, redirectToLogin bool
}

var cookieValue string
// Find the session token from:
// 1: The cookie
// 2: The old cookie
// 3. The coder_session_token query parameter
// 4. The custom auth header
cookie, err := r.Cookie(codersdk.SessionTokenKey)
if err != nil {
cookieValue = r.URL.Query().Get(codersdk.SessionTokenKey)
// TODO: @emyrk in October 2022, remove this oldCookie check.
// This is just to support the old cli for 1 release. Then everyone
// must update.
oldCookie, err := r.Cookie("session_token")
if err != nil {
cookieValue = r.URL.Query().Get(codersdk.SessionTokenKey)
if cookieValue == "" {
cookieValue = r.Header.Get(codersdk.SessionCustomHeader)
}
} else {
cookieValue = oldCookie.Value
}
} else {
cookieValue = cookie.Value
}

if cookieValue == "" {
write(http.StatusUnauthorized, codersdk.Response{
Message: signedOutErrorMessage,
Expand Down
60 changes: 12 additions & 48 deletions coderd/httpmw/apikey_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,7 @@ func TestAPIKey(t *testing.T) {
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: "test-wow-hello",
})
r.Header.Set(codersdk.SessionCustomHeader, "test-wow-hello")

httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
res := rw.Result()
Expand All @@ -92,10 +89,7 @@ func TestAPIKey(t *testing.T) {
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: "test-wow",
})
r.Header.Set(codersdk.SessionCustomHeader, "test-wow")

httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
res := rw.Result()
Expand All @@ -110,10 +104,7 @@ func TestAPIKey(t *testing.T) {
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: "testtestid-wow",
})
r.Header.Set(codersdk.SessionCustomHeader, "testtestid-wow")

httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
res := rw.Result()
Expand All @@ -129,10 +120,7 @@ func TestAPIKey(t *testing.T) {
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
res := rw.Result()
Expand All @@ -149,10 +137,7 @@ func TestAPIKey(t *testing.T) {
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

// Use a different secret so they don't match!
hashed := sha256.Sum256([]byte("differentsecret"))
Expand All @@ -178,10 +163,7 @@ func TestAPIKey(t *testing.T) {
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

_, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
Expand All @@ -206,10 +188,7 @@ func TestAPIKey(t *testing.T) {
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
Expand Down Expand Up @@ -280,10 +259,7 @@ func TestAPIKey(t *testing.T) {
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
Expand Down Expand Up @@ -316,10 +292,7 @@ func TestAPIKey(t *testing.T) {
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
Expand Down Expand Up @@ -352,10 +325,7 @@ func TestAPIKey(t *testing.T) {
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
Expand Down Expand Up @@ -395,10 +365,7 @@ func TestAPIKey(t *testing.T) {
rw = httptest.NewRecorder()
user = createUser(r.Context(), t, db)
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
Expand Down Expand Up @@ -449,10 +416,7 @@ func TestAPIKey(t *testing.T) {
user = createUser(r.Context(), t, db)
)
r.RemoteAddr = "1.1.1.1:3555"
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

_, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: id,
Expand Down
5 changes: 1 addition & 4 deletions coderd/httpmw/authorize_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,7 @@ func TestExtractUserRoles(t *testing.T) {
})

req := httptest.NewRequest("GET", "/", nil)
req.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: token,
})
req.Header.Set(codersdk.SessionCustomHeader, token)

rtr.ServeHTTP(rw, req)
resp := rw.Result()
Expand Down
63 changes: 63 additions & 0 deletions coderd/httpmw/csrf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package httpmw

import (
"net/http"
"regexp"

"github.com/justinas/nosurf"
"golang.org/x/xerrors"

"github.com/coder/coder/codersdk"
)

// CSRF is a middleware that verifies that a CSRF token is present in the request
// for non-GET requests.
func CSRF(secureCookie bool) 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.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
http.Error(w, "Something is wrong with your CSRF token. Please refresh the page. If this error persists, try clearing your cookies.", http.StatusBadRequest)
}))

// Exempt all requests that do not require CSRF protection.
// All GET requests are exempt by default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not an issue? Can't we still send data with a GET?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSRF really only needs to protect endpoints that have side effects. Our router checks the method on all API calls. See https://security.stackexchange.com/questions/115794/should-i-use-csrf-protection-for-get-requests.

If we design our api correctly, there is no need to enforce CSRF on GET requests.

mw.ExemptPath("/api/v2/csp/reports")

// Top level agent routes.
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/[^/]*$"))
// Agent authenticated routes
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/me/*"))
Comment on lines +23 to +30
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this is all we need to exempt. If provisionerd needs to make api calls, we should omit that as well.


mw.ExemptFunc(func(r *http.Request) bool {
// CSRF only affects requests that automatically attach credentials via a cookie.
// If no cookie is present, then there is no risk of CSRF.
sessCookie, err := r.Cookie(codersdk.SessionTokenKey)
if xerrors.Is(err, http.ErrNoCookie) {
return true
}

if token := r.Header.Get(codersdk.SessionCustomHeader); token == sessCookie.Value {
// If the cookie and header match, we can assume this is the same as just using the
// custom header auth. Custom header auth can bypass CSRF, as CSRF attacks
// cannot add custom headers.
return true
}

if token := r.URL.Query().Get(codersdk.SessionTokenKey); token == sessCookie.Value {
// If the auth is set in a url param and matches the cookie, it
// is the same as just using the url param.
return true
}

// If the X-CSRF-TOKEN header is set, we can exempt the func if it's valid.
// This is the CSRF check.
sent := r.Header.Get("X-CSRF-TOKEN")
if sent != "" {
return nosurf.VerifyToken(nosurf.Token(r), sent)
}
return false
})
return mw
}
}
5 changes: 1 addition & 4 deletions coderd/httpmw/organizationparam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ func TestOrganizationParam(t *testing.T) {
r = httptest.NewRequest("GET", "/", nil)
hashed = sha256.Sum256([]byte(secret))
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

userID := uuid.New()
username, err := cryptorand.String(8)
Expand Down
5 changes: 1 addition & 4 deletions coderd/httpmw/templateparam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ func TestTemplateParam(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
)
r := httptest.NewRequest("GET", "/", nil)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

userID := uuid.New()
username, err := cryptorand.String(8)
Expand Down
5 changes: 1 addition & 4 deletions coderd/httpmw/templateversionparam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ func TestTemplateVersionParam(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
)
r := httptest.NewRequest("GET", "/", nil)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

userID := uuid.New()
username, err := cryptorand.String(8)
Expand Down
5 changes: 1 addition & 4 deletions coderd/httpmw/userparam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ func TestUserParam(t *testing.T) {
r = httptest.NewRequest("GET", "/", nil)
rw = httptest.NewRecorder()
)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

user, err := db.InsertUser(r.Context(), database.InsertUserParams{
ID: uuid.New(),
Expand Down
5 changes: 1 addition & 4 deletions coderd/httpmw/workspaceagent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,7 @@ func TestWorkspaceAgent(t *testing.T) {
setup := func(db database.Store) (*http.Request, uuid.UUID) {
token := uuid.New()
r := httptest.NewRequest("GET", "/", nil)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: token.String(),
})
r.Header.Set(codersdk.SessionCustomHeader, token.String())
return r, token
}

Expand Down
5 changes: 1 addition & 4 deletions coderd/httpmw/workspaceagentparam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ func TestWorkspaceAgentParam(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
)
r := httptest.NewRequest("GET", "/", nil)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

userID := uuid.New()
username, err := cryptorand.String(8)
Expand Down
5 changes: 1 addition & 4 deletions coderd/httpmw/workspacebuildparam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ func TestWorkspaceBuildParam(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
)
r := httptest.NewRequest("GET", "/", nil)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

userID := uuid.New()
username, err := cryptorand.String(8)
Expand Down
10 changes: 2 additions & 8 deletions coderd/httpmw/workspaceparam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,7 @@ func TestWorkspaceParam(t *testing.T) {
hashed = sha256.Sum256([]byte(secret))
)
r := httptest.NewRequest("GET", "/", nil)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

userID := uuid.New()
username, err := cryptorand.String(8)
Expand Down Expand Up @@ -340,10 +337,7 @@ func setupWorkspaceWithAgents(t testing.TB, cfg setupConfig) (database.Store, *h
hashed = sha256.Sum256([]byte(secret))
)
r := httptest.NewRequest("GET", "/", nil)
r.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenKey,
Value: fmt.Sprintf("%s-%s", id, secret),
})
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))

userID := uuid.New()
username, err := cryptorand.String(8)
Expand Down
13 changes: 6 additions & 7 deletions codersdk/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ import (
// Be sure to strip additional cookies in httpapi.StripCoder Cookies!
const (
// SessionTokenKey represents the name of the cookie or query parameter the API key is stored in.
SessionTokenKey = "session_token"
OAuth2StateKey = "oauth_state"
OAuth2RedirectKey = "oauth_redirect"
SessionTokenKey = "coder_session_token"
// SessionCustomHeader is the custom header to use for authentication.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a custom header?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cli needs to bypass CSRF (it'd be dumb to use on the cli), so we use a custom header rather than a cookie. The old code required using a -H 'cooke:...' which is kinda odd.

In future we might want to use an Authorization Bearer token to be more standard?

SessionCustomHeader = "Coder-Session-Token"
OAuth2StateKey = "oauth_state"
OAuth2RedirectKey = "oauth_redirect"
)

// New creates a Coder client for the provided URL.
Expand Down Expand Up @@ -70,10 +72,7 @@ func (c *Client) Request(ctx context.Context, method, path string, body interfac
if err != nil {
return nil, xerrors.Errorf("create request: %w", err)
}
req.AddCookie(&http.Cookie{
Name: SessionTokenKey,
Value: c.SessionToken,
})
req.Header.Set(SessionCustomHeader, c.SessionToken)
if body != nil {
req.Header.Set("Content-Type", "application/json")
}
Expand Down
Loading