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

Proposed by Daniel Manrique
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
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-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.
Revision history for this message
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

Revision history for this message
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
=== modified file 'src/identityprovider/models/twofactor.py'
--- src/identityprovider/models/twofactor.py 2018-06-21 15:08:26 +0000
+++ src/identityprovider/models/twofactor.py 2019-02-27 21:00:38 +0000
@@ -202,7 +202,8 @@
202 backward_drift=settings.HOTP_BACKWARDS_DRIFT,202 backward_drift=settings.HOTP_BACKWARDS_DRIFT,
203 )203 )
204204
205 if valid:205 # Don't store drift value for totp devices (LP #1817075)
206 if valid and not self.is_totp():
206 self.counter = new_device_value207 self.counter = new_device_value
207 self.save()208 self.save()
208 return valid209 return valid
209210
=== modified file 'src/identityprovider/tests/test_models_device.py'
--- src/identityprovider/tests/test_models_device.py 2018-02-09 20:56:16 +0000
+++ src/identityprovider/tests/test_models_device.py 2019-02-27 21:00:38 +0000
@@ -1,7 +1,8 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the1# Copyright 2010-2019 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4from base64 import b16encode4from base64 import b16encode
5from time import time
56
6from django.conf import settings7from django.conf import settings
7from oath import hotp, totp8from oath import hotp, totp
@@ -146,3 +147,71 @@
146 def test_authenticate_invalid_dec8(self):147 def test_authenticate_invalid_dec8(self):
147 otp = totp(self.key, 'dec8')148 otp = totp(self.key, 'dec8')
148 self.assertFalse(self.device.authenticate(otp[1:]+otp[:1]))149 self.assertFalse(self.device.authenticate(otp[1:]+otp[:1]))
150
151 def test_authenticate_with_just_expired_code_ok(self):
152 # Calculate otp code with the previously valid, now expired code. Note
153 # this test verifies we are accepting slightly old codes, which
154 # requires TOTP_BACKWARD_DRIFT to be 1 or higher.
155 # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
156 now = time()
157 base = now - now % settings.TOTP_PERIOD
158 # 1 second into the previous window
159 the_past = base - 1
160 # Generate this past, valid code and auth with it
161 otp = totp(self.key, 'dec6', t=the_past)
162 self.assertTrue(
163 self.device.authenticate(otp),
164 "This test WILL fail if TOTP_BACKWARD_DRIFT is 0. "
165 "Double-check the setting and remove/skip this test "
166 "if the desired behavior/setting has changed.")
167 # A drifty client should NOT cause the device counter to
168 # be updated (LP #1817075)
169 self.assertEqual(self.device.counter, 0)
170
171 def test_authenticate_with_slightly_too_new_code_ok(self):
172 # Calculate otp code with the immediate next code. Note this test
173 # verifies we are accepting slightly new codes, which
174 # requires TOTP_FORWARD_DRIFT to be 1 or higher.
175 # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
176 now = time()
177 base = now - now % settings.TOTP_PERIOD
178 # 1 second into the next window
179 the_past = base + settings.TOTP_PERIOD + 1
180 # Generate this future, valid code and auth with it
181 otp = totp(self.key, 'dec6', t=the_past)
182 self.assertTrue(
183 self.device.authenticate(otp),
184 "This test WILL fail if TOTP_FORWARD_DRIFT is 0. "
185 "Double-check the setting and remove/skip this test "
186 "if the desired behavior/setting has changed.")
187 # A drifty client should NOT cause the device counter to
188 # be updated (LP #1817075)
189 self.assertEqual(self.device.counter, 0)
190
191 def test_authenticate_with_too_old_code_fails(self):
192 # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
193 now = time()
194 base = now - now % settings.TOTP_PERIOD
195 # Go TOTP_BACKWARD_DRIFT * TOTP_PERIOD seconds into the past,
196 # which should still be valid...
197 delta = settings.TOTP_PERIOD * settings.TOTP_BACKWARD_DRIFT
198 the_past = base - delta
199 # and now go 1 second into the past, which is now invalid
200 the_past -= 1
201 # Generate this past, invalid code and auth with it
202 otp = totp(self.key, 'dec6', t=the_past)
203 self.assertFalse(self.device.authenticate(otp))
204 self.assertEqual(self.device.counter, 0)
205
206 def test_authenticate_with_too_future_code_fails(self):
207 # Baseline, aligned to 30-second (TOTP_PERIOD, really) boundary
208 now = time()
209 base = now - now % settings.TOTP_PERIOD
210 # go TOTP_FORWARD_DRIFT * (TOTP_PERIOD + 1) seconds into the future,
211 # which is invalid (the ending value is non-inclusive)
212 delta = settings.TOTP_PERIOD * (settings.TOTP_FORWARD_DRIFT + 1)
213 the_future = base + delta
214 # Generate this future, invalid code and auth with it
215 otp = totp(self.key, 'dec6', t=the_future)
216 self.assertFalse(self.device.authenticate(otp))
217 self.assertEqual(self.device.counter, 0)