search

jazzband / django-oauth-toolkit

OAuth2 goodies for the Djangonauts!
https://django-oauth-toolkit.readthedocs.io
Other
3.13k stars 792 forks source link

Allow the use of unhashed secrets #1311

Closed gardenerik closed 1 year ago

gardenerik commented 1 year ago

Fixes #1239

Description of the Change

Adds an option to disable client secrets hashing on save. Also allows the use of applications with unhashed secrets in the HTTP API. This makes it possible to verify OIDC JWTs with HS256.

Checklist

n2ygk commented 1 year ago

Please request a review when you are ready....

codecov[bot] commented 1 year ago

Codecov Report

Merging #1311 (be42cc7) into master (f8c9f36) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1311      +/-   ##
==========================================
+ Coverage   97.37%   97.39%   +0.01%     
==========================================
  Files          32       32              
  Lines        2022     2035      +13     
==========================================
+ Hits         1969     1982      +13     
  Misses         53       53
Files Changed Coverage Δ
oauth2_provider/views/application.py 100.00% <ø> (ø)
..._provider/management/commands/createapplication.py 100.00% <100.00%> (ø)
oauth2_provider/models.py 98.57% <100.00%> (+0.01%) :arrow_up:
oauth2_provider/oauth2_validators.py 94.07% <100.00%> (+0.09%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

gardenerik commented 1 year ago

@n2ygk it seems I am unable to request a review through the UI, but feel free to review the PR

n2ygk commented 1 year ago

@n2ygk it seems I am unable to request a review through the UI, but feel free to review the PR

Looks like you added them (or someone did)

gardenerik commented 1 year ago

I am not sure what you are talking about :( The reviewers section is empty. image

n2ygk commented 1 year ago

I am not sure what you are talking about :( The reviewers section is empty. image

Haha I was looking at my suggestions:

image

n2ygk commented 1 year ago

@Andrew-Chen-Wang I've added you as a reviewer mostly as an FYI.

gardenerik commented 1 year ago

@n2ygk I have updated your points, thanks for the feedback

n2ygk commented 1 year ago

Looks like pytest broke probably due to some version issue. Will look at it tomorrow or Thursday

n2ygk commented 1 year ago

Hmm a migrations conflict: 

E           django.core.management.base.CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0008_alter_accesstoken_token, 0008_add_hash_client_secret in oauth2_provider).
E           To fix them run 'python manage.py makemigrations --merge'
n2ygk commented 1 year ago

My fault. I merged #1312 which created the migration conflict. Fixing it now.

hugochinchilla commented 10 months ago

I know I can install this from git but I think this fix is important enought to create a new release on PyPI.

Thank you for your work :)

relrod commented 5 months ago

@n2ygk (from one ham radio op to another :smiling_face_with_tear:) any chance of a release with this fix in the near-ish future?

n2ygk commented 4 months ago

FYI - as part of the release, I'm strengthening the language to recommend using RS256 keys which is a more secure approach than HS256 and does not require retaining the client_secret as cleartext.

blag commented 2 months ago

This may lead to incompatibilities with some OIDC Relying Party libraries.

Although I think I know the answer, I have a clarification question. Does the "This" in this sentence mean that:

  1. The case when the box is checked (and the client secret is hashed) may lead to incompatibilities with some OIDC RP libraries or...
  2. The case when the box is unchecked (and the client secret is left unhashed) may lead to incompatibilities with some OIDC RP libraries

?

Because it isn't 100% perfectly clear here to me, and I'm not an expert on OIDC.

Once I get an answer I'll provide a PR to tweak/fix the language.

gardenerik commented 2 months ago

When the box is checked and HS256 is used with OIDC.

HS256 requires that the token is signed by a symmetric key that is shared between the Relying Party and the Authorization Server. The application's secret is used for this signature.

When you leave "hash secret" checked, the Authorization Server will lose the original secret value and will use the hashed secret when signing the token. But this breaks the OIDC's contract as the Relying Party is no longer able to verify the token's signature.

The token is completely valid from the OAuth Toolkit's viewpoint - you can use it to authenticate against Django views, etc. Good OIDC libraries should check the signature by default, but when doing so, they will reject the provided token, as the signatures mismatch.

You can leave "hash secret" checked without any problems if you:

blag commented 1 month ago

@gardenerik Thanks for the explanation, that's what I thought. I'll whip up a PR to tweak the help text to be a tiny bit more clear about what state it's referring to.

As a slightly separate issue: when a user unchecks the box and uses HS256 with OIDC, does it make sense to leave the (unhashed) symmetric key visible to the user on future page loads? It seems to me that a lot of secret values are only shown once to the user (this is exactly how GitHub works with developer keys), and I assume that this is done to make it more difficult for an attacker to steal or the secret from the OIDC provider and/or spoof the provider entirely (with the correct shared secret).

When a user chooses to leave the "hash secret" option checked when registering an application with django-oauth-toolkit, it looks a bit unpolished to me to display the hashed value in the form again on subsequent page loads, so I'm considering a second PR to only show the secret value once, when it is generated. But I want to make sure I support the HS256+OIDC flow as well in that PR.

Thanks!