Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Sep 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion h/security/policy/_api_cookie.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

@seanh seanh Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if identity is None:
return identity
Expand Down
14 changes: 8 additions & 6 deletions h/security/policy/_cookie.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def __init__(

def identity(self, request):
self.helper.add_vary_by_cookie(request)
identity = self.helper.identity(self.html_authcookie, request)
identity, ticket_id = 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,
Expand All @@ -47,7 +47,7 @@ def identity(self, request):
# this won't happen but it could happen if the API auth cookie (but not
# the HTML one) was deleted somehow, for example by the user manually
# deleting the cookie in the browser's developer tools, or another way.
self._issue_api_authcookie(identity, request)
self._issue_api_authcookie(identity, request, ticket_id)

return identity

Expand Down Expand Up @@ -76,9 +76,11 @@ def remember(self, request, userid, **kw): # pylint:disable=unused-argument
# would set the same cookie twice.
request.h_api_authcookie_headers_added = True

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),
Comment on lines +79 to +83
Copy link
Contributor Author

@seanh seanh Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

]

def forget(self, request):
Expand All @@ -91,7 +93,7 @@ def forget(self, request):
def permits(self, request, context, permission) -> Allowed | Denied:
return identity_permits(self.identity(request), context, permission)

def _issue_api_authcookie(self, identity, request):
def _issue_api_authcookie(self, identity, request, ticket_id):
if not identity:
return

Expand All @@ -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
Copy link
Contributor Author

@seanh seanh Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

