Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[RateLimiter][Security] Improve performance of login/request rate limiter#46110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 5 commits intosymfony:6.2fromSeldaek:login_rate_limit_peek
Jul 24, 2022

Conversation

@Seldaek
Copy link
Member

@SeldaekSeldaek commentedApr 19, 2022•
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#40371
LicenseMIT
Doc PRsymfony/symfony-docs#...

The login rate limiter currently works this way:

  • consume 1 token on auth start (this READ + WRITES to cache backend / rate limiter storage), if this fails we fail the request as it exceeds the rate limit
  • if auth succeeded, clear the rate limit (this WRITES again)]
  • if auth failed, nothing further happens as we already consumed

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):

step11650362918.653657 [3 127.0.0.1:41584] "MGET" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6"1650362918.654027 [3 127.0.0.1:41584] "MGET" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6"1650362918.654294 [3 127.0.0.1:41584] "SETEX" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6" "119" "\x00\x00\x00\x02\x172Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x14\x01\x11\"\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00<login_per_ip-127.0.0.1\x0cA\xd8\x97\xa2\x98\xa9\xd8\xcf"1650362918.654651 [3 127.0.0.1:41584] "MGET" "lIoZODSNyI:d97b05c8178d81ea0e2009445cb6465d5fcffa39"1650362918.654877 [3 127.0.0.1:41584] "MGET" "lIoZODSNyI:d97b05c8178d81ea0e2009445cb6465d5fcffa39"1650362918.655146 [3 127.0.0.1:41584] "SETEX" "lIoZODSNyI:d97b05c8178d81ea0e2009445cb6465d5fcffa39" "119" "\x00\x00\x00\x02\x172Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x14\x01\x11\xcc\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00<login_per_ip_username-bd9913b290c5ecb5ebc0b925f736c15043ed9f214aa0bd9d980db7b9573b1a5f49943dc9a9fcccd22c9c0a630176ba7163df6cd32073cbe495fe94f7569c307a4b187e4917613982efbbe7ad2c3df785-127.0.0.1\x0cA\xd8\x97\xa2\x98\xa9\xe7\xb4"step 21650362918.691968 [3 127.0.0.1:41584] "UNLINK" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6"1650362918.692171 [3 127.0.0.1:41584] "UNLINK" "lIoZODSNyI:d97b05c8178d81ea0e2009445cb6465d5fcffa39"

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:

  • peek at token storage by consuming 0 tokens (4c2d670 allows doing this without writing to the cache to keep it to a simple READ from cache backend), if there are no tokens left available we fail the request as it is exceeding the rate limit
  • if auth success, nothing further happens
  • if auth fails, we then consume 1 token (which READ + WRITES to the cache)

Thanks to this, the happy path is much leaner on the cache backend:

1650374176.991495 [3 127.0.0.1:41926] "MGET" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6"1650374176.991774 [3 127.0.0.1:41926] "MGET" "lIoZODSNyI:d97b05c8178d81ea0e2009445cb6465d5fcffa39"

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).

1650373275.322053 [3 127.0.0.1:41906] "MGET" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6"1650373275.322431 [3 127.0.0.1:41906] "MGET" "lIoZODSNyI:a4df4a2604acb8d5003ba9789f3af4727f50fee5"1650373275.344146 [3 127.0.0.1:41906] "MGET" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6"1650373275.344402 [3 127.0.0.1:41906] "MGET" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6"1650373275.344666 [3 127.0.0.1:41906] "SETEX" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6" "62" "\x00\x00\x00\x02\x172Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x14\x01\x11\"\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00<login_per_ip-127.0.0.1\x0cA\xd8\x97\xac\xa7l1<"1650373275.344966 [3 127.0.0.1:41906] "MGET" "lIoZODSNyI:a4df4a2604acb8d5003ba9789f3af4727f50fee5"1650373275.345157 [3 127.0.0.1:41906] "MGET" "lIoZODSNyI:a4df4a2604acb8d5003ba9789f3af4727f50fee5"1650373275.345378 [3 127.0.0.1:41906] "SETEX" "lIoZODSNyI:a4df4a2604acb8d5003ba9789f3af4727f50fee5" "62" "\x00\x00\x00\x02\x172Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x14\x01\x11\xcd\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00<login_per_ip_username-bd9913b290c5ecb5ebc0b925f736c15043ed9f214aa0bd9d980db7b9573b1a5f49943dc9a9fcccd22c9c0a630176ba7163df6cd32073cbe495fe94f7569c307a4b187e4917613982efbbe7ad2c3df785z-127.0.0.1\x0cA\xd8\x97\xac\xa7l>\xc0"

@SeldaekSeldaek changed the title[RateLimiter][Security] Improve performance of login rate limiter[RateLimiter][Security] Improve performance of login/request rate limiterApr 19, 2022
@SeldaekSeldaekforce-pushed thelogin_rate_limit_peek branch 2 times, most recently from297a22a toa4005d5CompareApril 19, 2022 14:01
Copy link
Member

@wouterjwouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

@Seldaek
Copy link
MemberAuthor

Alright thanks for having a look :)

@chalasrchalasr modified the milestones:6.1,6.2May 8, 2022
@fabpot
Copy link
Member

@wouterj Can you have a look at this one? I'd love to be able to merge it for 6.2.

@wouterjwouterjforce-pushed thelogin_rate_limit_peek branch fromd842b72 to5c360dbCompareJuly 24, 2022 10:23
@wouterj
Copy link
Member

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 inf9229e6. Do you maybe know why you made these changes? If$availableTokens >= $consumedTokens, this means the token can be consumed immediately without waiting. I can't follow the reasoning to add wait time in this case.

@Seldaek
Copy link
MemberAuthor

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.

@fabpot
Copy link
Member

Thank you@Seldaek.

nicolas-grekas added a commit that referenced this pull requestDec 29, 2023
…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       | MITAlthough 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@wouterjwouterjwouterj left review comments

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

[RateLimiter][Security] Login rate limiting does not work for APIs/stateless firewalls

5 participants

@Seldaek@fabpot@wouterj@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp