Support RedisCacheStore (new in Rails 5.2) #281
Comments
|
Hi Brian, I am trying to understand exactly where this issue is coming from. Can you share what your rack-attack configuration looks like after upgrading to rails 5.2? Thank you! |
|
Unfortunately, I don't have a stack trace handy as my error logger dumps them after 30 days. Basically, none of the store proxies match an The The
Probably the best way to understand this is to look at the I hope that clears it up a bit. Let me know if you have any other questions. |
|
Sorry, forgot to include the config: class Rack::Attack
if Rails.env.production?
### Configure Cache ###
# If you don't want to use Rails.cache (Rack::Attack's default), then
# configure it here.
#
# Note: The store is only used for throttling (not blacklisting and
# whitelisting). It must implement .increment and .write like
# ActiveSupport::Cache::Store
# Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
### Throttle Spammy Clients ###
# If any single client IP is making tons of requests, then they're
# probably malicious or a poorly-configured scraper. Either way, they
# don't deserve to hog all of the app server's CPU. Cut them off!
#
# Note: If you're serving assets through rack, those requests may be
# counted by rack-attack and this throttle may be activated too
# quickly. If so, enable the condition to exclude them from tracking.
# Throttle all requests by IP (60rpm)
#
# Key: "rack::attack:#{Time.now.to_i/:period}:req/ip:#{req.ip}"
throttle('req/ip', :limit => 300, :period => 2.minutes) do |req|
req.ip # unless req.path.start_with?('/assets')
end
### Prevent Brute-Force Login Attacks ###
# The most common brute-force login attack is a brute-force password
# attack where an attacker simply tries a large number of emails and
# passwords to see if any credentials match.
#
# Another common method of attack is to use a swarm of computers with
# different IPs to try brute-forcing a password for a specific account.
# Throttle POST requests to /login by IP address
#
# Key: "rack::attack:#{Time.now.to_i/:period}:logins/ip:#{req.ip}"
throttle('logins/ip', :limit => 5, :period => 20.seconds) do |req|
if req.path == '/login' && req.post?
req.ip
end
end
# Throttle POST requests to /login by email param
#
# Key: "rack::attack:#{Time.now.to_i/:period}:logins/email:#{req.email}"
#
# Note: This creates a problem where a malicious user could intentionally
# throttle logins for another user and force their login requests to be
# denied, but that's not very common and shouldn't happen to you. (Knock
# on wood!)
throttle("logins/email", :limit => 5, :period => 20.seconds) do |req|
if req.path == '/login' && req.post?
# return the email if present, nil otherwise
req.params['email'].presence
end
end
### Custom Throttle Response ###
# By default, Rack::Attack returns an HTTP 429 for throttled responses,
# which is just fine.
#
# If you want to return 503 so that the attacker might be fooled into
# believing that they've successfully broken your app (or you just want to
# customize the response), then uncomment these lines.
# self.throttled_response = lambda do |env|
# [ 503, # status
# {}, # headers
# ['']] # body
# end
# Block requests bad URLs
# Rack::Attack.blocklist('block URLs') do |req|
# req.path.match /(wp-*|php|\.asp|cgi-bin|\/feed\/|wordpress|hubfs)/
# end
end
end |
|
Hi Brian, Thanks for sharing those details! I do understand the issue with |
|
I added acceptance test cases that use Can you think of a test case that is able to reproduce it? |
|
I just looked at the new tests. It seems like they should have caught the problem if ActiveSupport 5.2 was installed. The new cache store suppresses Redis errors by default, so maybe we need to make them noisy in the test. Something like: NOISY_ERROR_HANDLER = ->(method:, returning:, exception:) { Raise "ERROR" }
Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(error_handler: NOISY_ERROR_HANDLER)That still doesn't explain the correct throttling response in the test, though. I wonder if this has something to do with the gem dependencies. The new cache store doesn't depend on the redis-store or redis-activesupport gems. They're not in my Gemfile.lock. When I fire up a Rails console and run Maybe the presence of these gems in the testing environment causes the existing RedisStoreProxy to work with the new store without adding the new proxy. As far as reproducing it, that should be possible in production by setting up any Rails 5.2 app with the new store. In a testing environment, though, I'll have to give it some thought. |
|
Ok, I made a test app and reproduced the error. Stack trace: |
|
Strangely, I can't reproduce the error on command. It's an occasional thing. The stack trace above was part of a series of consecutive errors. Before and after the series of errors, the app worked fine with the same request. This is with a brand new Rails app, just showing the "Yay! You're on Rails!" page. |
|
Test app is a new Rails 5.2 app using Ruby 2.5.1. Other details:
class Rack::Attack
throttle('req/ip', :limit => 60, :period => 2.minutes) do |req|
req.ip # unless req.path.start_with?('/assets')
end
throttle('logins/ip', :limit => 5, :period => 20.seconds) do |req|
if req.path == '/login' && req.post?
req.ip
end
end
throttle("logins/email", :limit => 5, :period => 20.seconds) do |req|
if req.path == '/login' && req.post?
# return the email if present, nil otherwise
req.params['email'].presence
end
end
end
NOISY_ERROR_HANDLER = ->(method:, returning:, exception:) { raise exception }
config.action_controller.perform_caching = true
config.cache_store = :redis_cache_store, { error_handler: NOISY_ERROR_HANDLER }Please note that the original error report was from my production environment, so this has been observed in both environments. |
|
Ok, so I think I fixed the wrong thing in my pull request. My errors didn't go away because of the new cache store proxy, they went away because after I disabled rack-attack, I had some different Redis errors that I solved by setting I tried using my PR branch in the test app. The errors were different, but I still got errors in the same situation as when I used the current version. With either version of the gem, the errors went away when I set the cache config as described above. This explains the intermittent behavior of the errors in production, because they were the result of Redis disconnecting momentarily. They happened to resemble the errors caused by other means, so my search led me to bad conclusions. It might be worth documenting this setting for users of RedisCacheStore, but it doesn't seem like there's anything to fix in rack-attack itself. Sorry for the wild goose chase. I'll close the PR. Like I mentioned, the errors were different when using the store proxy from my PR. Here's the stack trace, if you're still interested in it for any reason: I'll leave this issue open until I hear whether you think |
Cool
I am not sure about that. Feels like something out of the scope of rack-attack to me. It's up to users to set their preferred redis configuration.
No worries. I think this discussion ended up being helpful. We now know rack-attack supports rails 5.2 |
|
Having the same issue here. Same stack. Full stack trace: |
|
When you look at redis, seems like the value being stored is indeed not an integer. So something is indeed dropping the wrong value. |
|
Did you read this comment above? In my case, I discovered the problem went away when I added I never did figure out why that seems to work, though. All my reasoning says there's a problem in this gem, but the acceptance tests and my experience using it says it works fine as is. 🤷♂️ Do you have a reliable repro? |
|
Thanks @brian-kephart . |
|
Hi all, I have just been experiencing this too. I checked that the redis cache config does not specify the reconnect attempts and as per the docs http://edgeguides.rubyonrails.org/caching_with_rails.html#activesupport-cache-rediscachestore the default should just be 0. It is intermittent and as per other posts clears after some time. (Rails 5.2, Ruby 2.5.1, on Heroku using Heroku Redis) |
|
Hi @stevegeek, Can you please provide the following info to help debug your issue?
Thank you! :-) |
|
@grzuy Sure thing!
Rack attack initializer module Rack
class Attack
if Rails.env.production?
throttle("csp/ip", limit: 3000, period: 5.minutes) do |req|
req.ip if req.path.start_with?("/csp-violation-report-endpoint")
end
throttle("req/ip", limit: 6000, period: 5.minutes) do |req|
req.ip unless req.path.start_with?("/csp-violation-report-endpoint")
end
throttle("logins/ip", limit: 5, period: 20.seconds) do |req|
req.ip if req.path == "/accounts/sign_in" && req.post?
end
end
end
endRails config: |
Let me know if it doesn't. |
|
@grzuy Ah ok thanks, sorry! missed that it was fixed |
`ActiveSupport::Cache::RedisCacheStore` is not compatible with the version of Rack Attack we are using (v4.4.1) per rack/rack-attack#281. Users that had rate limits enabled might see `Redis::CommandError: ERR value is not an integer or out of range` because the `raw` parameter wasn't passed along properly. Let's partially revert the change to the original cache store until we can update to Rack Attack v5.2.3 that has support for `ActiveSupport::Cache::RedisCacheStore` via rack/rack-attack#340. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/66449
`ActiveSupport::Cache::RedisCacheStore` is not compatible with the version of Rack Attack we are using (v4.4.1) per rack/rack-attack#281. Users that had rate limits enabled might see `Redis::CommandError: ERR value is not an integer or out of range` because the `raw` parameter wasn't passed along properly. As a result, the Rack Attack entry would be stored as an `ActiveSupport::Cache::Entry` instead of a raw string holding an integer value. Let's partially revert the change in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30966 to use the original cache store until we can update to Rack Attack v5.2.3 that has support for `ActiveSupport::Cache::RedisCacheStore` via rack/rack-attack#350. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/66449
`ActiveSupport::Cache::RedisCacheStore` is not compatible with the version of Rack Attack we are using (v4.4.1) per rack/rack-attack#281. Users that had rate limits enabled might see `Redis::CommandError: ERR value is not an integer or out of range` because the `raw` parameter wasn't passed along properly. As a result, the Rack Attack entry would be stored as an `ActiveSupport::Cache::Entry` instead of a raw string holding an integer value. Let's partially revert the change in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30966 to use the original cache store until we can update to Rack Attack v5.2.3 that has support for `ActiveSupport::Cache::RedisCacheStore` via rack/rack-attack#350. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/66449
|
Seeing this error on the latest version of the gem. |
|
Figured out our issue. Posting for anyone who has a similar setup. class CustomRedisCacheStore < ActiveSupport::Cache::RedisCacheStoreSince we do that, it bypasses the custom proxy instantiation done via https://github.com/kickstarter/rack-attack/blob/da418806638d0556fe15f20cea20194d4bbb85b3/lib/rack/attack/store_proxy.rb#L5-L18 Because of that, we missed the raw write that this offers Fixed it with a monkey patch class StoreProxy::RedisCacheStoreProxy
def self.handle?(store)
store.class.name == CustomRedisCacheStore.name
end
end |
After upgrading my app to Rails 5.2.0rc1, I'm occasionally getting the following error:
Redis::CommandError: ERR value is not an integer or out of rangeGoogle says this error is caused by not passing the
rawoption when writing to Redis, then later incrementing the key. This appears to be fixed in this library for other forms of Redis caching, but not for the new ActiveSupport::Cache::RedisCacheStore.Previous (working) config:
Ruby 2.5.0
Rails 5.1.4
ReadThisStore
Hiredis (driver)
Current (non-working) config:
Ruby 2.5.0
Rails 5.2.0rc1
RedisCacheStore
Hiredis (driver)
The text was updated successfully, but these errors were encountered: