From c57aefd48b10ca3cabc9df162bc32efa62a6a21e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 21 Sep 2017 13:04:07 +0200 Subject: [PATCH] Hash authentication tokens before storing in the database. --- pillar/__init__.py | 13 +++++++++ pillar/api/eve_settings.py | 4 +++ pillar/api/local_auth.py | 18 +++++++++--- pillar/api/utils/authentication.py | 24 ++++++++++++++-- pillar/config.py | 3 ++ tests/test_api/test_auth.py | 32 ++++++++++++--------- tests/test_api/test_blender_id_subclient.py | 12 ++++++-- tests/test_api/test_local_auth.py | 4 ++- 8 files changed, 86 insertions(+), 24 deletions(-) diff --git a/pillar/__init__.py b/pillar/__init__.py index c7085df9..74031d2b 100644 --- a/pillar/__init__.py +++ b/pillar/__init__.py @@ -91,6 +91,7 @@ class PillarServer(Eve): self.log = logging.getLogger('%s.%s' % (__name__, self.__class__.__name__)) self.log.info('Creating new instance from %r', self.app_root) + self._config_auth_token_hmac_key() self._config_tempdirs() self._config_git() self._config_bugsnag() @@ -149,6 +150,18 @@ class PillarServer(Eve): if self.config['DEBUG']: log.info('Pillar starting, debug=%s', self.config['DEBUG']) + def _config_auth_token_hmac_key(self): + """Load AUTH_TOKEN_HMAC_KEY, falling back to SECRET_KEY.""" + + hmac_key = self.config.get('AUTH_TOKEN_HMAC_KEY') + if not hmac_key: + self.log.warning('AUTH_TOKEN_HMAC_KEY not set, falling back to SECRET_KEY') + hmac_key = self.config['AUTH_TOKEN_HMAC_KEY'] = self.config['SECRET_KEY'] + + if isinstance(hmac_key, str): + self.log.warning('Converting AUTH_TOKEN_HMAC_KEY to bytes') + self.config['AUTH_TOKEN_HMAC_KEY'] = hmac_key.encode('utf8') + def _config_tempdirs(self): storage_dir = self.config['STORAGE_DIR'] if not os.path.exists(storage_dir): diff --git a/pillar/api/eve_settings.py b/pillar/api/eve_settings.py index 3d67755e..4c2adc11 100644 --- a/pillar/api/eve_settings.py +++ b/pillar/api/eve_settings.py @@ -320,6 +320,10 @@ tokens_schema = { 'required': True, }, 'token': { + 'type': 'string', + 'required': False, + }, + 'token_hashed': { 'type': 'string', 'required': True, }, diff --git a/pillar/api/local_auth.py b/pillar/api/local_auth.py index c9346837..456cb383 100644 --- a/pillar/api/local_auth.py +++ b/pillar/api/local_auth.py @@ -72,13 +72,16 @@ def make_token(): return jsonify(token=token['token']) -def generate_and_store_token(user_id, days=15, prefix=b''): +def generate_and_store_token(user_id, days=15, prefix=b'') -> dict: """Generates token based on random bits. + NOTE: the returned document includes the plain-text token. + DO NOT STORE OR LOG THIS unless there is a good reason to. + :param user_id: ObjectId of the owning user. :param days: token will expire in this many days. :param prefix: the token will be prefixed by these bytes, for easy identification. - :return: the token document. + :return: the token document with the token in plain text as well as hashed. """ if not isinstance(prefix, bytes): @@ -90,10 +93,17 @@ def generate_and_store_token(user_id, days=15, prefix=b''): # Use 'xy' as altargs to prevent + and / characters from appearing. # We never have to b64decode the string anyway. - token = prefix + base64.b64encode(random_bits, altchars=b'xy').strip(b'=') + token_bytes = prefix + base64.b64encode(random_bits, altchars=b'xy').strip(b'=') + token = token_bytes.decode('ascii') token_expiry = datetime.datetime.now(tz=tz_util.utc) + datetime.timedelta(days=days) - return store_token(user_id, token.decode('ascii'), token_expiry) + token_data = store_token(user_id, token, token_expiry) + + # Include the token in the returned document so that it can be stored client-side, + # in configuration, etc. + token_data['token'] = token + + return token_data def hash_password(password: str, salt: typing.Union[str, bytes]) -> str: diff --git a/pillar/api/utils/authentication.py b/pillar/api/utils/authentication.py index deb53a93..c26aca69 100644 --- a/pillar/api/utils/authentication.py +++ b/pillar/api/utils/authentication.py @@ -5,7 +5,10 @@ unique usernames from emails. Calls out to the pillar_server.modules.blender_id module for Blender ID communication. """ +import base64 import datetime +import hmac +import hashlib import logging import typing @@ -181,8 +184,10 @@ def find_token(token, is_subclient_token=False, **extra_filters): tokens_collection = current_app.data.driver.db['tokens'] - # TODO: remove expired tokens from collection. - lookup = {'token': token, + token_hashed = hash_auth_token(token) + + # TODO: remove matching on unhashed tokens once all tokens have been hashed. + lookup = {'$or': [{'token': token}, {'token_hashed': token_hashed}], 'is_subclient_token': True if is_subclient_token else {'$in': [False, None]}, 'expire_time': {"$gt": datetime.datetime.now(tz=tz_util.utc)}} lookup.update(extra_filters) @@ -191,6 +196,19 @@ def find_token(token, is_subclient_token=False, **extra_filters): return db_token +def hash_auth_token(token: str) -> str: + """Returns the hashed authentication token. + + The token is hashed using HMAC and then base64-encoded. + """ + + hmac_key = current_app.config['AUTH_TOKEN_HMAC_KEY'] + token_hmac = hmac.new(hmac_key, msg=token.encode('utf8'), digestmod=hashlib.sha256) + digest = token_hmac.digest() + + return base64.b64encode(digest).decode('ascii') + + def store_token(user_id, token: str, token_expiry, oauth_subclient_id=False): """Stores an authentication token. @@ -201,7 +219,7 @@ def store_token(user_id, token: str, token_expiry, oauth_subclient_id=False): token_data = { 'user': user_id, - 'token': token, + 'token_hashed': hash_auth_token(token), 'expire_time': token_expiry, } if oauth_subclient_id: diff --git a/pillar/config.py b/pillar/config.py index 7c45258d..526ae4a6 100644 --- a/pillar/config.py +++ b/pillar/config.py @@ -24,6 +24,9 @@ DEBUG = False # python3 -c 'import secrets; print(secrets.token_urlsafe(128))' SECRET_KEY = '' +# Authentication token hashing key. If empty falls back to UTF8-encoded SECRET_KEY with a warning. +AUTH_TOKEN_HMAC_KEY = b'' + # Authentication settings BLENDER_ID_ENDPOINT = 'http://blender_id:8000/' diff --git a/tests/test_api/test_auth.py b/tests/test_api/test_auth.py index f864a1a4..8f16abb1 100644 --- a/tests/test_api/test_auth.py +++ b/tests/test_api/test_auth.py @@ -78,6 +78,8 @@ class AuthenticationTests(AbstractPillarTest): from pillar.api.utils import authentication as auth + self.enter_app_context() + user_id = self.create_user() now = datetime.datetime.now(tz_util.utc) @@ -85,10 +87,15 @@ class AuthenticationTests(AbstractPillarTest): past = now - datetime.timedelta(days=1) subclient = self.app.config['BLENDER_ID_SUBCLIENT_ID'] - with self.app.test_request_context(): - auth.store_token(user_id, 'nonexpired-main', future, None) - auth.store_token(user_id, 'nonexpired-sub', future, subclient) - token3 = auth.store_token(user_id, 'expired-sub', past, subclient) + auth.store_token(user_id, 'nonexpired-main', future, None) + auth.store_token(user_id, 'nonexpired-sub', future, subclient) + token3 = auth.store_token(user_id, 'expired-sub', past, subclient) + + # We should not find the given tokens as unhashed tokens. + tokens_coll = self.app.db('tokens') + self.assertIsNone(tokens_coll.find_one({'token': 'nonespired-main'})) + self.assertIsNone(tokens_coll.find_one({'token': 'nonespired-sub'})) + self.assertIsNone(tokens_coll.find_one({'token': 'expired-sub'})) with self.app.test_request_context( headers={'Authorization': self.make_header('nonexpired-main')}): @@ -172,19 +179,19 @@ class AuthenticationTests(AbstractPillarTest): with self.app.test_request_context(): from pillar.api.utils import authentication as auth - auth.store_token(user_id, 'long-expired', - now - datetime.timedelta(days=365), None) - auth.store_token(user_id, 'short-expired', - now - datetime.timedelta(seconds=5), None) - auth.store_token(user_id, 'not-expired', - now + datetime.timedelta(days=1), None) + tokdat_le = auth.store_token(user_id, 'long-expired', + now - datetime.timedelta(days=365), None) + tokdat_se = auth.store_token(user_id, 'short-expired', + now - datetime.timedelta(seconds=5), None) + tokdat_ne = auth.store_token(user_id, 'not-expired', + now + datetime.timedelta(days=1), None) # Validation should clean up old tokens. auth.validate_this_token('je', 'moeder') token_coll = self.app.data.driver.db['tokens'] - self.assertEqual({'short-expired', 'not-expired'}, - {item['token'] for item in token_coll.find()}) + self.assertEqual({tokdat_se['token_hashed'], tokdat_ne['token_hashed']}, + {item['token_hashed'] for item in token_coll.find()}) class UserListTests(AbstractPillarTest): @@ -703,7 +710,6 @@ class UserCreationTest(AbstractPillarTest): class CurrentUserTest(AbstractPillarTest): - def test_current_user_logged_in(self): self.enter_app_context() diff --git a/tests/test_api/test_blender_id_subclient.py b/tests/test_api/test_blender_id_subclient.py index eb465c5c..6047fcb5 100644 --- a/tests/test_api/test_blender_id_subclient.py +++ b/tests/test_api/test_blender_id_subclient.py @@ -42,6 +42,8 @@ class BlenderIdSubclientTest(AbstractPillarTest): @responses.activate def test_store_multiple_tokens(self): + from pillar.api.utils.authentication import hash_auth_token + scst1 = '%s-1' % TEST_SUBCLIENT_TOKEN scst2 = '%s-2' % TEST_SUBCLIENT_TOKEN db_user1 = self._common_user_test(201, scst=scst1) @@ -51,8 +53,10 @@ class BlenderIdSubclientTest(AbstractPillarTest): # Now there should be two tokens. with self.app.test_request_context(): tokens = self.app.data.driver.db['tokens'] - self.assertIsNotNone(tokens.find_one({'user': db_user1['_id'], 'token': scst1})) - self.assertIsNotNone(tokens.find_one({'user': db_user1['_id'], 'token': scst2})) + self.assertIsNotNone(tokens.find_one( + {'user': db_user1['_id'], 'token_hashed': hash_auth_token(scst1)})) + self.assertIsNotNone(tokens.find_one( + {'user': db_user1['_id'], 'token_hashed': hash_auth_token(scst2)})) # There should still be only one auth element for blender-id in the user doc. self.assertEqual(1, len(db_user1['auth'])) @@ -76,6 +80,8 @@ class BlenderIdSubclientTest(AbstractPillarTest): def _common_user_test(self, expected_status_code, scst=TEST_SUBCLIENT_TOKEN, expected_full_name=TEST_FULL_NAME, mock_happy_blender_id=True): + from pillar.api.utils.authentication import hash_auth_token + if mock_happy_blender_id: self.mock_blenderid_validate_happy() @@ -104,7 +110,7 @@ class BlenderIdSubclientTest(AbstractPillarTest): # Check that the token was succesfully stored. tokens = self.app.data.driver.db['tokens'] db_token = tokens.find_one({'user': db_user['_id'], - 'token': scst}) + 'token_hashed': hash_auth_token(scst)}) self.assertIsNotNone(db_token) return db_user diff --git a/tests/test_api/test_local_auth.py b/tests/test_api/test_local_auth.py index 5c3e8285..f327fe0b 100644 --- a/tests/test_api/test_local_auth.py +++ b/tests/test_api/test_local_auth.py @@ -38,6 +38,8 @@ class LocalAuthTest(AbstractPillarTest): self.assertEqual(200, resp.status_code, resp.data) def test_login_expired_token(self): + from pillar.api.utils.authentication import hash_auth_token + user_id = self.create_test_user() resp = self.client.post('/api/auth/make-token', @@ -52,7 +54,7 @@ class LocalAuthTest(AbstractPillarTest): tokens = self.app.data.driver.db['tokens'] exp = datetime.datetime.now(tz=tz_util.utc) - datetime.timedelta(1) - result = tokens.update_one({'token': token}, + result = tokens.update_one({'token_hashed': hash_auth_token(token)}, {'$set': {'expire_time': exp}}) self.assertEqual(1, result.modified_count)