diff --git a/AUTHORS b/AUTHORS index 58ae037ee..d24447a5c 100644 --- 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 index 93176fe4b..d26ae6207 100644 --- 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 index d4ef8bd5a..0231127ae 100644 Binary files a/docs/_images/application-register-auth-code.png and b/docs/_images/application-register-auth-code.png differ diff --git a/docs/_images/application-register-client-credential.png b/docs/_images/application-register-client-credential.png index 7f2d1cb60..8d200d87f 100644 Binary files a/docs/_images/application-register-client-credential.png and b/docs/_images/application-register-client-credential.png differ diff --git a/docs/getting_started.rst b/docs/getting_started.rst index 2a7cb284f..9b79f9a32 100644 --- 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 `), 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 index c06af5c1a..7a758ed65 100644 --- 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 index 1d53de78a..9f79e895f 100644 --- 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 index dcc46e765..ad95a11b7 100644 --- 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 new file mode 100644 index 000000000..9452bce98 --- /dev/null +++ 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 index c1dec99c5..204579905 100644 --- 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 index 6847760e5..d452fd97c 100644 --- 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 index f9d525aff..271eb7649 100644 --- a/oauth2_provider/templates/oauth2_provider/application_detail.html +++ b/oauth2_provider/templates/oauth2_provider/application_detail.html @@ -16,6 +16,11 @@

{{ application.name }}

+
  • +

    {% trans "Hash client secret" %}

    +

    {{ application.hash_client_secret|yesno:_("yes,no") }}

    +
    • +

  • {% trans "Client type" %}

    {{ application.client_type }}

    diff --git a/oauth2_provider/views/application.py b/oauth2_provider/views/application.py index 9289483f6..9b5a8ffb6 100644 --- 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 new file mode 100644 index 000000000..80edd057e --- /dev/null +++ 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 index fe1fef084..5ebb1f0f9 100644 --- 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 index 7d2b0cbac..78d9ac982 100644 --- 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))