Silverstripe-framework: RFC: Deprecate password validation rules in favour of a strength rating

[robbieaverill]

Affected Version

4.x, 5.x

Description

SilverStripe supports a number of password strength rules, e.g. minimum and maximum length, and a number of regex based tests that must pass (e.g. lowercase, uppercase, digits, punctuation).

In a modern world, entropy and complexity can turn a passphrase (e.g. correct horse battery staple) into a more secure (and more memorable) password than something like oi(2hH!@!R#!x. The topic of using a passphrase instead of a password is not the point of this RFC.

We may deprecate these APIs in favour of using a library to measure entropy and complexity, giving a strength rating back, and replace the minimum requirements API with a "must be at least 50% rated" or something similar.

zxcvbn is a popular JavaScript library written by Dropbox that measures this, giving a rating of 0-4 for the strength of your password. It has a third-party PHP version.

I propose that we:

• Deprecate minimum length
• Deprecate maximum length
• Deprecate test patterns and minimum test score
• Introduce a third party library like this to measure entropy and complexity
• Introduce an API for the minimum strength rating
• Introduce this in a 4.x minor release via a new pluggable PasswordValidator
• Remove the deprecated code in 5.0

As a side note, the introduction of multi factor authentication may also reduce the need to have overly complex/strong passwords, since the second factor takes a lot of risk out of this equation. It may be acceptable for someone to use a weak password as long as they have MFA enabled.

cc @silverstripe/core-team

[dnsl48]

zxcvbn is a popular JavaScript library written by Dropbox

Just a side note that this particular one doesn't appear to be actively maintained anymore. The last PR was merged more than 2 years ago (Feb 11, 2017) and the last release has been made about that time too on Feb 7, 2017.

[robbieaverill]

Cool, will have a look for alternatives

[michalkleiner]

I think the RFC shouldn't be about deprecating/removing the lengths and patterns methods and replacing them with a strength library, it should rather be about deprecating them for being hardwired and replace with a password validator adaptor interface which, in a first instance, have the same features as the existing methods.
Then any kind of validation can be plugged in via this interface/service and it will eventually achieve the same as suggested here without causing too much pain for whoever wants to rely on the current behaviour.
If that's already the case behind the RFC, it's not obvious other than mentioning the pluggable validator (but nothing about being able to keep the current functionality).

Just remember how many hurdles SS had to skip to get to a stable versioned assets functionality. This is definitely smaller footprint change, but can be similarly difficult to deal with for some developers.

[sminnee]

The current PasswordValidator class was already intended to be used as the base class for a pluggable service. So I agree with @michalkleiner that we should leave the current behaviour as one option and implement strength as a second option.

We could introduce a new PasswordValidatorInterface and have two implementations , PasswordValidator (the current) and StrengthPasswordValidator.

These are accessed as an Injector service in Member::password_validator(). For BC we should keep the service name being PasswordValidator rather than PasswordValidatorInterface.

The only method the interface needs is:

    /**
     * @param String $password
     * @param Member $member
     * @return ValidationResult
     */
    public function validate($password, $member)

A strength-based password validator will need access to the "don't re-use historic passwords" feature of the current one. You could do this by breaking that out into a separate validator and providing a CompositePasswordValidator interface, or possibly by supplying the composite stuff as a helper class to the two validators we propose. A helper class is less flexible, but it's simpler, and that might be a better way to go for now.

[dhensby]

just a little pointer to https://github.com/dhensby/silverstripe-zxcvbn which does this already.

I would support moving to a strength based password validator over arbitrary rules.

---

My learnings from implementing zxcvbn was that a target score of 3 was about right but a pretty short password with a decent variance of letters/numbers/symbols would reach a 3. So a mix of minimum length and strength rating should be used together.

Therefore I'd urge caution when considering deprecating min-length.

[chillu]

Deprecate minimum length
Remove the deprecated code in 5.0

There's some regulatory environments which enforce different password standards. I'd say those are universal enough to keep in the framework. Our common point of reference is the NZ Common Web Platform, which needs to comply with the NZISM

The problem with custom "strength ratings" in third party libraries is that they'll never find their way into compliance frameworks like the NZISM. They have some additional rules like "don't reuse passwords", but in the end it needs to be expressed in a few sentences, not a bunch of code in a library.

I think we should push people towards strong passwords through libraries like that, but for that to happen we don't need to deprecate the existing logic. That push can happen through better defaults in new projects, and opt-in additions in new minor releases.

[robbieaverill]

Yep fair enough, would you see a strength-based validator in core as well as what's there at the moment, or perhaps a configurable option to enable it on the existing validator? I'm not 100% sold that zxcvbn is the right way to go at the moment to be honest, the PHP library looks a little under maintained at a glance, but it's still better than using only entropy calculations.

[chillu]

would you see a strength-based validator in core as well as what's there at the moment, or perhaps a configurable option to enable it on the existing validator?

I'd like this to be a module, we already do enough in core. It would probably be part of our "recommended default recipe", which likely also includes MFA. But that doesn't automatically mean it becomes a supported module which we're on the hook for forever (which would be the case if it's in "core")

[chillu]

From what I can tell, I have a 👍 from @maxime-rainville , and a 👎 from @robbieaverill @dhensby @dnsl48 @sminnee on replacing or deprecating the current validation rules.

And there's a general consensus that we should make this pluggable. Has anyone identified blockers with this? PasswordValidator doesn't have a PHP class interface, which makes it a bit wonky to re-implement, but its definitely injectable.

Does anyone feel strongly that this is high value enough to maintain as a parallel opt-in implementation in core, as opposed to allowing interested community devs to build this as a module? Unless that's the case, I'm inclined to close this RFC as rejected, since it doesn't require core adjustments (beyond the creation of a PHP interface?)

[robbieaverill]

It can be a satellite module =) let's close. Thanks everyone!