From 4bfbf2c225d7f45aff167613f35d7d9153abe252 Mon Sep 17 00:00:00 2001 From: Bill Pirkle Date: Thu, 26 Sep 2019 12:59:15 -0500 Subject: [PATCH] Add optional serialization_type and hmac_key param values to RESTBagOStuff T233537 made RESTBagOStuff work with the Kask external session storage service, but broke backward compatibility. Add optional values to the RESTBagOStuff $params constructor parameter to support communicating with Kask, and to allow using HMAC. The new values are: serialization_type: legacy (default), PHP, or JSON hmac_key: HMAC key to use for protecting the serialized blob If these new values are not specified, behavior remains unchanged (PHP serialization with no HMAC protection). Bug: T233963 Change-Id: Ia2625c04e08cfe9616569500f1d613be73c170a2 (cherry picked from commit 2ed69af15cb8971e35aafc2c08397ed38c480488) --- RELEASE-NOTES-1.34 | 1 + includes/libs/objectcache/RESTBagOStuff.php | 85 +++++++++++++++++-- .../objectcache/RESTBagOStuffTest.php | 58 +++++++++++-- 3 files changed, 133 insertions(+), 11 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 71d1a41874..e98af0acd2 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -604,6 +604,7 @@ because of Phabricator reports. * ApiQueryBase::showHiddenUsersAddBlockInfo() is deprecated. Use ApiQueryBlockInfoTrait instead. * PasswordReset is now a service, its direct instantiation is deprecated. +* RESTBagOStuff users should specify either "JSON" or "PHP" serialization type. * Language::getLocalisationCache() is deprecated. Use MediaWikiServices instead. diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php index 82b5ac05bf..aa79b37369 100644 --- a/includes/libs/objectcache/RESTBagOStuff.php +++ b/includes/libs/objectcache/RESTBagOStuff.php @@ -38,6 +38,7 @@ use Psr\Log\LoggerInterface; * 'writeHeaders' => [ 'content-type' => 'application/octet-stream' ], * 'deleteHeaders' => [], * 'writeMethod' => 'POST', + * 'serialization_type' => 'JSON', * ], * 'extendedErrorBodyFields' => [ 'type', 'title', 'detail', 'instance' ] * ); @@ -73,6 +74,22 @@ class RESTBagOStuff extends MediumSpecificBagOStuff { */ private $httpParams; + /** + * Optional serialization type to use. Allowed values: "PHP", "JSON", or "legacy". + * "legacy" is PHP serialization with no serialization type tagging or hmac protection. + * @var string + * @deprecated since 1.34, the "legacy" value will be removed in 1.35. + * Use either "PHP" or "JSON". + */ + private $serializationType; + + /** + * Optional HMAC Key for protecting the serialized blob. If omitted, or if serializationType + * is "legacy", then no protection is done + * @var string + */ + private $hmacKey; + /** * @var array additional body fields to log on error, if possible */ @@ -105,6 +122,8 @@ class RESTBagOStuff extends MediumSpecificBagOStuff { $this->httpParams['writeHeaders'] = $params['httpParams']['writeHeaders'] ?? []; $this->httpParams['deleteHeaders'] = $params['httpParams']['deleteHeaders'] ?? []; $this->extendedErrorBodyFields = $params['extendedErrorBodyFields'] ?? []; + $this->serializationType = $params['serialization_type'] ?? 'legacy'; + $this->hmacKey = $params['hmac_key'] ?? ''; // The parent constructor calls setLogger() which sets the logger in $this->client parent::__construct( $params ); @@ -211,22 +230,76 @@ class RESTBagOStuff extends MediumSpecificBagOStuff { * @return mixed|bool the processed body, or false on error */ private function decodeBody( $body ) { - $value = json_decode( $body, true ); - return ( json_last_error() === JSON_ERROR_NONE ) ? $value : false; + if ( $this->serializationType === 'legacy' ) { + $serialized = $body; + } else { + $pieces = explode( '.', $body, 3 ); + if ( count( $pieces ) !== 3 || $pieces[0] !== $this->serializationType ) { + return false; + } + list( , $hmac, $serialized ) = $pieces; + if ( $this->hmacKey !== '' ) { + $checkHmac = hash_hmac( 'sha256', $serialized, $this->hmacKey, true ); + if ( !hash_equals( $checkHmac, base64_decode( $hmac ) ) ) { + return false; + } + } + } + + switch ( $this->serializationType ) { + case 'JSON': + $value = json_decode( $serialized, true ); + return ( json_last_error() === JSON_ERROR_NONE ) ? $value : false; + + case 'PHP': + case 'legacy': + return unserialize( $serialized ); + + default: + throw new \DomainException( + "Unknown serialization type: $this->serializationType" + ); + } } /** * Prepares the request body (the "value" portion of our key/value store) for transmission. * * @param string $body request body to prepare - * @return string the prepared body, or an empty string on error + * @return string the prepared body * @throws LogicException */ private function encodeBody( $body ) { - $value = json_encode( $body ); - if ( $value === false ) { - throw new InvalidArgumentException( __METHOD__ . ": body could not be encoded." ); + switch ( $this->serializationType ) { + case 'JSON': + $value = json_encode( $body ); + if ( $value === false ) { + throw new InvalidArgumentException( __METHOD__ . ": body could not be encoded." ); + } + break; + + case 'PHP': + case "legacy": + $value = serialize( $body ); + break; + + default: + throw new \DomainException( + "Unknown serialization type: $this->serializationType" + ); + } + + if ( $this->serializationType !== 'legacy' ) { + if ( $this->hmacKey !== '' ) { + $hmac = base64_encode( + hash_hmac( 'sha256', $value, $this->hmacKey, true ) + ); + } else { + $hmac = ''; + } + $value = $this->serializationType . '.' . $hmac . '.' . $value; } + return $value; } diff --git a/tests/phpunit/unit/includes/objectcache/RESTBagOStuffTest.php b/tests/phpunit/unit/includes/objectcache/RESTBagOStuffTest.php index 459e3eebc2..689ac21df0 100644 --- a/tests/phpunit/unit/includes/objectcache/RESTBagOStuffTest.php +++ b/tests/phpunit/unit/includes/objectcache/RESTBagOStuffTest.php @@ -25,17 +25,41 @@ class RESTBagOStuffTest extends \MediaWikiUnitTestCase { $this->bag = new RESTBagOStuff( [ 'client' => $this->client, 'url' => 'http://test/rest/' ] ); } - public function testGet() { + /** + * @dataProvider dataGet + */ + public function testGet( $serializationType, $hmacKey, $data ) { + $classReflect = new ReflectionClass( RESTBagOStuff::class ); + + $serializationTypeReflect = $classReflect->getProperty( 'serializationType' ); + $serializationTypeReflect->setAccessible( true ); + $serializationTypeReflect->setValue( $this->bag, $serializationType ); + + $hmacKeyReflect = $classReflect->getProperty( 'hmacKey' ); + $hmacKeyReflect->setAccessible( true ); + $hmacKeyReflect->setValue( $this->bag, $hmacKey ); + $this->client->expects( $this->once() )->method( 'run' )->with( [ 'method' => 'GET', 'url' => 'http://test/rest/42xyz42', 'headers' => [] // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) - ] )->willReturn( [ 200, 'OK', [], '"somedata"', 0 ] ); + ] )->willReturn( [ 200, 'OK', [], $data, 0 ] ); $result = $this->bag->get( '42xyz42' ); $this->assertEquals( 'somedata', $result ); } + public static function dataGet() { + // Make sure the defaults are last, so the $bag is left as expected for the next test + return [ + [ 'JSON', '12345', 'JSON.Us1wli82zEJ6DNQnCG//w+MShOFrdx9wCdfTUhPPA2w=."somedata"' ], + [ 'JSON', '', 'JSON.."somedata"' ], + [ 'PHP', '12345', 'PHP.t2EKhUF4l65kZqWhoAnKW8ZPzekDYfrDxTkQcVmGsuM=.s:8:"somedata";' ], + [ 'PHP', '', 'PHP..s:8:"somedata";' ], + [ 'legacy', '', 's:8:"somedata";' ], + ]; + } + public function testGetNotExist() { $this->client->expects( $this->once() )->method( 'run' )->with( [ 'method' => 'GET', @@ -71,18 +95,42 @@ class RESTBagOStuffTest extends \MediaWikiUnitTestCase { $this->assertEquals( BagOStuff::ERR_UNEXPECTED, $this->bag->getLastError() ); } - public function testPut() { + /** + * @dataProvider dataPut + */ + public function testPut( $serializationType, $hmacKey, $data ) { + $classReflect = new ReflectionClass( RESTBagOStuff::class ); + + $serializationTypeReflect = $classReflect->getProperty( 'serializationType' ); + $serializationTypeReflect->setAccessible( true ); + $serializationTypeReflect->setValue( $this->bag, $serializationType ); + + $hmacKeyReflect = $classReflect->getProperty( 'hmacKey' ); + $hmacKeyReflect->setAccessible( true ); + $hmacKeyReflect->setValue( $this->bag, $hmacKey ); + $this->client->expects( $this->once() )->method( 'run' )->with( [ 'method' => 'PUT', 'url' => 'http://test/rest/42xyz42', - 'body' => '"postdata"', + 'body' => $data, 'headers' => [] // list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) ] )->willReturn( [ 200, 'OK', [], 'Done', 0 ] ); - $result = $this->bag->set( '42xyz42', 'postdata' ); + $result = $this->bag->set( '42xyz42', 'somedata' ); $this->assertTrue( $result ); } + public static function dataPut() { + // Make sure the defaults are last, so the $bag is left as expected for the next test + return [ + [ 'JSON', '12345', 'JSON.Us1wli82zEJ6DNQnCG//w+MShOFrdx9wCdfTUhPPA2w=."somedata"' ], + [ 'JSON', '', 'JSON.."somedata"' ], + [ 'PHP', '12345', 'PHP.t2EKhUF4l65kZqWhoAnKW8ZPzekDYfrDxTkQcVmGsuM=.s:8:"somedata";' ], + [ 'PHP', '', 'PHP..s:8:"somedata";' ], + [ 'legacy', '', 's:8:"somedata";' ], + ]; + } + public function testDelete() { $this->client->expects( $this->once() )->method( 'run' )->with( [ 'method' => 'DELETE', -- 2.20.1