Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F13180048
D20020.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
14 KB
Referenced Files
None
Subscribers
None
D20020.diff
View Options
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2265,6 +2265,7 @@
'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 @@
'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 @@
'PhabricatorAuthLoginMessageType' => 'PhabricatorAuthMessageType',
'PhabricatorAuthLogoutConduitAPIMethod' => 'PhabricatorAuthConduitAPIMethod',
'PhabricatorAuthMFAEditEngineExtension' => 'PhabricatorEditEngineExtension',
+ 'PhabricatorAuthMFASyncTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
'PhabricatorAuthMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension',
'PhabricatorAuthManagementCachePKCS8Workflow' => 'PhabricatorAuthManagementWorkflow',
'PhabricatorAuthManagementLDAPWorkflow' => 'PhabricatorAuthManagementWorkflow',
@@ -8101,7 +8102,6 @@
'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController',
'PhabricatorAuthSetupCheck' => 'PhabricatorSetupCheck',
'PhabricatorAuthStartController' => 'PhabricatorAuthController',
- 'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
'PhabricatorAuthTemporaryToken' => array(
'PhabricatorAuthDAO',
'PhabricatorPolicyInterface',
diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php
--- a/src/applications/auth/factor/PhabricatorAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorAuthFactor.php
@@ -245,4 +245,104 @@
}
+/* -( 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');
+ }
+
}
diff --git a/src/applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php b/src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php
rename from src/applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php
rename to src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php
--- a/src/applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php
+++ b/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');
}
}
diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
--- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
+++ b/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 @@
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 @@
}
}
- $form->addHiddenInput('totp', true);
- $form->addHiddenInput('totpkey', $key);
-
$form->appendRemarkupInstructions(
pht(
'First, download an authenticator application on your phone. Two '.
@@ -126,7 +79,7 @@
'otpauth://totp/%s:%s?secret=%s&issuer=%s',
$issuer,
$user->getUsername(),
- $key,
+ $secret,
$issuer);
$qrcode = $this->renderQRCode($uri);
@@ -135,7 +88,7 @@
$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 @@
return $value;
}
+
+ protected function newMFASyncTokenProperties(PhabricatorUser $user) {
+ return array(
+ 'secret' => self::generateNewTOTPKey(),
+ );
+ }
+
}
diff --git a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php
--- a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php
+++ b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php
@@ -15,6 +15,7 @@
private $sessionEngine;
private $factorProvider = self::ATTACHABLE;
+ private $mfaSyncToken;
protected function getConfiguration() {
return array(
@@ -61,6 +62,15 @@
return $this->sessionEngine;
}
+ public function setMFASyncToken(PhabricatorAuthTemporaryToken $token) {
+ $this->mfaSyncToken = $token;
+ return $this;
+ }
+
+ public function getMFASyncToken() {
+ return $this->mfaSyncToken;
+ }
+
/* -( PhabricatorPolicyInterface )----------------------------------------- */
diff --git a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php
--- a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php
+++ b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php
@@ -10,7 +10,9 @@
protected $tokenExpires;
protected $tokenCode;
protected $userPHID;
- protected $properties;
+ protected $properties = array();
+
+ private $isNew = false;
protected function getConfiguration() {
return array(
@@ -114,6 +116,14 @@
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 )----------------------------------------- */
diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
--- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
@@ -266,6 +266,13 @@
$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(),
File Metadata
Details
Attached
Mime Type
text/plain
Expires
May 9 2024, 9:59 PM (5 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6277553
Default Alt Text
D20020.diff (14 KB)
Attached To
Mode
D20020: Allow different MFA factor types (SMS, TOTP, Duo, ...) to share "sync" tokens when enrolling new factors
Attached
Detach File
Event Timeline
Log In to Comment