Page MenuHomePhabricator

Convert user MFA factors to point at configurable "MFA Providers", not raw "MFA Factors"
ClosedPublic

Authored by epriestley on Jan 15 2019, 9:19 PM.

Details

Summary

Ref T13222. Users configure "Factor Configs", which say "I have an entry on my phone for TOTP secret key XYZ".

Currently, these point at raw implementations -- always "TOTP" in practice.

To support configuring available MFA types (like "no MFA") and adding MFA types that need some options set (like "Duo", which needs API keys), bind "Factor Configs" to a "Factor Provider" instead.

In the future, several "Factors" will be available (TOTP, SMS, Duo, Postal Mail, ...). Administrators configure zero or more "MFA Providers" they want to use (e.g., "Duo" + here's my API key). Then users can add configs for these providers (e.g., "here's my Duo account").

Upshot:

  • Factor: a PHP subclass, implements the technical details of a type of MFA factor (TOTP, SMS, Duo, etc).
  • FactorProvider: a storage object, owned by administrators, configuration of a Factor that says "this should be available on this install", plus provides API keys, a human-readable name, etc.
  • FactorConfig: a storage object, owned by a user, says "I have a factor for provider X on my phone/whatever with secret key Q / my duo account is X / my address is Y".

Couple of things not covered here:

  • Statuses for providers ("Disabled", "Deprecated") don't do anything yet, but you can't edit them anyway.
  • Some bin/auth tools need to be updated.
  • When no providers are configured, the MFA panel should probably vanish.
  • Documentation.
Test Plan
  • Ran migration with providers, saw configs point at the first provider.
  • Ran migration without providers, saw a provider created and configs pointed at it.
  • Added/removed factors and providers. Passed MFA gates. Spot-checked database for general sanity.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 15 2019, 9:19 PM
Owners added a subscriber: Restricted Owners Package.Jan 15 2019, 9:19 PM
epriestley requested review of this revision.Jan 15 2019, 9:20 PM
epriestley updated this revision to Diff 47678.Jan 16 2019, 2:46 AM
  • Remove debugging code.
amckinley accepted this revision.Jan 16 2019, 10:00 PM

This all probably falls into "Couple of things not covered here:", so feel free to say "yep yep that's all coming in a subsequent diff" instead of typing it out again, but...

What's the behavior when the last MFA provider I have configured on my account gets disabled and security.require-multi-factor-auth is enabled? I feel like manipulating MFA providers should be a command line activity. Alternately, the UI could detect that the provider you're trying to disable has users with configured factors and refuse to let you disable it (and then modify bin/auth to let you strip all factors only for a given provider).

resources/sql/autopatches/20190115.mfa.02.migrate.php
8

"factor types"

This revision is now accepted and ready to land.Jan 16 2019, 10:00 PM

These answers are somewhat-theory for now since you can't actually disable providers yet, but:

What's the behavior when the last MFA provider I have configured on my account gets disabled and security.require-multi-factor-auth is enabled?

I think this case is okay: you'll prompted to add a new MFA factor when you next log in. As long as the install lets you add something, you should be alright. Example is maybe: install switches from TOTP to Duo, sends you 93 emails to switch, you don't. You come in on Monday and are forced to set up Duo. Seems okay.

A related question is "What happens when security.require-multi-factor-auth is enabled but no MFA providers exist / are enabled?". I believe the current answer is "you get a somewhat-clear-but-sort-of-dead-endy error that no providers are enabled, and can't continue logging in". I think users probably dug this hole for themselves so I don't think we need to bend over backwards to help them out of it, but it could probably be smoother.

We could possibly add a guard rail to prevent you from enabling the "require MFA" option if no providers are enabled (but I believe we have no other similar guard rails now, and we can't prevent users from editing .json files), or we could prevent you from disabling the last provider if the "Require MFA" option is enabled, but I'm inclined to wait until someone shoots themselves in the foot before actually putting these behaviors in place. I do think special-casing the error when it arises because of MFA-required-at-login is pretty reasonable, since we can easily give users more clear instructions ("You totally locked yourself out and should feel bad. Disable MFA requirements from the CLI like this, then add a provider so it's possible to add MFA, then you can turn MFA requirements back on again.").

(I don't think we should behave like MFA isn't required since this means the system fails wide open if there's some kind of bug in the MFA provider stuff -- better to lock everyone out if the config is nonsense than let everyone in.)

Actually, another question here that's just occurring to me is "What happens when MFA is required and only an SMS provider is enabled?". This is a question because you currently need to access Settings to add a contact number before you can add SMS! So that will need at least a little bit of adjustment, I think.

I'll make sure to run through the "MFA triggered by MFA-required config" stuff in more detail once these scenarios are actually testable.

  • Correct "factors types".

Another possible thing is that sessions are long-lived and you only get force-MFA-gated at login, so an administrator might reconfigure MFA, think all the users are on the new stuff, and then learn that they a bunch weren't actually forced to switch to Duo or whatever yet since they're still on old sessions.

The current remedy is to kill every session with bin/auth revoke.

I'm inclined to say that maybe documentation is good enough here, especially if Auth ends up getting some better "who has this provider configured?" tools so you can obviously see that only 9 users have actually set up Duo or whatever.

But we could also think about other approaches. One thing we can do is downgrade sessions to partial without actually revoking them: so you don't have to type your password again, but you do get MFA gated if MFA requirements have changed. No tool currently supports this.