Add Argon2i for password hashing #58

MatthiasKuehneEllerhold · 2018-05-08T11:39:34Z

This PR adds an extra class for the argon2i password hash algorithm introduced with PHP7.2.

There are some open questions with this:

Do we really want to have 1 new class for each algorithm PHP addes? "Argon2id" is just around the corner...
PHP's password_verify() accepts currently both Bcrypt and Argon2i hashes. So basically you could either use the Bcrypt class or the Argon2i class to verify either hashes.
This makes it pretty easy to migrate users from Bcrypt to Argon2i:

Use the new Argon2i class as the dependency
old bcrypt passwords are still verified correctly
new password will be created with argon2i

Do we need a wrapper function for password_needs_rehash() ? This would mean we need some kind of inter-class upgrade path (from class Bcrypt to Argon2i in future php-versions).
Although we dont know WHY password_needs_rehash() returns false: is it because of the algorithm or the cost value(s)?
This class is marked as PHP7.2+ only (it throws an exception in the constructor). Do we want to provide fallbacks of some kind for older PHP Versions?
Other than the algorithm no other PHP7.2+ specific features were used in this class (e. g. scalar type hints and return types) because a syntax error is much more heavy and harder to catch than a constructor-exception (Pre 7.0).
Should type hints and return types get added because its a PHP7.2+ class anyway?

MatthiasKuehneEllerhold · 2018-05-09T11:14:25Z

I've refactored out the usage of the default constants but it seems like the PHP7.2 binaries from travis-ci have no argon2 support whatsoever, see: travis-ci/travis-ci#8863

Ocramius

@MatthiasKuehneEllerhold please base this on top of develop, which is where new features should land 👍

Ocramius · 2018-05-09T11:13:33Z

src/Password/Argon2i.php

