Closed konarshankar07 closed 2 years ago
I'm not able to reproduce phpunit error for php8.1
Am I missing something?
Thanks
@konarshankar07 you'll have to do a composer update --prefer-lowest
and composer update
on 8.1 to run the suite and get the same results we got in CI.
Hello @Ocramius Can you please re-run the github action for the php 8.1? Cause I'm still not able to reproduce the issue with the phpunit test Thanks
@konarshankar07 I applied re-run
Thanks @froschdesign for the information but not able to reproduce in local
PHP version is same as the QA but still no luck. I'm curious why I'm not able to reproduce the same in the local
@konarshankar07
PHP version is same as the QA but still no luck.
A different version is used here:
PHP version: PHP 8.1.0RC4-dev (cli) (built: Oct 11 2021 00:13:42)
My suggestion: Check all the error messages here on GitHub and you will see that many things are repeated.
For example:
file_exists(): Passing null to parameter #1 ($filename) of type string is deprecated
base64_encode(): Passing null to parameter #1 ($string) of type string is deprecated
openssl_encrypt(): Passing null to parameter #7 ($aad) of type string is deprecated
Add new commits and look at results of the GitHub workflow. At the end you can squash the commits.
@konarshankar07
mhash_keygen_s2k
Warning This function has been DEPRECATED as of PHP 8.1.0. Relying on this function is highly discouraged.
This function has been deprecated. Use the hash_*() functions instead.
Hello @froschdesign
Regarding mhash_keygen_s2k
function depreciation, I think we need to deprecate the SaltedS2k
class and implement the new hash function class. because I'm not able to find the replacement for the mhash_keygen_s2k
function. Let me know your thoughts
Thanks
@konarshankar07 I think the deprecation message needs to be handled explicitly in the unit tests. Try changing test/Key/Derivation/SaltedS2kTest.php
as follows:
// test case for supressing deprecated message (obsolete)
public function testCalc()
{
if (! extension_loaded('hash')) {
$this->markTestSkipped('The hash extension is not available');
return;
}
if (PHP_VERSION_ID >= 80100) {
$this->expectDeprecation();
$this->expectDeprecationMessage('Function mhash_keygen_s2k() is deprecated');
}
$password = SaltedS2k::calc('sha256', 'test', $this->salt, 32);
$this->assertEquals(32, strlen($password));
$this->assertEquals('qzQISUBUSP1iqYtwe/druhdOVqluc/Y2TetdSHSbaw8=', base64_encode($password));
}
This would solve the issue in the unit tests. Nevertheless, we might need to think of a more complete solution.
It seems like there is indeed no replacement for the mhash_keygen_s2k()
function. I found a thread on StackOverflow that suggest a pure PHP implementation. I am quite skeptical of such things, but I tried putting it in src/Key/Derivation/SaltedS2k.php
like this:
// pure PHP implementation
public static function calc($hash, $password, $salt, $bytes)
{
if (! in_array($hash, array_keys(static::$supportedMhashAlgos))) {
throw new Exception\InvalidArgumentException("The hash algorithm $hash is not supported by " . __CLASS__);
}
if (mb_strlen($salt, '8bit') < 8) {
throw new Exception\InvalidArgumentException('The salt size must be at least of 8 bytes');
}
$result = '';
foreach (range(0, ceil($bytes / strlen(hash($hash, '', true))) - 1) as $i)
{
$result .= hash(
$hash,
str_repeat("\0", $i) . str_pad(
substr($salt, 0, 8),
8,
"\0",
STR_PAD_RIGHT
) . $password,
true
);
}
return substr($result, 0, intval($bytes));
}
And, indeed, that makes these tests pass for both PHP 8.0 and 8.1. @froschdesign what do you think of this? I saw that there is a pure PHP implementation in src/Key/Derivation/Pbkdf2.php
, so this solution might be acceptable.
The C source for this method is in php-src hash.c line 1287 ff..
@driehle
…we might need to think of a more complete solution.
This would be the optimum but on the other side this component is in security-only maintenance mode.
I saw that there is a pure PHP implementation in
src/Key/Derivation/Pbkdf2.php
, so this solution might be acceptable.
A pure implementation is okay. I'll add potential reviewers for that.
And, indeed, that makes these tests pass for both PHP 8.0 and 8.1. @froschdesign what do you think of this? I saw that there is a pure PHP implementation in
src/Key/Derivation/Pbkdf2.php
, so this solution might be acceptable.
We've done pure implementations in the past where either an implementation did not exist across all versions, or the behavior, for whatever reason, varied. I'm fine with it, so long as there are no significant changes to tests, and the implementation passes the tests.
Thanks, Matthew! @konarshankar07 would you mind adding the pure PHP implementation from here to your PR?
Hello @driehle Thanks.. I will add the PHP pure implementation in my PR
I don't think we should throw a deprecation message in SaltedS2k
(otherwise, what'd be the reason for switching to a pure PHP implementation?), but let's see what @froschdesign thinks.
ping @konarshankar07 workflow is failed. you did not sign your commit.
Fixed with #15
@konarshankar07 Thank you for your time and this contribution! 👍
Description
Declared php 8.1 support It has dependency to the laminas-authentication package