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

Invalid JWT signature because of hashed client secret #1239

Closed
1 of 2 tasks
dnsv opened this issue Jan 2, 2023 · 6 comments · Fixed by #1311
Closed
1 of 2 tasks

Invalid JWT signature because of hashed client secret #1239

dnsv opened this issue Jan 2, 2023 · 6 comments · Fixed by #1311

Comments

@dnsv
Copy link

dnsv commented Jan 2, 2023

Describe the bug

I'm using next-auth on the frontend and the latest django-oauth-toolkit with OIDC enabled and HS256 as the algorithm on the backend. After authorizing my oauth application I get the error "failed to validate JWT signature" in next-auth.

The JWT token provided by django-oauth-toolkit seems to be incorrect. It's prepared in oauth2_provider.oauth2_validators.finalize_id_token. The culprit is the jwk_key that's used for signing the token. It's generated with the client secret, which is being hashed since v2.0.0.

jwt_token.make_signed_token(request.client.jwk_key)

I'm assuming that hashing was added under the assumption that the raw secret isn't needed anymore after it's created, but that doesn't seem to be the case (see line 224).

@property
def jwk_key(self):
if self.algorithm == AbstractApplication.RS256_ALGORITHM:
if not oauth2_settings.OIDC_RSA_PRIVATE_KEY:
raise ImproperlyConfigured("You must set OIDC_RSA_PRIVATE_KEY to use RSA algorithm")
return jwk.JWK.from_pem(oauth2_settings.OIDC_RSA_PRIVATE_KEY.encode("utf8"))
elif self.algorithm == AbstractApplication.HS256_ALGORITHM:
return jwk.JWK(kty="oct", k=base64url_encode(self.client_secret))
raise ImproperlyConfigured("This application does not support signed tokens")

To Reproduce

I've created a test repo: https://github.com/dnsv/auth-test

  1. Setup Django (I'm using Python 3.11):

    1. poetry install
    2. poetry run python manage.py runserver
    3. poetry run python manage.py migrate
    4. poetry run python manage.py createsuperuser.
  2. Login into admin.

  3. Create a new Oauth application:

  4. Create the file frontend/.env:

    OAUTH_CLIENT_ID=...
    OAUTH_CLIENT_SECRET=...
    
  5. Setup Next.js (I'm using node v18.12.1, but i probably works the same with lower versions).

    1. cd frontend
    2. yarn install
    3. yarn dev
  6. Go to http://localhost:3000/ and try to sign it. You'll get the error shown in the console after the authorization step.

Highlights from the backend setup:

# backend/base/settings.py

OAUTH2_PROVIDER = {
    "OIDC_ENABLED": True,
    "OAUTH2_VALIDATOR_CLASS": "backend.oauth2.utils.CustomOAuth2Validator",
    "SCOPES": {
        "openid": "OpenID Connect scope",
    },
}

# backend/oauth2/utils.py

from oauth2_provider.oauth2_validators import OAuth2Validator

class CustomOAuth2Validator(OAuth2Validator):
    oidc_claim_scope = None

    def get_additional_claims(self, request):
        return {
            "id": request.user.id,
            "given_name": request.user.first_name,
            "family_name": request.user.last_name,
            "name": f"{request.user.first_name} {request.user.last_name}",
            "email": request.user.email,
        }

Expected behavior

Being able to login :)

Version

  • I have tested with the latest published release and it's still a problem.
  • I have tested with the master branch and it's still a problem.

Additional context

N/A

@dnsv dnsv added the bug label Jan 2, 2023
@n2ygk
Copy link
Member

n2ygk commented Jan 4, 2023

Ouch! We'll have to back off the hashing I guess. That will be a complicated migration. Maybe with a setting to enable/disable hashing and some code to deal with a hashed or unhashed secret.

@gardenerik
Copy link
Contributor

If I understand the spec correctly, the ID token must be signed by the secret key (if using MAC algorithm such as HS256).

As stated in 3.1.3.7. ID Token Validation:

  1. ... the octets of the UTF-8 representation of the client_secret ... are used as the key to validate the signature.

So that would make DOT 2.0+ not spec complaint.


I would like to open a PR to resolve this issue, but I am not certain how should that be handled. We obviously cannot revert the changes, as hashing is a one-way operation.

Secrets for new applications should be stored in plain text. We will also need to continue to support the use of hashed secrets in requests. We should also allow a simple way to regenerate existing secrets if wanted by the user.

Is there anything I am missing?

@n2ygk
Copy link
Member

n2ygk commented Sep 3, 2023

Hashing was added erroneously by not considering the requirement to sign jwt's. Perhaps undo hashing of new secrets and retain the ability to check old hashed ones. I think this should just work for hashed or clear secrets because the password check looks for the has prefix.

Perhaps make it an option when creating an application to disable hashing if the app will never use jwts?

@gardenerik
Copy link
Contributor

Do you mean like if an app is only using RSA based JWTs, so they can just leave the hashing on?

@n2ygk
Copy link
Member

n2ygk commented Sep 3, 2023

Many apps do not use jwts at all. They were obtain and provide an oauth2 Bearer access token and the backend contacts the AS to validate it.

@gardenerik
Copy link
Contributor

I see.. Ok, so I will open a PR in the following days in which I will add support to use both hashed and unhashed secrets with the option to enable/disable hashing them on save.

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

Successfully merging a pull request may close this issue.

3 participants