+            );
+        }
+
+        if (! empty($options)) {


Reverse the conditional to return early

Ocramius · 2018-05-09T11:13:46Z

src/Password/Argon2i.php

+     * @param array|Traversable $options
+     * @throws Exception\InvalidArgumentException
+     */
+    public function __construct($options = [])


@ezimuel can we get rid of this kind of un-typed ctors?

Ocramius · 2018-05-09T11:18:21Z

src/Password/Argon2i.php

+     *
+     * @return Argon2i
+     */
+    public function setMemoryCost($memoryCost)


Wouldn't expose any of these setter/getters anymore

@Ocramius I think we still need this in order to be compliant with the previous implementations like Zend\Crypt\Password\Bcrypt, Zend\Crypt\Password\BcryptSha and Zend\Crypt\Password\Apache.
Even, because we can add some check on setter to verify that the parameter is correct with a detailed error message if not. IMHO, this is actually one of the advantages to use a wrapper class like Argon2i instead of the PHP bult-in function.

MatthiasKuehneEllerhold · 2018-05-09T11:25:32Z

Rebased it unto develop and changed the PR target.

I've tried to orient myself on the existing bcrypt class.

Ocramius · 2018-05-09T11:27:10Z

I've tried to orient myself on the existing bcrypt class.

No worries, I'm just suggesting to get rid of some cruft, especially on new classes. @ezimuel needs to check these, since he's the lead here.

ezimuel

@MatthiasKuehneEllerhold thanks for this PR and sorry for the delay in reviewing. I provide my feedbakcs, please check and let me know. Thanks again for your great work!

ezimuel · 2018-07-12T12:39:53Z

src/Password/Argon2i.php

+     *
+     * @return Argon2i
+     */
+    public function setMemoryCost($memoryCost)


@Ocramius I think we still need this in order to be compliant with the previous implementations like Zend\Crypt\Password\Bcrypt, Zend\Crypt\Password\BcryptSha and Zend\Crypt\Password\Apache.
Even, because we can add some check on setter to verify that the parameter is correct with a detailed error message if not. IMHO, this is actually one of the advantages to use a wrapper class like Argon2i instead of the PHP bult-in function.

ezimuel · 2018-07-12T12:43:22Z

src/Password/Argon2i.php

+     *
+     * @var int|null
+     */
+    protected $memoryCost;


I will suggest to assign the default PHP values for $memoryCost, $timeCost and $threads. Using the constants:

PASSWORD_ARGON2_DEFAULT_MEMORY_COST PASSWORD_ARGON2_DEFAULT_TIME_COST PASSWORD_ARGON2_DEFAULT_THREADS

Using the constants on PHP < 7.2 leads to an "Undefined constant" error even if you just autoload the class. (See the <7.2 Unit-Tests).

ezimuel · 2018-07-12T12:47:23Z

src/Password/Argon2i.php

+    {
+        $options = [];
+
+        if ($this->memoryCost !== null) {


If we use the default PHP values, we can omit this check with null values and always pass the $options to password_hash.

ezimuel · 2018-07-12T12:49:43Z

src/Password/Argon2i.php

+     *
+     * @return Argon2i
+     */
+    public function setTimeCost($timeCost)


We should check the value of $timeCost and verify that is allowed values for $options in password_hash(). As commented before, we should offer a safe way to use the Argon2i parameter with a detailed message in case of invalid values.

ezimuel · 2018-07-12T12:58:00Z

@MatthiasKuehneEllerhold I forgot to answer to your main questions:

yes, this is the actual architecture of zend-crypt. Maybe, we will change it in the future.
yes, you right the password_verify is used by Bcrypt and Argon2i but this is ok and actually good for portability purpose, as you already noticed.
no, I don't think we need a wrapper for password_needs_rehash() function of PHP. The usage of this function is simple and people can use directly it. We don't see any advantages for a wrapper class here.
no, I don't think we need to support Argon2i for PHP < 7.2. I implemented, years ago, Scrypt directly in PHP but the performance are not so good as C, of course.
no, at the moment we don't use type hint for zend-crypt for backwards compatibility, we are still supporting PHP 5.6. That said, we will change it soon with a new major release.

weierophinney · 2019-12-31T21:21:23Z

This repository has been closed and moved to laminas/laminas-crypt; a new issue has been opened at laminas/laminas-crypt#1.

weierophinney · 2019-12-31T21:21:35Z

This repository has been moved to laminas/laminas-crypt. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone laminas/laminas-crypt to another directory.
Copy the files from the second bullet point to the clone of laminas/laminas-crypt.
In your clone of laminas/laminas-crypt, commit the files, push to your fork, and open the new PR.
We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Ocramius suggested changes May 9, 2018

View reviewed changes

MatthiasKuehneEllerhold changed the base branch from master to develop May 9, 2018 11:22

Add class for Argon2i

e9e52d7

ezimuel suggested changes Jul 12, 2018

View reviewed changes

Validate parameters on setter instead of during use

013d10b

weierophinney mentioned this pull request Dec 31, 2019

Add Argon2i for password hashing laminas/laminas-crypt#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Argon2i for password hashing #58

Add Argon2i for password hashing #58

MatthiasKuehneEllerhold commented May 8, 2018 •

edited

Loading

MatthiasKuehneEllerhold commented May 9, 2018

Ocramius left a comment

Ocramius May 9, 2018

MatthiasKuehneEllerhold May 9, 2018

Ocramius May 9, 2018

Ocramius May 9, 2018

ezimuel Jul 12, 2018

MatthiasKuehneEllerhold commented May 9, 2018

Ocramius commented May 9, 2018

ezimuel left a comment

ezimuel Jul 12, 2018

ezimuel Jul 12, 2018

MatthiasKuehneEllerhold Jul 17, 2018

ezimuel Jul 12, 2018

ezimuel Jul 12, 2018

ezimuel commented Jul 12, 2018

weierophinney commented Dec 31, 2019

weierophinney commented Dec 31, 2019

Add Argon2i for password hashing #58

Are you sure you want to change the base?

Add Argon2i for password hashing #58

Conversation

MatthiasKuehneEllerhold commented May 8, 2018 • edited Loading

MatthiasKuehneEllerhold commented May 9, 2018

Ocramius left a 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

MatthiasKuehneEllerhold commented May 9, 2018

Ocramius commented May 9, 2018

ezimuel left a 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

ezimuel commented Jul 12, 2018

weierophinney commented Dec 31, 2019

weierophinney commented Dec 31, 2019

MatthiasKuehneEllerhold commented May 8, 2018 •

edited

Loading