Merge pull request Styria-Digital#84 from ashokdelphia/improve-logout-security

Improve logout security

Author: fitodic

changelog.d/84.feature.md

@@ -0,0 +1,3 @@
+Apply 'blacklist' to any token from the same line of refreshed tokens as any invalidated token, where token ids are available.
+Avoid storing whole auth tokens when JWT_TOKEN_ID setting is set to 'require'.
+Please see notes in docs on migrating from JWT_TOKEN_ID 'allow' (the default) to 'require' (recommended).

docs/index.md

@@ -201,6 +201,10 @@ urlpatterns = [
 
 When called, deletes all blacklisted tokens that have expired.
 
+### Warning
+
+Unless `JWT_TOKEN_ID` is set to `require`, blacklisting tokens will store the entire token value. This creates a potential problem if someone is able to read and delete records from the blacklist, either directly in the database or via the administrative interface. Note that the default value is `include`, not `require`. See the section on `JWT_TOKEN_ID` for how to migrate to requiring token id claims in all tokens.
+
 ## Additional Settings
 There are some additional settings that you can override similar to how you'd do it with Django REST framework itself. Here are all the available defaults.
 
@@ -369,6 +373,8 @@ For new installations, please override the default and set this to `require`, as
 
 For existing installations, when migrating from an older version (pre-1.17) or when changing the setting from `off`, we recommend setting this to `require` once all of the valid tokens have the id claims. This will typically be after `JWT_EXPIRATION_DELTA` has elapsed since upgrading or allowing id claims to be included.
 
+Note that when set to `off` or `include`, the blacklist functionality - if used - will store the entire token value, which would allow someone with access to the administrative interface, or directly to the database, to steal an otherwise valid token and remove it from the blacklist. Using `require` for this setting means that only token identifiers are recorded for the blacklist and not entire tokens.
+
 Default is `include`.
 
 ### JWT_AUDIENCE

src/rest_framework_jwt/authentication.py

@@ -68,12 +68,6 @@ def authenticate(self, request):
         except MissingToken:
             return None
 
-        if apps.is_installed('rest_framework_jwt.blacklist'):
-            from rest_framework_jwt.blacklist.models import BlacklistedToken
-            if BlacklistedToken.objects.filter(token=force_str(token)).exists():
-                msg = _('Token is blacklisted.')
-                raise exceptions.PermissionDenied(msg)
-
         try:
             payload = self.jwt_decode_token(token)
         except jwt.ExpiredSignature:
@@ -86,6 +80,12 @@ def authenticate(self, request):
             msg = _('Invalid token.')
             raise exceptions.AuthenticationFailed(msg)
 
+        if apps.is_installed('rest_framework_jwt.blacklist'):
+            from rest_framework_jwt.blacklist.models import BlacklistedToken
+            if BlacklistedToken.is_blocked(token, payload):
+                msg = _('Token is blacklisted.')
+                raise exceptions.PermissionDenied(msg)
+
         user = self.authenticate_credentials(payload)
 
         return user, token

src/rest_framework_jwt/blacklist/migrations/0002_add_token_id.py

@@ -0,0 +1,49 @@
+# Generated by Django 3.1.4 on 2020-12-12 12:15
+
+from django.db import migrations, models
+from django import VERSION
+
+import jwt
+
+
+def add_token_id_values(apps, schema_editor):
+    """Fill in token_id values for existing token records that have a token id
+    """
+    BlacklistedToken = apps.get_model('blacklist', 'BlacklistedToken')
+    for row in BlacklistedToken.objects.filter(token_id=None):
+        payload = jwt.decode(row.token)
+        token_id = payload.get('orig_jti') or payload.get('jti')
+        if token_id:
+            row.token_id = token_id
+            row.save(update_fields=['token_id'])
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('blacklist', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='blacklistedtoken',
+            name='token_id',
+            field=models.UUIDField(db_index=True, null=True),
+        ),
+        migrations.AlterField(
+            model_name='blacklistedtoken',
+            name='token',
+            field=models.TextField(db_index=True, null=True),
+        ),
+        migrations.RunPython(add_token_id_values, reverse_code=migrations.RunPython.noop)
+    ]
+    if VERSION >= (2, 2):
+        operations.append(
+            migrations.AddConstraint(
+                model_name='blacklistedtoken',
+                constraint=models.CheckConstraint(
+                    check=models.Q(token_id__isnull=False) | models.Q(token__isnull=False),
+                    name='token_or_id_not_null',
+                ),
+            )
+        )

src/rest_framework_jwt/blacklist/models.py

@@ -1,8 +1,13 @@
 # -*- coding: utf-8 -*-
 
+from django import VERSION
 from django.conf import settings
 from django.db import models
+from django.db.models import Q
 from django.utils import timezone
+from django.utils.encoding import force_str
+
+from rest_framework_jwt.settings import api_settings
 
 
 class BlacklistedTokenManager(models.Manager):
@@ -11,7 +16,18 @@ def delete_stale_tokens(self):
 
 
 class BlacklistedToken(models.Model):
