Don't treat Application.client_secret as encrypted

In newer DOT than what AWX uses, Application.client_secret is hashed
automatically with no way to disable that functionality.

There's a PR that allows for disabling that functionality ([0]), but
that hasn't made it into a release.

The DOT hashing is incompatible with our standard encryption - when
DOT gets the value it ends up getting our encrypted string and trying
to act on that. Ideally we'd like to disable their hashing entirely
and use our standard encryption tooling.

AWX avoids this problem by pinning to an older DOT.

For now in DAB we'll just use the upstream hashing, and not treat the
field as an encrypted_fields field to avoid the "double encryption"
issue.

[0]: jazzband/django-oauth-toolkit#1311

Signed-off-by: Rick Elrod <[email protected]>

ansible_base/oauth2_provider/migrations/0004_alter_oauth2application_client_secret.py

@@ -0,0 +1,20 @@
+# Generated by Django 4.2.11 on 2024-04-28 16:14
+
+from django.db import migrations
+import oauth2_provider.generators
+import oauth2_provider.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('dab_oauth2_provider', '0003_alter_oauth2accesstoken_application'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='oauth2application',
+            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),
+        ),
+    ]

ansible_base/oauth2_provider/models/application.py

@@ -6,7 +6,6 @@
 from django.db import models
 from django.urls import reverse
 from django.utils.translation import gettext_lazy as _
-from oauth2_provider.generators import generate_client_secret
 
 from ansible_base.lib.abstract_models.common import NamedCommonModel
 
@@ -16,7 +15,8 @@
 class OAuth2Application(oauth2_models.AbstractApplication, NamedCommonModel):
     router_basename = 'application'
     ignore_relations = ['oauth2idtoken', 'grant', 'oauth2refreshtoken']
-    encrypted_fields = ['client_secret']
+    # We do NOT add client_secret to encrypted_fields because it is hashed by Django OAuth Toolkit
+    # and it would end up hashing the encrypted value.
 
     class Meta(oauth2_models.AbstractAccessToken.Meta):
         verbose_name = _('application')
@@ -50,13 +50,14 @@ class Meta(oauth2_models.AbstractAccessToken.Meta):
         on_delete=models.CASCADE,
         null=True,
     )
-    client_secret = models.CharField(
-        max_length=1024,
-        blank=True,
-        default=generate_client_secret,
-        db_index=True,
-        help_text=_('Used for more stringent verification of access to an application when creating a token.'),
-    )
+    # Not overriding client_secret... Details:
+    # It would be nice to just use our usual encrypted_fields flow here
+    # until DOT makes a release with https://github.com/jazzband/django-oauth-toolkit/pull/1311
+    # there is no way to disable its expectation of using its own hashing
+    # (which is Django's make_password/check_password).
+    # So we use their field here.
+    # Previous versions of DOT didn't hash the field at all and AWX pins
+    # to <2.0.0 so AWX used the AWX encryption with no issue.
     client_type = models.CharField(
         max_length=32, choices=CLIENT_TYPES, help_text=_('Set to Public or Confidential depending on how secure the client device is.')
     )

ansible_base/oauth2_provider/serializers/application.py

@@ -1,8 +1,9 @@
 from django.core.exceptions import ObjectDoesNotExist
 from django.utils.translation import gettext_lazy as _
+from oauth2_provider.generators import generate_client_secret
 
 from ansible_base.lib.serializers.common import NamedCommonModelSerializer
-from ansible_base.lib.utils.encryption import ENCRYPTED_STRING, ansible_encryption
+from ansible_base.lib.utils.encryption import ENCRYPTED_STRING
 from ansible_base.oauth2_provider.models import OAuth2Application
 
 
@@ -12,6 +13,8 @@ def has_model_field_prefetched(obj, thing):
 
 
 class OAuth2ApplicationSerializer(NamedCommonModelSerializer):
+    oauth2_client_secret = None
+
     class Meta:
         model = OAuth2Application
         fields = NamedCommonModelSerializer.Meta.fields + [x.name for x in OAuth2Application._meta.concrete_fields]
@@ -33,7 +36,11 @@ def _get_client_secret(self, obj):
             if obj.client_type == 'public':
                 return None
             elif request.method == 'POST':
