Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to reset specific throttles #113

Closed
rwdaigle opened this issue Feb 5, 2015 · 14 comments · Fixed by #128
Closed

Ability to reset specific throttles #113

rwdaigle opened this issue Feb 5, 2015 · 14 comments · Fixed by #128

Comments

@rwdaigle
Copy link

rwdaigle commented Feb 5, 2015

I am finding it very difficult to do the following, which makes me think I'm using this software incorrectly or for the wrong use-case:

I am currently throttling login attempts to my app. When a user resets their password, I would like to reset the throttle for that specific user. It seems as if the Rack::Attack API is very unfriendly to such under-the-cover shenanigans. There is no throttle-specific reset function, nor is it particularly easy to recreate the key used to store the incrementer to allow for manual resetting.

What is the right way to go about this? Thanks!

@ktheory
Copy link
Collaborator

ktheory commented Feb 5, 2015

I am currently throttling login attempts to my app.

Excellent! That's a good security practice.

When a user resets their password, I would like to reset the throttle for that specific user.

Can you explain further the use case?

Generally, this doesn't come up b/c login throttles have such a high threshold that legit attempts (e.g. users who forgot their password) don't reach the threshold. It just catches malicious crackers. So it's moot to reset the throttle when the password changes.

@rwdaigle
Copy link
Author

rwdaigle commented Feb 5, 2015

Hi @ktheory,

Generally, this doesn't come up b/c login throttles have such a high threshold

In our environment, and subject to the rigors of PCI compliance, our threshold limit is definitely within reach for some legitimate login attempts.

Also, it might be nice to have such functionality exposed, or at least a primitive available, to aid in resetting test environments. I currently have to do the following to un-block my test login attempts, which seems to be a bit heavy-handed:

Rack::Attack.cache.store.clear

Seems like for both situations, having a more accessible API would be advantageous?

@zmillman
Copy link
Contributor

zmillman commented Feb 6, 2015

