From 8107fddd8fe3d5e6e0411f3df0a62f55e8b0c999 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 19 Apr 2019 16:18:01 -0700
Subject: [PATCH] rdbms: add "secret" parameter to ChronologyProtector to use
 HMAC client IDs

Also make $posIndex mandatory and clean up some IDE warnings in LBFactory.

Change-Id: I9e686b670bc86eb377f14ca57a94e1aa3fd901d5
---
 includes/libs/rdbms/ChronologyProtector.php  | 14 ++++++++++----
 includes/libs/rdbms/lbfactory/ILBFactory.php |  1 +
 includes/libs/rdbms/lbfactory/LBFactory.php  | 12 +++++++++---
 tests/phpunit/includes/db/LBFactoryTest.php  |  3 ++-
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php
index 62a296827d..fa454c88d1 100644
--- a/includes/libs/rdbms/ChronologyProtector.php
+++ b/includes/libs/rdbms/ChronologyProtector.php
@@ -73,13 +73,19 @@ class ChronologyProtector implements LoggerAwareInterface {
 	/**
 	 * @param BagOStuff $store
 	 * @param array $client Map of (ip: <IP>, agent: <user-agent> [, clientId: <hash>] )
-	 * @param int|null $posIndex Write counter index [optional]
+	 * @param int|null $posIndex Write counter index
+	 * @param string $secret Secret string for HMAC hashing [optional]
 	 * @since 1.27
 	 */
-	public function __construct( BagOStuff $store, array $client, $posIndex = null ) {
+	public function __construct( BagOStuff $store, array $client, $posIndex, $secret = '' ) {
 		$this->store = $store;
-		$this->clientId = $client['clientId'] ??
-			md5( $client['ip'] . "\n" . $client['agent'] );
+		if ( isset( $client['clientId'] ) ) {
+			$this->clientId = $client['clientId'];
+		} else {
+			$this->clientId = strlen( $secret )
+				? hash_hmac( 'md5', $client['ip'] . "\n" . $client['agent'], $secret )
+				: md5( $client['ip'] . "\n" . $client['agent'] );
+		}
 		$this->key = $store->makeGlobalKey( __CLASS__, $this->clientId, 'v2' );
 		$this->waitForPosIndex = $posIndex;
 
diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php
index cb8be212b8..682561b651 100644
--- a/includes/libs/rdbms/lbfactory/ILBFactory.php
+++ b/includes/libs/rdbms/lbfactory/ILBFactory.php
@@ -57,6 +57,7 @@ interface ILBFactory {
 	 *  - perfLogger: PSR-3 logger instance. [optional]
 	 *  - errorLogger: Callback that takes an Exception and logs it. [optional]
 	 *  - deprecationLogger: Callback to log a deprecation warning. [optional]
+	 *  - secret: Secret string to use for HMAC hashing [optional]
 	 * @throws InvalidArgumentException
 	 */
 	public function __construct( array $conf );
diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php
index 4ea2eb9036..fc0606d429 100644
--- a/includes/libs/rdbms/lbfactory/LBFactory.php
+++ b/includes/libs/rdbms/lbfactory/LBFactory.php
@@ -24,6 +24,7 @@
 namespace Wikimedia\Rdbms;
 
 use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use Wikimedia\ScopedCallback;
 use BagOStuff;
 use EmptyBagOStuff;
@@ -74,6 +75,8 @@ abstract class LBFactory implements ILBFactory {
 	private $cliMode;
 	/** @var string Agent name for query profiling */
 	private $agent;
+	/** @var string Secret string for HMAC hashing */
+	private $secret;
 
 	/** @var array[] $aliases Map of (table => (dbname, schema, prefix) map) */
 	private $tableAliases = [];
@@ -123,7 +126,7 @@ abstract class LBFactory implements ILBFactory {
 		$this->wanCache = $conf['wanCache'] ?? WANObjectCache::newEmpty();
 
 		foreach ( self::$loggerFields as $key ) {
-			$this->$key = $conf[$key] ?? new \Psr\Log\NullLogger();
+			$this->$key = $conf[$key] ?? new NullLogger();
 		}
 		$this->errorLogger = $conf['errorLogger'] ?? function ( Exception $e ) {
 			trigger_error( get_class( $e ) . ': ' . $e->getMessage(), E_USER_WARNING );
@@ -148,6 +151,7 @@ abstract class LBFactory implements ILBFactory {
 		$this->hostname = $conf['hostname'] ?? gethostname();
 		$this->agent = $conf['agent'] ?? '';
 		$this->defaultGroup = $conf['defaultGroup'] ?? null;
+		$this->secret = $conf['secret'] ?? '';
 
 		$this->ticket = mt_rand();
 	}
@@ -463,7 +467,7 @@ abstract class LBFactory implements ILBFactory {
 			$this->perfLogger->error( __METHOD__ . ": $fname does not have outer scope.\n" .
 				( new RuntimeException() )->getTraceAsString() );
 
-			return;
+			return false;
 		}
 
 		// The transaction owner and any caller with the empty transaction ticket can commit
@@ -482,6 +486,7 @@ abstract class LBFactory implements ILBFactory {
 		if ( $fnameEffective !== $fname ) {
 			$this->beginMasterChanges( $fnameEffective );
 		}
+
 		return $waitSucceeded;
 	}
 
@@ -508,7 +513,8 @@ abstract class LBFactory implements ILBFactory {
 				'agent' => $this->requestInfo['UserAgent'],
 				'clientId' => $this->requestInfo['ChronologyClientId']
 			],
-			$this->requestInfo['ChronologyPositionIndex']
+			$this->requestInfo['ChronologyPositionIndex'],
+			$this->secret
 		);
 		$this->chronProt->setLogger( $this->replLogger );
 
diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php
index 7d13ac6ea6..b79cdf3896 100644
--- a/tests/phpunit/includes/db/LBFactoryTest.php
+++ b/tests/phpunit/includes/db/LBFactoryTest.php
@@ -347,7 +347,8 @@ class LBFactoryTest extends MediaWikiTestCase {
 			[
 				'ip' => '127.0.0.1',
 				'agent' => "Totally-Not-FireFox"
-			]
+			],
+			null
 		);
 
 		$mockDB1->expects( $this->exactly( 1 ) )->method( 'writesOrCallbacksPending' );
-- 
2.20.1