Skip to content
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

[v3 dev] needsRehash always returns true #38

Closed
frederikbosch opened this issue Jul 15, 2016 · 8 comments
Closed

[v3 dev] needsRehash always returns true #38

frederikbosch opened this issue Jul 15, 2016 · 8 comments

Comments

@frederikbosch
Copy link

frederikbosch commented Jul 15, 2016

I would asume that when I generate a new hash with Password::hash that needsRehash would return false on the generated hash. But as so it seems, it always returns true.

@paragonie-scott
Copy link
Member

Oh no, how did I mess that up?

Will fix in 2.1 immediately.

@paragonie-scott
Copy link
Member

Hmm, what version are you running?

    public function testRehash()
    {
        $key = new EncryptionKey(\str_repeat('A', 32));
        $legacyHash = '3142010064c0c42347b248372d9605621bd6e56e6ace8d2c6f6a3cf3d1a37a40' .
            '3f031b5be025f00763a92ffb47281065419663e972b1a8faa08ae34bd9fdb35b2ca7727f41' .
            'ca8edc75293d8f3bf12604ff4188d71473b605d48d1e378388465c6e4c733cae5f89802ebb' .
            '79ec6532b430a4799e545956113f116fa705e3ed2d7b17bb6dbf435f36a0f50dcb541adb82' .
            'a83f6d01ae66b2f4d46540161ba6cc37dbd0e870aed8334cb71f8162a9e7e7974396bdb1bc' .
            '4da5099423820b870e39a3ffe5';

        $this->assertTrue(Password::needsRehash($legacyHash, $key));

        $hash = Password::hash('test password', $key);
        $this->assertFalse(Password::needsRehash($hash, $key));
    }

This test is passing.

@paragonie-scott
Copy link
Member

Oh, I see. You're using the master branch? Please don't, the API changes aren't final.

@frederikbosch
Copy link
Author

I am on master, I am binding one of my libraries to your upcoming version 3.

@frederikbosch
Copy link
Author

frederikbosch commented Jul 15, 2016

I won't use the API in production until version 3 is released. But since you checked al your boxes for version 3, I thought why bind to version 2 when version 3 is almost there. I think it is a good idea to include a test for needsRehash. However, I do not see why it fails now, too little experience with the lib. Otherwise I would have created a PR for it.

@frederikbosch frederikbosch changed the title needsRehash always returns true [v3 dev] needsRehash always returns true Jul 15, 2016
@paragonie-scott
Copy link
Member

It's caused by the change from hex encoding to base64urlsafe. I'm working on the fix right now.

But since you checked al your boxes for version 3, I thought why bind to version 2 when version 3 is almost there.

Ah, I'm sorry that checklist caused confusion. We implemented the brainstormed list items, but the milestone hasn't been laid out quite yet. The changes I did make might not even be final (for example, we might only use HiddenString for plaintext and for passwords, not for ciphertext). I can't even estimate the ETA right now, except "certainly before 2017-01-01".

Version 2.x will continue to be maintained until CMS Airship v1 is EOL'd (slightly under three years). If you're shipping software that requires Halite, you'll probably be better off using 2.1 since it's already available.

Version 3's main benefits as laid out are:

  • Less data exposed in stack traces
  • Smaller encoded ciphertext output (33% larger than raw binary instead of 100% larger than raw binary)
  • The ability to specify any RFC 4648 encoding scheme for string encryption operations
  • No more scrypt support

None of these are a security advantage over version 2, and the HiddenString requirement may prove too cumbersome for most use-cases. (I'll probably change the logic to make it allow __toString() by default, for usability.)

@frederikbosch
Copy link
Author

@paragonie-scott Thanks for clearing this. I will switch to version 2 then. Thanks for the great library!

@paragonie-scott
Copy link
Member

66bef16 fixed this, thanks for reporting it :)

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

No branches or pull requests

2 participants