diff --git a/beaker/docs/modules/session.rst b/beaker/docs/modules/session.rst index 05aab40a..b9246329 100644 --- a/beaker/docs/modules/session.rst +++ b/beaker/docs/modules/session.rst @@ -1,4 +1,4 @@ -:mod:`beaker.session` -- Session classes +:mod:`beaker.session` -- Session classes ======================================== .. automodule:: beaker.session @@ -13,3 +13,4 @@ Module Contents .. autoclass:: SessionObject :members: persist, get_by_id, accessed .. autoclass:: SignedCookie +.. autodata:: InvalidSignature diff --git a/beaker/session.py b/beaker/session.py index 69627308..86c9d70c 100644 --- a/beaker/session.py +++ b/beaker/session.py @@ -9,7 +9,19 @@ from beaker.exceptions import BeakerException, InvalidCryptoBackendError from beaker.cookie import SimpleCookie -__all__ = ['SignedCookie', 'Session'] +__all__ = ['SignedCookie', 'Session', 'InvalidSignature'] + + +class _InvalidSignatureType(object): + """Returned from SignedCookie when the value's signature was invalid.""" + def __nonzero__(self): + return False + + def __bool__(self): + return False + + +InvalidSignature = _InvalidSignatureType() try: @@ -50,19 +62,22 @@ def __init__(self, secret, input=None): def value_decode(self, val): val = val.strip('"') + if not val: + return None, val + sig = HMAC.new(self.secret, val[40:].encode('utf-8'), SHA1).hexdigest() # Avoid timing attacks invalid_bits = 0 input_sig = val[:40] if len(sig) != len(input_sig): - return None, val + return InvalidSignature, val for a, b in zip(sig, input_sig): invalid_bits += a != b if invalid_bits: - return None, val + return InvalidSignature, val else: return val[40:], val @@ -163,14 +178,24 @@ def __init__(self, request, id=None, invalidate_corrupt=False, cookieheader = request.get('cookie', '') if secret: try: - self.cookie = SignedCookie(secret, input=cookieheader) + self.cookie = SignedCookie( + secret, + input=cookieheader, + ) except http_cookies.CookieError: - self.cookie = SignedCookie(secret, input=None) + self.cookie = SignedCookie( + secret, + input=None, + ) else: self.cookie = SimpleCookie(input=cookieheader) if not self.id and self.key in self.cookie: - self.id = self.cookie[self.key].value + cookie_data = self.cookie[self.key].value + # Should we check invalidate_corrupt here? + if cookie_data is InvalidSignature: + cookie_data = None + self.id = cookie_data self.is_new = self.id is None if self.is_new: @@ -180,7 +205,7 @@ def __init__(self, request, id=None, invalidate_corrupt=False, try: self.load() except Exception as e: - if invalidate_corrupt: + if self.invalidate_corrupt: util.warn( "Invalidating corrupt session %s; " "error was: %s. Set invalidate_corrupt=False " @@ -302,31 +327,16 @@ def _decrypt_data(self, session_data): """Bas64, decipher, then un-serialize the data for the session dict""" if self.encrypt_key: - try: - __, nonce_b64len = self.encrypt_nonce_size - nonce = session_data[:nonce_b64len] - encrypt_key = crypto.generateCryptoKeys(self.encrypt_key, - self.validate_key + nonce, 1) - payload = b64decode(session_data[nonce_b64len:]) - data = crypto.aesDecrypt(payload, encrypt_key) - except: - # As much as I hate a bare except, we get some insane errors - # here that get tossed when crypto fails, so we raise the - # 'right' exception - if self.invalidate_corrupt: - return None - else: - raise + __, nonce_b64len = self.encrypt_nonce_size + nonce = session_data[:nonce_b64len] + encrypt_key = crypto.generateCryptoKeys(self.encrypt_key, + self.validate_key + nonce, 1) + payload = b64decode(session_data[nonce_b64len:]) + data = crypto.aesDecrypt(payload, encrypt_key) else: data = b64decode(session_data) - try: - return self.serializer.loads(data) - except: - if self.invalidate_corrupt: - return None - else: - raise + return self.serializer.loads(data) def _delete_cookie(self): self.request['set_cookie'] = True @@ -526,12 +536,18 @@ class CookieSession(Session): :param encrypt_key: The key to use for the local session encryption, if not provided the session will not be encrypted. :param validate_key: The key used to sign the local encrypted session + :param invalidate_corrupt: How to handle corrupt data when loading. When + set to True, then corrupt data will be silently + invalidated and a new session created, + otherwise invalid data will cause an exception. + :type invalidate_corrupt: bool """ def __init__(self, request, key='beaker.session.id', timeout=None, save_accessed_time=True, cookie_expires=True, cookie_domain=None, cookie_path='/', encrypt_key=None, validate_key=None, secure=False, httponly=False, data_serializer='pickle', - encrypt_nonce_bits=DEFAULT_NONCE_BITS, **kwargs): + encrypt_nonce_bits=DEFAULT_NONCE_BITS, invalidate_corrupt=False, + **kwargs): if not crypto.has_aes and encrypt_key: raise InvalidCryptoBackendError("No AES library is installed, can't generate " @@ -550,7 +566,7 @@ def __init__(self, request, key='beaker.session.id', timeout=None, self.httponly = httponly self._domain = cookie_domain self._path = cookie_path - + self.invalidate_corrupt = invalidate_corrupt self._set_serializer(data_serializer) try: @@ -565,9 +581,15 @@ def __init__(self, request, key='beaker.session.id', timeout=None, raise BeakerException("timeout requires save_accessed_time") try: - self.cookie = SignedCookie(validate_key, input=cookieheader) + self.cookie = SignedCookie( + validate_key, + input=cookieheader, + ) except http_cookies.CookieError: - self.cookie = SignedCookie(validate_key, input=None) + self.cookie = SignedCookie( + validate_key, + input=None, + ) self['_id'] = _session_id() self.is_new = True @@ -577,10 +599,19 @@ def __init__(self, request, key='beaker.session.id', timeout=None, self.is_new = False try: cookie_data = self.cookie[self.key].value + if cookie_data is InvalidSignature: + raise BeakerException("Invalid signature") self.update(self._decrypt_data(cookie_data)) self._path = self.get('_path', '/') - except: - pass + except Exception as e: + if self.invalidate_corrupt: + util.warn( + "Invalidating corrupt session %s; " + "error was: %s. Set invalidate_corrupt=False " + "to propagate this exception." % (self.id, e)) + self.invalidate() + else: + raise if self.timeout is not None: now = time.time() diff --git a/tests/test_session.py b/tests/test_session.py index a0193036..45448d55 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -1,18 +1,19 @@ # -*- coding: utf-8 -*- from beaker._compat import u_, pickle +import binascii import shutil import sys import time import unittest import warnings -from nose import SkipTest +from nose import SkipTest, with_setup from beaker.container import MemoryNamespaceManager from beaker.crypto import has_aes from beaker.exceptions import BeakerException -from beaker.session import Session, SessionObject +from beaker.session import CookieSession, Session, SessionObject from beaker.util import assert_raises @@ -23,15 +24,45 @@ def get_session(**kwargs): return Session({}, **options) -def test_save_load(): +COOKIE_REQUEST = {} +def setup_cookie_request(): + COOKIE_REQUEST.clear() + + +def get_cookie_session(**kwargs): + """A shortcut for creating :class:`CookieSession` instance""" + options = {'validate_key': 'test_key'} + options.update(**kwargs) + if COOKIE_REQUEST.get('set_cookie'): + COOKIE_REQUEST['cookie'] = COOKIE_REQUEST.get('cookie_out') + return CookieSession(COOKIE_REQUEST, **options) + + +@with_setup(setup_cookie_request) +def test_session(): + for test_case in ( + check_save_load, + check_save_load_encryption, + check_decryption_failure, + check_delete, + check_revert, + check_invalidate, + check_timeout, + ): + for session_getter in (get_session, get_cookie_session,): + setup_cookie_request() + yield test_case, session_getter + + +def check_save_load(session_getter): """Test if the data is actually persistent across requests""" - session = get_session() + session = session_getter() session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id) + session = session_getter(id=session.id) assert u_('Suomi') in session assert u_('Great Britain') in session assert u_('Deutchland') in session @@ -41,18 +72,18 @@ def test_save_load(): assert session[u_('Deutchland')] == u_('Sebastian Vettel') -def test_save_load_encryption(): +def check_save_load_encryption(session_getter): """Test if the data is actually persistent across requests""" if not has_aes: raise SkipTest() - session = get_session(encrypt_key='666a19cf7f61c64c', + session = session_getter(encrypt_key='666a19cf7f61c64c', validate_key='hoobermas') session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id, encrypt_key='666a19cf7f61c64c', + session = session_getter(id=session.id, encrypt_key='666a19cf7f61c64c', validate_key='hoobermas') assert u_('Suomi') in session assert u_('Great Britain') in session @@ -63,26 +94,26 @@ def test_save_load_encryption(): assert session[u_('Deutchland')] == u_('Sebastian Vettel') -def test_decryption_failure(): +def check_decryption_failure(session_getter): """Test if the data fails without the right keys""" if not has_aes: raise SkipTest() - session = get_session(encrypt_key='666a19cf7f61c64c', + session = session_getter(encrypt_key='666a19cf7f61c64c', validate_key='hoobermas') session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id, encrypt_key='asfdasdfadsfsadf', + session = session_getter(id=session.id, encrypt_key='asfdasdfadsfsadf', validate_key='hoobermas', invalidate_corrupt=True) assert u_('Suomi') not in session assert u_('Great Britain') not in session -def test_delete(): +def check_delete(session_getter): """Test :meth:`Session.delete`""" - session = get_session() + session = session_getter() session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') @@ -93,15 +124,15 @@ def test_delete(): assert u_('Deutchland') not in session -def test_revert(): +def check_revert(session_getter): """Test :meth:`Session.revert`""" - session = get_session() + session = session_getter() session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id) + session = session_getter(id=session.id) del session[u_('Suomi')] session[u_('Great Britain')] = u_('Lewis Hamilton') session[u_('Deutchland')] = u_('Michael Schumacher') @@ -114,15 +145,17 @@ def test_revert(): assert u_('España') not in session -def test_invalidate(): +def check_invalidate(session_getter): """Test :meth:`Session.invalidate`""" - session = get_session() + session = session_getter() + session.save() id = session.id created = session.created session[u_('Suomi')] = u_('Kimi Räikkönen') session[u_('Great Britain')] = u_('Jenson Button') session[u_('Deutchland')] = u_('Sebastian Vettel') session.invalidate() + session.save() assert session.id != id assert session.created != created @@ -131,6 +164,7 @@ def test_invalidate(): assert u_('Deutchland') not in session +@with_setup(setup_cookie_request) def test_regenerate_id(): """Test :meth:`Session.regenerate_id`""" # new session & save @@ -167,9 +201,10 @@ def test_regenerate_id(): assert session[u_('foo')] == u_('bar') -def test_timeout(): +def check_timeout(session_getter): """Test if the session times out properly""" - session = get_session(timeout=2) + session = session_getter(timeout=2) + session.save() id = session.id created = session.created session[u_('Suomi')] = u_('Kimi Räikkönen') @@ -177,7 +212,7 @@ def test_timeout(): session[u_('Deutchland')] = u_('Sebastian Vettel') session.save() - session = get_session(id=session.id, timeout=2) + session = session_getter(id=session.id, timeout=2) assert session.id == id assert session.created == created assert session[u_('Suomi')] == u_('Kimi Räikkönen') @@ -185,7 +220,7 @@ def test_timeout(): assert session[u_('Deutchland')] == u_('Sebastian Vettel') time.sleep(2) - session = get_session(id=session.id, timeout=2) + session = session_getter(id=session.id, timeout=2) assert session.id != id assert session.created != created assert u_('Suomi') not in session @@ -193,6 +228,7 @@ def test_timeout(): assert u_('Deutchland') not in session +@with_setup(setup_cookie_request) def test_timeout_requires_accessed_time(): """Test that it doesn't allow setting save_accessed_time to False with timeout enabled @@ -205,6 +241,7 @@ def test_timeout_requires_accessed_time(): save_accessed_time=False) +@with_setup(setup_cookie_request) def test_cookies_enabled(): """ Test if cookies are sent out properly when ``use_cookies`` @@ -276,6 +313,7 @@ def test_cookies_disabled(): assert 'cookie_out' not in session.request +@with_setup(setup_cookie_request) def test_file_based_replace_optimization(): """Test the file-based backend with session, which includes the 'replace' optimization. @@ -321,6 +359,7 @@ def test_file_based_replace_optimization(): session.namespace.do_close() +@with_setup(setup_cookie_request) def test_invalidate_corrupt(): session = get_session(use_cookies=False, type='file', data_dir='./cache') @@ -332,7 +371,7 @@ def test_invalidate_corrupt(): f.close() assert_raises( - pickle.UnpicklingError, + (pickle.UnpicklingError, EOFError, TypeError, binascii.Error), get_session, use_cookies=False, type='file', data_dir='./cache', id=session.id @@ -344,6 +383,67 @@ def test_invalidate_corrupt(): assert "foo" not in dict(session) +@with_setup(setup_cookie_request) +def test_invalidate_empty_cookie(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = ' beaker.session.id=' + session = get_cookie_session(id=session.id, invalidate_corrupt=False, **kwargs) + assert "foo" not in dict(session) + + +@with_setup(setup_cookie_request) +def test_unrelated_cookie(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = COOKIE_REQUEST['cookie_out'] + '; some.other=cookie' + session = get_cookie_session(id=session.id, invalidate_corrupt=False, **kwargs) + assert "foo" in dict(session) + + +@with_setup(setup_cookie_request) +def test_invalidate_invalid_signed_cookie(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = ( + COOKIE_REQUEST['cookie_out'][:20] + + 'aaaaa' + + COOKIE_REQUEST['cookie_out'][25:] + ) + + assert_raises( + BeakerException, + get_cookie_session, + id=session.id, + invalidate_corrupt=False, + ) + + +@with_setup(setup_cookie_request) +def test_invalidate_invalid_signed_cookie_invalidate_corrupt(): + kwargs = {'validate_key': 'test_key', 'encrypt_key': 'encrypt'} + session = get_cookie_session(**kwargs) + session['foo'] = 'bar' + session.save() + + COOKIE_REQUEST['cookie_out'] = ( + COOKIE_REQUEST['cookie_out'][:20] + + 'aaaaa' + + COOKIE_REQUEST['cookie_out'][25:] + ) + + session = get_cookie_session(id=session.id, invalidate_corrupt=True, **kwargs) + assert "foo" not in dict(session) + class TestSaveAccessedTime(unittest.TestCase): # These tests can't use the memory session type since it seems that loading # winds up with references to the underlying storage and makes changes to