Fix WithUnauthenticatedLimiter and WithAccessDeniedLimiter rate limiters #61788
Conversation
juliaogris
left a comment
juliaogris
left a comment
There was a problem hiding this comment.
LGTM with a few question.
Generic testing / compatibility question: For new test code should we favor synctest over clockwork.FakeClock?
| } | ||
|
|
||
| // IsRateLimited checks if consuming the specified number of tokens would be rate-limited | ||
| // without actually consuming any tokens. Returns true if rate-limited, false otherwise. |
There was a problem hiding this comment.
The comment says "consuming the specified number of tokens" but the method takes no parameters and always checks if there's at least 1 token available.
Maybe update to:
// IsRateLimited checks if the bucket is currently rate-limited (no tokens available)
// without actually consuming any tokens. Returns true if rate-limited, false otherwise.
| } | ||
|
|
||
| func rateLimitRequest(r *http.Request, limiter *limiter.RateLimiter) error { | ||
| remote, _, err := net.SplitHostPort(r.RemoteAddr) |
There was a problem hiding this comment.
Should this be updated to:
remote, err := getRemoteAddressFromRequest(r)| func isRequestRateLimited(r *http.Request, limiter *limiter.RateLimiter) bool { | ||
| remote, err := getRemoteAddressFromRequest(r) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Silently swallows error - does this deserve a comment?
conditionalLimiterFunc returns an error if getRemoteAddressFromRequest fails
unauthenticatedLimiterFunc does not, should they be consistent?
public-teleport-github-review-bot
bot
removed request for
kimlisa and
kopiczko
| callCount := 0 | ||
| handle := h.WithUnauthenticatedLimiter(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (any, error) { | ||
| callCount++ | ||
| return nil, trace.BadParameter("invalid request") |
There was a problem hiding this comment.
Is there a specific reason for not returning 200 here?
I mean, is the specific status code important, other than
- being preserved by the
WithUnauthenticatedLimiteradapter, and - not being
StatusBadRequestso you can tell when the rate limiter kicks in?
What
Fix rate limiting logic in
WithUnauthenticatedLimiterandWithAccessDeniedLimitermiddleware to check rate limits before performing authentication or executing the handler.Problem
Previously, these middleware functions would:
Requires #61776
Address #61510
Contributes to #61511