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

Improve IP blocking after unsuccessful logins #2047

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

jayjay-w
Copy link
Contributor

Description

Improve IP blocking after unsuccessful logins

  • Only add IPs to the track list on unsuccessful login
  • On successful login, remove IP completely from tracklist

We have also moved code related to authentication blocking to devise.rb

References: CV2-4866

How has this been tested?

Tested using the script below. When the credentials are correct, you should be able to log in repeatedly more than 100 times.

When using incorrect credentials, your IP should get blocked after 100 attempts.

#!/bin/bash

for i in {1..110}
do
  curl -X POST 127.0.0.1:3000/api/users/sign_in \
       -H "Content-Type: application/json" \
       -d '{"api_user": {"email": "[email protected]", "password": "PASSWORD", "otp_attempt": ""}}'
  echo "Attempt $i"
done

If your IP gets blocked, access the rails console and run the script below. This will unblock all blocked IPs.

redis = Redis.new(REDIS_CONFIG)

tracked_keys = redis.keys('track:*')
blocked_keys = redis.keys('block:*')

tracked_keys.each do |key|
  redis.del(key)
end

blocked_keys.each do |key|
  redis.del(key)
  puts "Unblocked #{key}"
end

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

- Only add IPs to the track list on unsuccessful login
- On successful login, remove IP completely from tracklist

We have also moved code related to authentication blocking to devise.rb
@jayjay-w jayjay-w force-pushed the CV2-4866-enhance-login-rate-limiting branch from c3a6b4b to b426748 Compare September 23, 2024 21:56
@jayjay-w jayjay-w marked this pull request as ready for review September 23, 2024 21:57
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Thanks Jay, great stuff! I tested locally using the script but also using the web client and it works as expected. I just noticed one thing, please review before merging: in the code, the configuration key is login_block_limit, but in config/config.yml.example it's login_rate_limit. Please review and make it consistent.

Normalize login_block_limit setting name
@jayjay-w
Copy link
Contributor Author

Thank you Caio for catching that

@jayjay-w jayjay-w merged commit 56c1463 into develop Sep 24, 2024
9 checks passed
jayjay-w added a commit that referenced this pull request Sep 24, 2024
* Improve IP blocking after unsuccessful logins

- Only add IPs to the track list on unsuccessful login
- On successful login, remove IP completely from tracklist

We have also moved code related to authentication blocking to devise.rb
@jayjay-w jayjay-w deleted the CV2-4866-enhance-login-rate-limiting branch October 6, 2024 15:24
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.

2 participants