Hash application client secrets using Django password hashing (#1093)

* Add ClientSecretField field to use Django password hashing algorithms (#1020)

Co-authored-by: Andrew Chen Wang <[email protected]>
Co-authored-by: Peter Karman <[email protected]>
Co-authored-by: Andrew Chen Wang <[email protected]>

Author: 3 people

AUTHORS

@@ -64,6 +64,7 @@ Jadiel Teófilo
 pySilver
 Łukasz Skarżyński
 Shaheed Haque
+Peter Karman
 Vinay Karanam
 Eduardo Oliveira
 Andrea Greco

CHANGELOG.md

@@ -14,6 +14,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Security
   -->
 
+## [unreleased]
+
+## [2.0.0] unreleased
+
+### Changed
+* #1093 (**Breaking**) Changed to implement [hashed](https://docs.djangoproject.com/en/stable/topics/auth/passwords/)
+  client_secret values. This is a **breaking change** that will migrate all your existing
+  cleartext `application.client_secret` values to be hashed with Django's default password hashing algorithm
+  and can not be reversed. When adding or modifying an Application in the Admin console, you must copy the
+  auto-generated or manually-entered `client_secret` before hitting Save.
+
+
 ## [1.7.0] 2022-01-23
 
 ### Added

docs/settings.rst

@@ -97,19 +97,19 @@ of those three can be a callable) must be passed here directly and classes
 must be instantiated (callables should accept request as their only argument).
 
 GRANT_MODEL
-~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~
 The import string of the class (model) representing your grants. Overwrite
 this value if you wrote your own implementation (subclass of
 ``oauth2_provider.models.Grant``).
 
 APPLICATION_ADMIN_CLASS
-~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~
 The import string of the class (model) representing your application admin class.
 Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.ApplicationAdmin``).
 
 ACCESS_TOKEN_ADMIN_CLASS
-~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~
 The import string of the class (model) representing your access token admin class.
 Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.AccessTokenAdmin``).
@@ -121,7 +121,7 @@ Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.GrantAdmin``).
 
 REFRESH_TOKEN_ADMIN_CLASS
-~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~
 The import string of the class (model) representing your refresh token admin class.
 Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.RefreshTokenAdmin``).
@@ -154,7 +154,7 @@ If you don't change the validator code and don't run cleartokens all refresh
 tokens will last until revoked or the end of time. You should change this.
 
 REFRESH_TOKEN_GRACE_PERIOD_SECONDS
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 The number of seconds between when a refresh token is first used when it is
 expired. The most common case of this for this is native mobile applications
 that run into issues of network connectivity during the refresh cycle and are
@@ -178,7 +178,7 @@ See also: validator's rotate_refresh_token method can be overridden to make this
 when close to expiration, theoretically).
 
 REFRESH_TOKEN_GENERATOR
-~~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~
 See `ACCESS_TOKEN_GENERATOR`. This is the same but for refresh tokens.
 Defaults to access token generator if not provided.
 
@@ -265,7 +265,7 @@ Default: ``""``
 The RSA private key used to sign OIDC ID tokens. If not set, OIDC is disabled.
 
 OIDC_RSA_PRIVATE_KEYS_INACTIVE
-~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Default: ``[]``
 
 An array of *inactive* RSA private keys. These keys are not used to sign tokens,
@@ -276,7 +276,7 @@ This is useful for providing a smooth transition during key rotation.
 should be retained in this inactive list.
 
 OIDC_JWKS_MAX_AGE_SECONDS
-~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~
 Default: ``3600``
 
 The max-age value for the Cache-Control header on jwks_uri.
@@ -354,9 +354,9 @@ load when clearing large batches of expired tokens.
 
 
 Settings imported from Django project
---------------------------
+-------------------------------------
 
 USE_TZ
-~~~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~
 
 Used to determine whether or not to make token expire dates timezone aware.

oauth2_provider/migrations/0006_alter_application_client_secret.py

@@ -0,0 +1,31 @@
+from django.db import migrations
+from django.contrib.auth.hashers import identify_hasher, make_password
+import logging
+import oauth2_provider.generators
+import oauth2_provider.models
+
+
+def forwards_func(apps, schema_editor):
+    """
+    Forward migration touches every application.client_secret which will cause it to be hashed if not already the case.
+    """
+    Application = apps.get_model('oauth2_provider', 'application')
+    applications = Application.objects.all()
+    for application in applications:
+        application.save(update_fields=['client_secret'])
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('oauth2_provider', '0005_auto_20211222_2352'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='application',
+            name='client_secret',
+            field=oauth2_provider.models.ClientSecretField(blank=True, db_index=True, default=oauth2_provider.generators.generate_client_secret, help_text='Hashed on Save. Copy it now if this is a new secret.', max_length=255),
+        ),
+        migrations.RunPython(forwards_func),
+    ]

