Allow the use of unhashed secrets (#1311)

gardenerik · n2ygk · web-flow · commit 0965100ce9e3 · 2023-09-13T14:51:21.000-04:00
* enable configuration of Applications to keep the client_secret unhashed to enable properly signed JWTs

---------

Co-authored-by: Alan Crosswell &lt;alan@crosswell.us&gt;
Co-authored-by: Alan Crosswell &lt;alan@columbia.edu&gt;
diff --git a/AUTHORS b/AUTHORS
@@ -9,6 +9,7 @@ Contributors
 
 Abhishek Patel
 Adam Johnson
+Adam Zahradník
 Adheeth P Praveen
 Alan Crosswell
 Alejandro Mantecon Guillen
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * #1185 Add middleware for adding access token to request
 * #1273 Add caching of loading of OIDC private key.
 * #1285 Add post_logout_redirect_uris field in application views.
+* #1311 Add option to disable client_secret hashing to allow verifying JWTs' signatures.
 
 - ### Fixed
 * #1284 Allow to logout whith no id_token_hint even if the browser session already expired
diff --git a/docs/_images/application-register-auth-code.png b/docs/_images/application-register-auth-code.png
diff --git a/docs/_images/application-register-client-credential.png b/docs/_images/application-register-client-credential.png
diff --git a/docs/getting_started.rst b/docs/getting_started.rst
@@ -244,7 +244,9 @@ Start the development server::
 
 Point your browser to http://127.0.0.1:8000/o/applications/register/ lets create an application.
 
-Fill the form as show in the screenshot below and before save take note of ``Client id`` and ``Client secret`` we will use it in a minute.
+Fill the form as show in the screenshot below and before save take note of ``Client id`` and ``Client secret``, we will use it in a minute.
+
+If you want to use this application with OIDC and ``HS256`` (see :doc:`OpenID Connect <oidc>`), uncheck ``Hash client secret`` to allow verifying tokens using JWT signatures. This means your client secret will be stored in cleartext but is the only way to successfully use signed JWT's.
 
 .. image:: _images/application-register-auth-code.png
    :alt: Authorization code application registration
diff --git a/docs/oidc.rst b/docs/oidc.rst
@@ -133,6 +133,9 @@ If you would prefer to use just ``HS256`` keys, you don't need to create any
 additional keys, ``django-oauth-toolkit`` will just use the application's
 ``client_secret`` to sign the JWT token.
 
+To be able to verify the JWT's signature using the ``client_secret``, you
+must set the application's ``hash_client_secret`` to ``False``.
+
 In this case, you just need to enable OIDC and add ``openid`` to your list of
 scopes in your ``settings.py``::
 
diff --git a/docs/tutorial/tutorial_01.rst b/docs/tutorial/tutorial_01.rst
@@ -99,6 +99,11 @@ point your browser to http://localhost:8000/o/applications/ and add an Applicati
  * `Name`: this is the name of the client application on the server, and will be displayed on the authorization request
    page, where users can allow/deny access to their data.
 
+ * `Hash client secret`: checking this hashes the client secret on save so it cannot be retrieved later. This should be
+   unchecked if you plan to use OIDC with ``HS256`` and want to check the tokens' signatures using JWT. Otherwise,
+   Django OAuth Toolkit cannot use `Client Secret` to sign the tokens (as it cannot be retrieved later) and the hashed
+   value will be used when signing. This may lead to incompatibilities with some OIDC Relying Party libraries.
+
 Take note of the `Client id` and the `Client Secret` then logout (this is needed only for testing the authorization
 process we'll explain shortly)
 
diff --git a/oauth2_provider/management/commands/createapplication.py b/oauth2_provider/management/commands/createapplication.py
@@ -48,6 +48,13 @@ def add_arguments(self, parser):
             type=str,
             help="The secret for this application",
         )
+        parser.add_argument(
+            "--no-hash-client-secret",
+            dest="hash_client_secret",
+            action="store_false",
+            help="Don't hash the client secret",
+        )
+        parser.set_defaults(hash_client_secret=True)
         parser.add_argument(
             "--name",
             type=str,
@@ -74,7 +81,7 @@ def handle(self, *args, **options):
             # Data in options must be cleaned because there are unneeded key-value like
             # verbosity and others. Also do not pass any None to the Application
             # instance so default values will be generated for those fields
-            if key in application_fields and value:
+            if key in application_fields and (isinstance(value, bool) or value):
                 if key == "user":
                     application_data.update({"user_id": value})
                 else:
diff --git a/oauth2_provider/migrations/0009_add_hash_client_secret.py b/oauth2_provider/migrations/0009_add_hash_client_secret.py
@@ -0,0 +1,18 @@
+# Generated by Django 4.2.5 on 2023-09-07 19:26
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('oauth2_provider', '0008_alter_accesstoken_token'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='application',
+            name='hash_client_secret',
+            field=models.BooleanField(default=True),
+        ),
+    ]
diff --git a/oauth2_provider/models.py b/oauth2_provider/models.py
@@ -29,6 +29,10 @@
 class ClientSecretField(models.CharField):
     def pre_save(self, model_instance, add):
         secret = getattr(model_instance, self.attname)
+        should_be_hashed = getattr(model_instance, "hash_client_secret", True)
+        if not should_be_hashed:
+            return super().pre_save(model_instance, add)
+
         try:
             hasher = identify_hasher(secret)
             logger.debug(f"{model_instance}: {self.attname} is already hashed with {hasher}.")
@@ -120,6 +124,7 @@ class AbstractApplication(models.Model):
         db_index=True,
         help_text=_("Hashed on Save. Copy it now if this is a new secret."),
     )
