mirror of
https://github.com/dani-garcia/vaultwarden.git
synced 2026-02-05 00:29:40 +03:00
Customizable TOTP tolerance #477
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @arjones85 on GitHub (Nov 5, 2019).
When logging into Bitwarden, I am able to use an "old" OTP code from Authy long after it has already changed over to another one. Is this expected?
@fbartels commented on GitHub (Nov 5, 2019):
Hi @arjones85,
how "long" ago are you talking? Could be related to https://github.com/dani-garcia/bitwarden_rs/pull/657
@arjones85 commented on GitHub (Nov 5, 2019):
When I tested it was a full 10 seconds old but was still accepted.
I don't have any time drift, all devices involved here are NTP bound and syncing their time fine.
@timendum commented on GitHub (Nov 5, 2019):
It's a standard feature, recommended in the rfc[1].
The time window is 30s, so the code is valid for 1 minute.
[1] rfc6238
@dani-garcia commented on GitHub (Nov 5, 2019):
Right, as @timendum said, we allow not only the current timestamp code, but the previous one and the next one, which gives the system a 1 minute 30 second window, to try to be more resilient against time drift. For someone using NTP in both server and all devices, this could be reduced to just the current one for a small security gain.
At the moment we don't expose such an option, but if there's interest we could add it.
@arjones85 commented on GitHub (Nov 5, 2019):
Got it, that makes sense!
I'm not sure it's necessary for my setup, but I can see how technically reducing the security by 66% (by allowing up to 3 codes that could be guessed vs just 1) could raise a stink in larger organizations.
I'm in support of a configuration option for this.
@BlackDex commented on GitHub (Nov 5, 2019):
Umm i think this should not be possible since it should mark that code as already used if it has been used already. If the current code or a future code as been used it should not be able to be used again. That is at least how i changed it a few commits ago.
Making this configurable is a nice addition. But i think it should be either false, no time drift, or true, just the previous, current or future one. Nothing more
@dani-garcia commented on GitHub (Nov 5, 2019):
I think the OP means using an unused code that isn't the current one, not using one that's already used.
About giving options, yeah allowing a higher range doesn't make much sense, but at the same time we expect admins to know what they are doing, for example we allow someone to do ADMIN_TOKEN=a, even if having a one character password is a terrible idea. Personally as long as the setting is explained correctly I'm okay with either option.
@BlackDex commented on GitHub (Nov 5, 2019):
Ah, that makes sense indeed about the unused code. Well, if someone needs more then 1 step they better check the time on the devices ;).
But good option indeed.
(As a workaround for now, set the server 30 seconds ahead :p )
@dani-garcia commented on GitHub (Nov 9, 2019):
Resolved in #713, setting AUTHENTICATOR_DISABLE_TIME_DRIFT=true will disable the extra tolerance, only allowing the current time step.