search

silverstripe / cwp-core

CWP basic compatibility module
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

Password length requirements no longer meet NZISM #52

Closed chillu closed 5 years ago

chillu commented 5 years ago

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

Context: https://github.com/silverstripe/silverstripe-framework/issues/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

https://github.com/silverstripe/cwp-core/commit/217ddec2e26ea839e1549425d7e07eb5517a5df7#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

chillu commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

NightJar commented 5 years ago

PR #53

NightJar commented 5 years ago

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

robbieaverill commented 5 years ago

Not fixed in CWP 2.2.0-rc1

NightJar commented 5 years ago

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

robbieaverill commented 5 years ago

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 commented 5 years ago

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

robbieaverill commented 5 years ago

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 commented 5 years ago

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

robbieaverill commented 5 years ago

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 https://github.com/silverstripe/recipe-plugin/issues/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 commented 5 years ago

Ok new pull requests are up, ready for discussion

robbieaverill commented 5 years ago

This probably need a docs update, will do that shortly

robbieaverill commented 5 years ago

Docs PR added https://github.com/silverstripe/silverstripe-framework/pull/8600

robbieaverill commented 5 years ago

I found a bug, new PR at https://github.com/silverstripe/recipe-core/pull/35

NightJar commented 5 years ago

🤔 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 commented 5 years ago

@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 commented 5 years ago

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 https://github.com/silverstripe/recipe-core/pull/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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

All PRs merged again =)