Skip to content

Add ClientSecretField field to use Django password hashing algorithms #1020

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.

Sign up for GitHub

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

Jump to bottom
Merged
n2ygk merged 16 commits into from
Jan 3, 2022
Merged

Add ClientSecretField field to use Django password hashing algorithms #1020

n2ygk merged 16 commits into from
Jan 3, 2022

Conversation

@pkarman
Copy link
Contributor

@pkarman pkarman commented Oct 13, 2021 •
edited
Loading

Fixes #729

Description of the Change

The client_secret is effectively not a secret if stored as plaintext. This becomes a security risk if a database is compromised, especially for 2-legged OAuth2 (client credentials flow) where there is no other mitigating credential to be used in authentication.

NOTE that this preserves the existing behavior of not hashing client_secret by default. To enable it, it must be explicitly set. In theory this should provide backwards compatibility with existing installations.

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

@pkarman pkarman mentioned this pull request Oct 13, 2021
Closed
auvipy
auvipy previously requested changes Oct 15, 2021
View reviewed changes
@codecov
Copy link

codecov bot commented Oct 15, 2021 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.70%. Comparing base (e657d7b) to head (cb1698b).
Report is 216 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1020      +/-   ##
==========================================
+ Coverage   96.67%   96.70%   +0.02%     
==========================================
  Files          31       31              
  Lines        1775     1791      +16     
==========================================
+ Hits         1716     1732      +16     
  Misses         59       59

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@n2ygk n2ygk added this to the 1.6.1 milestone Dec 22, 2021
@n2ygk n2ygk self-requested a review December 22, 2021 15:55
@n2ygk
Copy link
Contributor

n2ygk commented Dec 22, 2021

@pkarman sorry for the review delay. I'm targeting this change for the next minor release. Can you rebase and respond to my review questions? This looks like good stuff. Thanks.

@n2ygk n2ygk modified the milestones: 1.6.1, 1.7.0 Dec 22, 2021
@karpet
Copy link

karpet commented Jan 2, 2022

@n2ygk -- no problem. I don't see any review comments? This branch has been merged with upstream.

n2ygk
n2ygk requested changes Jan 2, 2022
View reviewed changes
Copy link
Contributor

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my review comments were pending. I thought I had submitted them.

Please rebase with current master branch as well

n2ygk
n2ygk requested changes Jan 2, 2022
View reviewed changes
@pkarman
Copy link
Contributor Author

pkarman commented Jan 3, 2022

@n2ygk thanks for the review. I think I've addressed all your comments and rebased with current master.

n2ygk
n2ygk approved these changes Jan 3, 2022
View reviewed changes
Copy link
Contributor

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for this!

@n2ygk n2ygk dismissed auvipy’s stale review January 3, 2022 14:53

I believe these changes were made but to marked resolved.

n2ygk
n2ygk reviewed Jan 3, 2022
View reviewed changes
@n2ygk n2ygk merged commit 250120d into django-oauth:master Jan 3, 2022
@pkarman pkarman deleted the pek-scrypt-client-secret branch January 3, 2022 15:55
This was referenced Jan 5, 2022
Closed
Closed
Merged
Closed
Closed
@pkarman pkarman mentioned this pull request Jan 9, 2022
Merged
n2ygk pushed a commit that referenced this pull request Jan 9, 2022
@pkarman
Revert client secret hash #1020 (#1082)
94d42a8 
* Revert "Add migration that alters client_secret to ClientSecretField. (#1075)"

This reverts commit 58f4f5f.

* revert 250120d

* bad merge
n2ygk added a commit to n2ygk/django-oauth-toolkit that referenced this pull request Jan 20, 2022
@pkarman @auvipy @n2ygk
Add ClientSecretField field to use Django password hashing algorithms (…
e44c88c 
…django-oauth#1020)

Co-authored-by: Asif Saif Uddin <[email protected]>
Co-authored-by: Alan Crosswell <[email protected]>
@n2ygk n2ygk mentioned this pull request Jan 20, 2022
Merged
5 tasks
n2ygk added a commit to n2ygk/django-oauth-toolkit that referenced this pull request Jan 24, 2022
@pkarman @n2ygk
Add ClientSecretField field to use Django password hashing algorithms (…
36f767c 
…django-oauth#1020)

Co-authored-by: Alan Crosswell <[email protected]>
n2ygk added a commit that referenced this pull request Jan 25, 2022
@n2ygk @Andrew-Chen-Wang @pkarman
Hash application client secrets using Django password hashing (#1093)
691870c 
* Add ClientSecretField field to use Django password hashing algorithms (#1020)

Co-authored-by: Andrew Chen Wang <[email protected]>
Co-authored-by: Peter Karman <[email protected]>
Co-authored-by: Andrew Chen Wang <[email protected]>
flow3d pushed a commit to singular-labs/django-oauth-toolkit that referenced this pull request Mar 22, 2022
@n2ygk @Andrew-Chen-Wang @pkarman
Hash application client secrets using Django password hashing (django…
5ef4225 
…-oauth#1093)

* Add ClientSecretField field to use Django password hashing algorithms (django-oauth#1020)

Co-authored-by: Andrew Chen Wang <[email protected]>
Co-authored-by: Peter Karman <[email protected]>
Co-authored-by: Andrew Chen Wang <[email protected]>
@tonial tonial mentioned this pull request May 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@n2ygk n2ygk n2ygk approved these changes

@wiliamsouza wiliamsouza Awaiting requested review from wiliamsouza

@ermissa ermissa Awaiting requested review from ermissa

+1 more reviewer

@auvipy auvipy auvipy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

Why are secrets in plain text?

4 participants

@pkarman @n2ygk @karpet @auvipy