Page MenuHomePhabricator

Update `bin/auth` MFA commands for the new "MFA Provider" indirection layer
ClosedPublic

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

Details

Summary

Ref T13222. This updates the CLI tools and documentation for the changes in D19975.

The flags --type and --all-types retain their current meaning. In most cases, bin/auth strip --type totp is sufficient and you don't need to bother looking up the relevant provider PHID. The existing bin/auth list-factors is also unchanged.

The new --provider flag allows you to select configs from a particular provider in a more granular way. The new bin/auth list-mfa-providers provides an easy way to get PHIDs.

(In the Phacility cluster, the "Strip MFA" action just reaches into the database and deletes rows manually, so this isn't terribly important. I verified that the code should still work properly.)

Test Plan
  • Ran bin/auth list-mfa-providers.
  • Stripped by user / type / provider.
  • Grepped for list-factors and auth strip.
  • Hit all (?) of the various possible error cases.

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:38 PM
epriestley requested review of this revision.Jan 15 2019, 9:40 PM
amckinley accepted this revision.Jan 16 2019, 11:53 PM

I'm still marking this as accepted; just put it back in "Changes Planned" if either of my issues resonates with you.

src/applications/auth/management/PhabricatorAuthManagementStripWorkflow.php
28–29

I'm not wild about forcing everyone to run auth list-providers before auth strip, especially because this is likely to be a flow carried out during extreme stress/urgency after a security incident. My naive suggestion is to make providers provider a "shortname" like sms, totp, etc, but I'm sure there's an obvious reason not to do that. This also means that we can't provide a documentation example like bin/auth strip --type sms or similar.

96–97

"Specify either specific providers"

106–107

"which provider to strip"

107–110

"strip all providers"

171

Ok this one should actually be "factors".

216

Do ProviderConfigs have a status that we could set to disabled instead?

Thinking about potential advantages of non-destructive deletes:

  • Maaaaaaaybe it's a good idea to require that all factor configs must be globally unique across all of history? I'm thinking of something like 1) install decides to strip everyone's SMS MFA factors because an attacker was playing social engineering games and took control of a subset of employee phone numbers, 2) one of those users with a compromised phone doesn't know it yet, 3) user gets forced on next login to configure SMS and naively puts their compromised phone number back in. This attack is (mostly?) defeated by forcing the user to pass an SMS MFA check before marking that provider as active. This is obviously contrived, and not letting anyone reuse a phone number would screw up lots of expectations about recovering from a breach like this anyway, so maybe ignore all this(?). (But as a note, UI-wise it'll be important to separate "Add SMS number to my Settings" from "enable SMS as an MFA provider" anyway).
  • We could add a DisableProviderConfigTransaction that would log the dastardly user that stripped everyone's factors through malice, but if you already have access to bin/auth you can alter the DB anyway.
  • Provides better post-mortem audit-ability in a nebulous hand-wavy way by being able to answer the question "who had which providers enabled" at any point in time.
This revision is now accepted and ready to land.Jan 16 2019, 11:53 PM

My naive suggestion is to make providers provider a "shortname" like sms, totp, etc, but I'm sure there's an obvious reason not to do that.

Not really -- just that you can have more than one, say, duo provider.

I generally agree that where I ended up here doesn't feel especially clean. I'm going to take another stab at it and do this instead:

  • --type and --all-types are retained and have the same behavior, so --type sms has the same meaning it used to.
  • bin/auth list-factors is retained without any behavioral changes, to get a list of factor types ("sms", "totp").
  • New --provider <phid> flag.
  • New bin/auth list-mfa-providers workflow to get PHIDs.

Do ProviderConfigs have a status that we could set to disabled instead?

Not right now. They should, and I agree with your list of advantages, they just got implemented a while ago and aren't quite up to modern product standards. I'll either do this in a followup or file a task for it depending on how ambitious I'm feeling.

epriestley edited the summary of this revision. (Show Details)Jan 18 2019, 3:41 PM
epriestley edited the test plan for this revision. (Show Details)
epriestley updated this revision to Diff 47737.Jan 18 2019, 3:44 PM
  • Keep --type with the current meaning: --type sms means "every SMS config, regardless of provider".
  • Add --provider so you can kill one of your Duo providers or something, in a very complex world where your MFA setup is a big mess.
  • Keep bin/auth list-factors, which spits out sms, totp, etc.
  • Add bin/auth list-mfa-providers, which spits out PHID-FPRV-xxx Duo (Ops, DEPRECATED DO NOT USE), etc.

They should [disable instead of delete], and I agree with your list of advantages ... I'll either do this in a followup or file a task for it depending on how ambitious I'm feeling.

Filed as T13237 for now. I might still get to this, but there's a slightly tricky interaction with hosted instances. (Of course, it would be fine for the admin instance button to keep doing a DELETE for now -- we'd still be in a better place overall if bin/auth strip just disables.)

epriestley requested review of this revision.Jan 18 2019, 3:49 PM

I changed course a bit here, let me know if this feels like we're on firmer ground?

This revision is now accepted and ready to land.Jan 18 2019, 5:56 PM
This revision was automatically updated to reflect the committed changes.