Merge branch 'brew-1941-httponly'

CHANGELOG.rst

@@ -32,8 +32,13 @@ API changes and deprecations
 * Cross-Origin Resource Sharing (CORS) support is no longer enabled by
   default. Previously, Access-Control-Allow-* response headers were added for
   all requests, with Access-Control-Allow-Origin set to the wildcard value
-  ``*``. To re-enable CORS, use the new ``ckan.cors`` settings detailed in the
-  Config File Options documentation (:doc:`/maintaining/configuration`)
+  ``*``. To re-enable CORS, use the new ``ckan.cors`` configuration settings
+  (:ref:`ckan.cors.origin_allow_all` and :ref:`ckan.cors.origin_whitelist`).
+
+* The HttpOnly flag will be set on the authorization cookie by default. For
+  enhanced security, we recommend using the HttpOnly flag, but this behaviour
+  can be changed in the ``Repoze.who`` settings detailed in the Config File
+  Options documentation (:ref:`who.httponly`).
 
 Template changes
 ----------------

ckan/config/middleware.py

@@ -18,7 +18,6 @@
 from routes.middleware import RoutesMiddleware
 from repoze.who.config import WhoConfig
 from repoze.who.middleware import PluggableAuthenticationMiddleware
-from repoze.who.plugins.auth_tkt import make_plugin as auth_tkt_make_plugin
 from fanstatic import Fanstatic
 
 from ckan.plugins import PluginImplementations
@@ -192,11 +191,6 @@ def make_app(conf, full_stack=True, static_files=True, **app_conf):
 
     return app
 
