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.

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

Refresh Token Reuse Protection #1452

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

soerface
Copy link
Contributor

@soerface soerface commented Aug 9, 2024

Fixes #1404

Description of the Change

This PR adds a new setting / feature REFRESH_TOKEN_REUSE_PROTECTION. It allows django-oauth-toolkit to revoke related refresh and access tokens whenever an already revoked refresh token is used again. This indicates a breach - for details see #1404.

The implementation does it by adding a new column token_family to the refresh token table. When a new refresh token is created with an existing refresh token, the new refresh token get's assigned the same token_family value as the original token. This ensures that all tokens from the same session share the same family. Otherwise (first token of a session, gathered by e.g. username & password), the token_family is a random value.

Whenever a revoked refresh token is presented, the server will query for all tokens with the same token_family value, and call the .revoke() method on it.

To discuss: The "select all tokens of a family and revoke" code is always being run when a revoked token is presented. Maybe this could provide a vector for a denial of service attack. Therefore, I was wondering if we should set the token_family value to NULL after the whole family has been revoked. Another thought to ease database load was furthermore to use objects.filter(...).delete(), however I was afraid that the logic inside the .revoke() method was relevant and shouldn't be skipped lightheartedly. Furthermore, do you think an index on that new column would be required?

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@soerface soerface force-pushed the 1404-refresh-token-reuse-protection branch from 2774980 to eb8501e Compare August 9, 2024 17:18
@soerface soerface force-pushed the 1404-refresh-token-reuse-protection branch from eb8501e to fe093a0 Compare August 9, 2024 20:02
Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

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

@soerface This is a really nice security improvement by following the newer BCPs. I agree that the default for now has to be False to avoid immediate breakage for existing users. We can perhaps raise a DeprecationWarning and eventually make the default be True for a future major release.

I have some questions about the migrations and whether they need migrations.swappable_dependency added to them. I'm stumped by this swappable stuff and you can see there are a number of issues raised over the years by people trying to use the functionality....

According to https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-29#name-recommendations, the authorization server needs a way to determine which refresh tokens belong to the same session, so it is able to figure out which tokens to revoke. Therefore, this commit introduces a "token_family" field to the RefreshToken table. Whenever a revoked refresh token is reused, the auth server uses the token family to revoke all related tokens.
@n2ygk n2ygk force-pushed the 1404-refresh-token-reuse-protection branch from a357ff0 to 302da0d Compare August 13, 2024 13:53
@n2ygk n2ygk merged commit 51d9798 into jazzband:master Aug 13, 2024
27 checks passed
shaleh pushed a commit to shaleh/django-oauth-toolkit that referenced this pull request Aug 13, 2024
* Implement REFRESH_TOKEN_REUSE_PROTECTION (jazzband#1404)

According to https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-29#name-recommendations, the authorization server needs a way to determine which refresh tokens belong to the same session, so it is able to figure out which tokens to revoke. Therefore, this commit introduces a "token_family" field to the RefreshToken table. Whenever a revoked refresh token is reused, the auth server uses the token family to revoke all related tokens.
@n2ygk n2ygk added this to the 3.0.0 milestone Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh token reuse detection
2 participants