From 51c825bf80f9277e2dce24d291da6f231cd30602 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 4 Jan 2023 14:38:45 +0000 Subject: [PATCH 1/5] feat: add more rate limit control flags --- cli/deployment/config.go | 24 ++++- cli/scaletest.go | 6 +- cli/server.go | 4 +- ..._scaletest_create-workspaces_--help.golden | 2 + cli/testdata/coder_server_--help.golden | 22 ++++- coderd/coderd.go | 94 +++++++++++-------- coderd/coderdtest/coderdtest.go | 19 +++- coderd/templateversions_test.go | 4 +- coderd/users_test.go | 4 +- codersdk/deploymentconfig.go | 8 +- 10 files changed, 129 insertions(+), 58 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index 97177d235996c..0b686bf49992d 100644 --- 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 index 01ad716805269..7cda26e81f9e7 100644 --- 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 index 740782779ab19..1ff9548ffc2ed 100644 --- 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 index 64d82b4f75917..0a5fbdcccacf7 100644 --- 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 index 43b97ccb2189b..c045641a1db6a 100644 --- 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 index 673ba22fb0fc8..cc325fbc3e95a 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -82,25 +82,21 @@ 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 @@ -108,6 +104,13 @@ type Options struct { 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 index 7da86ad83d6f6..be48868e46c9e 100644 --- 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 index 293a3d38d0ab3..f8bc407839106 100644 --- 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 index 56b891b3301e0..60e5c657a50b1 100644 --- 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 index 7a336595f91e4..30167e91753c4 100644 --- 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"` } From 91da46fdcc4900fc66e91bb2f4dd755a42be311d Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 4 Jan 2023 14:42:36 +0000 Subject: [PATCH 2/5] feat: add flag to disable all rate limits --- cli/deployment/config.go | 6 ++++++ cli/server.go | 8 ++++++++ cli/testdata/coder_server_--help.golden | 3 +++ codersdk/deploymentconfig.go | 7 ++++--- 4 files changed, 21 insertions(+), 3 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index 0b686bf49992d..865a5d9f77e96 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -430,6 +430,12 @@ func newConfig() *codersdk.DeploymentConfig { }, }, RateLimit: &codersdk.RateLimitConfig{ + DisableAll: &codersdk.DeploymentConfigField[bool]{ + Name: "Disable All Rate Limits", + Usage: "Disables all rate limits. This is not recommended in production.", + Flag: "dangerous-disable-rate-limits", + Default: false, + }, 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.", diff --git a/cli/server.go b/cli/server.go index 1ff9548ffc2ed..89eb7faf6f125 100644 --- a/cli/server.go +++ b/cli/server.go @@ -104,6 +104,14 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co return xerrors.Errorf("either HTTP or TLS must be enabled") } + // Disable rate limits if the `--dangerous-disable-rate-limits` flag + // was specified. + if cfg.RateLimit.DisableAll.Value { + cfg.RateLimit.API.Value = -1 + cfg.RateLimit.Login.Value = -1 + cfg.RateLimit.Files.Value = -1 + } + printLogo(cmd) logger := slog.Make(sloghuman.Sink(cmd.ErrOrStderr())) if ok, _ := cmd.Flags().GetBool(varVerbose); ok { diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index c045641a1db6a..7fef878ce77cd 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -28,6 +28,9 @@ Flags: with systemd. Consumes $CODER_CACHE_DIRECTORY (default "/tmp/coder-cli-test-cache") + --dangerous-disable-rate-limits Disables all rate limits. This is not + recommended in production. + Consumes $CODER_RATE_LIMIT_DISABLE_ALL --derp-config-path string Path to read a DERP mapping from. See: https://tailscale.com/kb/1118/custom-derp-servers/ Consumes $CODER_DERP_CONFIG_PATH diff --git a/codersdk/deploymentconfig.go b/codersdk/deploymentconfig.go index 30167e91753c4..1742ba1b3c48e 100644 --- a/codersdk/deploymentconfig.go +++ b/codersdk/deploymentconfig.go @@ -147,9 +147,10 @@ type ProvisionerConfig struct { } type RateLimitConfig struct { - API *DeploymentConfigField[int] `json:"api" typescript:",notnull"` - Files *DeploymentConfigField[int] `json:"files" typescript:",notnull"` - Login *DeploymentConfigField[int] `json:"login" typescript:",notnull"` + DisableAll *DeploymentConfigField[bool] `json:"disable_all" typescript:",notnull"` + API *DeploymentConfigField[int] `json:"api" typescript:",notnull"` + Files *DeploymentConfigField[int] `json:"files" typescript:",notnull"` + Login *DeploymentConfigField[int] `json:"login" typescript:",notnull"` } type SwaggerConfig struct { From 6b738c33cc4c7f6a5c1f9b2d1cae5ee31bed865a Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 5 Jan 2023 17:38:08 +0000 Subject: [PATCH 3/5] chore: remove new ratelimit flags --- cli/deployment/config.go | 12 ---- cli/server.go | 10 +-- cli/testdata/coder_server_--help.golden | 16 ----- coderd/apidoc/docs.go | 17 ++++- coderd/apidoc/swagger.json | 17 ++++- codersdk/deploymentconfig.go | 2 - docs/api/general.md | 35 +++++++--- docs/api/schemas.md | 73 ++++++++++++++++---- scripts/apidocgen/markdown-template/main.dot | 6 +- site/src/api/typesGenerated.ts | 8 ++- 10 files changed, 129 insertions(+), 67 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index 89758a844b4fd..f96bb10d6970b 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -442,18 +442,6 @@ func newConfig() *codersdk.DeploymentConfig { 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/server.go b/cli/server.go index c977e0ce5a49d..9d07673fbd640 100644 --- a/cli/server.go +++ b/cli/server.go @@ -106,10 +106,12 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co // Disable rate limits if the `--dangerous-disable-rate-limits` flag // was specified. + loginRateLimit := 60 + filesRateLimit := 12 if cfg.RateLimit.DisableAll.Value { cfg.RateLimit.API.Value = -1 - cfg.RateLimit.Login.Value = -1 - cfg.RateLimit.Files.Value = -1 + loginRateLimit = -1 + filesRateLimit = -1 } printLogo(cmd) @@ -441,8 +443,8 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co DeploymentConfig: cfg, PrometheusRegistry: prometheus.NewRegistry(), APIRateLimit: cfg.RateLimit.API.Value, - LoginRateLimit: cfg.RateLimit.Login.Value, - FilesRateLimit: cfg.RateLimit.Files.Value, + LoginRateLimit: loginRateLimit, + FilesRateLimit: filesRateLimit, HTTPClient: httpClient, } if tlsConfig != nil { diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 41ba7c8dd9be5..684612af5d2c9 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -64,27 +64,11 @@ 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/apidoc/docs.go b/coderd/apidoc/docs.go index 051674d0661c3..bb38f325e0a5c 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -2325,9 +2325,6 @@ const docTemplate = `{ "agent_stat_refresh_interval": { "$ref": "#/definitions/codersdk.DeploymentConfigField-time_Duration" }, - "api_rate_limit": { - "$ref": "#/definitions/codersdk.DeploymentConfigField-int" - }, "audit_logging": { "$ref": "#/definitions/codersdk.DeploymentConfigField-bool" }, @@ -2385,6 +2382,9 @@ const docTemplate = `{ "proxy_trusted_origins": { "$ref": "#/definitions/codersdk.DeploymentConfigField-array_string" }, + "rate_limit": { + "$ref": "#/definitions/codersdk.RateLimitConfig" + }, "scim_api_key": { "$ref": "#/definitions/codersdk.DeploymentConfigField-string" }, @@ -2950,6 +2950,17 @@ const docTemplate = `{ } } }, + "codersdk.RateLimitConfig": { + "type": "object", + "properties": { + "api": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-int" + }, + "disable_all": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-bool" + } + } + }, "codersdk.Response": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 71193925a37d3..265abb4d5d0ac 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2058,9 +2058,6 @@ "agent_stat_refresh_interval": { "$ref": "#/definitions/codersdk.DeploymentConfigField-time_Duration" }, - "api_rate_limit": { - "$ref": "#/definitions/codersdk.DeploymentConfigField-int" - }, "audit_logging": { "$ref": "#/definitions/codersdk.DeploymentConfigField-bool" }, @@ -2118,6 +2115,9 @@ "proxy_trusted_origins": { "$ref": "#/definitions/codersdk.DeploymentConfigField-array_string" }, + "rate_limit": { + "$ref": "#/definitions/codersdk.RateLimitConfig" + }, "scim_api_key": { "$ref": "#/definitions/codersdk.DeploymentConfigField-string" }, @@ -2662,6 +2662,17 @@ } } }, + "codersdk.RateLimitConfig": { + "type": "object", + "properties": { + "api": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-int" + }, + "disable_all": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-bool" + } + } + }, "codersdk.Response": { "type": "object", "properties": { diff --git a/codersdk/deploymentconfig.go b/codersdk/deploymentconfig.go index 69f755cf68c19..b0423cb927f06 100644 --- a/codersdk/deploymentconfig.go +++ b/codersdk/deploymentconfig.go @@ -149,8 +149,6 @@ type ProvisionerConfig struct { type RateLimitConfig struct { DisableAll *DeploymentConfigField[bool] `json:"disable_all" typescript:",notnull"` API *DeploymentConfigField[int] `json:"api" typescript:",notnull"` - Files *DeploymentConfigField[int] `json:"files" typescript:",notnull"` - Login *DeploymentConfigField[int] `json:"login" typescript:",notnull"` } type SwaggerConfig struct { diff --git a/docs/api/general.md b/docs/api/general.md index 53927245a057b..df45ce62b366e 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -129,17 +129,6 @@ curl -X GET http://coder-server:8080/api/v2/config/deployment \ "usage": "string", "value": 0 }, - "api_rate_limit": { - "default": 0, - "enterprise": true, - "flag": "string", - "hidden": true, - "name": "string", - "secret": true, - "shorthand": "string", - "usage": "string", - "value": 0 - }, "audit_logging": { "default": true, "enterprise": true, @@ -664,6 +653,30 @@ curl -X GET http://coder-server:8080/api/v2/config/deployment \ "usage": "string", "value": "string" }, + "rate_limit": { + "api": { + "default": 0, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": 0 + }, + "disable_all": { + "default": true, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": true + } + }, "scim_api_key": { "default": "string", "enterprise": true, diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 97bbf9adb496f..23a21d918ed20 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -844,17 +844,6 @@ CreateParameterRequest is a structure used to create a new parameter value for a "usage": "string", "value": 0 }, - "api_rate_limit": { - "default": 0, - "enterprise": true, - "flag": "string", - "hidden": true, - "name": "string", - "secret": true, - "shorthand": "string", - "usage": "string", - "value": 0 - }, "audit_logging": { "default": true, "enterprise": true, @@ -1379,6 +1368,30 @@ CreateParameterRequest is a structure used to create a new parameter value for a "usage": "string", "value": "string" }, + "rate_limit": { + "api": { + "default": 0, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": 0 + }, + "disable_all": { + "default": true, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": true + } + }, "scim_api_key": { "default": "string", "enterprise": true, @@ -1640,7 +1653,6 @@ CreateParameterRequest is a structure used to create a new parameter value for a | `address` | [codersdk.DeploymentConfigField-string](#codersdkdeploymentconfigfield-string) | false | | | | `agent_fallback_troubleshooting_url` | [codersdk.DeploymentConfigField-string](#codersdkdeploymentconfigfield-string) | false | | | | `agent_stat_refresh_interval` | [codersdk.DeploymentConfigField-time_Duration](#codersdkdeploymentconfigfield-time_duration) | false | | | -| `api_rate_limit` | [codersdk.DeploymentConfigField-int](#codersdkdeploymentconfigfield-int) | false | | | | `audit_logging` | [codersdk.DeploymentConfigField-bool](#codersdkdeploymentconfigfield-bool) | false | | | | `autobuild_poll_interval` | [codersdk.DeploymentConfigField-time_Duration](#codersdkdeploymentconfigfield-time_duration) | false | | | | `browser_only` | [codersdk.DeploymentConfigField-bool](#codersdkdeploymentconfigfield-bool) | false | | | @@ -1660,6 +1672,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a | `provisioner` | [codersdk.ProvisionerConfig](#codersdkprovisionerconfig) | false | | | | `proxy_trusted_headers` | [codersdk.DeploymentConfigField-array_string](#codersdkdeploymentconfigfield-array_string) | false | | | | `proxy_trusted_origins` | [codersdk.DeploymentConfigField-array_string](#codersdkdeploymentconfigfield-array_string) | false | | | +| `rate_limit` | [codersdk.RateLimitConfig](#codersdkratelimitconfig) | false | | | | `scim_api_key` | [codersdk.DeploymentConfigField-string](#codersdkdeploymentconfigfield-string) | false | | | | `secure_auth_cookie` | [codersdk.DeploymentConfigField-bool](#codersdkdeploymentconfigfield-bool) | false | | | | `ssh_keygen_algorithm` | [codersdk.DeploymentConfigField-string](#codersdkdeploymentconfigfield-string) | false | | | @@ -2532,6 +2545,42 @@ Parameter represents a set value for the scope. | ---------- | ------ | -------- | ------------ | ----------- | | `deadline` | string | true | | | +## codersdk.RateLimitConfig + +```json +{ + "api": { + "default": 0, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": 0 + }, + "disable_all": { + "default": true, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": true + } +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------------- | -------------------------------------------------------------------------- | -------- | ------------ | ----------- | +| `api` | [codersdk.DeploymentConfigField-int](#codersdkdeploymentconfigfield-int) | false | | | +| `disable_all` | [codersdk.DeploymentConfigField-bool](#codersdkdeploymentconfigfield-bool) | false | | | + ## codersdk.Response ```json diff --git a/scripts/apidocgen/markdown-template/main.dot b/scripts/apidocgen/markdown-template/main.dot index f2eea883eff57..6c324570d353e 100644 --- a/scripts/apidocgen/markdown-template/main.dot +++ b/scripts/apidocgen/markdown-template/main.dot @@ -45,7 +45,7 @@ return ""; } - let description = p.description.replaceAll("

", "\n").replaceAll("
", " "); + let description = p.description.replace(/

/g, "\n").replace(/
/g, " "); const words = description.split(' '); if (words.length == 0) { return ""; @@ -53,8 +53,8 @@ const countUppercase = words[0].length - words[0].replace(/[A-Z]/g, '').length; if (countUppercase > 1) { - let displayName = p.displayName.replaceAll("» **additionalProperties**", "It"); - displayName = displayName.charAt(0).toUpperCase() + displayName.replaceAll("_", " ").toLowerCase().slice(1); + let displayName = p.displayName.replace(/» \*\*additionalProperties\*\*/g, "It"); + displayName = displayName.charAt(0).toUpperCase() + displayName.replace(/_/g, " ").toLowerCase().slice(1); description = displayName + " " + words.slice(1).join(' '); } return correctLetterCase(description); diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index fd847b07241f8..78015f4376c43 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -308,7 +308,7 @@ export interface DeploymentConfig { readonly browser_only: DeploymentConfigField readonly scim_api_key: DeploymentConfigField readonly provisioner: ProvisionerConfig - readonly api_rate_limit: DeploymentConfigField + readonly rate_limit: RateLimitConfig readonly experimental: DeploymentConfigField readonly update_check: DeploymentConfigField readonly max_token_lifetime: DeploymentConfigField @@ -585,6 +585,12 @@ export interface PutExtendWorkspaceRequest { readonly deadline: string } +// From codersdk/deploymentconfig.go +export interface RateLimitConfig { + readonly disable_all: DeploymentConfigField + readonly api: DeploymentConfigField +} + // From codersdk/replicas.go export interface Replica { readonly id: string From fea5eaec59506f70c022e1f3a4babfe2f885697f Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 5 Jan 2023 17:51:22 +0000 Subject: [PATCH 4/5] fix: add env var override field to deployconfig --- cli/deployment/config.go | 45 ++++++++++++++++++------- cli/testdata/coder_server_--help.golden | 9 ++--- codersdk/deploymentconfig.go | 24 ++++++++----- 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index f96bb10d6970b..bc26f365207e4 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -437,10 +437,13 @@ func newConfig() *codersdk.DeploymentConfig { Default: false, }, 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, + 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 strict rate limits regardless of this value to prevent denial-of-service or brute force attacks.", + // Change the env from the auto-generated CODER_RATE_LIMIT_API to the + // old value to avoid breaking existing deployments. + EnvOverride: "CODER_API_RATE_LIMIT", + Flag: "api-rate-limit", + Default: 512, }, }, Experimental: &codersdk.DeploymentConfigField[bool]{ @@ -506,21 +509,30 @@ func setConfig(prefix string, vip *viper.Viper, target interface{}) { // assigned a value. if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") { value := val.FieldByName("Value").Interface() + + env, ok := val.FieldByName("EnvOverride").Interface().(string) + if !ok { + panic("DeploymentConfigField[].EnvOverride must be a string") + } + if env == "" { + env = formatEnv(prefix) + } + switch value.(type) { case string: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetString(vip.GetString(prefix)) case bool: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetBool(vip.GetBool(prefix)) case int: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetInt(int64(vip.GetInt(prefix))) case time.Duration: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetInt(int64(vip.GetDuration(prefix))) case []string: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) // As of October 21st, 2022 we supported delimiting a string // with a comma, but Viper only supports with a space. This // is a small hack around it! @@ -634,7 +646,7 @@ func setViperDefaults(prefix string, vip *viper.Viper, target interface{}) { val := reflect.ValueOf(target).Elem() val = reflect.Indirect(val) typ := val.Type() - if strings.HasPrefix(typ.Name(), "DeploymentConfigField") { + if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") { value := val.FieldByName("Default").Interface() vip.SetDefault(prefix, value) return @@ -671,7 +683,7 @@ func AttachFlags(flagset *pflag.FlagSet, vip *viper.Viper, enterprise bool) { func setFlags(prefix string, flagset *pflag.FlagSet, vip *viper.Viper, target interface{}, enterprise bool) { val := reflect.Indirect(reflect.ValueOf(target)) typ := val.Type() - if strings.HasPrefix(typ.Name(), "DeploymentConfigField") { + if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") { isEnt := val.FieldByName("Enterprise").Bool() if enterprise != isEnt { return @@ -680,8 +692,17 @@ func setFlags(prefix string, flagset *pflag.FlagSet, vip *viper.Viper, target in if flg == "" { return } + + env, ok := val.FieldByName("EnvOverride").Interface().(string) + if !ok { + panic("DeploymentConfigField[].EnvOverride must be a string") + } + if env == "" { + env = formatEnv(prefix) + } + usage := val.FieldByName("Usage").String() - usage = fmt.Sprintf("%s\n%s", usage, cliui.Styles.Placeholder.Render("Consumes $"+formatEnv(prefix))) + usage = fmt.Sprintf("%s\n%s", usage, cliui.Styles.Placeholder.Render("Consumes $"+env)) shorthand := val.FieldByName("Shorthand").String() hidden := val.FieldByName("Hidden").Bool() value := val.FieldByName("Default").Interface() diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 684612af5d2c9..49f2da97c55b0 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -18,10 +18,11 @@ Flags: 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. - Consumes $CODER_RATE_LIMIT_API (default 512) + API endpoints have separate strict rate + limits regardless of this value to + prevent denial-of-service or brute force + attacks. + Consumes $CODER_API_RATE_LIMIT (default 512) --cache-dir string The directory to cache temporary files. If unspecified and $CACHE_DIRECTORY is set, it will be used for compatibility diff --git a/codersdk/deploymentconfig.go b/codersdk/deploymentconfig.go index b0423cb927f06..6842f15a66c23 100644 --- a/codersdk/deploymentconfig.go +++ b/codersdk/deploymentconfig.go @@ -160,15 +160,21 @@ type Flaggable interface { } type DeploymentConfigField[T Flaggable] struct { - Name string `json:"name"` - Usage string `json:"usage"` - Flag string `json:"flag"` - Shorthand string `json:"shorthand"` - Enterprise bool `json:"enterprise"` - Hidden bool `json:"hidden"` - Secret bool `json:"secret"` - Default T `json:"default"` - Value T `json:"value"` + Name string `json:"name"` + Usage string `json:"usage"` + Flag string `json:"flag"` + // EnvOverride will override the automatically generated environment + // variable name. Useful if you're moving values around but need to keep + // backwards compatibility with old environment variable names. + // + // NOTE: this is not supported for array flags. + EnvOverride string `json:"-"` + Shorthand string `json:"shorthand"` + Enterprise bool `json:"enterprise"` + Hidden bool `json:"hidden"` + Secret bool `json:"secret"` + Default T `json:"default"` + Value T `json:"value"` } // MarshalJSON removes the Value field from the JSON output of any fields marked Secret. From f3d5166935af8b05080f72b777b994a802a3f96b Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 5 Jan 2023 17:53:30 +0000 Subject: [PATCH 5/5] fixup! fix: add env var override field to deployconfig --- cli/deployment/config.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index bc26f365207e4..69451de3a16e3 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -600,6 +600,9 @@ func readSliceFromViper[T any](vip *viper.Viper, key string, value any) []T { // Ensure the env entry for this key is registered // before checking value. + // + // We don't support DeploymentConfigField[].EnvOverride for array flags so + // this is fine to just use `formatEnv` here. vip.MustBindEnv(configKey, formatEnv(configKey)) value := vip.Get(configKey) @@ -709,7 +712,7 @@ func setFlags(prefix string, flagset *pflag.FlagSet, vip *viper.Viper, target in // Allow currently set environment variables // to override default values in help output. - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) switch value.(type) { case string: