coderd: tighten /login rate limiting #4432
Conversation
ammario
commented
Oct 8, 2022
- Add ability for Owners to bypass rate limiting
- Improve overall test coverage of rate limit middleware
| @@ -24,6 +24,9 @@ const ( | |||
| SessionCustomHeader = "Coder-Session-Token" | |||
| OAuth2StateKey = "oauth_state" | |||
| OAuth2RedirectKey = "oauth_redirect" | |||
|
|
|||
| // nolint: gosec | |||
| BypassRatelimitHeader = "X-Coder-Bypass-Ratelimit" | |||
There was a problem hiding this comment.
Why not just allow owner to bypass implicitly without needing to set a header?
There was a problem hiding this comment.
I thought it may lead to bugs / misunderstandings with other features that rely on the rate limit. For example, a spinning script.
Also, we may not realize if the front end is sending too many requests since we usually test with Owner.
coderd/httpmw/ratelimit.go
Outdated
| return httprate.KeyByIP(r) | ||
| } | ||
|
|
||
| if r.Header.Get(codersdk.BypassRatelimitHeader) == "" { |
There was a problem hiding this comment.
Do you care if the value of the header is false? Should you strconv.ParseBool?
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
Another way to do this is add a resource for bypassing the rate limit. Then the FE can actually check if the capability exists, and we can give this to other roles as well.
ResourceRateLimitBypass = Object {
Type: "bypass_rate_limit",
}There was a problem hiding this comment.
Last I checked Rego was taking 20%+ of test CPU time, so it seems counterproductive the DoS prevention goals of the rate limiter. I added a comment to that effect.