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

System Hardening Phase 1: Catch errors at top level #639

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ See the [Backing & Hacking blog post](https://www.kickstarter.com/backing-and-ha
- [Customizing responses](#customizing-responses)
- [RateLimit headers for well-behaved clients](#ratelimit-headers-for-well-behaved-clients)
- [Logging & Instrumentation](#logging--instrumentation)
- [Custom Error Handling](#custom-error-handling)
- [Testing](#testing)
- [How it works](#how-it-works)
- [About Tracks](#about-tracks)
Expand Down Expand Up @@ -400,11 +401,21 @@ ActiveSupport::Notifications.subscribe(/rack_attack/) do |name, start, finish, r
end
```

## Custom Error Handling

You may specify the list of internal errors to allow (i.e. not raise an error)
as either an array of Class and/or String values.

```ruby
# in initializers/rack_attack.rb
Rack::Attack.allowed_errors += [MyErrorClass, 'MyOtherErrorClass']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strong, thoughts on naming ignored_errors instead allowed_errors? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grzuy the reason for "allowed" here is that when these errors occur, RackAttack "allows" the request. This naming will be made more obvious in the next PR, which allows you to add error handlers which throttle or block based on certain errors.

Copy link
Collaborator

@santib santib Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to better understand the need to make this configurable, apart from Redis::BaseConnectionError/Redis::BaseError, and Dalli::DalliError which other errors could potentially need to be handled? What was the exception that caused your issue in production?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some reasons:

  • If you have some complex logic inside your Rack::Attack handlers that raises errors.
  • If you implemented your own store proxy (non-Redis/Dalli)

That being said I'd estimate 98%+ of users are fine with the default Redis/Dalli error handling.

```

## Testing

A note on developing and testing apps using Rack::Attack - if you are using throttling in particular, you will
need to enable the cache in your development environment. See [Caching with Rails](http://guides.rubyonrails.org/caching_with_rails.html)
for more on how to do this.
When developing and testing apps using Rack::Attack, if you are using throttling in particular,
you must enable the cache in your development environment. See
[Caching with Rails](http://guides.rubyonrails.org/caching_with_rails.html) for how to do this.

### Disabling

Expand Down
22 changes: 19 additions & 3 deletions lib/rack/attack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ class IncompatibleStoreError < Error; end
autoload :Allow2Ban, 'rack/attack/allow2ban'

class << self
attr_accessor :enabled, :notifier, :throttle_discriminator_normalizer
attr_accessor :enabled,
:notifier,
:throttle_discriminator_normalizer

attr_reader :configuration

def instrument(request)
Expand All @@ -59,6 +62,15 @@ def reset!
cache.reset!
end

def allow_error?(error)
allowed_errors&.any? do |allowed_error|
case allowed_error
when String then error.class.ancestors.any? {|a| a.name == allowed_error }
else error.is_a?(allowed_error)
end
end
end

extend Forwardable
def_delegators(
:@configuration,
Expand All @@ -82,11 +94,13 @@ def reset!
:safelists,
:blocklists,
:throttles,
:tracks
:tracks,
:allowed_errors,
:allowed_errors=
)
end

# Set defaults
# Set instance defaults
@enabled = true
@notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications)
@throttle_discriminator_normalizer = lambda do |discriminator|
Expand Down Expand Up @@ -128,6 +142,8 @@ def call(env)
configuration.tracked?(request)
@app.call(env)
end
rescue StandardError => error
self.class.allow_error?(error) ? @app.call(request.env) : raise(error)
Comment on lines +145 to +146
Copy link
Collaborator

@santib santib Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, here we are rescuing StandardError so we can allow strings in the allowed_errors configuration, right? Is there any reason why to not do this?

rescue *self.allowed_errors => error
  @app.call(request.env)
end

because I prefer the simplicity of this alternative if possible

Copy link
Author

@johnnyshields johnnyshields Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way so we can define the allowed_errors as strings, and therefore not have a dependency on Dalli or Redis constants being loaded. Note the allow_error? method is as follows:

      def allow_error?(error)
        allowed_errors&.any? do |allowed_error|
          case allowed_error
          when String then error.class.ancestors.any? {|a| a.name == allowed_error }
          else error.is_a?(allowed_error)
          end
        end
      end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not have a dependency on Dalli or Redis constants being loaded

Agree on this.

Not really sold on the allow_error? method, I'd prefer to use Ruby's exception handling instead, while finding other way to solve the need to not load Dalli/Redis constants.

I know you didn't like it before, but one way could be the Rack::Attack::CacheError class, but there could be others, for example having this class wrap and re-raise the original error, maybe? Or maybe having the defaults defined by each proxy class which also checks for defined?(Dalli) as they already do in other places?

I think this point is really important and we need to take the time to brainstorm/discuss it properly.

Copy link
Author

@johnnyshields johnnyshields Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santib In order to invoke rescue *allowed_errors, the allowed_errors variable must contain constants, not strings. We could (could != should) do something like:

def allowed_errors_as_consts
  @cached_error_classes ||= {}
  allowed_errors.map do |err|
    if err.is_a?(String)
      @cached_error_classes[err] ||= Object.const_get(err) rescue NameError
    else
      err
    end
  end.compact
end

This would need to be done dynamically because Dalli/Redis could be loaded at anytime even after we started using Rack::Attack (and allowed_errors could also be modified.)

This is a lot of silly overhead just to achieve the rescue *list line. The code I have is reasonably fast (string comparison of class names) and low-complexity, so lets go with it for now and not let "perfect" become the enemy of "good enough." We can consider further once the full series of PRs have been merged.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to invoke rescue *allowed_errors, the allowed_errors variable must contain constants, not strings.

I know that. But why do we need to also allow Strings? If supporting only constants makes the code simpler, then that works for me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, there is precedent in other Ruby libs to reference potentially unloaded classes with strings, such as Rails where you can see this:

belongs_to :manager, class_name: "Employee"

Copy link
Author

@johnnyshields johnnyshields Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last thing: this is a moot point. Please see these line in the intended final state of this series of PRs:

https://github.com/rack/rack-attack/pull/577/files#diff-9114718ab0fa2d856234c04d73b07c3758a6aaab1dbfbe87401f4eb4e358f127R218-R225

Note I'm adding an additional error_handler here.

Copy link
Author

@johnnyshields johnnyshields Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rack::Attack::CacheError

I would really prefer that we do not obscure the errors coming from Redis/Dalli itself. It makes it harder to understand the reasons why Redis/Dalli are failing.

Copy link
Collaborator

@santib santib Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rack::Attack::CacheError

I would really prefer that we do not obscure the errors coming from Redis/Dalli itself. It makes it harder to understand the reasons why Redis/Dalli are failing.

it could be just a wrapper (as I said in option 2.), and then pass the original error to the application. Having that wrapper would solve the constant issue we are discussing here.

Copy link
Author

@johnnyshields johnnyshields Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's defer this discussion until I get my other PRs merged. There's no reason to bikeshed and introduce complexity at this point.

end
end
end
19 changes: 16 additions & 3 deletions lib/rack/attack/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
module Rack
class Attack
class Configuration
DEFAULT_ALLOWED_ERRORS = %w[Dalli::DalliError Redis::BaseError].freeze

DEFAULT_BLOCKLISTED_RESPONDER = lambda { |_req| [403, { 'content-type' => 'text/plain' }, ["Forbidden\n"]] }

DEFAULT_THROTTLED_RESPONDER = lambda do |req|
Expand All @@ -19,10 +21,20 @@ class Configuration
end
end

attr_reader :safelists, :blocklists, :throttles, :anonymous_blocklists, :anonymous_safelists
attr_accessor :blocklisted_responder, :throttled_responder, :throttled_response_retry_after_header
attr_reader :safelists,
:blocklists,
:throttles,
:anonymous_blocklists,
:anonymous_safelists

attr_accessor :allowed_errors,
:blocklisted_responder,
:throttled_responder,
:throttled_response_retry_after_header

attr_reader :blocklisted_response, :throttled_response # Keeping these for backwards compatibility
# Keeping these for backwards compatibility
attr_reader :blocklisted_response,
:throttled_response

def blocklisted_response=(responder)
warn "[DEPRECATION] Rack::Attack.blocklisted_response is deprecated. "\
Expand Down Expand Up @@ -116,6 +128,7 @@ def set_defaults
@anonymous_blocklists = []
@anonymous_safelists = []
@throttled_response_retry_after_header = false
@allowed_errors = DEFAULT_ALLOWED_ERRORS.dup

@blocklisted_responder = DEFAULT_BLOCKLISTED_RESPONDER
@throttled_responder = DEFAULT_THROTTLED_RESPONDER
Expand Down
30 changes: 8 additions & 22 deletions lib/rack/attack/store_proxy/dalli_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,34 +24,26 @@ def initialize(client)
end

def read(key)
rescuing do
with do |client|
client.get(key)
end
with do |client|
client.get(key)
end
end

def write(key, value, options = {})
rescuing do
with do |client|
client.set(key, value, options.fetch(:expires_in, 0), raw: true)
end
with do |client|
client.set(key, value, options.fetch(:expires_in, 0), raw: true)
end
end

def increment(key, amount, options = {})
rescuing do
with do |client|
client.incr(key, amount, options.fetch(:expires_in, 0), amount)
end
with do |client|
client.incr(key, amount, options.fetch(:expires_in, 0), amount)
end
end

def delete(key)
rescuing do
with do |client|
client.delete(key)
end
with do |client|
client.delete(key)
end
end

Expand All @@ -66,12 +58,6 @@ def with
end
end
end

def rescuing
yield
rescue Dalli::DalliError
nil
end
end
end
end
Expand Down
38 changes: 13 additions & 25 deletions lib/rack/attack/store_proxy/redis_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,50 +19,38 @@ def self.handle?(store)
end

def read(key)
rescuing { get(key) }
get(key)
end

def write(key, value, options = {})
if (expires_in = options[:expires_in])
rescuing { setex(key, expires_in, value) }
setex(key, expires_in, value)
else
rescuing { set(key, value) }
set(key, value)
end
end

def increment(key, amount, options = {})
rescuing do
pipelined do |redis|
redis.incrby(key, amount)
redis.expire(key, options[:expires_in]) if options[:expires_in]
end.first
end
pipelined do |redis|
redis.incrby(key, amount)
redis.expire(key, options[:expires_in]) if options[:expires_in]
end.first
end

def delete(key, _options = {})
rescuing { del(key) }
del(key)
end

def delete_matched(matcher, _options = nil)
cursor = "0"

rescuing do
# Fetch keys in batches using SCAN to avoid blocking the Redis server.
loop do
cursor, keys = scan(cursor, match: matcher, count: 1000)
del(*keys) unless keys.empty?
break if cursor == "0"
end
# Fetch keys in batches using SCAN to avoid blocking the Redis server.
loop do
cursor, keys = scan(cursor, match: matcher, count: 1000)
del(*keys) unless keys.empty?
break if cursor == "0"
end
end

private

def rescuing
yield
rescue Redis::BaseConnectionError
nil
end
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions lib/rack/attack/store_proxy/redis_store_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ def self.handle?(store)
end

def read(key)
rescuing { get(key, raw: true) }
get(key, raw: true)
end

def write(key, value, options = {})
if (expires_in = options[:expires_in])
rescuing { setex(key, expires_in, value, raw: true) }
setex(key, expires_in, value, raw: true)
else
rescuing { set(key, value, raw: true) }
set(key, value, raw: true)
end
end
end
Expand Down
1 change: 1 addition & 0 deletions rack-attack.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ Gem::Specification.new do |s|
s.add_development_dependency "bundler", ">= 1.17", "< 3.0"
s.add_development_dependency 'minitest', "~> 5.11"
s.add_development_dependency "minitest-stub-const", "~> 0.6"
s.add_development_dependency 'rspec-mocks', '~> 3.11.0'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the in more detail but would be great if we wouldn't need to add extra dep.

Copy link
Author

@johnnyshields johnnyshields Nov 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this PR need some form of mocking in our tests, and RSpec's mock library is widely-used and has minitest integration.

In the tests you can see I am stubbing certain methods to raise errors. There is no other way to do this in the specs b/c these error would only occur if certain infra such as Redis is down. The only alternative would be to write monkey patches in the tests, which is far worse/messy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your point, but maybe we can change where this gem is used for something similar to how exceptions are currently being forced in tests, and we avoid introducing a new dependency? Or are there other places where rspec-mocks is needed?

Copy link
Author

@johnnyshields johnnyshields Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read my spec/acceptance/error_handling_spec.rb. I am using mocks to trigger various errors which do not normally occur and asserting how they are handled. rspec-mocks was designed exactly for this sort of use case (e.g. testing how novel errors propagate through a system.)

It is certainly possible to force certain errors by strange Redis configurations, however this ultimately is limited in what types of errors we can trigger. I will be using mocks further in subsequent PRs.

s.add_development_dependency 'rack-test', "~> 2.0"
s.add_development_dependency 'rake', "~> 13.0"
s.add_development_dependency "rubocop", "1.12.1"
Expand Down
93 changes: 93 additions & 0 deletions spec/acceptance/error_handling_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# frozen_string_literal: true

require_relative "../spec_helper"

describe "error handling" do

let(:store) do
ActiveSupport::Cache::MemoryStore.new
end

before do
Rack::Attack.cache.store = store

Rack::Attack.blocklist("fail2ban pentesters") do |request|
Rack::Attack::Fail2Ban.filter(request.ip, maxretry: 0, bantime: 600, findtime: 30) { true }
end
end

describe '.allowed_errors' do
before do
allow(store).to receive(:read).and_raise(RuntimeError)
end

it 'has default value' do
assert_equal Rack::Attack.allowed_errors, %w[Dalli::DalliError Redis::BaseError]
end

it 'can get and set value' do
Rack::Attack.allowed_errors = %w[Foobar]
assert_equal Rack::Attack.allowed_errors, %w[Foobar]
end

it 'can ignore error as Class' do
Rack::Attack.allowed_errors = [RuntimeError]

get "/", {}, "REMOTE_ADDR" => "1.2.3.4"

assert_equal 200, last_response.status
end

it 'can ignore error ancestor as Class' do
Rack::Attack.allowed_errors = [StandardError]

get "/", {}, "REMOTE_ADDR" => "1.2.3.4"

assert_equal 200, last_response.status
end

it 'can ignore error as String' do
Rack::Attack.allowed_errors = %w[RuntimeError]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just go with ["RuntimeError"] so it's more obvious what type it is?

Copy link
Author

@johnnyshields johnnyshields Nov 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Rubocop style guide (according to default settings) says a one-element array can be either ['RuntimeError'] or %w[RuntimeError], but a two-element array must use %w[] e.g. %w[RuntimeError OtherError]

Since I'm using both 1 and 2 element arrays this test (as well as elsewhere), I think it is better to use %w[ ] consistently everywhere. Please confirm whether you'd like me to (a) keep as-is, (b) change only one-element arrays, (c) change both 1 and 2-element arrays.

See: https://docs.rubocop.org/rubocop/cops_style.html#stylewordarray


get "/", {}, "REMOTE_ADDR" => "1.2.3.4"

assert_equal 200, last_response.status
end

it 'can ignore error error ancestor as String' do
Rack::Attack.allowed_errors = %w[StandardError]

get "/", {}, "REMOTE_ADDR" => "1.2.3.4"

assert_equal 200, last_response.status
end

it 'raises error if not ignored' do
assert_raises(RuntimeError) do
get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
end
end
end

describe '.allowed_errors?' do

it 'can match String or Class' do
Rack::Attack.allowed_errors = ['ArgumentError', RuntimeError]
assert Rack::Attack.allow_error?(ArgumentError.new)
assert Rack::Attack.allow_error?(RuntimeError.new)
refute Rack::Attack.allow_error?(StandardError.new)
end

it 'can match Class ancestors' do
Rack::Attack.allowed_errors = [StandardError]
assert Rack::Attack.allow_error?(ArgumentError.new)
refute Rack::Attack.allow_error?(Exception.new)
end

it 'can match String ancestors' do
Rack::Attack.allowed_errors = ['StandardError']
assert Rack::Attack.allow_error?(ArgumentError.new)
refute Rack::Attack.allow_error?(Exception.new)
end
end
end
3 changes: 2 additions & 1 deletion spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require "bundler/setup"

require "minitest/autorun"
require "minitest/pride"
require "rspec/mocks/minitest_integration"
require "rack/test"
require "active_support"
require "rack/attack"
Expand Down Expand Up @@ -35,6 +35,7 @@ class Minitest::Spec
after do
Rack::Attack.clear_configuration
Rack::Attack.instance_variable_set(:@cache, nil)
Rack::Attack.allowed_errors = Rack::Attack::Configuration::DEFAULT_ALLOWED_ERRORS.dup
end

def app
Expand Down