Allow the use of unhashed secrets #1311

gardenerik · 2023-09-07T19:52:02Z

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

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
CHANGELOG.md updated (only for user relevant changes)
author name in AUTHORS

n2ygk · 2023-09-07T20:02:40Z

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

codecov · 2023-09-07T20:06:09Z

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%)`	⬆️
oauth2_provider/oauth2_validators.py	`94.07% <100.00%> (+0.09%)`	⬆️

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

gardenerik · 2023-09-07T20:14:28Z

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

n2ygk · 2023-09-07T20:15:01Z

@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 · 2023-09-07T20:32:25Z

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

n2ygk · 2023-09-08T12:55:43Z

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

Haha I was looking at my suggestions:

n2ygk

This looks great but please do the following additional:

add the option to the HTML template for application registration
Add option to createapplication management command
document that new hash_client_secret option more thoroughly. See, for example references to hashing in getting_started and tutorial_01 (including screen shots).

n2ygk · 2023-09-08T13:35:00Z

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

gardenerik · 2023-09-10T20:27:49Z

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

n2ygk

Thanks for leaving the campground cleaner than when you found it:-)

I have proposed one minor wording change just to make clear that there's a security exposure to not hashing client secrets.

docs/getting_started.rst

n2ygk · 2023-09-12T20:57:23Z

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

n2ygk · 2023-09-13T15:37:20Z

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'

Co-authored-by: Alan Crosswell <[email protected]>

n2ygk · 2023-09-13T18:22:29Z

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

hugochinchilla · 2023-11-24T08:38:34Z

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 :)

jazzband/django-oauth-toolkit#1311 was merged, but still not released. We would like to use it, so...

relrod · 2024-04-29T00:31:11Z

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

In newer DOT than what AWX uses, Application.client_secret is hashed automatically with no way to disable that functionality. There's a PR that allows for disabling that functionality ([0]), but that hasn't made it into a release. The DOT hashing is incompatible with our standard encryption - when DOT gets the value it ends up getting our encrypted string and trying to act on that. Ideally we'd like to disable their hashing entirely and use our standard encryption tooling. AWX avoids this problem by pinning to an older DOT. For now in DAB we'll just use the upstream hashing, and not treat the field as an encrypted_fields field to avoid the "double encryption" issue. [0]: jazzband/django-oauth-toolkit#1311 Signed-off-by: Rick Elrod <[email protected]>

n2ygk · 2024-05-13T15:02:50Z

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.

…an HS256.

* in-process release 2.4.0 pending some late PR merges. * Update #1311 documentation to recommend using RS256 rather than HS256. * editorial changes to CHANGELOG * fix line too long

blag · 2024-08-01T06:40:22Z

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:

The case when the box is checked (and the client secret is hashed) may lead to incompatibilities with some OIDC RP libraries or...
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 · 2024-08-01T09:06:23Z

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:

use RS256 (the token is signed by OIDC_RSA_PRIVATE_KEY and thus verified using the toolkit's public key)
use HS256, but do not use OIDC, only OAuth2 - OAuth2 does not have any specifications on the token, so it is just treated as is, without any signature checking

blag · 2024-08-23T16:55:59Z

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

n2ygk self-requested a review September 8, 2023 12:55

n2ygk requested changes Sep 8, 2023

View reviewed changes

n2ygk requested a review from Andrew-Chen-Wang September 8, 2023 13:34

n2ygk requested changes Sep 12, 2023

View reviewed changes

docs/getting_started.rst Outdated Show resolved Hide resolved

n2ygk approved these changes Sep 12, 2023

View reviewed changes

gardenerik and others added 12 commits September 13, 2023 11:37

accept unhashed secrets in requests

3e3764a

allow choosing whether to hash the client secret per application

580fffa

add tests

80cf4e3

add tests' migrations

c85aa8c

add mention to oidc docs

441dde3

add name to AUTHORS

4ac0fac

add changelog entry

7708a36

add hash_client_secret to frontend forms

79ccde9

add parameter to createapplication

ee02d36

add more documentation

1a5962a

client credentials registration should not contain redirect uri, oops

57e9041

apply suggestion from review

8ab8f8d

Co-authored-by: Alan Crosswell <[email protected]>

resolve conflicting migrations.

be42cc7

n2ygk merged commit 0965100 into jazzband:master Sep 13, 2023
26 checks passed

amanning9 mentioned this pull request Oct 6, 2023

Fix unhashed secret to work with request body authentication. #1334

Merged

5 tasks

gardenerik added a commit to trojsten/id that referenced this pull request Dec 16, 2023

master: use django-oauth-toolkit from git

cd70625

jazzband/django-oauth-toolkit#1311 was merged, but still not released. We would like to use it, so...

n2ygk added a commit to n2ygk/django-oauth-toolkit that referenced this pull request May 13, 2024

Update jazzband#1311 documentation to recommend using RS256 rather th…

685a7b0

…an HS256.

islam-kamel pushed a commit that referenced this pull request May 19, 2024

Release 2 4 0 (#1420)

f34ba7c

* in-process release 2.4.0 pending some late PR merges. * Update #1311 documentation to recommend using RS256 rather than HS256. * editorial changes to CHANGELOG * fix line too long

n2ygk mentioned this pull request Aug 30, 2024

Upgrading from 2.3.0 to 2.4.0 raises JSONDecodeError and psycopg.errors.UndefinedColumn #1471

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the use of unhashed secrets #1311

Allow the use of unhashed secrets #1311

gardenerik commented Sep 7, 2023 •

edited

Loading

n2ygk commented Sep 7, 2023

codecov bot commented Sep 7, 2023 •

edited

Loading

gardenerik commented Sep 7, 2023

n2ygk commented Sep 7, 2023

gardenerik commented Sep 7, 2023 •

edited

Loading

n2ygk commented Sep 8, 2023

n2ygk left a comment

n2ygk commented Sep 8, 2023

gardenerik commented Sep 10, 2023

n2ygk left a comment

n2ygk commented Sep 12, 2023

n2ygk commented Sep 13, 2023

n2ygk commented Sep 13, 2023

hugochinchilla commented Nov 24, 2023

relrod commented Apr 29, 2024

n2ygk commented May 13, 2024

blag commented Aug 1, 2024

gardenerik commented Aug 1, 2024 •

edited

Loading

blag commented Aug 23, 2024

Allow the use of unhashed secrets #1311

Allow the use of unhashed secrets #1311

Conversation

gardenerik commented Sep 7, 2023 • edited Loading

Description of the Change

Checklist

n2ygk commented Sep 7, 2023

codecov bot commented Sep 7, 2023 • edited Loading

Codecov Report

gardenerik commented Sep 7, 2023

n2ygk commented Sep 7, 2023

gardenerik commented Sep 7, 2023 • edited Loading

n2ygk commented Sep 8, 2023

n2ygk left a comment

Choose a reason for hiding this comment

n2ygk commented Sep 8, 2023

gardenerik commented Sep 10, 2023

n2ygk left a comment

Choose a reason for hiding this comment

n2ygk commented Sep 12, 2023

n2ygk commented Sep 13, 2023

n2ygk commented Sep 13, 2023

hugochinchilla commented Nov 24, 2023

relrod commented Apr 29, 2024

n2ygk commented May 13, 2024

blag commented Aug 1, 2024

gardenerik commented Aug 1, 2024 • edited Loading

blag commented Aug 23, 2024

gardenerik commented Sep 7, 2023 •

edited

Loading

codecov bot commented Sep 7, 2023 •

edited

Loading

gardenerik commented Sep 7, 2023 •

edited

Loading

gardenerik commented Aug 1, 2024 •

edited

Loading