A few major (and somewhat philosophical changes) #4

wants to merge 13 commits into


None yet
2 participants

jlovison commented Jan 11, 2013

I was up last night thinking about this app, and had a few thoughts on slightly different approaches.

I'm not sure what of my commits you'll agree with, but by all means incorporate anything you like.

Expire via Reset Password

Problem: I'd like to have really long lockout periods to stop bad people, but don't want to keep forgetful users out of my app.
Solution: I can add a signal receiver that gets called before the User object is saved, check if the password for the new instance is different from the old one, and if so, expire outstanding locks for that user. This way, if a user gets blocked, I force them over to reset their password, and since they verify using their email address, I can remove the block after the reset.

Tricky Bad Guys

Problem: Ok, but now if my limit for attempts per ip is 40, a smart attacker could just have their final login attempt be from an account they have access to, having guessed 39 other users passwords, and after lockout, reset that account and start all over with a clean slate.
Solution: Track login attempts by the combination of username/ip, and use the aggregate method for querysets to calculate the sum of the attempts for a given ip. Even if they reset their own account, they only get one more login attempt before lockout all over again.

Distributed attacks

Problem: What if the bad guys have a botnet, and are trying to log in from a lot of different ips? That's still a lot of login attempts that are getting by the blocker.
Solution: Calculate the sum of failed logins for a given username, and if greater than the limit, block the request. Bye bye botnets.

Denial of Service

Problem: But wait - if we're blocking based on failed logins for a given username, users can be blocked from being able to log in from a malicious attacker, causing a DoS.
Solution: Add a setting for a "failsafe" limit. After the limit for a given ip or username, we still allow up to this number of logins for that specific username/ip combination. This would allow an expiration duration of say, 1 month, a normal limit of say, 50, and a failsafe limit of 3. Most users would never have 50 failed logins in a month, so they'll be fine. If an attacker tries breaking into their account, they'll still have 3 logins from any IP before getting blocked. But the attacker, after hitting this limit, can only try 3 logins for any given user before getting blocked from another attempt on that user.


Problem: It'd be nifty if we could get notifications when someone has been blocked. But building in a notification system is perhaps beyond the scope of a specialized app like this.
Solution: Oh hey, but the Django logging system has a lot of options for notifications, from email to sending to a backend like Sentry. How about we create a setting that allows users to set a custom loglevel for logging blocked IPs, so they can set that up with their own logging backends however they want?

Implementation Details

No longer blocking the login page on GET requests, as we're tracking by username, which requires a POST to grab.

In order to prevent people from submitting a login attempt, we check if the ip or username is blocked prior to calling the login function, and only actually grab the response if they are allowed, otherwise we immediately render the blocked response.

This also fixes an issue in the previous implementation that allowed me to continue to tie up the login processor even if invalid as long as I was directly submitting POST requests and never accessing the page via a GET request to render the form.

Had to remove the failed_access from the user-locked template generation, as sometimes blocking based on aggregate query, and not a specific failed_access object.

There are a few more DB queries than before.

Wrap up

I think that's all. If you have any questions about specific changes, by all means ask. Also, you might want to take a look over the code from a formatting/efficiency perspective anyways. I think your code was much more elegant than my pass through here, though I am (obviously) a fan of some of the changes I added.


AdrianRibao commented Jan 11, 2013

Wow, that's a great contribution.

Let me check your commits and I'll come back with some feedback soon.

Thank you.

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