revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-20 01:08:50 +02:00
Code Issues Releases Wiki Activity

Make partial sessions expire after 30 minutes, and do not extend them

Browse source 
Summary:
Depends on D19904. Ref T13226. Ref T13222. Currently, partial sessions (where you've provided a primary auth factor like a password, but not yet provided MFA) work like normal sessions: they're good for 30 days and extend indefinitely under regular use.

This behavior is convenient for full sessions, but normal users don't ever spend 30 minutes answering MFA, so there's no real reason to do it for partial sessions. If we add login alerts in the future, limiting partial sessions to a short lifetime will make them more useful, since an attacker can't get one partial session and keep extending it forever while waiting for an opportunity to get past your MFA.

Test Plan:
  - Did a partial login (to the MFA prompt), checked database, saw a ~29 minute partial session.
  - Did a full login, saw session extend to ~30 days.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13226, T13222

Differential Revision: https://secure.phabricator.com/D19905
This commit is contained in:
epriestley 2018-12-18 11:54:05 -08:00
parent 38c48ae7d0
commit ca39be6091
2 changed files with 46 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

61
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
View file

@ -213,26 +213,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
$session = id(new PhabricatorAuthSession())->loadFromArray($session_dict);
$ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
// If more than 20% of the time on this session has been used, refresh the
// TTL back up to the full duration. The idea here is that sessions are
// good forever if used regularly, but get GC'd when they fall out of use.
// NOTE: If we begin rotating session keys when extending sessions, the
// CSRF code needs to be updated so CSRF tokens survive session rotation.
if (time() + (0.80 * $ttl) > $session->getSessionExpires()) {
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
$conn_w = $session_table->establishConnection('w');
queryfx(
$conn_w,
'UPDATE %T SET sessionExpires = UNIX_TIMESTAMP() + %d WHERE id = %d',
$session->getTableName(),
$ttl,
$session->getID());
unset($unguarded);
}
$this->extendSession($session);
// TODO: Remove this, see T13225.
if ($is_weak) {
@ -284,7 +265,9 @@ final class PhabricatorAuthSessionEngine extends Phobject {
$conn_w = $session_table->establishConnection('w');
// This has a side effect of validating the session type.
$session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
$session_ttl = PhabricatorAuthSession::getSessionTypeTTL(
$session_type,
$partial);
$digest_key = PhabricatorAuthSession::newSessionDigest(
new PhutilOpaqueEnvelope($session_key));
@ -1086,4 +1069,40 @@ final class PhabricatorAuthSessionEngine extends Phobject {
}
}
private function extendSession(PhabricatorAuthSession $session) {
$is_partial = $session->getIsPartial();
// Don't extend partial sessions. You have a relatively short window to
// upgrade into a full session, and your session expires otherwise.
if ($is_partial) {
return;
}
$session_type = $session->getType();
$ttl = PhabricatorAuthSession::getSessionTypeTTL(
$session_type,
$session->getIsPartial());
// If more than 20% of the time on this session has been used, refresh the
// TTL back up to the full duration. The idea here is that sessions are
// good forever if used regularly, but get GC'd when they fall out of use.
$now = PhabricatorTime::getNow();
if ($now + (0.80 * $ttl) <= $session->getSessionExpires()) {
return;
}
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
queryfx(
$session->establishConnection('w'),
'UPDATE %R SET sessionExpires = UNIX_TIMESTAMP() + %d
WHERE id = %d',
$session,
$ttl,
$session->getID());
unset($unguarded);
}
}

8
src/applications/auth/storage/PhabricatorAuthSession.php
View file

@ -72,10 +72,14 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
return $this->assertAttached($this->identityObject);
}
public static function getSessionTypeTTL($session_type) {
public static function getSessionTypeTTL($session_type, $is_partial) {
switch ($session_type) {
case self::TYPE_WEB:
return phutil_units('30 days in seconds');
if ($is_partial) {
return phutil_units('30 minutes in seconds');
} else {
return phutil_units('30 days in seconds');
}
case self::TYPE_CONDUIT:
return phutil_units('24 hours in seconds');
default: