Feat/update hashing - Add password hasher #18

t0mmie · 2016-10-03T09:51:18Z

Jira https://angrybytes.atlassian.net/browse/ABC-564

Changelog

2.0.0

New Password Hasher

Version 2.0.0 comes with the new AngryBytes\Hash\Hasher\Password hasher. This hasher is intended to be used for
hashing passwords and other secure tokens. The hasher utilises PHP's native password hashing functions like
password_hash() and password_verify(), see Password Hashing.

The password hasher has been setup to use PHP's default cost and algorithm. The default cost can however be overwritten
via the setCost() method, like so:

$hasher = new \AngryBytes\Hash\Hasher\Password;
$hasher->setCost(15);

The password hasher offers a method to check if an existing hash needs to be rehashed. This can be the case if
the cost and/or algorithm of the hasher has changed. If this is the case you'll want to rehash and store your existing hashes.

Example

In this example we will check if an existing password hash needs to be rehashed, if so we'll hash the password value.
Note, in order to rehash the password you will need access to the plaintext value. It's advised to to this after
a user has successfully entered their password.

use AngryBytes\Hash\Hasher\Password;
use AngryBytes\Hash\Hash;

// Create a password hasher
$hasher = new Hash(
    new Password()
);

$password = 'plaintext password'
$passwordHash = 'password hash';

// Verify the password against the hash
if ($hasher->verify($password, $passwordHash)) {
    // Check if the hash needs to be rehashed
    if ($hasher->getHasher()->needsRehash($passwordHash)) {
        // Rehash the password
        $passwordHash = $hasher->hash($password);
    }
}

Refactored AngryBytes\Hash\HasherInterface

AngryBytes\Hash\HasherInterface::verify() is a new method for verify an existing hash. The method accepts the
following three arguments:

$data - The data that needs to be hashed.
$hash - The hash that needs to match the hashed value of $data.
$salt - The salt used for hashing.

Refactored AngryBytes\Hash\Hash

AngryBytes\Hash\Hash::construct() second argument ($salt) has become optional to accommodate for hashers that
handle their own salt, like AngryBytes\Hash\Hasher\Password.

AngryBytes\Hash\Hash::hash() and AngryBytes\Hash\Hash::shortHash() no longer accept any number of arguments but
only one. This single argument can however be of any type. All non-scalar types will be serialized before hashing.

AngryBytes\Hash\Hash::verify() is a new method that's available to validate a hash in a time-safe manner.
The method accepts the following two arguments:

$data - The data that needs to be hashed. This data can be of any type, all non-scalar types will be
serialized before hashing.
$hash - The hash that needs to match the hashed value of $data.

AngryBytes\Hash\Hash::matchesShortHash() is replaced by AngryBytes\Hash\Hash::verifyShortHash() this methods
accepts two arguments ($data and $hash) just like AngryBytes\Hash\Hash::verify().

Minor Changes

Scalar values passed to hash() and shortHash() are no longer serialized.
compare() Now uses PHP native hash_equals() function.
Fixed namespace issues for AngryBytes\Hash\HMAC.

Upgrading

Please refer to the upgrade notes.

Deprecated & Removed Components

AngryBytes\Hash\RandomString has been removed.

ivarb · 2016-10-03T10:08:39Z

Zie kapotte build. Zo te zien falen de unit tests.

ivarb · 2016-10-03T10:09:35Z

Wil je ook even kijken naar de composer.json en test file zodat hij PHP 5.6+ compatible wordt en de laatste unit test versie gebruikt.

t0mmie · 2016-10-03T12:18:28Z

@ivarb Composer op PHP 5.6+ gezet maar hij blijft testen voor 5.3, 5.4 en 5.5. Kan dat uit? :)

ivarb · 2016-10-04T14:27:17Z

src/AngryBytes/Hash/Hash.php

     **/
-    public function __construct(HasherInterface $hasher, $salt)
+    public function __construct(HasherInterface $hasher, $salt = false)


Liever null als default waarde voor optionele params.

ivarb · 2016-10-04T14:28:22Z

src/AngryBytes/Hash/Hash.php

-                'Provided salt "%s" is not long enough. A minimum length of 20 characters is required',
-                $salt
-            ));
+        if ($salt) {


if (is_string($salt) && strlen($salt) < 20))

....doh 🙈

ivarb · 2016-10-04T14:30:58Z

src/AngryBytes/Hash/Hash.php

     * @return string
     **/
-    public function shortHash()
+    public function shortHash($data)


Waarom doe je niet ...$data ipv $data? Dan kan je namelijk gewoon blijven doen shortHash(foo, bar, baz)?

Bij hash en verify zou dat inderdaad kunnen, shortHash zet die data direct door naar hash.

...$data en dan foo(...$data)

ivarb · 2016-10-04T14:32:24Z

src/AngryBytes/Hash/Hash.php

    {
-        return serialize(func_get_args());
+        if (is_scalar($data)) {


Boolean is ook scalar, weet niet of dat goed gaat aangezien het geen string is? Misschien moet je return (string) $data; doen voor de zekerheid. Dan wordt true -> 1 en blijft false ''

Hij geeft nu inderdaad een boolean terug, zal de typehint toevoegen

ivarb · 2016-10-04T14:33:20Z

src/AngryBytes/Hash/Hasher/Blowfish.php

-     * @return string
-     **/
+     * {@inheritDoc}
+     */
    public function hash($data, $salt)


Kan je $data niet beter $string noemen? Meer een stijl dingetje.

Klinkt wel logischer

laat maar staan hoor

ivarb · 2016-10-04T14:33:52Z

src/AngryBytes/Hash/HasherInterface.php

@@ -31,4 +31,14 @@
     * @return string
     **/
    public function hash($data, $salt);


Moet $salt hier niet default null zijn oid?

Ik denk het niet, voor de md5 hasher is de salt verplicht maar voor de password hasher weer niet.

ivarb · 2016-10-04T14:35:12Z

tests/AngryBytes/Hash/Test/PasswordTest.php

+use AngryBytes\Hash\Hasher\Password as PasswordHasher;
+
+/**
+ * PasswordTest


1e regel graag summary maken.

ivarb · 2016-10-04T14:53:01Z

src/AngryBytes/Hash/Hasher/Password.php

+            $options['salt'] = $salt;
+        }
+
+        $hash = password_hash($data, self::ALGORITHM, $options);


Waarom doe je niet PASSWORD_DEFAULT. Nu is het beetje dubbel?
En ik zou met die vergelijking gewoon positive check doen: if ($hash === false)

Ik had met het idee zitten spelen om dat ook instelbaar te maken maar ik denk dat de default beter is. Is nu inderdaad een beetje dubbel.

ivarb · 2016-10-04T14:53:22Z

@angrytom ik start de review vast, anders vergeet ik het weer :P

ivarb · 2016-10-04T14:59:16Z

Ik zou ook nog even de class docs nalopen, zie nu veel classes met classname als 1e class doc regel, waar dit beter summary kan zijn.

Qua gebruik moeten we misschien even kijken naar Hash vs Hasher. Was nooit zon fan van de naam Hash, maar dit is meer een helper om een Hasher heen. Je kan dus in de meeste gevallen ook een Hasher gebruiken zonder Hash denk ik.

ivarb · 2016-10-04T15:01:24Z

Als je in https://github.com/angryTom/hash/blob/feat/update-hashing/src/AngryBytes/Hash/HMAC.php#L97 gebruikt maakt van variadic functions kan de bestaande logica wat simpler:

public function hmac($sharedSecret, ...$args) {

ivarb · 2016-10-04T15:02:30Z

Ik zie ook nog veel (bestaande) public getter/setters. vraag me af of deze allemaal nog nodig zijn of /danwel public moeten zijn. Setter wordt vaak niet gebruikt omdat het via constructor gaat, en getter wordt vaak alleen intern gebruikt en hoeft niet echt voor public API beschikbaar te zijn.

ivarb · 2016-10-04T15:04:54Z

src/AngryBytes/Hash/Hash.php

-            ));
+        if ($salt) {
+            // Make sure it's of sufficient length
+            if (strlen($salt) < 20) {


Ik denk dat je salt max length ook moet checken tegen CRYPT_SALT_LENGTH, zie crypt() docs.

ivarb · 2016-10-04T15:07:15Z

src/AngryBytes/Hash/Hasher/Password.php

+     * @throws RuntimeException If the hashing fails
+     * @param string|bool $salt (optional) When omitted `password_hash()` will generate it's own salt
+     */
+    public function hash($data, $salt = false)


Dit lijkt niet compatible met de Interface?

ivarb · 2016-10-04T15:08:49Z

src/AngryBytes/Hash/Hasher/Password.php

+     * @param int|null $cost
+     * @return $this
+     */
+    public function setCost($cost)


Is het niet makkelijker dit ook als optionele contructor arg op te nemen zodat je dit tijdens aanmaken al kan zetten?

Aan de andere kant, het is een optionele override, misschien ook maar laten staan zo, aangezien het niet per see aangemoedigd wordt.

ivarb · 2016-10-04T15:09:39Z

src/AngryBytes/Hash/Hasher/Password.php

+     */
+    public function setCost($cost)
+    {
+        if (is_null($cost)) {


Waarom is dit toegestaan? Als het toegestaan is, dan moet je denk ik wel $this->cost overschrijven. De vraag is of we cost overschrijfbaar moeten maken nadat een hasher gemaakt is.

ivarb · 2016-10-04T15:11:27Z

src/AngryBytes/Hash/HasherInterface.php

+     * @param string $salt
+     * @return bool
+     */
+    public function verify($data, $hash, $salt);


Kunnen we hier niet, net zoals bij password_hash de 2e (en hier 3e) arg array $options = [] maken zodat daar meerdere opties (e.g. salt) in kunnen zonder dat bij uitbreidingen de API (interface) aangepast hoeft te worden?

ivarb · 2016-10-04T15:13:48Z

src/AngryBytes/Hash/Hash.php

     * @return string
     **/
-    private function getDataString()
+    private function getDataString($data)


Deze kan static zijn.

ivarb · 2016-10-04T15:14:29Z

src/AngryBytes/Hash/Hash.php

@@ -79,7 +74,7 @@ public function setHasher(HasherInterface $hasher)
    /**
     * Get the salt
     *
-     * @return string
+     * @return string|bool
     */
    public function getSalt()


Ik denk dat get/setSalt protected mogen zijn.

ivarb · 2016-10-04T15:14:59Z

Heb nu review gedaan (moet nog testen). Kan je iig kijken om nog een paar dingen op te schonen en wat gelijk te trekken?

Update HMAC lib for a slightly improved API

- port calls, preventing Hash::getHasher() - Update public API to prevent superfluous methods

t0mmie · 2016-10-05T15:18:58Z

@ivarb bijgewerkt

ivarb · 2016-10-06T06:33:35Z

@angrytom added some doc updates t0mmie#3

ivarb · 2016-10-06T06:43:32Z

src/AngryBytes/Hash/Hash.php

+     */
+    protected function setSalt($salt)
+    {
+        if (is_string($salt) && strlen($salt) < 20 && strlen($salt) > CRYPT_SALT_LENGTH) {


Dit moet een || ipv && zijn.

Some doc updates.

Feat/update hashing

Feat/update hashing - Doc updates

scrutinizer-notifier · 2016-10-06T07:58:24Z

The inspection completed: 1 Issues, 1 Patches

t0mmie added 15 commits September 29, 2016 14:58

Refactor hashing and add a new password hasher

3a24f2f

Fix HMAC namespace issue

d98fcb0

Remove random string generator

0ce09ab

Remove random string test

f1735f9

Fix unitests

588bb47

Add HMAC unittest

a71d1cc

Add bloffish verify tests

7093fef

Add password hasher unittest

1f0b235

Use default cost factor for password hasher

fb666db

Update password unit test for new cost

d67d4dc

Skip cost check if cost is null

c6ac261

Fix double serialization for short hash

66a54a9

Add changelog and upgrading files

4f0d8e4

Fix code example

9555885

Fix doc typo

6246962

Set php requirement and update phpunit

5c4653d

Update php version for travis

72d80b9

ivarb suggested changes Oct 4, 2016

View reviewed changes

ivarb reviewed Oct 4, 2016

View reviewed changes

Ivar van der Burg and others added 10 commits October 5, 2016 10:10

Update HMAC lib for a slightly improved API

25b0e1c

PR feedback

4b2b1d9

Merge pull request #1 from ivarb/feat/update-hashing

40d41ae

Update HMAC lib for a slightly improved API

Update hasher and Hash lib

b59b89b

- port calls, preventing Hash::getHasher() - Update public API to prevent superfluous methods

Update password hasher interface and public API

2007572

Check salt max length

14246b1

Resolve conflicts

a01bea1

Add hasher options, fix __call method

f3e8e73

Fix unit tests

093c434

Update changelog

27cd096

Ivar vd Burg added 2 commits October 6, 2016 08:31

Update method

3fef7ee

Docblock

da27d68

ivarb reviewed Oct 6, 2016

View reviewed changes

t0mmie and others added 7 commits October 6, 2016 08:55

Merge pull request #3 from ivarb/feat/update-hashing

53fbb87

Some doc updates.

Fix salt length test

82e7826

Add salt length unit test

2939d3f

Merge pull request #4 from ivarb/feat/update-hashing

6076129

Feat/update hashing

Update docs

ec7887c

Update readme

5b98eb3

Merge pull request #5 from ivarb/feat/update-hashing

62353d5

Feat/update hashing - Doc updates

ivarb approved these changes Oct 6, 2016

View reviewed changes

ivarb merged commit babfa56 into AngryBytes:master Oct 6, 2016

t0mmie deleted the feat/update-hashing branch October 10, 2016 07:35

Feat/update hashing - Add password hasher #18

Feat/update hashing - Add password hasher #18

Conversation

t0mmie commented Oct 3, 2016

Changelog

2.0.0

New Password Hasher

Example

Refactored AngryBytes\Hash\HasherInterface

Refactored AngryBytes\Hash\Hash

Minor Changes

Upgrading

Deprecated & Removed Components

ivarb commented Oct 3, 2016

ivarb commented Oct 3, 2016

t0mmie commented Oct 3, 2016

Choose a reason for hiding this comment

ivarb Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarb commented Oct 4, 2016

ivarb commented Oct 4, 2016

ivarb commented Oct 4, 2016

ivarb commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarb commented Oct 4, 2016

t0mmie commented Oct 5, 2016

ivarb commented Oct 6, 2016

Choose a reason for hiding this comment

scrutinizer-notifier commented Oct 6, 2016

ivarb Oct 4, 2016 •

edited

Loading