Skip to content
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

Merged
merged 2 commits into from
Apr 29, 2020
Merged

fix(verdaccio-htpasswd): generate non-constant legacy 2 byte salt #357

merged 2 commits into from
Apr 29, 2020

Conversation

michaellotz-iart
Copy link
Contributor

@michaellotz-iart michaellotz-iart commented Apr 28, 2020

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.

@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #357 into 9.x will increase coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            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              
Flag Coverage Δ
#core 86.36% <ø> (ø)
#plugins 66.45% <100.00%> (+0.03%) ⬆️
Impacted Files Coverage Δ
plugins/htpasswd/src/utils.ts 74.24% <80.00%> (ø)
plugins/htpasswd/src/crypt3.ts 100.00% <100.00%> (ø)

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 461e008...f885801. Read the comment docs.

@michaellotz-iart
Copy link
Contributor Author

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.

@michaellotz-iart
Copy link
Contributor Author

Noticed that the password change function actually failed when the salt changed. Fixed in the followup commit including a test.

DanielRuf
DanielRuf previously approved these changes Apr 29, 2020
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

LGTM

@juanpicado
Copy link
Member

@michaellotz-iart could you rebase on top of 9.x? ... there was an error and master has breaking changes .... the difference is minimal but I cannot merge on master this PR.

@DanielRuf DanielRuf changed the base branch from master to 9.x April 29, 2020 20:04
@DanielRuf DanielRuf dismissed their stale review April 29, 2020 20:04

The base branch was changed.

@DanielRuf
Copy link
Contributor

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.
@juanpicado
Copy link
Member

juanpicado commented Apr 30, 2020

will be released here verdaccio/verdaccio#1790

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants