From 2cf1c7f712dc87e249984c70597cb5d2ade3dccb Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sat, 25 Apr 2020 11:48:49 -0700 Subject: [PATCH] Fix handling of SFTP authorization; closes #1972 --- .../Remote/SftpAuthenticationController.php | 103 +++++--- .../Sftp/AuthenticateUsingPasswordService.php | 108 --------- .../AuthenticateUsingPasswordServiceTest.php | 228 ------------------ 3 files changed, 72 insertions(+), 367 deletions(-) delete mode 100644 app/Services/Sftp/AuthenticateUsingPasswordService.php delete mode 100644 tests/Unit/Services/Sftp/AuthenticateUsingPasswordServiceTest.php diff --git a/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php b/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php index e93c1dfc1..fc13c8f29 100644 --- a/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php +++ b/app/Http/Controllers/Api/Remote/SftpAuthenticationController.php @@ -3,12 +3,15 @@ namespace Pterodactyl\Http\Controllers\Api\Remote; use Illuminate\Http\Request; -use Illuminate\Http\Response; use Illuminate\Http\JsonResponse; +use Pterodactyl\Models\Permission; use Pterodactyl\Http\Controllers\Controller; use Illuminate\Foundation\Auth\ThrottlesLogins; -use Pterodactyl\Exceptions\Repository\RecordNotFoundException; -use Pterodactyl\Services\Sftp\AuthenticateUsingPasswordService; +use Pterodactyl\Repositories\Eloquent\UserRepository; +use Pterodactyl\Exceptions\Http\HttpForbiddenException; +use Pterodactyl\Repositories\Eloquent\ServerRepository; +use Pterodactyl\Services\Servers\GetUserPermissionsService; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Pterodactyl\Http\Requests\Api\Remote\SftpAuthenticationFormRequest; @@ -17,18 +20,35 @@ class SftpAuthenticationController extends Controller use ThrottlesLogins; /** - * @var \Pterodactyl\Services\Sftp\AuthenticateUsingPasswordService + * @var \Pterodactyl\Repositories\Eloquent\UserRepository */ - private $authenticationService; + private $userRepository; + + /** + * @var \Pterodactyl\Repositories\Eloquent\ServerRepository + */ + private $serverRepository; + + /** + * @var \Pterodactyl\Services\Servers\GetUserPermissionsService + */ + private $permissionsService; /** * SftpController constructor. * - * @param \Pterodactyl\Services\Sftp\AuthenticateUsingPasswordService $authenticationService + * @param \Pterodactyl\Services\Servers\GetUserPermissionsService $permissionsService + * @param \Pterodactyl\Repositories\Eloquent\UserRepository $userRepository + * @param \Pterodactyl\Repositories\Eloquent\ServerRepository $serverRepository */ - public function __construct(AuthenticateUsingPasswordService $authenticationService) - { - $this->authenticationService = $authenticationService; + public function __construct( + GetUserPermissionsService $permissionsService, + UserRepository $userRepository, + ServerRepository $serverRepository + ) { + $this->userRepository = $userRepository; + $this->serverRepository = $serverRepository; + $this->permissionsService = $permissionsService; } /** @@ -38,7 +58,7 @@ class SftpAuthenticationController extends Controller * @param \Pterodactyl\Http\Requests\Api\Remote\SftpAuthenticationFormRequest $request * @return \Illuminate\Http\JsonResponse * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException + * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException */ public function __invoke(SftpAuthenticationFormRequest $request): JsonResponse { @@ -52,33 +72,54 @@ class SftpAuthenticationController extends Controller ]; $this->incrementLoginAttempts($request); - if ($this->hasTooManyLoginAttempts($request)) { - return response()->json([ - 'error' => 'Logins throttled.', - ], Response::HTTP_TOO_MANY_REQUESTS); + return JsonResponse::create([ + 'error' => 'Too many logins attempted too quickly.', + ], JsonResponse::HTTP_TOO_MANY_REQUESTS); } - try { - $data = $this->authenticationService->handle( - $connection['username'], - $request->input('password'), - object_get($request->attributes->get('node'), 'id', 0), - empty($connection['server']) ? null : $connection['server'] + /** @var \Pterodactyl\Models\Node $node */ + $node = $request->attributes->get('node'); + if (empty($connection['server'])) { + throw new NotFoundHttpException; + } + + /** @var \Pterodactyl\Models\User $user */ + $user = $this->userRepository->findFirstWhere([ + ['username', '=', $connection['username']], + ]); + + $server = $this->serverRepository->getByUuid($connection['server'] ?? ''); + if (! password_verify($request->input('password'), $user->password) || $server->node_id !== $node->id) { + throw new HttpForbiddenException( + 'Authorization credentials were not correct, please try again.' ); - - $this->clearLoginAttempts($request); - } catch (BadRequestHttpException $exception) { - return response()->json([ - 'error' => 'The server you are trying to access is not installed or is suspended.', - ], Response::HTTP_BAD_REQUEST); - } catch (RecordNotFoundException $exception) { - return response()->json([ - 'error' => 'Unable to locate a resource using the username and password provided.', - ], Response::HTTP_NOT_FOUND); } - return response()->json($data); + if (! $user->root_admin && $server->owner_id !== $user->id) { + $permissions = $this->permissionsService->handle($server, $user); + + if (! in_array(Permission::ACTION_FILE_SFTP, $permissions)) { + throw new HttpForbiddenException( + 'You do not have permission to access SFTP for this server.' + ); + } + } + + // Remeber, for security purposes, only reveal the existence of the server to people that + // have provided valid credentials, and have permissions to know about it. + if ($server->installed !== 1 || $server->suspended) { + throw new BadRequestHttpException( + 'Server is not installed or is currently suspended.' + ); + } + + return JsonResponse::create([ + 'server' => $server->uuid, + // Deprecated, but still needed at the moment for Wings. + 'token' => '', + 'permissions' => $permissions ?? ['*'], + ]); } /** diff --git a/app/Services/Sftp/AuthenticateUsingPasswordService.php b/app/Services/Sftp/AuthenticateUsingPasswordService.php deleted file mode 100644 index 1930f71b9..000000000 --- a/app/Services/Sftp/AuthenticateUsingPasswordService.php +++ /dev/null @@ -1,108 +0,0 @@ -keyProviderService = $keyProviderService; - $this->repository = $repository; - $this->subuserRepository = $subuserRepository; - $this->userRepository = $userRepository; - } - - /** - * Attempt to authenticate a provided username and password and determine if they - * have permission to access a given server. This function does not account for - * subusers currently. Only administrators and server owners can login to access - * their files at this time. - * - * Server must exist on the node that the API call is being made from in order for a - * valid response to be provided. - * - * @param string $username - * @param string $password - * @param int $node - * @param string|null $server - * @return array - * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException - * @throws \Symfony\Component\HttpKernel\Exception\BadRequestHttpException - */ - public function handle(string $username, string $password, int $node, string $server = null): array - { - if (is_null($server)) { - throw new RecordNotFoundException; - } - - $user = $this->userRepository->setColumns(['id', 'root_admin', 'password'])->findFirstWhere([['username', '=', $username]]); - if (! password_verify($password, $user->password)) { - throw new RecordNotFoundException; - } - - $server = $this->repository->setColumns(['id', 'node_id', 'owner_id', 'uuid', 'installed', 'suspended'])->getByUuid($server); - if ($server->node_id !== $node) { - throw new RecordNotFoundException; - } - - if (! $user->root_admin && $server->owner_id !== $user->id) { - $subuser = $this->subuserRepository->getWithPermissionsUsingUserAndServer($user->id, $server->id); - $permissions = $subuser->getRelation('permissions')->pluck('permission')->toArray(); - - if (! in_array('access-sftp', $permissions)) { - throw new RecordNotFoundException; - } - } - - if ($server->installed !== 1 || $server->suspended) { - throw new BadRequestHttpException; - } - - return [ - 'server' => $server->uuid, - 'token' => $this->keyProviderService->handle($server, $user), - 'permissions' => $permissions ?? ['*'], - ]; - } -} diff --git a/tests/Unit/Services/Sftp/AuthenticateUsingPasswordServiceTest.php b/tests/Unit/Services/Sftp/AuthenticateUsingPasswordServiceTest.php deleted file mode 100644 index 50f1d5e7e..000000000 --- a/tests/Unit/Services/Sftp/AuthenticateUsingPasswordServiceTest.php +++ /dev/null @@ -1,228 +0,0 @@ -keyProviderService = m::mock(DaemonKeyProviderService::class); - $this->repository = m::mock(ServerRepositoryInterface::class); - $this->subuserRepository = m::mock(SubuserRepositoryInterface::class); - $this->userRepository = m::mock(UserRepositoryInterface::class); - } - - /** - * Test that an account can be authenticated. - */ - public function testNonAdminAccountIsAuthenticated() - { - $user = factory(User::class)->make(['root_admin' => 0]); - $server = factory(Server::class)->make(['node_id' => 1, 'owner_id' => $user->id]); - - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', $user->username]])->once()->andReturn($user); - - $this->repository->shouldReceive('setColumns')->with(['id', 'node_id', 'owner_id', 'uuid', 'installed', 'suspended'])->once()->andReturnSelf(); - $this->repository->shouldReceive('getByUuid')->with($server->uuidShort)->once()->andReturn($server); - - $this->keyProviderService->shouldReceive('handle')->with($server, $user)->once()->andReturn('server_token'); - - $response = $this->getService()->handle($user->username, 'password', 1, $server->uuidShort); - $this->assertNotEmpty($response); - $this->assertArrayHasKey('server', $response); - $this->assertArrayHasKey('token', $response); - $this->assertSame($server->uuid, $response['server']); - $this->assertSame('server_token', $response['token']); - } - - /** - * Test that an administrative user can access servers that they are not - * set as the owner of. - */ - public function testAdminAccountIsAuthenticated() - { - $user = factory(User::class)->make(['root_admin' => 1]); - $server = factory(Server::class)->make(['node_id' => 1, 'owner_id' => $user->id + 1]); - - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', $user->username]])->once()->andReturn($user); - - $this->repository->shouldReceive('setColumns')->with(['id', 'node_id', 'owner_id', 'uuid', 'installed', 'suspended'])->once()->andReturnSelf(); - $this->repository->shouldReceive('getByUuid')->with($server->uuidShort)->once()->andReturn($server); - - $this->keyProviderService->shouldReceive('handle')->with($server, $user)->once()->andReturn('server_token'); - - $response = $this->getService()->handle($user->username, 'password', 1, $server->uuidShort); - $this->assertNotEmpty($response); - $this->assertArrayHasKey('server', $response); - $this->assertArrayHasKey('token', $response); - $this->assertSame($server->uuid, $response['server']); - $this->assertSame('server_token', $response['token']); - } - - /** - * Test exception gets thrown if no server is passed into the function. - * - * @expectedException \Pterodactyl\Exceptions\Repository\RecordNotFoundException - */ - public function testExceptionIsThrownIfNoServerIsProvided() - { - $this->getService()->handle('username', 'password', 1); - } - - /** - * Test that an exception is thrown if the user account exists but the wrong - * credentials are passed. - * - * @expectedException \Pterodactyl\Exceptions\Repository\RecordNotFoundException - */ - public function testExceptionIsThrownIfUserDetailsAreIncorrect() - { - $user = factory(User::class)->make(); - - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', $user->username]])->once()->andReturn($user); - - $this->getService()->handle($user->username, 'wrongpassword', 1, '1234'); - } - - /** - * Test that an exception is thrown if no user account is found. - * - * @expectedException \Pterodactyl\Exceptions\Repository\RecordNotFoundException - */ - public function testExceptionIsThrownIfNoUserAccountIsFound() - { - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', 'something']])->once()->andThrow(new RecordNotFoundException); - - $this->getService()->handle('something', 'password', 1, '1234'); - } - - /** - * Test that an exception is thrown if the user is not the owner of the server, - * is not a sub user and is not an administrator. - * - * @expectedException \Pterodactyl\Exceptions\Repository\RecordNotFoundException - */ - public function testExceptionIsThrownIfUserDoesNotOwnServer() - { - $user = factory(User::class)->make(['root_admin' => 0]); - $server = factory(Server::class)->make(['node_id' => 1, 'owner_id' => $user->id + 1]); - - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', $user->username]])->once()->andReturn($user); - - $this->repository->shouldReceive('setColumns')->with(['id', 'node_id', 'owner_id', 'uuid', 'installed', 'suspended'])->once()->andReturnSelf(); - $this->repository->shouldReceive('getByUuid')->with($server->uuidShort)->once()->andReturn($server); - - $this->subuserRepository->shouldReceive('getWithPermissionsUsingUserAndServer')->with($user->id, $server->id)->once()->andThrow(new RecordNotFoundException); - - $this->getService()->handle($user->username, 'password', 1, $server->uuidShort); - } - - /** - * Test that an exception is thrown if the requested server does not belong to - * the node that the request is made from. - * - * @expectedException \Pterodactyl\Exceptions\Repository\RecordNotFoundException - */ - public function testExceptionIsThrownIfServerDoesNotExistOnCurrentNode() - { - $user = factory(User::class)->make(['root_admin' => 0]); - $server = factory(Server::class)->make(['node_id' => 2, 'owner_id' => $user->id]); - - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', $user->username]])->once()->andReturn($user); - - $this->repository->shouldReceive('setColumns')->with(['id', 'node_id', 'owner_id', 'uuid', 'installed', 'suspended'])->once()->andReturnSelf(); - $this->repository->shouldReceive('getByUuid')->with($server->uuidShort)->once()->andReturn($server); - - $this->getService()->handle($user->username, 'password', 1, $server->uuidShort); - } - - /** - * Test that a suspended server throws an exception. - * - * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException - */ - public function testSuspendedServer() - { - $user = factory(User::class)->make(['root_admin' => 1]); - $server = factory(Server::class)->make(['node_id' => 1, 'owner_id' => $user->id + 1, 'suspended' => 1]); - - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', $user->username]])->once()->andReturn($user); - - $this->repository->shouldReceive('setColumns')->with(['id', 'node_id', 'owner_id', 'uuid', 'installed', 'suspended'])->once()->andReturnSelf(); - $this->repository->shouldReceive('getByUuid')->with($server->uuidShort)->once()->andReturn($server); - - $this->getService()->handle($user->username, 'password', 1, $server->uuidShort); - } - - /** - * Test that a server that is not yet installed throws an exception. - * - * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException - */ - public function testNotInstalledServer() - { - $user = factory(User::class)->make(['root_admin' => 1]); - $server = factory(Server::class)->make(['node_id' => 1, 'owner_id' => $user->id + 1, 'installed' => 0]); - - $this->userRepository->shouldReceive('setColumns')->with(['id', 'root_admin', 'password'])->once()->andReturnSelf(); - $this->userRepository->shouldReceive('findFirstWhere')->with([['username', '=', $user->username]])->once()->andReturn($user); - - $this->repository->shouldReceive('setColumns')->with(['id', 'node_id', 'owner_id', 'uuid', 'installed', 'suspended'])->once()->andReturnSelf(); - $this->repository->shouldReceive('getByUuid')->with($server->uuidShort)->once()->andReturn($server); - - $this->getService()->handle($user->username, 'password', 1, $server->uuidShort); - } - - /** - * Return an instance of the service with mocked dependencies. - * - * @return \Pterodactyl\Services\Sftp\AuthenticateUsingPasswordService - */ - private function getService(): AuthenticateUsingPasswordService - { - return new AuthenticateUsingPasswordService($this->keyProviderService, $this->repository, $this->subuserRepository, $this->userRepository); - } -}