Merge lp:~roadmr/canonical-identity-provider/non-drifting-totp into lp:canonical-identity-provider/release
Status: | Merged |
---|---|
Approved by: | Daniel Manrique |
Approved revision: | no longer in the source branch. |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | lp:~roadmr/canonical-identity-provider/non-drifting-totp |
Merge into: | lp:canonical-identity-provider/release |
Diff against target: |
99 lines (+72/-2) 2 files modified
src/identityprovider/models/twofactor.py (+2/-1) src/identityprovider/tests/test_models_device.py (+70/-1) |
To merge this branch: | bzr merge lp:~roadmr/canonical-identity-provider/non-drifting-totp |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Natalia Bidart (community) | Approve | ||
Review via email: mp+363558@code.launchpad.net |
Commit message
Do not store/use an OATH TOTP client's calculated "absolute drift".
Per LP bug #1817075, the "stored absolute drift" functionality of python-oath
is broken and allows a client to reuse a token that is just expired (due to
allowing relative drift of +/-30 seconds), and keep reusing it just past the
end of the previously-
indefinitely.
A side-effect of this is that we will require OATH TOTP devices to have
*accurate* clocks, which is deemed acceptable since the vast majority of clients
are either phones or computers. "Accurate" is quite lenient though, because
a device can be +/- 45 seconds off and still generate valid codes.
Description of the change
Do not store/use an OATH TOTP client's calculated "absolute drift".
Per LP bug #1817075, the "stored absolute drift" functionality of python-oath
is broken and allows a client to reuse a token that is just expired (due to
allowing relative drift of +/-30 seconds), and keep reusing it just past the
end of the previously-
indefinitely.
A side-effect of this is that we will require OATH TOTP devices to have
*accurate* clocks, which is deemed acceptable since the vast majority of clients
are either phones or computers. "Accurate" is quite lenient though, because
a device can be +/- 45 seconds off and still generate valid codes.
As usual, the fix is ONE LINE; the rest is tests :/
Note that this will entirely leave alone existing AuthDevice records which contain non-zero drifts. We can ask for a DB query (or try to do it via the admin) to see how many devices are in that situation and decide whether we want to reset them all to 0 absolute drift. I expect a relatively large number of devices with drift of 1 or -1, and those would mostly still work if we reset to 0 drift.
18:00:30 <wgrant> nessita, roadmr: if we don't allow any drift, then a clock being even a second off will result in codes sometimes not being valid. Theres already a 30s replay window, so increasing that the 60-90s doesn't change the security significantly while also preventing a clock that's off by a few seconds (common) from being a serious problem. Pretty sure other TOTP consumers do this, but we should experiment