feat: add rate limiting to 2fa attempts #18584
Conversation
Limits specifically calls to the 2FA-related actions: - checking a recovery code - checking a TOTP value - checking a Webauthn value The rate limits were selected to be a balance of usability vs how long it would take a slow-roll actor to continue trying. Metrics are emitted for monitoring and alerting purposes. Refs: pypi#8456 Signed-off-by: Mike Fiedler <[email protected]>
There was a problem hiding this comment.
The change in the ratelimit, unless I'm mistaken, means attacks will be 2.4 times slower (so >50% chance of success in 7 months)
The new metrics will surely help measuring the normal "noise" level of users failing to 2FA (assuming those kinds of attacks are rare) (maybe we're about to discover something huge :D )
This will definitely help setting the right thresholds when later pulling out the effective measures that have been discussed (exponential backoff, auto-lockout, ...)
👍 for me and thanks for taking the time to act on this 5-year-old issue :) (issue birthday was just a few days ago) <3
| @@ -178,6 +178,19 @@ def includeme(config): | |||
| config.register_service_factory( | |||
| RateLimit(global_login_ratelimit_string), IRateLimiter, name="global.login" | |||
| ) | |||
| # Register separate rate limiters for 2FA attempts | |||
| twofa_user_ratelimit_string = config.registry.settings.get( | |||
There was a problem hiding this comment.
I'd tend to suggest "mfa" as a synonyme of 2fa that doesn't start with a digit, but we've used 2fa elsewhere, so that's just a suggestion.
Limits specifically calls to the 2FA-related actions:
The rate limits were selected to be a balance of usability vs how long it would take a slow-roll actor to continue trying.
Metrics are emitted for monitoring and alerting purposes.
Refs: #8456