Skip to content

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

Open
remicollet wants to merge 2 commits intophp:PHP-7.4from
remicollet:issue-sodium-argon2
Open

password_hash: don't fail on unsupported threads value#7099
remicollet wants to merge 2 commits intophp:PHP-7.4from
remicollet:issue-sodium-argon2

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