-                return ansible_encryption.decrypt_string(obj.client_secret)
+                if self.oauth2_client_secret is None:
+                    # This should be an impossible case, but...
+                    return ENCRYPTED_STRING
+                # Show the secret, one time, on POST
+                return self.oauth2_client_secret
             else:
                 return ENCRYPTED_STRING
         except ObjectDoesNotExist:
@@ -49,7 +56,7 @@ def to_representation(self, instance):
         if secret is None:
             del ret['client_secret']
         else:
-            ret['client_secret'] = self._get_client_secret(instance)
+            ret['client_secret'] = secret
         return ret
 
     def _summary_field_tokens(self, obj):
@@ -67,3 +74,27 @@ def get_summary_fields(self, obj):
         ret = super(OAuth2ApplicationSerializer, self).get_summary_fields(obj)
         ret['tokens'] = self._summary_field_tokens(obj)
         return ret
+
+    def create(self, validated_data):
+        # This is hacky:
+        # There is a cascading set of issues here.
+        # 1. The first thing to know is that DOT automatically hashes the client_secret
+        #    in a pre_save method on the client_secret field.
+        # 2. In current released versions, there is no way to disable (1). It uses
+        #    the built-in Django password hashing stuff to do this. There's a merged
+        #    PR to allow disabling this (DOT #1311), but it's not released yet.
+        # 3. If we use our own encrypted_field stuff, it conflicts with (1) and (2).
+        #    They end up giving our encrypted field to Django's password check
+        #    and *we* end up showing *their* hashed value to the user on POST, which
+        #    doesn't work, the user needs to see the real (decrypted) value. So
+        #    until upstream #1311 is released, we do NOT treat the field as an
+        #    encrypted_field, we just defer to the upstream hashing.
+        # 4. But we have no way to see the client_secret on POST, if we let the
+        #    model generate it, because it's hashed by the time we get to the
+        #    serializer...
+        #
+        # So to that end, on POST, we'll make the client secret here, and then
+        # we can access it to show the user the value (once) on POST.
+        validated_data['client_secret'] = generate_client_secret()
+        self.oauth2_client_secret = validated_data['client_secret']
+        return super().create(validated_data)

test_app/settings.py

@@ -35,6 +35,11 @@
             'handlers': ['console'],
             'level': 'DEBUG',
         },
+        '': {
+            'handlers': ['console'],
+            'level': 'DEBUG',
+            'propagate': True,
+        },
     },
 }
 for logger in LOGGING["loggers"]:  # noqa: F405

test_app/tests/oauth2_provider/conftest.py

@@ -8,13 +8,21 @@
 
 @pytest.fixture
 def oauth2_application(randname):
