django-oauth-toolkit icon indicating copy to clipboard operation
django-oauth-toolkit copied to clipboard

Allow Authorization Code flow without a `client_secret`

Open R70YNS opened this issue 3 years ago • 16 comments

After enabling PKCE with PKCE_REQUIRED': True I'm unable to retrieve an oauth response when submitting a request to /o/token/ . It provides a authentication code fine.

This article has been used as reference: https://www.liip.ch/en/blog/authorization-code-with-pkce-on-django-using-django-oauth-toolkit

Here is the request (with client_id and code redacted)

Screenshot 2022-01-19 at 17 04 13

R70YNS avatar Jan 19 '22 17:01 R70YNS

Have you tried the same with PKCE_REQUIRED set to False? What's in your console log?

I haven't looked at the blog post. Generally I test with Postman. Make sure your application has https://www.getpostman.com/oauth2/callback as one of the redirect URIs. Postman has Authorization Code with PKCE vs Authorization Code.

n2ygk avatar Jan 19 '22 17:01 n2ygk

I've just tried with PKCE_REQUIRED set to False and it worked fine.

client_secret replaced code_verifier within the request (see below)

Screenshot 2022-01-19 at 18 17 21

It also triggered my debug breakpoint on if challenge is not None: within oauthlib/oauth2/rfc6749/grant_types/authorization_code.py, which doesn't get triggered when PKCE_REQUIRED is set to True.

I'll try updating postman, although I would still expect it to work with parameters manually set.

R70YNS avatar Jan 19 '22 18:01 R70YNS

Instead of a POST to the endpoint, try using Postman's native OAuth2 auth:

image

n2ygk avatar Jan 19 '22 18:01 n2ygk

I've upgraded Postman and tried a request to a protected endpoint (previously working without PKCE) using Authorisation Code (With PKCE), resulted in the same issue though.

Screenshot 2022-01-19 at 19 25 33

R70YNS avatar Jan 19 '22 19:01 R70YNS

Anything in the console? You can set debug-level logging in settings.py something like this:

LOGGING = {
    'version': 1,
    'disable_existing_loggers': False,
    'filters': {
        'require_debug_true': {
            '()': 'django.utils.log.RequireDebugTrue',
        }
    },
    'formatters': {
        'verbose': {
        'format': '%(asctime)s %(levelname)s %(name)s.%(funcName)s:%(lineno)d: %(message)s'
        },
        'simple': {
            'format': '%(levelname)s %(message)s'
        },
    },
    'handlers': {
        'console': {
            'level': 'DEBUG',
            'filters': ['require_debug_true'],
            'class': 'logging.StreamHandler',
            'formatter': 'verbose'
        }
    },
    'loggers': {
        'django.db.backends': {
            'level': 'INFO',
            'handlers': ['console'],
        },
        'oauth2_provider': {
            'level': 'DEBUG',
            'handlers': ['console'],
        },
        'oauthlib': {
            'level': 'DEBUG',
            'handlers': ['console'],
        },
        'myapp': {
            'level': 'INFO',
            'handlers': ['console'],
        },
        'oauth': {
            'level': 'DEBUG',
            'handlers': ['console'],
        },
    }
}

n2ygk avatar Jan 19 '22 19:01 n2ygk

Thanks, I set the debugging config and (through the log) noticed my client_secret was incorrect, it now works correctly through postman using Authentication tab.

I've tried replicating the postman process manually but still get invalid_client error (using my old postman installation).

Unsure what the difference between manual and postman process is but happy to close this issue.

R70YNS avatar Jan 19 '22 22:01 R70YNS

Not being able to rest without figuring out why this wasn't working. I delved into the request logs in Postman and compared them to my own. The difference was that client_secret was left out of my token request, but was used within the request via Postman. Sure enough, if I disable the parameter within my own request it fails again.

I believe this is incorrect behaviour as Authorisation flow with PKCE shouldn't require the client_secret, this has effectively been replaced with the code_verifier.

R70YNS avatar Jan 21 '22 18:01 R70YNS

I do not believe that to he the case. Do you have an RFC document reference that says that? PKCE supplements, not replaces use of client_secret. However one can have a null client_secret now. Not sure if DOT implements that correctly, but that would be a different problem. This was the top hit on a google search and is on the Internet so it must be true ;-)

https://www.scottbrady91.com/oauth/client-authentication-vs-pkce#:~:text=PKCE%20is%20not%20a%20replacement,token%20on%20a%20login%20page.

n2ygk avatar Jan 21 '22 19:01 n2ygk

