Page MenuHomePhabricator

Bring Duo MFA upstream
ClosedPublic

Authored by epriestley on Jan 25 2019, 9:22 PM.

Details

Summary

Depends on D20038. Ref T13231. Although I planned to keep this out of the upstream (see T13229) it ended up having enough pieces that I imagine it may need more fixes/updates than we can reasonably manage by copy/pasting stuff around. Until T5055, we don't really have good tools for managing this. Make my life easier by just upstreaming this.

Test Plan

See T13231 for a bunch of workflow discussion.

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, 9:22 PM
epriestley requested review of this revision.Jan 25 2019, 9:24 PM
epriestley updated this revision to Diff 47849.Jan 25 2019, 9:44 PM
  • Minor cleanup.

This is half-reviewed; more coming soon.

src/applications/auth/factor/PhabricatorDuoAuthFactor.php
25

"Supports for"

321

I know QR codes are designed to survive translation and rotation, but can they actually survive reflection? Either way, this is still funny, but should maybe be behind a serious-business check, since I predict a link between Duo environments and an expectation of Serious Business.

349–351

Shouldn't we wrap this in a try block? I guess just throwing a standard yellow dialogue box exception is ok, but since this is a 3rd party service dependency, it would be nice to make it look less like a generic internal Phabricator error.

354

I know resolve() will throw on a non-2xx response, but does a 2xx response guarantee that response and result will be populated? Or does the Duo API do the goofy thing where a 2xx response means "the API handled your request, but you need to look for $result['response']['error'] before you can say if there were any application-level issues"?

360–364

I'm not sure what our style guide says about case statements that return without a following break, but every branch here results in a return so I don't really care.

459–465

This is pretty wacky. What was the install's intent if they have Duo configured as an MFA provider but then disable the challenge behavior? I'm picturing something we probably wouldn't want to support/encourage like "because Reasons™, an install configured auth.mfa-required, but the CISO can't be bothered to jump through silly security hoops and demands that he, and only he, can skip MFA", so they configure Duo to skip challenge requirements only for that user.

471–472

Agreed that this isn't really our problem, but maybe we should provide some guidance, like "contact your Duo administrator and show them this message:"

487–493

Why not something like:

$capabilities = array_fuse(ipull($devices, 'capabilities'));
if (isset($capabilities['push'])) {
  $has_push = true'
}

This is more of a "curious why you implemented it like this" question as opposed to a "why didn't you implement it with my clearly superior way" question.

500

This is actually more like "The Duo factor that is associated with your Phabricator account is no longer recognized by Duo. To continue...", right?

505–511

This is really the state-of-the-art for urlencoding? I figured we'd be doing this stuff all day/every day. Or does the libphutil URL stuff expect that we're passing in a full, well-formed URL instead of this little snippet that's getting wrapped again inside another URL parameter later?

525

Maybe just distinguish between "Login" and "High-security Action"? Not sure if that information is carried sufficiently down the stack to expose it here, but it's not a big deal either way.

epriestley added inline comments.Jan 28 2019, 11:01 PM
src/applications/auth/factor/PhabricatorDuoAuthFactor.php
321

Turns out they do! I took a TOTP QR screenshot, flipped it horizontally in Photoshop, and Google Authenticator scanned it in fine.

354

Duo does "non-200 for error, 200 for okay".

360–364

See D1824. It's okay for a block to end with return, break, throw, exit(), continue, have an explicit fallthrough comment, or be empty.

Since D19931, continue must be continue 2 or better and occur inside a loop which it can target.

459–465

I'm hoping it's usually "someone made a mistake with configuration", but worry your scenario may also happen. If that's the case, I'd at least want to carry this flag out of the MFA process so that applications can distinguish between "user did actual MFA" and "user did silly pointless MFA".

For now, I'm hoping that this is a mistake and the remedy is to fix Duo configuration.

471–472

I don't actually know how accounts could possibly get into this state so I'm not sure who you should contact. Possibly us to tell us how you did it?

487–493

In a pure technical sense, ipull($devices, 'capabilities') would give us this:

array(
  0 => array('push', 'sms'),
  1 => array('push', 'voice'),
)

Since the values aren't scalars, array_fuse() would fatal.

But something like $capabilities = array_fuse(array_mergev(ipull($devices, 'capabilities'))); would work.

One reason I used a loop is that I'm imagining we may eventually need to extend this into "Choose which device we should push to", or "Here's the phone number of the device we're pushing to". Features like that are likely easier with the loop, so we can figure out which devices we actually matched with.

Another is just a side effect of it being easier to write the code incrementally, e.g. I think I wrote something like this first:

foreach ($devices as $device) {
  var_dump($device);
}

...then looked at the values to make sure they were what I expected and added the next part, and so on until the logic worked.

500

The way I trigger this locally is by going into Duo on my phone and deleting the entry for Phabricator. (In the general case, this is deleting my entry for "The Company I Work At").

Duo still recognizes the account, but you've unpaired the account with all your phones so you can't prove you own the account. Put another way, you've sort of intentionally lost your own phone.

I think Duo might let us enroll you again in this state but I'm not entirely sure, and it's potentially quite a mess to do an enroll from within a challenge.

Two remedies which likely work:

  1. A Phabricator administrator strips the MFA from your Phabricator account, you re-add it, and you'll re-enroll if we're allowed to enroll you. If not, or alternatively:
  2. A Duo administrator sends you a fresh enroll email, you go through the enroll process again out-of-band, and Phabricator will magically start working again.

While you're in this state, you're locked out of every Duo tool in your org, so I'm guessing this will almost always be remedy (2) and users will hit some other application before they end up here.

505–511

We can do this instead:

$uri = id(new PhutilURI('/'))
  ->setQueryParams($push_info);
$uri = ltrim((string)$uri, '/?');

...and we could add a getQueryString() to PhutilURI to avoid the ltrim(). There's also http_build_query(), which is what PhutilURI uses internally, but it requires weird arguments to guarantee sensible behavior.

We don't have a phutil_build_query_with_sane_behavior($exactly_one_argument); so mostly the latter: fairly good tools for actually working with URIs, less-clean tools for double-encoding URI pieces inside another URI. I could clean this up -- we have similar code in the Duo future for signing.

525

Right now, we can't (reasonably) tell which is which at this layer.

I would sort of like to pass human-readable workflow names down to this layer so we can say Login, Edit T123, etc -- Duo isn't super flexible, but we can put ~anything in an SMS at least. But this feels like a pretty fluff feature.

  • "Supports" typo.
  • Don't recommend using a mirror even though this would definitely work great.
  • Blame Duo administrators for everything.
amckinley accepted this revision.Jan 29 2019, 2:07 AM
amckinley added inline comments.
src/applications/auth/factor/PhabricatorDuoAuthFactor.php
321

howneatisthat

525

The only sense in which it's not fluffy is that it would maybe help defeat a lot of goofy hypothetical social engineering attacks like this:

Hello, this is Bob User, who's calling please?

Hi Bob, this is Robert Hackerman, from the county password inspection department. I need to send you a Duo challenge and have you approve it to make sure your phone conforms to the Fiscal Year 2019 standards.

Sure, I've got the app open now... hey, wait a minute, Bob! Normally my challenges say "Approve Lunch Expenses Request", but this one says "Launch all the Missiles Request! GOOD DAY SIR! <slams phone down>

Anyway, out of scope for this revision, but maybe worth a followup note (especially because it could share a lot of code with SMS).

578

I doubt this is ever going to be a problem in practice, but have you tried running this with a really short timeout so we hammer the endpoint a bunch of times? Since we're supposed to long-poll, I'm slightly worried that Duo would rate limit us.

711–715

This is pretty cool; are we considering switching the other "sensitive" config keys like AWS credentials to use Passphrase?

src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoCredentialTransaction.php
19–25

Copy/paste mistake.

src/docs/user/userguide/multi_factor_auth.diviner
112

Probably worth adding a link here.

120

"sensitive" vs "high security"

This revision is now accepted and ready to land.Jan 29 2019, 2:07 AM
epriestley added inline comments.Jan 29 2019, 2:15 AM
src/applications/auth/factor/PhabricatorDuoAuthFactor.php
711–715

Anything that's UI-configurable should already be in Passphrase, I think (Diffusion, Drydock, Harbormaster).

Stuff like AWS isn't UI-configurable today.

The major problem with making stuff like AWS UI-configurable is that then an attacker can UI-reconfigure it to use their account and intercept all your email/storage/etc.

I think there's also at least some value in being able to check cluster.mailers and such into version control.

Having a bunch of secret keys in a JSON file doesn't feel great, but it's sort of like the least-bad alternative in some senses (attacker resistance, compatibility with ops flows that want stuff to be in VCS)?

We can possibly solve "attacker resistance" with bin/auth lock-style stuff ("unlock, edit via web UI, lock") but I kind of think that no one will ever actually run a bin/<something> lock command willingly so we'll end up with poor attacker resistance.

I suppose we could make bin/auth unlock temporary and automatically re-lock it after, say, 8 hours. But at this point we're kind of fighting administrators and still don't have a good answer for "I want to check the config into Git so I don't blow it away with git clean --force --really", which we've now both done, albeit not in production. 🐙

Also, BOY, this is a lot of code.

Yeah, I thought this was going to be like 200 lines long when I started down this path seventy five years ago. 😿

epriestley updated this revision to Diff 47879.Jan 29 2019, 2:20 AM
  • Fix transaction story typo.
  • Link to Duo.
This revision was automatically updated to reflect the committed changes.