Skip to content

Remove Rack::Attack#1574

Merged
zachmargolis merged 1 commit intomasterfrom
margolis-remove-rack-attack
Aug 24, 2017
Merged

Remove Rack::Attack#1574
zachmargolis merged 1 commit intomasterfrom
margolis-remove-rack-attack

Conversation

@zachmargolis
Copy link
Contributor

Why:
It has been disabled for a while now, we moved critical parts
like SMS rate limiting to app code.

@jmhooper
Copy link
Contributor

Do we have rate limiting for brute force login attacks somewhere in the application code? That looks to me like on of the more important features that was disabled.

@zachmargolis
Copy link
Contributor Author

@jmhooper my understanding was we moved the rate limiting upstream to AWS

Copy link
Contributor

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

If this is rate limiting has been moved to AWS, and Rack Attack is currently disabled, then yeah this is dead code and we should remove it.

@monfresh
Copy link
Contributor

One nice thing Rack Attack provided was the ability to log each time a throttle was triggered. Does AWS provide that as well? How is rate limiting currently configured in AWS?

@zachmargolis zachmargolis force-pushed the margolis-remove-rack-attack branch 2 times, most recently from addf8b1 to d427f66 Compare August 24, 2017 14:31
**Why**:
It has been disabled for a while now, we moved critical parts
like SMS rate limiting to app code.
@zachmargolis zachmargolis merged commit cbcddb2 into master Aug 24, 2017
@zachmargolis zachmargolis deleted the margolis-remove-rack-attack branch August 24, 2017 17:58
@monfresh
Copy link
Contributor

Did we ever get answers to my questions? Are we able to log and monitor when a limit is being exceeded, such as when someone is trying to guess a password?

@zachmargolis
Copy link
Contributor Author

@monfresh no, right now we don't. But my feelings were that since it's currently disabled, it's just cruft. We can always git revert to bring it back if needed.

@monfresh
Copy link
Contributor

It was only disabled temporarily to help us troubleshoot the issue we had in prod a while back. We resolved that by removing the OTP sending from Rack Attack. My opinion is that we should turn it back on until we find a replacement that also includes logging. I think it's valuable to be able to see how often rate limits get triggered.

@zachmargolis
Copy link
Contributor Author

zachmargolis commented Aug 24, 2017

It was "temporarily" disabled back in May -- I'm going to leave this as-is but 100% supportive of other PRs to fix/revert/etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants