iMerica / dj-rest-auth

Authentication for Django Rest Framework
https://dj-rest-auth.readthedocs.io/en/latest/index.html
MIT License
1.64k stars 306 forks source link

PKCE code verifier not specified #476

Open VISHNU-P-M opened 1 year ago

VISHNU-P-M commented 1 year ago

In Social Authentication with Keycloak, I'm facing an issue retrieving tokens with the Keycloak SocialLoginView extended view.

here my view

class KeycloakLogin(SocialLoginView): adapter_class = KeycloakOAuth2Adapter client_class = OAuth2Client callback_url = f"{settings.APP_UI_URL}/login"

URL

re_path(r'login/keycloak/?$', views.KeycloakLogin.as_view()),

Error

allauth.socialaccount.providers.oauth2.client.OAuth2Error: Error retrieving access token: b'{"error":"invalid_grant","error_description":"PKCE code verifier not specified"}'

aswinm commented 1 year ago

Had the same issue with Microsoft Graph. The PKCE code verifier-challenge protocol is not supported by dj-rest-auth yet. Allauth sets a pkce_code_verifier in the session before redirecting the user to auth, and uses the verifier in the request to get the access token.

For now I am overriding the OAuth2Client to induce the verifier in the request, but would be great if there is an inbuilt way for rest auth to handle it.

READ10 commented 1 year ago

I submitted #470 to add support for submitting the code_verifier but @iMerica closed it, not sure if that was in error, but I'd be happy to work on this if it's something the project will take.

vecchp commented 11 months ago

Commented on the PR as well, even with the readme link provided I do not understand the reasoning. PKCE is part of the OAuth standard and not a specific allauth feature.

https://github.com/iMerica/dj-rest-auth/pull/470#issuecomment-1726144252

Updated Note 9/21/23

Warning: Lack of PKCE Support in dj-rest-auth

Users considering dj-rest-auth for their projects should be aware that the library currently does not support PKCE (Proof Key for Code Exchange) which is a crucial security enhancement in the OAuth 2.0 Authorization Code Flow, especially important for public clients like Single Page Applications and mobile apps.

Risks:

Exploitation Scenario:

  1. Interception of the Authorization Code:
    • An attacker could intercept the authorization code as it is transmitted to the client if, for example, the redirection URI is exposed or compromised, especially in environments like mobile apps where it is harder to secure redirection URIs.
  2. Unauthorized Code Exchange:
    • Since PKCE is not implemented, there is no additional secret (code_verifier) to protect the exchange of the intercepted authorization code for an access token. The attacker can use the intercepted code to obtain an access token.
  3. Gaining Unauthorized Access
    • With the access token, the attacker could potentially access the protected resources and perform actions on behalf of the user, leading to unauthorized data access or manipulation.

Why PKCE Matters

PKCE is specifically designed to mitigate this kind of vulnerability. It ensures that even if the authorization code is intercepted, it cannot be exchanged for an access token without the unique code_verifier which is not transmitted until the token exchange step and is never transmitted in a URL or through redirects, making it much more secure.

Recommendations:

Further Reading

https://curity.io/resources/learn/oauth-pkce/

vecchp commented 11 months ago

Here's a forked repo which adds PKCE support that @READ10 contributed - https://github.com/Solidangle-net/dj-rest-auth

gabn88 commented 5 months ago

@iMerica Any reason not to merge #470 ? It is a security requirement for all apps that use the OAuth2.0 flow for SPA's (so probably for most DRF apps).

gabn88 commented 5 months ago

On Microsoft graph the above is not working. You get the error: Client is public so neither \'client_assertion\' nor \'client_secret\' should be presented.

Code to blame probably is around: https://github.com/iMerica/dj-rest-auth/blob/069cd112acd2b31e5c0b37e607c1f927d2235a84/dj_rest_auth/registration/serializers.py#L126

READ10 commented 5 months ago

I only tested against Google and IIRC I was surprised that they required some of the bits they did. Anyway, I've moved away from dj-rest-auth because I didn't want to maintain a fork, but somebody ought to be able to get it to work more broadly without a ton of effort.

vecchp commented 5 months ago

I only tested against Google and IIRC I was surprised that they required some of the bits they did. Anyway, I've moved away from dj-rest-auth because I didn't want to maintain a fork, but somebody ought to be able to get it to work more broadly without a ton of effort.

I only tested this against id.me and google as well, so unsure of the above error. What did you end up moving to @READ10 ?

For a personal project I moved to https://github.com/st4lk/django-rest-social-auth. With this implementation https://github.com/st4lk/django-rest-social-auth/issues/136

I'm waiting for https://allauth.org/news/2024/03/ngi-zero-grant-plan/ to complete so I can move a production app to the official allauth flow when implemented.

READ10 commented 5 months ago

Th project I was working on had strong requirements to use DRF and relatively stable underlying libraries so I ended up using straight django-allauth which might not be the right answer for most of the folks here.