Page MenuHomePhabricator
Files F13259060

D19894.id47502.diff
No OneTemporary
Actions

D19894.id47502.diff
View Options

diff --git a/resources/sql/autopatches/20181217.auth.01.digest.sql b/resources/sql/autopatches/20181217.auth.01.digest.sql
new file mode 100644
--- /dev/null
+++ b/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};
diff --git a/resources/sql/autopatches/20181217.auth.02.ttl.sql b/resources/sql/autopatches/20181217.auth.02.ttl.sql
new file mode 100644
--- /dev/null
+++ b/resources/sql/autopatches/20181217.auth.02.ttl.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_challenge
+ ADD responseTTL INT UNSIGNED;
diff --git a/resources/sql/autopatches/20181217.auth.03.completed.sql b/resources/sql/autopatches/20181217.auth.03.completed.sql
new file mode 100644
--- /dev/null
+++ b/resources/sql/autopatches/20181217.auth.03.completed.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_challenge
+ ADD isCompleted BOOL NOT NULL;
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -576,6 +576,8 @@
continue;
}
+ $issued_challenges = idx($challenge_map, $factor_phid, array());
+
$impl = $factor->requireImplementation();
$validation_result = $impl->getResultFromChallengeResponse(
@@ -592,6 +594,16 @@
}
if ($ok) {
+ // We're letting you through, so mark all the challenges you
+ // responded to as completed. These challenges may 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()),
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
@@ -45,7 +45,7 @@
$engine = $config->getSessionEngine();
- return id(new PhabricatorAuthChallenge())
+ return PhabricatorAuthChallenge::initializeNewChallenge()
->setUserPHID($viewer->getPHID())
->setSessionPHID($viewer->getSession()->getPHID())
->setFactorPHID($config->getPHID())
diff --git a/src/applications/auth/factor/PhabricatorAuthFactorResult.php b/src/applications/auth/factor/PhabricatorAuthFactorResult.php
--- a/src/applications/auth/factor/PhabricatorAuthFactorResult.php
+++ b/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) {
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
@@ -283,6 +283,17 @@
'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 @@
(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');
diff --git a/src/applications/auth/storage/PhabricatorAuthChallenge.php b/src/applications/auth/storage/PhabricatorAuthChallenge.php
--- a/src/applications/auth/storage/PhabricatorAuthChallenge.php
+++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php
@@ -10,8 +10,20 @@
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 @@
'challengeKey' => 'text255',
'challengeTTL' => 'epoch',
'workflowKey' => 'text255',
+ 'responseDigest' => 'text255?',
+ 'responseTTL' => 'epoch?',
+ 'isCompleted' => 'bool',
),
self::CONFIG_KEY_SCHEMA => array(
'key_issued' => array(
@@ -38,6 +53,104 @@
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 )----------------------------------------- */

File Metadata

Mime Type
text/plain
Expires
Mon, May 27, 3:58 PM (4 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6309539
Default Alt Text
D19894.id47502.diff (9 KB)

Event Timeline

Phabricator · Built by Phacility