Skip to content

password_hash: don't fail on unsupported threads value #7099

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

Open
wants to merge 2 commits into
base: PHP-7.4
Choose a base branch
from

Conversation

remicollet
Copy link
Member

@remicollet remicollet commented Jun 4, 2021

As this is only about how hash is computed (in other implementation)
and have no effect on result.

This will improve code compatibility for the various implementations

Affects only distribution without argon2 library, and use sodium alternative implementation (which is faster)

As this is only about how hash is computed (in other implementation)
and have no effect on result.

This will improve code compatibility for the various implementations
@remicollet
Copy link
Member Author

Perhaps we can even drop this check

Notice: on PHP 8 this is even a value error (which also have to be changed to notice, or drop)

@sgolemon as you are the author of this implementation, please review.

@remicollet
Copy link
Member Author

  • 1st commit move from error to notice
  • 2nd commit, alternative, drop the check

@nikic
Copy link
Member

nikic commented Jun 4, 2021

I'm not convinced we should drop this. Yes, it may not impact the result, but it may affect timing assumptions. The programmer should drop the option for a compatible implementation.

@remicollet
Copy link
Member Author

About timing, for memory

  • bcrypt: 0.040"
  • libargon2, 1 thread: 0.143"
  • libargon2, 2 thread:s 0.083"
  • libargon2, 3 threads: 0.056"
  • libsodium, 1 thread: 0.082"

@remicollet
Copy link
Member Author

The main issue, for now, is that the 'thread' usage raises an error, and no password is generated.

Even if I don't really understand the perf point (both implementations have very different performance, and sodium is usually faster)...

At least switching from E_WARNING + FAILURE to E_NOTICE seems needed (1st commit)

P.S. I even think libargon2 should be deprecated when libsodium is used.

@Girgias Girgias requested review from TimWolla and removed request for TimWolla January 25, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants