Page MenuHomePhabricator

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

Authored by epriestley on Dec 17 2018, 6:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 10:13 AM
Unknown Object (File)
Thu, Dec 12, 12:46 AM
Unknown Object (File)
Mon, Dec 9, 2:46 AM
Unknown Object (File)
Thu, Dec 5, 8:24 AM
Unknown Object (File)
Sun, Dec 1, 3:00 PM
Unknown Object (File)
Wed, Nov 27, 2:55 PM
Unknown Object (File)
Nov 23 2024, 10:09 AM
Unknown Object (File)
Nov 19 2024, 3:40 PM
Subscribers
Restricted Owners Package

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
Branch
mfa12
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/auth/storage/PhabricatorAuthChallenge.php:61XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 21356
Build 29066: Run Core Tests
Build 29065: arc lint + arc unit

Event Timeline

Owners added a subscriber: Restricted Owners Package.Dec 17 2018, 6:10 PM
amckinley added inline comments.
resources/sql/autopatches/20181217.auth.01.digest.sql
1–2

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
resources/sql/autopatches/20181217.auth.01.digest.sql
1–2

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

resources/sql/autopatches/20181217.auth.01.digest.sql
1–2

(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 )

  • Clarify "can never" vs "may never".
This revision was automatically updated to reflect the committed changes.