allura-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From brond...@apache.org
Subject allura git commit: [#8126] rate limits for 2FA
Date Thu, 15 Sep 2016 14:07:48 GMT
Repository: allura
Updated Branches:
  refs/heads/master abc3b8e76 -> 9e3c139e6


[#8126] rate limits for 2FA


Project: http://git-wip-us.apache.org/repos/asf/allura/repo
Commit: http://git-wip-us.apache.org/repos/asf/allura/commit/9e3c139e
Tree: http://git-wip-us.apache.org/repos/asf/allura/tree/9e3c139e
Diff: http://git-wip-us.apache.org/repos/asf/allura/diff/9e3c139e

Branch: refs/heads/master
Commit: 9e3c139e655609476aee286bb65c802fc0f3442f
Parents: abc3b8e
Author: Dave Brondsema <dave@brondsema.net>
Authored: Thu Sep 8 15:43:36 2016 -0400
Committer: Dave Brondsema <dave@brondsema.net>
Committed: Thu Sep 15 10:07:41 2016 -0400

----------------------------------------------------------------------
 Allura/allura/controllers/auth.py           |   9 +-
 Allura/allura/lib/exceptions.py             |   4 +
 Allura/allura/lib/multifactor.py            | 100 ++++++++++++++--
 Allura/allura/tests/functional/test_auth.py |  28 +++++
 Allura/allura/tests/test_multifactor.py     | 141 ++++++++++++++---------
 5 files changed, 213 insertions(+), 69 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/allura/blob/9e3c139e/Allura/allura/controllers/auth.py
----------------------------------------------------------------------
diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index 2009515..a5031d1 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -23,7 +23,7 @@ from urlparse import urlparse, urljoin
 
 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
@@ -359,7 +359,7 @@ class AuthController(BaseController):
             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)
@@ -367,6 +367,9 @@ class AuthController(BaseController):
         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'))
@@ -719,7 +722,7 @@ class PreferencesController(BaseController):
         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.'

http://git-wip-us.apache.org/repos/asf/allura/blob/9e3c139e/Allura/allura/lib/exceptions.py
----------------------------------------------------------------------
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 @@ class ProjectRatelimitError(RatelimitError):
     pass
 
 
+class MultifactorRateLimitError(RatelimitError):
+    pass
+
+
 class ProjectPhoneVerificationError(ForgeError):
     pass
 

http://git-wip-us.apache.org/repos/asf/allura/blob/9e3c139e/Allura/allura/lib/multifactor.py
----------------------------------------------------------------------
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 @@ from time import time
 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 @@ from allura.model.multifactor import RecoveryCode
 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 @@ class TotpService(object):
 
         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 @@ class TotpService(object):
         '''
         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 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)
 
-class MongodbTotpService(TotpService):
+        if not ok:
+            raise MultifactorRateLimitError
+
+
+class MongodbTotpService(MongodbMultifactorCommon, TotpService):
     '''
     Store in TOTP keys in mongodb.
     '''
@@ -218,7 +261,9 @@ class GoogleAuthenticatorPamFilesystemMixin(object):
             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 @@ class GoogleAuthenticatorPamFilesystemMixin(object):
         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)
+
+        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(TotpService, GoogleAuthenticatorPamFilesystemMixin):
+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 @@ class RecoveryCodeService(object):
 
     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 @@ class MongodbRecoveryCodeService(RecoveryCodeService):
             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 @@ class MongodbRecoveryCodeService(RecoveryCodeService):
             raise InvalidRecoveryCode
 
 
-class GoogleAuthenticatorPamFilesystemRecoveryCodeService(RecoveryCodeService, GoogleAuthenticatorPamFilesystemMixin):
+class GoogleAuthenticatorPamFilesystemRecoveryCodeService(GoogleAuthenticatorPamFilesystemMixin,
RecoveryCodeService):
 
     def get_codes(self, user):
         gaf = self.read_file(user)
@@ -363,8 +434,13 @@ class GoogleAuthenticatorPamFilesystemRecoveryCodeService(RecoveryCodeService,
G
 
     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

http://git-wip-us.apache.org/repos/asf/allura/blob/9e3c139e/Allura/allura/tests/functional/test_auth.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py
index 232ff16..4ba27e3 100644
--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -2235,6 +2235,34 @@ class TestTwoFactor(TestController):
         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()
 

http://git-wip-us.apache.org/repos/asf/allura/blob/9e3c139e/Allura/allura/tests/test_multifactor.py
----------------------------------------------------------------------
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 textwrap
 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 @@ class TestGoogleAuthenticatorFile(object):
         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 @@ class TestTotpService(object):
     @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,40 +124,64 @@ class TestTotpService(object):
         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 setUp(self):
-        config = {
-            'ming.main.uri': 'mim://allura_test',
-        }
-        ming.configure(**config)
+    def mock_user(self):
+        return M.User(username='some-user-guy')
 
     def test_none(self):
-        srv = MongodbTotpService()
-        user = Mock(_id=bson.ObjectId(),
-                    is_anonymous=lambda: False,
-                    )
+        srv = self.Service()
+        user = self.mock_user()
         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 = 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 = MongodbTotpService()
-        user = Mock(_id=bson.ObjectId(),
-                    is_anonymous=lambda: False,
-                    )
+        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 = {
+            'ming.main.uri': 'mim://allura_test',
+        }
+        ming.configure(**config)
+
 
 class TestGoogleAuthenticatorPamFilesystemMixin(object):
 
@@ -165,28 +194,17 @@ class TestGoogleAuthenticatorPamFilesystemMixin(object):
             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'
-
-    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))
+    __test__ = True
+    Service = GoogleAuthenticatorPamFilesystemTotpService
 
-    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 @@ class TestAnyRecoveryCodeServiceImplementation(object):
 
     __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 @@ class TestAnyRecoveryCodeServiceImplementation(object):
         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 @@ class TestMongodbRecoveryCodeService(TestAnyRecoveryCodeServiceImplementation):
         }
         ming.configure(**config)
 
-    def mock_user(self):
-        return Mock(_id=bson.ObjectId())
-
 
 class TestGoogleAuthenticatorPamFilesystemRecoveryCodeService(TestAnyRecoveryCodeServiceImplementation,
                                                               TestGoogleAuthenticatorPamFilesystemMixin):
@@ -280,9 +316,6 @@ class TestGoogleAuthenticatorPamFilesystemRecoveryCodeService(TestAnyRecoveryCod
 
     Service = GoogleAuthenticatorPamFilesystemRecoveryCodeService
 
-    def mock_user(self):
-        return Mock(username='some-user-guy')
-
     def setUp(self):
         super(TestGoogleAuthenticatorPamFilesystemRecoveryCodeService, self).setUp()
 


Mime
View raw message