[#8126] rate limits for 2FA
diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index 3c665cf..39d5b56 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -23,7 +23,7 @@
import bson
import tg
-from allura.lib.exceptions import InvalidRecoveryCode
+from allura.lib.exceptions import InvalidRecoveryCode, MultifactorRateLimitError
from tg import expose, flash, redirect, validate, config, session
from tg.decorators import with_trailing_slash, without_trailing_slash
from pylons import tmpl_context as c, app_globals as g
@@ -346,7 +346,7 @@
if mode == 'totp':
totp_service = TotpService.get()
totp = totp_service.get_totp(user)
- totp_service.verify(totp, code)
+ totp_service.verify(totp, code, user)
elif mode == 'recovery':
recovery = RecoveryCodeService.get()
recovery.verify_and_remove_code(user, code)
@@ -354,6 +354,9 @@
except (InvalidToken, InvalidRecoveryCode):
c.form_errors['code'] = 'Invalid code, please try again.'
return self.multifactor(mode=mode, **kwargs)
+ except MultifactorRateLimitError:
+ c.form_errors['code'] = 'Multifactor rate limit exceeded, slow down and try again later.'
+ return self.multifactor(mode=mode, **kwargs)
else:
plugin.AuthenticationProvider.get(request).login(user=user, multifactor_success=True)
return_to = self._verify_return_to(kwargs.get('return_to'))
@@ -706,7 +709,7 @@
totp_service = TotpService.get()
totp = totp_service.Totp(key)
try:
- totp_service.verify(totp, code)
+ totp_service.verify(totp, code, c.user)
except InvalidToken:
h.auditlog_user('Failed to set up multifactor TOTP (wrong code)')
c.form_errors['code'] = 'Invalid code, please try again.'
diff --git a/Allura/allura/lib/exceptions.py b/Allura/allura/lib/exceptions.py
index e43a8fc..32be05d 100644
--- a/Allura/allura/lib/exceptions.py
+++ b/Allura/allura/lib/exceptions.py
@@ -48,6 +48,10 @@
pass
+class MultifactorRateLimitError(RatelimitError):
+ pass
+
+
class ProjectPhoneVerificationError(ForgeError):
pass
diff --git a/Allura/allura/lib/multifactor.py b/Allura/allura/lib/multifactor.py
index cfd6247..45b732d 100644
--- a/Allura/allura/lib/multifactor.py
+++ b/Allura/allura/lib/multifactor.py
@@ -25,7 +25,7 @@
import errno
import bson
-from allura.lib.exceptions import InvalidRecoveryCode
+from allura.lib.exceptions import InvalidRecoveryCode, MultifactorRateLimitError
from tg import config
from pylons import app_globals as g
from paste.deploy.converters import asint
@@ -42,11 +42,29 @@
log = logging.getLogger(__name__)
+def check_rate_limit(num_allowed, time_allowed, attempts):
+ '''
+ :param int num_allowed:
+ :param int time_allowed:
+ :param list[int] attempts:
+ :return: tuple: ok (bool), attempts still in window (list[int])
+ '''
+ attempts_in_limit = []
+ now = int(time())
+ for prev_attempt in attempts:
+ if now - prev_attempt <= time_allowed:
+ attempts_in_limit.append(prev_attempt)
+ attempts_in_limit.append(now)
+
+ ok = len(attempts_in_limit) <= num_allowed
+ return ok, attempts_in_limit
+
+
class TotpService(object):
'''
An interface for handling multifactor auth TOTP secret keys. Common functionality
is provided in this base class, and specific subclasses implement different storage options.
- A provider must implement :meth:`get_secret_key` and :meth:`set_secret_key`.
+ A provider must implement :meth:`get_secret_key` and :meth:`set_secret_key` and :meth:`enforce_rate_limit`
To use a new provider, expose an entry point in setup.py::
@@ -80,10 +98,12 @@
return totp
- def verify(self, totp, code):
+ def verify(self, totp, code, user):
code = code.replace(' ', '') # Google authenticator puts a space in their codes
code = bytes(code) # can't be unicode
+ self.enforce_rate_limit(user)
+
# TODO prohibit re-use of a successful code, although it seems unlikely with a 30s window
# see https://tools.ietf.org/html/rfc6238#section-5.2 paragraph 5
@@ -126,8 +146,31 @@
'''
raise NotImplementedError('set_secret_key')
+ def enforce_rate_limit(self, user):
+ '''
+ :param user: a :class:`User <allura.model.auth.User>`
+ :raises: MultifactorRateLimitError
+ '''
+ raise NotImplementedError('enforce_rate_limit')
-class MongodbTotpService(TotpService):
+
+class MongodbMultifactorCommon(object):
+
+ def enforce_rate_limit(self, user):
+ prev_attempts = user.get_tool_data('allura', 'multifactor_attempts') or []
+
+ num_allowed = asint(config.get('auth.multifactor.rate_limit.num', 3))
+ time_allowed = asint(config.get('auth.multifactor.rate_limit.time', 30))
+
+ ok, attempts_in_limit = check_rate_limit(num_allowed, time_allowed, prev_attempts)
+
+ user.set_tool_data('allura', multifactor_attempts=attempts_in_limit)
+
+ if not ok:
+ raise MultifactorRateLimitError
+
+
+class MongodbTotpService(MongodbMultifactorCommon, TotpService):
'''
Store in TOTP keys in mongodb.
'''
@@ -218,7 +261,9 @@
if e.errno == errno.ENOENT: # file doesn't exist
if autocreate:
gaf = GoogleAuthenticatorFile()
- gaf.options['RATE_LIMIT'] = '3 30'
+ gaf.options['RATE_LIMIT'] = '{} {}'.format(
+ asint(config.get('auth.multifactor.rate_limit.num', 3)),
+ asint(config.get('auth.multifactor.rate_limit.time', 30)))
gaf.options['DISALLOW_REUSE'] = None
gaf.options['TOTP_AUTH'] = None
return gaf
@@ -231,8 +276,30 @@
with open(self.config_file(user), 'w') as f:
f.write(gaf.dump())
+ def enforce_rate_limit(self, user, existing_gaf=None):
+ if existing_gaf:
+ gaf = existing_gaf
+ else:
+ gaf = self.read_file(user)
+ if not gaf:
+ return
+ rate_limits = gaf.options['RATE_LIMIT'].split(' ')
+ num_allowed = int(rate_limits.pop(0))
+ time_allowed = int(rate_limits.pop(0))
+ prev_attempts = map(int, rate_limits)
-class GoogleAuthenticatorPamFilesystemTotpService(TotpService, GoogleAuthenticatorPamFilesystemMixin):
+ ok, attempts_in_limit = check_rate_limit(num_allowed, time_allowed, prev_attempts)
+
+ gaf.options['RATE_LIMIT'] = ' '.join(map(str, [num_allowed, time_allowed] + attempts_in_limit))
+
+ if not existing_gaf:
+ self.write_file(user, gaf)
+
+ if not ok:
+ raise MultifactorRateLimitError
+
+
+class GoogleAuthenticatorPamFilesystemTotpService(GoogleAuthenticatorPamFilesystemMixin, TotpService):
'''
Store in home directories, compatible with the TOTP PAM module for Google Authenticator
https://github.com/google/google-authenticator/tree/master/libpam
@@ -315,14 +382,17 @@
def verify_and_remove_code(self, user, code):
'''
+ Verify and remove recovery codes. Also check for rate limiting.
+
:param user: a :class:`User <allura.model.auth.User>`
:param code: str
:raises: InvalidRecoveryCode
+ :raises: MultifactorRateLimitError
'''
raise NotImplementedError('verify_and_remove_code')
-class MongodbRecoveryCodeService(RecoveryCodeService):
+class MongodbRecoveryCodeService(MongodbMultifactorCommon, RecoveryCodeService):
def get_codes(self, user):
return [rc.code for rc in
@@ -335,6 +405,7 @@
session(rc).flush(rc)
def verify_and_remove_code(self, user, code):
+ self.enforce_rate_limit(user)
rc = RecoveryCode.query.get(user_id=user._id, code=code)
if rc:
rc.query.delete()
@@ -344,7 +415,7 @@
raise InvalidRecoveryCode
-class GoogleAuthenticatorPamFilesystemRecoveryCodeService(RecoveryCodeService, GoogleAuthenticatorPamFilesystemMixin):
+class GoogleAuthenticatorPamFilesystemRecoveryCodeService(GoogleAuthenticatorPamFilesystemMixin, RecoveryCodeService):
def get_codes(self, user):
gaf = self.read_file(user)
@@ -363,8 +434,13 @@
def verify_and_remove_code(self, user, code):
gaf = self.read_file(user)
- if gaf and code in gaf.recovery_codes:
- gaf.recovery_codes.remove(code)
- self.write_file(user, gaf)
- return True
+ if gaf:
+ try:
+ self.enforce_rate_limit(user, gaf)
+ if code in gaf.recovery_codes:
+ gaf.recovery_codes.remove(code)
+ return True
+ finally:
+ # write both rate limit & recovery code changes
+ self.write_file(user, gaf)
raise InvalidRecoveryCode
diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py
index 2b957b9..8d09d8c 100644
--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -2204,6 +2204,34 @@
assert_equal(r.session['username'], 'test-admin')
assert r.location.endswith('/p/foo'), r
+ def test_login_rate_limit(self):
+ self._init_totp()
+
+ # so test-admin isn't automatically logged in for all requests
+ self.app.extra_environ = {'disable_auth_magic': 'True'}
+
+ # regular login
+ r = self.app.get('/auth/?return_to=/p/foo')
+ r.form['username'] = 'test-admin'
+ r.form['password'] = 'foo'
+ r = r.form.submit()
+ r = r.follow()
+
+ # try some invalid codes
+ for i in xrange(3):
+ r.form['code'] = 'invalid-code'
+ r = r.form.submit()
+ assert_in('Invalid code', r)
+
+ # use a valid code, but it'll hit rate limit
+ totp = TotpService().Totp(self.sample_key)
+ code = totp.generate(time_time())
+ r.form['code'] = code
+ r = r.form.submit()
+
+ assert_in('rate limit exceeded', r)
+ assert not r.session.get('username')
+
def test_login_totp_disrupted(self):
self._init_totp()
diff --git a/Allura/allura/tests/test_multifactor.py b/Allura/allura/tests/test_multifactor.py
index 3c7bb03..6b28c79 100644
--- a/Allura/allura/tests/test_multifactor.py
+++ b/Allura/allura/tests/test_multifactor.py
@@ -19,15 +19,15 @@
import os
import bson
-from allura.lib.exceptions import InvalidRecoveryCode
from paste.deploy.converters import asint
-
import ming
from cryptography.hazmat.primitives.twofactor import InvalidToken
from mock import patch, Mock
from nose.tools import assert_equal, assert_raises
from tg import config
+from allura import model as M
+from allura.lib.exceptions import InvalidRecoveryCode, MultifactorRateLimitError
from allura.lib.multifactor import GoogleAuthenticatorFile, TotpService, MongodbTotpService
from allura.lib.multifactor import GoogleAuthenticatorPamFilesystemTotpService
from allura.lib.multifactor import RecoveryCodeService, MongodbRecoveryCodeService
@@ -75,6 +75,11 @@
assert_equal(gaf.dump(), self.sample2)
+class GenericTotpService(TotpService):
+ def enforce_rate_limit(self, *args, **kwargs):
+ pass
+
+
class TestTotpService(object):
sample_key = b'\x00K\xda\xbfv\xc2B\xaa\x1a\xbe\xa5\x96b\xb2\xa0Z:\xc9\xcf\x8a'
@@ -88,28 +93,28 @@
@patch('allura.lib.multifactor.time')
def test_verify_types(self, time):
time.return_value = self.sample_time
- srv = TotpService()
+ srv = GenericTotpService()
totp = srv.Totp(key=self.sample_key)
- srv.verify(totp, u'283 397')
- srv.verify(totp, b'283397')
+ srv.verify(totp, u'283 397', None)
+ srv.verify(totp, b'283397', None)
@patch('allura.lib.multifactor.time')
def test_verify_window(self, time):
time.return_value = self.sample_time
- srv = TotpService()
+ srv = GenericTotpService()
totp = srv.Totp(key=self.sample_key)
- srv.verify(totp, b'283397')
+ srv.verify(totp, b'283397', None)
time.return_value = self.sample_time + 30
- srv.verify(totp, b'283397')
+ srv.verify(totp, b'283397', None)
time.return_value = self.sample_time + 60
with assert_raises(InvalidToken):
- srv.verify(totp, b'283397')
+ srv.verify(totp, b'283397', None)
time.return_value = self.sample_time - 30
with assert_raises(InvalidToken):
- srv.verify(totp, b'283397')
+ srv.verify(totp, b'283397', None)
def test_get_qr_code(self):
srv = TotpService()
@@ -119,8 +124,57 @@
assert srv.get_qr_code(totp, user)
-class TestMongodbTotpService():
+class TestAnyTotpServiceImplementation(object):
+
+ __test__ = False
+
sample_key = b'\x00K\xda\xbfv\xc2B\xaa\x1a\xbe\xa5\x96b\xb2\xa0Z:\xc9\xcf\x8a'
+ sample_time = 1472502664
+ # these generate code 283397
+
+ def mock_user(self):
+ return M.User(username='some-user-guy')
+
+ def test_none(self):
+ srv = self.Service()
+ user = self.mock_user()
+ assert_equal(None, srv.get_secret_key(user))
+
+ def test_set_get(self):
+ srv = self.Service()
+ user = self.mock_user()
+ srv.set_secret_key(user, self.sample_key)
+ assert_equal(self.sample_key, srv.get_secret_key(user))
+
+ def test_delete(self):
+ srv = self.Service()
+ user = self.mock_user()
+ srv.set_secret_key(user, self.sample_key)
+ assert_equal(self.sample_key, srv.get_secret_key(user))
+ srv.set_secret_key(user, None)
+ assert_equal(None, srv.get_secret_key(user))
+
+ @patch('allura.lib.multifactor.time')
+ def test_rate_limiting(self, time):
+ time.return_value = self.sample_time
+ srv = self.Service()
+ user = self.mock_user()
+ totp = srv.Totp(key=self.sample_key)
+
+ # 4th attempt (good or bad) will trip over the default limit of 3 in 30s
+ with assert_raises(InvalidToken):
+ srv.verify(totp, b'34dfvdasf', user)
+ with assert_raises(InvalidToken):
+ srv.verify(totp, b'234asdfsadf', user)
+ srv.verify(totp, b'283397', user)
+ with assert_raises(MultifactorRateLimitError):
+ srv.verify(totp, b'283397', user)
+
+
+class TestMongodbTotpService(TestAnyTotpServiceImplementation):
+
+ __test__ = True
+ Service = MongodbTotpService
def setUp(self):
config = {
@@ -128,31 +182,6 @@
}
ming.configure(**config)
- def test_none(self):
- srv = MongodbTotpService()
- user = Mock(_id=bson.ObjectId(),
- is_anonymous=lambda: False,
- )
- assert_equal(None, srv.get_secret_key(user))
-
- def test_set_get(self):
- srv = MongodbTotpService()
- user = Mock(_id=bson.ObjectId(),
- is_anonymous=lambda: False,
- )
- srv.set_secret_key(user, self.sample_key)
- assert_equal(self.sample_key, srv.get_secret_key(user))
-
- def test_delete(self):
- srv = MongodbTotpService()
- user = Mock(_id=bson.ObjectId(),
- is_anonymous=lambda: False,
- )
- srv.set_secret_key(user, self.sample_key)
- assert_equal(self.sample_key, srv.get_secret_key(user))
- srv.set_secret_key(user, None)
- assert_equal(None, srv.get_secret_key(user))
-
class TestGoogleAuthenticatorPamFilesystemMixin(object):
@@ -165,28 +194,17 @@
shutil.rmtree(self.totp_basedir)
-class TestGoogleAuthenticatorPamFilesystemTotpService(TestGoogleAuthenticatorPamFilesystemMixin):
+class TestGoogleAuthenticatorPamFilesystemTotpService(TestAnyTotpServiceImplementation,
+ TestGoogleAuthenticatorPamFilesystemMixin):
- sample_key = b'\x00K\xda\xbfv\xc2B\xaa\x1a\xbe\xa5\x96b\xb2\xa0Z:\xc9\xcf\x8a'
+ __test__ = True
+ Service = GoogleAuthenticatorPamFilesystemTotpService
- def test_none(self):
- srv = GoogleAuthenticatorPamFilesystemTotpService()
- user = Mock(username='some-user-guy')
- assert_equal(None, srv.get_secret_key(user))
-
- def test_set_get(self):
- srv = GoogleAuthenticatorPamFilesystemTotpService()
- user = Mock(username='some-user-guy')
- srv.set_secret_key(user, self.sample_key)
- assert_equal(self.sample_key, srv.get_secret_key(user))
-
- def test_delete(self):
- srv = GoogleAuthenticatorPamFilesystemTotpService()
- user = Mock(username='some-user-guy')
- srv.set_secret_key(user, self.sample_key)
- assert_equal(self.sample_key, srv.get_secret_key(user))
- srv.set_secret_key(user, None)
- assert_equal(None, srv.get_secret_key(user))
+ def test_rate_limiting(self):
+ # make a regular .google-authenticator file first, so rate limit info has somewhere to go
+ self.Service().set_secret_key(self.mock_user(), self.sample_key)
+ # then run test
+ super(TestGoogleAuthenticatorPamFilesystemTotpService, self).test_rate_limiting()
class TestRecoveryCodeService(object):
@@ -215,6 +233,9 @@
__test__ = False
+ def mock_user(self):
+ return M.User(username='some-user-guy')
+
def test_get_codes_none(self):
recovery = self.Service()
user = self.mock_user()
@@ -256,6 +277,24 @@
assert_equal(result, True)
assert_equal(recovery.get_codes(user), ['67890'])
+ def test_rate_limiting(self):
+ recovery = self.Service()
+ user = self.mock_user()
+ codes = [
+ '11111',
+ '22222',
+ ]
+ recovery.replace_codes(user, codes)
+
+ # 4th attempt (good or bad) will trip over the default limit of 3 in 30s
+ with assert_raises(InvalidRecoveryCode):
+ recovery.verify_and_remove_code(user, '13485u0233')
+ with assert_raises(InvalidRecoveryCode):
+ recovery.verify_and_remove_code(user, '34123rdxafs')
+ recovery.verify_and_remove_code(user, '11111')
+ with assert_raises(MultifactorRateLimitError):
+ recovery.verify_and_remove_code(user, '22222')
+
class TestMongodbRecoveryCodeService(TestAnyRecoveryCodeServiceImplementation):
@@ -269,9 +308,6 @@
}
ming.configure(**config)
- def mock_user(self):
- return Mock(_id=bson.ObjectId())
-
class TestGoogleAuthenticatorPamFilesystemRecoveryCodeService(TestAnyRecoveryCodeServiceImplementation,
TestGoogleAuthenticatorPamFilesystemMixin):
@@ -280,9 +316,6 @@
Service = GoogleAuthenticatorPamFilesystemRecoveryCodeService
- def mock_user(self):
- return Mock(username='some-user-guy')
-
def setUp(self):
super(TestGoogleAuthenticatorPamFilesystemRecoveryCodeService, self).setUp()
