From 472cf6e4e41af2d07f476d20604a83652e39a516 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Sat, 8 Oct 2022 18:26:54 +0000 Subject: [PATCH 1/4] coderd: tighten /login rate limit --- coderd/coderd.go | 16 +++++++++++----- coderd/httpmw/ratelimit.go | 6 +++--- coderd/httpmw/ratelimit_test.go | 3 ++- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 35bcfdf9e63a8..e78ca6c4a8086 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -187,7 +187,7 @@ func New(options *Options) *API { // app URL. If it is, it will serve that application. api.handleSubdomainApplications( // Middleware to impose on the served application. - httpmw.RateLimitPerMinute(options.APIRateLimit), + httpmw.RateLimit(options.APIRateLimit, time.Minute), httpmw.ExtractAPIKey(httpmw.ExtractAPIKeyConfig{ DB: options.Database, OAuth2Configs: oauthConfigs, @@ -212,7 +212,7 @@ func New(options *Options) *API { apps := func(r chi.Router) { r.Use( tracing.Middleware(api.TracerProvider), - httpmw.RateLimitPerMinute(options.APIRateLimit), + httpmw.RateLimit(options.APIRateLimit, time.Minute), apiKeyMiddlewareRedirect, httpmw.ExtractUserParam(api.Database), // Extracts the from the url @@ -240,7 +240,7 @@ func New(options *Options) *API { r.Use( tracing.Middleware(api.TracerProvider), // Specific routes can specify smaller limits. - httpmw.RateLimitPerMinute(options.APIRateLimit), + httpmw.RateLimit(options.APIRateLimit, time.Minute), ) r.Get("/", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusOK, codersdk.Response{ @@ -273,7 +273,7 @@ func New(options *Options) *API { apiKeyMiddleware, // This number is arbitrary, but reading/writing // file content is expensive so it should be small. - httpmw.RateLimitPerMinute(12), + httpmw.RateLimit(12, time.Minute), ) r.Get("/{hash}", api.fileByHash) r.Post("/", api.postFile) @@ -359,7 +359,13 @@ func New(options *Options) *API { r.Route("/users", func(r chi.Router) { r.Get("/first", api.firstUser) r.Post("/first", api.postFirstUser) - r.Post("/login", api.postLogin) + r.Group(func(r chi.Router) { + // We use a tight limit for password login to protect + // against audit-log write DoS, pbkdf2 DoS, and simple + // brute-force attacks. + r.Use(httpmw.RateLimit(10, time.Minute)) + r.Post("/login", api.postLogin) + }) r.Get("/authmethods", api.userAuthMethods) r.Route("/oauth2", func(r chi.Router) { r.Route("/github", func(r chi.Router) { diff --git a/coderd/httpmw/ratelimit.go b/coderd/httpmw/ratelimit.go index 5fbe9298471e8..fb009e6c2c500 100644 --- a/coderd/httpmw/ratelimit.go +++ b/coderd/httpmw/ratelimit.go @@ -11,9 +11,9 @@ import ( "github.com/coder/coder/codersdk" ) -// RateLimitPerMinute returns a handler that limits requests per-minute based +// RateLimit returns a handler that limits requests per-minute based // on IP, endpoint, and user ID (if available). -func RateLimitPerMinute(count int) func(http.Handler) http.Handler { +func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler { // -1 is no rate limit if count <= 0 { return func(handler http.Handler) http.Handler { @@ -22,7 +22,7 @@ func RateLimitPerMinute(count int) func(http.Handler) http.Handler { } return httprate.Limit( count, - 1*time.Minute, + window, httprate.WithKeyFuncs(func(r *http.Request) (string, error) { // Prioritize by user, but fallback to IP. apiKey, ok := r.Context().Value(apiKeyContextKey{}).(database.APIKey) diff --git a/coderd/httpmw/ratelimit_test.go b/coderd/httpmw/ratelimit_test.go index 3ed0178a1699a..fdbd5335344fb 100644 --- a/coderd/httpmw/ratelimit_test.go +++ b/coderd/httpmw/ratelimit_test.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" @@ -17,7 +18,7 @@ func TestRateLimit(t *testing.T) { t.Run("NoUser", func(t *testing.T) { t.Parallel() rtr := chi.NewRouter() - rtr.Use(httpmw.RateLimitPerMinute(5)) + rtr.Use(httpmw.RateLimit(5, time.Second)) rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { rw.WriteHeader(http.StatusOK) }) From 8b3eb3a86e5479d219f394f1042d54408290cc1f Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Sat, 8 Oct 2022 19:33:58 +0000 Subject: [PATCH 2/4] coderd: add Bypass rate limit header --- coderd/httpmw/apikey_test.go | 10 ++- coderd/httpmw/ratelimit.go | 30 ++++++- coderd/httpmw/ratelimit_test.go | 139 +++++++++++++++++++++++++++++++- codersdk/client.go | 3 + 4 files changed, 176 insertions(+), 6 deletions(-) diff --git a/coderd/httpmw/apikey_test.go b/coderd/httpmw/apikey_test.go index 2ad9b8b5be922..f55f70c3906c7 100644 --- a/coderd/httpmw/apikey_test.go +++ b/coderd/httpmw/apikey_test.go @@ -591,8 +591,8 @@ func TestAPIKey(t *testing.T) { }) } -func createUser(ctx context.Context, t *testing.T, db database.Store) database.User { - user, err := db.InsertUser(ctx, database.InsertUserParams{ +func createUser(ctx context.Context, t *testing.T, db database.Store, opts ...func(u *database.InsertUserParams)) database.User { + insert := database.InsertUserParams{ ID: uuid.New(), Email: "email@coder.com", Username: "username", @@ -600,7 +600,11 @@ func createUser(ctx context.Context, t *testing.T, db database.Store) database.U CreatedAt: time.Now(), UpdatedAt: time.Now(), RBACRoles: []string{}, - }) + } + for _, opt := range opts { + opt(&insert) + } + user, err := db.InsertUser(ctx, insert) require.NoError(t, err, "create user") return user } diff --git a/coderd/httpmw/ratelimit.go b/coderd/httpmw/ratelimit.go index fb009e6c2c500..d40a97be41c74 100644 --- a/coderd/httpmw/ratelimit.go +++ b/coderd/httpmw/ratelimit.go @@ -5,10 +5,13 @@ import ( "time" "github.com/go-chi/httprate" + "golang.org/x/xerrors" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" + "github.com/coder/coder/cryptorand" ) // RateLimit returns a handler that limits requests per-minute based @@ -26,10 +29,33 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler httprate.WithKeyFuncs(func(r *http.Request) (string, error) { // Prioritize by user, but fallback to IP. apiKey, ok := r.Context().Value(apiKeyContextKey{}).(database.APIKey) - if ok { + if !ok { + return httprate.KeyByIP(r) + } + + if r.Header.Get(codersdk.BypassRatelimitHeader) == "" { return apiKey.UserID.String(), nil } - return httprate.KeyByIP(r) + + // Allow Owner to bypass rate limiting for load tests + // and automation. + auth := UserAuthorization(r) + + for _, role := range auth.Roles { + if role == rbac.RoleOwner() { + // HACK: use a random key each time to + // de facto disable rate limiting. The + // `httprate` package has no + // support for selectively changing the limit + // for particular keys. + return cryptorand.String(16) + } + } + + return apiKey.UserID.String(), xerrors.Errorf( + "%q provided but user is not %v", + codersdk.BypassRatelimitHeader, rbac.RoleOwner(), + ) }, httprate.KeyByEndpoint), httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusTooManyRequests, codersdk.Response{ diff --git a/coderd/httpmw/ratelimit_test.go b/coderd/httpmw/ratelimit_test.go index fdbd5335344fb..26d7de8ed70da 100644 --- a/coderd/httpmw/ratelimit_test.go +++ b/coderd/httpmw/ratelimit_test.go @@ -1,21 +1,55 @@ package httpmw_test import ( + "context" + "crypto/sha256" + "fmt" + "math/rand" + "net" "net/http" "net/http/httptest" "testing" "time" "github.com/go-chi/chi/v5" + "github.com/google/uuid" "github.com/stretchr/testify/require" + "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/rbac" + "github.com/coder/coder/codersdk" "github.com/coder/coder/testutil" ) +func insertAPIKey(ctx context.Context, t *testing.T, db database.Store, userID uuid.UUID) string { + id, secret := randomAPIKeyParts() + hashed := sha256.Sum256([]byte(secret)) + + _, err := db.InsertAPIKey(ctx, database.InsertAPIKeyParams{ + ID: id, + HashedSecret: hashed[:], + LastUsed: database.Now().AddDate(0, 0, -1), + ExpiresAt: database.Now().AddDate(0, 0, 1), + UserID: userID, + LoginType: database.LoginTypePassword, + Scope: database.APIKeyScopeAll, + }) + require.NoError(t, err) + + return fmt.Sprintf("%s-%s", id, secret) +} + +func randRemoteAddr() string { + var b [4]byte + rand.Read(b[:]) + return fmt.Sprintf("%s:%v", net.IP(b[:]).String(), rand.Int31()%(1<<16)) +} + func TestRateLimit(t *testing.T) { t.Parallel() - t.Run("NoUser", func(t *testing.T) { + t.Run("NoUserSucceeds", func(t *testing.T) { t.Parallel() rtr := chi.NewRouter() rtr.Use(httpmw.RateLimit(5, time.Second)) @@ -32,4 +66,107 @@ func TestRateLimit(t *testing.T) { return resp.StatusCode == http.StatusTooManyRequests }, testutil.WaitShort, testutil.IntervalFast) }) + + t.Run("RandomIPs", func(t *testing.T) { + t.Parallel() + rtr := chi.NewRouter() + rtr.Use(httpmw.RateLimit(5, time.Second)) + rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + }) + + require.Never(t, func() bool { + req := httptest.NewRequest("GET", "/", nil) + rec := httptest.NewRecorder() + req.RemoteAddr = randRemoteAddr() + rtr.ServeHTTP(rec, req) + resp := rec.Result() + defer resp.Body.Close() + return resp.StatusCode == http.StatusTooManyRequests + }, testutil.WaitShort, testutil.IntervalFast) + }) + + t.Run("RegularUser", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + db := databasefake.New() + + u := createUser(ctx, t, db) + key := insertAPIKey(ctx, t, db, u.ID) + + rtr := chi.NewRouter() + rtr.Use(httpmw.ExtractAPIKey(httpmw.ExtractAPIKeyConfig{ + DB: db, + Optional: false, + })) + + rtr.Use(httpmw.RateLimit(5, time.Second)) + rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + }) + + // Bypass must fail + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set(codersdk.SessionCustomHeader, key) + req.Header.Set(codersdk.BypassRatelimitHeader, "true") + rec := httptest.NewRecorder() + // Assert we're not using IP address. + req.RemoteAddr = randRemoteAddr() + rtr.ServeHTTP(rec, req) + resp := rec.Result() + defer resp.Body.Close() + require.Equal(t, http.StatusPreconditionRequired, resp.StatusCode) + + require.Eventually(t, func() bool { + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set(codersdk.SessionCustomHeader, key) + rec := httptest.NewRecorder() + // Assert we're not using IP address. + req.RemoteAddr = randRemoteAddr() + rtr.ServeHTTP(rec, req) + resp := rec.Result() + defer resp.Body.Close() + return resp.StatusCode == http.StatusTooManyRequests + }, testutil.WaitShort, testutil.IntervalFast) + }) + + t.Run("OwnerBypass", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + db := databasefake.New() + + u := createUser(ctx, t, db, func(u *database.InsertUserParams) { + u.RBACRoles = []string{rbac.RoleOwner()} + }) + + key := insertAPIKey(ctx, t, db, u.ID) + + rtr := chi.NewRouter() + rtr.Use(httpmw.ExtractAPIKey(httpmw.ExtractAPIKeyConfig{ + DB: db, + Optional: false, + })) + + rtr.Use(httpmw.RateLimit(5, time.Second)) + rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + }) + + require.Never(t, func() bool { + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set(codersdk.SessionCustomHeader, key) + req.Header.Set(codersdk.BypassRatelimitHeader, "true") + rec := httptest.NewRecorder() + // Assert we're not using IP address. + req.RemoteAddr = randRemoteAddr() + rtr.ServeHTTP(rec, req) + resp := rec.Result() + defer resp.Body.Close() + return resp.StatusCode == http.StatusTooManyRequests + }, testutil.WaitShort, testutil.IntervalFast) + }) } diff --git a/codersdk/client.go b/codersdk/client.go index afc37396feaa5..26d296b681b01 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -24,6 +24,9 @@ const ( SessionCustomHeader = "Coder-Session-Token" OAuth2StateKey = "oauth_state" OAuth2RedirectKey = "oauth_redirect" + + // nolint: gosec + BypassRatelimitHeader = "X-Coder-Bypass-Ratelimit" ) // New creates a Coder client for the provided URL. From 9f611a2ec2c5c5b28a982826aff2477c1eaa8532 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Sun, 9 Oct 2022 18:31:55 +0000 Subject: [PATCH 3/4] fix linter --- .golangci.yaml | 12 ++++++++---- coderd/httpmw/ratelimit_test.go | 2 ++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 171a80e8df8ff..5fe37e4c121e1 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -235,10 +235,15 @@ linters: - noctx - paralleltest - revive - - rowserrcheck - - sqlclosecheck + + # These don't work until the following issue is solved. + # https://github.com/golangci/golangci-lint/issues/2649 + # - rowserrcheck + # - sqlclosecheck + # - structcheck + # - wastedassign + - staticcheck - - structcheck - tenv # In Go, it's possible for a package to test it's internal functionality # without testing any exported functions. This is enabled to promote @@ -253,4 +258,3 @@ linters: - unconvert - unused - varcheck - - wastedassign diff --git a/coderd/httpmw/ratelimit_test.go b/coderd/httpmw/ratelimit_test.go index 26d7de8ed70da..9e2ec370e8664 100644 --- a/coderd/httpmw/ratelimit_test.go +++ b/coderd/httpmw/ratelimit_test.go @@ -43,7 +43,9 @@ func insertAPIKey(ctx context.Context, t *testing.T, db database.Store, userID u func randRemoteAddr() string { var b [4]byte + // nolint:gosec rand.Read(b[:]) + // nolint:gosec return fmt.Sprintf("%s:%v", net.IP(b[:]).String(), rand.Int31()%(1<<16)) } From 2862eb7a93e743146c40302c552ce5cdc9c507ee Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Thu, 20 Oct 2022 16:50:14 +0000 Subject: [PATCH 4/4] Comments --- coderd/coderd.go | 4 +++- coderd/httpmw/ratelimit.go | 10 ++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 3d92e729fc00c..d97c71bb90029 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -395,7 +395,9 @@ func New(options *Options) *API { // We use a tight limit for password login to protect // against audit-log write DoS, pbkdf2 DoS, and simple // brute-force attacks. - r.Use(httpmw.RateLimit(10, time.Minute)) + // + // Making this too small can break tests. + r.Use(httpmw.RateLimit(60, time.Minute)) r.Post("/login", api.postLogin) }) r.Get("/authmethods", api.userAuthMethods) diff --git a/coderd/httpmw/ratelimit.go b/coderd/httpmw/ratelimit.go index d40a97be41c74..1b5890196b11f 100644 --- a/coderd/httpmw/ratelimit.go +++ b/coderd/httpmw/ratelimit.go @@ -1,7 +1,9 @@ package httpmw import ( + "fmt" "net/http" + "strconv" "time" "github.com/go-chi/httprate" @@ -23,6 +25,7 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler return handler } } + return httprate.Limit( count, window, @@ -33,7 +36,8 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler return httprate.KeyByIP(r) } - if r.Header.Get(codersdk.BypassRatelimitHeader) == "" { + if ok, _ := strconv.ParseBool(r.Header.Get(codersdk.BypassRatelimitHeader)); !ok { + // No bypass attempt, just ratelimit. return apiKey.UserID.String(), nil } @@ -41,6 +45,8 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler // and automation. auth := UserAuthorization(r) + // We avoid using rbac.Authorizer since rego is CPU-intensive + // and undermines the DoS-prevention goal of the rate limiter. for _, role := range auth.Roles { if role == rbac.RoleOwner() { // HACK: use a random key each time to @@ -59,7 +65,7 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler }, httprate.KeyByEndpoint), httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusTooManyRequests, codersdk.Response{ - Message: "You've been rate limited for sending too many requests!", + Message: fmt.Sprintf("You've been rate limited for sending more than %v requests in %v.", count, window), }) }), )