def add_api_authcookie_headers(
Expand Down
22 changes: 16 additions & 6 deletions h/security/policy/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Copy link
Contributor Author

@seanh seanh Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

userid, ticket_id = self.get_cookie_value(cookie)

user = request.find_service(AuthTicketService).verify_ticket(userid, ticket_id)
ticket = request.find_service(AuthTicketService).verify_ticket(
userid, ticket_id
)
Comment on lines +22 to +24
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (not user) or user.deleted:
return None
if (not ticket) or ticket.user.deleted:
return (None, None)

return Identity.from_models(user=user)
return (Identity.from_models(user=ticket.user), ticket)

def remember(self, cookie: SignedCookieProfile, request: Request, userid: str):
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.
"""
Comment on lines +31 to +36
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
]

ticket_id = AuthTicket.generate_ticket_id()
request.find_service(AuthTicketService).add_ticket(userid, ticket_id)
return ticket_id

def remember(self, cookie: SignedCookieProfile, userid: str, ticket_id):
return cookie.get_headers([userid, ticket_id])

def forget(self, cookie: SignedCookieProfile, request: Request):
Expand Down
29 changes: 10 additions & 19 deletions h/security/policy/top_level.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

httponly=True,
secure=request.scheme == "https",
samesite="strict",
Expand All @@ -61,6 +61,15 @@ def get_subpolicy(request):
)
api_authcookie = api_authcookie.bind(request)

if is_api_request(request):
return APIPolicy(
[
BearerTokenPolicy(),
AuthClientPolicy(),
APICookiePolicy(api_authcookie, AuthTicketCookieHelper()),
]
)
Comment on lines +64 to +71
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# The cookie that's used to authenticate HTML page requests.
html_authcookie = webob.cookies.SignedCookieProfile(
secret=request.registry.settings["h_auth_cookie_secret"],
Expand All @@ -71,22 +80,4 @@ def get_subpolicy(request):
secure=request.scheme == "https",
)
html_authcookie = html_authcookie.bind(request)

if is_api_request(request):
return APIPolicy(
[
BearerTokenPolicy(),
AuthClientPolicy(),
APICookiePolicy(
# Use html_authcookie rather than api_authcookie to
# authenticate API requests, at least for now. This is a
# hopefully temporary workaround for an undiagnosed issue
# we've been seeing in production, see:
# https://hypothes-is.slack.com/archives/C2BLQDKHA/p1725041696040459
html_authcookie,
AuthTicketCookieHelper(),
),
]
)

return CookiePolicy(html_authcookie, api_authcookie, AuthTicketCookieHelper())
44 changes: 26 additions & 18 deletions h/services/auth_ticket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import sqlalchemy as sa

from h.models import AuthTicket, User
from h.models import AuthTicket


class AuthTicketService:
Expand All @@ -16,19 +16,21 @@ class AuthTicketService:
def __init__(self, session, user_service):
self._session = session
self._user_service = user_service
self._user = None
self._ticket = None

def verify_ticket(self, userid: str | None, ticket_id: str | None) -> User | None:
def verify_ticket(
self, userid: str | None, ticket_id: str | None
) -> AuthTicket | None:
Comment on lines +21 to +23
Copy link
Contributor Author

@seanh seanh Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Return the User object matching the given userid and ticket_id, or None.
Return the AuthTicket matching the given userid and ticket_id, or None.

Verify that there is an unexpired AuthTicket in the DB matching the
given `userid` and `ticket_id` and if so return the corresponding User.
given `userid` and `ticket_id` and if so return the AuthTicket.
"""

if self._user:
# We've already vetted the user!
return self._user
if self._ticket:
# We've already verified this request's ticket.
return self._ticket

if not userid or not ticket_id:
return None
Expand All @@ -53,34 +55,40 @@ def verify_ticket(self, userid: str | None, ticket_id: str | None) -> User | Non
if (datetime.utcnow() - ticket.updated) > self.TICKET_REFRESH_INTERVAL:
ticket.expires = datetime.utcnow() + self.TICKET_TTL

# Update the user cache to allow quick checking if we are called again
self._user = ticket.user
# Update the cache to allow quick checking if we are called again
self._ticket = ticket

return self._user
return self._ticket

def add_ticket(self, userid: str, ticket_id: str) -> None:
"""Add a new auth ticket for the given userid and token_id to the DB."""

# Update the user cache to allow quick checking if we are called again
self._user = self._user_service.fetch(userid)
if self._user is None:
user = self._user_service.fetch(userid)

if user is None:
raise ValueError(f"Cannot find user with userid {userid}")

ticket = AuthTicket(
id=ticket_id,
user=self._user,
user_userid=self._user.userid,
user=user,
user_userid=user.userid,
expires=datetime.utcnow() + self.TICKET_TTL,
)

self._session.add(ticket)

# Update the cache to allow quick checking if we are called again.
self._ticket = ticket

return ticket

def remove_ticket(self, ticket_id: str) -> None:
"""Remove any ticket with the given ID from the DB."""

self._session.query(AuthTicket).filter_by(id=ticket_id).delete()

# Empty the cached user to force revalidation.
self._user = None
# Empty the cache to force revalidation.
self._ticket = None


def factory(_context, request):
Expand Down
18 changes: 12 additions & 6 deletions tests/unit/h/security/policy/_api_cookie_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from pyramid.csrf import SessionCSRFStoragePolicy
from pyramid.exceptions import BadCSRFOrigin, BadCSRFToken

from h.security.identity import Identity as Identity_
from h.security.policy._api_cookie import APICookiePolicy
from h.security.policy.helpers import AuthTicketCookieHelper

Expand Down Expand Up @@ -31,12 +32,12 @@ def test_identity(self, api_cookie_policy, helper, pyramid_csrf_request):

helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request)
helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request)
assert identity == helper.identity.return_value
assert identity == helper.identity.return_value[0]

def test_identity_with_no_auth_cookie(
self, api_cookie_policy, helper, pyramid_request
):
helper.identity.return_value = None
helper.identity.return_value = (None, None)

assert api_cookie_policy.identity(pyramid_request) is None

Expand Down Expand Up @@ -64,7 +65,7 @@ def test_authenticated_userid(
helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request)
helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request)
Identity.authenticated_userid.assert_called_once_with(
helper.identity.return_value
helper.identity.return_value[0]
)
assert authenticated_userid == Identity.authenticated_userid.return_value