From the PCI compliance guidelines (copied from http://security.stackexchange.com/a/523)

  • 8.5.13 (Limit repeated access attempts by locking out the user ID after not more than six attempts)
  • 8.5.14 (Set the lockout duration to thirty minutes or until administrator enables the user ID).

These restrictions only apply to accounts which are accessing cardholder data which is usually only internally authorized employees...but it looks like @rwdaigle is working for Spreedly so there's compliance issues that most other developers wouldn't have.

It looks like it's possible to infer the key name which is built here: https://github.com/kickstarter/rack-attack/blob/master/lib/rack/attack/throttle.rb#L26 and prefixed with the epoch here: https://github.com/kickstarter/rack-attack/blob/master/lib/rack/attack/cache.rb#L17

If you have your throttle configured as

Rack::Attack.throttle('logins/email', :limit => 6, :period => 30.minutes) do |req|
  req.params['email'] if req.path == '/login' && req.post?
end

then you should be able to reset the login attempts for [email protected] like this

email = "[email protected]"
epoch_time = Time.now.to_i
Rack::Attack.cache.store.delete "#{Rack::Attack.cache.prefix}:#{epoch_time / 30.minutes}:logins/email:#{email}"

Yeah, looking at all this, it's a really ugly hack. Having a delete(unprefixed_key, period) method on Rack::Attack::Cache and a cache_key(discriminator) method on Rack::Attack::Throttle would be really helpful for this.

@zmillman
Copy link
Contributor

zmillman commented Feb 6, 2015

Maybe a better implementation for above using monkey patches:

class Rack::Attack
  def self.reset_throttle(name, discriminator)
    if (throttle = @throttles.detect { |t| t.name == name })
      throttle.reset(discriminator)
    end
  end
end

class Rack::Attack::Throttle
  def reset(discriminator)
    current_period = period.respond_to?(:call) ? period.call(req) : period
    cache.reset_count "#{name}:#{discriminator}", current_period
  end
end

class Rack::Attack::Cache
  def reset_count(unprefixed_key, period)
    epoch_time = Time.now.to_i
    # Add 1 to expires_in to avoid timing error: http://git.io/i1PHXA
    expires_in = period - (epoch_time % period) + 1
    key = "#{prefix}:#{(epoch_time/period).to_i}:#{unprefixed_key}"
    store.write(key, 0, :expires_in => expires_in)
  end
end

Example usage:

Rack::Attack.reset_throttle "logins/email", "[email protected]"

@zmillman
Copy link
Contributor

zmillman commented Feb 6, 2015

I think this might be a good candidate feature for 4.3.0, but in the meantime I added it to Advanced Configuration in the wiki: https://github.com/kickstarter/rack-attack/wiki/Advanced-Configuration#reset-specific-throttles

@rwdaigle
Copy link
Author

rwdaigle commented Feb 9, 2015

@zmillman Thanks for this! We are using the monkey patch for the time being. Can't wait to see a proper API emerge for this use case, eventually.

Cheers.

@stanhu
Copy link
Contributor

stanhu commented Mar 15, 2015

Yes, I would like to see this too. One issue with this line:

store.write(key, 0, :expires_in => expires_in)

If you're using a Redis store, the problem with this line is 0 is encoded via Marshal.dump(0), which serializes the value. Using increment later doesn't work because the value is set to "\x04\bi\x00".

The issue is that Rack Attack doesn't have the ability to tell the Redis store that 0 is a raw value via the options hash. I'll submit a pull request that passes this hash along.

stanhu added a commit to stanhu/rack-attack that referenced this issue Mar 15, 2015
… possible

to implement something like:

```store.write(key, 0, :expires_in => expires_in)```

See rack#113
@zmillman
Copy link
Contributor

Hmm, maybe it's just easier to delete the key? It would have the same effect without introducing extra complexity to the codepaths that other people are already using.

@stanhu
Copy link
Contributor

stanhu commented Mar 15, 2015

Yeah, deleting the key seems clearer. I just submitted a pull request to be able to delete a key from Redis.

I still think Rack Attack's proxy class should either support the options hash or just read/write data in raw mode. It's really not obvious that calling write with the 0 value would result in "\x04\bi\x00". Given that increment already stores strings in raw mode, it seems to make sense that read and write should use the same mode.

dzaporozhets added a commit to gitlabhq/gitlabhq that referenced this issue Mar 24, 2015
Reduce Rack Attack false positives causing 403 errors during HTTP authentication

### What does this MR do?

This MR reduces false positives causing `403 Forbidden` messages after HTTP authentication.

A Git client may attempt to access a repository without a password. If it receives a 401 error, the client often will try again, this time supplying a password. The problem is that `grack_auth.rb` considers a blank password an authentication failure and increases a Redis counter each time this happens. With enough requests, an IP can be banned temporarily even though previous attempts may have been successful. This leads users to see `403 Forbidden` errors until the ban times out (default: 1 hour).

To reduce the chance of a false positive, this MR resets the counter upon a successful authentication from an IP.

In addition, this MR logs when a user has been banned and introduces the ability to disable Rack Attack via a config variable.

### Are there points in the code the reviewer needs to double check?

rack-attack v4.2.0 doesn't support the ability to clear counters out of the box, so `rack_attack_helpers.rb` includes a number of monkey patches to make it work. It looks like this functionality may be added in v4.3.0. I've also sent pull requests to rack-attack to add the functionality necessary to delete a key.

Each time an authentication is successful, the Redis counter for that IP is cleared. I deemed it better to clear the counter than to allow for blank passwords, since the latter seems like a security risk.

### Why was this MR needed?

It was quite difficult to figure out why users were seeing `403 Forbidden`, which is why the log message was added. Users were getting a lot of false positives when accessing repositories with HTTPS. Including the username in the HTTPS URL (e.g. `https://[email protected]/account/repo.git`) caused authentication failures because while the git client provided the username, it left the password blank, leading to an authentication failure.

### What are the relevant issue numbers / [Feature requests](http://feedback.gitlab.com/)?

See Issue #1171

rack/rack-attack#113

See merge request !392
@stanhu
Copy link
Contributor

stanhu commented Apr 16, 2015

Is there any interest in supporting this feature in 4.3.0? GitLab will be shipping with a monkey patch for Rack Attack.

stanhu added a commit to stanhu/rack-attack that referenced this issue May 11, 2015
stanhu added a commit to stanhu/rack-attack that referenced this issue May 11, 2015
stanhu added a commit to stanhu/rack-attack that referenced this issue May 11, 2015
stanhu added a commit to stanhu/rack-attack that referenced this issue May 22, 2015
stanhu added a commit to stanhu/rack-attack that referenced this issue May 22, 2015
stanhu added a commit to stanhu/rack-attack that referenced this issue May 22, 2015
stanhu added a commit to stanhu/rack-attack that referenced this issue May 22, 2015
@ryantm
Copy link

ryantm commented Nov 18, 2016

The monkey patch described here doesn't seem to work anymore with the error "undefined method `name' for #<Array..." Was this issue solved in some other way? I am trying to make my test cases more repeatable by clearing the throttles. What do people do for that?

@ryantm
Copy link

ryantm commented Nov 18, 2016

I updated the monkey patch to work with rack-attack 5.0.1:

class Rack::Attack
  def self.reset_throttle(name, discriminator)
    if (throttle = (@throttles.detect { |t| t.first == name })[1])
      throttle.reset(discriminator)
    end
  end
end

class Rack::Attack::Throttle
  def reset(discriminator)
    current_period = period.respond_to?(:call) ? period.call(req) : period
    cache.reset_count "#{name}:#{discriminator}", current_period
  end
end

class Rack::Attack::Cache
  def reset_count(unprefixed_key, period)
    epoch_time = Time.now.to_i
    # Add 1 to expires_in to avoid timing error: http://git.io/i1PHXA
    expires_in = period - (epoch_time % period) + 1
    key = "#{prefix}:#{(epoch_time/period).to_i}:#{unprefixed_key}"
    store.write(key, 0, :expires_in => expires_in)
  end
end

@brandoncc
Copy link

With no way to look up the name, I am not sure how to get that at the time when I want to clear the throttle. For that reason, I changed reset_throttle to this:

def self.reset_throttle(discriminator)
  @throttles.each { |_, throttle| throttle.reset(discriminator) }
end

It writes the cache entries, but the user is still throttled.

This is my logic for creating the throttles, which works and nests them nicely:

get_user = lambda do |req|
  user_id = req.session["user_id"]
  User.find(user_id) if user_id
end

get_filter_identifier = lambda do |req|
  user = get_user[req]

  if user&.id
    "user_#{user.id}"
  else
    "ip_#{req.ip}"
  end
end

define_throttle = lambda do |limit, period|
  # the name format is important because it is parsed and the user state is
  # updated using the parsed data
  Rack::Attack.throttle("requests/#{limit}in#{period.to_i}seconds", limit: limit, period: period) do |req|
    throttle = unthrottled_access_allowed_paths.none? do |path|
      req.path.start_with?(path)
    end

    get_filter_identifier if throttle
  end
end

# throttles need to be set in order of length descending in order to nest properly
define_throttle[1000, 1.day]
define_throttle[100, 1.hour]
define_throttle[60, 1.minute]
define_throttle[1, 1.second]

Here is one of my memcached slabs:

stats cachedump 2 100
ITEM rack::attack:1497914156:requests/1in1seconds:user_1 [4 b; 1497914146 s]
ITEM rack::attack:24965235:requests/60in60seconds:user_1 [4 b; 1497914149 s]
ITEM rack::attack:1497914145:requests/1in1seconds:user_1 [4 b; 1497914134 s]
ITEM rack::attack:1497913687:requests/1in1seconds:user_1 [4 b; 1497913677 s]
ITEM rack::attack:24965228:requests/60in60seconds:user_1 [4 b; 1497913729 s]
ITEM rack::attack:1497913674:requests/1in1seconds:user_1 [4 b; 1497913663 s]
ITEM rack::attack:24965227:requests/60in60seconds:user_1 [4 b; 1497913668 s]
ITEM rack::attack:1497913610:requests/1in1seconds:user_1 [4 b; 1497913600 s]
ITEM rack::attack:24965226:requests/60in60seconds:user_1 [4 b; 1497913609 s]
ITEM 1 [53 b; 0 s]
END

Is there something obvious I am missing which would make the throttles not be cleared properly? I'm using version 5.0.1, by the way.

@brandoncc
Copy link

I found my problem. get_filter_identifier if throttle needed to be get_filter_identifier[req] if throttle. That didn't resolve the code not working for me though. I get an error from dalli saying that it can't increment a non-number after I use it. In order to fix that, I changed the write call to store.delete(key) and it now works on memcached.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants