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

Migration path should be clear for the leeway/window #195

Closed
Spomky opened this issue Oct 18, 2023 · 5 comments · Fixed by #201
Closed

Migration path should be clear for the leeway/window #195

Spomky opened this issue Oct 18, 2023 · 5 comments · Fixed by #201
Assignees
Labels

Comments

@Spomky
Copy link
Member

Spomky commented Oct 18, 2023

          It's your code and decision, but the behavior change should really have been mentioned in the v10 to v11 upgrade notes.

It's not a literal break because of an api/syntax change, but it's a more subtle, hard to detect behavior change that everybody should think about when upgrading.

Projects cannot be compatible with both v10 and v11 at once without feature detection or having a leaking abstraction.

Originally posted by @talkinnl in #152 (comment)

@Spomky Spomky self-assigned this Oct 18, 2023
@Spomky Spomky added the DX label Oct 18, 2023
@glo05363
Copy link

@Spomky Is there any update on this issue as while using the library i am facing the same issue where i get the following error: "The leeway must be lower than the TOTP period" with v11. it works fine with v10.

@glo71317
Copy link

@Spomky your inputs are very valuable for us for above query! We are upgrading the v10 to v11 but mentioned issue we are getting. If you can help us with regard then it will be good to take the further action at our end.

@talkinnl
Copy link

If you used window=1 in v10, you might like the behavior of leeway=(period-1) in v11.

If you used bigger windows, you’ll have to loop through time offsets in you own code. I.e. just multiple calls with leeway=0, for […, time()-30, time(), time()+30, …]

@glo71317
Copy link

Thanks for quick response we will check and investigate this at our end if anything will require will come back again

@Spomky
Copy link
Member Author

Spomky commented Nov 28, 2023

Hi,

I will update the migration path page.

A note on #195 (comment) where the exception "Leeway must be less than TOTP period" is thrown. This is one of the reasons why the behavior changed between versions.
If the OTP has the default configuration (period is "30", 6 digits...), a window value of 30 or more is a security issue that should be resolved as soon as possible. This means that the acceptable OTPs are at least 30 before and after the current = at least 61 acceptable values.
In other words, more chances for an attacker to guess the OTP (at least 61/10^6 possibilities, or ~1/16400, which is very low).

If you used window=1 in v10, you might like the behavior of leeway=(period-1) in v11.

To me with previous versions, the window should have been 1 at most.

@Spomky Spomky linked a pull request Nov 28, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants