Page MenuHomePhabricator

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

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

Details

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
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.Dec 17 2018, 6:10 PM
Owners added a subscriber: Restricted Owners Package.Dec 17 2018, 6:10 PM
epriestley requested review of this revision.Dec 17 2018, 6:11 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.)

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 ALTERs in a single statement: https://stackoverflow.com/questions/27195977/adding-multiple-columns-in-mysql-with-one-statement )

epriestley updated this revision to Diff 47522.Dec 18 2018, 8:19 PM
  • Clarify "can never" vs "may never".
This revision was automatically updated to reflect the committed changes.