Page MenuHomePhabricator

Allow MFA providers to be deprecated or disabled
ClosedPublic

Authored by epriestley on Jan 25 2019, 2:00 PM.

Details

Summary

Ref T13222. Providers can now be deprecated (existing factors still work, but users can't add new factors for the provider) or disabled (factors stop working, also can't add new ones).

Test Plan
  • Enabled, deprecated, and disabled some providers.
  • Viewed provider detail, provider list.
  • Viewed MFA settings list.
  • Verified that I'm prompted for enabled + deprecated only at gates.
  • Tried to disable final provider, got an error.
  • Hit the MFA setup gate by enabling "Require MFA" with no providers, got a more useful message.
  • Immediately forced a user to the "MFA Setup Gate" by disabling their only active provider with another provider enabled ("We no longer support TOTP, you HAVE to finish Duo enrollment to continue starting Monday.").

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 25 2019, 2:00 PM
epriestley requested review of this revision.Jan 25 2019, 2:02 PM
epriestley updated this revision to Diff 47838.Jan 25 2019, 2:03 PM
  • Fix a double-render on the "MFA Setup Gate" screen.
epriestley added inline comments.Jan 25 2019, 2:11 PM
src/applications/auth/constants/PhabricatorAuthFactorProviderStatus.php
82

I don't love this name (it means "No New Enrollments Under This Provider, But Existing Configs Will Continue to Work"), but can't come up with anything short and obviously-better. Currently planning to try to just document my way out of this.

src/applications/auth/factor/PhabricatorAuthFactor.php
57–67

These API changes are partly for consistency, and partly to support providers like Duo saying "you can't configure two factors for this provider", since it's redundant with Duo.

src/applications/auth/xaction/PhabricatorAuthFactorProviderStatusTransaction.php
84–102

This causes all users accounts to immediately hit the "MFA Setup Gate" on their next request if they need it. (If they don't need to hit the setup gate, we'll update the cache back to "1" and continue normally on their next request.)

The way this cache works is that "1" is a fast path to skip the checks, and "0" puts us on the slow path where we do all the checks correctly and actually read the full application state. So this is really "dirty a cache", not "mark all users as having no MFA in some meaningful way".

In this case, a more precise name for the column might be something like canUserDefinitelySkipMFASetupGateBecauseWeKnowTheyHaveMFA.

epriestley updated this revision to Diff 47839.Jan 25 2019, 2:12 PM
  • Spelling correction, minor wordsmithing for clarity.
amckinley accepted this revision.Jan 26 2019, 6:53 PM
amckinley added inline comments.
src/applications/auth/constants/PhabricatorAuthFactorProviderStatus.php
82

STATUS_SUNSETTING?
STATUS_PLANNED_OBSOLESCENCE?

Or we could get rid of this state by making the enrollment in a new provider much more in-your-face: you can't disable an existing provider until you configure a new one (or sign a waiver accepting that you're killing MFA for everyone), and then all users immediately get prompted to configure the new provider.

Or we could just say that "slowly migrating to a new provider" (as opposed to a crisis migration because of a breach) is an edge case that isn't worth supporting, and prompt users at their next login/high-sec action to setup a new factor.

src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php
230–235

I think this is a little too verbose for end users. Maybe just say "You are required to enable multifactor authentication to continue, but this install does not have a valid multifactor configuration. Contact your administrator." (or similar).

239–242

Also, this is probably worth doing as a setup check?

This revision is now accepted and ready to land.Jan 26 2019, 6:53 PM

On the "ways to lock users out by misconfiguring stuff" issues in general, I think security.require-multi-factor-auth will eventually get replaced with a configuration UI so you can set policies like "Administrators need strong MFA" or "Everyone needs MFA: Technical people need good MFA but everyone else can use SMS" (see T6115). Once that happens, you should only be able to get into a bad state by using UpDaTe ... statements to forcefully break the database. I also think it's probably (?) quite hard to break things now without being pretty aggressive about it, even though it's possible, so I'm not too worried about completely covering the lock-yourself-out edge cases.

src/applications/auth/constants/PhabricatorAuthFactorProviderStatus.php
82

I think this "migrate/sunset" step is at least somewhat valuable because jumping straight to "Disabled" is effectively the same as stripping the old factor -- if you move "TOTP" from "active" to "disabled", that's largely equivalent to just removing it from everyone's accounts. This might give an attacker a window to add MFA to an account without needing old MFA, since at the "MFA Setup Gate" they can just add a factor they control and then escalate their attack from there.

If the change is "corporate policy", that seems possibly bad. Even if there was a breach, it seems possibly bad unless the nature of the breach was catastrophic and the factor is completely compromised to all attackers. If the breach wasn't a total compromise, it seems bad to say "anyone who got up from their desk for a minute to get coffee can now have adversarial MFA added to their account by an opportunistic attacker".

(We do prevent you from disabling or deprecating the last active provider if security.require-multi-factor-auth is enabled, but not if it's disabled, and you can always UpDaTe ... yourself into that state or disable "Require MFA", disable all your providers, and then enable "Require MFA" to get yourself into a mess.

src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php
230–235

This message is mostly aimed at administrators, I'm just not calling them out explicitly.

I don't want to dead-end administrators here since I expect almost every user to ever see this to be an administrator who just screwed up the configuration and doesn't quite know what they're doing.

239–242

You can't get to soft setup checks (in Config) in this state.

We could raise this as a hard fatal setup check, but then we'd always be guaranteed to lock out every user immediately, which seems worse to me.

It's possible for an administrator to deprecate a provider which they have a factor for, then turn on MFA requirements, and end up in a state where new users will hit this but the administrator themselves won't. In that case, a soft setup check might help. But you have to try pretty hard to do this, and the message above should get you on the right path.

We could prevent bin/config set ... from turning on the flag in this state, but users can still edit .json or .php files, or a SiteConfig class can provide the configuration.

This revision was automatically updated to reflect the committed changes.