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

Add notification email on invalid second authenticator #28822

Merged
merged 1 commit into from Jan 22, 2024

Conversation

ClearlyClaire
Copy link
Contributor

While reviewing our second factor authentication security, I realized that while we had appropriate rate-limiting and logging of authentication failures, we actually never notified the user of those failures, meaning they would be unlikely to notice such attempts.

This PR addresses that by sending a mail as soon as a 2FA attempt fails. Mails are limited to one per hour to avoid spamming in case of an attack or multiple failures.

To further reduce spam, this should not be triggered immediately, but after a few minutes without a successful log-in, as failing to enter a TOTP token correctly on time is not that unusual. However, that would be more work, and I think this change is a worthwhile security improvement.

image

@ClearlyClaire ClearlyClaire added the security Security issues and fixes, vulnerabilities label Jan 19, 2024
@ClearlyClaire ClearlyClaire requested a review from a team January 19, 2024 16:57
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3593ee2) 85.07% compared to head (4686bd9) 85.08%.
Report is 1 commits behind head on main.

❗ Current head 4686bd9 differs from pull request most recent head 0e6401f. Consider uploading reports for the commit 0e6401f to get more accurate results

Files Patch % Lines
app/controllers/auth/sessions_controller.rb 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #28822   +/-   ##
=======================================
  Coverage   85.07%   85.08%           
=======================================
  Files        1038     1038           
  Lines       28142    28152   +10     
  Branches     4532     4533    +1     
=======================================
+ Hits        23943    23952    +9     
  Misses       3039     3039           
- Partials     1160     1161    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ClearlyClaire ClearlyClaire force-pushed the features/2fa-failure-notification branch from 4686bd9 to 0e6401f Compare January 19, 2024 17:17
@ClearlyClaire ClearlyClaire added this pull request to the merge queue Jan 22, 2024
Merged via the queue into main with commit e2d9635 Jan 22, 2024
47 checks passed
@ClearlyClaire ClearlyClaire deleted the features/2fa-failure-notification branch January 22, 2024 13:55
@kmycode
Copy link
Contributor

kmycode commented Feb 7, 2024

@ClearlyClaire

return if redis.set("2fa_failure_notification:#{user.id}", '1', ex: 1.hour, get: true).present?

The get option was added to the set command in Redis 6.2.0 and later.
Two-step verification may not work properly with Redis prior to 6.

Am I correct in understanding that we are raising the minimum version of Redis in Mastodon from 4 to 6?

Ref: https://redis.io/commands/set/

Starting with Redis version 6.2.0: Added the GET, EXAT and PXAT option.
Starting with Redis version 7.0.0: Allowed the NX and GET options to be used together.

README.md is not updated.

### Requirements

- **PostgreSQL** 12+
- **Redis** 4+
- **Ruby** 2.7+
- **Node.js** 16+
- ```

@ClearlyClaire
Copy link
Contributor Author

ClearlyClaire commented Feb 7, 2024

Good catch, I missed the fact this bumped the requirements… Redis 5 has been EOL for a while, so I believe it should be safe to bump the version requirement, but that indeed needs to be documented! Thanks!

EDIT: that being said, distributions like Ubuntu 20.04, which are not EOL yet, still distribute Redis 5… I'll see if I can rewrite this in a way that does not require something more recent than Redis 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security issues and fixes, vulnerabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants