From de8010f8ee21802bfc0eea8bb3eacf820d03140d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Wegener?= Date: Mon, 17 Jun 2024 11:39:15 +0200 Subject: [PATCH 1/4] Add tests which describe desired behaviour for REFRESH_TOKEN_REUSE_PROTECTION (#1404) --- tests/test_authorization_code.py | 104 +++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/tests/test_authorization_code.py b/tests/test_authorization_code.py index b77f4f9ba..fad55c124 100644 --- a/tests/test_authorization_code.py +++ b/tests/test_authorization_code.py @@ -985,6 +985,53 @@ def test_refresh_fail_repeating_requests(self): response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) self.assertEqual(response.status_code, 400) + def test_refresh_repeating_requests_revokes_old_token(self): + """ + If a refresh token is reused, the server should invalidate *all* access tokens that have a relation + to the re-used token. This forces a malicious actor to be logged out. + The server can't determine whether the first or the second client was legitimate, so it needs to + revoke both. + See https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-29#name-recommendations + """ + self.oauth2_settings.REFRESH_TOKEN_REUSE_PROTECTION = True + self.client.login(username="test_user", password="123456") + authorization_code = self.get_auth() + + token_request_data = { + "grant_type": "authorization_code", + "code": authorization_code, + "redirect_uri": "http://example.org", + } + auth_headers = get_basic_auth_header(self.application.client_id, CLEARTEXT_SECRET) + + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + content = json.loads(response.content.decode("utf-8")) + self.assertTrue("refresh_token" in content) + + token_request_data = { + "grant_type": "refresh_token", + "refresh_token": content["refresh_token"], + "scope": content["scope"], + } + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 200) + new_tokens = json.loads(response.content.decode("utf-8")) + + # Second request fails + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 400) + + # Previously returned tokens are now invalid + new_token_request_data = { + "grant_type": "refresh_token", + "refresh_token": new_tokens["refresh_token"], + "scope": new_tokens["scope"], + } + response = self.client.post( + reverse("oauth2_provider:token"), data=new_token_request_data, **auth_headers + ) + self.assertEqual(response.status_code, 400) + def test_refresh_repeating_requests(self): """ Trying to refresh an access token with the same refresh token more than @@ -1024,6 +1071,63 @@ def test_refresh_repeating_requests(self): response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) self.assertEqual(response.status_code, 400) + def test_refresh_repeating_requests_grace_period_with_reuse_protection(self): + """ + Trying to refresh an access token with the same refresh token more than + once succeeds. Should work within the grace period, but should revoke previous tokens + """ + self.oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS = 120 + self.oauth2_settings.REFRESH_TOKEN_REUSE_PROTECTION = True + self.client.login(username="test_user", password="123456") + authorization_code = self.get_auth() + + token_request_data = { + "grant_type": "authorization_code", + "code": authorization_code, + "redirect_uri": "http://example.org", + } + auth_headers = get_basic_auth_header(self.application.client_id, CLEARTEXT_SECRET) + + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + content = json.loads(response.content.decode("utf-8")) + self.assertTrue("refresh_token" in content) + + refresh_token_1 = content["refresh_token"] + token_request_data = { + "grant_type": "refresh_token", + "refresh_token": refresh_token_1, + "scope": content["scope"], + } + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 200) + refresh_token_2 = json.loads(response.content.decode("utf-8"))["refresh_token"] + + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 200) + refresh_token_3 = json.loads(response.content.decode("utf-8"))["refresh_token"] + + self.assertEqual(refresh_token_2, refresh_token_3) + + # Let the first refresh token expire + rt = RefreshToken.objects.get(token=refresh_token_1) + rt.revoked = timezone.now() - datetime.timedelta(minutes=10) + rt.save() + + # Using the expired token fails + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 400) + + # Because we used the expired token, the recently issued token is also revoked + new_token_request_data = { + "grant_type": "refresh_token", + "refresh_token": refresh_token_2, + "scope": content["scope"], + } + response = self.client.post( + reverse("oauth2_provider:token"), data=new_token_request_data, **auth_headers + ) + self.assertEqual(response.status_code, 400) + def test_refresh_repeating_requests_non_rotating_tokens(self): """ Try refreshing an access token with the same refresh token more than once when not rotating tokens. From 529ad806c14fcadec6e884f836ce11ac62d18562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Wegener?= Date: Fri, 9 Aug 2024 18:48:18 +0200 Subject: [PATCH 2/4] Implement REFRESH_TOKEN_REUSE_PROTECTION (#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. --- .../0011_refreshtoken_token_family.py | 18 ++++++++ oauth2_provider/models.py | 1 + oauth2_provider/oauth2_validators.py | 36 ++++++++++------ oauth2_provider/settings.py | 1 + .../0006_basetestapplication_token_family.py | 43 +++++++++++++++++++ tests/test_authorization_code.py | 3 +- 6 files changed, 89 insertions(+), 13 deletions(-) create mode 100644 oauth2_provider/migrations/0011_refreshtoken_token_family.py create mode 100644 tests/migrations/0006_basetestapplication_token_family.py diff --git a/oauth2_provider/migrations/0011_refreshtoken_token_family.py b/oauth2_provider/migrations/0011_refreshtoken_token_family.py new file mode 100644 index 000000000..082f444de --- /dev/null +++ b/oauth2_provider/migrations/0011_refreshtoken_token_family.py @@ -0,0 +1,18 @@ +# Generated by Django 5.2 on 2024-08-09 16:40 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('oauth2_provider', '0010_application_allowed_origins'), + ] + + operations = [ + migrations.AddField( + model_name='refreshtoken', + name='token_family', + field=models.UUIDField(blank=True, editable=False, null=True), + ), + ] diff --git a/oauth2_provider/models.py b/oauth2_provider/models.py index 661bd7dfc..9895528de 100644 --- a/oauth2_provider/models.py +++ b/oauth2_provider/models.py @@ -490,6 +490,7 @@ class AbstractRefreshToken(models.Model): null=True, related_name="refresh_token", ) + token_family = models.UUIDField(null=True, blank=True, editable=False) created = models.DateTimeField(auto_now_add=True) updated = models.DateTimeField(auto_now=True) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index 47d65e851..d1cb8b9b6 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -15,7 +15,6 @@ from django.contrib.auth.hashers import check_password, identify_hasher from django.core.exceptions import ObjectDoesNotExist from django.db import transaction -from django.db.models import Q from django.http import HttpRequest from django.utils import dateformat, timezone from django.utils.crypto import constant_time_compare @@ -644,7 +643,9 @@ def save_bearer_token(self, token, request, *args, **kwargs): source_refresh_token=refresh_token_instance, ) - self._create_refresh_token(request, refresh_token_code, access_token) + self._create_refresh_token( + request, refresh_token_code, access_token, refresh_token_instance + ) else: # make sure that the token data we're returning matches # the existing token @@ -688,9 +689,17 @@ def _create_authorization_code(self, request, code, expires=None): claims=json.dumps(request.claims or {}), ) - def _create_refresh_token(self, request, refresh_token_code, access_token): + def _create_refresh_token(self, request, refresh_token_code, access_token, previous_refresh_token): + if previous_refresh_token: + token_family = previous_refresh_token.token_family + else: + token_family = uuid.uuid4() return RefreshToken.objects.create( - user=request.user, token=refresh_token_code, application=request.client, access_token=access_token + user=request.user, + token=refresh_token_code, + application=request.client, + access_token=access_token, + token_family=token_family, ) def revoke_token(self, token, token_type_hint, request, *args, **kwargs): @@ -752,22 +761,25 @@ def validate_refresh_token(self, refresh_token, client, request, *args, **kwargs Also attach User instance to the request object """ - null_or_recent = Q(revoked__isnull=True) | Q( - revoked__gt=timezone.now() - timedelta(seconds=oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS) - ) - rt = ( - RefreshToken.objects.filter(null_or_recent, token=refresh_token) - .select_related("access_token") - .first() - ) + rt = RefreshToken.objects.filter(token=refresh_token).select_related("access_token").first() if not rt: return False + if rt.revoked is not None and rt.revoked <= timezone.now() - timedelta( + seconds=oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS + ): + if oauth2_settings.REFRESH_TOKEN_REUSE_PROTECTION and rt.token_family: + rt_token_family = RefreshToken.objects.filter(token_family=rt.token_family) + for related_rt in rt_token_family.all(): + related_rt.revoke() + return False + request.user = rt.user request.refresh_token = rt.token # Temporary store RefreshToken instance to be reused by get_original_scopes and save_bearer_token. request.refresh_token_instance = rt + return rt.application == client @transaction.atomic diff --git a/oauth2_provider/settings.py b/oauth2_provider/settings.py index 950ab5643..329a1b354 100644 --- a/oauth2_provider/settings.py +++ b/oauth2_provider/settings.py @@ -54,6 +54,7 @@ "ID_TOKEN_EXPIRE_SECONDS": 36000, "REFRESH_TOKEN_EXPIRE_SECONDS": None, "REFRESH_TOKEN_GRACE_PERIOD_SECONDS": 0, + "REFRESH_TOKEN_REUSE_PROTECTION": False, "ROTATE_REFRESH_TOKEN": True, "ERROR_RESPONSE_WITH_SCOPES": False, "APPLICATION_MODEL": APPLICATION_MODEL, diff --git a/tests/migrations/0006_basetestapplication_token_family.py b/tests/migrations/0006_basetestapplication_token_family.py new file mode 100644 index 000000000..1a4dd19e1 --- /dev/null +++ b/tests/migrations/0006_basetestapplication_token_family.py @@ -0,0 +1,43 @@ +# Generated by Django 5.2 on 2024-08-09 16:40 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('tests', '0005_basetestapplication_allowed_origins_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='samplerefreshtoken', + name='token_family', + field=models.UUIDField(blank=True, editable=False, null=True), + ), + migrations.AlterField( + model_name='basetestapplication', + name='allowed_origins', + field=models.TextField(blank=True, default='', help_text='Allowed origins list to enable CORS, space separated'), + ), + migrations.AlterField( + model_name='basetestapplication', + name='post_logout_redirect_uris', + field=models.TextField(blank=True, default='', help_text='Allowed Post Logout URIs list, space separated'), + ), + migrations.AlterField( + model_name='sampleaccesstoken', + name='token', + field=models.CharField(db_index=True, max_length=255, unique=True), + ), + migrations.AlterField( + model_name='sampleapplication', + name='allowed_origins', + field=models.TextField(blank=True, default='', help_text='Allowed origins list to enable CORS, space separated'), + ), + migrations.AlterField( + model_name='sampleapplication', + name='post_logout_redirect_uris', + field=models.TextField(blank=True, default='', help_text='Allowed Post Logout URIs list, space separated'), + ), + ] diff --git a/tests/test_authorization_code.py b/tests/test_authorization_code.py index fad55c124..ae6e7e76e 100644 --- a/tests/test_authorization_code.py +++ b/tests/test_authorization_code.py @@ -1013,6 +1013,7 @@ def test_refresh_repeating_requests_revokes_old_token(self): "refresh_token": content["refresh_token"], "scope": content["scope"], } + # First response works as usual response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) self.assertEqual(response.status_code, 200) new_tokens = json.loads(response.content.decode("utf-8")) @@ -1021,7 +1022,7 @@ def test_refresh_repeating_requests_revokes_old_token(self): response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) self.assertEqual(response.status_code, 400) - # Previously returned tokens are now invalid + # Previously returned tokens are now invalid as well new_token_request_data = { "grant_type": "refresh_token", "refresh_token": new_tokens["refresh_token"], From 975a6ea27803322aa85e7259bd2ec7306290ff3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Wegener?= Date: Fri, 9 Aug 2024 18:59:38 +0200 Subject: [PATCH 3/4] Update documentation --- AUTHORS | 1 + CHANGELOG.md | 1 + docs/settings.rst | 12 ++++++++++++ 3 files changed, 14 insertions(+) diff --git a/AUTHORS b/AUTHORS index 17447b108..ce5ec2ec8 100644 --- a/AUTHORS +++ b/AUTHORS @@ -105,6 +105,7 @@ Shaheed Haque Shaun Stanworth Silvano Cerza Sora Yanai +Sören Wegener Spencer Carroll Stéphane Raimbault Tom Evans diff --git a/CHANGELOG.md b/CHANGELOG.md index 826ae43bc..ed1ec2e89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [unreleased] ### Added +* #1404 Add a new setting `REFRESH_TOKEN_REUSE_PROTECTION` ### Changed ### Deprecated ### Removed diff --git a/docs/settings.rst b/docs/settings.rst index 901fe8575..4ebe6cc47 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -185,6 +185,18 @@ The import string of the class (model) representing your refresh tokens. Overwri this value if you wrote your own implementation (subclass of ``oauth2_provider.models.RefreshToken``). +REFRESH_TOKEN_REUSE_PROTECTION +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +When this is set to ``True`` (default ``False``), and ``ROTATE_REFRESH_TOKEN`` is used, the server will check +if a previously, already revoked refresh token is used a second time. If it detects a reuse, it will automatically +revoke all related refresh tokens. +A reused refresh token indicates a breach. Since the server can't determine which request came from the legitimate +user and which from an attacker, it will end the session for both. The user is required to perform a new login. + +Can be used in combination with ``REFRESH_TOKEN_GRACE_PERIOD_SECONDS`` + +More details at https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-29#name-recommendations + ROTATE_REFRESH_TOKEN ~~~~~~~~~~~~~~~~~~~~ When is set to ``True`` (default) a new refresh token is issued to the client when the client refreshes an access token. From 302da0ddfddc01125d6f11852ebe7cfa5f6ebafa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Wegener?= Date: Tue, 13 Aug 2024 11:46:01 +0200 Subject: [PATCH 4/4] Add swappable dependency to REFRESH_TOKEN_MODEL --- .../0011_refreshtoken_token_family.py | 3 ++- .../0006_basetestapplication_token_family.py | 27 ++----------------- 2 files changed, 4 insertions(+), 26 deletions(-) diff --git a/oauth2_provider/migrations/0011_refreshtoken_token_family.py b/oauth2_provider/migrations/0011_refreshtoken_token_family.py index 082f444de..94fb4e171 100644 --- a/oauth2_provider/migrations/0011_refreshtoken_token_family.py +++ b/oauth2_provider/migrations/0011_refreshtoken_token_family.py @@ -1,12 +1,13 @@ # Generated by Django 5.2 on 2024-08-09 16:40 from django.db import migrations, models - +from oauth2_provider.settings import oauth2_settings class Migration(migrations.Migration): dependencies = [ ('oauth2_provider', '0010_application_allowed_origins'), + migrations.swappable_dependency(oauth2_settings.REFRESH_TOKEN_MODEL) ] operations = [ diff --git a/tests/migrations/0006_basetestapplication_token_family.py b/tests/migrations/0006_basetestapplication_token_family.py index 1a4dd19e1..6b065a242 100644 --- a/tests/migrations/0006_basetestapplication_token_family.py +++ b/tests/migrations/0006_basetestapplication_token_family.py @@ -1,12 +1,14 @@ # Generated by Django 5.2 on 2024-08-09 16:40 from django.db import migrations, models +from oauth2_provider.settings import oauth2_settings class Migration(migrations.Migration): dependencies = [ ('tests', '0005_basetestapplication_allowed_origins_and_more'), + migrations.swappable_dependency(oauth2_settings.REFRESH_TOKEN_MODEL) ] operations = [ @@ -15,29 +17,4 @@ class Migration(migrations.Migration): name='token_family', field=models.UUIDField(blank=True, editable=False, null=True), ), - migrations.AlterField( - model_name='basetestapplication', - name='allowed_origins', - field=models.TextField(blank=True, default='', help_text='Allowed origins list to enable CORS, space separated'), - ), - migrations.AlterField( - model_name='basetestapplication', - name='post_logout_redirect_uris', - field=models.TextField(blank=True, default='', help_text='Allowed Post Logout URIs list, space separated'), - ), - migrations.AlterField( - model_name='sampleaccesstoken', - name='token', - field=models.CharField(db_index=True, max_length=255, unique=True), - ), - migrations.AlterField( - model_name='sampleapplication', - name='allowed_origins', - field=models.TextField(blank=True, default='', help_text='Allowed origins list to enable CORS, space separated'), - ), - migrations.AlterField( - model_name='sampleapplication', - name='post_logout_redirect_uris', - field=models.TextField(blank=True, default='', help_text='Allowed Post Logout URIs list, space separated'), - ), ]