Use a separate auth cookie for API requests (fixed version) #8946

seanh · 2024-09-11T09:34:56Z

This is a second attempt at getting h to use two separate auth cookies: one for HTML page requests and another for JSON API requests. This enables the API auth cookie to be more secure: it 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).

History:

An earlier attempt to introduce separate auth cookies was made in: Use a separate auth cookie for API requests #8861
There was an issue that the API auth cookie and it's paired HTML auth cookie each had their own corresponding auth ticket in the DB. The HTML auth cookie's ticket's expiry time would be reset each time an HTML page request was made, but the API auth cookie's ticket's expiry time would only be reset when a JSON API request was made (which is much rarer). As a result the API auth cookies would end up unusable because their auth tickets had expired, whilst the HTML auth cookies were still usable. This created a situation where the browser had a usable HTML auth cookie so it could load HTML pages and the user would appear to be logged in, but the browser did not have a usable API auth cookie so any API requests made by those HTML pages would fail and the user would see errors. See: Errors when creating or editing groups due to expired auth cookie tickets #8914
As a result of this issue the API auth cookie was "reverted" in Revert to HTML auth cookie #8913 (note: not an actual revert, just the minimal change necessary to make h once again use a single cookie for both HTML and API requests)

This PR reintroduces the separate API auth cookies but with a change that should avoid ever getting into a situation where a browser has a valid HTML auth cookie but does not have a valid API auth cookie: each API auth cookie and its paired HTML auth cookie share the same auth ticket in the DB. So whenever an HTML request or an API request is made the shared ticket's expiry time will be reset. And if the shared ticket expires that will invalidate both auth cookies at once.

Testing

Reduce the auth ticket TTL and refresh interval times, so that you don't have to wait around for weeks to reproduce the issue:

diff --git a/h/services/auth_ticket.py b/h/services/auth_ticket.py
index 59a8772d9..84351bbe1 100644
--- a/h/services/auth_ticket.py
+++ b/h/services/auth_ticket.py
@@ -6,12 +6,12 @@ from h.models import AuthTicket, User


 class AuthTicketService:
-    TICKET_TTL = timedelta(days=7)
+    TICKET_TTL = timedelta(minutes=1)

     # We only want to update the `expires` column when the tickets `expires` is
     # at least one minute smaller than the potential new value. This prevents
     # that we update the `expires` column on every single request.
-    TICKET_REFRESH_INTERVAL = timedelta(minutes=1)
+    TICKET_REFRESH_INTERVAL = timedelta(seconds=1)

     def __init__(self, session, user_service):
         self._session = session

Log in and load http://localhost:5000/groups/new/
Wait 30 seconds, then reload the page. Reloading will refresh the auth ticket.
Wait another 30 seconds.
Try to create a group, it should succeed.

seanh · 2024-09-11T09:48:56Z

h/security/policy/_api_cookie.py

@@ -29,7 +29,7 @@ def handles(request: Request) -> bool:
        ) in COOKIE_AUTHENTICATABLE_API_REQUESTS

    def identity(self, request: Request) -> Identity | None:
-        identity = self.helper.identity(self.cookie, request)
+        identity, _ = self.helper.identity(self.cookie, request)


self.helper.identity() now returns an (Identity, AuthTicket) 2-tuple instead of just an Identity. APICookiePolicy only needs the Identity so it can just discard the AuthTicket.

seanh · 2024-09-11T09:50:35Z

h/security/policy/_cookie.py

