Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Authorization Code flow without a client_secret #1092

Closed
R70YNS opened this issue Jan 19, 2022 · 26 comments · Fixed by #1276
Closed

Allow Authorization Code flow without a client_secret #1092

R70YNS opened this issue Jan 19, 2022 · 26 comments · Fixed by #1276

Comments

@R70YNS
Copy link

R70YNS commented Jan 19, 2022

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

@n2ygk
Copy link
Member

n2ygk commented Jan 19, 2022

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.

@R70YNS
Copy link
Author

R70YNS commented Jan 19, 2022

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.

@n2ygk
Copy link
Member

n2ygk commented Jan 19, 2022

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

image

@R70YNS
Copy link
Author

R70YNS commented Jan 19, 2022

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

@n2ygk
Copy link
Member

n2ygk commented Jan 19, 2022

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'],
        },
    }
}

@R70YNS
Copy link
Author

R70YNS commented Jan 19, 2022

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
Copy link
Author

R70YNS commented Jan 21, 2022

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.

@n2ygk
Copy link
Member

n2ygk commented Jan 21, 2022

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.

@R70YNS
Copy link
Author

R70YNS commented Jan 21, 2022

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
postmanlabs/postman-app-support#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.

@n2ygk
Copy link
Member

n2ygk commented Jan 21, 2022

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 n2ygk changed the title 401 Unauthorized on PKCE requests 401 Unauthorized on PKCE requests without a client_secret Jan 21, 2022
@n2ygk n2ygk added the bug label Jan 21, 2022
@R70YNS
Copy link
Author

R70YNS commented Jan 21, 2022

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.

@n2ygk
Copy link
Member

n2ygk commented Jan 21, 2022

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.

@R70YNS
Copy link
Author

R70YNS commented Jan 25, 2022

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

@n2ygk
Copy link
Member

n2ygk commented Jan 27, 2022

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

@R70YNS
Copy link
Author

R70YNS commented Jan 31, 2022

@n2ygk I can confirm it returns:

{ "error": "invalid_client" }

@n2ygk
Copy link
Member

n2ygk commented Jan 31, 2022

A PR implementing this would be gladly accepted!

@n2ygk n2ygk changed the title 401 Unauthorized on PKCE requests without a client_secret Allow Authorization Code flow without a client_secret Mar 21, 2022
@bull500
Copy link
Contributor

bull500 commented May 7, 2023

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?

@n2ygk
Copy link
Member

n2ygk commented May 7, 2023

Sure. No IRC. Just issue comments.

@bull500
Copy link
Contributor

bull500 commented May 9, 2023

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

@n2ygk
Copy link
Member

n2ygk commented May 10, 2023

@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.

@bull500
Copy link
Contributor

bull500 commented May 24, 2023

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

@n2ygk
Copy link
Member

n2ygk commented May 25, 2023

Add some test cases and should be good to go.

@bull500
Copy link
Contributor

bull500 commented May 30, 2023

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", "test@example.com", "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))

@n2ygk
Copy link
Member

n2ygk commented May 30, 2023

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, "")

@s1monj
Copy link

s1monj commented May 22, 2024

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

@n2ygk
Copy link
Member

n2ygk commented May 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants