Adds rack-attack throttles #209
Conversation
Pull Request Test Coverage Report for Build 14381121857Details
💛 - Coveralls |
Why are these changes being introduced: * Bots and pentesters are using a significant amount of our resources Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-480 How does this address that need: * Adds a rack-attack configuration for a few scenarios * Overall throttle for extremely high volume (not including assets) * Lower throttle for redirect detected requests (these are bots behaving badly) * a few pentester bans * campus IPs are safelisted, which should include ezproxy and vpn Document any side effects to this change: It's difficult to test to ensure the best tweaks to this type of work until it is in production.
JPrevost
force-pushed
the
timx-480-rack-attack-config
branch
from
e36e66c to
e12a1a5
Compare
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
I'm doing this review from campus, so I can't confirm any negative behavior myself. I have seen, though, that campus addresses are not blocked when I repeatedly request paths that should trigger fail2ban however.
This said, I think the configurations here seem pretty straightforward - so in theory I think this works pretty well. As you say, we probably need to get this in production and then tweak from there?
The only question I have about any details, which isn't a blocking question, is to ask whether we should specify MIT addresses in the IPv6 space, and not just what's left of the IPv4 space? As with other details, though, this can be something we deal with after merging this.
if you're comfortable with my review being done from a safelisted location. If you'd like me to confirm the negative behavior, I'll take another look when I'm back home.
| # http://kb.mit.edu/confluence/x/F4DCAg | ||
| # Main IP range (includes campus, NAT pool, and VPNs) | ||
| # This also affects bot_challenge_page logic which uses rack_attack under the hood | ||
| Rack::Attack.safelist_ip("18.0.0.0/11") |
There was a problem hiding this comment.
I confess that I'm a little fuzzy on how IPv6 relates to IPv4, but do we need to specify any blocks of IPv6 addresses here as well?
There was a problem hiding this comment.
Great question. We should consider safe listing the IPv6 range as well, but I'll open that as something to explore further
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Document any side effects to this change:
It's difficult to test to ensure the best tweaks to this type of work until it is in production.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing