From 6b5ebebd04b65509d3fb0d7ce4011ce2ee356ed2 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 7 Aug 2024 15:00:31 +0100 Subject: [PATCH] Use a separate auth cookie for API requests Use two separate auth cookies to authenticate requests for HTML pages (these use the pre-existing auth cookie) and requests from h's own frontend code to h's API endpoints (these use a new, separate API auth cookie that's introduced by this commit). This enables the API auth cookie to be more secure. In particular, the API auth cookie can use SameSite=Strict rather than Lax, which gives better protection against CSRF attacks but would not be desirable for the HTML auth cookie (because whenever a user followed a link from another site to Hypothesis they would find themselves not authenticated). How this works: `CookiePolicy` (which is used for HTML page requests) sets and deletes both the HTML auth cookie and the API auth cookie on log in and log out, but reads only the HTML auth cookie to authenticate HTML page requests. `APICookiePolicy` reads only the API auth cookie to authenticate API requests, and never sets or deletes either cookie. In detail: 1. Recall that the top-level security policy (`TopLevelPolicy`) delegates all method calls to either `CookiePolicy` (for HTML endpoints) or `APIPolicy` (for API endpoints). 2. When a user logs in the view calls `pyramid.security.remember()` which calls `TopLevelPolicy.remember()`. The login form submission is an HTML endpoint so `TopLevelPolicy.remember()` delegates to `CookiePolicy.remember()` which sets both the HTML and API auth cookies. 3. Similarly when the user logs out `CookiePolicy.forget()` deletes both the HTML and API auth cookies. 4. To authenticate HTML page requests `CookiePolicy.identity()` reads only the HTML auth cookie. 5. To authenticate API requests `TopLevelPolicy` delegates to `APIPolicy` which delegates to a list of API subpolicies (`BearerTokenPolicy`, `AuthClientPolicy` and `APICookiePolicy`). If there's no bearer token or auth client authentication in the request then `APICookiePolicy` reads only the API auth cookie to authenticate API requests. --- h/config.py | 13 ++ h/security/__init__.py | 5 + h/security/policy/_cookie.py | 85 +++++++- h/security/policy/top_level.py | 27 ++- tests/functional/conftest.py | 4 + tests/unit/h/config_test.py | 2 + tests/unit/h/security/policy/_cookie_test.py | 192 ++++++++++++++++-- .../unit/h/security/policy/top_level_test.py | 67 ++++-- tox.ini | 2 + 9 files changed, 347 insertions(+), 50 deletions(-) diff --git a/h/config.py b/h/config.py index 632285c2aa6..ce05c3e0d1f 100644 --- a/h/config.py +++ b/h/config.py @@ -133,6 +133,19 @@ def configure(environ=None, settings=None): # pylint: disable=too-many-statemen settings_manager.set("mail.port", "MANDRILL_PORT", default=587) settings_manager.set("mail.tls", "MANDRILL_TLS", default=True) + settings_manager.set( + "h_api_auth_cookie_secret_key", + "H_API_AUTH_COOKIE_SECRET_KEY", + type_=_to_utf8, + required=True, + ) + settings_manager.set( + "h_api_auth_cookie_salt", + "H_API_AUTH_COOKIE_SALT", + type_=_to_utf8, + required=True, + ) + # Get resolved settings. settings = settings_manager.settings diff --git a/h/security/__init__.py b/h/security/__init__.py index 0646bea939b..c0d99d603ab 100644 --- a/h/security/__init__.py +++ b/h/security/__init__.py @@ -27,6 +27,11 @@ def includeme(config): # pragma: no cover settings["h_auth_cookie_secret"] = derive_key( settings["secret_key"], settings["secret_salt"], b"h.auth.cookie_secret" ) + settings["h_api_auth_cookie_secret"] = derive_key( + settings["h_api_auth_cookie_secret_key"], + settings["h_api_auth_cookie_salt"], + b"h_api_auth_cookie_secret", + ) # Set the default authentication policy. This can be overridden by modules # that include this one. diff --git a/h/security/policy/_cookie.py b/h/security/policy/_cookie.py index b1d657a58b4..c06d78143a4 100644 --- a/h/security/policy/_cookie.py +++ b/h/security/policy/_cookie.py @@ -14,13 +14,46 @@ class CookiePolicy: straps the login for the client (when the popup shows). """ - def __init__(self, cookie: SignedCookieProfile, helper: AuthTicketCookieHelper): - self.cookie = cookie + def __init__( + self, + html_authcookie: SignedCookieProfile, + api_authcookie: SignedCookieProfile, + helper: AuthTicketCookieHelper, + ): + self.html_authcookie = html_authcookie + self.api_authcookie = api_authcookie self.helper = helper def identity(self, request): self.helper.add_vary_by_cookie(request) - return self.helper.identity(self.cookie, request) + identity = self.helper.identity(self.html_authcookie, request) + + # If a request was successfully authenticated using the HTML auth + # cookie and that request did *not* also contain the API auth cookie, + # then add the API auth cookie to the user's browser. + # + # This is a temporary hack that was needed when we first added the + # separate API auth cookie: we needed to add the API auth cookie to the + # browsers of users who were already logged in with just the HTML auth + # cookie, we couldn't just rely on logging in to set the API auth + # cookie for users who were *already* logged in. + # + # This code should be deleted after it has been deployed to production for + # at least 30 days (the max_age of the HTML auth cookie). + # + # When deleting this code we should also change the `path` attribute of + # api_authcookie to "/api/" so that the API auth cookie is only sent + # with API requests. For already-logged-in users this path change won't + # take effect until they delete and re-create their API auth cookie by + # logging out and in again, which will take at most 30 days (the + # max_age of the cookie). + # + # When changing the `path` attribute of api_authcookie to "/api/" we + # should also remove the api_authcookie from the test requests in + # _cookie_test.py (see corresponding comment in _cookie_test.py). + self._issue_api_authcookie(identity, request) + + return identity def authenticated_userid(self, request): return Identity.authenticated_userid(self.identity(request)) @@ -41,11 +74,53 @@ def remember(self, request, userid, **kw): # pylint:disable=unused-argument request.session.update(data) request.session.new_csrf_token() - return self.helper.remember(self.cookie, request, userid) + # We're about to add the response headers to set the API auth cookie. + # Set this attribute so that _issue_api_authcookie() below won't add + # the same headers again. Otherwise responses to login form submissions + # would set the same cookie twice. + # + # This line of code can be deleted, along with _issue_api_authcookie() + # itself, at least 30 days after it has been deployed to production. + request.h_api_authcookie_headers_added = True + + return [ + *self.helper.remember(self.html_authcookie, request, userid), + *self.helper.remember(self.api_authcookie, request, userid), + ] def forget(self, request): self.helper.add_vary_by_cookie(request) - return self.helper.forget(self.cookie, request) + return [ + *self.helper.forget(self.html_authcookie, request), + *self.helper.forget(self.api_authcookie, request), + ] def permits(self, request, context, permission) -> Allowed | Denied: return identity_permits(self.identity(request), context, permission) + + def _issue_api_authcookie(self, identity, request): + if not identity: + return + + if not identity.user: + return + + if self.api_authcookie.cookie_name in request.cookies: + return + + if hasattr(request, "h_api_authcookie_headers_added"): + return + + headers = self.helper.remember( + self.api_authcookie, request, identity.user.userid + ) + + def add_api_authcookie_headers( + request, # pylint:disable=unused-argument + response, + ): + for key, value in headers: + response.headerlist.append((key, value)) + + request.add_response_callback(add_api_authcookie_headers) + request.h_api_authcookie_headers_added = True diff --git a/h/security/policy/top_level.py b/h/security/policy/top_level.py index 53ca312328f..8a8327c8731 100644 --- a/h/security/policy/top_level.py +++ b/h/security/policy/top_level.py @@ -41,23 +41,36 @@ def _load_identity(self, request): @RequestLocalCache() def get_subpolicy(request): """Return the subpolicy for TopLevelSecurityPolicy to delegate to for `request`.""" - cookie = webob.cookies.SignedCookieProfile( - secret=request.registry.settings["h_auth_cookie_secret"], - salt="authsanity", - cookie_name="auth", + + # The cookie that's used to authenticate API requests. + api_authcookie = webob.cookies.SignedCookieProfile( + secret=request.registry.settings["h_api_auth_cookie_secret"], + salt=request.registry.settings["h_api_auth_cookie_salt"], + cookie_name="h_api_authcookie", max_age=30 * 24 * 3600, # 30 days httponly=True, secure=request.scheme == "https", + samesite="strict", ) - cookie = cookie.bind(request) + api_authcookie = api_authcookie.bind(request) if is_api_request(request): return APIPolicy( [ BearerTokenPolicy(), AuthClientPolicy(), - APICookiePolicy(cookie, AuthTicketCookieHelper()), + APICookiePolicy(api_authcookie, AuthTicketCookieHelper()), ] ) - return CookiePolicy(cookie, AuthTicketCookieHelper()) + # The cookie that's used to authenticate HTML page requests. + html_authcookie = webob.cookies.SignedCookieProfile( + secret=request.registry.settings["h_auth_cookie_secret"], + salt="authsanity", + cookie_name="auth", + max_age=30 * 24 * 3600, # 30 days + httponly=True, + secure=request.scheme == "https", + ) + html_authcookie = html_authcookie.bind(request) + return CookiePolicy(html_authcookie, api_authcookie, AuthTicketCookieHelper()) diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py index c95f3b5aa45..92feb871f5d 100644 --- a/tests/functional/conftest.py +++ b/tests/functional/conftest.py @@ -25,6 +25,8 @@ "h.sentry_dsn_frontend": "TEST_SENTRY_DSN_FRONTEND", "pyramid.debug_all": False, "secret_key": "notasecret", + "h_api_auth_cookie_secret_key": b"test_h_api_auth_cookie_secret_key", + "h_api_auth_cookie_salt": b"test_h_api_auth_cookie_salt", "sqlalchemy.url": os.environ["DATABASE_URL"], } @@ -35,6 +37,8 @@ "AUTH_DOMAIN": TEST_SETTINGS["h.authority"], "SENTRY_DSN_FRONTEND": TEST_SETTINGS["h.sentry_dsn_frontend"], "SECRET_KEY": TEST_SETTINGS["secret_key"], + "H_API_AUTH_COOKIE_SECRET_KEY": TEST_SETTINGS["h_api_auth_cookie_secret_key"], + "H_API_AUTH_COOKIE_SALT": TEST_SETTINGS["h_api_auth_cookie_salt"], "DATABASE_URL": TEST_SETTINGS["sqlalchemy.url"], } diff --git a/tests/unit/h/config_test.py b/tests/unit/h/config_test.py index 763d89417a1..e6aac7f34eb 100644 --- a/tests/unit/h/config_test.py +++ b/tests/unit/h/config_test.py @@ -30,6 +30,8 @@ def test_configure_updates_settings_from_env_vars( # Required settings "es.url": "https://es6-search-cluster", "secret_key": "notasecret", + "h_api_auth_cookie_secret_key": b"test_h_api_auth_cookie_secret_key", + "h_api_auth_cookie_salt": b"test_h_api_auth_cookie_salt", "sqlalchemy.url": "postgres://user@dbhost/dbname", } diff --git a/tests/unit/h/security/policy/_cookie_test.py b/tests/unit/h/security/policy/_cookie_test.py index 3d77443a89a..51a372f48c6 100644 --- a/tests/unit/h/security/policy/_cookie_test.py +++ b/tests/unit/h/security/policy/_cookie_test.py @@ -1,41 +1,137 @@ -from unittest.mock import create_autospec, sentinel +from unittest.mock import call, create_autospec, sentinel import pytest +from webob.cookies import SignedCookieProfile +from h.security.identity import Identity as Identity_ from h.security.policy._cookie import CookiePolicy from h.security.policy.helpers import AuthTicketCookieHelper class TestCookiePolicy: - def test_identity(self, cookie_policy, helper, pyramid_request): + def test_identity(self, cookie_policy, helper, pyramid_request, html_authcookie): identity = cookie_policy.identity(pyramid_request) helper.add_vary_by_cookie.assert_called_once_with(pyramid_request) - helper.identity.assert_called_once_with(sentinel.cookie, pyramid_request) + helper.identity.assert_called_once_with(html_authcookie, pyramid_request) assert identity == helper.identity.return_value + def test_identity_issues_api_authcookie( + self, cookie_policy, helper, pyramid_request, api_authcookie + ): + helper.remember.return_value = [ + ("header_name_1", "header_value_1"), + ("header_name_2", "header_value_2"), + ] + del pyramid_request.cookies[api_authcookie.cookie_name] + + cookie_policy.identity(pyramid_request) + + pyramid_request._process_response_callbacks( # pylint:disable=protected-access + pyramid_request.response + ) + helper.remember.assert_called_once_with( + api_authcookie, pyramid_request, helper.identity.return_value.user.userid + ) + for header in helper.remember.return_value: + assert header in pyramid_request.response.headerlist + + @pytest.mark.parametrize( + "identity", + [ + None, + Identity_(user=None, auth_client=None), + ], + ) + def test_identity_doesnt_issue_api_authcookie_if_user_not_authenticated( + self, + cookie_policy, + helper, + pyramid_request, + api_authcookie, + identity, + assert_api_authcookie_not_issued, + ): + helper.identity.return_value = identity + del pyramid_request.cookies[api_authcookie.cookie_name] + + cookie_policy.identity(pyramid_request) + + assert_api_authcookie_not_issued() + + def test_identity_doesnt_issue_api_authcookie_if_already_issued( + self, cookie_policy, pyramid_request, assert_api_authcookie_not_issued + ): + cookie_policy.identity(pyramid_request) + + assert_api_authcookie_not_issued() + + def test_identity_issues_api_authcookie_only_once( + self, + cookie_policy, + helper, + pyramid_request, + api_authcookie, + assert_api_authcookie_not_issued, + ): + helper.remember.return_value = [("header_name_1", "header_value_1")] + del pyramid_request.cookies[api_authcookie.cookie_name] + cookie_policy.identity(pyramid_request) + pyramid_request._process_response_callbacks( # pylint:disable=protected-access + pyramid_request.response + ) + + cookie_policy.identity(pyramid_request) + + helper.reset_mock() + assert_api_authcookie_not_issued() + def test_authenticated_userid( - self, cookie_policy, helper, pyramid_request, Identity + self, cookie_policy, helper, pyramid_request, Identity, html_authcookie ): authenticated_userid = cookie_policy.authenticated_userid(pyramid_request) helper.add_vary_by_cookie.assert_called_once_with(pyramid_request) - helper.identity.assert_called_once_with(sentinel.cookie, pyramid_request) + helper.identity.assert_called_once_with(html_authcookie, pyramid_request) Identity.authenticated_userid.assert_called_once_with( helper.identity.return_value ) assert authenticated_userid == Identity.authenticated_userid.return_value - def test_remember(self, cookie_policy, helper, pyramid_request): + def test_remember( + self, cookie_policy, html_authcookie, api_authcookie, helper, pyramid_request + ): pyramid_request.session["data"] = "old" + def remember(cookie, *_args, **_kwargs): + if cookie == html_authcookie: + return [ + sentinel.html_authcookie_header_1, + sentinel.html_authcookie_header_2, + ] + if cookie == api_authcookie: + return [ + sentinel.api_authcookie_header_1, + sentinel.api_authcookie_header_2, + ] + + assert False, "Should never reach here" # pragma: no cover + + helper.remember.side_effect = remember + headers = cookie_policy.remember(pyramid_request, sentinel.userid, foo="bar") assert not pyramid_request.session - helper.remember.assert_called_once_with( - sentinel.cookie, pyramid_request, sentinel.userid - ) - assert headers == helper.remember.return_value + assert helper.remember.call_args_list == [ + call(html_authcookie, pyramid_request, sentinel.userid), + call(api_authcookie, pyramid_request, sentinel.userid), + ] + assert headers == [ + sentinel.html_authcookie_header_1, + sentinel.html_authcookie_header_2, + sentinel.api_authcookie_header_1, + sentinel.api_authcookie_header_2, + ] def test_remember_with_existing_user( self, cookie_policy, pyramid_request, factories, Identity @@ -51,32 +147,92 @@ def test_remember_with_existing_user( assert pyramid_request.session["data"] == "old" assert pyramid_request.session["_csrft_"] != "old_csrf_token" - def test_forget(self, cookie_policy, helper, pyramid_request): + def test_forget( + self, cookie_policy, helper, pyramid_request, html_authcookie, api_authcookie + ): + def forget(cookie, *_args, **_kwargs): + if cookie == html_authcookie: + return [ + sentinel.html_authcookie_header_1, + sentinel.html_authcookie_header_2, + ] + if cookie == api_authcookie: + return [ + sentinel.api_authcookie_header_1, + sentinel.api_authcookie_header_2, + ] + + assert False, "Should never reach here" # pragma: no cover + + helper.forget.side_effect = forget + headers = cookie_policy.forget(pyramid_request) helper.add_vary_by_cookie.assert_called_once_with(pyramid_request) - helper.forget.assert_called_once_with(sentinel.cookie, pyramid_request) - assert headers == helper.forget.return_value - - def test_permits(self, cookie_policy, helper, pyramid_request, identity_permits): + assert helper.forget.call_args_list == [ + call(html_authcookie, pyramid_request), + call(api_authcookie, pyramid_request), + ] + assert headers == [ + sentinel.html_authcookie_header_1, + sentinel.html_authcookie_header_2, + sentinel.api_authcookie_header_1, + sentinel.api_authcookie_header_2, + ] + + def test_permits( + self, cookie_policy, helper, pyramid_request, identity_permits, html_authcookie + ): permits = cookie_policy.permits( pyramid_request, sentinel.context, sentinel.permission ) helper.add_vary_by_cookie.assert_called_once_with(pyramid_request) - helper.identity.assert_called_once_with(sentinel.cookie, pyramid_request) + helper.identity.assert_called_once_with(html_authcookie, pyramid_request) identity_permits.assert_called_once_with( helper.identity.return_value, sentinel.context, sentinel.permission ) assert permits == identity_permits.return_value + @pytest.fixture + def html_authcookie(self, pyramid_request): + html_authcookie = SignedCookieProfile("secret", "salt", cookie_name="auth") + pyramid_request.cookies[html_authcookie.cookie_name] = html_authcookie + return html_authcookie.bind(pyramid_request) + + @pytest.fixture + def api_authcookie(self, pyramid_request): + api_authcookie = SignedCookieProfile( + "secret", "salt", cookie_name="h_api_authcookie" + ) + + # Add the API auth cookie to the request. + # In future, if we set the API auth cookie's `path` attribute to + # "/api/" so that the API auth cookie is no longer included in HTML + # requests, we should remove this line from the unit tests as well. + pyramid_request.cookies[api_authcookie.cookie_name] = api_authcookie + + return api_authcookie.bind(pyramid_request) + @pytest.fixture def helper(self): return create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True) @pytest.fixture - def cookie_policy(self, helper): - return CookiePolicy(sentinel.cookie, helper) + def cookie_policy(self, html_authcookie, api_authcookie, helper): + return CookiePolicy(html_authcookie, api_authcookie, helper) + + @pytest.fixture + def assert_api_authcookie_not_issued(self, helper, pyramid_request): + def assert_api_authcookie_not_issued(): + headerlist_before = list(pyramid_request.response.headerlist) + pyramid_request._process_response_callbacks( # pylint:disable=protected-access + pyramid_request.response + ) + helper.remember.assert_not_called() + assert pyramid_request.response.headerlist == headerlist_before + + return assert_api_authcookie_not_issued @pytest.fixture(autouse=True) diff --git a/tests/unit/h/security/policy/top_level_test.py b/tests/unit/h/security/policy/top_level_test.py index dfdd0e85394..9c33bcc20d0 100644 --- a/tests/unit/h/security/policy/top_level_test.py +++ b/tests/unit/h/security/policy/top_level_test.py @@ -1,6 +1,7 @@ -from unittest.mock import sentinel +from unittest.mock import call, create_autospec, sentinel import pytest +from webob.cookies import SignedCookieProfile from h.security.policy.top_level import TopLevelPolicy, get_subpolicy @@ -69,25 +70,28 @@ def test_api_request( AuthTicketCookieHelper, ): is_api_request.return_value = True + api_authcookie = create_autospec( + SignedCookieProfile, instance=True, spec_set=True + ) + webob.cookies.SignedCookieProfile.return_value = api_authcookie policy = get_subpolicy(pyramid_request) BearerTokenPolicy.assert_called_once_with() AuthClientPolicy.assert_called_once_with() webob.cookies.SignedCookieProfile.assert_called_once_with( - secret="test_h_auth_cookie_secret", - salt="authsanity", - cookie_name="auth", + secret="test_h_api_auth_cookie_secret", + salt="test_h_api_auth_cookie_salt", + cookie_name="h_api_authcookie", max_age=2592000, httponly=True, secure=True, + samesite="strict", ) - webob.cookies.SignedCookieProfile.return_value.bind.assert_called_once_with( - pyramid_request - ) + api_authcookie.bind.assert_called_once_with(pyramid_request) AuthTicketCookieHelper.assert_called_once_with() APICookiePolicy.assert_called_once_with( - webob.cookies.SignedCookieProfile.return_value.bind.return_value, + api_authcookie.bind.return_value, AuthTicketCookieHelper.return_value, ) APIPolicy.assert_called_once_with( @@ -108,23 +112,44 @@ def test_non_api_request( AuthTicketCookieHelper, ): is_api_request.return_value = False + html_authcookie = create_autospec( + SignedCookieProfile, instance=True, spec_set=True + ) + api_authcookie = create_autospec( + SignedCookieProfile, instance=True, spec_set=True + ) + webob.cookies.SignedCookieProfile.side_effect = [ + api_authcookie, + html_authcookie, + ] policy = get_subpolicy(pyramid_request) - webob.cookies.SignedCookieProfile.assert_called_once_with( - secret="test_h_auth_cookie_secret", - salt="authsanity", - cookie_name="auth", - max_age=2592000, - httponly=True, - secure=True, - ) - webob.cookies.SignedCookieProfile.return_value.bind.assert_called_once_with( - pyramid_request - ) + assert webob.cookies.SignedCookieProfile.call_args_list == [ + call( + secret="test_h_api_auth_cookie_secret", + salt="test_h_api_auth_cookie_salt", + cookie_name="h_api_authcookie", + max_age=2592000, + httponly=True, + secure=True, + samesite="strict", + ), + call( + secret="test_h_auth_cookie_secret", + salt="authsanity", + cookie_name="auth", + max_age=2592000, + httponly=True, + secure=True, + ), + ] + api_authcookie.bind.assert_called_once_with(pyramid_request) + html_authcookie.bind.assert_called_once_with(pyramid_request) AuthTicketCookieHelper.assert_called_once_with() CookiePolicy.assert_called_once_with( - webob.cookies.SignedCookieProfile.return_value.bind.return_value, + html_authcookie.bind.return_value, + api_authcookie.bind.return_value, AuthTicketCookieHelper.return_value, ) assert policy == CookiePolicy.return_value @@ -170,6 +195,8 @@ def CookiePolicy(mocker): @pytest.fixture def pyramid_settings(pyramid_settings): pyramid_settings["h_auth_cookie_secret"] = "test_h_auth_cookie_secret" + pyramid_settings["h_api_auth_cookie_secret"] = "test_h_api_auth_cookie_secret" + pyramid_settings["h_api_auth_cookie_salt"] = "test_h_api_auth_cookie_salt" return pyramid_settings diff --git a/tox.ini b/tox.ini index 1cf2af1c207..d9758c08a0c 100644 --- a/tox.ini +++ b/tox.ini @@ -99,6 +99,8 @@ setenv = dev: ALEMBIC_CONFIG = {env:ALEMBIC_CONFIG:conf/alembic.ini} dev: JWE_SECRET_LMS = {env:JWE_SECRET_LMS:SUPER_SECRET} dev: WEB_CONCURRENCY = {env:WEB_CONCURRENCY:2} + dev: H_API_AUTH_COOKIE_SECRET_KEY = {env:H_API_AUTH_COOKIE_SECRET_KEY:"dev_h_api_auth_cookie_secret_key"} + dev: H_API_AUTH_COOKIE_SALT = {env:H_API_AUTH_COOKIE_SALT:"dev_h_api_auth_cookie_salt"} tests: COVERAGE_FILE = {env:COVERAGE_FILE:.coverage.{envname}} dev: DATABASE_URL = {env:DATABASE_URL:postgresql://postgres@localhost/postgres} dev: REPLICA_DATABASE_URL = {env:DATABASE_URL:postgresql://postgres@localhost/postgres}