Skip to content

fix: relax csrf to exclude path based apps #11430

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 12 commits into from
Jan 8, 2024
Merged

fix: relax csrf to exclude path based apps #11430

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
658d7df
fix: relax csrf to exclude path based apps
Emyrk Jan 4, 2024
fe4e2ba
add unit test to verify path based apps are not CSRF blocked
Emyrk Jan 5, 2024
f2e615d
testing needs to be parallel
Emyrk Jan 5, 2024
a4bbc80
Linting
Emyrk Jan 5, 2024
63133cf
Merge remote-tracking branch 'origin/main' into stevenmasley/relax_csrf
Emyrk Jan 5, 2024
3828354
Merge remote-tracking branch 'origin/main' into stevenmasley/relax_csrf
Emyrk Jan 5, 2024
c6c4141
Try exempting first user
Emyrk Jan 5, 2024
42a3021
Merge remote-tracking branch 'origin/main' into stevenmasley/relax_csrf
Emyrk Jan 8, 2024
043c79d
we need to add csrf mw to set cookie
Emyrk Jan 8, 2024
388e56e
add comment
Emyrk Jan 8, 2024
fee870b
add unit tests
Emyrk Jan 8, 2024
6966f87
Linting
Emyrk Jan 8, 2024
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
5 changes: 4 additions & 1 deletion coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,6 @@ func New(options *Options) *API {
next.ServeHTTP(w, r)
})
},
httpmw.CSRF(options.SecureAuthCookie),
)

r.Get("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("OK")) })
Expand Down Expand Up @@ -633,6 +632,10 @@ func New(options *Options) *API {
// limit must be configurable by the admin.
apiRateLimiter,
httpmw.ReportCLITelemetry(api.Logger, options.Telemetry),
// CSRF only needs to apply to /api routes. It does not apply to GET requests
// anyway, which is most other routes. We want to exempt any external auth or
// application type routes.
httpmw.CSRF(options.SecureAuthCookie),
Copy link
Member

Choose a reason for hiding this comment

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

There are a bunch of ExemptRegexp() calls inside this middleware that may need to be reviewed after this change. It might be nicer to have those passed explicitly as arguments to httpmw.CSRF instead of being hidden inside the middleware.

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 thing is, this will pretty quickly fail on someone when they add a new route if it breaks. The error is a CSRF error, which brings you to here. I prefer to have this all in 1 place, and this should never be configurable by a customer except maybe to disable it entirely.

I think the current exempts are overkill. I intentionally went a bit overkill because I was afraid of breaking anything. Some

The reason there are exempts like this is when I turned this back on, I was trying to error on the side of caution. I think we should have a PR to remove an exempt 1 by 1 when we determine they can never cause an issue.

An example is the /provisionerdaemons ones. They are currently only GET requests, and I do not think the actual daemons are using cookies to authenticate. However I error-ed on the side of caution since these routes should never use CSRF anyway.

I can make follow up PRs to drop these with rational for each one

)
r.Get("/", apiRoot)
// All CSP errors will be logged
Expand Down
64 changes: 64 additions & 0 deletions coderd/coderd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package coderd_test
import (
"context"
"flag"
"fmt"
"io"
"net/http"
"net/netip"
Expand All @@ -21,6 +22,9 @@ import (

"cdr.dev/slog"
"cdr.dev/slog/sloggers/slogtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbfake"
"github.com/coder/coder/v2/provisionersdk/proto"

"github.com/coder/coder/v2/agent/agenttest"
"github.com/coder/coder/v2/buildinfo"
Expand Down Expand Up @@ -315,3 +319,63 @@ func TestSwagger(t *testing.T) {
require.Equal(t, "<pre>\n</pre>\n", string(body))
})
}

func TestCSRFExempt(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I am working on a unit test to verify

Is it this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. All of our existing unit tests bypass CSRF since the codersdk uses the HEADER method to auth, not the cookie.

t.Parallel()

// This test build a workspace with an agent and an app. The app is not
// a real http server, so it will fail to serve requests. We just want
// to make sure the failure is not a CSRF failure, as path based
// apps should be exempt.
t.Run("PathBasedApp", func(t *testing.T) {
t.Parallel()

client, _, api := coderdtest.NewWithAPI(t, nil)
first := coderdtest.CreateFirstUser(t, client)
owner, err := client.User(context.Background(), "me")
require.NoError(t, err)

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium)
defer cancel()

// Create a workspace.
const agentSlug = "james"
const appSlug = "web"
wrk := dbfake.WorkspaceBuild(t, api.Database, database.Workspace{
OwnerID: owner.ID,
OrganizationID: first.OrganizationID,
}).
WithAgent(func(agents []*proto.Agent) []*proto.Agent {
agents[0].Name = agentSlug
agents[0].Apps = []*proto.App{{
Slug: appSlug,
DisplayName: appSlug,
Subdomain: false,
Url: "/",
}}

return agents
}).
Do()

u := client.URL.JoinPath(fmt.Sprintf("/@%s/%s.%s/apps/%s", owner.Username, wrk.Workspace.Name, agentSlug, appSlug)).String()
req, err := http.NewRequestWithContext(ctx, http.MethodPost, u, nil)
req.AddCookie(&http.Cookie{
Name: codersdk.SessionTokenCookie,
Value: client.SessionToken(),
Path: "/",
Domain: client.URL.String(),
})
require.NoError(t, err)

resp, err := client.HTTPClient.Do(req)
require.NoError(t, err)
data, _ := io.ReadAll(resp.Body)
_ = resp.Body.Close()

// A StatusBadGateway means Coderd tried to proxy to the agent and failed because the agent
// was not there. This means CSRF did not block the app request, which is what we want.
require.Equal(t, http.StatusBadGateway, resp.StatusCode, "status code 500 is CSRF failure")
require.NotContains(t, string(data), "CSRF")
})
}
3 changes: 3 additions & 0 deletions coderd/httpmw/csrf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler {
// All GET requests are exempt by default.
mw.ExemptPath("/api/v2/csp/reports")

// This should not be required?
mw.ExemptRegexp(regexp.MustCompile("/api/v2/users/first"))

// Agent authenticated routes
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/me/*"))
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/*"))
Expand Down
7 changes: 4 additions & 3 deletions enterprise/wsproxy/wsproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,10 @@ func New(ctx context.Context, opts *Options) (*Server, error) {
next.ServeHTTP(w, r)
})
},
// TODO: @emyrk we might not need this? But good to have if it does
// not break anything.
httpmw.CSRF(s.Options.SecureAuthCookie),
// CSRF middleware is intentionally excluded here. All coder requests
// which require CSRF protection are forwarded to the primary Coderd
// via a proxy function. Since the primary enforces this, the proxy does
// not.
)

// Attach workspace apps routes.
Expand Down