Skip to content

LG-13689: Rate-limit sign-in attempts based on IP+user ID#10982

Merged
aduth merged 21 commits intomainfrom
aduth-lg-13689-rate-limit-sign-in-user-id-ip
Aug 1, 2024
Merged

LG-13689: Rate-limit sign-in attempts based on IP+user ID#10982
aduth merged 21 commits intomainfrom
aduth-lg-13689-rate-limit-sign-in-user-id-ip

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 24, 2024

🎫 Ticket

LG-13689

🛠 Summary of changes

Adds additional rate-limiting to the sign-in page to limit attempts based on a combination of user and IP address.

In doing so, it enhances RateLimiter class to support exponentially-scaling rate-limit expirations, including maximum period.

📜 Testing Plan

It will be helpful to adjust local configuration to simplify testing, e.g.

sign_in_user_id_per_ip_attempt_window_exponential_factor: 4
sign_in_user_id_per_ip_attempt_window_in_minutes: 60
sign_in_user_id_per_ip_attempt_window_max_minutes: 1_440
sign_in_user_id_per_ip_max_attempts: 4

With this configuration, it's expected that the lockout would max at 24 hours, which would quickly be hit with an exponential factor of 4:

(1..4).to_a.map { |i| [i, (4 ** (i - 1)).hours] }
# [[1, 1 hour], [2, 4 hours], [3, 16 hours], [4, 64 hours]]

Throughout testing, you can reset your local throttle cache by flushing the Redis store, using rails console:

REDIS_THROTTLE_POOL.with { |client| client.flushdb }

It's also recommended to use a fresh private browsing window to avoid conflicts with max_bad_passwords blocking, or set the configuration for that to a high value:

development:
  max_bad_passwords: 10_000

Verify that you cannot make any attempts past your 4th, and you're locked out for 24 hours:

  1. Go to http://localhost:3000
  2. Enter a username and password incorrectly 5 times
  3. See error message "You have exceeded the maximum sign in attempts. You must wait [~24 hours] before trying again."

👀 Screenshot

image

@aduth aduth changed the title Support exponential factor for rate limit expiration LG-13689: Rate-limit sign-in attempts based on IP+user ID Jul 25, 2024
@aduth aduth marked this pull request as ready for review July 25, 2024 13:34
@aduth aduth requested review from a team and mitchellhenke July 25, 2024 13:34
@aduth aduth force-pushed the aduth-lg-13689-rate-limit-sign-in-user-id-ip branch from d7e7cfa to f55fd24 Compare July 26, 2024 14:22
Copy link
Contributor

Choose a reason for hiding this comment

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

better labeling than "locked out" imo

Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally and confirm expected outcome.

aduth and others added 6 commits August 1, 2024 11:29
Co-Authored-By: Mitchell Henke <1430443+mitchellhenke@users.noreply.github.com>
See: #10982 (comment)
Co-Authored-By: Mitchell Henke <1430443+mitchellhenke@users.noreply.github.com>
It's not quite as performant and the primary use-case doesn't require it, so only use if needed
@aduth aduth force-pushed the aduth-lg-13689-rate-limit-sign-in-user-id-ip branch from fd0dd83 to 41ddd97 Compare August 1, 2024 15:29
@aduth aduth merged commit fd81274 into main Aug 1, 2024
@aduth aduth deleted the aduth-lg-13689-rate-limit-sign-in-user-id-ip branch August 1, 2024 17:48
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.

4 participants