Reset push rate limit on successful gem push

[sonalkr132]

We have rate limit with exp bacioff on endpoints with mfa to mitigate brute force of the otp code
https://security.stackexchange.com/questions/185905/maximum-tries-for-2fa-code
Check #2078 for more info on the attack.

Ideally, we only need to throttle 401 unauthorized requests to mitigate this, however rack-attack doesn't have option of rate limit on response status. resetting rate limit cache on successful push is the closest alternative (smiliar usecase).

Exp rate limit was removed from push as we had used incorrect back off period and users were seeing unreasonable backoff time. This has since been fixed, max back off period is now of 11 days.

[sonalkr132]

I am hoping to avoid #2081 and #2263, which were resolved by #2268. However, we do need exponential backoff on mfa endpoints.

Please let me know something is not clear.

[simi]

Ideally, we only need to throttle 401 unauthorized requests to mitigate this, however rack-attack doesn't have option of rate limit on response status. resetting rate limit cache on successful push is the closest alternative (smiliar usecase).

Not sure how hurry are you in here @sonalkr132, but alternativelly I can take a look if it would not be possible to contribute this to rack-test actually. Would that work in here?

Rack::Attack.throttle("requests by ip", limit: 5, period: 2, status: 401) do |request|
  request.ip
end

[simi]

Should we do that only when @remote_ip.present??

[sonalkr132]

Not sure how hurry are you in here

This attack is only possible if you already know the api key or the password. Even after that success of the brute force (after continuous attempt for days) is very probabilistic. So, not urgent.

I can take a look if it would not be possible to contribute this to rack-test actually

They have declined the request to limit on response in past rack/rack-attack#216

[sonalkr132]

Calling Rack::Attack.cache.count increments the key, hence this asserts for 1, instead of 0. I couldn't get read cache to work.. something about key prefix.

[simi]

I'll take a look. 👀

[simi]

yes, you need to specify "time" part manually for read

this code works (and fails if I comment out reset in rack_attack_reset.rb)

should "reset gem push rate limit rack attack key" do
  Rack::Attack::EXP_BACKOFF_LEVELS.each do |level|
    push_exp_throttle_level_key = "#{Rack::Attack::PUSH_EXP_THROTTLE_KEY}/#{level}:#{@ip_address}"
    period = exp_base_limit_period**level
    key = "#{Time.now.to_i / period}:#{push_exp_throttle_level_key}"

    assert_equal nil, Rack::Attack.cache.read(key)
  end
end

got inspired at https://github.com/kickstarter/rack-attack/blob/580368faddcb563b55ad2ec4aa4cd05aae845547/spec/rack_attack_throttle_spec.rb#L72-L78

[sonalkr132]

Thanks simi. Updated to this.

#{Time.now.to_i / period}

Doesn't this mean if period was much smaller 1-2 seconds, key could change between setup and assert?

[simi]

It can happen with any period actually. It should be safer to wrap those specs with freeze_time. 👍

https://api.rubyonrails.org/classes/ActiveSupport/Testing/TimeHelpers.html#method-i-freeze_time

[sonalkr132]

The idea of the key changing between the time key is set (in rack_attack.rb) and reset (in pusher.rb) did cross my mind but I assumed their reset_count implementation must be correct. Looking at this more closing after your comment, I don't think it is. Consider following:

# time during `count` in rack_attack.rb
> Time.now.to_i 
 => 1587326999
> Time.now.to_i / 100 
 => 1587326

# churn code, query pg, upload to s3. time during `reset_count`  in pusher.rb
> Time.now.to_i 
=> 1587327001
> Time.now.to_i / 100 
=> 1587327

To reiterate, key_and_expiry gets called from both count and reset_count but there is no grantee that Time.now will be the same.

I suppose similar to their workaround for key expiry rack/rack-attack#85 we should purge be purging two keys (rack::attack:1587326:.. and rack::attack:1587327:..).

[sonalkr132]

@simi updated this to reset two keys. check last commit message or this comment