Password length requirements no longer meet NZISM

[chillu]

We require 8 chars min length, but NZISM requires 10 since at least v2.4 (Nov 2015) - see archive.

Context: silverstripe/silverstripe-framework#8522. Tracking this separately for CWP, because we might need to handle this faster or in a different way. In open source, it's a matter of opinion. In CWP, it's a compliance issue. Note that agencies can choose to increase those requirements on their own instances, but is about setting compliant defaults.

https://www.nzism.gcsb.govt.nz/ism-document/#1858

Agencies SHOULD implement a password policy enforcing either:
a minimum password length o
f 16 characters with no complexity requirement; or
a minimum password length of ten characters, consisting of at least three of the following character sets:
lowercase characters (a-z);
uppercase characters (A-Z);
digits (0-9); and
punctuation and special characters.

This also aligns with OWASP guidance: https://www.owasp.org/index.php/Authentication_Cheat_Sheet#Password_Length

217ddec#diff-8a7315557cd9f672e36f7e8f0ce2c29e removed the password requirements from CWP, because they're now part of recipe-core: https://github.com/silverstripe/recipe-core/blob/4/app/_config.php#L8

Pull requests

• move PasswordValidator configuration into _config #53
• FIX Move password complexity requirements into framework recipe-core#34
• FIX Move password complexity requirements into framework silverstripe-framework#8587
• FIX Run password config after core configuration #59
• DOCS Update documentation for password validation rule configuration silverstripe-framework#8600
• FIX Ensure that the PasswordValidator is registered with Injector recipe-core#35
• FIX Ensure that tests setting passwords have stubbed configuration silverstripe-framework#8604

[chillu]

We might need to make PasswordValidator configurable via YAML instead of using Member::set_password_validator($validator) to allow for this. The class already uses config(), so it's just a matter of NOT using a custom instance and falling back to YAML.

[sminnee]

We’d also want to think about the UX for if someone logs in with a no-longer-compliant password. Do we force a reset?

[ScopeyNZ]

It's not really core team - We should try to get this in for CWP 2.2. We can just remove the milestone.

[NightJar]

PR #53

[NightJar]

I've raised your comment @sminnee as a separate issue - #54

[robbieaverill]

Not fixed in CWP 2.2.0-rc1

[NightJar]

🤔 does the test pass on kitchen sink?
Perhaps I should add some functional tests to the test suite somehow...

[robbieaverill]

The PR was merged into master, which is probably why this is "not fixed". I'm just setting up a local environment to verify that before I cherry pick it into 2.2

[NightJar]

oh, whoops! That's my bad with the targeting.

[robbieaverill]

https://github.com/silverstripe/recipe-core/blob/4.3/app/_config.php this file is coming through because we don't have an equivalent file to override it with. The config in that file is taking precedence over the YAML in #53

Patched in cwp-recipe-core and I've cherry-picked #53 back into the 2.2 branch. All working now!

[robbieaverill]

Sorry, that recipe patch hasn't fixed it yet. The silverstripe/recipe-core file is still being copied in first and taking precedence.

[robbieaverill]

The problem is that this file is not extensible: https://github.com/silverstripe/recipe-core/blob/4.3/app/_config.php

It gets loaded first by composer, and the file gets copied into app/_config.php first, then all subsequent recipes will ignore their own _config.php files because it already exists. The logic in this file is executed after the config manifests are built, and after all module _config.php files, so we can't override it at the moment.

I thought we could move the password requirements from a PHP file into YAML config, but since it'll exist in user project code it'd also get loaded after modules, so the CWP config would need to say After: '#myprojectpasswords' or something, and since that file is in user code it could have its name changed easily.

I've raised an issue at silverstripe/recipe-plugin#12 since we don't have a way of controlling the priority of project files between recipes.

TBH I think the only way we can fix this is to move the password requirements out of user code (via a recipe) and put them into default YAML config in framework. Then user code can extend if they want to (with config), as well as CWP. I'm going to make PRs to do this and we can decide later if we want to.

[robbieaverill]

Ok new pull requests are up, ready for discussion

[robbieaverill]

This probably need a docs update, will do that shortly

[robbieaverill]

Docs PR added silverstripe/silverstripe-framework#8600

[robbieaverill]

I found a bug, new PR at silverstripe/recipe-core#35

[NightJar]

🤔 I would have thought that framework using injector if one is not set should cover that...
I guess if it sets one by default though, it'd be too late for relying on that.

[robbieaverill]

@NightJar yeah, but Injector doesn't has() one until one has been instantiated via injector. I've added that back in in silverstripe/recipe-core#35

[NightJar]

True, but here it sets config so it does has one 🤔

To be clear I'm not particularly caring about reintroducing the setting method in silverstripe/recipe-core#35 - I'm just puzzled as to why it's necessary - it seems like it shouldn't be (if nothing else replaces the defined service linked above)... I wonder if there's anything deeper at play.

---

Oh derp - sorry getting confused between cwp-recipe-core and recipe-core
In that case, would it perhaps not be easier to introduce yaml such as in cwp-core here rather than creating a singleton instance at boot time?

...

Like this? How come this doesn't work? :/

[robbieaverill]

@NightJar injector has() won’t return true with config only, it needs to have an instance created already for it to return true. Config only is not enough

[robbieaverill]

Here's a unit test to show what I mean. Framework has the passwords.yml config in it.

public function testInjectorHas()
{
    $this->assertFalse(Injector::inst()->has(PasswordValidator::class));

    Member::set_password_validator(PasswordValidator::singleton());
    $this->assertTrue(Injector::inst()->has(PasswordValidator::class));
}

[NightJar]

@NightJar injector has() won’t return true with config only, it needs to have an instance created already for it to return true. Config only is not enough

If this is true then there's a bug in framework.
edit: tl;dr read the last line :>

has calls getServiceName
getServiceName calls getServiceSpec
getServiceSpec will return true if there is a registered spec, not an instance.

If there is not an already loaded spec, then getServiceSpec performs a lazy load which involves:
configLocator->locateConfigFor fetches the spec
load registers the spec - and replaces the instance in memory if and only if there is already an instance in memory.

So in both cases has should return true if a spec (i.e. config) is loaded, and not be concerned with instances.

So for the configuration in passwords.yml to not be loaded at the time the assertFalse is run, something must surely be wrong elsewhere.

---

edit: Say like... not being included in 4.3.0-rc1 (which CWP 2.2.0-rc1 relies on - which is the context for this testing)

[robbieaverill]

All PRs merged again =)