+    hash_client_secret = models.BooleanField(default=True)
     name = models.CharField(max_length=255, blank=True)
     skip_authorization = models.BooleanField(default=False)
 
diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py
@@ -12,12 +12,13 @@
 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.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
 from django.utils.timezone import make_aware
 from django.utils.translation import gettext_lazy as _
 from jwcrypto import jws, jwt
@@ -112,6 +113,18 @@ def _extract_basic_auth(self, request):
 
         return auth_string
 
+    def _check_secret(self, provided_secret, stored_secret):
+        """
+        Checks whether the provided client secret is valid.
+
+        Supports both hashed and unhashed secrets.
+        """
+        try:
+            identify_hasher(stored_secret)
+            return check_password(provided_secret, stored_secret)
+        except ValueError:  # Raised if the stored_secret is not hashed.
+            return constant_time_compare(provided_secret, stored_secret)
+
     def _authenticate_basic_auth(self, request):
         """
         Authenticates with HTTP Basic Auth.
@@ -152,7 +165,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 not check_password(client_secret, request.client.client_secret):
+        elif not self._check_secret(client_secret, request.client.client_secret):
             log.debug("Failed basic auth: wrong client secret %s" % client_secret)
             return False
         else:
diff --git a/oauth2_provider/templates/oauth2_provider/application_detail.html b/oauth2_provider/templates/oauth2_provider/application_detail.html
@@ -16,6 +16,11 @@ <h3 class="block-center-heading">{{ application.name }}</h3>
                 <input class="input-block-level" type="text" value="{{ application.client_secret }}" readonly>
             </li>
 
+            <li>
+                <p><b>{% trans "Hash client secret" %}</b></p>
+                <p>{{ application.hash_client_secret|yesno:_("yes,no") }}</p>
+            </li>
+
             <li>
                 <p><b>{% trans "Client type" %}</b></p>
                 <p>{{ application.client_type }}</p>
diff --git a/oauth2_provider/views/application.py b/oauth2_provider/views/application.py
@@ -34,6 +34,7 @@ def get_form_class(self):
                 "name",
                 "client_id",
                 "client_secret",
+                "hash_client_secret",
                 "client_type",
                 "authorization_grant_type",
                 "redirect_uris",
@@ -93,6 +94,7 @@ def get_form_class(self):
                 "name",
                 "client_id",
                 "client_secret",
+                "hash_client_secret",
                 "client_type",
                 "authorization_grant_type",
                 "redirect_uris",
diff --git a/tests/migrations/0004_basetestapplication_hash_client_secret_and_more.py b/tests/migrations/0004_basetestapplication_hash_client_secret_and_more.py
@@ -0,0 +1,31 @@
+# Generated by Django 4.2.5 on 2023-09-07 19:28
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.OAUTH2_PROVIDER_ID_TOKEN_MODEL),
+        ('tests', '0003_basetestapplication_post_logout_redirect_uris_and_more'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='basetestapplication',
+            name='hash_client_secret',
+            field=models.BooleanField(default=True),
+        ),
+        migrations.AddField(
+            model_name='sampleapplication',
+            name='hash_client_secret',
+            field=models.BooleanField(default=True),
+        ),
+        migrations.AlterField(
+            model_name='sampleaccesstoken',
+            name='id_token',
+            field=models.OneToOneField(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='s_access_token', to=settings.OAUTH2_PROVIDER_ID_TOKEN_MODEL),
+        ),
+    ]
diff --git a/tests/test_models.py b/tests/test_models.py
@@ -2,6 +2,7 @@
 
 import pytest
 from django.contrib.auth import get_user_model
+from django.contrib.auth.hashers import check_password
 from django.core.exceptions import ImproperlyConfigured, ValidationError
 from django.test import TestCase
 from django.test.utils import override_settings
@@ -19,6 +20,8 @@
 from . import presets
 
 
+CLEARTEXT_SECRET = "1234567890abcdefghijklmnopqrstuvwxyz"
+
 Application = get_application_model()
 Grant = get_grant_model()
 AccessToken = get_access_token_model()
@@ -54,6 +57,33 @@ def test_allow_scopes(self):
         self.assertTrue(access_token.allow_scopes([]))
         self.assertFalse(access_token.allow_scopes(["write", "destroy"]))
 
+    def test_hashed_secret(self):
+        app = Application.objects.create(
+            name="test_app",
+            redirect_uris="http://localhost http://example.com http://example.org",
+            user=self.user,
+            client_type=Application.CLIENT_CONFIDENTIAL,
+            authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE,
+            client_secret=CLEARTEXT_SECRET,
+            hash_client_secret=True,
+        )
+
+        self.assertNotEqual(app.client_secret, CLEARTEXT_SECRET)
+        self.assertTrue(check_password(CLEARTEXT_SECRET, app.client_secret))
+
+    def test_unhashed_secret(self):
+        app = Application.objects.create(
+            name="test_app",
+            redirect_uris="http://localhost http://example.com http://example.org",
+            user=self.user,
+            client_type=Application.CLIENT_CONFIDENTIAL,
+            authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE,
+            client_secret=CLEARTEXT_SECRET,
+            hash_client_secret=False,
+        )
+
+        self.assertEqual(app.client_secret, CLEARTEXT_SECRET)
+
     def test_grant_authorization_code_redirect_uris(self):
         app = Application(
             name="test_app",
diff --git a/tests/test_oauth2_validators.py b/tests/test_oauth2_validators.py
@@ -4,6 +4,7 @@
 
 import pytest
 from django.contrib.auth import get_user_model
+from django.contrib.auth.hashers import make_password
 from django.test import TestCase, TransactionTestCase
 from django.utils import timezone
 from jwcrypto import jwt
@@ -111,7 +112,16 @@ def test_extract_basic_auth(self):
         self.request.headers = {"HTTP_AUTHORIZATION": "Basic 123456 789"}
         self.assertEqual(self.validator._extract_basic_auth(self.request), "123456 789")
 
-    def test_authenticate_basic_auth(self):
+    def test_authenticate_basic_auth_hashed_secret(self):
+        self.request.encoding = "utf-8"
+        self.request.headers = get_basic_auth_header("client_id", CLEARTEXT_SECRET)
+        self.assertTrue(self.validator._authenticate_basic_auth(self.request))
+
+    def test_authenticate_basic_auth_unhashed_secret(self):
+        self.application.client_secret = CLEARTEXT_SECRET
+        self.application.hash_client_secret = False
+        self.application.save()
+
         self.request.encoding = "utf-8"
         self.request.headers = get_basic_auth_header("client_id", CLEARTEXT_SECRET)
         self.assertTrue(self.validator._authenticate_basic_auth(self.request))
@@ -148,6 +158,13 @@ def test_authenticate_basic_auth_not_utf8(self):
         self.request.headers = {"HTTP_AUTHORIZATION": "Basic test"}
         self.assertFalse(self.validator._authenticate_basic_auth(self.request))
 
+    def test_authenticate_check_secret(self):
+        hashed = make_password(CLEARTEXT_SECRET)
+        self.assertTrue(self.validator._check_secret(CLEARTEXT_SECRET, CLEARTEXT_SECRET))
+        self.assertTrue(self.validator._check_secret(CLEARTEXT_SECRET, hashed))
+        self.assertFalse(self.validator._check_secret(hashed, hashed))
+        self.assertFalse(self.validator._check_secret(hashed, CLEARTEXT_SECRET))
+
     def test_authenticate_client_id(self):
         self.assertTrue(self.validator.authenticate_client_id("client_id", self.request))
 
