Merge ~cjwatson/launchpad:py3-loggerhead-secure-cookie into launchpad:master

Proposed by Colin Watson on 2021-03-03
Status: Merged
Approved by: Colin Watson on 2021-03-04
Approved revision: 8d2e473bee7b3ebd094c2207c86eaf47d5232f80
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:py3-loggerhead-secure-cookie
Merge into: launchpad:master
Diff against target: 172 lines (+54/-37)
4 files modified
lib/launchpad_loggerhead/session.py (+48/-32)
lib/launchpad_loggerhead/tests.py (+2/-5)
requirements/launchpad.txt (+2/-0)
setup.py (+2/-0)
Reviewer Review Type Date Requested Status
Cristian Gonzalez 2021-03-03 Approve on 2021-03-04
Review via email: mp+399127@code.launchpad.net

Commit message

Replace Loggerhead cookie handling using secure-cookie

Description of the change

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.

This will break existing Loggerhead sessions. I think that's OK since it's a one-time thing, but it's worth noting.

Dependencies MP: https://code.launchpad.net/~cjwatson/lp-source-dependencies/+git/lp-source-dependencies/+merge/399126

To post a comment you must log in.
8d2e473... by Colin Watson on 2021-03-03

Replace Loggerhead cookie handling using secure-cookie

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.

This will break existing Loggerhead sessions. I think that's OK since
it's a one-time thing, but it's worth noting.

Cristian Gonzalez (cristiangsp) wrote :

Nice change even with the trade off, totally worth it.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/launchpad_loggerhead/session.py b/lib/launchpad_loggerhead/session.py
2index c78c021..2adb69c 100644
3--- a/lib/launchpad_loggerhead/session.py
4+++ b/lib/launchpad_loggerhead/session.py
5@@ -1,13 +1,36 @@
6 # Copyright 2009-2010 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9-"""Simple paste-y session manager tuned for the needs of launchpad-loggerhead.
10-"""
11+"""Simple session manager tuned for the needs of launchpad-loggerhead."""
12
13+import hashlib
14 import pickle
15
16-from paste.auth.cookie import AuthCookieHandler
17-import six
18+from secure_cookie.cookie import SecureCookie
19+from werkzeug.http import (
20+ dump_cookie,
21+ parse_cookie,
22+ )
23+
24+from lp.services.config import config
25+
26+
27+class LaunchpadSecureCookie(SecureCookie):
28+
29+ # The default of sha1 is a bit too weak.
30+ hash_method = staticmethod(hashlib.sha256)
31+
32+ # The OpenID consumer stores non-JSON-encodable objects in the session.
33+ class serialization_method(object):
34+
35+ @classmethod
36+ def dumps(cls, value):
37+ # Use protocol 2 for Python 2 compatibility.
38+ return pickle.dumps(value, protocol=2)
39+
40+ @classmethod
41+ def loads(cls, value):
42+ return pickle.loads(value)
43
44
45 class SessionHandler(object):
46@@ -30,40 +53,33 @@ class SessionHandler(object):
47 SessionHandler.
48 """
49 self.application = application
50- self.cookie_handler = AuthCookieHandler(
51- self._process, scanlist=[session_var], secret=secret)
52 self.session_var = session_var
53+ self._secret = secret
54+ self.cookie_name = "%s.lh" % config.launchpad_session.cookie
55
56 def __call__(self, environ, start_response):
57- # We need to put the request through the cookie handler first, so we
58- # can access the validated string in the environ in `_process` below.
59- return self.cookie_handler(environ, start_response)
60-
61- def _process(self, environ, start_response):
62- """Process a request.
63-
64- AuthCookieHandler takes care of getting the text value of the session
65- in and out of the cookie (and validating the text using HMAC) so we
66- just need to convert that string to and from a real dictionary using
67- pickle.
68- """
69- if self.session_var in environ:
70- session = pickle.loads(six.ensure_binary(
71- environ[self.session_var], encoding="ISO-8859-1"))
72- else:
73- session = {}
74+ """Process a request."""
75+ cookie = parse_cookie(environ).get(self.cookie_name, "")
76+ session = LaunchpadSecureCookie.unserialize(cookie, self._secret)
77 existed = bool(session)
78 environ[self.session_var] = session
79+
80 def response_hook(status, response_headers, exc_info=None):
81 session = environ.pop(self.session_var)
82- # paste.auth.cookie does not delete cookies (see
83- # http://trac.pythonpaste.org/pythonpaste/ticket/139). A
84- # reasonable workaround is to make the value empty. Therefore,
85- # we explicitly set the value in the session (to be encrypted)
86- # if the value is non-empty *or* if it was non-empty at the start
87- # of the request.
88- if existed or session:
89- # Use protocol 2 for Python 2 compatibility.
90- environ[self.session_var] = pickle.dumps(session, protocol=2)
91+ cookie_kwargs = {
92+ "path": "/",
93+ "httponly": True,
94+ "secure": environ["wsgi.url_scheme"] == "https",
95+ }
96+ if session:
97+ cookie = dump_cookie(
98+ self.cookie_name, session.serialize(), **cookie_kwargs)
99+ response_headers.append(("Set-Cookie", cookie))
100+ elif existed:
101+ # Delete the cookie.
102+ cookie = dump_cookie(
103+ self.cookie_name, "", expires=0, **cookie_kwargs)
104+ response_headers.append(("Set-Cookie", cookie))
105 return start_response(status, response_headers, exc_info)
106+
107 return self.application(environ, response_hook)
108diff --git a/lib/launchpad_loggerhead/tests.py b/lib/launchpad_loggerhead/tests.py
109index 4c8482c..f0b8244 100644
110--- a/lib/launchpad_loggerhead/tests.py
111+++ b/lib/launchpad_loggerhead/tests.py
112@@ -73,7 +73,7 @@ class TestLogout(TestCase):
113 app = session_scribbler(app, self)
114 app = HTTPExceptionHandler(app)
115 app = SessionHandler(app, SESSION_VAR, SECRET)
116- self.cookie_name = app.cookie_handler.cookie_name
117+ self.cookie_name = app.cookie_name
118 self.browser = Browser(wsgi_app=app)
119 self.browser.open(
120 config.codehosting.secure_codebrowse_root + '+login')
121@@ -95,10 +95,7 @@ class TestLogout(TestCase):
122 self.assertEqual(
123 self.browser.url, allvhosts.configs['mainsite'].rooturl)
124
125- # The session cookie still exists, because of how
126- # paste.auth.cookie works (see
127- # http://trac.pythonpaste.org/pythonpaste/ticket/139 ) but the user
128- # does in fact have an empty session now.
129+ # The user has an empty session now.
130 self.browser.open(
131 config.codehosting.secure_codebrowse_root + 'favicon.ico')
132 self.assertEqual(self.session, {})
133diff --git a/requirements/launchpad.txt b/requirements/launchpad.txt
134index 3444639..35eb07d 100644
135--- a/requirements/launchpad.txt
136+++ b/requirements/launchpad.txt
137@@ -136,6 +136,7 @@ requests-toolbelt==0.9.1
138 responses==0.9.0
139 s3transfer==0.3.3
140 scandir==1.7
141+secure-cookie==0.1.0
142 service-identity==18.1.0
143 setproctitle==1.1.7
144 setuptools-git==1.2
145@@ -169,6 +170,7 @@ waitress==1.3.1
146 webencodings==0.5.1
147 WebOb==1.8.5
148 WebTest==2.0.35
149+Werkzeug==1.0.1
150 wsgi-intercept==1.9.2
151 WSGIProxy2==0.4.6
152 wsgiref==0.1.2
153diff --git a/setup.py b/setup.py
154index 1e7495d..4114644 100644
155--- a/setup.py
156+++ b/setup.py
157@@ -226,6 +226,7 @@ setup(
158 'requests-toolbelt',
159 'responses',
160 'scandir',
161+ 'secure-cookie',
162 'setproctitle',
163 'setuptools',
164 'six',
165@@ -247,6 +248,7 @@ setup(
166 'wadllib',
167 'WebOb',
168 'WebTest',
169+ 'Werkzeug',
170 'WSGIProxy2',
171 'z3c.ptcompat',
172 'zc.zservertracelog',

Subscribers

People subscribed via source and target branches

to status/vote changes: