launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-loggerhead-secure-cookie into launchpad:master.

Commit message:
Replace Loggerhead cookie handling using secure-cookie

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/399127

paste.auth.cookie is deprecated, and getting it to work with Python 3 is cumbersome at best.  Switch to secure-cookie, which seems to have a nicer API for our purposes, appears to be better-maintained (by the Werkzeug folks), and allows us to fix a long-standing minor deficiency around deleting session cookies on logout.

Dependencies MP: https://code.launchpad.net/~cjwatson/lp-source-dependencies/+git/lp-source-dependencies/+merge/399126
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-loggerhead-secure-cookie into launchpad:master.

diff --git a/lib/launchpad_loggerhead/session.py b/lib/launchpad_loggerhead/session.py
index c78c021..2adb69c 100644
--- a/lib/launchpad_loggerhead/session.py
+++ b/lib/launchpad_loggerhead/session.py
@@ -1,13 +1,36 @@
 # Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Simple paste-y session manager tuned for the needs of launchpad-loggerhead.
-"""
+"""Simple session manager tuned for the needs of launchpad-loggerhead."""
 
+import hashlib
 import pickle
 
-from paste.auth.cookie import AuthCookieHandler
-import six
+from secure_cookie.cookie import SecureCookie
+from werkzeug.http import (
+    dump_cookie,
+    parse_cookie,
+    )
+
+from lp.services.config import config
+
+
+class LaunchpadSecureCookie(SecureCookie):
+
+    # The default of sha1 is a bit too weak.
+    hash_method = staticmethod(hashlib.sha256)
+
+    # The OpenID consumer stores non-JSON-encodable objects in the session.
+    class serialization_method(object):
+
+        @classmethod
+        def dumps(cls, value):
+            # Use protocol 2 for Python 2 compatibility.
+            return pickle.dumps(value, protocol=2)
+
+        @classmethod
+        def loads(cls, value):
+            return pickle.loads(value)
 
 
 class SessionHandler(object):
@@ -30,40 +53,33 @@ class SessionHandler(object):
             SessionHandler.
         """
         self.application = application
-        self.cookie_handler = AuthCookieHandler(
-            self._process, scanlist=[session_var], secret=secret)
         self.session_var = session_var
+        self._secret = secret
+        self.cookie_name = "%s.lh" % config.launchpad_session.cookie
 
     def __call__(self, environ, start_response):
-        # We need to put the request through the cookie handler first, so we
-        # can access the validated string in the environ in `_process` below.
-        return self.cookie_handler(environ, start_response)
-
-    def _process(self, environ, start_response):
-        """Process a request.
-
-        AuthCookieHandler takes care of getting the text value of the session
-        in and out of the cookie (and validating the text using HMAC) so we
-        just need to convert that string to and from a real dictionary using
-        pickle.
-        """
-        if self.session_var in environ:
-            session = pickle.loads(six.ensure_binary(
-                environ[self.session_var], encoding="ISO-8859-1"))
-        else:
-            session = {}
+        """Process a request."""
+        cookie = parse_cookie(environ).get(self.cookie_name, "")
+        session = LaunchpadSecureCookie.unserialize(cookie, self._secret)
         existed = bool(session)
         environ[self.session_var] = session
+
         def response_hook(status, response_headers, exc_info=None):
             session = environ.pop(self.session_var)
-            # paste.auth.cookie does not delete cookies (see
-            # http://trac.pythonpaste.org/pythonpaste/ticket/139).  A
-            # reasonable workaround is to make the value empty.  Therefore,
-            # we explicitly set the value in the session (to be encrypted)
-            # if the value is non-empty *or* if it was non-empty at the start
-            # of the request.
-            if existed or session:
-                # Use protocol 2 for Python 2 compatibility.
-                environ[self.session_var] = pickle.dumps(session, protocol=2)
+            cookie_kwargs = {
+                "path": "/",
+                "httponly": True,
+                "secure": environ["wsgi.url_scheme"] == "https",
+                }
+            if session:
+                cookie = dump_cookie(
+                    self.cookie_name, session.serialize(), **cookie_kwargs)
+                response_headers.append(("Set-Cookie", cookie))
+            elif existed:
+                # Delete the cookie.
+                cookie = dump_cookie(
+                    self.cookie_name, "", expires=0, **cookie_kwargs)
+                response_headers.append(("Set-Cookie", cookie))
             return start_response(status, response_headers, exc_info)
+
         return self.application(environ, response_hook)
diff --git a/lib/launchpad_loggerhead/tests.py b/lib/launchpad_loggerhead/tests.py
index 4c8482c..f0b8244 100644
--- a/lib/launchpad_loggerhead/tests.py
+++ b/lib/launchpad_loggerhead/tests.py
@@ -73,7 +73,7 @@ class TestLogout(TestCase):
         app = session_scribbler(app, self)
         app = HTTPExceptionHandler(app)
         app = SessionHandler(app, SESSION_VAR, SECRET)
-        self.cookie_name = app.cookie_handler.cookie_name
+        self.cookie_name = app.cookie_name
         self.browser = Browser(wsgi_app=app)
         self.browser.open(
             config.codehosting.secure_codebrowse_root + '+login')
@@ -95,10 +95,7 @@ class TestLogout(TestCase):
         self.assertEqual(
             self.browser.url, allvhosts.configs['mainsite'].rooturl)
 
-        # The session cookie still exists, because of how
-        # paste.auth.cookie works (see
-        # http://trac.pythonpaste.org/pythonpaste/ticket/139 ) but the user
-        # does in fact have an empty session now.
+        # The user has an empty session now.
         self.browser.open(
             config.codehosting.secure_codebrowse_root + 'favicon.ico')
         self.assertEqual(self.session, {})
diff --git a/requirements/launchpad.txt b/requirements/launchpad.txt
index 3444639..35eb07d 100644
--- a/requirements/launchpad.txt
+++ b/requirements/launchpad.txt
@@ -136,6 +136,7 @@ requests-toolbelt==0.9.1
 responses==0.9.0
 s3transfer==0.3.3
 scandir==1.7
+secure-cookie==0.1.0
 service-identity==18.1.0
 setproctitle==1.1.7
 setuptools-git==1.2
@@ -169,6 +170,7 @@ waitress==1.3.1
 webencodings==0.5.1
 WebOb==1.8.5
 WebTest==2.0.35
+Werkzeug==1.0.1
 wsgi-intercept==1.9.2
 WSGIProxy2==0.4.6
 wsgiref==0.1.2
diff --git a/setup.py b/setup.py
index 1e7495d..4114644 100644
--- a/setup.py
+++ b/setup.py
@@ -226,6 +226,7 @@ setup(
         'requests-toolbelt',
         'responses',
         'scandir',
+        'secure-cookie',
         'setproctitle',
         'setuptools',
         'six',
@@ -247,6 +248,7 @@ setup(
         'wadllib',
         'WebOb',
         'WebTest',
+        'Werkzeug',
         'WSGIProxy2',
         'z3c.ptcompat',
         'zc.zservertracelog',