fix: use sufficient computational effort for password hash by jsjoeio · Pull Request #3422 · coder/code-server · GitHub

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:

Link to Excalidraw

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

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

bcrypt!

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

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.

Notes:

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

@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

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.

Removed the docs as requested! Ready for another review

All good! Thanks for sticking through this ❤️

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 added this to the 3.11.0 milestone May 19, 2021

jsjoeio added the security Security related label May 19, 2021

jsjoeio self-assigned this May 19, 2021

jsjoeio changed the title title here fix: use sufficient computational effort for password hash May 19, 2021

jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 72d281f to 1c4a557 Compare May 19, 2021 18:31

ammario approved these changes May 20, 2021

View reviewed changes

jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 5672008 to 158865b Compare May 20, 2021 23:31

jsjoeio marked this pull request as ready for review May 20, 2021 23:31

jsjoeio requested a review from a team as a code owner May 20, 2021 23:31

jsjoeio requested a review from oxy May 20, 2021 23:32

jsjoeio modified the milestones: 3.10.2, 3.11.0 May 21, 2021

jawnsy reviewed May 24, 2021

View reviewed changes

Comment thread package.json Outdated

jsjoeio force-pushed the jsjoeio/fix-password-hash branch 2 times, most recently from 9ef6d84 to cc6e284 Compare May 25, 2021 22:24

jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 9316f61 to 56b413a Compare June 1, 2021 23:23

oxy suggested changes Jun 2, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread CHANGELOG.md Outdated

Comment thread package.json Outdated

Comment thread src/node/routes/login.ts Outdated

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

jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 886ed21 to d929ea4 Compare June 2, 2021 20:03

jsjoeio marked this pull request as draft June 3, 2021 00:25

jsjoeio force-pushed the jsjoeio/fix-password-hash branch 3 times, most recently from 20786db to 6d240a0 Compare June 4, 2021 21:28

This was referenced Jun 4, 2021

Improve authentication flow with auth token #3546

Open

Write tests for login route #3547

Closed

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

chore: update to argon2 in test

51f8341

refactor: use argon2 instead of bcrypt

70197bb

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.

refactor: update unit tests for hash fns

fd3cb6c

Since the hash and isHashMatch are now async, I had to update the tests accordingly. Now everything is working.

refactor: update login logic with new async hashing

fcc3f0d

This adds the proper await logic for the hashing of passwords.

refactor: make authenticated async everywhere

0cdbd33

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.

refactor: make ensureAuthenticated async

91303d4

refactor: make wsProxy async

1134780

refactor: update hash fn in test config

788b958

feat: update cli and test for hashed-password

ffa5c16

feat: add getPasswordMethod & test for it

7ff4117

feat: add handlePasswordValidation + tests

a14ea39

refactor: rewrite password logic at /login

409b473

feat: add isCookieValid function and tests

6020480

refactor: password logic in http w/ isCookieValid

923761c

docs: update FAQ with new hashing instructions

517aaf7

feat: add splitOnFirstEquals function

531b7c0

refactor: parse options with multiple = in cli

8c2bb61

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.

feat: add npm_config_build_from_source to build scripts

deaa224

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.

fix: sanitize password and cookie key

3b50bfc

jsjoeio force-pushed the jsjoeio/fix-password-hash branch from c6c5f3f to 4e074f8 Compare June 8, 2021 21:34

jsjoeio requested a review from oxy June 8, 2021 21:48

feat: check for empty str in isHashMatch

1e55a64

jsjoeio force-pushed the jsjoeio/fix-password-hash branch from 4e074f8 to 1e55a64 Compare June 8, 2021 22:11

oxy approved these changes Jun 9, 2021

View reviewed changes

jsjoeio merged commit 717eaa6 into main Jun 9, 2021

jsjoeio deleted the jsjoeio/fix-password-hash branch June 9, 2021 21:32

jsjoeio mentioned this pull request Nov 2, 2021

Investigate argon2 issue (Termux/Raspberry Pi) #4422

Closed

jsjoeio mentioned this pull request Jan 12, 2022

Build instructions #17

Closed

fix: use sufficient computational effort for password hash by jsjoeio · Pull Request #3422 · coder/code-server · GitHub

Uh oh!

fix: use sufficient computational effort for password hash - #3422

fix: use sufficient computational effort for password hash#3422
jsjoeio merged 27 commits into
mainfrom
jsjoeio/fix-password-hash

jsjoeio commented May 19, 2021 •
edited
Loading

Uh oh!

Uh oh!

codecov Bot commented May 19, 2021 •
edited
Loading

Uh oh!

Uh oh!

ammario left a comment

Uh oh!

jsjoeio commented May 24, 2021

Uh oh!

Uh oh!

oxy left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsjoeio commented Jun 2, 2021 •
edited
Loading

Uh oh!

Uh oh!

jsjoeio commented Jun 4, 2021

Uh oh!

jsjoeio commented Jun 8, 2021

Uh oh!

oxy left a comment

Uh oh!

jsjoeio commented Jun 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fix: use sufficient computational effort for password hash by jsjoeio · Pull Request #3422 · coder/code-server · GitHub

Uh oh!

fix: use sufficient computational effort for password hash - #3422

fix: use sufficient computational effort for password hash#3422jsjoeio merged 27 commits intomaincoder/code-server:mainfrom jsjoeio/fix-password-hashcoder/code-server:jsjoeio/fix-password-hashCopy head branch name to clipboard

Conversation

jsjoeio commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Screenshots

Checklist

Notes

Uh oh!

codecov Bot commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ammario left a comment

Choose a reason for hiding this comment

Uh oh!

jsjoeio commented May 24, 2021

Uh oh!

Uh oh!

oxy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsjoeio commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsjoeio commented Jun 4, 2021

Uh oh!

jsjoeio commented Jun 8, 2021

Uh oh!

oxy left a comment

Choose a reason for hiding this comment

Uh oh!

jsjoeio commented Jun 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fix: use sufficient computational effort for password hash#3422
jsjoeio merged 27 commits into
mainfrom
jsjoeio/fix-password-hash

jsjoeio commented May 19, 2021 •
edited
Loading

codecov Bot commented May 19, 2021 •
edited
Loading

jsjoeio commented Jun 2, 2021 •
edited
Loading