Expand All @@ -78,13 +79,18 @@ def test_permits(
helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request)
helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request)
identity_permits.assert_called_once_with(
helper.identity.return_value, sentinel.context, sentinel.permission
helper.identity.return_value[0], sentinel.context, sentinel.permission
)
assert permits == identity_permits.return_value

@pytest.fixture
def helper(self):
return create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
def helper(self, factories):
helper = create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
helper.identity.return_value = (
create_autospec(Identity_, instance=True, spec_set=True),
factories.AuthTicket(),
)
return helper

@pytest.fixture
def api_cookie_policy(self, helper):
Expand Down
30 changes: 19 additions & 11 deletions tests/unit/h/security/policy/_cookie_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def test_identity(self, cookie_policy, helper, pyramid_request, html_authcookie)

helper.add_vary_by_cookie.assert_called_once_with(pyramid_request)
helper.identity.assert_called_once_with(html_authcookie, pyramid_request)
assert identity == helper.identity.return_value
assert identity == helper.identity.return_value[0]

def test_identity_issues_api_authcookie(
self, cookie_policy, helper, pyramid_request, api_authcookie
Expand All @@ -31,16 +31,18 @@ def test_identity_issues_api_authcookie(
pyramid_request.response
)
helper.remember.assert_called_once_with(
api_authcookie, pyramid_request, helper.identity.return_value.user.userid
api_authcookie,
helper.identity.return_value[0].user.userid,
helper.identity.return_value[1],
)
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),
(None, None),
(Identity_(user=None, auth_client=None), sentinel.auth_ticket),
],
)
def test_identity_doesnt_issue_api_authcookie_if_user_not_authenticated(
Expand Down Expand Up @@ -94,7 +96,7 @@ def test_authenticated_userid(
helper.add_vary_by_cookie.assert_called_once_with(pyramid_request)
helper.identity.assert_called_once_with(html_authcookie, pyramid_request)
Identity.authenticated_userid.assert_called_once_with(
helper.identity.return_value
helper.identity.return_value[0]
)
assert authenticated_userid == Identity.authenticated_userid.return_value

Expand Down Expand Up @@ -122,9 +124,10 @@ def remember(cookie, *_args, **_kwargs):
headers = cookie_policy.remember(pyramid_request, sentinel.userid, foo="bar")

assert not pyramid_request.session
helper.add_ticket.assert_called_once_with(pyramid_request, sentinel.userid)
assert helper.remember.call_args_list == [
call(html_authcookie, pyramid_request, sentinel.userid),
call(api_authcookie, pyramid_request, sentinel.userid),
call(html_authcookie, sentinel.userid, helper.add_ticket.return_value),
call(api_authcookie, sentinel.userid, helper.add_ticket.return_value),
]
assert headers == [
sentinel.html_authcookie_header_1,
Expand Down Expand Up @@ -190,7 +193,7 @@ def test_permits(
helper.add_vary_by_cookie.assert_called_once_with(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
helper.identity.return_value[0], sentinel.context, sentinel.permission
)
assert permits == identity_permits.return_value

Expand All @@ -203,7 +206,7 @@ def html_authcookie(self, pyramid_request):
@pytest.fixture
def api_authcookie(self, pyramid_request):
api_authcookie = SignedCookieProfile(
"secret", "salt", cookie_name="h_api_authcookie"
"secret", "salt", cookie_name="h_api_authcookie.v2"
)

# Add the API auth cookie to the request.
Expand All @@ -215,8 +218,13 @@ def api_authcookie(self, pyramid_request):
return api_authcookie.bind(pyramid_request)

@pytest.fixture
def helper(self):
return create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
def helper(self, factories):
helper = create_autospec(AuthTicketCookieHelper, instance=True, spec_set=True)
helper.identity.return_value = (
create_autospec(Identity_, instance=True, spec_set=True),
factories.AuthTicket(),
)
return helper

@pytest.fixture
def cookie_policy(self, html_authcookie, api_authcookie, helper):
Expand Down
Loading
Loading