feat: add more rate limit control flags

coder · deansheather · Jan 5, 2023 · Jan 4, 2023 · Jan 4, 2023 · Jan 5, 2023
commit 51c825bf80f9277e2dce24d291da6f231cd30602
diff --git a/cli/deployment/config.go b/cli/deployment/config.go
@@ -429,11 +429,25 @@ func newConfig() *codersdk.DeploymentConfig {
 				Default: 10 * time.Minute,
 			},
 		},
-		APIRateLimit: &codersdk.DeploymentConfigField[int]{
-			Name:    "API Rate Limit",
-			Usage:   "Maximum number of requests per minute allowed to the API per user, or per IP address for unauthenticated users. Negative values mean no rate limit. Some API endpoints are always rate limited regardless of this value to prevent denial-of-service attacks.",
-			Flag:    "api-rate-limit",
-			Default: 512,
+		RateLimit: &codersdk.RateLimitConfig{
+			API: &codersdk.DeploymentConfigField[int]{
+				Name:    "API Rate Limit",
+				Usage:   "Maximum number of requests per minute allowed to the API per user, or per IP address for unauthenticated users. Negative values mean no rate limit. Some API endpoints have separate flags to control rate limits to help prevent denial-of-service attacks.",
+				Flag:    "api-rate-limit",
+				Default: 512,
+			},
+			Login: &codersdk.DeploymentConfigField[int]{
+				Name:    "Login Rate Limit",
+				Usage:   "Maximum number of requests per minute allowed to the login API per IP address. This rate limit is intentionally lower than the API rate limit to help prevent brute force attacks. Setting this to a value higher than the API rate limit or to a negative value will have no effect.",
+				Flag:    "login-api-rate-limit",
+				Default: 60,
+			},
+			Files: &codersdk.DeploymentConfigField[int]{
+				Name:    "Files API Rate Limit",
+				Usage:   "Maximum number of requests per minute allowed to the files API per user. This rate limit is intentionally lower than the API rate limit as the files API is database intensive. Setting this to a value higher than the API rate limit or to a negative value will have no effect.",
+				Flag:    "files-api-rate-limit",
+				Default: 12,
+			},
 		},
 		Experimental: &codersdk.DeploymentConfigField[bool]{
 			Name:  "Experimental",

diff --git a/cli/scaletest.go b/cli/scaletest.go
@@ -265,7 +265,7 @@ func requireAdmin(ctx context.Context, client *codersdk.Client) (codersdk.User,
 	// Only owners can do scaletests. This isn't a very strong check but there's
 	// not much else we can do. Ratelimits are enforced for non-owners so
 	// hopefully that limits the damage if someone disables this check and runs
-	// it against a non-owner account.
+	// it against a non-owner account on a production deployment.
 	ok := false
 	for _, role := range me.Roles {
 		if role.Name == "owner" {
@@ -488,7 +488,9 @@ func scaletestCreateWorkspaces() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "create-workspaces",
 		Short: "Creates many workspaces and waits for them to be ready",
-		Long:  "Creates many users, then creates a workspace for each user and waits for them finish building and fully come online. Optionally runs a command inside each workspace, and connects to the workspace over WireGuard.",
+		Long: `Creates many users, then creates a workspace for each user and waits for them finish building and fully come online. Optionally runs a command inside each workspace, and connects to the workspace over WireGuard.
+
+It is recommended that all rate limits are disabled on the server before running this scaletest. This test generates many login events which will be rate limited against the (most likely single) IP.`,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			ctx := cmd.Context()
 			client, err := CreateClient(cmd)

diff --git a/cli/server.go b/cli/server.go
@@ -454,7 +454,9 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
 				AgentStatsRefreshInterval:   cfg.AgentStatRefreshInterval.Value,
 				DeploymentConfig:            cfg,
 				PrometheusRegistry:          prometheus.NewRegistry(),
-				APIRateLimit:                cfg.APIRateLimit.Value,
+				APIRateLimit:                cfg.RateLimit.API.Value,
+				LoginRateLimit:              cfg.RateLimit.Login.Value,
+				FilesRateLimit:              cfg.RateLimit.Files.Value,
 				HTTPClient:                  httpClient,
 			}
 			if tlsConfig != nil {

diff --git a/cli/testdata/coder_scaletest_create-workspaces_--help.golden b/cli/testdata/coder_scaletest_create-workspaces_--help.golden
@@ -1,5 +1,7 @@
 Creates many users, then creates a workspace for each user and waits for them finish building and fully come online. Optionally runs a command inside each workspace, and connects to the workspace over WireGuard.
 
+It is recommended that all rate limits are disabled on the server before running this scaletest. This test generates many login events which will be rate limited against the (most likely single) IP.
+
 Usage:
   coder scaletest create-workspaces [flags]
 

diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden
@@ -18,10 +18,10 @@ Flags:
                                                      allowed to the API per user, or per IP
                                                      address for unauthenticated users.
                                                      Negative values mean no rate limit. Some
-                                                     API endpoints are always rate limited
-                                                     regardless of this value to prevent
+                                                     API endpoints have separate flags to
+                                                     control rate limits to help prevent
                                                      denial-of-service attacks.
-                                                     Consumes $CODER_API_RATE_LIMIT (default 512)
+                                                     Consumes $CODER_RATE_LIMIT_API (default 512)
       --cache-dir string                             The directory to cache temporary files.
                                                      If unspecified and $CACHE_DIRECTORY is
                                                      set, it will be used for compatibility
@@ -61,11 +61,27 @@ Flags:
                                                      Experimental features are not ready for
                                                      production.
                                                      Consumes $CODER_EXPERIMENTAL
+      --files-api-rate-limit int                     Maximum number of requests per minute
+                                                     allowed to the files API per user. This
+                                                     rate limit is intentionally lower than
+                                                     the API rate limit as the files API is
+                                                     database intensive. Setting this to a
+                                                     value higher than the API rate limit or
+                                                     to a negative value will have no effect.
+                                                     Consumes $CODER_RATE_LIMIT_FILES (default 12)
   -h, --help                                         help for server
       --http-address string                          HTTP bind address of the server. Unset to
                                                      disable the HTTP endpoint.
                                                      Consumes $CODER_HTTP_ADDRESS (default
                                                      "127.0.0.1:3000")
+      --login-api-rate-limit int                     Maximum number of requests per minute
+                                                     allowed to the login API per IP address.
+                                                     This rate limit is intentionally lower
+                                                     than the API rate limit to help prevent
+                                                     brute force attacks. Setting this to a
+                                                     value higher than the API rate limit or
+                                                     to a negative value will have no effect.
+                                                     Consumes $CODER_RATE_LIMIT_LOGIN (default 60)
       --max-token-lifetime duration                  The maximum lifetime duration for any
                                                      user creating a token.
                                                      Consumes $CODER_MAX_TOKEN_LIFETIME

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -82,32 +82,35 @@ type Options struct {
 	Auditor                        audit.Auditor
 	AgentConnectionUpdateFrequency time.Duration
 	AgentInactiveDisconnectTimeout time.Duration
-	// APIRateLimit is the minutely throughput rate limit per user or ip.
-	// Setting a rate limit <0 will disable the rate limiter across the entire
-	// app. Specific routes may have their own limiters.
-	APIRateLimit         int
-	AWSCertificates      awsidentity.Certificates
-	Authorizer           rbac.Authorizer
-	AzureCertificates    x509.VerifyOptions
-	GoogleTokenValidator *idtoken.Validator
-	GithubOAuth2Config   *GithubOAuth2Config
-	OIDCConfig           *OIDCConfig
-	PrometheusRegistry   *prometheus.Registry
-	SecureAuthCookie     bool
-	SSHKeygenAlgorithm   gitsshkey.Algorithm
-	Telemetry            telemetry.Reporter
-	TracerProvider       trace.TracerProvider
-	AutoImportTemplates  []AutoImportTemplate
-	GitAuthConfigs       []*gitauth.Config
-	RealIPConfig         *httpmw.RealIPConfig
-	TrialGenerator       func(ctx context.Context, email string) error
+	AWSCertificates                awsidentity.Certificates
+	Authorizer                     rbac.Authorizer
+	AzureCertificates              x509.VerifyOptions
+	GoogleTokenValidator           *idtoken.Validator
+	GithubOAuth2Config             *GithubOAuth2Config
+	OIDCConfig                     *OIDCConfig
+	PrometheusRegistry             *prometheus.Registry
+	SecureAuthCookie               bool
+	SSHKeygenAlgorithm             gitsshkey.Algorithm
+	Telemetry                      telemetry.Reporter
+	TracerProvider                 trace.TracerProvider
+	AutoImportTemplates            []AutoImportTemplate
+	GitAuthConfigs                 []*gitauth.Config
+	RealIPConfig                   *httpmw.RealIPConfig
+	TrialGenerator                 func(ctx context.Context, email string) error
 	// TLSCertificates is used to mesh DERP servers securely.
 	TLSCertificates    []tls.Certificate
 	TailnetCoordinator tailnet.Coordinator
 	DERPServer         *derp.Server
 	DERPMap            *tailcfg.DERPMap
 	SwaggerEndpoint    bool
 
+	// APIRateLimit is the minutely throughput rate limit per user or ip.
+	// Setting a rate limit <0 will disable the rate limiter across the entire
+	// app. Some specific routes have their own configurable rate limits.
+	APIRateLimit   int
+	LoginRateLimit int
+	FilesRateLimit int
+
 	MetricsCacheRefreshInterval time.Duration
 	AgentStatsRefreshInterval   time.Duration
 	Experimental                bool
@@ -158,6 +161,12 @@ func New(options *Options) *API {
 	if options.APIRateLimit == 0 {
 		options.APIRateLimit = 512
 	}
+	if options.LoginRateLimit == 0 {
+		options.LoginRateLimit = 60
+	}
+	if options.FilesRateLimit == 0 {
+		options.FilesRateLimit = 12
+	}
 	if options.Authorizer == nil {
 		options.Authorizer = rbac.NewAuthorizer()
 	}
@@ -231,6 +240,10 @@ func New(options *Options) *API {
 		Optional:        false,
 	})
 
+	// API rate limit middleware. The counter is local and not shared between
+	// replicas or instances of this middleware.
+	apiRateLimiter := httpmw.RateLimit(options.APIRateLimit, time.Minute)
+
 	r.Use(
 		httpmw.Recover(api.Logger),
 		tracing.StatusWriterMiddleware,
@@ -242,8 +255,8 @@ func New(options *Options) *API {
 		// handleSubdomainApplications checks if the first subdomain is a valid
 		// app URL. If it is, it will serve that application.
 		api.handleSubdomainApplications(
+			apiRateLimiter,
 			// Middleware to impose on the served application.
-			httpmw.RateLimit(options.APIRateLimit, time.Minute),
 			httpmw.ExtractAPIKey(httpmw.ExtractAPIKeyConfig{
 				DB:            options.Database,
 				OAuth2Configs: oauthConfigs,
@@ -269,7 +282,7 @@ func New(options *Options) *API {
 
 	apps := func(r chi.Router) {
 		r.Use(
-			httpmw.RateLimit(options.APIRateLimit, time.Minute),
+			apiRateLimiter,
 			httpmw.ExtractAPIKey(httpmw.ExtractAPIKeyConfig{
 				DB:            options.Database,
 				OAuth2Configs: oauthConfigs,
@@ -316,8 +329,9 @@ func New(options *Options) *API {
 
 		r.NotFound(func(rw http.ResponseWriter, r *http.Request) { httpapi.RouteNotFound(rw) })
 		r.Use(
-			// Specific routes can specify smaller limits.
-			httpmw.RateLimit(options.APIRateLimit, time.Minute),
+			// Specific routes can specify different limits, but every rate
+			// limit must be configurable by the admin.
+			apiRateLimiter,
 		)
 		r.Get("/", apiRoot)
 		// All CSP errors will be logged
@@ -340,9 +354,7 @@ func New(options *Options) *API {
 		r.Route("/files", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
-				// This number is arbitrary, but reading/writing
-				// file content is expensive so it should be small.
-				httpmw.RateLimit(12, time.Minute),
+				httpmw.RateLimit(options.FilesRateLimit, time.Minute),
 			)
 			r.Get("/{fileID}", api.fileByID)
 			r.Post("/", api.postFile)
@@ -427,25 +439,25 @@ func New(options *Options) *API {
 		r.Route("/users", func(r chi.Router) {
 			r.Get("/first", api.firstUser)
 			r.Post("/first", api.postFirstUser)
+			r.Get("/authmethods", api.userAuthMethods)
 			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.
+				// We use a tight limit for password login to protect against
+				// audit-log write DoS, pbkdf2 DoS, and simple brute-force
+				// attacks.
 				//
-				// Making this too small can break tests.
-				r.Use(httpmw.RateLimit(60, time.Minute))
+				// This value is intentionally increased during tests.
+				r.Use(httpmw.RateLimit(options.LoginRateLimit, 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) {
-					r.Use(httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient))
-					r.Get("/callback", api.userOAuth2Github)
+				r.Route("/oauth2", func(r chi.Router) {
+					r.Route("/github", func(r chi.Router) {
+						r.Use(httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient))
+						r.Get("/callback", api.userOAuth2Github)
+					})
+				})
+				r.Route("/oidc/callback", func(r chi.Router) {
+					r.Use(httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient))
+					r.Get("/", api.userOIDC)
 				})
-			})
-			r.Route("/oidc/callback", func(r chi.Router) {
-				r.Use(httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient))
-				r.Get("/", api.userOIDC)
 			})
 			r.Group(func(r chi.Router) {
 				r.Use(

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -92,7 +92,6 @@ type Options struct {
 	OIDCConfig           *coderd.OIDCConfig
 	GoogleTokenValidator *idtoken.Validator
 	SSHKeygenAlgorithm   gitsshkey.Algorithm
-	APIRateLimit         int
 	AutoImportTemplates  []coderd.AutoImportTemplate
 	AutobuildTicker      <-chan time.Time
 	AutobuildStats       chan<- executor.Stats
@@ -101,6 +100,11 @@ type Options struct {
 	GitAuthConfigs       []*gitauth.Config
 	TrialGenerator       func(context.Context, string) error
 
+	// All rate limits default to -1 (unlimited) in tests if not set.
+	APIRateLimit   int
+	LoginRateLimit int
+	FilesRateLimit int
+
 	// IncludeProvisionerDaemon when true means to start an in-memory provisionerD
 	IncludeProvisionerDaemon    bool
 	MetricsCacheRefreshInterval time.Duration
@@ -178,6 +182,17 @@ func NewOptions(t *testing.T, options *Options) (func(http.Handler), context.Can
 		options.DeploymentConfig = DeploymentConfig(t)
 	}
 
+	// If no ratelimits are set, disable all rate limiting for tests.
+	if options.APIRateLimit == 0 {
+		options.APIRateLimit = -1
+	}
+	if options.LoginRateLimit == 0 {
+		options.LoginRateLimit = -1
+	}
+	if options.FilesRateLimit == 0 {
+		options.FilesRateLimit = -1
+	}
+
 	ctx, cancelFunc := context.WithCancel(context.Background())
 	lifecycleExecutor := executor.New(
 		ctx,
@@ -271,6 +286,8 @@ func NewOptions(t *testing.T, options *Options) (func(http.Handler), context.Can
 			SSHKeygenAlgorithm:   options.SSHKeygenAlgorithm,
 			DERPServer:           derpServer,
 			APIRateLimit:         options.APIRateLimit,
+			LoginRateLimit:       options.LoginRateLimit,
+			FilesRateLimit:       options.FilesRateLimit,
 			Authorizer:           options.Authorizer,
 			Telemetry:            telemetry.NewNoop(),
 			TLSCertificates:      options.TLSCertificates,

diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go
@@ -661,7 +661,7 @@ func TestTemplateVersionDryRun(t *testing.T) {
 			Type: "cool_resource_type",
 		}
 
-		client := coderdtest.New(t, &coderdtest.Options{APIRateLimit: -1, IncludeProvisionerDaemon: true})
+		client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
 		user := coderdtest.CreateFirstUser(t, client)
 		version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{
 			Parse: echo.ParseComplete,
@@ -882,7 +882,7 @@ func TestTemplateVersionDryRun(t *testing.T) {
 func TestPaginatedTemplateVersions(t *testing.T) {
 	t.Parallel()
 
-	client := coderdtest.New(t, &coderdtest.Options{APIRateLimit: -1})
+	client := coderdtest.New(t, nil)
 	user := coderdtest.CreateFirstUser(t, client)
 	version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
 	template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -1397,7 +1397,7 @@ func TestWorkspacesByUser(t *testing.T) {
 func TestSuspendedPagination(t *testing.T) {
 	t.Parallel()
 	t.Skip("This fails when two users are created at the exact same time. The reason is unknown... See: https://github.com/coder/coder/actions/runs/3057047622/jobs/4931863163")
-	client := coderdtest.New(t, &coderdtest.Options{APIRateLimit: -1})
+	client := coderdtest.New(t, nil)
 	coderdtest.CreateFirstUser(t, client)
 
 	ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
@@ -1442,7 +1442,7 @@ func TestSuspendedPagination(t *testing.T) {
 // them using different page sizes.
 func TestPaginatedUsers(t *testing.T) {
 	t.Parallel()
-	client := coderdtest.New(t, &coderdtest.Options{APIRateLimit: -1})
+	client := coderdtest.New(t, nil)
 	coderdtest.CreateFirstUser(t, client)
 
 	// This test takes longer than a long time.

diff --git a/codersdk/deploymentconfig.go b/codersdk/deploymentconfig.go
@@ -41,7 +41,7 @@ type DeploymentConfig struct {
 	BrowserOnly                     *DeploymentConfigField[bool]            `json:"browser_only" typescript:",notnull"`
 	SCIMAPIKey                      *DeploymentConfigField[string]          `json:"scim_api_key" typescript:",notnull"`
 	Provisioner                     *ProvisionerConfig                      `json:"provisioner" typescript:",notnull"`
-	APIRateLimit                    *DeploymentConfigField[int]             `json:"api_rate_limit" typescript:",notnull"`
+	RateLimit                       *RateLimitConfig                        `json:"rate_limit" typescript:",notnull"`
 	Experimental                    *DeploymentConfigField[bool]            `json:"experimental" typescript:",notnull"`
 	UpdateCheck                     *DeploymentConfigField[bool]            `json:"update_check" typescript:",notnull"`
 	MaxTokenLifetime                *DeploymentConfigField[time.Duration]   `json:"max_token_lifetime" typescript:",notnull"`
@@ -146,6 +146,12 @@ type ProvisionerConfig struct {
 	ForceCancelInterval *DeploymentConfigField[time.Duration] `json:"force_cancel_interval" typescript:",notnull"`
 }
 
+type RateLimitConfig struct {
+	API   *DeploymentConfigField[int] `json:"api" typescript:",notnull"`
+	Files *DeploymentConfigField[int] `json:"files" typescript:",notnull"`
+	Login *DeploymentConfigField[int] `json:"login" typescript:",notnull"`
+}
+
 type SwaggerConfig struct {
 	Enable *DeploymentConfigField[bool] `json:"enable" typescript:",notnull"`
 }