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

Make it possible to block requests based on response. #646

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NOX73
Copy link

@NOX73 NOX73 commented Jan 6, 2024

I want to use rack-attack to block pentesters based on 4xx requests. It's not possible with the current gem API.

So here is a POC how it might look like:

    Rack::Attack.postrequest("fail2ban for 404") do |request, response|
      Rack::Attack::Fail2Ban.filter(request.ip, maxretry: 2, findtime: 30, bantime: 60) do
        if response.nil?
          false
        else
          response[0] == 404
        end
      end
    end

It might look clumsy, but you have to check if a response exists with such API.

So, I'm ready to add more specs and finish this to get it ready to merge if you think this is a good idea.

Naming is also debatable.

@seliverstov-maxim
Copy link

1 similar comment
@senzpo
Copy link

senzpo commented Jan 10, 2024

assert_equal 200, last_response.status
end


Choose a reason for hiding this comment

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

spec/acceptance/postrequest_spec.rb:29:1: C: [Correctable] Layout/EmptyLines: Extra blank line detected.
spec/acceptance/postrequest_spec.rb:51:1: C: [Correctable] Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.

68 files inspected, 2 offenses detected, 2 offenses auto-correctable
RuboCop failed!

@type = :postrequest
end

def matched_by?(request, response)
Copy link

Choose a reason for hiding this comment

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

It seems that this method calling twice:

  • the first time before app.call
  • the second time after app.call

I guess that we can accidentally mark a request as matched even when it doesn't blocked.

Choose a reason for hiding this comment

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

mb change postrequest.matched_by? to postrequest.block.call?

#lib/rack/attack/configuration.rb

def process_postrequests(request, response)
    @postrequests.each { |_name, postrequest| postrequest.matched_by?(request, response) }
end

@seliverstov-maxim
Copy link

@santib The solution is feet to the gem philosophy? Is there any chance the PR to be accepted? I would like to have the opportunity to use this feature in my projects.

@santib
Copy link
Collaborator

santib commented Jan 16, 2024

Hi @NOX73 and @seliverstov-maxim 👋 First of all, thanks for your contribution.

I've been thinking about this PR for some days already, but didn't comment because I haven't formed an opinion just yet.

You said that the reason to add this feature is because

I want to use rack-attack to block pentesters based on 4xx requests

Would you be able to expand it a bit more? Like why do you want to do it only based on 4xx? Why wouldn't throttling or blocklist+fail2ban be enough for your use case?

@NOX73
Copy link
Author

NOX73 commented Jan 17, 2024

Would you be able to expand it a bit more? Like why do you want to do it only based on 4xx? Why wouldn't throttling or blocklist+fail2ban be enough for your use case?

Sure, no problem. Some scanning programs scan some URLs, but there are some clients who might put same load, like the same requests per time. The only different we see - it's the response. In the scanning case it a lot of 404 errors. So would be very helpful to ban them based on what status code the request ended with.

@santib
Copy link
Collaborator

santib commented Jan 22, 2024

@NOX73 Honestly, I'm not sold on adding this feature to rack-attack. I'm wondering, would it be possible to add this feature as a separate gem? I mean something like a rack-attack extension (e.g. rack-attack-postrequest). If not, what would be needed to make it work? Also, would it make sense to hook into rack.response_finished for this postprocess stuff?

@NOX73
Copy link
Author

NOX73 commented Jan 23, 2024

@santib Do you think it does not fit the gem idea, or is it our corner case?

Sure I think it might be a separate gem, why not. I just was surprised there is no way to implement such common (in my opinion) functionality.

@seliverstov-maxim
Copy link

seliverstov-maxim commented Jan 23, 2024

I mean something like a rack-attack extension (e.g. rack-attack-postrequest)

Sounds good to me

@johnnyshields
Copy link

This seems totally reasonable to include in the RackAttack gem itself. I don't like the idea of having to make this a separate gem.

@JulienItard
Copy link

JulienItard commented Mar 20, 2024

@seliverstov-maxim @NOX73 Hello, i have more or less same use case. We have a public API and some of our customers use our API in wrong way, so we returns errors. I would like to add throlling when there is too many errors in a period. Like "error limits exceeded".

@seliverstov-maxim
Copy link

I investigated the point "I mean something like a rack-attack extension (e.g. rack-attack-postrequest)"
Imho it's impossible to do this, because @NOX73 changes interface of core classes. Support core classes in this gem and in extension looks like overhead.
One way to do this is big refactoring.

@alexsmartens
Copy link


damn, this would be sick! Any progress?

Any suggestions for a temporary workaround? I was thinking about caching response codes and basing the throttle/blocking logic based on those cached response codes

@gregnavis
Copy link

I think this fits with the overall goal of rack-attack. An extension would be a good place for preventing specific kinds of attacks. This is different, though: we're adding a generic mechanism for taking responses into account. Such mechanism is something a third-party extension could be built upon.

Additionally, if we're talking about supporting extensions then some of rack-attack classes would become published interfaces, and we'd need to take backwards compatibility into account.

I have a use case for this. A side project of mine has been subjected to some account spam. Bots used by spammer send requests that cause ActionController::ParameterMissing to be raised. The parameters I can see in error captures indicate these requests were not generated by the app (it's impossible to send such malformed request + decoy input is filled indicating bot activity).

Currently, in order to handle that case, I'd need to replicate a lot of controller logic in my rack-attack initializer. This can be problematic for a single controller action, not to mention when many disparate controller actions need to be protected.

@alexsmartens
Copy link

My use case is to prevent brute forcing things like login. If there are N failures I want to block anyone from logging in to this account for some time, say a day. Because this logic is based on the response code there is no good way to make it work with Rack::Attack without a workaround.

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

Successfully merging this pull request may close these issues.

None yet

8 participants