zend-crypt icon indicating copy to clipboard operation
zend-crypt copied to clipboard

Add Argon2i for password hashing

Open MatthiasKuehneEllerhold opened this issue 7 years ago • 6 comments

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

There are some open questions with this:

  1. Do we really want to have 1 new class for each algorithm PHP addes? "Argon2id" is just around the corner...

  2. 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
  1. 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)?

  2. 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?

  3. 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 avatar May 08 '18 11:05 MatthiasKuehneEllerhold

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: https://github.com/travis-ci/travis-ci/issues/8863

MatthiasKuehneEllerhold avatar May 09 '18 11:05 MatthiasKuehneEllerhold

Rebased it unto develop and changed the PR target.

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

MatthiasKuehneEllerhold avatar May 09 '18 11:05 MatthiasKuehneEllerhold

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.

Ocramius avatar May 09 '18 11:05 Ocramius

@MatthiasKuehneEllerhold I forgot to answer to your main questions:

  1. yes, this is the actual architecture of zend-crypt. Maybe, we will change it in the future.
  2. 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.
  3. 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.
  4. 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.
  5. 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.

ezimuel avatar Jul 12 '18 12:07 ezimuel

This repository has been closed and moved to laminas/laminas-crypt; a new issue has been opened at https://github.com/laminas/laminas-crypt/issues/1.

weierophinney avatar Dec 31 '19 21:12 weierophinney

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.

weierophinney avatar Dec 31 '19 21:12 weierophinney