Allow different MFA factor types (SMS, TOTP, Duo, ...) to share "sync" tokens when enrolling new factors

Summary:
Depends on D20019. Ref T13222. Currently, TOTP uses a temporary token to make sure you've set up the app on your phone properly and that you're providing an answer to a secret which we generated (not an attacker-generated secret).

However, most factor types need some kind of sync token. SMS needs to send you a code; Duo needs to store a transaction ID. Turn this "TOTP" token into an "MFA Sync" token and lift the implementation up to the base class.

Also, slightly simplify some of the HTTP form gymnastics.

Test Plan:
  - Hit the TOTP enroll screen.
  - Reloaded it, got new secrets.
  - Reloaded it more than 10 times, got told to stop generating new challenges.
  - Answered a challenge properly, got a new TOTP factor.
  - Grepped for removed class name.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

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

src/__phutil_library_map__.php

@@ -2265,6 +2265,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthLoginMessageType' => 'applications/auth/message/PhabricatorAuthLoginMessageType.php',
     'PhabricatorAuthLogoutConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthLogoutConduitAPIMethod.php',
     'PhabricatorAuthMFAEditEngineExtension' => 'applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php',
+    'PhabricatorAuthMFASyncTemporaryTokenType' => 'applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php',
     'PhabricatorAuthMainMenuBarExtension' => 'applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php',
     'PhabricatorAuthManagementCachePKCS8Workflow' => 'applications/auth/management/PhabricatorAuthManagementCachePKCS8Workflow.php',
     'PhabricatorAuthManagementLDAPWorkflow' => 'applications/auth/management/PhabricatorAuthManagementLDAPWorkflow.php',
@@ -2359,7 +2360,6 @@ phutil_register_library_map(array(
     'PhabricatorAuthSetPasswordController' => 'applications/auth/controller/PhabricatorAuthSetPasswordController.php',
     'PhabricatorAuthSetupCheck' => 'applications/config/check/PhabricatorAuthSetupCheck.php',
     'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php',
-    'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php',
     'PhabricatorAuthTemporaryToken' => 'applications/auth/storage/PhabricatorAuthTemporaryToken.php',
     'PhabricatorAuthTemporaryTokenGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthTemporaryTokenGarbageCollector.php',
     'PhabricatorAuthTemporaryTokenQuery' => 'applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php',
@@ -7986,6 +7986,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthLoginMessageType' => 'PhabricatorAuthMessageType',
     'PhabricatorAuthLogoutConduitAPIMethod' => 'PhabricatorAuthConduitAPIMethod',
     'PhabricatorAuthMFAEditEngineExtension' => 'PhabricatorEditEngineExtension',
+    'PhabricatorAuthMFASyncTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
     'PhabricatorAuthMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension',
     'PhabricatorAuthManagementCachePKCS8Workflow' => 'PhabricatorAuthManagementWorkflow',
     'PhabricatorAuthManagementLDAPWorkflow' => 'PhabricatorAuthManagementWorkflow',
@@ -8101,7 +8102,6 @@ phutil_register_library_map(array(
     'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController',
     'PhabricatorAuthSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorAuthStartController' => 'PhabricatorAuthController',
-    'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
     'PhabricatorAuthTemporaryToken' => array(
       'PhabricatorAuthDAO',
       'PhabricatorPolicyInterface',

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -245,4 +245,104 @@ abstract class PhabricatorAuthFactor extends Phobject {
   }
 
 
+/* -(  Synchronizing New Factors  )------------------------------------------ */
+
+
+  final protected function loadMFASyncToken(
+    AphrontRequest $request,
+    AphrontFormView $form,
+    PhabricatorUser $user) {
+
+    // If the form included a synchronization key, load the corresponding
+    // token. The user must synchronize to a key we generated because this
+    // raises the barrier to theoretical attacks where an attacker might
+    // provide a known key for factors like TOTP.
+
+    // (We store and verify the hash of the key, not the key itself, to limit
+    // how useful the data in the table is to an attacker.)
+
+    $sync_type = PhabricatorAuthMFASyncTemporaryTokenType::TOKENTYPE;
+    $sync_token = null;
+
+    $sync_key = $request->getStr($this->getMFASyncTokenFormKey());
+    if (strlen($sync_key)) {
+      $sync_key_digest = PhabricatorHash::digestWithNamedKey(
+        $sync_key,
+        PhabricatorAuthMFASyncTemporaryTokenType::DIGEST_KEY);
+
+      $sync_token = id(new PhabricatorAuthTemporaryTokenQuery())
+        ->setViewer($user)
+        ->withTokenResources(array($user->getPHID()))
+        ->withTokenTypes(array($sync_type))
+        ->withExpired(false)
+        ->withTokenCodes(array($sync_key_digest))
+        ->executeOne();
+    }
+
+    if (!$sync_token) {
+
+      // Don't generate a new sync token if there are too many outstanding
+      // tokens already. This is mostly relevant for push factors like SMS,
+      // where generating a token has the side effect of sending a user a
+      // message.
+
+      $outstanding_limit = 10;
+      $outstanding_tokens = id(new PhabricatorAuthTemporaryTokenQuery())
+        ->setViewer($user)
+        ->withTokenResources(array($user->getPHID()))
+        ->withTokenTypes(array($sync_type))
+        ->withExpired(false)
+        ->execute();
+      if (count($outstanding_tokens) > $outstanding_limit) {
+        throw new Exception(
+          pht(
+            'Your account has too many outstanding, incomplete MFA '.
+            'synchronization attempts. Wait an hour and try again.'));
+      }
+
+      $now = PhabricatorTime::getNow();
+
+      $sync_key = Filesystem::readRandomCharacters(32);
+      $sync_key_digest = PhabricatorHash::digestWithNamedKey(
+        $sync_key,
+        PhabricatorAuthMFASyncTemporaryTokenType::DIGEST_KEY);
+      $sync_ttl = $this->getMFASyncTokenTTL();
+
+      $sync_token = id(new PhabricatorAuthTemporaryToken())
+        ->setIsNewTemporaryToken(true)
+        ->setTokenResource($user->getPHID())
+        ->setTokenType($sync_type)
+        ->setTokenCode($sync_key_digest)
+        ->setTokenExpires($now + $sync_ttl);
+
+      // Note that property generation is unguarded, since factors that push
+      // a challenge generally need to perform a write there.
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        $properties = $this->newMFASyncTokenProperties($user);
+
+        foreach ($properties as $key => $value) {
+          $sync_token->setTemporaryTokenProperty($key, $value);
+        }
+
+        $sync_token->save();
+      unset($unguarded);
+    }
+
+    $form->addHiddenInput($this->getMFASyncTokenFormKey(), $sync_key);
+
+    return $sync_token;
+  }
+
+  protected function newMFASyncTokenProperties(PhabricatorUser $user) {
+    return array();
+  }
+
+  private function getMFASyncTokenFormKey() {
+    return 'sync.key';
+  }
+
+  private function getMFASyncTokenTTL() {
+    return phutil_units('1 hour in seconds');
+  }
+
 }

src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php

@@ -1,17 +1,18 @@
 <?php
 
-final class PhabricatorAuthTOTPKeyTemporaryTokenType
+final class PhabricatorAuthMFASyncTemporaryTokenType
   extends PhabricatorAuthTemporaryTokenType {
 
-  const TOKENTYPE = 'mfa:totp:key';
+  const TOKENTYPE = 'mfa.sync';
+  const DIGEST_KEY = 'mfa.sync';
 
   public function getTokenTypeDisplayName() {
-    return pht('TOTP Synchronization');
+    return pht('MFA Sync');
   }
 
   public function getTokenReadableTypeName(
     PhabricatorAuthTemporaryToken $token) {
-    return pht('TOTP Sync Token');
+    return pht('MFA Sync Token');
   }
 
 }

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -2,8 +2,6 @@
 
 final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
 
-  const DIGEST_TEMPORARY_KEY = 'mfa.totp.sync';
-
   public function getFactorKey() {
     return 'totp';
   }
@@ -31,68 +29,26 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     AphrontRequest $request,
     PhabricatorUser $user) {
 
-    $totp_token_type = PhabricatorAuthTOTPKeyTemporaryTokenType::TOKENTYPE;
-
-    $key = $request->getStr('totpkey');
-    if (strlen($key)) {
-      // If the user is providing a key, make sure it's a key we generated.
-      // This raises the barrier to theoretical attacks where an attacker might
-      // provide a known key (such attacks are already prevented by CSRF, but
-      // this is a second barrier to overcome).
-
-      // (We store and verify the hash of the key, not the key itself, to limit
-      // how useful the data in the table is to an attacker.)
-
-      $token_code = PhabricatorHash::digestWithNamedKey(
-        $key,
-        self::DIGEST_TEMPORARY_KEY);
-
-      $temporary_token = id(new PhabricatorAuthTemporaryTokenQuery())
-        ->setViewer($user)
-        ->withTokenResources(array($user->getPHID()))
-        ->withTokenTypes(array($totp_token_type))
-        ->withExpired(false)
-        ->withTokenCodes(array($token_code))
-        ->executeOne();
-      if (!$temporary_token) {
-        // If we don't have a matching token, regenerate the key below.
-        $key = null;
-      }
-    }
-
-    if (!strlen($key)) {
-      $key = self::generateNewTOTPKey();
-
-      // Mark this key as one we generated, so the user is allowed to submit
-      // a response for it.
-
-      $token_code = PhabricatorHash::digestWithNamedKey(
-        $key,
-        self::DIGEST_TEMPORARY_KEY);
-
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        id(new PhabricatorAuthTemporaryToken())
-          ->setTokenResource($user->getPHID())
-          ->setTokenType($totp_token_type)
-          ->setTokenExpires(time() + phutil_units('1 hour in seconds'))
-          ->setTokenCode($token_code)
-          ->save();
-      unset($unguarded);
-    }
+    $sync_token = $this->loadMFASyncToken(
+      $request,
+      $form,
+      $user);
+    $secret = $sync_token->getTemporaryTokenProperty('secret');
 
     $code = $request->getStr('totpcode');
 
     $e_code = true;
-    if ($request->getExists('totp')) {
+    if (!$sync_token->getIsNewTemporaryToken()) {
       $okay = (bool)$this->getTimestepAtWhichResponseIsValid(
         $this->getAllowedTimesteps($this->getCurrentTimestep()),
-        new PhutilOpaqueEnvelope($key),
+        new PhutilOpaqueEnvelope($secret),
         $code);
 
       if ($okay) {
         $config = $this->newConfigForUser($user)
           ->setFactorName(pht('Mobile App (TOTP)'))
-          ->setFactorSecret($key);
+          ->setFactorSecret($secret)
+          ->setMFASyncToken($sync_token);
 
         return $config;
       } else {
@@ -104,9 +60,6 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       }
     }
 
-    $form->addHiddenInput('totp', true);
-    $form->addHiddenInput('totpkey', $key);
-
     $form->appendRemarkupInstructions(
       pht(
         'First, download an authenticator application on your phone. Two '.
@@ -126,7 +79,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       'otpauth://totp/%s:%s?secret=%s&issuer=%s',
       $issuer,
       $user->getUsername(),
-      $key,
+      $secret,
       $issuer);
 
     $qrcode = $this->renderQRCode($uri);
@@ -135,7 +88,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     $form->appendChild(
       id(new AphrontFormStaticControl())
         ->setLabel(pht('Key'))
-        ->setValue(phutil_tag('strong', array(), $key)));
+        ->setValue(phutil_tag('strong', array(), $secret)));
 
     $form->appendInstructions(
       pht(
@@ -526,4 +479,11 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
 
     return $value;
   }
+
+  protected function newMFASyncTokenProperties(PhabricatorUser $user) {
+    return array(
+      'secret' => self::generateNewTOTPKey(),
+    );
+  }
+
 }

src/applications/auth/storage/PhabricatorAuthFactorConfig.php

@@ -15,6 +15,7 @@ final class PhabricatorAuthFactorConfig
 
   private $sessionEngine;
   private $factorProvider = self::ATTACHABLE;
+  private $mfaSyncToken;
 
   protected function getConfiguration() {
     return array(
@@ -61,6 +62,15 @@ final class PhabricatorAuthFactorConfig
     return $this->sessionEngine;
   }
 
+  public function setMFASyncToken(PhabricatorAuthTemporaryToken $token) {
+    $this->mfaSyncToken = $token;
+    return $this;
+  }
+
+  public function getMFASyncToken() {
+    return $this->mfaSyncToken;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 

src/applications/auth/storage/PhabricatorAuthTemporaryToken.php

@@ -10,7 +10,9 @@ final class PhabricatorAuthTemporaryToken extends PhabricatorAuthDAO
   protected $tokenExpires;
   protected $tokenCode;
   protected $userPHID;
-  protected $properties;
+  protected $properties = array();
+
+  private $isNew = false;
 
   protected function getConfiguration() {
     return array(
@@ -114,6 +116,14 @@ final class PhabricatorAuthTemporaryToken extends PhabricatorAuthDAO
     return $this->getTemporaryTokenProperty('force-full-session', false);
   }
 
+  public function setIsNewTemporaryToken($is_new) {
+    $this->isNew = $is_new;
+    return $this;
+  }
+
+  public function getIsNewTemporaryToken() {
+    return $this->isNew;
+  }
 
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */

src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php

@@ -266,6 +266,13 @@ final class PhabricatorMultiFactorSettingsPanel
 
       $config->save();
 
+      // If we used a temporary token to handle synchronizing the factor,
+      // revoke it now.
+      $sync_token = $config->getMFASyncToken();
+      if ($sync_token) {
+        $sync_token->revokeToken();
+      }
+
       $log = PhabricatorUserLog::initializeNewLog(
         $viewer,
         $user->getPHID(),