silverstripe / cwp-installer

CWP project template
BSD 3-Clause "New" or "Revised" License
0 stars 5 forks source link

Include MFA modules #35

Closed brynwhyman closed 4 years ago

brynwhyman commented 4 years ago

Overview

With the next minor release of CWP, the multi-factor authentication module suite should be included for all new sites built with the cwp-installer.

What's in the MFA module suite?

Acceptance Criteria

Notes

Pull Requests

chillu commented 4 years ago

@brynwhyman Is this a duplicate of the Epic: MFA is in CWP by default?

brynwhyman commented 4 years ago

Is this a duplicate of the Epic: MFA is in CWP by default?

@chillu no, https://github.com/silverstripeltd/product-issues/issues/153 was created as an internal epic to capture all related work under one card.

brynwhyman commented 4 years ago

I'd like to suggest that we leave out the Security token option as what's included in the CWP module by default. There's a number of edge cases and potential for havoc without the site owner and development agency taking the time to understand these before requesting this option, including:

There's another open issue suggesting to disable the security token functionality if the subsites module is installed, but that's not covering all the edge cases. I believe something like this should still happen, but it shouldn't block sites getting access to the much more popular and accessible MFA method, TOTP.

brynwhyman commented 4 years ago

@Cheddam has noted that there's at least one issue in the login-forms module that we'll want to look at as part of this issue. Will add to our internal epic.

brynwhyman commented 4 years ago

We're talking about a few different options to actually make this happen. Should we:

Or:

chillu commented 4 years ago

Bryn mentioned internally that the current inclination is to leave it out of the recipes, which would introduce silverstripe/mfa and silverstripe/login-forms on minor upgrades. Reasons:

So in summary, I recommend that we use the third option described here:

add it to cwp-installer (the upgrade instructions that's provided in each release references the modules in the cwp-installer). Noting that this will probably only serve new site builds

Also, the installer would only include the TOTP MFA method, not the WebAuthn method since that requires a bit more thought by developers (subsites, multi env usage). This should be outlined in the upgrade docs though, to ensure that WebAuthn is actually considered. It's far more secure than TOTP (lower phishing potential, domain verification built in)

chillu commented 4 years ago

I'm also advocating for the same approach in core: Add this to the installer rather than recipes. https://github.com/silverstripe/silverstripe-installer/issues/280

brynwhyman commented 4 years ago

Thanks for the comments @chillu, I agree that pushing it through the CWP-installer is still a good thing to do.

Do you have any ideas on how to ensure that Developers get the TOTP encryption key .env variable? It's on the CWP platform by default so a deployed site would be covered, but can we only rely on documentation to ensure Developers are aware of this?

brynwhyman commented 4 years ago

I've moved this issue to the cwp-installer repository and update the description to focus on getting the modules added to this repo and some related changelog notes.

Other points around futher documentation will be captured in this issue: https://github.com/silverstripe/cwp/issues/269

chillu commented 4 years ago

Do you have any ideas on how to ensure that Developers get the TOTP encryption key .env variable?

Within the scope of CWP, it's a default on Revera, and hopefully soon a default on AWS. Until that point, I think we need to add something to https://www.cwp.govt.nz/developer-docs/en/2/getting_started/

brynwhyman commented 4 years ago

Alright, we'll have that covered in https://github.com/silverstripe/cwp/issues/269

brynwhyman commented 4 years ago

The change log for the associated CWP release includes clear guidance on:

I think this AC can be removed, it's already covered in: https://github.com/silverstripe/cwp/issues/269