Explicitly mark MFA challenges as "answered" and "completed"
ClosedPublic
Actions

Authored by epriestley on Dec 17 2018, 6:10 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13222: 2018 Week 48-51 Bonus Content

Commits

rPce953ea44790: Explicitly mark MFA challenges as "answered" and "completed"

Summary

Depends on D19893. Ref T13222. See PHI873. A challenge is "answered" if you provide a valid response. A challenge is "completed" if we let you through the MFA check and do whatever actual action the check is protecting.

If you only have one MFA factor, challenges will be "completed" immediately after they are "answered". However, if you have two or more factors, it's possible to "answer" one or more prompts, but fewer than all of the prompts, and end up with "answered" challenges that are not "completed".

In the future, it may also be possible to answer all the challenges but then have an error occur before they are marked "completed" (for example, a unique key collision in the transaction code). For now, nothing interesting happens between "answered" and "completed". This would take the form of the caller explicitly providing flags like "wait to mark the challenges as completed until I do something" and "okay, mark the challenges as completed now".

This change prevents all token reuse, even on the same workflow. Future changes will let the answered challenges "stick" to the client form so you don't have to re-answer challenges for a short period of time if you hit a unique key collision.

Test Plan

Used a token to get through an MFA gate.
Tried to go through another gate, was told to wait for a long time for the next challenge window.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Dec 17 2018, 6:10 PM

Owners added a subscriber: Restricted Owners Package.Dec 17 2018, 6:10 PM

Harbormaster completed remote builds in B21356: Diff 47502.Dec 17 2018, 6:11 PM

epriestley requested review of this revision.Dec 17 2018, 6:11 PM

epriestley added a child revision: D19895: Carry MFA responses which have been "answered" but not "completed" through the MFA workflow.Dec 17 2018, 7:12 PM

amckinley accepted this revision.Dec 18 2018, 12:19 AM

amckinley added inline comments.

resources/sql/autopatches/20181217.auth.01.digest.sql
2–3	Why not combine these three migrations? Or did you just add them one at a time during development? I get that in practice this won't affect anyone's upgrade time because all these tables will be empty; just wondering if there's a grander reason here that isn't obvious.
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
598	"These challenges can't ever be used" would be less ambiguous.

This revision is now accepted and ready to land.Dec 18 2018, 12:19 AM

epriestley added inline comments.Dec 18 2018, 1:16 AM

resources/sql/autopatches/20181217.auth.01.digest.sql
2–3	The big reason is that if a migration looks like `ALTER TABLE ADD A; ALTER TABLE ADD B;` and `ALTER TABLE ADD B;` fails for some reason we didn't anticipate (say, you run out of disk space or something), `bin/storage upgrade` will no longer work since it will try to run `ALTER TABLE ADD A;` again. If each statement is in a separate file, the migrations are more likely to resume safely. (There's still a window between `ALTER TABLE ADD B` taking effect and us recording "we applied patch XYZ" that can break things in a similar way, but that's a pretty narrow window.)

epriestley mentioned this in D19893: When accepting a TOTP response, require it respond explicitly to a specific challenge.Dec 18 2018, 1:16 AM

amckinley added inline comments.Dec 18 2018, 6:33 PM

resources/sql/autopatches/20181217.auth.01.digest.sql
2–3	(FWIW, because I didn't know this, MySQL supports multiple `ALTER`s in a single statement: https://stackoverflow.com/questions/27195977/adding-multiple-columns-in-mysql-with-one-statement )