[RateLimiter][Security] Improve performance of login/request rate limiter #46110
Conversation
0e4b202 to
495f679
Compare
297a22a to
a4005d5
Compare
There was a problem hiding this comment.
Hi! I had a quick look at the changes and read the PR description. Definitely sounds like a good way forward, and without any BC break/deprecations.
However, this really is a new feature and not a bugfix. That means that unfortunately, it won't be included in 6.1 (feature freeze was a month and a few days ago).
I'll have a deeper look when the development phase of 6.2 begins (especially for the TokenBucket implementation). I need to dig into the workings of the rate limiters again, and I prefer to focus on stabilizing 6.1 for the next 4 weeks.
src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php
Outdated
Show resolved
Hide resolved
|
Alright thanks for having a look :) |
|
@wouterj Can you have a look at this one? I'd love to be able to merge it for 6.2. |
…stener to reduce amount of writes on the cache backend, fixes symfony#40371
…eking/consuming 0 tokens
Co-authored-by: Wouter de Jong <wouterj@users.noreply.github.com>
d842b72 to
5c360db
Compare
|
I've checked the changes and added a test for all policies to make sure we don't break peeking behavior. @Seldaek I've basically reverted the changes you did in f9229e6. Do you maybe know why you made these changes? If |
|
I don't recall exactly sorry, mentally quite far away from this PR at this point.. But I think your change makes sense. I will for sure check in real life conditions whenever it is merged that the original intent is still achieved. |
|
Thank you @Seldaek. |
…licies (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [RateLimit] Test and fix peeking behavior on rate limit policies | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Ref #52835 | License | MIT Although heavily discouraged for user-land code, we've implemented peeking behavior for rate limiting in 6.2 with #46110. However, I found that our rate limit policies show very inconsistent behavior on this. As we didn't have great test covering peeking return values, we broke BC with #51676 in 6.4. I propose this PR to verify the behavior of the policies and also make it inconsistent. I target 6.3 because we rely on this in the login throttling since 6.2 and this shows buggy error messages ("try again in 0 minute") when not using the default policy for login throttling. > [!NOTE] > When merging this PR, there will be heavy merge conflicts in the SlidingWindowLimiter. You can ignore the changes in this PR for this policy in 6.4. I'll rebase and update #52835 to fix the sliding window limiter in 6.4+ Commits ------- e4a8c33 [RateLimit] Test and fix peeking behavior on rate limit policies
The login rate limiter currently works this way:
So the happy path which is the most common (login succeeds) looks something like that on redis, as there are two rate limiter (global per IP and local per username+IP combo):
This workflow is fine for form logins, but when rate limiting an API authenticator which works with a token, you end up doing a login on every request, so in that case it's less than optimal. This PR aims to improve that by changing the workflow to the following:
Thanks to this, the happy path is much leaner on the cache backend:
And the login failed path consumes a token still of course so it looks a lot like the previous step1 but with an additional READ (that's the only trade-off this PR makes perf wise as far as I can tell).