From a8c4d6afdb42f4d6c9001ebb004eaf63be563049 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 13 Sep 2017 23:07:02 -0500 Subject: [PATCH] Update random ID method to use str_random and not random_bytes The use of random_bytes in combination with bin2hex was producing a lot of duplicate keys when tested in batches of 10k (anywhere from 2 to 6). The use of str_random yielded no duplicates even at scales of 100k keys that were 8 characters. --- app/Services/Api/KeyCreationService.php | 8 ++++---- app/Services/Nodes/NodeCreationService.php | 4 ++-- app/Services/Nodes/NodeUpdateService.php | 2 +- .../Servers/DetailsModificationService.php | 4 +++- app/Services/Servers/ServerCreationService.php | 6 ++++-- .../Servers/UsernameGenerationService.php | 2 +- app/Services/Subusers/SubuserCreationService.php | 5 ++--- .../Unit/Services/Api/KeyCreationServiceTest.php | 10 +++++----- .../Services/Nodes/NodeCreationServiceTest.php | 6 +++--- .../Services/Nodes/NodeUpdateServiceTest.php | 14 +++----------- .../Servers/DetailsModificationServiceTest.php | 16 ++++++++-------- .../Servers/ServerCreationServiceTest.php | 10 +++++----- .../Servers/UsernameGenerationServiceTest.php | 13 +++++-------- .../Subusers/SubuserCreationServiceTest.php | 9 ++++----- 14 files changed, 50 insertions(+), 59 deletions(-) diff --git a/app/Services/Api/KeyCreationService.php b/app/Services/Api/KeyCreationService.php index 4beaf3a4d..b9683fb24 100644 --- a/app/Services/Api/KeyCreationService.php +++ b/app/Services/Api/KeyCreationService.php @@ -30,8 +30,8 @@ use Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface; class KeyCreationService { - const PUB_CRYPTO_BYTES = 8; - const PRIV_CRYPTO_BYTES = 32; + const PUB_CRYPTO_LENGTH = 16; + const PRIV_CRYPTO_LENGTH = 64; /** * @var \Illuminate\Database\ConnectionInterface @@ -86,8 +86,8 @@ class KeyCreationService */ public function handle(array $data, array $permissions, array $administrative = []) { - $publicKey = bin2hex(random_bytes(self::PUB_CRYPTO_BYTES)); - $secretKey = bin2hex(random_bytes(self::PRIV_CRYPTO_BYTES)); + $publicKey = str_random(self::PUB_CRYPTO_LENGTH); + $secretKey = str_random(self::PRIV_CRYPTO_LENGTH); // Start a Transaction $this->connection->beginTransaction(); diff --git a/app/Services/Nodes/NodeCreationService.php b/app/Services/Nodes/NodeCreationService.php index 30f31a29b..18fd22574 100644 --- a/app/Services/Nodes/NodeCreationService.php +++ b/app/Services/Nodes/NodeCreationService.php @@ -28,7 +28,7 @@ use Pterodactyl\Contracts\Repository\NodeRepositoryInterface; class NodeCreationService { - const DAEMON_SECRET_LENGTH = 18; + const DAEMON_SECRET_LENGTH = 36; /** * @var \Pterodactyl\Contracts\Repository\NodeRepositoryInterface @@ -55,7 +55,7 @@ class NodeCreationService */ public function handle(array $data) { - $data['daemonSecret'] = bin2hex(random_bytes(self::DAEMON_SECRET_LENGTH)); + $data['daemonSecret'] = str_random(self::DAEMON_SECRET_LENGTH); return $this->repository->create($data); } diff --git a/app/Services/Nodes/NodeUpdateService.php b/app/Services/Nodes/NodeUpdateService.php index 9fd27332d..a00b0bda0 100644 --- a/app/Services/Nodes/NodeUpdateService.php +++ b/app/Services/Nodes/NodeUpdateService.php @@ -83,7 +83,7 @@ class NodeUpdateService } if (! is_null(array_get($data, 'reset_secret'))) { - $data['daemonSecret'] = bin2hex(random_bytes(NodeCreationService::DAEMON_SECRET_LENGTH)); + $data['daemonSecret'] = str_random(NodeCreationService::DAEMON_SECRET_LENGTH); unset($data['reset_secret']); } diff --git a/app/Services/Servers/DetailsModificationService.php b/app/Services/Servers/DetailsModificationService.php index 6e31e2de8..0e9a815af 100644 --- a/app/Services/Servers/DetailsModificationService.php +++ b/app/Services/Servers/DetailsModificationService.php @@ -29,6 +29,7 @@ use Pterodactyl\Models\Server; use Illuminate\Database\DatabaseManager; use GuzzleHttp\Exception\RequestException; use Pterodactyl\Exceptions\DisplayException; +use Pterodactyl\Services\Nodes\NodeCreationService; use Pterodactyl\Repositories\Eloquent\ServerRepository; use Pterodactyl\Repositories\Daemon\ServerRepository as DaemonServerRepository; @@ -83,6 +84,7 @@ class DetailsModificationService * * @throws \Pterodactyl\Exceptions\DisplayException * @throws \Pterodactyl\Exceptions\Model\DataValidationException + * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException */ public function edit($server, array $data) { @@ -97,7 +99,7 @@ class DetailsModificationService (isset($data['reset_token']) && ! is_null($data['reset_token'])) || (isset($data['owner_id']) && $data['owner_id'] != $server->owner_id) ) { - $data['daemonSecret'] = bin2hex(random_bytes(18)); + $data['daemonSecret'] = str_random(NodeCreationService::DAEMON_SECRET_LENGTH); $shouldUpdate = true; } diff --git a/app/Services/Servers/ServerCreationService.php b/app/Services/Servers/ServerCreationService.php index a2863424e..d7977c9d7 100644 --- a/app/Services/Servers/ServerCreationService.php +++ b/app/Services/Servers/ServerCreationService.php @@ -29,6 +29,7 @@ use Illuminate\Log\Writer; use Illuminate\Database\DatabaseManager; use GuzzleHttp\Exception\RequestException; use Pterodactyl\Exceptions\DisplayException; +use Pterodactyl\Services\Nodes\NodeCreationService; use Pterodactyl\Contracts\Repository\NodeRepositoryInterface; use Pterodactyl\Contracts\Repository\UserRepositoryInterface; use Pterodactyl\Contracts\Repository\ServerRepositoryInterface; @@ -134,12 +135,13 @@ class ServerCreationService * * @throws \Pterodactyl\Exceptions\DisplayException * @throws \Pterodactyl\Exceptions\Model\DataValidationException + * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException */ public function create(array $data) { // @todo auto-deployment $validator = $this->validatorService->isAdmin()->setFields($data['environment'])->validate($data['option_id']); - $uniqueShort = bin2hex(random_bytes(4)); + $uniqueShort = str_random(8); $this->database->beginTransaction(); @@ -163,7 +165,7 @@ class ServerCreationService 'option_id' => $data['option_id'], 'pack_id' => (! isset($data['pack_id']) || $data['pack_id'] == 0) ? null : $data['pack_id'], 'startup' => $data['startup'], - 'daemonSecret' => bin2hex(random_bytes(18)), + 'daemonSecret' => str_random(NodeCreationService::DAEMON_SECRET_LENGTH), 'image' => $data['docker_image'], 'username' => $this->usernameService->generate($data['name'], $uniqueShort), 'sftp_password' => null, diff --git a/app/Services/Servers/UsernameGenerationService.php b/app/Services/Servers/UsernameGenerationService.php index 488288b47..0c1b24308 100644 --- a/app/Services/Servers/UsernameGenerationService.php +++ b/app/Services/Servers/UsernameGenerationService.php @@ -37,7 +37,7 @@ class UsernameGenerationService public function generate($name, $identifier = null) { if (is_null($identifier) || ! ctype_alnum($identifier)) { - $unique = bin2hex(random_bytes(4)); + $unique = str_random(8); } else { if (strlen($identifier) < 8) { $unique = $identifier . str_random((8 - strlen($identifier))); diff --git a/app/Services/Subusers/SubuserCreationService.php b/app/Services/Subusers/SubuserCreationService.php index 4a13adfde..0bec408cb 100644 --- a/app/Services/Subusers/SubuserCreationService.php +++ b/app/Services/Subusers/SubuserCreationService.php @@ -29,6 +29,7 @@ use Pterodactyl\Models\Server; use GuzzleHttp\Exception\RequestException; use Illuminate\Database\ConnectionInterface; use Pterodactyl\Exceptions\DisplayException; +use Pterodactyl\Services\Nodes\NodeCreationService; use Pterodactyl\Services\Users\UserCreationService; use Pterodactyl\Contracts\Repository\UserRepositoryInterface; use Pterodactyl\Exceptions\Repository\RecordNotFoundException; @@ -40,8 +41,6 @@ use Pterodactyl\Contracts\Repository\Daemon\ServerRepositoryInterface as DaemonS class SubuserCreationService { - const DAEMON_SECRET_BYTES = 18; - /** * @var \Illuminate\Database\ConnectionInterface */ @@ -158,7 +157,7 @@ class SubuserCreationService $subuser = $this->subuserRepository->create([ 'user_id' => $user->id, 'server_id' => $server->id, - 'daemonSecret' => bin2hex(random_bytes(self::DAEMON_SECRET_BYTES)), + 'daemonSecret' => str_random(NodeCreationService::DAEMON_SECRET_LENGTH), ]); $daemonPermissions = $this->permissionService->handle($subuser->id, $permissions); diff --git a/tests/Unit/Services/Api/KeyCreationServiceTest.php b/tests/Unit/Services/Api/KeyCreationServiceTest.php index 87d148c1b..2a8331dfb 100644 --- a/tests/Unit/Services/Api/KeyCreationServiceTest.php +++ b/tests/Unit/Services/Api/KeyCreationServiceTest.php @@ -84,15 +84,15 @@ class KeyCreationServiceTest extends TestCase */ public function testKeyIsCreated() { - $this->getFunctionMock('\\Pterodactyl\\Services\\Api', 'bin2hex') - ->expects($this->exactly(2))->willReturn('bin2hex'); + $this->getFunctionMock('\\Pterodactyl\\Services\\Api', 'str_random') + ->expects($this->exactly(2))->willReturn('random_string'); $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->encrypter->shouldReceive('encrypt')->with('bin2hex')->once()->andReturn('encrypted-secret'); + $this->encrypter->shouldReceive('encrypt')->with('random_string')->once()->andReturn('encrypted-secret'); $this->repository->shouldReceive('create')->with([ 'test-data' => 'test', - 'public' => 'bin2hex', + 'public' => 'random_string', 'secret' => 'encrypted-secret', ], true, true)->once()->andReturn((object) ['id' => 1]); @@ -113,6 +113,6 @@ class KeyCreationServiceTest extends TestCase ); $this->assertNotEmpty($response); - $this->assertEquals('bin2hex', $response); + $this->assertEquals('random_string', $response); } } diff --git a/tests/Unit/Services/Nodes/NodeCreationServiceTest.php b/tests/Unit/Services/Nodes/NodeCreationServiceTest.php index 998ebe057..10611467e 100644 --- a/tests/Unit/Services/Nodes/NodeCreationServiceTest.php +++ b/tests/Unit/Services/Nodes/NodeCreationServiceTest.php @@ -61,12 +61,12 @@ class NodeCreationServiceTest extends TestCase */ public function testNodeIsCreatedAndDaemonSecretIsGenerated() { - $this->getFunctionMock('\\Pterodactyl\\Services\\Nodes', 'bin2hex') - ->expects($this->once())->willReturn('hexResult'); + $this->getFunctionMock('\\Pterodactyl\\Services\\Nodes', 'str_random') + ->expects($this->once())->willReturn('random_string'); $this->repository->shouldReceive('create')->with([ 'name' => 'NodeName', - 'daemonSecret' => 'hexResult', + 'daemonSecret' => 'random_string', ])->once()->andReturnNull(); $this->assertNull($this->service->handle(['name' => 'NodeName'])); diff --git a/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php b/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php index b5c1c4869..e23e1805d 100644 --- a/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php +++ b/tests/Unit/Services/Nodes/NodeUpdateServiceTest.php @@ -33,7 +33,6 @@ use Pterodactyl\Models\Node; use GuzzleHttp\Exception\RequestException; use Pterodactyl\Exceptions\DisplayException; use Pterodactyl\Services\Nodes\NodeUpdateService; -use Pterodactyl\Services\Nodes\NodeCreationService; use Pterodactyl\Contracts\Repository\NodeRepositoryInterface; use Pterodactyl\Contracts\Repository\Daemon\ConfigurationRepositoryInterface; @@ -97,20 +96,13 @@ class NodeUpdateServiceTest extends TestCase */ public function testNodeIsUpdatedAndDaemonSecretIsReset() { - $this->getFunctionMock('\\Pterodactyl\\Services\\Nodes', 'random_bytes') - ->expects($this->once())->willReturnCallback(function ($bytes) { - $this->assertEquals(NodeCreationService::DAEMON_SECRET_LENGTH, $bytes); - - return '\00'; - }); - - $this->getFunctionMock('\\Pterodactyl\\Services\\Nodes', 'bin2hex') - ->expects($this->once())->willReturn('hexResponse'); + $this->getFunctionMock('\\Pterodactyl\\Services\\Nodes', 'str_random') + ->expects($this->once())->willReturn('random_string'); $this->repository->shouldReceive('withoutFresh')->withNoArgs()->once()->andReturnSelf() ->shouldReceive('update')->with($this->node->id, [ 'name' => 'NewName', - 'daemonSecret' => 'hexResponse', + 'daemonSecret' => 'random_string', ])->andReturn(true); $this->configRepository->shouldReceive('setNode')->with($this->node->id)->once()->andReturnSelf() diff --git a/tests/Unit/Services/Servers/DetailsModificationServiceTest.php b/tests/Unit/Services/Servers/DetailsModificationServiceTest.php index a56697ba5..a33170bca 100644 --- a/tests/Unit/Services/Servers/DetailsModificationServiceTest.php +++ b/tests/Unit/Services/Servers/DetailsModificationServiceTest.php @@ -84,8 +84,8 @@ class DetailsModificationServiceTest extends TestCase $this->repository = m::mock(ServerRepository::class); $this->writer = m::mock(Writer::class); - $this->getFunctionMock('\\Pterodactyl\\Services\\Servers', 'bin2hex') - ->expects($this->any())->willReturn('randomString'); + $this->getFunctionMock('\\Pterodactyl\\Services\\Servers', 'str_random') + ->expects($this->any())->willReturn('random_string'); $this->service = new DetailsModificationService( $this->database, @@ -171,7 +171,7 @@ class DetailsModificationServiceTest extends TestCase 'owner_id' => $data['owner_id'], 'name' => $data['name'], 'description' => $data['description'], - 'daemonSecret' => 'randomString', + 'daemonSecret' => 'random_string', ], true, true)->once()->andReturnNull(); $this->daemonServerRepository->shouldReceive('setNode')->with($server->node_id)->once()->andReturnSelf() @@ -179,7 +179,7 @@ class DetailsModificationServiceTest extends TestCase ->shouldReceive('update')->with([ 'keys' => [ $server->daemonSecret => [], - 'randomString' => DaemonServerRepository::DAEMON_PERMISSIONS, + 'random_string' => DaemonServerRepository::DAEMON_PERMISSIONS, ], ])->once()->andReturnNull(); @@ -206,7 +206,7 @@ class DetailsModificationServiceTest extends TestCase 'owner_id' => $data['owner_id'], 'name' => $data['name'], 'description' => $data['description'], - 'daemonSecret' => 'randomString', + 'daemonSecret' => 'random_string', ], true, true)->once()->andReturnNull(); $this->daemonServerRepository->shouldReceive('setNode')->with($server->node_id)->once()->andReturnSelf() @@ -214,7 +214,7 @@ class DetailsModificationServiceTest extends TestCase ->shouldReceive('update')->with([ 'keys' => [ $server->daemonSecret => [], - 'randomString' => DaemonServerRepository::DAEMON_PERMISSIONS, + 'random_string' => DaemonServerRepository::DAEMON_PERMISSIONS, ], ])->once()->andReturnNull(); @@ -244,7 +244,7 @@ class DetailsModificationServiceTest extends TestCase 'owner_id' => $data['owner_id'], 'name' => $data['name'], 'description' => $data['description'], - 'daemonSecret' => 'randomString', + 'daemonSecret' => 'random_string', ], true, true)->once()->andReturnNull(); $this->daemonServerRepository->shouldReceive('setNode')->andThrow($this->exception); @@ -286,7 +286,7 @@ class DetailsModificationServiceTest extends TestCase 'owner_id' => $data['owner_id'], 'name' => $data['name'], 'description' => $data['description'], - 'daemonSecret' => 'randomString', + 'daemonSecret' => 'random_string', ], true, true)->once()->andReturnNull(); $this->daemonServerRepository->shouldReceive('setNode')->andThrow(new Exception()); diff --git a/tests/Unit/Services/Servers/ServerCreationServiceTest.php b/tests/Unit/Services/Servers/ServerCreationServiceTest.php index f083e3855..4d4f89abc 100644 --- a/tests/Unit/Services/Servers/ServerCreationServiceTest.php +++ b/tests/Unit/Services/Servers/ServerCreationServiceTest.php @@ -155,8 +155,8 @@ class ServerCreationServiceTest extends TestCase $this->uuid = m::mock('overload:Ramsey\Uuid\Uuid'); $this->writer = m::mock(Writer::class); - $this->getFunctionMock('\\Pterodactyl\\Services\\Servers', 'bin2hex') - ->expects($this->any())->willReturn('randomstring'); + $this->getFunctionMock('\\Pterodactyl\\Services\\Servers', 'str_random') + ->expects($this->any())->willReturn('random_string'); $this->getFunctionMock('\\Ramsey\\Uuid\\Uuid', 'uuid4') ->expects($this->any())->willReturn('s'); @@ -187,12 +187,12 @@ class ServerCreationServiceTest extends TestCase $this->database->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); $this->uuid->shouldReceive('uuid4')->withNoArgs()->once()->andReturnSelf() ->shouldReceive('toString')->withNoArgs()->once()->andReturn('uuid-0000'); - $this->usernameService->shouldReceive('generate')->with($this->data['name'], 'randomstring') + $this->usernameService->shouldReceive('generate')->with($this->data['name'], 'random_string') ->once()->andReturn('user_name'); $this->repository->shouldReceive('create')->with([ 'uuid' => 'uuid-0000', - 'uuidShort' => 'randomstring', + 'uuidShort' => 'random_string', 'node_id' => $this->data['node_id'], 'name' => $this->data['name'], 'description' => $this->data['description'], @@ -210,7 +210,7 @@ class ServerCreationServiceTest extends TestCase 'option_id' => $this->data['option_id'], 'pack_id' => null, 'startup' => $this->data['startup'], - 'daemonSecret' => 'randomstring', + 'daemonSecret' => 'random_string', 'image' => $this->data['docker_image'], 'username' => 'user_name', 'sftp_password' => null, diff --git a/tests/Unit/Services/Servers/UsernameGenerationServiceTest.php b/tests/Unit/Services/Servers/UsernameGenerationServiceTest.php index c0d80cd54..2afb17d94 100644 --- a/tests/Unit/Services/Servers/UsernameGenerationServiceTest.php +++ b/tests/Unit/Services/Servers/UsernameGenerationServiceTest.php @@ -46,12 +46,9 @@ class UsernameGenerationServiceTest extends TestCase $this->service = new UsernameGenerationService(); - $this->getFunctionMock('\\Pterodactyl\\Services\\Servers', 'bin2hex') - ->expects($this->any())->willReturn('dddddddd'); - $this->getFunctionMock('\\Pterodactyl\\Services\\Servers', 'str_random') ->expects($this->any())->willReturnCallback(function ($count) { - return str_pad('', $count, 'a'); + return str_pad('', $count, '0'); }); } @@ -62,7 +59,7 @@ class UsernameGenerationServiceTest extends TestCase { $response = $this->service->generate('testname'); - $this->assertEquals('testna_dddddddd', $response); + $this->assertEquals('testna_00000000', $response); } /** @@ -82,7 +79,7 @@ class UsernameGenerationServiceTest extends TestCase { $response = $this->service->generate('testname', 'xyz'); - $this->assertEquals('testna_xyzaaaaa', $response); + $this->assertEquals('testna_xyz00000', $response); } /** @@ -102,7 +99,7 @@ class UsernameGenerationServiceTest extends TestCase { $response = $this->service->generate(''); - $this->assertEquals('aaaaaa_dddddddd', $response); + $this->assertEquals('000000_00000000', $response); } /** @@ -112,7 +109,7 @@ class UsernameGenerationServiceTest extends TestCase { $response = $this->service->generate('$%#*#(@#(#*$&#(#!#@'); - $this->assertEquals('aaaaaa_dddddddd', $response); + $this->assertEquals('000000_00000000', $response); } /** diff --git a/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php b/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php index d188e22a3..8f351fa03 100644 --- a/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php +++ b/tests/Unit/Services/Subusers/SubuserCreationServiceTest.php @@ -100,8 +100,7 @@ class SubuserCreationServiceTest extends TestCase { parent::setUp(); - $this->getFunctionMock('\\Pterodactyl\\Services\\Subusers', 'bin2hex')->expects($this->any())->willReturn('bin2hex'); - $this->getFunctionMock('\\Pterodactyl\\Services\\Subusers', 'str_random')->expects($this->any())->willReturn('123456'); + $this->getFunctionMock('\\Pterodactyl\\Services\\Subusers', 'str_random')->expects($this->any())->willReturn('random_string'); $this->connection = m::mock(ConnectionInterface::class); $this->daemonRepository = m::mock(DaemonServerRepositoryInterface::class); @@ -138,7 +137,7 @@ class SubuserCreationServiceTest extends TestCase $this->userRepository->shouldReceive('findFirstWhere')->with([['email', '=', $user->email]])->once()->andThrow(new RecordNotFoundException); $this->userCreationService->shouldReceive('handle')->with([ 'email' => $user->email, - 'username' => substr(strtok($user->email, '@'), 0, 8) . '_' . '123456', + 'username' => substr(strtok($user->email, '@'), 0, 8) . '_' . 'random_string', 'name_first' => 'Server', 'name_last' => 'Subuser', 'root_admin' => false, @@ -147,7 +146,7 @@ class SubuserCreationServiceTest extends TestCase $this->subuserRepository->shouldReceive('create')->with([ 'user_id' => $user->id, 'server_id' => $server->id, - 'daemonSecret' => 'bin2hex', + 'daemonSecret' => 'random_string', ])->once()->andReturn($subuser); $this->permissionService->shouldReceive('handle')->with($subuser->id, array_keys($permissions))->once() @@ -184,7 +183,7 @@ class SubuserCreationServiceTest extends TestCase $this->subuserRepository->shouldReceive('create')->with([ 'user_id' => $user->id, 'server_id' => $server->id, - 'daemonSecret' => 'bin2hex', + 'daemonSecret' => 'random_string', ])->once()->andReturn($subuser); $this->permissionService->shouldReceive('handle')->with($subuser->id, $permissions)->once()