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

[1.x] Cache 2FA token timestamp #366

Merged
merged 3 commits into from
Mar 4, 2022
Merged

[1.x] Cache 2FA token timestamp #366

merged 3 commits into from
Mar 4, 2022

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Feb 28, 2022

This PR allows to cache the 2FA timestamp of a token so it cannot be re-used during the time window of the 2FA engine. It's the solution detailed at https://github.com/antonioribeiro/google2fa#validation-window.

Atm I'm storing the timestamp according with the token as its key in cache but the example from the link above associates it with the user. I don't see how that's important so I think the solution from this PR is probably fine. Happy to be proven otherwise if anyone has any objections.

The solution is backwards compatible because it introduces a new optional dependency for the TwoFactorAuthenticationProvider class.

Addresses concerns raised in #201

@securit
Copy link

securit commented Feb 28, 2022

The reason for associating with a user is to limit the likelihood of possible collisions as it provides a level of uniqueness in the stored key. However the flip side of that is that it could provide a point of enumeration.

I know Taylor said that this was low risk anyway because you need the username, password and a valid token, but haveyoubeenpwnd demonstrates how easy it is to obtain two of the required factors - the reality is that users are lazy and reuse their passwords all over the place.

In terms of risk, this would be low as the cache exists only for as long as the window and these days, the data should be encrypted in transport over HTTPS.

In terms of this solution, I believe that this would satisfy the requirements of the RFC.

Thank you for your Great Work!

@driesvints
Copy link
Member Author

Thanks @securit for verifying that 🙂

@driesvints driesvints marked this pull request as ready for review February 28, 2022 19:26
@x7ryan
Copy link

x7ryan commented Feb 28, 2022

Yes as @securit mentioned the user ID would be used to prevent the extremely unlikely but still possible situation where two users independently generate the same 2fa code within the same window. It's so unlikely the chances of it happen ate almost certainly considered statistically insignificant but if it somehow did itd be quite the corner case. That said the only side effect I can see from not doing it would the second user would have their code invalidated before they get to use it once.

Another usability note, I wonder if it should return a specic error saying the 2fa code has already been used and that the user needs to wait for their app to generate a new one. Perhaps it could be confusing if the user is certain the code is accurate (but has been used) so they get an error saying it's invalid. Doing so would technically acknowledge that user recently logged in but only to someone who already entered a valid username and password. Generally it's best to keep authentication errors as general as possible but I don't see an issue with being more specific here and would probably be more user friendly. But I can also see not bothering to either. I think it comes down to personal choice I guess.

@driesvints
Copy link
Member Author

@x7ryan definitely a good point. The way I'd tackle that is to throw a specific exception that can then be converted into a validation error. But the thing is that that would be a breaking change so it's probably best to leave things be for now. I doubt the case would actually be a problem for a real user since they'd have to use the code, logout, then re-use it in the same one-minute window. For an attacker who attempts to re-use the code, no message might even be better ;)

@driesvints driesvints requested a review from taylorotwell March 1, 2022 10:46
@taylorotwell taylorotwell merged commit ada0678 into 1.x Mar 4, 2022
@taylorotwell taylorotwell deleted the cache-2fa-code branch March 4, 2022 15:06
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