-    token = models.TextField(db_index=True)
+    class Meta:
+        if VERSION >= (2, 2):
+            constraints = [
+                models.CheckConstraint(
+                    check=Q(token_id__isnull=False) | Q(token__isnull=False),
+                    name='token_or_id_not_null',
+                )
+            ]
+
+    # This holds the original token id for refreshed tokens with ids
+    token_id = models.UUIDField(db_index=True, null=True)
+    token = models.TextField(db_index=True, null=True)
     user = models.ForeignKey(
         settings.AUTH_USER_MODEL, on_delete=models.CASCADE)
     expires_at = models.DateTimeField(db_index=True)
@@ -21,3 +37,22 @@ class BlacklistedToken(models.Model):
 
     def __str__(self):
         return 'Blacklisted token - {} - {}'.format(self.user, self.token)
+
+
+    @staticmethod
+    def is_blocked(token, payload):
+        token = force_str(token)
+
+        # For invalidated tokens that have an original token id (orig_jti),
+        # we record that in the list, so that the whole family of tokens
+        # refreshed from the same initial token is rejected.
+        token_id = payload.get('orig_jti') or payload.get('jti')
+
+        if api_settings.JWT_TOKEN_ID == 'require':
+            query = Q(token_id=token_id)
+        elif api_settings.JWT_TOKEN_ID == 'off':
+            query = Q(token=token)
+        else:
+            query = Q(token__isnull=False, token=token) | Q(token_id__isnull=False, token_id=token_id)
+
+        return BlacklistedToken.objects.filter(query).exists()

src/rest_framework_jwt/blacklist/permissions.py

@@ -1,3 +1,5 @@
+import jwt
+
 from rest_framework.permissions import BasePermission
 
 from rest_framework_jwt.authentication import JSONWebTokenAuthentication
@@ -9,6 +11,12 @@ class IsNotBlacklisted(BasePermission):
     message = _('You have been blacklisted.')
 
     def has_permission(self, request, view):
-        return not BlacklistedToken.objects.filter(
-            token=JSONWebTokenAuthentication.get_token_from_request(request)
-        ).exists()
+        token = JSONWebTokenAuthentication.get_token_from_request(request)
+
+        # Don't check the blacklist for requests with no token.
+        if token is None:
+            return True
+
+        # The token should already be validated before we call this.
+        payload = jwt.decode(token, None, False)
+        return not BlacklistedToken.is_blocked(token, payload)

src/rest_framework_jwt/blacklist/serializers.py

@@ -37,9 +37,21 @@ def save(self, **kwargs):
         iat = payload.get('iat', unix_epoch())
         expires_at_unix_time = iat + api_settings.JWT_EXPIRATION_DELTA.total_seconds()
 
+        # For refreshed tokens, record the token id of the original token.
+        # This allows us to invalidate the whole family of tokens from
+        # the same original authentication event.
+        token_id = payload.get('orig_jti') or payload.get('jti')
+
         self.validated_data.update({
+            'token_id': token_id,
             'user': check_user(payload),
             'expires_at':
                 make_aware(datetime.utcfromtimestamp(expires_at_unix_time)),
         })
+
+        # Don't store the token if we can rely on token IDs.
+        # The token values are still sensitive until they expire.
+        if api_settings.JWT_TOKEN_ID == 'require':
+            del self.validated_data['token']
+
         return super(BlacklistTokenSerializer, self).save(**kwargs)

src/rest_framework_jwt/utils.py

@@ -10,7 +10,6 @@
 
 from django.apps import apps
 from django.contrib.auth import get_user_model
-from django.utils.encoding import force_str
 
 from rest_framework import serializers
 from rest_framework.utils.encoders import JSONEncoder
