Use a separate auth cookie for API requests #8861

seanh · 2024-08-07T14:12:42Z

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:

Recall that the top-level security policy (TopLevelPolicy) delegates all method calls to either CookiePolicy (for HTML endpoints) or APIPolicy (for API endpoints).
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.
Similarly when the user logs out CookiePolicy.forget() deletes both the HTML and API auth cookies.
To authenticate HTML page requests CookiePolicy.identity() reads only the HTML auth cookie.
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.

Testing

Log in and try to use some pages.

Try enabling the preact_create_group_form feature flag and creating a new group using the Preact version of the new-group page.

Log out again.

Look at the request and response cookies in developer tools. When logged out there should be only a session cookie, neither auth cookie. On log in both auth cookies should be set. When logged in both auth cookies should be sent in both HTML and API requests.

Testing the cookie transition for already-logged-in users

We can't rely on the login form submission (which calls CookiePolicy.remember()) to create the new API auth cookie for users who're already logged-in to production. Instead when a request to any HTML page is authenticated from an already-logged-in user who has the HTML auth cookie but not the API auth cookie we add the API auth cookie to their browser. To test this:

Create a situation where you're logged in and your browser has the HTML auth cookie but not the new API auth cookie. For example:
1. Delete cookies from your browser to get rid of any existing API auth cookie
2. Check out the main branch
3. Log in to create an HTML auth cookie but no API one
4. Check out this branch
5. Go to http://localhost:5000/groups/new with the preact_create_group_form feature flag enabled and try to create a group
6. You should be able to create a group successfully. In dev tools you should see that the request for the /groups/new page contained only the HTML auth cookie but the response added the API auth cookie. The request to /api/groups then contained both cookies.

seanh · 2024-08-07T14:15:12Z

h/security/policy/_cookie.py

+    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


We'll be able to remove this whole helper method after the 30 days transition period for already-logged-in users.

The method for issuing the cookie using a response callback plus a request attribute to record that the response callback has already been added is copied from how Pyramid itself does it.

I've gone ahead and written the PR to remove this transitional stuff, even though we can't merge it yet: #8865

seanh · 2024-08-07T14:18:41Z

tests/unit/h/security/policy/_cookie_test.py

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


Bunch of slightly awkward tests for the transitional API auth cookie issuing. We'll be able to delete these after the 30-days transition period.

Factor out the `IdentityBasedPolicy` base class, favouring helper methods that're used by composition instead. This base class is making future refactorings (see #8860 and #8861) more difficult to reason about by coupling together different security policy classes that I want to evolve in separate directions. The base class is also responsible for some disturbing unintended behaviours. For example, on `main`: * `IdentityBasedPolicy.authenticated_userid()` calls `identity()` (which must be implemented by `IdentityBasedPolicy` sub classes). * `CookiePolicy` has an `identity()` method and inherits from `IdentityBasedPolicy`, so `CookiePolicy` inherits `IdentityBasedPolicy`'s `authenticated_userid()` method that calls `CookiePolicy.identity()` * As with all h's security policies `CookiePolicy` is never used directly as a security policy, rather it's always delegated to by `TopLevelPolicy` * `TopLevelPolicy` has an `identity()` method that delegates to `CookiePolicy.identity()` for HTML pages, and `TopLevelPolicy` also inherits from `IdentityBasedPolicy`, so `TopLevelPolicy` inherits an `authenticated_userid()` method that calls `TopLevelPolicy.identity()`. * So the end result is that `CookiePolicy.authenticated_userid()` (which is inherited from `IdentityBasedPolicy`) is never actually used! Instead `TopLevelPolicy`'s copy of `authenticated_userid()` (also inherited from `TopLevelPolicy`) is used and calls `TopLevelPolicy.identity()` which delegates to `CookiePolicy.identity()`. The same thing happens when `APIPolicy` (which also inherits from `IdentityBasedPolicy`) delegates to API subpolicies. This is true for the `authenticated_userid()` and `permits()` methods of all h's security policies other than `TopLevelPolicy`: none of them are ever called. This hasn't resulted in any bugs because all the sub-policies and `TopLevelPolicy` inherit same `authenticated_userid()` and `permits()` methods from `IdentityBasedPolicy`, so `TopLevelPolicy`'s by-passing the `authenticated_userid()` and `permits()` methods of sub-policies makes no difference. In fact you could remove the `IdentityBasedPolicy` base class from all classes except `TopLevelPolicy` and it would make no difference, the methods that all those classes inherit from `IdentityBasedPolicy` are never called. But clearly this is very confusing and unintended, and would lead to bugs if there's ever a sub-policy in future that has a different `authenticated_userid()` or `permits()` method.

robertknight

This works as expected. I did initially wonder whether using a combination of two cookies for HTML vs API requests plus a CSRF token was overkill. I think I convinced myself they each have value (notes in Slack).

robertknight · 2024-08-08T10:34:59Z

h/security/policy/_cookie.py

+
+        # 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.


What about cases where, for any reason, the API cookie expires or gets deleted independently of the auth cookie?

This should never happen:

Both cookies are always set at the same time (at login) and deleted at the same time (on logout) and have the same max-age, and for existing logged-in users who have the HTML cookie but not the API cookie this PR contains transitional code to detect that situation and issue an API cookie that will have a max-age beyond that of the existing HTML cookie.

