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.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19894

resources/sql/autopatches/20181217.auth.01.digest.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_challenge
+  ADD responseDigest VARCHAR(255) COLLATE {$COLLATE_TEXT};

resources/sql/autopatches/20181217.auth.02.ttl.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_challenge
+  ADD responseTTL INT UNSIGNED;

resources/sql/autopatches/20181217.auth.03.completed.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_challenge
+  ADD isCompleted BOOL NOT NULL;

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -576,6 +576,8 @@ final class PhabricatorAuthSessionEngine extends Phobject {
             continue;
           }
 
+          $issued_challenges = idx($challenge_map, $factor_phid, array());
+
           $impl = $factor->requireImplementation();
 
           $validation_result = $impl->getResultFromChallengeResponse(
@@ -592,6 +594,16 @@ final class PhabricatorAuthSessionEngine extends Phobject {
         }
 
         if ($ok) {
+          // We're letting you through, so mark all the challenges you
+          // responded to as completed. These challenges can never be used
+          // again, even by the same session and workflow: you can't use the
+          // same response to take two different actions, even if those actions
+          // are of the same type.
+          foreach ($validation_results as $validation_result) {
+            $challenge = $validation_result->getAnsweredChallenge()
+              ->markChallengeAsCompleted();
+          }
+
           // Give the user a credit back for a successful factor verification.
           PhabricatorSystemActionEngine::willTakeAction(
             array($viewer->getPHID()),

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -45,7 +45,7 @@ abstract class PhabricatorAuthFactor extends Phobject {
 
     $engine = $config->getSessionEngine();
 
-    return id(new PhabricatorAuthChallenge())
+    return PhabricatorAuthChallenge::initializeNewChallenge()
       ->setUserPHID($viewer->getPHID())
       ->setSessionPHID($viewer->getSession()->getPHID())
       ->setFactorPHID($config->getPHID())

src/applications/auth/factor/PhabricatorAuthFactorResult.php

@@ -3,19 +3,36 @@
 final class PhabricatorAuthFactorResult
   extends Phobject {
 
-  private $isValid = false;
+  private $answeredChallenge;
   private $isWait = false;
   private $errorMessage;
   private $value;
   private $issuedChallenges = array();
 
-  public function setIsValid($is_valid) {
-    $this->isValid = $is_valid;
+  public function setAnsweredChallenge(PhabricatorAuthChallenge $challenge) {
+    if (!$challenge->getIsAnsweredChallenge()) {
+      throw new PhutilInvalidStateException('markChallengeAsAnswered');
+    }
+
+    if ($challenge->getIsCompleted()) {
+      throw new Exception(
+        pht(
+          'A completed challenge was provided as an answered challenge. '.
+          'The underlying factor is implemented improperly, challenges '.
+          'may not be reused.'));
+    }
+
+    $this->answeredChallenge = $challenge;
+
     return $this;
   }
 
+  public function getAnsweredChallenge() {
+    return $this->answeredChallenge;
+  }
+
   public function getIsValid() {
-    return $this->isValid;
+    return (bool)$this->getAnsweredChallenge();
   }
 
   public function setIsWait($is_wait) {

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -283,6 +283,17 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
               'the code to cycle, then try again.',
               new PhutilNumber($wait_duration)));
       }
+
+      if ($challenge->getIsReusedChallenge()) {
+        return $this->newResult()
+          ->setIsWait(true)
+          ->setErrorMessage(
+            pht(
+              'You recently provided a response to this factor. Responses '.
+              'may not be reused. Wait %s second(s) for the code to cycle, '.
+              'then try again.',
+              new PhutilNumber($wait_duration)));
+      }
     }
 
     return null;
@@ -325,7 +336,16 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       (string)$code);
 
     if ($valid_timestep) {
-      $result->setIsValid(true);
+      $now = PhabricatorTime::getNow();
+      $step_duration = $this->getTimestepDuration();
+      $step_window = $this->getTimestepWindowSize();
+      $ttl = $now + ($step_duration * $step_window);
+
+      $challenge
+        ->setProperty('totp.timestep', $valid_timestep)
+        ->markChallengeAsAnswered($ttl);
+
+      $result->setAnsweredChallenge($challenge);
     } else {
       if (strlen($code)) {
         $error_message = pht('Invalid');

src/applications/auth/storage/PhabricatorAuthChallenge.php

@@ -10,8 +10,20 @@ final class PhabricatorAuthChallenge
   protected $workflowKey;
   protected $challengeKey;
   protected $challengeTTL;
+  protected $responseDigest;
+  protected $responseTTL;
+  protected $isCompleted;
   protected $properties = array();
 
+  private $responseToken;
+
+  const TOKEN_DIGEST_KEY = 'auth.challenge.token';
+
+  public static function initializeNewChallenge() {
+    return id(new self())
+      ->setIsCompleted(0);
+  }
+
   protected function getConfiguration() {
     return array(
       self::CONFIG_SERIALIZATION => array(
@@ -22,6 +34,9 @@ final class PhabricatorAuthChallenge
         'challengeKey' => 'text255',
         'challengeTTL' => 'epoch',
         'workflowKey' => 'text255',
+        'responseDigest' => 'text255?',
+        'responseTTL' => 'epoch?',
+        'isCompleted' => 'bool',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'key_issued' => array(
@@ -38,6 +53,104 @@ final class PhabricatorAuthChallenge
     return PhabricatorAuthChallengePHIDType::TYPECONST;
   }
 
+  public function getIsReusedChallenge() {
+    if ($this->getIsCompleted()) {
+      return true;
+    }
+
+    // TODO: A challenge is "reused" if it has been answered previously and
+    // the request doesn't include proof that the client provided the answer.
+    // Since we aren't tracking client responses yet, any answered challenge
+    // is always a reused challenge for now.
+
+    return $this->getIsAnsweredChallenge();
+  }
+
+  public function getIsAnsweredChallenge() {
+    return (bool)$this->getResponseDigest();
+  }
+
+  public function markChallengeAsAnswered($ttl) {
+    $token = Filesystem::readRandomCharacters(32);
+    $token = new PhutilOpaqueEnvelope($token);
+
+    return $this
+      ->setResponseToken($token, $ttl)
+      ->save();
+  }
+
+  public function markChallengeAsCompleted() {
+    return $this
+      ->setIsCompleted(true)
+      ->save();
+  }
+
+  public function setResponseToken(PhutilOpaqueEnvelope $token, $ttl) {
+    if (!$this->getUserPHID()) {
+      throw new PhutilInvalidStateException('setUserPHID');
+    }
+
+    if ($this->responseToken) {
+      throw new Exception(
+        pht(
+          'This challenge already has a response token; you can not '.
+          'set a new response token.'));
+    }
+
+    $now = PhabricatorTime::getNow();
+    if ($ttl < $now) {
+      throw new Exception(
+        pht(
+          'Response TTL is invalid: TTLs must be an epoch timestamp '.
+          'coresponding to a future time (did you use a relative TTL by '.
+          'mistake?).'));
+    }
+
+    if (preg_match('/ /', $token->openEnvelope())) {
+      throw new Exception(
+        pht(
+          'The response token for this challenge is invalid: response '.
+          'tokens may not include spaces.'));
+    }
+
+    $digest = PhabricatorHash::digestWithNamedKey(
+      $token->openEnvelope(),
+      self::TOKEN_DIGEST_KEY);
+
+    if ($this->responseDigest !== null) {
+      if (!phutil_hashes_are_identical($digest, $this->responseDigest)) {
+        throw new Exception(
+          pht(
+            'Invalid response token for this challenge: token digest does '.
+            'not match stored digest.'));
+      }
+    } else {
+      $this->responseDigest = $digest;
+    }
+
+    $this->responseToken = $token;
+    $this->responseTTL = $ttl;
+
+    return $this;
+  }
+
+  public function setResponseDigest($value) {
+    throw new Exception(
+      pht(
+        'You can not set the response digest for a challenge directly. '.
+        'Instead, set a response token. A response digest will be computed '.
+        'automatically.'));
+  }
+
+  public function setProperty($key, $value) {
+    $this->properties[$key] = $value;
+    return $this;
+  }
+
+  public function getProperty($key, $default = null) {
+    return $this->properties[$key];
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */