PHP7 - password_hash(): Use of the 'salt' option to password_hash is deprecated

[garethellis36]

Warning: password_hash(): Use of the 'salt' option to password_hash is deprecated

This is emitted from lines 51-54 of UpgradeDecorator::isValid().

        if ($isValid === true) {
            $passwordHash = password_hash($password, PASSWORD_DEFAULT, array(
                'cost' => 4,
                'salt' => 'CostAndSaltForceRehash',
            ));
        }

I'm assuming that this is just a temporary hash that can then be re-hashed in PasswordValidator again. And I'm guessing that you have to make sure you have a reasonably unique salt/cost or password_needs_rehash will return false. Not sure what to suggest.

Could the salt not be removed here? If it produces a hash which matches the options set for PasswordValidator then all will be good; otherwise it will just be re-hashed again by PasswordValidator. Right?

[garethellis36]

I ran the test suite under PHP7 and removing the salt from UpgradeDecorator removes some issues. However, because the same salt is used in UpgradeDecoratorTest in order to produce a hash that can be compared with $this->assertEquals, some tests still fail because of the above warning.

Of course, removing the salt here as well removes all warnings but the tests fail because without the salt, different hashes are produced. I ran into this same issue while writing an integration test in PHP 5.6 earlier today. In the end I just had to test that the hash had changed rather than testing the exact output, and trust that password_hash() was doing what it was supposed to. Unless there's another way?

[jeremykendall]

You're right that the salt should be remove. It's there to force a rehash, but there's probably another, better way.

Without looking at the code (I'm at a conference and still working), I'll bet we could use assertRegExp might be a good choice to validate that a result has been rehashed with the proper algo and cost.

If that gets you where you need to go, awesome. Otherwise I'll look as soon as I can.

[jeremykendall]

One problem here is I want the library to support PHP >= 5.3.7. Removing the salt to support PHP >= 7 is necessary, but that breaks the library for earlier versions of PHP. I wonder if having two different versions of the library is the right way to go? Hmmm.

[garethellis36]

For UpgradeDecoratorTest - presumably you don't have to test the actual hash that is produced, we just need to test that it is a valid hash as produced by password_hash() given the options set using setOptions. Thinking aloud, couldn't you just ensure that the options are the same in the test and then assert that password_needs_rehash() returns false? I would have thought that if it returns false then it will be a valid password according to the input params.

Regarding legacy support: does the password_compat library require a salt then? I'm not entirely familiar with it. I suppose one option would be to maintain a version 3.x branch which supports up to PHP 5.6, and have all future versions onwards only support PHP 5.5 onwards?

The only other alternative would be to suppress the warning with the @ symbol. Eww 😆

[garethellis36]

Right, I've had a go at implementing what I've suggested above.

Commits (not submitted PR yet):
The work: garethellis36@58075ce
Adding PHP7 to Travis: garethellis36@e4a8639

I've hooked up my fork to Travis and all builds are passing (including PHP 5.3) except PHP 7. There, the tests are passing but there's some kind of segfault after:
https://travis-ci.org/garethellis36/password-validator/jobs/100884781

This is the first time I've used Travis so I've no idea what this means.

[jeremykendall]

Blerg. That has to do with a code coverage dependency.

Change this line and replace travis.phpunit.xml with phpunit.dist.xml and remove all the following lines. That should fix the PHP 7 issues.

[jeremykendall]

@garethellis36 Added a comment on your commit.

[garethellis36]

Slightly tweaked approach following discussion on previous commit:
garethellis36@a0e97b6
garethellis36@c422df0

[garethellis36]

garethellis36@f3d6b8f
This broke all builds in Travis 😆

[jeremykendall]

Geez. Bad advice! I should have some time to look this afternoon. Between the two of us we should get it worked out.

[garethellis36]

I'm going to submit this as a PR so that the commits and discussion are together.

[jeremykendall]

Fixed in #21.