Section [4.3] of RFC7636 (https://datatracker.ietf.org/doc/html/rfc7636#page-9) references Section 4.1.1 of RFC6749 which doesn't include the client_secret in the request.

Interestingly I found a similar issue reported within Postman https://github.com/postmanlabs/postman-app-support/issues/9409

There does seem to be a bit of confusion about this online, I can't imagine sending the client secret would have any negative affects other than being unnecessary.

R70YNS avatar Jan 21 '22 22:01 R70YNS

Hey @R70YNS, Thanks for the research. I'm going to rename this issue to something about client_secret not being sent. I know that in at least one commercial OAuth2 server there is an explicit configuration option of either having no client_secret or having a value for it. We use this with Authorization Code flow with PKCE for our SPA apps that formerly used Implicit flow.

I suspect that this may not be implemented in DOT

n2ygk avatar Jan 21 '22 22:01 n2ygk

No problem @n2ygk. I think the name change is better too, it more accurately describes the problem. I do think that those who can secure the client_secret may as well continue to send it as an added layer of protection. This issue only really affects those who can't secure the client secret.

For a workaround I will use a null value for the client_secret as you mentioned above.

R70YNS avatar Jan 21 '22 22:01 R70YNS

For a workaround I will use a null value for the client_secret as you mentioned above.

I'm curious to know if that will actually work. Looking forward to your report.

n2ygk avatar Jan 21 '22 22:01 n2ygk

I can confirm that attempting to retrieve a new oAuth token whilst sending a blank string (empty field in Postman) still returns a 401.

Screenshot 2022-01-25 at 21 15 52

R70YNS avatar Jan 25 '22 23:01 R70YNS

Are you able to test with leaving the client_secret out of the request body?

n2ygk avatar Jan 27 '22 16:01 n2ygk

@n2ygk I can confirm it returns:

{ "error": "invalid_client" }

R70YNS avatar Jan 31 '22 21:01 R70YNS

A PR implementing this would be gladly accepted!

n2ygk avatar Jan 31 '22 21:01 n2ygk

Hello @n2ygk Can I take this issue up please? Also Is there any place like IRC or other forms of contact where i can reach out?

bull500 avatar May 07 '23 19:05 bull500

Sure. No IRC. Just issue comments.

n2ygk avatar May 07 '23 22:05 n2ygk

Hey @n2ygk

I did some looking around i think I've narrowed down the problem The error seems to arise from '_def authenticate_request_body' of oauth2_validators.py file:

Original Code below:

    def _authenticate_request_body(self, request):
        """
        Try to authenticate the client using client_id and client_secret
        parameters included in body.

        Remember that this method is NOT RECOMMENDED and SHOULD be limited to
        clients unable to directly utilize the HTTP Basic authentication scheme.
        See rfc:'2.3.1' for more details.
        """
        # TODO: check if oauthlib has already unquoted client_id and client_secret
        try:
            client_id = request.client_id
            client_secret = request.client_secret
        except AttributeError:
            return False

        if self._load_application(client_id, request) is None:
            log.debug("Failed body auth: Application %s does not exists" % client_id)
            return False
        elif not check_password(client_secret, request.client.client_secret):
            log.debug("Failed body auth: wrong client secret %s" % client_secret)
            return False
        else:
            return True

By commenting out the elif part and the 'client_secret' assignment, Ive been able to get a token without problems.

    def _authenticate_request_body(self, request):
        """
        Try to authenticate the client using client_id and client_secret
        parameters included in body.

        Remember that this method is NOT RECOMMENDED and SHOULD be limited to
        clients unable to directly utilize the HTTP Basic authentication scheme.
        See rfc:'2.3.1' for more details.
        """
        # TODO: check if oauthlib has already unquoted client_id and client_secret
        try:
            client_id = request.client_id
            # client_secret = request.client_secret
        except AttributeError:
            return False

        if self._load_application(client_id, request) is None:
            log.debug("Failed body auth: Application %s does not exists" % client_id)
            return False
        # elif not check_password(client_secret, request.client.client_secret):
        #     log.debug("Failed body auth: wrong client secret %s" % client_secret)
        #    return False
        else:
            return True

Please let me know if there are other things i should be taking care of while addressing this issue. Also is it better to comment the code or to remove it entirely if the above findings is the only change required

bull500 avatar May 09 '23 20:05 bull500

@bull500 commenting out the client_secret means the auth check will succeed even when there is a required client_secret without comparing it. I believe instead you want to check to see when the client_secret query parameter is missing or has an empty value whether the application.client_secret is the empty string. This should align with https://www.rfc-editor.org/rfc/rfc6749.html#section-2.3.1 where it says:

REQUIRED. The client secret. The client MAY omit the parameter if the client secret is an empty string.

n2ygk avatar May 10 '23 17:05 n2ygk

hey @n2ygk Thank you for the clarity!

I've added a simple if-else check to the client secret assignment. I tested it with 2 apps, one with auto generated client secret and one with a empty client secret, while passing and omitting the client_secret value during POST and it seems to be working fine

Please let me know your thought and if this is good for PR

    def _authenticate_request_body(self, request):
        """
        Try to authenticate the client using client_id and client_secret
        parameters included in body.

        Remember that this method is NOT RECOMMENDED and SHOULD be limited to
        clients unable to directly utilize the HTTP Basic authentication scheme.
        See rfc:'2.3.1' for more details.
        """
        # TODO: check if oauthlib has already unquoted client_id and client_secret
        try:
            client_id = request.client_id
            client_secret = request.client_secret if request.client_secret else ''
        except AttributeError:
            return False

        if self._load_application(client_id, request) is None:
            log.debug("Failed body auth: Application %s does not exists" % client_id)
            return False
        elif not check_password(client_secret, request.client.client_secret):
            log.debug("Failed body auth: wrong client secret %s" % client_secret)
            return False
        else:
            return True

bull500 avatar May 24 '23 06:05 bull500

Add some test cases and should be good to go.

n2ygk avatar May 25 '23 16:05 n2ygk

Hello @n2ygk

I've made a few changes and included test cases:

In oauth2_validators.py used hasattr() to check object property exists

    def _authenticate_request_body(self, request):
        """
        Try to authenticate the client using client_id and client_secret
        parameters included in body.

        Remember that this method is NOT RECOMMENDED and SHOULD be limited to
        clients unable to directly utilize the HTTP Basic authentication scheme.
        See rfc:`2.3.1` for more details.
        """
        # TODO: check if oauthlib has already unquoted client_id and client_secret
        try:
            client_id = request.client_id
            client_secret = request.client_secret if hasattr(request, "client_secret") else ""
        except AttributeError:
            return False

        if self._load_application(client_id, request) is None:
            log.debug("Failed body auth: Application %s does not exists" % client_id)
            return False
        elif not check_password(client_secret, request.client.client_secret):
            log.debug("Failed body auth: wrong client secret %s" % client_secret)
            return False
        else:
            return True

With respect to testcases, i edited the test_oauth2_validators.py file. I've added a separate def setUp(self) portion to create an app with blank client secret Also made changes and addition to the def test_authenticate_request_body(self) method I ran tox and the outputs seem fine Im not sure if my setup is correct on testing. Still confused even after reading the contributing guidelines

Check is present if client_secret parameter is omitted when app is created with/without client secret. Also other generic tests

Code below:

CLEARTEXT_BLANK_SECRET = ""

class TestOAuth2Validator(TransactionTestCase):
    def setUp(self):
        self.user = UserModel.objects.create_user("user", "[email protected]", "123456")
        self.request = mock.MagicMock(wraps=Request)
        self.request.user = self.user
        self.request.grant_type = "not client"
        self.validator = OAuth2Validator()
        self.application = Application.objects.create(
            client_id="client_id",
            client_secret=CLEARTEXT_SECRET,
            user=self.user,
            client_type=Application.CLIENT_PUBLIC,
            authorization_grant_type=Application.GRANT_PASSWORD,
        )
        self.request.client = self.application

        self.blank_secret_request = mock.MagicMock(wraps=Request)
        self.blank_secret_request.user = self.user
        self.blank_secret_request.grant_type = "not client"
        self.blank_secret_application = Application.objects.create(
            client_id="blank_secret_client_id",
            client_secret=CLEARTEXT_BLANK_SECRET,
            user=self.user,
            client_type=Application.CLIENT_PUBLIC,
            authorization_grant_type=Application.GRANT_PASSWORD,
        )
        self.blank_secret_request.client = self.blank_secret_application

    def tearDown(self):
        self.application.delete()

    def test_authenticate_request_body(self):
        self.request.client_id = "client_id"
        self.assertFalse(self.validator._authenticate_request_body(self.request))
        self.request.client_secret = ""
        self.assertFalse(self.validator._authenticate_request_body(self.request))
        self.request.client_secret = "wrong_client_secret"
        self.assertFalse(self.validator._authenticate_request_body(self.request))
        self.request.client_secret = CLEARTEXT_SECRET
        self.assertTrue(self.validator._authenticate_request_body(self.request))

        self.blank_secret_request.client_id = "blank_secret_client_id"
        self.assertTrue(self.validator._authenticate_request_body(self.blank_secret_request))
        self.blank_secret_request.client_secret = CLEARTEXT_BLANK_SECRET
        self.assertTrue(self.validator._authenticate_request_body(self.blank_secret_request))
        self.blank_secret_request.client_secret = "wrong_client_secret"
        self.assertFalse(self.validator._authenticate_request_body(self.blank_secret_request))

bull500 avatar May 30 '23 11:05 bull500

Please put these changes in a PR so I can review and comment inline. FWIW, you can use getattr():

1c1
< client_secret = request.client_secret if hasattr(request, "client_secret") else ""
---
> client_secret = getattr(request.client_secret, "")

n2ygk avatar May 30 '23 13:05 n2ygk

I'm trying to achieve an Authorization Code with PKCE flow and public client for a Single Page web App (that can't keep a client_secret safe). I am using the latest release 2.4.0 but having the same issue described above.

  • I register a new app with Client type = Public Grant type = Authorization code and Algorithm = RSA 256 (because I'm using OIDC) - screenshot below
  • When I include the Client Secret in Postman, I get both the code and token correctly ✅
  • When I omit the Client Secret, I get the code returned OK but the "POST /o/token/ returns 401 and gives me "Error: invalid_client" ❌
  • I've tried with both PKCE_REQUIRED true and false

@n2ygk do you (or anyone else) have any ideas as to what I might be doing wrong?

Screenshot 2024-05-22 at 3 36 56 PM

s1monj avatar May 22 '24 19:05 s1monj

@s1monj I made this into a new issue #1426 since this PR is closed.

n2ygk avatar May 23 '24 13:05 n2ygk