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

@@ -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
 

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.

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

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())

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"],
 }
 

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",
     }