Add exponential backoff rate limit to endpoints using mfa #2078
Conversation
config/initializers/rack_attack.rb
Outdated
| "/api/v1/gems/yank", # gem yank | ||
| "/api/v1/gems" # gem push | ||
| ] | ||
| add_owner_regex = Regexp.new("\/api\/v1\/gems\/\\w+\/owners") |
There was a problem hiding this comment.
The \w (word character [a-zA-Z0-9_]) in the add_owner_regex might be a bit too strict. Here's the route:
POST
/api/v1/gems/:rubygem_id/owners(.:format)
api/v1/owners#create {:format=>/json|yaml/, :rubygem_id=>/[A-Za-z0-9\.\-_]+?/}
The pattern for rubygem_id is a bit more relaxed than \w+. Paths with rubygem_id values containing ., - would bypass the throttle.
Consider using Rails.application.routes.recognize_path(req.path) to reuse its pattern matching here to prevent this category of issues.
recognize_path returns the controller and action for matching and is less brittle than duplicating and matching on route patterns.
There was a problem hiding this comment.
Thanks @eliotsykes 💯
I have updated it to use recognize_path.
|
What do you think about throttling closer to the OTP verification step? (in Its throttle key could include the user's id to protect against attacks on a single user that are spread across multiple IPs. |
Exponential backoff was needed as with 100 req/10 min limit used in other endpoints would have been inefficient against brute force of mfa code. 30 sec window would have had probability of 1/10000 (1 - (1 - p)^n, where p = 10^-6 and n = 100) for successfully guessing the key *at least once* if limits were 100 req/10 min. Using binomial distribution, a HO reporter calculcated that after 6932 trials, he would have had more than 50% chance of successfully guessing the key at least once. Limits used in this change would allow 4 trails per week and total of 33 years for 6932 trials.
sonalkr132
force-pushed
the
mfa-rate-limit
branch
2 times, most recently
from
d238190
to
48dcecf
Compare
I would prefer to keep all throttling logic in rack attack initializer, which doesn't mean we can't throttle on per user basis. You have already noticed the use of |
regex matching was prone to errors/loopholes.
sonalkr132
force-pushed
the
mfa-rate-limit
branch
from
48dcecf
to
85e39b2
Compare
Exponential backoff was needed as with 100 req/10 min limit used in
other endpoints would have been inefficient against brute force of
mfa code.
30 sec window would have had probability of 1/10000 (1 - (1 - p)^n, where p = 10^-6 and n = 100)
for successfully guessing the key at least once if limits were 100 req/10 min.
Using binomial distribution, a HO reporter calculcated that after
6932 trials, he would have had more than 50% chance of successfully
guessing the key at least once.
Limits used in this change would allow 4 trails per week and total
of 33 years for 6932 trials.