Skip to content

Conversation

mallardduck
Copy link
Contributor

If sodium provides the argon2 then threads must be 1, if "standard" (libargon2) is the provider it can be 1 or 2.
Not sure why this seems to only affect PHP 8.1 however as the behaviour seems to date back quite a bit.

https://github.com/php/php-src/blob/2a3760a2d12b1a4c5c0207386b6cae84e404ee7e/ext/sodium/sodium_pwhash.c#L197

Related to thread here: https://twitter.com/nikita_ppv/status/1443525930979401733

This could be a potential fix if there's not a more elegant solution "upstream" in php-src found.

At this point it appears there is no way to control or manually set the PASSWORD_ARGON2_PROVIDER. As when you use/load the sodium extension it (libsodium) takes over functionality of hashing argon2 from libargon2. This is not something you can control via php.ini at this time - so I think the choice is either:

  1. Compile with sodium extension and use libsodium for hashing,
  2. do NOT compile sodium extension and use libargon2 for argon2 hashing.

If sodium provides the argon2 then threads must be 1, if "standard" (libargon2) is the provider it can be 1 or 2.
Not sure why this seems to only affect PHP 8.1 however as the behaviour seems to date back quite a bit.

https://github.com/php/php-src/blob/2a3760a2d12b1a4c5c0207386b6cae84e404ee7e/ext/sodium/sodium_pwhash.c#L197
@driesvints
Copy link
Member

Can you remove the PHP 8.1 skips from the encryption tests to verify this is working now?

@mallardduck
Copy link
Contributor Author

Absolutely! Completely overlooked those were skipped - updated now.
They did pass locally, but no i'm anxiously waiting to see CI results.
(hoping I'm not making a big goof of myself lol)

@driesvints
Copy link
Member

Seems like there are some failures (ignore the Redis ones for now).

@mallardduck
Copy link
Contributor Author

While this PR isn't "terrible" for a fix - after more digging I suspect this is a new bug in PHP 8.1 that does not affect PHP 8.0 builds. Specifically based on what Nikita said on twitter - my theory that loading ext-sodium disabled libargon hashing was wrong. I see that now as if libargon2 is loaded the init function will bail with success much sooner.

However with that in mind, we can infer that libargon2 is NOT being loaded. Otherwise the PASSWORD_ARGON2_PROVIDER would be standard instead of sodium. So this lead me to doing a local build of PHP 8.1 with sodium completely disabled. This should force it to use libargon2 no matter what - however instead I just got errors claiming Agon2 isn't supported at all.

All that is enough to confirm - for my system - that PHP 8.1 is no longer building with libargon2 properly. I suspect the same bug is present in the GitHub builds as well. I've also done testing with custom compiled PHP 8.0 as well. The results of that have (soft) confirmed (IMHO) to be a bug in PHP 8.1.

When using PHP 8.0 w/o sodium, I found that libargon2 was used as expected and working fine. So this helps confirm that PHP 8.1 is acting differently than PHP 8.0 compiled with the same options on the same system. That said, this PR may not be a bad idea to make the PW hashing more resilent/flexible. But I think there is a "deeper" issue within PHP 8.1 causing this.

@driesvints
Copy link
Member

Thanks a bunch for investigating all this @mallardduck. I'll put this PR in draft for now until Ben or Nikita have had time to investigate so we can choose on the best fix.

@driesvints driesvints marked this pull request as draft September 30, 2021 18:15
@mallardduck
Copy link
Contributor Author

Yep that sounds great!
I'll keep an eye out here: php/php-src#7538

@driesvints
Copy link
Member

@mallardduck I restarted the jobs and it seems with your merged fix that everything works again : )

Thanks a bunch!

@driesvints driesvints marked this pull request as ready for review October 4, 2021 12:05
@driesvints driesvints merged commit b52f8f4 into laravel:php8.1-fixes Oct 4, 2021
driesvints pushed a commit that referenced this pull request Oct 5, 2021
* Make ArgonHasher aware of argon provider

