-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix(verdaccio-htpasswd): generate non-constant legacy 2 byte salt #357
Conversation
Codecov Report
@@ Coverage Diff @@
## 9.x #357 +/- ##
==========================================
+ Coverage 68.14% 68.16% +0.02%
==========================================
Files 29 29
Lines 1400 1401 +1
Branches 204 204
==========================================
+ Hits 954 955 +1
Misses 406 406
Partials 40 40
Continue to review full report at Codecov.
|
I have updated my change with mocks for the randomBytes so that the tests work with a constant input. It would possibly be more obvious to not generate the previous prefix bytes but update the snapshots instead. This however shows that the change is backwards compatible. |
Noticed that the password change function actually failed when the salt changed. Fixed in the followup commit including a test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@michaellotz-iart could you rebase on top of |
I changed the target / base branch. |
The crypt implementation in use does only support the legacy crypt hashing with a 2 byte salt. Supplying it with a prefixed salt will simply make the given prefix a constant salt value ('$6' with the defaults here). Note that the crypt hashing is weak either way, this just further weakens it by essentially removing the extra complexity a random salt provides for the individual entries. This change is backwards compatible. Existing entries will retain their constant '$6' salt, new and updated ones will get variable salts.
To generate the proper comparison string, crypt3 needs the salt from the original entry. This previously worked only due to the constant salt. The verifyPassword function already does this correctly. Add a test that verifies that the password change works and a different result is returned when a new salt is generated.
will be released here verdaccio/verdaccio#1790 |
Type: bug/feature
Scope: verdaccio-htpasswd
The crypt implementation in use does not actually support the newer hash types. Calling it with a prefixed salt results in the prefix becoming the salt and rendering the salt useless as it is now constant.
Description:
This PR adds an unprefixed salt type for legacy 2 byte salting and requests that by default.
I have not updated the snapshot tests as it seems not possible to provide a single fixed password string to test against. Mocking the salt generation as is done in the crypt3.ts tests would possibly work, but I am not familiar enough with the test framework to provide this right now.