Page MenuHomePhabricator

Add support for scrypt-based password hashing.
AbandonedPublic

Authored by thoughtpolice on Aug 29 2014, 5:47 PM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

This adds support for the 'scrypt' algorithm to the password hashing
infrastructure. This requires the 'scrypt' PHP extension from PECL to
be available (and specified in php.ini).

The extension only computes the hash of a string given the required
inputs. As a result, we store a 'serialized' hash in the database,
including the parameters used for hashing, and the salt as well.

The default login settings reflect the recommended 'interactive'
scrypt parameters: 16MB of memory per hash computation, which should
be quite safe.

scrypt is currently rated as a 'Great' hasher with a strength
of 4.0 (well above bcrypt, the next best choice).

Test Plan

Plan 1: Start a server without scrypt extension, enable user/pass
authentication. Check 'Password' Settings page, see that 'bcrypt' or
'iterated-md5' is the current hashing algorithm. Enable scrypt
extension in php.ini and restarted server. Was prompted to upgrade
password, as scrypt is now available.

Plan 2: Start a server with scrypt extension enabled in php.ini,
enable user/pass authentication. Check 'Password' Settings page, and
scrypt is already selected as the default.

Diff Detail

Repository
rP Phabricator
Branch
scrypt
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/infrastructure/util/password/PhabricatorScryptPasswordHasher.php:89XHP16TODO Comment
Unit
Tests Skipped
Build Status
Buildable 2391
Build 2395: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

thoughtpolice retitled this revision from to Add support for scrypt-based password hashing..
thoughtpolice updated this object.
thoughtpolice edited the test plan for this revision. (Show Details)

(This looks good to me, but I want to fix the login experience in the presence of a missing hasher before we upstream it, and I haven't had a chance to look at that yet.)

I'm abandoning this revision; with the advent of PHP 7.2 (and Phabricator's ongoing support for PHP 7.x, now), password_hash can now use the new "Argon2I" algorithm when given a proper $algo parameter. See [The password_hash entry on php.net](http://php.net/manual/en/function.password-hash.php#refsect1-function.password-hash-description). From the PHP/admin side, this integration will be easier to use and install for Phabricator admins/maintainers (your distro/package manager will probably enable it for you) -- and from the Phabricator side, the code will be dramatically simpler and easier to reason about than this integration (e.g. no need to manually serialize parameters and validate them.)

For background, Argon2I is a cryptographic password function, reviewed by an independent set of experts in the 2013-2015 Password Hashing Competition, against many other candidates (including a more powerful, cleaned-up and robust variant of scrypt, called yescrypt). Argon2i was chosen as the final winner for modern, secure password hashing. As of Specification v1.3, the Argon2 design (and libraries) should remain stable for the foreseeable future.

Note that in lieu of this diff, anyone who wants scrypt support, or is using it already, can continue to use my libphutil-scrypt library and the notes above to install it (if you weren't already, for some reason). And of course in the future/now you can migrate away from this.

It is possible that I might submit a diff upstream to enable Argon2i support iff your PHP version is >= 7.2 sometime soon, but I'm more likely to craft it into an extension -- the PhabricatorPasswordHasher interface has been very stable for a long time, so it wouldn't be a huge burden/suffer from major API shifts. Plus, I don't think the Phacility cluster is using such a bleeding edge PHP version (or will be for quite a while), and as a result such code will effectively be dead weight that won't be tested. Which I'm know Evan doesn't care for. :)

Plus, considering this is all using standard password_hash now, the end result is going to look like a copy and paste of the bcrypt implementation, with, like, 15 characters added/removed, in the long run. Not much to add later when necessary.

src/infrastructure/util/password/PhabricatorScryptPasswordHasher.php
89

This can be phutil_hashes_are_identical() nowadays.

src/infrastructure/util/password/PhabricatorScryptPasswordHasher.php
89

TIL (via function docs) about "type juggling" values in PHP through abuse of behavior when parsing exponents? Love being horrified while drinking coffee.

src/infrastructure/util/password/PhabricatorScryptPasswordHasher.php
89

Personally, T6861 -- where lists containing only distinct values are sorted differently depending on input order -- is my favorite example of PHP being a very special language.