Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incude MFA module in new projects via installer #280

Open
2 tasks
chillu opened this issue Jun 3, 2020 · 6 comments
Open
2 tasks

Incude MFA module in new projects via installer #280

chillu opened this issue Jun 3, 2020 · 6 comments

Comments

@chillu
Copy link
Member

chillu commented Jun 3, 2020

The upcoming 4.6 release already includes silverstripe/loginforms via the installer. This way we can ensure the functionality finds its way into new projects, without causing disruption in existing projects (through an inclusion in recipes). I think we should do the same with the silverstripe/mfa and silverstripe/totp-authenticator modules. According to the MFA module readme, this should result in TOTP being enabled by default, but optional for users. The aim here is to provide more security choices by default for users. Adding MFA to new projects is close to pointless if it relies on a CMS admin caring enough about this problem space to enable the feature through a checkbox somewhere. Instead, CMS admins should be empowered to disable MFA, or require it. But the right defaults need to be in place without this interaction.

silverstripe/webauthn-authenticator should not be installed by default due to complexities around copying logins between environments, multi-domain usage, etc.

When doing this, we should also update docs in https://docs.silverstripe.org/en/4/developer_guides/security/

Since we've already released 4.6.0-beta1 of the installer, this would need to be targeted at 4.7.0.

This is a very similar discussion to the inclusion in the cwp/installer.

Pull Requests

chillu added a commit to open-sausages/silverstripe-installer that referenced this issue Jun 3, 2020
chillu added a commit to open-sausages/silverstripe-installer that referenced this issue Jun 3, 2020
chillu added a commit to open-sausages/silverstripe-framework that referenced this issue Jun 3, 2020
@robbieaverill
Copy link
Contributor

I think you'd still have to define an encryption key for the TOTP module before it would be enabled by default. We've talked in the past about having an app key that could be used for that, but that might be a blocker for automatically enabling this out of the box.

@chillu
Copy link
Member Author

chillu commented Jun 3, 2020

Argh you're right, the screens kick in when it's not configured:

image

And then you get into the rabbithole of the Encryption API RFC.

Option 1: Change MFA login flow to not show when no authenticators are available (configured correctly). This is only marginally better than not shipping MFA by default, you're requiring devs to perform one task (generate secret) rather than two tasks (install mfa and generate secret)

Option 2: Generate key during composer create-project (which is the only supported way to get a new project going now that we've removed support for the PHP installer). This would write to .env. Very similar to the Laravel approach (via a composer hook and a command). Ideally this would be an "application key" rather than something specific to MFA though. This auto-generating won't happen when you transition to other environments (Dev > UAT > Prod), but that's the same issue we currently have already. I don't think we'd need to add openssl to our server requirements to achieve this, random_bytes() should be sufficient.

Option 3: Allow MFA operation with an unsecure default key, and strongly advise to generate one during project setup. I think even with a default key, we're delivering 80% of the security improvement that MFA provides. It would take a reasonably targeted attacker to know about default keys in Silverstripe and generate TOTP codes based on it. We could add a warning in the admin/security section about this?

@chillu
Copy link
Member Author

chillu commented Jun 3, 2020

Another reason not to include webauthn in the installer: It avoids adding GMP as a required PHP extension.

@brynwhyman
Copy link

I like option 2.

Although, maybe another one:

Option 4: Provide clear and easily searchable documentation (perhaps also through composer messaging?) advising how to complete the process for setting up TOTP after a project has been created. At the point of seeing this screen, the user is still able to skip the MFA flow and proceed to login.

@chillu
Copy link
Member Author

chillu commented Jun 3, 2020

Yeah good point about Option 4 - we could add something to the screen I posted above, e.g. "are you a developer? Here's how you fix this "

@chillu
Copy link
Member Author

chillu commented Jun 3, 2020

With both Option 3 and Option 4 though, the developer experience is a bit fraught. "I've just installed the project, now I need to run an arcane command in my terminal and copy/paste some output in a file I don't understand". I've tried to make this a bit more universal through silverstripe/silverstripe-totp-authenticator#55, at least you don't have to worry about having the openssl command available.

Option 2 would require us to automate this process, but Option 3 and 4 would still benefit from a task that's the equivalent of Laravel's laravel generate:key. That looks pretty straightforward, unless we think that it requires us to define an encryption API (rather than using random_bytes() directly in this task).

chillu added a commit to silverstripe/silverstripe-framework that referenced this issue Aug 20, 2020
chillu added a commit to open-sausages/silverstripe-installer that referenced this issue Aug 20, 2020
chillu added a commit to silverstripe/silverstripe-framework that referenced this issue Aug 20, 2020
Underlying feature isn't merged yet,
see silverstripe/silverstripe-installer#280

Revert "Update docs/en/02_Developer_Guides/09_Security/03_Authentication.md"

This reverts commit 72a02a3.

Revert "Update docs/en/02_Developer_Guides/09_Security/03_Authentication.md"

This reverts commit c54f8e4.

Revert "DOCS MFA authentication"

This reverts commit 5fe5833.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants