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

Mention setting MFA_TOTP_VALID_WINDOW in README.md #18

Closed
wants to merge 2 commits into from

Conversation

humphrey
Copy link
Contributor

After following the Usage steps, I regularly received "Validation failed" errors when verifying with my TOPT code. Once I noticed that this only happened when the code was about to expire, I dug through the code and discovered the MFA_TOTP_VALID_WINDOW setting. Since I can't imagine a scenario where 0 (the default value) is a usable window size in the real world, I think this setting should be mentioned in the Usage guide.

I also think a value of 1 or 2 would be a better default, but I do like that a default of 0 forces the developer to think about what this value should be for their usage.

@humphrey
Copy link
Contributor Author

Oh, I just noticed I didn't update the rest of the step numbers. Sorry!

@xi
Copy link
Owner

xi commented Apr 24, 2023

Why do you think that 0 is not a useful value in real world scenarios? If it doesn't work for you that probably indicates that the clock is off on either the server of client. Also note that a value bigger than 1 is explicitly discouraged by the spec. See #11 for prior discussion.

Still, it is true that this setting is not mentioned in the README at all. Do you think a generic step in the usage section could work (.e.g. "Check settings.py for additional settings")?

@humphrey
Copy link
Contributor Author

Thanks for linking the RFC - I struggled to find a good suggestion for window size. I now think 1 is the perfect choice 👌

Why do you think that 0 is not a useful value in real world scenarios?

Even if clocks are perfectly in sync, there is a delay from when the code is generated/shown in an authentication app and verified by the server. If it takes me 10 seconds to read the current code, type it, and submit it, I will fail validation 33% of the time (whenever there is less than 10 seconds left in the 30 second interval window). I currently have my code generator and server running on the same machine (so the clocks should be identical) and I am getting frequently invalid codes.

I am still quite new to working with TOTP's so there could be something I am missing. However, the RFC you linked mentions that having a window of 0 would cause this issue:

When an OTP is generated at the end of a time-step window, the receiving time most likely falls into the next time-step window. A validation system SHOULD typically set a policy for an acceptable OTP transmission delay window forvalidation. The validation system should compare OTPs not only with the receiving timestamp but also the past timestamps that are within the transmission delay. A larger acceptable delay window would expose a larger window for attacks. We RECOMMEND that at most one time step is allowed as the network delay.

Do you think a generic step in the usage section could work (.e.g. "Check settings.py for additional settings")?

Yes - if there is something that I've missed, and a window size of 0 doesn't cause the issue I am experience.

No - if the behavior I am experiencing is because of entering a code that is about to expire with a window size of 0, then I think the README.md absolutely should mention the setting, or have a default value of 1.

@xi
Copy link
Owner

xi commented Apr 24, 2023

I don't really want to use a different recommendation/default than pyotp. Could you please open an issue there? If you can convince them to change the default, I will adapt the django-mfa3 code accordingly.

@humphrey
Copy link
Contributor Author

humphrey commented Apr 26, 2023

I don't really want to use a different recommendation/default than pyotp.

I totally understand that - and I think having a default of 0 is a totally acceptable value.

I still think it would be worth having a brief mention of this setting in the usage docs. It took me more than a couple of hours of digging around to track down this setting and work out what a good value for it was, and it would be nice to save future devs having to do that :)

@xi xi force-pushed the main branch 2 times, most recently from ccb7982 to a989df1 Compare June 16, 2023 14:56
@xi
Copy link
Owner

xi commented Aug 30, 2023

This should be covered by 384160f.

@xi xi closed this Aug 30, 2023
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.

2 participants