From: Aaron Schulz Date: Fri, 25 May 2018 07:04:23 +0000 (-0700) Subject: rdbms: include client ID hash in ChronologyProtector cookies X-Git-Tag: 1.34.0-rc.0~5196^2 X-Git-Url: http://git.cyclocoop.org/data/Fool?a=commitdiff_plain;h=fb51330084b4bde1880c76589e55e7cd87ed0c6d;p=lhc%2Fweb%2Fwiklou.git rdbms: include client ID hash in ChronologyProtector cookies Previously, if an internal service forwarded the cookies for a user (e.g. for permissions) but not the User-Agent header or not the IP address (e.g. XFF), ChronologyProtector could timeout waiting for a matching writeIndex to appear for the wrong key. The cookie now tethers the client to the key that holds the DB positions from their last state-changing request. Bug: T194403 Bug: T190082 Change-Id: I84f2cbea82532d911cdfed14644008894498813a --- diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index 1642377e34..79787272eb 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -631,7 +631,8 @@ class MediaWiki { // Record ChronologyProtector positions for DBs affected in this request at this point $cpIndex = null; - $lbFactory->shutdown( $flags, $postCommitWork, $cpIndex ); + $cpClientId = null; + $lbFactory->shutdown( $flags, $postCommitWork, $cpIndex, $cpClientId ); wfDebug( __METHOD__ . ': LBFactory shutdown completed' ); if ( $cpIndex > 0 ) { @@ -639,7 +640,7 @@ class MediaWiki { $now = time(); $expires = $now + ChronologyProtector::POSITION_COOKIE_TTL; $options = [ 'prefix' => '' ]; - $value = LBFactory::makeCookieValueFromCPIndex( $cpIndex, $now ); // T190082 + $value = LBFactory::makeCookieValueFromCPIndex( $cpIndex, $now, $cpClientId ); $request->response()->setCookie( 'cpPosIndex', $value, $expires, $options ); } diff --git a/includes/Setup.php b/includes/Setup.php index 3cc52f8e25..512ecfc6e2 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -730,20 +730,20 @@ if ( !$wgDBerrorLogTZ ) { // Initialize the request object in $wgRequest $wgRequest = RequestContext::getMain()->getRequest(); // BackCompat // Set user IP/agent information for agent session consistency purposes +$cpPosInfo = LBFactory::getCPInfoFromCookieValue( + // The cookie has no prefix and is set by MediaWiki::preOutputCommit() + $wgRequest->getCookie( 'cpPosIndex', '' ), + // Mitigate broken client-side cookie expiration handling (T190082) + time() - ChronologyProtector::POSITION_COOKIE_TTL +); MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->setRequestInfo( [ 'IPAddress' => $wgRequest->getIP(), 'UserAgent' => $wgRequest->getHeader( 'User-Agent' ), 'ChronologyProtection' => $wgRequest->getHeader( 'ChronologyProtection' ), - 'ChronologyPositionIndex' => $wgRequest->getInt( - 'cpPosIndex', - LBFactory::getCPIndexFromCookieValue( - // The cookie has no prefix and is set by MediaWiki::preOutputCommit() - $wgRequest->getCookie( 'cpPosIndex', '' ), - // Mitigate broken client-side cookie expiration handling (T190082) - time() - ChronologyProtector::POSITION_COOKIE_TTL - ) - ) + 'ChronologyPositionIndex' => $wgRequest->getInt( 'cpPosIndex', $cpPosInfo['index'] ), + 'ChronologyClientId' => $cpPosInfo['clientId'] ] ); +unset( $cpPosInfo ); // Make sure that object caching does not undermine the ChronologyProtector improvements if ( $wgRequest->getCookie( 'UseDC', '' ) === 'master' ) { // The user is pinned to the primary DC, meaning that they made recent changes which should diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php index 099f172e7f..f5cef9eab7 100644 --- a/includes/libs/rdbms/ChronologyProtector.php +++ b/includes/libs/rdbms/ChronologyProtector.php @@ -70,13 +70,15 @@ class ChronologyProtector implements LoggerAwareInterface { /** * @param BagOStuff $store - * @param array[] $client Map of (ip: , agent: ) + * @param array[] $client Map of (ip: , agent: [, clientId: ] ) * @param int|null $posIndex Write counter index [optional] * @since 1.27 */ public function __construct( BagOStuff $store, array $client, $posIndex = null ) { $this->store = $store; - $this->clientId = md5( $client['ip'] . "\n" . $client['agent'] ); + $this->clientId = isset( $client['clientId'] ) + ? $client['clientId'] + : md5( $client['ip'] . "\n" . $client['agent'] ); $this->key = $store->makeGlobalKey( __CLASS__, $this->clientId, 'v2' ); $this->waitForPosIndex = $posIndex; $this->logger = new NullLogger(); @@ -86,6 +88,14 @@ class ChronologyProtector implements LoggerAwareInterface { $this->logger = $logger; } + /** + * @return string Client ID hash + * @since 1.32 + */ + public function getClientId() { + return $this->clientId; + } + /** * @param bool $enabled Whether to no-op all method calls * @since 1.27 diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php index 16d0e31e27..85ab1157c5 100644 --- a/includes/libs/rdbms/lbfactory/ILBFactory.php +++ b/includes/libs/rdbms/lbfactory/ILBFactory.php @@ -142,9 +142,13 @@ interface ILBFactory { * @param int $mode One of the class SHUTDOWN_* constants * @param callable|null $workCallback Work to mask ChronologyProtector writes * @param int|null &$cpIndex Position key write counter for ChronologyProtector + * @param string|null &$cpClientId Client ID hash for ChronologyProtector */ public function shutdown( - $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null, &$cpIndex = null + $mode = self::SHUTDOWN_CHRONPROT_SYNC, + callable $workCallback = null, + &$cpIndex = null, + &$cpClientId = null ); /** diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index 2ee3419e2c..52c2df7a26 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -130,9 +130,9 @@ abstract class LBFactory implements ILBFactory { $this->requestInfo = [ 'IPAddress' => $_SERVER[ 'REMOTE_ADDR' ] ?? '', 'UserAgent' => $_SERVER['HTTP_USER_AGENT'] ?? '', - 'ChronologyProtection' => 'true', - // phpcs:ignore MediaWiki.Usage.SuperGlobalsUsage.SuperGlobals -- library can't use $wgRequest - 'ChronologyPositionIndex' => $_GET['cpPosIndex'] ?? null + // Headers application can inject via LBFactory::setRequestInfo() + 'ChronologyClientId' => null, // prior $cpClientId value from LBFactory::shutdown() + 'ChronologyPositionIndex' => null // prior $cpIndex value from LBFactory::shutdown() ]; $this->cliMode = $conf['cliMode'] ?? ( PHP_SAPI === 'cli' || PHP_SAPI === 'phpdbg' ); @@ -148,7 +148,10 @@ abstract class LBFactory implements ILBFactory { } public function shutdown( - $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null, &$cpIndex = null + $mode = self::SHUTDOWN_CHRONPROT_SYNC, + callable $workCallback = null, + &$cpIndex = null, + &$cpClientId = null ) { $chronProt = $this->getChronologyProtector(); if ( $mode === self::SHUTDOWN_CHRONPROT_SYNC ) { @@ -157,6 +160,8 @@ abstract class LBFactory implements ILBFactory { $this->shutdownChronologyProtector( $chronProt, null, 'async', $cpIndex ); } + $cpClientId = $chronProt->getClientId(); + $this->commitMasterChanges( __METHOD__ ); // sanity } @@ -488,6 +493,7 @@ abstract class LBFactory implements ILBFactory { [ 'ip' => $this->requestInfo['IPAddress'], 'agent' => $this->requestInfo['UserAgent'], + 'clientId' => $this->requestInfo['ChronologyClientId'] ], $this->requestInfo['ChronologyPositionIndex'] ); @@ -633,32 +639,38 @@ abstract class LBFactory implements ILBFactory { /** * @param int $index Write index - * @param int $time UNIX timestamp - * @return string Timestamp-qualified write index of the form "." + * @param int $time UNIX timestamp; can be used to detect stale cookies (T190082) + * @param string $clientId Agent ID hash from ILBFactory::shutdown() + * @return string Timestamp-qualified write index of the form "@#" * @since 1.32 */ - public static function makeCookieValueFromCPIndex( $index, $time ) { - return $index . '@' . $time; + public static function makeCookieValueFromCPIndex( $index, $time, $clientId ) { + return "$index@$time#$clientId"; } /** - * @param string $value String possibly of the form "" or "@" + * @param string $value Possible result of LBFactory::makeCookieValueFromCPIndex() * @param int $minTimestamp Lowest UNIX timestamp of non-expired values (if present) - * @return int|null Write index or null if $value is empty or expired + * @return array (index: int or null, clientId: string or null) * @since 1.32 */ - public static function getCPIndexFromCookieValue( $value, $minTimestamp ) { - if ( !preg_match( '/^(\d+)(?:@(\d+))?$/', $value, $m ) ) { - return null; + public static function getCPInfoFromCookieValue( $value, $minTimestamp ) { + static $placeholder = [ 'index' => null, 'clientId' => null ]; + + if ( !preg_match( '/^(\d+)(?:@(\d+))?(?:#([0-9a-f]{32}))?$/', $value, $m ) ) { + return $placeholder; // invalid } $index = (int)$m[1]; - - if ( isset( $m[2] ) && $m[2] !== '' && (int)$m[2] < $minTimestamp ) { - return null; // expired + if ( $index <= 0 ) { + return $placeholder; // invalid + } elseif ( isset( $m[2] ) && $m[2] !== '' && (int)$m[2] < $minTimestamp ) { + return $placeholder; // expired } - return ( $index > 0 ) ? $index : null; + $clientId = ( isset( $m[3] ) && $m[3] !== '' ) ? $m[3] : null; + + return [ 'index' => $index, 'clientId' => $clientId ]; } public function setRequestInfo( array $info ) { diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 0395bff2b4..fac34867d8 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -393,6 +393,8 @@ class LBFactoryTest extends MediaWikiTestCase { $cp->shutdown( null, 'sync', $cpIndex ); $this->assertEquals( null, $cpIndex, "CP write index retained" ); + + $this->assertEquals( '45e93a9c215c031d38b7c42d8e4700ca', $cp->getClientId() ); } private function newLBFactoryMulti( array $baseOverride = [], array $serverOverride = [] ) { @@ -419,7 +421,8 @@ class LBFactoryTest extends MediaWikiTestCase { 'test-db1' => $wgDBserver, ], 'loadMonitorClass' => LoadMonitorNull::class, - 'localDomain' => new DatabaseDomain( $wgDBname, null, $wgDBprefix ) + 'localDomain' => new DatabaseDomain( $wgDBname, null, $wgDBprefix ), + 'agent' => 'MW-UNIT-TESTS' ] ); } @@ -609,35 +612,65 @@ class LBFactoryTest extends MediaWikiTestCase { /** * @covers \Wikimedia\Rdbms\LBFactory::makeCookieValueFromCPIndex() - * @covers \Wikimedia\Rdbms\LBFactory::getCPIndexFromCookieValue() + * @covers \Wikimedia\Rdbms\LBFactory::getCPInfoFromCookieValue() */ public function testCPPosIndexCookieValues() { - $this->assertEquals( '3@542', LBFactory::makeCookieValueFromCPIndex( 3, 542 ) ); - $time = 1526522031; + $agentId = md5( 'Ramsey\'s Loyal Presa Canario' ); + + $lbFactory = $this->newLBFactoryMulti(); + $this->assertEquals( + '3@542#c47dcfb0566e7d7bc110a6128a45c93a', + LBFactory::makeCookieValueFromCPIndex( 3, 542, $agentId ) + ); + + $lbFactory = $this->newLBFactoryMulti(); + $lbFactory->setRequestInfo( [ 'IPAddress' => '10.64.24.52', 'UserAgent' => 'meow' ] ); + $this->assertEquals( + '1@542#c47dcfb0566e7d7bc110a6128a45c93a', + LBFactory::makeCookieValueFromCPIndex( 1, 542, $agentId ) + ); $this->assertSame( 5, - LBFactory::getCPIndexFromCookieValue( "5", $time - 10 ) + LBFactory::getCPInfoFromCookieValue( "5", $time - 10 )['index'], + 'No time set' ); $this->assertSame( null, - LBFactory::getCPIndexFromCookieValue( "0", $time - 10 ) + LBFactory::getCPInfoFromCookieValue( "0", $time - 10 )['index'], + 'Bad index' ); + $this->assertSame( 2, - LBFactory::getCPIndexFromCookieValue( "2@$time", $time - 10 ) + LBFactory::getCPInfoFromCookieValue( "2@$time", $time - 10 )['index'], + 'Fresh' ); $this->assertSame( 2, - LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 9 - 10 ) + LBFactory::getCPInfoFromCookieValue( "2@$time", $time + 9 - 10 )['index'], + 'Almost stale' + ); + $this->assertSame( + null, + LBFactory::getCPInfoFromCookieValue( "0@$time", $time + 9 - 10 )['index'], + 'Almost stale; bad index' ); $this->assertSame( null, - LBFactory::getCPIndexFromCookieValue( "0@$time", $time + 9 - 10 ) + LBFactory::getCPInfoFromCookieValue( "2@$time", $time + 11 - 10 )['index'], + 'Stale' + ); + + $this->assertSame( + $agentId, + LBFactory::getCPInfoFromCookieValue( "5@$time#$agentId", $time - 10 )['clientId'], + 'Live (client ID)' ); $this->assertSame( null, - LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 11 - 10 ) + LBFactory::getCPInfoFromCookieValue( "5@$time", $time + 11 - 10 )['clientId'], + 'Stale (client ID)' ); } }