+        ticket_id = self.helper.add_ticket(request, userid)
+
        return [
-            *self.helper.remember(self.html_authcookie, request, userid),
-            *self.helper.remember(self.api_authcookie, request, userid),
+            *self.helper.remember(self.html_authcookie, userid, ticket_id),
+            *self.helper.remember(self.api_authcookie, userid, ticket_id),


This is the first of the two places where cookies are issued. This is the code that's executed when logging in. It now creates a single AuthTicket in the DB and then creates a pair of cookies (html_authcookie and api_authcookie) that both share that same ticket.

seanh · 2024-09-11T09:51:52Z

h/security/policy/_cookie.py

@@ -105,7 +107,7 @@ def _issue_api_authcookie(self, identity, request):
            return

        headers = self.helper.remember(
-            self.api_authcookie, request, identity.user.userid
+            self.api_authcookie, identity.user.userid, ticket_id


This is the second of the two places where cookies are issued. It's the possibly temporary "transitional" code that's executed when a request contains a valid HTML auth cookie but does not have an API auth cookie. As before, it issues the browser with a new API auth cookie. But that new API auth cookie will now re-use the existing HTML auth cookie's DB ticket rather than creating a new one.

seanh · 2024-09-11T09:56:29Z

h/security/policy/helpers.py

@@ -16,19 +16,29 @@ def is_api_request(request) -> bool:
 class AuthTicketCookieHelper:
    def identity(
        self, cookie: SignedCookieProfile, request: Request
-    ) -> Identity | None:
+    ) -> tuple[Identity, AuthTicket] | tuple[None, None]:


Because of the "temporary" / "transitional" cookie-issuing code it's necessary for AuthTicketCookieHelper.identity() to return not only the Identity (as before) but also the corresponding valid AuthTicket that was found. When verifying an HTML auth cookie (with helper.identity(html_authcookie, request)), CookiePolicy also needs to get the HTML auth cookie's AuthTicket so that it can re-use the same ticket if it has to issue the browser with a new API auth cookie to go with a lone HTML one.

seanh · 2024-09-11T09:59:44Z

h/security/policy/helpers.py

+    def add_ticket(self, request: Request, userid):
+        """
+        Add a new auth ticket for the given user to the DB.
+
+        Returns the ID of the newly-created auth ticket.
+        """


remember() (which used to generate both a new auth ticket and a new cookie) has been split into two methods add_ticket() and remember() so that CookiePolicy can issue two cookies with the same ticket, like this:

ticket_id = helper.add_ticket(request, userid) headers = [ *helper.remember(html_authcookie, userid, ticket_id), *helper.remember(api_authcookie, userid, ticket_id), ]

seanh · 2024-09-11T10:01:12Z

h/security/policy/top_level.py

@@ -48,7 +48,7 @@ def get_subpolicy(request):
    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",
+        cookie_name="h_api_authcookie.v2",


There are existing "h_api_authcookie"'s in the wild that have their own auth tickets in the DB (separate from the tickets of their corresponding HTML auth cookies) and these tickets will now have expired. We need a way to invalidate those existing cookies. So I've just renamed the cookie.

seanh · 2024-09-11T12:16:41Z

h/security/policy/helpers.py

+        ticket = request.find_service(AuthTicketService).verify_ticket(
+            userid, ticket_id
+        )


AuthTicketService.verify_ticket() was changed to return the AuthTicket rather than the User (see below).

seanh · 2024-09-11T12:19:14Z

h/security/policy/top_level.py

+    if is_api_request(request):
+        return APIPolicy(
+            [
+                BearerTokenPolicy(),
+                AuthClientPolicy(),
+                APICookiePolicy(api_authcookie, AuthTicketCookieHelper()),
+            ]
+        )


This is just reverting #8913. The diff is slightly confusing because #8913 also had to move the code, but the only real difference is that api_authcookie is now passed to APICookiePolicy() rather than html_authcookie.

seanh · 2024-09-11T12:20:12Z

h/services/auth_ticket.py

+    def verify_ticket(
+        self, userid: str | None, ticket_id: str | None
+    ) -> AuthTicket | None:


AuthTicketService.verify_ticket() had to be changed to return the AuthTicket rather than the User, because the caller code (see above) now needs the auth ticket. The user is still available to the caller via AuthTicket.user.

The change to this file is complicated by the fact that AuthTicketService caches the result of verify_ticket() as self._user (now self._ticket). This caching is probably an unnecessary optimization that's just creating confusion and code maintenance difficulty, but I didn't want to risk removing it right now.

The code does do a DB lookup that won't be cached, so this might be a reasonable thing to do if there is a chance that a request might access the same ticket multiple times. I'm not familiar enough with the code to know if that is the case.

robertknight

LGTM, works as expected. The added complexity of having two cookies is an unfortunate downside of the security measures.

robertknight · 2024-09-19T13:00:31Z

h/services/auth_ticket.py

+    def verify_ticket(
+        self, userid: str | None, ticket_id: str | None
+    ) -> AuthTicket | None:


The code does do a DB lookup that won't be cached, so this might be a reasonable thing to do if there is a chance that a request might access the same ticket multiple times. I'm not familiar enough with the code to know if that is the case.

Fixes #8914. This reverts commit 87b8133 and adds a change that should avoid ever getting into a situation where a browser has an HTML auth cookie with a valid auth ticket in the DB but has an API auth cookie whose auth ticket has expired: Each API auth cookie and its paired HTML auth cookie now share the same auth ticket in the DB. This is a second attempt at getting h to use two separate auth cookies: one for HTML page requests and another for JSON API requests. This enables the API auth cookie to be more secure: it 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). History: * An earlier attempt to introduce separate auth cookies was made in: #8861 * There was an issue that the API auth cookie and it's paired HTML auth cookie each had their own corresponding auth _ticket_ in the DB. The HTML auth cookie's ticket's expiry time would be reset each time an HTML page request was made, but the API auth cookie's ticket's expiry time would only be reset when a JSON API request was made (which is much rarer). As a result the API auth cookies would end up unusable because their auth tickets had expired, whilst the HTML auth cookies were still usable. This created a situation where the browser had a usable HTML auth cookie so it could load HTML pages and the user would appear to be logged in, but the browser did not have a usable API auth cookie so any API requests made by those HTML pages would fail and the user would see errors. See: #8914 * As a result of this issue the API auth cookie was "reverted" in #8913 (note: not an actual revert, just the minimal change necessary to make h once again use a single cookie for both HTML and API requests). * This commit now reintroduces the reverted change, plus a fix for the issue.

seanh requested a review from robertknight September 11, 2024 09:34

seanh changed the title ~~fix api cookie~~ Use a separate auth cookie for API requests (fixed version) Sep 11, 2024

seanh commented Sep 11, 2024

View reviewed changes

seanh force-pushed the fix-api-cookie branch from 844aa75 to a82adb6 Compare September 11, 2024 12:12

seanh commented Sep 11, 2024

View reviewed changes

seanh force-pushed the fix-api-cookie branch 2 times, most recently from 4ac67fc to 7a4ce72 Compare September 11, 2024 12:24

seanh marked this pull request as ready for review September 11, 2024 12:25

seanh force-pushed the fix-api-cookie branch from 7a4ce72 to bcf5371 Compare September 19, 2024 11:03

robertknight approved these changes Sep 19, 2024

View reviewed changes

seanh force-pushed the fix-api-cookie branch from bcf5371 to 4a4a643 Compare September 19, 2024 13:44

seanh force-pushed the fix-api-cookie branch from 4a4a643 to f6ef9f6 Compare September 21, 2024 13:42

seanh merged commit a593708 into main Sep 25, 2024
9 checks passed

seanh deleted the fix-api-cookie branch September 25, 2024 14:28

This was referenced Sep 25, 2024

Revert "Use a separate auth cookie for API requests (fixed version)" #8968

Merged

Revert "Revert "Use a separate auth cookie for API requests (fixed version)"" #8969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a separate auth cookie for API requests (fixed version) #8946

Use a separate auth cookie for API requests (fixed version) #8946

seanh commented Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024

seanh Sep 11, 2024

seanh Sep 11, 2024

seanh Sep 11, 2024

seanh Sep 11, 2024 •

edited

Loading

robertknight Sep 19, 2024

robertknight left a comment

robertknight Sep 19, 2024

Use a separate auth cookie for API requests (fixed version) #8946

Use a separate auth cookie for API requests (fixed version) #8946

Conversation

seanh commented Sep 11, 2024 • edited Loading

Testing

seanh Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Sep 11, 2024

Choose a reason for hiding this comment

seanh Sep 11, 2024

Choose a reason for hiding this comment

seanh Sep 11, 2024

Choose a reason for hiding this comment

seanh Sep 11, 2024

Choose a reason for hiding this comment

seanh Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

robertknight Sep 19, 2024

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

robertknight Sep 19, 2024

Choose a reason for hiding this comment

seanh commented Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading

seanh Sep 11, 2024 •

edited

Loading