Merge lp:~roadmr/canonical-identity-provider/non-drifting-totp into lp:canonical-identity-provider

Proposed by Daniel Manrique on 2019-02-22
Status: Merged
Approved by: Daniel Manrique on 2019-03-06
Approved revision: 1683
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
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
Reviewer Review Type Date Requested Status
Natalia Bidart 2019-02-22 Approve on 2019-03-06
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-calculated absolute drift to keep it "alive"
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-calculated absolute drift to keep it "alive"
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.

To post a comment you must log in.
1682. By Daniel Manrique on 2019-02-22

oops, stray print removed

1683. By Daniel Manrique on 2019-02-27

Slight comment clarifications

Daniel Manrique (roadmr) wrote :

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

Natalia Bidart (nataliabidart) wrote :

Looks good!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/identityprovider/models/twofactor.py'
2--- src/identityprovider/models/twofactor.py 2018-06-21 15:08:26 +0000
3+++ src/identityprovider/models/twofactor.py 2019-02-27 21:00:38 +0000
4@@ -202,7 +202,8 @@
5 backward_drift=settings.HOTP_BACKWARDS_DRIFT,
6 )
7
8- if valid:
9+ # Don't store drift value for totp devices (LP #1817075)
10+ if valid and not self.is_totp():
11 self.counter = new_device_value
12 self.save()
13 return valid
14
15=== modified file 'src/identityprovider/tests/test_models_device.py'
16--- src/identityprovider/tests/test_models_device.py 2018-02-09 20:56:16 +0000
17+++ src/identityprovider/tests/test_models_device.py 2019-02-27 21:00:38 +0000
18@@ -1,7 +1,8 @@
19-# Copyright 2010 Canonical Ltd. This software is licensed under the
20+# Copyright 2010-2019 Canonical Ltd. This software is licensed under the
21 # GNU Affero General Public License version 3 (see the file LICENSE).
22
23 from base64 import b16encode
24+from time import time
25
26 from django.conf import settings
27 from oath import hotp, totp
28@@ -146,3 +147,71 @@
29 def test_authenticate_invalid_dec8(self):
30 otp = totp(self.key, 'dec8')
31 self.assertFalse(self.device.authenticate(otp[1:]+otp[:1]))
32+
33+ def test_authenticate_with_just_expired_code_ok(self):
34+ # Calculate otp code with the previously valid, now expired code. Note
35+ # this test verifies we are accepting slightly old codes, which
36+ # requires TOTP_BACKWARD_DRIFT to be 1 or higher.
37+ # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
38+ now = time()
39+ base = now - now % settings.TOTP_PERIOD
40+ # 1 second into the previous window
41+ the_past = base - 1
42+ # Generate this past, valid code and auth with it
43+ otp = totp(self.key, 'dec6', t=the_past)
44+ self.assertTrue(
45+ self.device.authenticate(otp),
46+ "This test WILL fail if TOTP_BACKWARD_DRIFT is 0. "
47+ "Double-check the setting and remove/skip this test "
48+ "if the desired behavior/setting has changed.")
49+ # A drifty client should NOT cause the device counter to
50+ # be updated (LP #1817075)
51+ self.assertEqual(self.device.counter, 0)
52+
53+ def test_authenticate_with_slightly_too_new_code_ok(self):
54+ # Calculate otp code with the immediate next code. Note this test
55+ # verifies we are accepting slightly new codes, which
56+ # requires TOTP_FORWARD_DRIFT to be 1 or higher.
57+ # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
58+ now = time()
59+ base = now - now % settings.TOTP_PERIOD
60+ # 1 second into the next window
61+ the_past = base + settings.TOTP_PERIOD + 1
62+ # Generate this future, valid code and auth with it
63+ otp = totp(self.key, 'dec6', t=the_past)
64+ self.assertTrue(
65+ self.device.authenticate(otp),
66+ "This test WILL fail if TOTP_FORWARD_DRIFT is 0. "
67+ "Double-check the setting and remove/skip this test "
68+ "if the desired behavior/setting has changed.")
69+ # A drifty client should NOT cause the device counter to
70+ # be updated (LP #1817075)
71+ self.assertEqual(self.device.counter, 0)
72+
73+ def test_authenticate_with_too_old_code_fails(self):
74+ # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
75+ now = time()
76+ base = now - now % settings.TOTP_PERIOD
77+ # Go TOTP_BACKWARD_DRIFT * TOTP_PERIOD seconds into the past,
78+ # which should still be valid...
79+ delta = settings.TOTP_PERIOD * settings.TOTP_BACKWARD_DRIFT
80+ the_past = base - delta
81+ # and now go 1 second into the past, which is now invalid
82+ the_past -= 1
83+ # Generate this past, invalid code and auth with it
84+ otp = totp(self.key, 'dec6', t=the_past)
85+ self.assertFalse(self.device.authenticate(otp))
86+ self.assertEqual(self.device.counter, 0)
87+
88+ def test_authenticate_with_too_future_code_fails(self):
89+ # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
90+ now = time()
91+ base = now - now % settings.TOTP_PERIOD
92+ # go TOTP_FORWARD_DRIFT * (TOTP_PERIOD + 1) seconds into the future,
93+ # which is invalid (the ending value is non-inclusive)
94+ delta = settings.TOTP_PERIOD * (settings.TOTP_FORWARD_DRIFT + 1)
95+ the_future = base + delta
96+ # Generate this future, invalid code and auth with it
97+ otp = totp(self.key, 'dec6', t=the_future)
98+ self.assertFalse(self.device.authenticate(otp))
99+ self.assertEqual(self.device.counter, 0)