oauth2_provider/models.py

@@ -6,6 +6,7 @@
 
 from django.apps import apps
 from django.conf import settings
+from django.contrib.auth.hashers import identify_hasher, make_password
 from django.core.exceptions import ImproperlyConfigured
 from django.db import models, transaction
 from django.urls import reverse
@@ -24,6 +25,20 @@
 logger = logging.getLogger(__name__)
 
 
+class ClientSecretField(models.CharField):
+    def pre_save(self, model_instance, add):
+        secret = getattr(model_instance, self.attname)
+        try:
+            hasher = identify_hasher(secret)
+            logger.debug(f"{model_instance}: {self.attname} is already hashed with {hasher}.")
+        except ValueError:
+            logger.debug(f"{model_instance}: {self.attname} is not hashed; hashing it now.")
+            hashed_secret = make_password(secret)
+            setattr(model_instance, self.attname, hashed_secret)
+            return hashed_secret
+        return super().pre_save(model_instance, add)
+
+
 class AbstractApplication(models.Model):
     """
     An Application instance represents a Client on the Authorization server.
@@ -90,8 +105,12 @@ class AbstractApplication(models.Model):
     )
     client_type = models.CharField(max_length=32, choices=CLIENT_TYPES)
     authorization_grant_type = models.CharField(max_length=32, choices=GRANT_TYPES)
-    client_secret = models.CharField(
-        max_length=255, blank=True, default=generate_client_secret, db_index=True
+    client_secret = ClientSecretField(
+        max_length=255,
+        blank=True,
+        default=generate_client_secret,
+        db_index=True,
+        help_text=_("Hashed on Save. Copy it now if this is a new secret."),
     )
     name = models.CharField(max_length=255, blank=True)
     skip_authorization = models.BooleanField(default=False)

oauth2_provider/oauth2_validators.py

@@ -12,6 +12,7 @@
 import requests
 from django.conf import settings
 from django.contrib.auth import authenticate, get_user_model
+from django.contrib.auth.hashers import check_password
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import transaction
 from django.db.models import Q
@@ -123,7 +124,7 @@ def _authenticate_basic_auth(self, request):
         elif request.client.client_id != client_id:
             log.debug("Failed basic auth: wrong client id %s" % client_id)
             return False
-        elif request.client.client_secret != client_secret:
+        elif not check_password(client_secret, request.client.client_secret):
             log.debug("Failed basic auth: wrong client secret %s" % client_secret)
             return False
         else:
@@ -148,7 +149,7 @@ def _authenticate_request_body(self, request):
         if self._load_application(client_id, request) is None:
             log.debug("Failed body auth: Application %s does not exists" % client_id)
             return False
-        elif request.client.client_secret != client_secret:
+        elif not check_password(client_secret, request.client.client_secret):
             log.debug("Failed body auth: wrong client secret %s" % client_secret)
             return False
         else:

tests/conftest.py

@@ -16,6 +16,8 @@
 Application = get_application_model()
 UserModel = get_user_model()
 
+CLEARTEXT_SECRET = "1234567890abcdefghijklmnopqrstuvwxyz"
+
 
 class OAuthSettingsWrapper:
     """
@@ -101,12 +103,14 @@ def application():
         client_type=Application.CLIENT_CONFIDENTIAL,
         authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE,
         algorithm=Application.RS256_ALGORITHM,
+        client_secret=CLEARTEXT_SECRET,
     )
 
 
 @pytest.fixture
 def hybrid_application(application):
     application.authorization_grant_type = application.GRANT_OPENID_HYBRID
+    application.client_secret = CLEARTEXT_SECRET
     application.save()
     return application
 
@@ -141,7 +145,7 @@ def oidc_tokens(oauth2_settings, application, test_user, client):
             "code": code,
             "redirect_uri": "http://example.org",
             "client_id": application.client_id,
-            "client_secret": application.client_secret,
+            "client_secret": CLEARTEXT_SECRET,
             "scope": "openid",
         },
     )