@@ -204,12 +203,6 @@ def jwt_create_response_payload(
 def check_payload(token):
     from rest_framework_jwt.authentication import JSONWebTokenAuthentication
 
-    if apps.is_installed('rest_framework_jwt.blacklist'):
-        from rest_framework_jwt.blacklist.models import BlacklistedToken
-        if BlacklistedToken.objects.filter(token=force_str(token)).exists():
-            msg = _('Token is blacklisted.')
-            raise serializers.ValidationError(msg)
-
     try:
         payload = JSONWebTokenAuthentication.jwt_decode_token(token)
     except jwt.ExpiredSignature:
@@ -222,6 +215,12 @@ def check_payload(token):
         msg = _('Invalid token.')
         raise serializers.ValidationError(msg)
 
+    if apps.is_installed('rest_framework_jwt.blacklist'):
+        from rest_framework_jwt.blacklist.models import BlacklistedToken
+        if BlacklistedToken.is_blocked(token, payload):
+            msg = _('Token is blacklisted.')
+            raise serializers.ValidationError(msg)
+
     return payload
 
 

tests/conftest.py

@@ -6,6 +6,7 @@
 
 from django.contrib.auth import get_user_model
 
+from rest_framework.reverse import reverse
 from rest_framework.test import APIClient
 
 from rest_framework_jwt.authentication import JSONWebTokenAuthentication
@@ -80,3 +81,13 @@ def _create_user(**kwargs):
         return User.objects.create_user(**kwargs)
 
     return _create_user
+
+
+@pytest.fixture
+def call_auth_refresh_endpoint(api_client, db):
+    def _call_auth_refresh_endpoint(token):
+        url = reverse("auth-refresh")
+        data = {"token": token}
+        return api_client.post(path=url, data=data)
+
+    return _call_auth_refresh_endpoint

tests/models/test_blacklisted_token.py

@@ -0,0 +1,102 @@
+# -*- coding: utf-8 -*-
+
+from datetime import timedelta
+from django.utils import timezone
+
+import pytest
+
+from rest_framework_jwt.authentication import JSONWebTokenAuthentication
+from rest_framework_jwt.blacklist.models import BlacklistedToken
+from rest_framework_jwt.settings import api_settings
+
+import uuid
+
+
+@pytest.mark.parametrize(
+    'id_setting', ['require', 'include']
+)
+def test_token_is_blocked_by_id(user, monkeypatch, id_setting):
+    monkeypatch.setattr(api_settings, "JWT_TOKEN_ID", id_setting)
+    payload = JSONWebTokenAuthentication.jwt_create_payload(user)
+    token = JSONWebTokenAuthentication.jwt_encode_payload(payload)
+
+    expiration = timezone.now() + timedelta(days=1)
+    BlacklistedToken(
+        token_id=payload['jti'],
+        expires_at=expiration,
+        user=user,
+    ).save()
+
+    assert BlacklistedToken.is_blocked(token, payload) is True
+
+
+
+@pytest.mark.parametrize(
+    'id_setting', ['require', 'include']
+)
+def test_refreshed_token_is_blocked_by_original_id(user, call_auth_refresh_endpoint, monkeypatch, id_setting):
+    monkeypatch.setattr(api_settings, "JWT_TOKEN_ID", id_setting)
+    original_payload = JSONWebTokenAuthentication.jwt_create_payload(user)
+    original_token = JSONWebTokenAuthentication.jwt_encode_payload(original_payload)
+
+    refresh_response = call_auth_refresh_endpoint(original_token)
+    refreshed_token = refresh_response.json()['token']
+    payload = JSONWebTokenAuthentication.jwt_decode_token(refreshed_token)
+
+    expiration = timezone.now() + timedelta(days=1)
+    BlacklistedToken(
+        token_id=original_payload['jti'],
+        expires_at=expiration,
+        user=user,
+    ).save()
+
+    assert BlacklistedToken.is_blocked(refreshed_token, payload) is True
+
+
+@pytest.mark.parametrize(
+    'id_setting', ['include', 'off']
+)
+def test_token_is_blocked_by_value(user, monkeypatch, id_setting):
+    monkeypatch.setattr(api_settings, "JWT_TOKEN_ID", id_setting)
+    payload = JSONWebTokenAuthentication.jwt_create_payload(user)
+    token = JSONWebTokenAuthentication.jwt_encode_payload(payload)
+
+    expiration = timezone.now() + timedelta(days=1)
+    BlacklistedToken(
+        token=token,
+        expires_at=expiration,
+        user=user,
+    ).save()
+
+    assert BlacklistedToken.is_blocked(token, payload) is True
+
+
+def test_token_is_not_blocked_by_value_when_ids_required(user, monkeypatch):
+    monkeypatch.setattr(api_settings, "JWT_TOKEN_ID", "require")
+    payload = JSONWebTokenAuthentication.jwt_create_payload(user)
+    token = JSONWebTokenAuthentication.jwt_encode_payload(payload)
+
+    expiration = timezone.now() + timedelta(days=1)
+    BlacklistedToken(
+        token=token,
+        expires_at=expiration,
+        user=user,
+    ).save()
+
+    assert BlacklistedToken.is_blocked(token, payload) is False
+
+
+def test_token_is_not_blocked_by_id_when_ids_disabled(user, monkeypatch):
+    monkeypatch.setattr(api_settings, "JWT_TOKEN_ID", "off")
+    payload = JSONWebTokenAuthentication.jwt_create_payload(user)
+    payload['jti'] = uuid.uuid4()
+    token = JSONWebTokenAuthentication.jwt_encode_payload(payload)
+
+    expiration = timezone.now() + timedelta(days=1)
+    BlacklistedToken(
+        token_id=payload['jti'],
+        expires_at=expiration,
+        user=user,
+    ).save()
+
+    assert BlacklistedToken.is_blocked(token, payload) is False

tests/views/conftest.py

@@ -25,13 +25,3 @@ def _call_auth_verify_endpoint(token):
         return api_client.post(path=url, data=data)
 
     return _call_auth_verify_endpoint
-
-
-@pytest.fixture
-def call_auth_refresh_endpoint(api_client, db):
-    def _call_auth_refresh_endpoint(token):
-        url = reverse("auth-refresh")
-        data = {"token": token}
-        return api_client.post(path=url, data=data)
-
-    return _call_auth_refresh_endpoint