So the browser should always have either both cookies or neither.

This can't currently be broken by a user going into their dev tools and deleting the API cookie but not the HTML one: the transitional code would detect that situation and issue a new API cookie. If #8865 was merged then the transitional code would no longer be present to save us in that situation, so perhaps we should consider closing #8865 and making the transitional code permanent.

Having a browser extension that blocks the API cookie but not the HTML one would still be a problem, but that seems unlikely to me particularly because they would have to specifically block or delete the API cookie but not the HTML one.

Nonetheless, I've artificially created this condition (when the browser has the HTML cookie but not the API one). On the create-group page this is what happens:

The response that the frontend receives to its unauthenticated create-group API request is 404 Not Found with this JSON body:

{ "status": "failure", "reason": "Either the resource you requested doesn't exist, or you are not currently authorized to see it." }

I've created an issue to track this as potential follow-up work: #8881

I've added a commit to make API cookies live longer than HTML ones to make this situation less likely: fc746dd

Added a commit to remove comments about the transitional API cookie issuing code being temporary, in case we decide to make it permanent: d7ef927

robertknight · 2024-08-15T12:52:56Z

h/security/policy/_cookie.py

+        # 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)


When the API auth cookie is issued as a result of this method, it will have a different expiry than the HTML auth cookie, specifically it will always be some later date. Is there a scenario where this will be a problem?

I don't think this would ever be a problem, no. When the user's HTML auth cookie expires they may find themselves with an API auth cookie but no HTML one. They'll be unable to do anything with their API auth cookie because if they try to load any h pages they'll find themselves logged out (due to the expired HTML auth cookie). If they log back in they'll be issued new HTML and API auth cookies with matching expiry times.

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.

We may end up deciding to keep this code permanently, see: #8861 (comment)

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 August 7, 2024 14:12

seanh commented Aug 7, 2024

View reviewed changes

seanh force-pushed the separate-api-cookie branch from 26cfb84 to af0e4bf Compare August 7, 2024 14:38

seanh marked this pull request as ready for review August 7, 2024 14:39

seanh mentioned this pull request Aug 7, 2024

Remove IdentityBasedPolicy base class #8859

Merged

seanh mentioned this pull request Aug 7, 2024

Extract AuthTicketCookieHelper class #8860

Merged

seanh force-pushed the AuthTicketCookieHelper branch from fdc7ba9 to 1f9b99c Compare August 7, 2024 15:27

seanh force-pushed the separate-api-cookie branch from af0e4bf to 9ae05d9 Compare August 7, 2024 15:29

This was referenced Aug 7, 2024

Replace AuthCookieService with AuthTicketService #8851

Merged

Remove no-longer-needed transitional cookie code #8865

Closed

seanh force-pushed the AuthTicketCookieHelper branch from 1f9b99c to 0318d44 Compare August 15, 2024 12:51

Base automatically changed from AuthTicketCookieHelper to main August 15, 2024 12:54

robertknight approved these changes Aug 15, 2024

View reviewed changes

seanh force-pushed the separate-api-cookie branch from 8f59437 to 6b5ebeb Compare August 16, 2024 14:46

seanh mentioned this pull request Aug 16, 2024

Show a more specific error message if a user has h's HTML auth cookie but not the API auth cookie #8881

Open

seanh added 2 commits August 16, 2024 17:18

Make API auth cookies stay fresh longer than HTML ones

fc746dd

Remove comments about temporary API auth cookie code

d7ef927

We may end up deciding to keep this code permanently, see: #8861 (comment)

seanh merged commit a6ddc78 into main Aug 20, 2024
9 checks passed

seanh deleted the separate-api-cookie branch August 20, 2024 10:13

seanh mentioned this pull request Aug 30, 2024

Errors when creating or editing groups due to expired auth cookie tickets #8914

Closed

seanh mentioned this pull request Sep 11, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a separate auth cookie for API requests #8861

Use a separate auth cookie for API requests #8861

seanh commented Aug 7, 2024 •

edited

Loading

seanh Aug 7, 2024 •

edited

Loading

seanh Aug 8, 2024

seanh Aug 7, 2024

robertknight left a comment

robertknight Aug 8, 2024

seanh Aug 16, 2024

seanh Aug 16, 2024

seanh Aug 16, 2024

robertknight Aug 15, 2024

seanh Aug 16, 2024 •

edited

Loading

Use a separate auth cookie for API requests #8861

Use a separate auth cookie for API requests #8861

Conversation

seanh commented Aug 7, 2024 • edited Loading

Testing

Testing the cookie transition for already-logged-in users

seanh Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Aug 8, 2024

Choose a reason for hiding this comment

seanh Aug 7, 2024

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

robertknight Aug 8, 2024

Choose a reason for hiding this comment

seanh Aug 16, 2024

Choose a reason for hiding this comment

seanh Aug 16, 2024

Choose a reason for hiding this comment

seanh Aug 16, 2024

Choose a reason for hiding this comment

robertknight Aug 15, 2024

Choose a reason for hiding this comment

seanh Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

seanh commented Aug 7, 2024 •

edited

Loading

seanh Aug 7, 2024 •

edited

Loading

seanh Aug 16, 2024 •

edited

Loading