Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.

Add optional recaptcha brute force protection #764

Conversation

gnordens
Copy link

@gnordens gnordens commented Mar 6, 2018

Hi,
This PR adds an optional recaptcha brute force protection to the login form (default is disabled).

Login attempts and last login attempt are tracked on each user and require two extra columns:
last_login_attempt_at = db.Column(db.DateTime, default=db.func.current_timestamp(), nullable=False) login_attempts = db.Column(db.Integer, default=0)
A user is required to enter a recaptcha after security_recaptcha_max_attempts is reached.
Login attempts are reset for a user after security_recaptcha_timeout is reached.

Related:
#348
#161

@Avamander
Copy link

There already is optional recaptcha protection ability in flask-security.

from flask_wtf import RecaptchaField

class ExtendedRegistrerForm(forms.RegisterForm):
    recaptcha = RecaptchaField("Captcha")

security = Security(app, user_datastore,
                    confirm_register_form=ExtendedRegistrerForm)

You can override every form like this.

@gnordens
Copy link
Author

@Avamander
I might be misunderstanding you (or missing something in the docs), but in your example aren't you simply overriding a form to include a recaptcha element? This PR instead looks to add a conditional recaptcha based on consecutive logins for a user rather than always displaying one. This is to reduce annoyance for the user trying to login while also adding some protection against brute force attacks. A similar feature has been request in the issues mentioned in the OP.

@Avamander
Copy link

Avamander commented Mar 12, 2018

@gnordens Nothing stops you from counting login attempts in a custom user field inside the custom form and displaying the captcha based on that. It's almost what you're doing with this PR, the primary reason I commented was that I personally can't see a big improvement over the override method and your PR, as it doesn't touch the forms that would actually really require captchas (PW reset, register for ex.). What I'm trying to say that this is only half of the solution and just including this might give a false sense of security and every form should be sufficiently protected that f-security has.

@jirikuncar
Copy link
Contributor

This looks like a change that should be handled per instance basis. Personally, I would rather store the login attempts in Redis rather than in SQL database.

Copy link
Contributor

@jirikuncar jirikuncar left a comment

Choose a reason for hiding this comment

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

Can you rewrite it as a tutorial that can be shown in docs?

+ timedelta(milliseconds=_security.recaptcha_timeout):
user.login_attempts = 0

user.login_attempts = user.login_attempts + 1 if user.login_attempts else 1
Copy link
Contributor

Choose a reason for hiding this comment

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

The counter might not be very reliable for parallel attempts.

@jirikuncar
Copy link
Contributor

I am going to close the PR mainly for following reasons:

  1. The ReCaptcha field can be already added to the custom forms;
  2. The login counter is not very reliable in the current implementation;
  3. Lack of tests.

If you find that 2. needs to be implemented in this package please send a separate PR.

@jirikuncar jirikuncar closed this Jul 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants