Mark Côté [:mcote] Reporter
	Description • 7 years ago

At the moment, our Phabricator deployments (dev, staging, and prod) all require users to configure MFA (via TOTP) before being able to use the site.  However, all logins, except admin accounts, go through BMO, which has MFA support (via Duo).  Furthermore, MFA support will be required for all employees in before the end of 2017 (see bug 1364233).  At this point, MFA in Phabricator will be completely redundant for many accounts.  I think we should remove the requirement for MFA on Phabricator at this point, or possibly before.  It doesn't make much sense to require it on Phabricator when it is not required on BMO, and it makes even less sense to require it on Phabricator when the user has it enabled on BMO.

	Julien Vehent
	Comment 1 • 7 years ago

Just to clarify: when you say "MFA support will be required for all employees in before the end of 2017", you do mean that all employees will be required to have MFA setup in BMO, not that they will just have the option to use MFA, correct?

We could drop the MFA requirement for this population, but that still leaves the question of contributors who are not employees. Some of those contributors are valuable targets, so it would be nice to make sure their accounts are secure too.

Though it does seem the entire problem needs to be handled by BMO, not phabricator. I agree that phabricator should drop the MFA requirement, and let BMO do the job.

	Mark Côté [:mcote] Reporter
	Comment 2 • 7 years ago

Correct.  Everyone already has the option to use MFA on BMO. :)

So, thing is, it seems more important to enable MFA on patch authors and reviewers.  The vast majority of BMO users don't, as far as I know, do either.  So forcing MFA on the general BMO population seems perhaps rather heavy handed.

I suppose we could block a person from signing onto Phabricator without MFA enabled, though.  Maybe that's the best solution?

	Julien Vehent
	Comment 3 • 7 years ago

> I suppose we could block a person from signing onto Phabricator without MFA enabled, though.  Maybe that's the best solution?

I like that approach.

	Mark Côté [:mcote] Reporter
	Comment 4 • 7 years ago

Not sure if this is the correct component, but it does belong in the BMO product somewhere.

To expand on comment 2, in the auth-delegation process, if we determine that the person is signing into Phabricator, then we should disallow access, with an appropriate error message, if the user doesn't have MFA enabled (in BMO).

	Dylan Hardison [:dylan] (he/him)
	Comment 5 • 7 years ago

I want to capture the desired outcome here, so I'm going to state what I think that is -- followed by what we need to do accomplish those. I want to also call attention to the fact that we (Mozilla) have a predilection for wanting features that are orthogonal to some (possibly unspoken) desired outcome -- or put more simply, sometimes we ask for three right turns when we actually want to turn left.  Taking those turns can be more efficient, but I want to make sure that is communicated: "We're going to turn left by turning right three times". 

First there are some details that need to explained:

Bug 1364233 provides the ability to make a group require 2fa. It seems like from :ulfr's perspective this should apply to all important users -- and doing that is a trivial consequence configuring the parameter introduced in that bug.

For the purposes of discussion we can refer to "trivial groups" as the ones that have low membership requirements
such as everyone, canconfirm, or editbugs. Other groups are "important groups".
In the context of bug 1364233 we can say that it will apply to all important groups. 

Now, to outcomes as declarative statements:

1. Everyone that uses Phabricator must use 2FA.
2. 2FA is not required for some Bugzilla users.

I am listing both to ensure that #2 is definitely true, because I feel that #1 
could really come from a desire to have #2, but not wanting to champion for that.
This is important, because if we decide that everyone must use 2FA, that is as simple as adding the "everyone" group to the set defined in bug 1364233.

Finally implementation details:

1. We might consider restricting auth delegation based on group rather than directly observing MFA status. 
2. We should invalidate the api key delegated to phabricator when someone disables MFA.
3. Phabricator should be checking if an account/api key is still valid periodically, or have quite short sessions limits.

	Julien Vehent
	Comment 6 • 7 years ago

> 3. Phabricator should be checking if an account/api key is still valid periodically, or have quite short sessions limits.

From experience with auth0 integration, having BMO provide an endpoint Phabricator can call in real-time to verify a user still exists would be a good feature. Short lived sessions tend to get annoying by requiring re-login, and don't achieve the goal of propagating an account disabling in real-time.

	Dylan Hardison [:dylan] (he/him)
	Comment 7 • 7 years ago

Even better, it does not need to add any new API, as it can just check if the API key it holds is still valid (which is an existing API)

	Dylan Hardison [:dylan] (he/him)
	Comment 8 • 7 years ago

So how do you want to do this?

My proposal (rather than tweaking the login system further) is to add a mfa_status: true|false to the whoami
endpoint, and then have phabricator auth plugin refuse login when it is false.

You can additionally periodically check if a particular phabricator user has mfa enabled still.

	Dylan Hardison [:dylan] (he/him)
	Comment 10 • 7 years ago

I suggest this because handling the error on the phabricator side is better experience, given that you don't even really see BMO's UI when logging in currently.

	David Walsh :davidwalsh
	Comment 11 • 7 years ago

Dylan's basic mfa_status check sounds reasonable to me.

	Phabricator Automation
	Comment 12 • 7 years ago

Comment on attachment 8932618 [details]
bmo auth: Support for blocking login if MFA not enabled in Bugzilla.

David Walsh :davidwalsh has approved the revision.

https://phabricator.services.mozilla.com/D295#7694

	Botond Ballo [:botond]
	Comment 13 • 7 years ago

Is part of this change going to be not requiring users to set up Phabricator's own MFA?

	David Lawrence [:dkl] Assignee
	Comment 14 • 7 years ago

(In reply to Botond Ballo [:botond] from comment #13)
> Is part of this change going to be not requiring users to set up
> Phabricator's own MFA?

That is correct.