If sodium provides the argon2 then threads must be 1, if "standard" (libargon2) is the provider it can be 1 or 2.
Not sure why this seems to only affect PHP 8.1 however as the behaviour seems to date back quite a bit.

https://github.com/php/php-src/blob/2a3760a2d12b1a4c5c0207386b6cae84e404ee7e/ext/sodium/sodium_pwhash.c#L197

* remove skips for Hashing tests
driesvints pushed a commit that referenced this pull request Oct 15, 2021
* Make ArgonHasher aware of argon provider

If sodium provides the argon2 then threads must be 1, if "standard" (libargon2) is the provider it can be 1 or 2.
Not sure why this seems to only affect PHP 8.1 however as the behaviour seems to date back quite a bit.

https://github.com/php/php-src/blob/2a3760a2d12b1a4c5c0207386b6cae84e404ee7e/ext/sodium/sodium_pwhash.c#L197

* remove skips for Hashing tests
driesvints pushed a commit that referenced this pull request Oct 22, 2021
* Make ArgonHasher aware of argon provider

If sodium provides the argon2 then threads must be 1, if "standard" (libargon2) is the provider it can be 1 or 2.
Not sure why this seems to only affect PHP 8.1 however as the behaviour seems to date back quite a bit.

https://github.com/php/php-src/blob/2a3760a2d12b1a4c5c0207386b6cae84e404ee7e/ext/sodium/sodium_pwhash.c#L197

* remove skips for Hashing tests
taylorotwell pushed a commit that referenced this pull request Oct 22, 2021
* Remove platform requirement

* Enable prefer-lowest builds for PHP 8.1

* Bump whoops

* Minimum symfony version

* Bump carbon

* Try dev

* Require commonmark

* Add phpunit

* Bump predis

* More clearly describe failing mail tests

* Remove skips for Redis tests

* try predis dev

* Make ArgonHasher aware of argon provider (#39046)

* Make ArgonHasher aware of argon provider

If sodium provides the argon2 then threads must be 1, if "standard" (libargon2) is the provider it can be 1 or 2.
Not sure why this seems to only affect PHP 8.1 however as the behaviour seems to date back quite a bit.

https://github.com/php/php-src/blob/2a3760a2d12b1a4c5c0207386b6cae84e404ee7e/ext/sodium/sodium_pwhash.c#L197

* remove skips for Hashing tests

* Enable tests

* Re-skip test

* Adjust skips

* Re-enable dynamodb tests

* Bump predis

* Bump PHP AWS SDK

* Remove test skipping

* Update tests.yml

* Update tests.yml

* Bump SwiftMailer

* string fallbacks

* Update phpunit.xml.dist

* Remove skips

Co-authored-by: Dan <self@danpock.me>
chu121su12 pushed a commit to chu121su12/framework that referenced this pull request Oct 23, 2021
* Remove platform requirement

* Enable prefer-lowest builds for PHP 8.1

* Bump whoops

* Minimum symfony version

* Bump carbon

* Try dev

* Require commonmark

* Add phpunit

* Bump predis

* More clearly describe failing mail tests

* Remove skips for Redis tests

* try predis dev

* Make ArgonHasher aware of argon provider (laravel#39046)

* Make ArgonHasher aware of argon provider

If sodium provides the argon2 then threads must be 1, if "standard" (libargon2) is the provider it can be 1 or 2.
Not sure why this seems to only affect PHP 8.1 however as the behaviour seems to date back quite a bit.

https://github.com/php/php-src/blob/2a3760a2d12b1a4c5c0207386b6cae84e404ee7e/ext/sodium/sodium_pwhash.c#L197

* remove skips for Hashing tests

* Enable tests

* Re-skip test

* Adjust skips

* Re-enable dynamodb tests

* Bump predis

* Bump PHP AWS SDK

* Remove test skipping

* Update tests.yml

* Update tests.yml

* Bump SwiftMailer

* string fallbacks

* Update phpunit.xml.dist

* Remove skips

Co-authored-by: Dan <self@danpock.me>
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.

2 participants