-def ckan_auth_tkt_make_app(**kw):
-    if not len(kw.get('secret', '')) or kw.get('secret') == 'somesecret':
-        kw['secret'] = config['beaker.session.secret']
-    return auth_tkt_make_plugin(**kw)
-
 
 class I18nMiddleware(object):
     """I18n Middleware selects the language based on the url

ckan/config/who.ini

@@ -1,5 +1,5 @@
 [plugin:auth_tkt]
-use = ckan.config.middleware:ckan_auth_tkt_make_app
+use = ckan.lib.auth_tkt:make_plugin
 # If no secret key is defined here, beaker.session.secret will be used
 #secret = somesecret
 

ckan/lib/auth_tkt.py

@@ -0,0 +1,75 @@
+import os
+
+from pylons import config
+from repoze.who.plugins import auth_tkt as repoze_auth_tkt
+
+_bool = repoze_auth_tkt._bool
+
+import logging
+log = logging.getLogger(__name__)
+
+
+class CkanAuthTktCookiePlugin(repoze_auth_tkt.AuthTktCookiePlugin):
+
+    def __init__(self, httponly, *args, **kwargs):
+        super(CkanAuthTktCookiePlugin, self).__init__(*args, **kwargs)
+        self.httponly = httponly
+
+    def _get_cookies(self, *args, **kwargs):
+        '''
+        Override method in superclass to ensure HttpOnly is set appropriately.
+        '''
+        super_cookies = super(CkanAuthTktCookiePlugin, self). \
+            _get_cookies(*args, **kwargs)
+
+        cookies = []
+        for k, v in super_cookies:
+            replace_with = '; HttpOnly' if self.httponly else ''
+            v = v.replace('; HttpOnly', '') + replace_with
+            cookies.append((k, v))
+
+        return cookies
+
+
+def make_plugin(secret=None,
+                secretfile=None,
+                cookie_name='auth_tkt',
+                secure=False,
+                include_ip=False,
+                timeout=None,
+                reissue_time=None,
+                userid_checker=None):
+    from repoze.who.utils import resolveDotted
+
+    # ckan specific: get secret from beaker setting if necessary
+    if secret is None or secret == 'somesecret':
+        secret = config['beaker.session.secret']
+
+    # Set httponly based on config value. Default is True
+    httponly = config.get('who.httponly', True)
+
+    # back to repoze boilerplate
+    if (secret is None and secretfile is None):
+        raise ValueError("One of 'secret' or 'secretfile' must not be None.")
+    if (secret is not None and secretfile is not None):
+        raise ValueError("Specify only one of 'secret' or 'secretfile'.")
+    if secretfile:
+        secretfile = os.path.abspath(os.path.expanduser(secretfile))
+        if not os.path.exists(secretfile):
+            raise ValueError("No such 'secretfile': %s" % secretfile)
+        secret = open(secretfile).read().strip()
+    if timeout:
+        timeout = int(timeout)
+    if reissue_time:
+        reissue_time = int(reissue_time)
+    if userid_checker is not None:
+        userid_checker = resolveDotted(userid_checker)
+    plugin = CkanAuthTktCookiePlugin(_bool(httponly),
+                                     secret,
+                                     cookie_name,
+                                     _bool(secure),
+                                     _bool(include_ip),
+                                     timeout,
+                                     reissue_time,
+                                     userid_checker)
+    return plugin

ckan/new_tests/lib/test_auth_tkt.py

@@ -0,0 +1,66 @@
+from ckan.new_tests import helpers
+from ckan.lib.auth_tkt import CkanAuthTktCookiePlugin, make_plugin
+
+
+class TestCkanAuthTktCookiePlugin(object):
+
+    '''
+    Test the added methods used by this subclass of
+    repoze.who.plugins.auth_tkt.AuthTktCookiePlugin
+    '''
+
+    def _make_plugin(self, httponly):
+        '''Only httponly needs to be set.'''
+        return CkanAuthTktCookiePlugin(httponly=httponly,
+                                       secret=None,
+                                       cookie_name='auth_tkt',
+                                       secure=False,
+                                       include_ip=False,
+                                       timeout=None,
+                                       reissue_time=None,
+                                       userid_checker=None)
+
+    @helpers.change_config('who.httponly', True)
+    def test_httponly_expected_cookies_with_config_httponly_true(self):
+        '''
+        The returned cookies are in the format we expect, with HttpOnly flag.
+        '''
+        plugin = make_plugin(secret='sosecret')
+        cookies = plugin._get_cookies(environ={'SERVER_NAME': '0.0.0.0'},
+                                      value='HELLO')
+        expected_cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=0.0.0.0; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.0.0.0.0; HttpOnly')
+        ]
+        assert cookies == expected_cookies
+
+    @helpers.change_config('who.httponly', False)
+    def test_httponly_expected_cookies_with_config_httponly_false(self):
+        '''
+        The returned cookies are in the format we expect, without HttpOnly
+        flag.
+        '''
+        plugin = make_plugin(secret='sosecret')
+        cookies = plugin._get_cookies(environ={'SERVER_NAME': '0.0.0.0'},
+                                      value='HELLO')
+        expected_cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=0.0.0.0'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.0.0.0.0')
+        ]
+        assert cookies == expected_cookies
+
+    def test_httponly_expected_cookies_without_config_httponly(self):
+        '''
+        The returned cookies are in the format we expect, with HttpOnly flag.
+        '''
+        plugin = make_plugin(secret='sosecret')
+        cookies = plugin._get_cookies(environ={'SERVER_NAME': '0.0.0.0'},
+                                      value='HELLO')
+        expected_cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=0.0.0.0; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.0.0.0.0; HttpOnly')
+        ]
+        assert cookies == expected_cookies

doc/maintaining/configuration.rst

@@ -60,6 +60,22 @@ files, and enables CKAN templates' debugging features.
    commands.
 
 
+Repoze.who Settings
+-------------------
+
+.. _who.httponly:
+
+who.httponly
+^^^^^^^^^^^^
+
+Default value: True
+
+This determines whether the HttpOnly flag will be set on the repoze.who
+authorization cookie. The default in the absence of the setting is ``True``.
+For enhanced security it is recommended to use the HttpOnly flag and not set
+this to ``False``, unless you have a good reason for doing so.
+
+
 Database Settings
 -----------------