"Supports for"

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.

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.

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"?

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.

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.

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:"

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.

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

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?

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.

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

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

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.

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.

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?

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.

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.

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.

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.

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. 🐙