Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

fix: use sufficient computational effort for password hash #3422

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

Merged
jsjoeio merged 27 commits into main from jsjoeio/fix-password-hash
Jun 9, 2021

Conversation

Copy link
Contributor

@jsjoeio jsjoeio commented May 19, 2021 •
edited
Loading

This PR modifies the underlying algorithm used in the hash function to use sufficient computational effort.

Changes

  • refactor to use argon2 instead of sha256
  • add unit tests for hash and helper functions

Screenshots

Using a hashed-password (sha256)

Screen.Recording.2021年06月03日.at.11.37.25.AM.mov

Using a regular password (not hashed)

Screen.Recording.2021年06月03日.at.11.40.37.AM.mov

Using a hashed-password (argon2)

Screen.Recording.2021年06月04日.at.1.46.16.PM.mov

Checklist

  • tested locally
  • added new dependencies
  • added tests
  • updated CHANGELOG.md
  • ensure we still support hashed_password in the config which is hashed with sha256

Fixes #3381

Follow-up: #3432

Notes

Here is how authentication works in code-server:

image

Link to Excalidraw

@jsjoeio jsjoeio added this to the 3.11.0 milestone May 19, 2021
@jsjoeio jsjoeio added the security Security related label May 19, 2021
@jsjoeio jsjoeio self-assigned this May 19, 2021
@jsjoeio jsjoeio changed the title (削除) title here (削除ここまで) (追記) fix: use sufficient computational effort for password hash (追記ここまで) May 19, 2021
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 72d281f to 1c4a557 Compare May 19, 2021 18:31
Copy link

codecov bot commented May 19, 2021 •
edited
Loading

Codecov Report

Merging #3422 (0fbeb93) into main (d8c3ba6) will increase coverage by 1.32%.
The diff coverage is 76.78%.

❗ Current head 0fbeb93 differs from pull request most recent head 1e55a64. Consider uploading reports for the commit 1e55a64 to get more accurate results
Impacted file tree graph 

@@ Coverage Diff @@
## main #3422 +/- ##
==========================================
+ Coverage 59.21% 60.53% +1.32% 
==========================================
 Files 35 35 
 Lines 1709 1784 +75 
 Branches 379 403 +24 
==========================================
+ Hits 1012 1080 +68 
- Misses 559 562 +3 
- Partials 138 142 +4
Impacted Files Coverage Δ
src/node/routes/vscode.ts 29.78% <0.00%> (-0.33%) ⬇️
src/node/routes/login.ts 42.59% <20.00%> (-2.70%) ⬇️
src/node/routes/domainProxy.ts 62.50% <40.00%> (-1.61%) ⬇️
src/node/routes/index.ts 77.14% <50.00%> (ø)
src/node/routes/pathProxy.ts 67.85% <50.00%> (ø)
src/node/routes/static.ts 61.36% <66.66%> (+0.89%) ⬆️
src/node/http.ts 36.06% <76.92%> (+3.27%) ⬆️
src/node/util.ts 69.94% <91.30%> (+14.70%) ⬆️
src/node/cli.ts 79.83% <100.00%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8c3ba6...1e55a64. Read the comment docs.

Copy link
Member

@ammario ammario left a comment

Choose a reason for hiding this comment

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

bcrypt!

jsjoeio and ajhalili2006 reacted with hooray emoji
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 5672008 to 158865b Compare May 20, 2021 23:31
@jsjoeio jsjoeio marked this pull request as ready for review May 20, 2021 23:31
@jsjoeio jsjoeio requested a review from a team as a code owner May 20, 2021 23:31
@jsjoeio jsjoeio requested a review from oxy May 20, 2021 23:32
@jsjoeio jsjoeio modified the milestones: 3.10.2, 3.11.0 May 21, 2021
Copy link
Contributor Author

jsjoeio commented May 24, 2021

@oxy do you mind taking a look at this today or tomorrow?

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch 2 times, most recently from 9ef6d84 to cc6e284 Compare May 25, 2021 22:24
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 9316f61 to 56b413a Compare June 1, 2021 23:23
Copy link

@oxy oxy left a comment

Choose a reason for hiding this comment

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

argon2 is a stronger algorithm but bcrypt is also alright - though I'm still a little confused about how you're handling authentication, what hashedPassword is used for with the cookie, and how/if we compare against bcrypt passwords in the auth route itself.

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 56b413a to 8f45524 Compare June 2, 2021 17:08
Copy link
Contributor Author

jsjoeio commented Jun 2, 2021 •
edited
Loading

Notes:

  1. merge in PR (try argon2)
  2. write tests for router (testing one level up from hashing) - make an issue
  3. add token exchange -> make an issue
  4. rewrite other parts if needed? -> add to 3

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 886ed21 to d929ea4 Compare June 2, 2021 20:03
@jsjoeio jsjoeio marked this pull request as draft June 3, 2021 00:25
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch 3 times, most recently from 20786db to 6d240a0 Compare June 4, 2021 21:28
Copy link
Contributor Author

jsjoeio commented Jun 4, 2021

@oxy do you mind taking a look at the security warnings/errors flagged by CodeQL? I know we plan to fix most of these in a follow-up PR but want to make sure my code isn't introducing anything you would flag: https://github.com/cdr/code-server/pull/3422/checks?check_run_id=2749748946

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 6d240a0 to cf2a570 Compare June 4, 2021 21:39
jsjoeio added 19 commits June 8, 2021 14:33 
This uses argon2 instead of bcrypt.
Note: this means the hash functions are now async which means we have to
refactor a lot of other code around auth.
Since the hash and isHashMatch are now async, I had to update the tests
accordingly. Now everything is working.
This adds the proper await logic for the hashing of passwords.
Since this checks if they are authenticated using the hash/password and it's
async, we need to update authenticated to be async, which means we have to
update it everywhere it's used.
There was a case with the hashed-password which had multiple equal signs in the
value and it wasn't being parsed correctly. This uses a new function and adds a
few tests.
This is necessary due to argon2 being added and an upstream issue where it uses
a Linux build that is too new for CentOS 7.
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from c6c5f3f to 4e074f8 Compare June 8, 2021 21:34
Copy link
Contributor Author

jsjoeio commented Jun 8, 2021

Removed the docs as requested! Ready for another review

@jsjoeio jsjoeio requested a review from oxy June 8, 2021 21:48
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 4e074f8 to 1e55a64 Compare June 8, 2021 22:11
Copy link

@oxy oxy left a comment

Choose a reason for hiding this comment

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

All good! Thanks for sticking through this ❤️

jsjoeio reacted with heart emoji
Copy link
Contributor Author

jsjoeio commented Jun 9, 2021

Thanks for sticking through this

Thanks for sharing your security knowledge with me! I feel like I'm slowing becoming more mindful of it thanks to you 😂

@jsjoeio jsjoeio merged commit 717eaa6 into main Jun 9, 2021
@jsjoeio jsjoeio deleted the jsjoeio/fix-password-hash branch June 9, 2021 21:32
@jsjoeio jsjoeio mentioned this pull request Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@code-asher code-asher code-asher left review comments

@ammario ammario ammario approved these changes

+3 more reviewers

@jawnsy jawnsy jawnsy approved these changes

@RA80533 RA80533 RA80533 left review comments

@oxy oxy oxy approved these changes

Reviewers whose approvals may not affect merge requirements
Labels
security Security related
Projects
None yet
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.

Fix password hash to use sufficient computational effort

AltStyle によって変換されたページ (->オリジナル) /