-
-
Notifications
You must be signed in to change notification settings - Fork 794
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
Refresh Token Reuse Protection #1452
Conversation
2774980
to
eb8501e
Compare
eb8501e
to
fe093a0
Compare
There was a problem hiding this 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.
a357ff0
to
302da0d
Compare
* 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.
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 sametoken_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), thetoken_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 useobjects.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
CHANGELOG.md
updated (only for user relevant changes)AUTHORS