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

Instrument fail2ban #655

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
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
29 changes: 29 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,35 @@ end

Note that `Fail2Ban` filters are not automatically scoped to the blocklist, so when using multiple filters in an application the scoping must be added to the discriminator e.g. `"pentest:#{req.ip}"`.

Instrumentation of fail2ban requires passing the request to the filter, and
subscribing to `ban.rack_attack`:

```ruby
Rack::Attack.blocklist('fail2ban') do |req|
# same as above, plus passing in the request
Rack::Attack::Fail2Ban.filter(..., request: req) { ... }
end

# Then you can subscribe to the ban event
ActiveSupport::Notifications.subscribe("ban.rack_attack") do |name, start, finish, request_id, payload|
# Request object available in payload[:request].
#
# For ease of use, the payload echoes the fail2ban settings that triggered
# the ban:
#
# payload[:request].env['rack.attack.match_data'] #=> {
# name: "fail2ban",
# discriminator: "1.2.3.4",
# count: 2,
# maxretry: 5,
# findtime: 60,
# bantime: 3600
# }

# Your code here
end
```

#### Allow2Ban

`Allow2Ban.filter` works the same way as the `Fail2Ban.filter` except that it *allows* requests from misbehaving
Expand Down
8 changes: 3 additions & 5 deletions lib/rack/attack/allow2ban.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ def key_prefix

# everything is the same here except we only return true
# (blocking the request) if they have tripped the limit.
def fail!(discriminator, bantime, findtime, maxretry)
count = cache.count("#{key_prefix}:count:#{discriminator}", findtime)
if count >= maxretry
ban!(discriminator, bantime)
end
def fail!(discriminator, bantime, findtime, maxretry, request)
super

# we may not block them this time, but they're banned for next time
false
end
Expand Down
7 changes: 5 additions & 2 deletions lib/rack/attack/check.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@ def initialize(name, options = {}, &block)
def matched_by?(request)
block.call(request).tap do |match|
if match
request.env["rack.attack.matched"] = name
request.env["rack.attack.match_type"] = type
# Can already be set if the match just resulted in a ban,
# in which case we want to notify about the ban, not the
# blocklist check
request.env["rack.attack.matched"] ||= name
request.env["rack.attack.match_type"] ||= type
Comment on lines +20 to +21
Copy link
Collaborator

Choose a reason for hiding this comment

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

note to self: this can be considered a breaking change since it alters the way rack-attack currently notifies about blocklist.rack_attack

Copy link
Author

Choose a reason for hiding this comment

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

I can look at this more closely tomorrow, but the reason for this change was that, without this, the request was first getting a match_type of :ban set in the environment as it was banned, then as the request was blocklisted, it was overridden here as :blocklist. I believe the blocklist should work the same as it did previously, definitely if the developer hasn't passed in the request (an awkward requirement for this to work), maybe in general. I could look at the test suite with this in mind if it's a concern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just had some time to come back to this.

I opened this PR #656 with my concern about this. This gem is so widely used that I wouldn't like to change existing behavior (unless it's in a new major version). I'm wondering if we can keep the existing events + adding the new one :ban that you are introducing. Maybe we can achieve that by not modifying the request.env in https://github.com/rack/rack-attack/pull/655/files#diff-aa6124b2b316bd2edb9f054a2a3200ab0c12248c79f4bc30da5195122c949363R73 and instead passing another object to Rack::Attack.instrument ?

Rack::Attack.instrument(request)
end
end
Expand Down
25 changes: 23 additions & 2 deletions lib/rack/attack/fail2ban.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@ def filter(discriminator, options)
bantime = options[:bantime] or raise ArgumentError, "Must pass bantime option"
findtime = options[:findtime] or raise ArgumentError, "Must pass findtime option"
maxretry = options[:maxretry] or raise ArgumentError, "Must pass maxretry option"
request = options[:request]

if banned?(discriminator)
# Return true for blocklist
true
elsif yield
fail!(discriminator, bantime, findtime, maxretry)
fail!(discriminator, bantime, findtime, maxretry, request)
end
end

Expand All @@ -34,10 +35,23 @@ def key_prefix
'fail2ban'
end

def fail!(discriminator, bantime, findtime, maxretry)
def fail!(discriminator, bantime, findtime, maxretry, request)
count = cache.count("#{key_prefix}:count:#{discriminator}", findtime)
if count >= maxretry
ban!(discriminator, bantime)

if request # must be passed in just for instrumentation
annotate_request_with_matched_data(
request,
name: key_prefix,
discriminator: discriminator,
count: count,
maxretry: maxretry,
findtime: findtime,
bantime: bantime
)
Rack::Attack.instrument(request)
end
end

true
Expand All @@ -52,6 +66,13 @@ def ban!(discriminator, bantime)
def cache
Rack::Attack.cache
end

def annotate_request_with_matched_data(request, data)
request.env['rack.attack.matched'] = data[:name]
request.env['rack.attack.match_discriminator'] = data[:discriminator]
request.env['rack.attack.match_type'] = :ban
request.env['rack.attack.match_data'] = data
end
end
end
end
Expand Down
38 changes: 37 additions & 1 deletion spec/fail2ban_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@
@f2b_options = { bantime: @bantime, findtime: @findtime, maxretry: 2 }

Rack::Attack.blocklist('pentest') do |req|
Rack::Attack::Fail2Ban.filter(req.ip, @f2b_options) { req.query_string =~ /OMGHAX/ }
Rack::Attack::Fail2Ban.filter(req.ip, @f2b_options.merge(request: req)) do
req.query_string =~ /OMGHAX/
end
end

ActiveSupport::Notifications.subscribe("ban.rack_attack") do |name, start, finish, id, payload|
@notification = { name: name, start: start, finish: finish, id: id, payload: payload }
end
end

Expand Down Expand Up @@ -41,6 +47,10 @@
key = "rack::attack:fail2ban:1.2.3.4"
_(@cache.store.read(key)).must_be_nil
end

it 'does not notify' do
_(@notification).must_be_nil
end
end

describe 'when at maxretry' do
Expand All @@ -63,6 +73,22 @@
key = "rack::attack:fail2ban:ban:1.2.3.4"
_(@cache.store.read(key)).must_equal 1
end

it "notifies" do
_(@notification).wont_be_nil

_(@notification[:payload][:request].env['rack.attack.match_type'])\
.must_equal(:ban)
_(@notification[:payload][:request].env['rack.attack.match_data'])\
.must_equal(
name: "fail2ban",
discriminator: "1.2.3.4",
count: 2,
maxretry: 2,
findtime: 60,
bantime: 60
)
end
end

describe 'reset after success' do
Expand Down Expand Up @@ -105,6 +131,7 @@

describe 'making ok request' do
before do
@notification = nil
get '/', {}, 'REMOTE_ADDR' => '1.2.3.4'
end

Expand All @@ -121,10 +148,15 @@
key = "rack::attack:fail2ban:ban:1.2.3.4"
_(@cache.store.read(key)).must_equal 1
end

it 'does not notify' do
_(@notification).must_be_nil
end
end

describe 'making failing request' do
before do
@notification = nil
get '/?foo=OMGHAX', {}, 'REMOTE_ADDR' => '1.2.3.4'
end

Expand All @@ -141,6 +173,10 @@
key = "rack::attack:fail2ban:ban:1.2.3.4"
_(@cache.store.read(key)).must_equal 1
end

it 'does not notify' do
_(@notification).must_be_nil
end
end
end
end