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

feat: allow other password hashing algorithms #580

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

juanpicado
Copy link
Member

@juanpicado juanpicado commented Jun 11, 2022

⚠️ TLDR: Use bcrypt is much more secure, verdaccio 5 uses crypt by default to avoid breaking changes, the next major (v6) will switch to bcrypt by default and invalidate your current passwords. This does not apply if you are using your own authentication plugin.
⚠️ Switching to bcrypt will invalidate all your current passwords, it's recommended to remove the htpassw file and start from scratch.

Type: feat
Scope: plugin

The following has been addressed in the PR:

  • There is a related issue?
  • Unit or Functional tests are included in the PR

Description:

copied from v6 plugins by @greshilov verdaccio/verdaccio#2072

To avoid a breaking change, the default algorithm is crypt.

Context

The current implementation of the htpasswd module supports multiple hash formats on verify, but only crypt on sign in.
crypt is an insecure old format, so to improve the security of the new verdaccio release we introduce the support of multiple hash algorithms on sign in step.

New hashing algorithms

The new possible hash algorithms to use are bcrypt, md5, sha1. bcrypt is chosen as a default, because of its customizable complexity and overall reliability. You can read more about them here.

Two new properties are added to auth section in the configuration file:

  • algorithm to choose the way you want to hash passwords.
  • rounds is used to determine bcrypt complexity. So one can improve security according to increasing computational power.

Example of the new auth config file section:

auth:
htpasswd:
  file: ./htpasswd
  max_users: 1000
  # Hash algorithm, possible options are: "bcrypt", "md5", "sha1", "crypt".
  algorithm: bcrypt
  # Rounds number for "bcrypt", will be ignored for other algorithms.
  rounds: 10

@juanpicado juanpicado marked this pull request as ready for review June 11, 2022 22:14
@juanpicado juanpicado added the enhancement New feature or request label Jun 11, 2022

switch (hashConfig.algorithm) {
case HtpasswdHashAlgorithm.bcrypt:
hash = bcrypt.hashSync(passwd, hashConfig.rounds);
Copy link
Contributor

@greshilov greshilov Jun 14, 2022

Choose a reason for hiding this comment

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

generateHtpasswdLine is used in asynchronous changePasswordToHTPasswd, so I think it would be great to use asynchronous bcrypt.hash instead of bcrypt.hashSync to prevent blocking and make this function async.

I can try to do this in a separate PR :)

Copy link
Member Author

@juanpicado juanpicado Jun 14, 2022

Choose a reason for hiding this comment

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

Does bcrypt.hash worked out last time? I barely remember there were few issues with it. Doens't? In anyway, LGTM for a separated PR, I think won't break any API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried, and it seemed to work according to tests. Please, take a look here:
#589

Copy link
Contributor

@greshilov greshilov 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 juanpicado merged commit 49ca26d into 9.x Jun 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the jota/htpasswd-algorithms branch June 14, 2022 16:27
@juanpicado
Copy link
Member Author

Thanks @greshilov for the review, really appreciated 💯

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

Successfully merging this pull request may close these issues.

2 participants