Add support for scrypt-based password hashing.
AbandonedPublic
Actions

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

Severity	Location	Code	Message
Advice	src/infrastructure/util/password/PhabricatorScryptPasswordHasher.php:89	XHP16	TODO Comment

Unit

Tests Skipped

Build Status

Buildable 2391
Build 2395: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

thoughtpolice updated this revision to Diff 25002.Aug 29 2014, 5:47 PM

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)

thoughtpolice added reviewers: Blessed Reviewers, epriestley.

Herald edited edge metadata. · View Herald TranscriptAug 29 2014, 5:47 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B2391: Diff 25002.Aug 29 2014, 5:48 PM

(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.)

hvr added a subscriber: hvr.Sep 14 2014, 9:38 AM

avivey mentioned this in Community Resources.Oct 31 2014, 9:24 PM

• seschwar added a subscriber: • seschwar.Apr 23 2015, 9:55 PM

eadler added a subscriber: eadler.Apr 24 2015, 12:25 AM

johnny-bit added a subscriber: johnny-bit.Apr 25 2015, 9:48 PM

thoughtpolice mentioned this in T9212: Community Feedback: How should we handle free support?.Aug 23 2015, 5:49 PM

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.

epriestley added inline comments.Feb 27 2018, 10:41 PM

src/infrastructure/util/password/PhabricatorScryptPasswordHasher.php
89	This can be `phutil_hashes_are_identical()` nowadays.

thoughtpolice added inline comments.Feb 27 2018, 10:56 PM

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.

epriestley added inline comments.Feb 27 2018, 11:00 PM

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.