-    return OAuth2Application.objects.create(
+    """
+    Creates an OAuth2 application with a random name and returns
+    both the application and its client secret.
+    """
+    app = OAuth2Application(
         name=randname("OAuth2 Application"),
         description="Test OAuth2 Application",
         redirect_uris="http://example.com/callback",
         authorization_grant_type="authorization-code",
         client_type="confidential",
     )
+    # Store this before it gets hashed
+    secret = app.client_secret
+    app.save()
+    return (app, secret)
 
 
 @pytest.fixture

test_app/tests/oauth2_provider/views/test_application.py

@@ -1,8 +1,8 @@
 import pytest
-from django.db import connection
+from django.contrib.auth.hashers import check_password
 from django.urls import reverse
 
-from ansible_base.lib.utils.encryption import ENCRYPTED_STRING, ansible_encryption
+from ansible_base.lib.utils.encryption import ENCRYPTED_STRING
 from ansible_base.oauth2_provider.models import OAuth2Application
 
 
@@ -25,7 +25,7 @@ def test_oauth2_provider_application_list(request, client_fixture, expected_stat
     assert response.status_code == expected_status
     if expected_status == 200:
         assert len(response.data['results']) == OAuth2Application.objects.count()
-        assert response.data['results'][0]['name'] == oauth2_application.name
+        assert response.data['results'][0]['name'] == oauth2_application[0].name
 
 
 @pytest.mark.parametrize(
@@ -42,6 +42,7 @@ def test_oauth2_provider_application_related(admin_api_client, oauth2_applicatio
     Organization should only be shown if the application is associated with an organization.
     Associating an application with an organization should not affect other related fields.
     """
+    oauth2_application = oauth2_application[0]
     if view == "application-list":
         url = reverse(view)
     else:
@@ -75,6 +76,7 @@ def test_oauth2_provider_application_detail(request, client_fixture, expected_st
     """
     Test that we can view the detail of an OAuth2 application iff we are authenticated.
     """
+    oauth2_application = oauth2_application[0]
     client = request.getfixturevalue(client_fixture)
     url = reverse("application-detail", args=[oauth2_application.pk])
     response = client.get(url)
@@ -152,6 +154,7 @@ def test_oauth2_provider_application_update(request, client_fixture, expected_st
     """
     Test that we can update oauth2 applications iff we are authenticated.
     """
+    oauth2_application = oauth2_application[0]
     client = request.getfixturevalue(client_fixture)
     url = reverse("application-detail", args=[oauth2_application.pk])
     response = client.patch(
@@ -197,11 +200,22 @@ def test_oauth2_provider_application_client_secret_encrypted(admin_api_client, o
     )
     assert response.status_code == 201, response.data
     application = OAuth2Application.objects.get(pk=response.data['id'])
-    with connection.cursor() as cursor:
-        cursor.execute("SELECT client_secret FROM dab_oauth2_provider_oauth2application WHERE id = %s", [application.pk])
-        encrypted = cursor.fetchone()[0]
-    assert encrypted.startswith(ENCRYPTED_STRING), encrypted
-    assert ansible_encryption.decrypt_string(encrypted) == response.data['client_secret'], response.data
+
+    # If we ever switch to using *our* encryption, this is a good test.
+    # But until a release with jazzband/django-oauth-toolkit#1311 hits pypi,
+    # we have no way to disable their built-in hashing (which conflicts with our
+    # own encryption).
+    # with connection.cursor() as cursor:
+    #     cursor.execute("SELECT client_secret FROM dab_oauth2_provider_oauth2application WHERE id = %s", [application.pk])
+    #     encrypted = cursor.fetchone()[0]
+    # assert encrypted.startswith(ENCRYPTED_STRING), encrypted
+    # assert ansible_encryption.decrypt_string(encrypted) == response.data['client_secret'], response.data
+    # assert response.data['client_secret'] == application.client_secret
+
+    # For now we just make sure it shows the real client secret on POST
+    # and never on any other method.
+    assert 'client_secret' in response.data
+    assert check_password(response.data['client_secret'], application.client_secret)
 
     # GET
     response = admin_api_client.get(reverse("application-detail", args=[application.pk]))

test_app/tests/oauth2_provider/views/test_token.py

@@ -1,20 +1,40 @@
+import base64
+
 import pytest
 from django.urls import reverse
+from django.utils.http import urlencode
+
+
+@pytest.mark.django_db
+def test_oauth2_personal_access_token_creation(oauth2_application, user, unauthenticated_api_client):
+    app = oauth2_application[0]
+    app.authorization_grant_type = 'password'
+    app.save()
+
+    secret = oauth2_application[1]
+    url = reverse('token')
+    data = {
+        "grant_type": "password",
+        "username": "user",
+        "password": "password",
+        "scope": "read",
+    }
+    resp = unauthenticated_api_client.post(
+        url,
+        data=urlencode(data),
+        content_type='application/x-www-form-urlencoded',
+        headers={'Authorization': 'Basic ' + base64.b64encode(f"{app.client_id}:{secret}".encode()).decode()},
+    )
+
+    assert resp.status_code == 200, resp.content
+    resp_json = resp.json()
+    assert 'access_token' in resp_json
+    assert len(resp_json['access_token']) > 0
+    assert 'scope' in resp_json
+    assert resp_json['scope'] == 'read'
+    assert 'refresh_token' in resp_json
+
 
-# @pytest.mark.django_db
-# def test_personal_access_token_creation(oauth_application, post, alice):
-#     url = drf_reverse('api:oauth_authorization_root_view') + 'token/'
-#     resp = post(
-#         url,
-#         data='grant_type=password&username=alice&password=alice&scope=read',
-#         content_type='application/x-www-form-urlencoded',
-#         HTTP_AUTHORIZATION='Basic ' + smart_str(base64.b64encode(smart_bytes(':'.join([oauth_application.client_id, oauth_application.client_secret])))),
-#     )
-#     resp_json = smart_str(resp._container[0])
-#     assert 'access_token' in resp_json
-#     assert 'scope' in resp_json
-#     assert 'refresh_token' in resp_json
-#
 #
 # @pytest.mark.django_db
 # @pytest.mark.parametrize('allow_oauth, status', [(True, 201), (False, 403)])