From 285485d7b02c405ff4900bdabc00be06c48ce1c9 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 3 Dec 2017 14:29:14 -0600 Subject: [PATCH] Change how API keys are validated (#771) --- CHANGELOG.md | 3 + .../Repository/ApiKeyRepositoryInterface.php | 10 + app/Http/Controllers/Base/APIController.php | 27 +-- app/Http/Kernel.php | 18 +- .../Middleware/API/AuthenticateIPAccess.php | 40 ++++ app/Http/Middleware/API/AuthenticateKey.php | 68 ++++++ .../API/HasPermissionToResource.php | 58 +++++ app/Http/Middleware/API/SetSessionDriver.php | 52 +++++ app/Http/Middleware/HMACAuthorization.php | 209 ------------------ app/Models/APIKey.php | 15 +- app/Providers/RouteServiceProvider.php | 14 +- .../Eloquent/ApiKeyRepository.php | 16 ++ app/Services/Api/KeyCreationService.php | 43 +--- database/factories/ModelFactory.php | 19 ++ ...122708_MigratePubPrivFormatToSingleKey.php | 59 +++++ resources/lang/en/base.php | 2 +- resources/lang/en/strings.php | 2 +- .../pterodactyl/base/api/index.blade.php | 9 +- .../Controllers/Base/APIControllerTest.php | 75 +++---- .../API/AuthenticateIPAccessTest.php | 82 +++++++ .../Middleware/API/AuthenticateKeyTest.php | 90 ++++++++ .../API/HasPermissionToResourceTest.php | 109 +++++++++ .../Middleware/API/SetSessionDriverTest.php | 69 ++++++ .../Services/Api/KeyCreationServiceTest.php | 68 +++--- 24 files changed, 774 insertions(+), 383 deletions(-) create mode 100644 app/Http/Middleware/API/AuthenticateIPAccess.php create mode 100644 app/Http/Middleware/API/AuthenticateKey.php create mode 100644 app/Http/Middleware/API/HasPermissionToResource.php create mode 100644 app/Http/Middleware/API/SetSessionDriver.php delete mode 100644 app/Http/Middleware/HMACAuthorization.php create mode 100644 database/migrations/2017_11_19_122708_MigratePubPrivFormatToSingleKey.php create mode 100644 tests/Unit/Http/Middleware/API/AuthenticateIPAccessTest.php create mode 100644 tests/Unit/Http/Middleware/API/AuthenticateKeyTest.php create mode 100644 tests/Unit/Http/Middleware/API/HasPermissionToResourceTest.php create mode 100644 tests/Unit/Http/Middleware/API/SetSessionDriverTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e193cb4..4b368012 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ This project follows [Semantic Versioning](http://semver.org) guidelines. ### Added * Added star indicators to user listing in Admin CP to indicate users who are set as a root admin. +### Changed +* API keys have been changed to only use a single public key passed in a bearer token. All existing keys can continue being used, however only the first 32 characters should be sent. + ## v0.7.0-beta.2 (Derelict Dermodactylus) ### Fixed * `[beta.1]` — Fixes a CORS header issue due to a wrong API endpoint being provided in the administrative node listing. diff --git a/app/Contracts/Repository/ApiKeyRepositoryInterface.php b/app/Contracts/Repository/ApiKeyRepositoryInterface.php index 5b8f638e..2fce09cd 100644 --- a/app/Contracts/Repository/ApiKeyRepositoryInterface.php +++ b/app/Contracts/Repository/ApiKeyRepositoryInterface.php @@ -9,6 +9,16 @@ namespace Pterodactyl\Contracts\Repository; +use Pterodactyl\Models\APIKey; + interface ApiKeyRepositoryInterface extends RepositoryInterface { + /** + * Load permissions for a key onto the model. + * + * @param \Pterodactyl\Models\APIKey $model + * @param bool $refresh + * @return \Pterodactyl\Models\APIKey + */ + public function loadPermissions(APIKey $model, bool $refresh = false): APIKey; } diff --git a/app/Http/Controllers/Base/APIController.php b/app/Http/Controllers/Base/APIController.php index 4bf89012..c7366177 100644 --- a/app/Http/Controllers/Base/APIController.php +++ b/app/Http/Controllers/Base/APIController.php @@ -1,27 +1,4 @@ - * Some Modifications (c) 2015 Dylan Seidt . - * - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to deal - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE - * SOFTWARE. - */ namespace Pterodactyl\Http\Controllers\Base; @@ -120,7 +97,7 @@ class APIController extends Controller 'memo' => $request->input('memo'), ], $request->input('permissions', []), $adminPermissions); - $this->alert->success(trans('base.api.index.keypair_created', ['token' => $secret]))->flash(); + $this->alert->success(trans('base.api.index.keypair_created'))->flash(); return redirect()->route('account.api'); } @@ -136,7 +113,7 @@ class APIController extends Controller { $this->repository->deleteWhere([ ['user_id', '=', $request->user()->id], - ['public', '=', $key], + ['token', '=', $key], ]); return response('', 204); diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 79c6e9ec..13a7b29c 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -11,17 +11,20 @@ use Pterodactyl\Http\Middleware\EncryptCookies; use Pterodactyl\Http\Middleware\VerifyCsrfToken; use Pterodactyl\Http\Middleware\VerifyReCaptcha; use Pterodactyl\Http\Middleware\AdminAuthenticate; -use Pterodactyl\Http\Middleware\HMACAuthorization; use Illuminate\Routing\Middleware\ThrottleRequests; use Pterodactyl\Http\Middleware\LanguageMiddleware; use Illuminate\Foundation\Http\Kernel as HttpKernel; +use Pterodactyl\Http\Middleware\API\AuthenticateKey; use Illuminate\Routing\Middleware\SubstituteBindings; use Pterodactyl\Http\Middleware\AccessingValidServer; +use Pterodactyl\Http\Middleware\API\SetSessionDriver; use Illuminate\View\Middleware\ShareErrorsFromSession; use Pterodactyl\Http\Middleware\RedirectIfAuthenticated; use Illuminate\Auth\Middleware\AuthenticateWithBasicAuth; +use Pterodactyl\Http\Middleware\API\AuthenticateIPAccess; use Pterodactyl\Http\Middleware\Daemon\DaemonAuthenticate; use Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse; +use Pterodactyl\Http\Middleware\API\HasPermissionToResource; use Pterodactyl\Http\Middleware\Server\AuthenticateAsSubuser; use Pterodactyl\Http\Middleware\Server\SubuserBelongsToServer; use Pterodactyl\Http\Middleware\RequireTwoFactorAuthentication; @@ -42,10 +45,6 @@ class Kernel extends HttpKernel EncryptCookies::class, AddQueuedCookiesToResponse::class, TrimStrings::class, - - /* - * Custom middleware applied to all routes. - */ TrustProxies::class, ]; @@ -66,9 +65,11 @@ class Kernel extends HttpKernel RequireTwoFactorAuthentication::class, ], 'api' => [ - HMACAuthorization::class, 'throttle:60,1', - 'bindings', + SubstituteBindings::class, + SetSessionDriver::class, + AuthenticateKey::class, + AuthenticateIPAccess::class, ], 'daemon' => [ SubstituteBindings::class, @@ -95,6 +96,9 @@ class Kernel extends HttpKernel 'bindings' => SubstituteBindings::class, 'recaptcha' => VerifyReCaptcha::class, + // API specific middleware. + 'api..user_level' => HasPermissionToResource::class, + // Server specific middleware (used for authenticating access to resources) // // These are only used for individual server authentication, and not gloabl diff --git a/app/Http/Middleware/API/AuthenticateIPAccess.php b/app/Http/Middleware/API/AuthenticateIPAccess.php new file mode 100644 index 00000000..aa0af7e2 --- /dev/null +++ b/app/Http/Middleware/API/AuthenticateIPAccess.php @@ -0,0 +1,40 @@ +attributes->get('api_key'); + + if (is_null($model->allowed_ips) || empty($model->allowed_ips)) { + return $next($request); + } + + $find = new IP($request->ip()); + foreach ($model->allowed_ips as $ip) { + if (Range::parse($ip)->contains($find)) { + return $next($request); + } + } + + throw new AccessDeniedHttpException('This IP address does not have permission to access the API using these credentials.'); + } +} diff --git a/app/Http/Middleware/API/AuthenticateKey.php b/app/Http/Middleware/API/AuthenticateKey.php new file mode 100644 index 00000000..eb82682c --- /dev/null +++ b/app/Http/Middleware/API/AuthenticateKey.php @@ -0,0 +1,68 @@ +auth = $auth; + $this->repository = $repository; + } + + /** + * Handle an API request by verifying that the provided API key + * is in a valid format, and the route being accessed is allowed + * for the given key. + * + * @param \Illuminate\Http\Request $request + * @param \Closure $next + * @return mixed + * + * @throws \Symfony\Component\HttpKernel\Exception\HttpException + * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException + */ + public function handle(Request $request, Closure $next) + { + if (is_null($request->bearerToken())) { + throw new HttpException(401, null, null, ['WWW-Authenticate' => 'Bearer']); + } + + try { + $model = $this->repository->findFirstWhere([['token', '=', $request->bearerToken()]]); + } catch (RecordNotFoundException $exception) { + throw new AccessDeniedHttpException; + } + + $this->auth->guard()->loginUsingId($model->user_id); + $request->attributes->set('api_key', $model); + + return $next($request); + } +} diff --git a/app/Http/Middleware/API/HasPermissionToResource.php b/app/Http/Middleware/API/HasPermissionToResource.php new file mode 100644 index 00000000..1d99ffbf --- /dev/null +++ b/app/Http/Middleware/API/HasPermissionToResource.php @@ -0,0 +1,58 @@ +repository = $repository; + } + + /** + * Determine if an API key has permission to access the given route. + * + * @param \Illuminate\Http\Request $request + * @param \Closure $next + * @param string $role + * @return mixed + */ + public function handle(Request $request, Closure $next, string $role = 'admin') + { + /** @var \Pterodactyl\Models\APIKey $model */ + $model = $request->attributes->get('api_key'); + + if ($role === 'admin' && ! $request->user()->root_admin) { + throw new NotFoundHttpException; + } + + $this->repository->loadPermissions($model); + $routeKey = str_replace(['api.', 'admin.'], '', $request->route()->getName()); + + $count = $model->getRelation('permissions')->filter(function ($permission) use ($routeKey) { + return $routeKey === str_replace('-', '.', $permission->permission); + })->count(); + + if ($count === 1) { + return $next($request); + } + + throw new AccessDeniedHttpException('Cannot access resource without required `' . $routeKey . '` permission.'); + } +} diff --git a/app/Http/Middleware/API/SetSessionDriver.php b/app/Http/Middleware/API/SetSessionDriver.php new file mode 100644 index 00000000..9cc5d60e --- /dev/null +++ b/app/Http/Middleware/API/SetSessionDriver.php @@ -0,0 +1,52 @@ +app = $app; + $this->config = $config; + } + + /** + * Set the session for API calls to only last for the one request. + * + * @param \Illuminate\Http\Request $request + * @param \Closure $next + * @return mixed + */ + public function handle(Request $request, Closure $next) + { + if ($this->app->environment() !== 'production') { + $this->app->make(LaravelDebugbar::class)->disable(); + } + + $this->config->set('session.driver', 'array'); + + return $next($request); + } +} diff --git a/app/Http/Middleware/HMACAuthorization.php b/app/Http/Middleware/HMACAuthorization.php deleted file mode 100644 index fa048b59..00000000 --- a/app/Http/Middleware/HMACAuthorization.php +++ /dev/null @@ -1,209 +0,0 @@ -. - * - * This software is licensed under the terms of the MIT license. - * https://opensource.org/licenses/MIT - */ - -namespace Pterodactyl\Http\Middleware; - -use Auth; -use Crypt; -use Config; -use Closure; -use Debugbar; -use IPTools\IP; -use IPTools\Range; -use Illuminate\Http\Request; -use Pterodactyl\Models\APIKey; -use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; // 400 -use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; // 403 - -class HMACAuthorization -{ - /** - * The algorithm to use for handling HMAC encryption. - * - * @var string - */ - const HMAC_ALGORITHM = 'sha256'; - - /** - * Stored values from the Authorization header. - * - * @var array - */ - protected $token = []; - - /** - * The eloquent model for the API key. - * - * @var \Pterodactyl\Models\APIKey - */ - protected $key; - - /** - * The illuminate request object. - * - * @var \Illuminate\Http\Request - */ - private $request; - - /** - * Construct class instance. - */ - public function __construct() - { - Debugbar::disable(); - Config::set('session.driver', 'array'); - } - - /** - * Handle an incoming request for the API. - * - * @param \Illuminate\Http\Request $request - * @param \Closure $next - * @return mixed - */ - public function handle(Request $request, Closure $next) - { - $this->request = $request; - - $this->checkBearer(); - $this->validateRequest(); - $this->validateIPAccess(); - $this->validateContents(); - - Auth::loginUsingId($this->key()->user_id); - - return $next($request); - } - - /** - * Checks that the Bearer token is provided and in a valid format. - * - * - * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException - */ - protected function checkBearer() - { - if (empty($this->request()->bearerToken())) { - throw new BadRequestHttpException('Request was missing required Authorization header.'); - } - - $token = explode('.', $this->request()->bearerToken()); - if (count($token) !== 2) { - throw new BadRequestHttpException('The Authorization header passed was not in a validate public/private key format.'); - } - - $this->token = [ - 'public' => $token[0], - 'hash' => $token[1], - ]; - } - - /** - * Determine if the request contains a valid public API key - * as well as permissions for the resource. - * - * - * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException - */ - protected function validateRequest() - { - $this->key = APIKey::where('public', $this->public())->first(); - if (! $this->key) { - throw new AccessDeniedHttpException('Unable to identify requester. Authorization token is invalid.'); - } - } - - /** - * Determine if the requesting IP address is allowed to use this API key. - * - * @return bool - * - * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException - */ - protected function validateIPAccess() - { - if (! is_null($this->key()->allowed_ips)) { - foreach (json_decode($this->key()->allowed_ips) as $ip) { - if (Range::parse($ip)->contains(new IP($this->request()->ip()))) { - return true; - } - } - - throw new AccessDeniedHttpException('This IP address does not have permission to access the API using these credentials.'); - } - - return true; - } - - /** - * Determine if the HMAC sent in the request matches the one generated - * on the panel side. - * - * - * @throws \Symfony\Component\HttpKernel\Exception\BadRequestHttpException - */ - protected function validateContents() - { - if (! hash_equals(base64_decode($this->hash()), $this->generateSignature())) { - throw new BadRequestHttpException('The HMAC for the request was invalid.'); - } - } - - /** - * Generate a HMAC from the request and known API secret key. - * - * @return string - */ - protected function generateSignature() - { - $content = urldecode($this->request()->fullUrl()) . $this->request()->getContent(); - - return hash_hmac(self::HMAC_ALGORITHM, $content, Crypt::decrypt($this->key()->secret), true); - } - - /** - * Return the public key passed in the Authorization header. - * - * @return string - */ - protected function public() - { - return $this->token['public']; - } - - /** - * Return the base64'd HMAC sent in the Authorization header. - * - * @return string - */ - protected function hash() - { - return $this->token['hash']; - } - - /** - * Return the API Key model. - * - * @return \Pterodactyl\Models\APIKey - */ - protected function key() - { - return $this->key; - } - - /** - * Return the Illuminate Request object. - * - * @return \Illuminate\Http\Request - */ - private function request() - { - return $this->request; - } -} diff --git a/app/Models/APIKey.php b/app/Models/APIKey.php index cbfc996d..0542fa02 100644 --- a/app/Models/APIKey.php +++ b/app/Models/APIKey.php @@ -19,6 +19,8 @@ class APIKey extends Model implements CleansAttributes, ValidableContract { use Eloquence, Validable; + const KEY_LENGTH = 32; + /** * The table associated with the model. * @@ -26,13 +28,6 @@ class APIKey extends Model implements CleansAttributes, ValidableContract */ protected $table = 'api_keys'; - /** - * The attributes excluded from the model's JSON form. - * - * @var array - */ - protected $hidden = ['secret']; - /** * Cast values to correct type. * @@ -57,8 +52,7 @@ class APIKey extends Model implements CleansAttributes, ValidableContract protected static $applicationRules = [ 'memo' => 'required', 'user_id' => 'required', - 'secret' => 'required', - 'public' => 'required', + 'token' => 'required', ]; /** @@ -68,8 +62,7 @@ class APIKey extends Model implements CleansAttributes, ValidableContract */ protected static $dataIntegrityRules = [ 'user_id' => 'exists:users,id', - 'public' => 'string|size:16', - 'secret' => 'string', + 'token' => 'string|size:32', 'memo' => 'nullable|string|max:500', 'allowed_ips' => 'nullable|json', 'expires_at' => 'nullable|datetime', diff --git a/app/Providers/RouteServiceProvider.php b/app/Providers/RouteServiceProvider.php index 57ae43fa..a0f90285 100644 --- a/app/Providers/RouteServiceProvider.php +++ b/app/Providers/RouteServiceProvider.php @@ -29,13 +29,9 @@ class RouteServiceProvider extends ServiceProvider */ public function map() { - Route::middleware(['api'])->prefix('/api/user') - ->namespace($this->namespace . '\API\User') - ->group(base_path('routes/api.php')); - - Route::middleware(['api'])->prefix('/api/admin') - ->namespace($this->namespace . '\API\Admin') - ->group(base_path('routes/api-admin.php')); +// Route::middleware(['api'])->prefix('/api/user') +// ->namespace($this->namespace . '\API\User') +// ->group(base_path('routes/api.php')); Route::middleware(['web', 'auth', 'csrf']) ->namespace($this->namespace . '\Base') @@ -53,6 +49,10 @@ class RouteServiceProvider extends ServiceProvider ->namespace($this->namespace . '\Server') ->group(base_path('routes/server.php')); + Route::middleware(['api', 'api..user_level:admin'])->prefix('/api/admin') + ->namespace($this->namespace . '\API\Admin') + ->group(base_path('routes/api-admin.php')); + Route::middleware(['daemon'])->prefix('/api/remote') ->namespace($this->namespace . '\API\Remote') ->group(base_path('routes/api-remote.php')); diff --git a/app/Repositories/Eloquent/ApiKeyRepository.php b/app/Repositories/Eloquent/ApiKeyRepository.php index facac39c..107e0b6c 100644 --- a/app/Repositories/Eloquent/ApiKeyRepository.php +++ b/app/Repositories/Eloquent/ApiKeyRepository.php @@ -21,4 +21,20 @@ class ApiKeyRepository extends EloquentRepository implements ApiKeyRepositoryInt { return APIKey::class; } + + /** + * Load permissions for a key onto the model. + * + * @param \Pterodactyl\Models\APIKey $model + * @param bool $refresh + * @return \Pterodactyl\Models\APIKey + */ + public function loadPermissions(APIKey $model, bool $refresh = false): APIKey + { + if (! $model->relationLoaded('permissions') || $refresh) { + $model->load('permissions'); + } + + return $model; + } } diff --git a/app/Services/Api/KeyCreationService.php b/app/Services/Api/KeyCreationService.php index 1a431254..891a3243 100644 --- a/app/Services/Api/KeyCreationService.php +++ b/app/Services/Api/KeyCreationService.php @@ -1,60 +1,42 @@ . - * - * This software is licensed under the terms of the MIT license. - * https://opensource.org/licenses/MIT - */ namespace Pterodactyl\Services\Api; +use Pterodactyl\Models\APIKey; use Illuminate\Database\ConnectionInterface; -use Illuminate\Contracts\Encryption\Encrypter; use Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface; class KeyCreationService { - const PUB_CRYPTO_LENGTH = 16; - const PRIV_CRYPTO_LENGTH = 64; - /** * @var \Illuminate\Database\ConnectionInterface */ - protected $connection; - - /** - * @var \Illuminate\Contracts\Encryption\Encrypter - */ - protected $encrypter; + private $connection; /** * @var \Pterodactyl\Services\Api\PermissionService */ - protected $permissionService; + private $permissionService; /** * @var \Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface */ - protected $repository; + private $repository; /** * ApiKeyService constructor. * * @param \Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface $repository * @param \Illuminate\Database\ConnectionInterface $connection - * @param \Illuminate\Contracts\Encryption\Encrypter $encrypter * @param \Pterodactyl\Services\Api\PermissionService $permissionService */ public function __construct( ApiKeyRepositoryInterface $repository, ConnectionInterface $connection, - Encrypter $encrypter, PermissionService $permissionService ) { $this->repository = $repository; $this->connection = $connection; - $this->encrypter = $encrypter; $this->permissionService = $permissionService; } @@ -64,24 +46,17 @@ class KeyCreationService * @param array $data * @param array $permissions * @param array $administrative - * @return string + * @return \Pterodactyl\Models\APIKey * * @throws \Exception * @throws \Pterodactyl\Exceptions\Model\DataValidationException */ - public function handle(array $data, array $permissions, array $administrative = []) + public function handle(array $data, array $permissions, array $administrative = []): APIKey { - $publicKey = str_random(self::PUB_CRYPTO_LENGTH); - $secretKey = str_random(self::PRIV_CRYPTO_LENGTH); + $token = str_random(APIKey::KEY_LENGTH); + $data = array_merge($data, ['token' => $token]); - // Start a Transaction $this->connection->beginTransaction(); - - $data = array_merge($data, [ - 'public' => $publicKey, - 'secret' => $this->encrypter->encrypt($secretKey), - ]); - $instance = $this->repository->create($data, true, true); $nodes = $this->permissionService->getPermissions(); @@ -115,6 +90,6 @@ class KeyCreationService $this->connection->commit(); - return $secretKey; + return $instance; } } diff --git a/database/factories/ModelFactory.php b/database/factories/ModelFactory.php index 1df550e0..e117b59b 100644 --- a/database/factories/ModelFactory.php +++ b/database/factories/ModelFactory.php @@ -221,3 +221,22 @@ $factory->define(Pterodactyl\Models\DaemonKey::class, function (Faker\Generator 'expires_at' => \Carbon\Carbon::now()->addMinutes(10)->toDateTimeString(), ]; }); + +$factory->define(Pterodactyl\Models\APIKey::class, function (Faker\Generator $faker) { + return [ + 'id' => $faker->unique()->randomNumber(), + 'user_id' => $faker->randomNumber(), + 'token' => str_random(Pterodactyl\Models\APIKey::KEY_LENGTH), + 'memo' => 'Test Function Key', + 'created_at' => \Carbon\Carbon::now()->toDateTimeString(), + 'updated_at' => \Carbon\Carbon::now()->toDateTimeString(), + ]; +}); + +$factory->define(Pterodactyl\Models\APIPermission::class, function (Faker\Generator $faker) { + return [ + 'id' => $faker->unique()->randomNumber(), + 'key_id' => $faker->randomNumber(), + 'permission' => mb_strtolower($faker->word), + ]; +}); diff --git a/database/migrations/2017_11_19_122708_MigratePubPrivFormatToSingleKey.php b/database/migrations/2017_11_19_122708_MigratePubPrivFormatToSingleKey.php new file mode 100644 index 00000000..c2947ee0 --- /dev/null +++ b/database/migrations/2017_11_19_122708_MigratePubPrivFormatToSingleKey.php @@ -0,0 +1,59 @@ +get()->each(function ($item) { + try { + $decrypted = Crypt::decrypt($item->secret); + } catch (DecryptException $exception) { + $decrypted = str_random(32); + } finally { + DB::table('api_keys')->where('id', $item->id)->update([ + 'secret' => $decrypted, + ]); + } + }); + }); + + Schema::table('api_keys', function (Blueprint $table) { + $table->dropColumn('public'); + $table->string('secret', 32)->change(); + }); + + DB::statement('ALTER TABLE `api_keys` CHANGE `secret` `token` CHAR(32) NOT NULL, ADD UNIQUE INDEX `api_keys_token_unique` (`token`(32))'); + } + + /** + * Reverse the migrations. + */ + public function down() + { + DB::statement('ALTER TABLE `api_keys` CHANGE `token` `secret` TEXT, DROP INDEX `api_keys_token_unique`'); + + Schema::table('api_keys', function (Blueprint $table) { + $table->char('public', 16)->after('user_id'); + }); + + DB::transaction(function () { + DB::table('api_keys')->get()->each(function ($item) { + DB::table('api_keys')->where('id', $item->id)->update([ + 'public' => str_random(16), + 'secret' => Crypt::encrypt($item->secret), + ]); + }); + }); + } +} diff --git a/resources/lang/en/base.php b/resources/lang/en/base.php index 9c7bd9d0..579ac341 100644 --- a/resources/lang/en/base.php +++ b/resources/lang/en/base.php @@ -33,7 +33,7 @@ return [ 'header_sub' => 'Manage your API access keys.', 'list' => 'API Keys', 'create_new' => 'Create New API key', - 'keypair_created' => 'An API Key-Pair has been generated. Your API secret token is :token. Please take note of this key as it will not be displayed again.', + 'keypair_created' => 'An API key has been successfully generated and is listed below.', ], 'new' => [ 'header' => 'New API Key', diff --git a/resources/lang/en/strings.php b/resources/lang/en/strings.php index d98a7d01..c9983e64 100644 --- a/resources/lang/en/strings.php +++ b/resources/lang/en/strings.php @@ -32,7 +32,7 @@ return [ 'memo' => 'Memo', 'created' => 'Created', 'expires' => 'Expires', - 'public_key' => 'Public key', + 'public_key' => 'Token', 'api_access' => 'Api Access', 'never' => 'never', 'sign_out' => 'Sign out', diff --git a/resources/themes/pterodactyl/base/api/index.blade.php b/resources/themes/pterodactyl/base/api/index.blade.php index a07657c9..0d8fc0a3 100644 --- a/resources/themes/pterodactyl/base/api/index.blade.php +++ b/resources/themes/pterodactyl/base/api/index.blade.php @@ -20,12 +20,7 @@ @section('content')
-
- API functionality is disabled in this beta release. -
-
-

@lang('base.api.index.list')

@@ -44,7 +39,7 @@ @foreach ($keys as $key) - {{ $key->public }} + {{ $key->token }} {{ $key->memo }} {{ (new Carbon($key->created_at))->toDayDateTimeString() }} @@ -57,7 +52,7 @@ @endif - + @endforeach diff --git a/tests/Unit/Http/Controllers/Base/APIControllerTest.php b/tests/Unit/Http/Controllers/Base/APIControllerTest.php index 055cbd84..579fb43b 100644 --- a/tests/Unit/Http/Controllers/Base/APIControllerTest.php +++ b/tests/Unit/Http/Controllers/Base/APIControllerTest.php @@ -1,54 +1,34 @@ . - * - * This software is licensed under the terms of the MIT license. - * https://opensource.org/licenses/MIT - */ namespace Tests\Unit\Http\Controllers\Base; use Mockery as m; -use Tests\TestCase; -use Illuminate\Http\Request; use Pterodactyl\Models\User; +use Pterodactyl\Models\APIKey; use Prologue\Alerts\AlertsMessageBag; -use Tests\Assertions\ControllerAssertionsTrait; use Pterodactyl\Services\Api\KeyCreationService; +use Tests\Unit\Http\Controllers\ControllerTestCase; use Pterodactyl\Http\Controllers\Base\APIController; use Pterodactyl\Http\Requests\Base\ApiKeyFormRequest; use Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface; -class APIControllerTest extends TestCase +class APIControllerTest extends ControllerTestCase { - use ControllerAssertionsTrait; - /** - * @var \Prologue\Alerts\AlertsMessageBag + * @var \Prologue\Alerts\AlertsMessageBag|\Mockery\Mock */ protected $alert; /** - * @var \Pterodactyl\Http\Controllers\Base\APIController - */ - protected $controller; - - /** - * @var \Pterodactyl\Services\Api\KeyCreationService + * @var \Pterodactyl\Services\Api\KeyCreationService|\Mockery\Mock */ protected $keyService; /** - * @var \Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface + * @var \Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface|\Mockery\Mock */ protected $repository; - /** - * @var \Illuminate\Http\Request - */ - protected $request; - /** * Setup tests. */ @@ -59,9 +39,6 @@ class APIControllerTest extends TestCase $this->alert = m::mock(AlertsMessageBag::class); $this->keyService = m::mock(KeyCreationService::class); $this->repository = m::mock(ApiKeyRepositoryInterface::class); - $this->request = m::mock(Request::class); - - $this->controller = new APIController($this->alert, $this->repository, $this->keyService); } /** @@ -69,12 +46,11 @@ class APIControllerTest extends TestCase */ public function testIndexController() { - $model = factory(User::class)->make(); + $model = $this->setRequestUser(); - $this->request->shouldReceive('user')->withNoArgs()->once()->andReturn($model); $this->repository->shouldReceive('findWhere')->with([['user_id', '=', $model->id]])->once()->andReturn(['testkeys']); - $response = $this->controller->index($this->request); + $response = $this->getController()->index($this->request); $this->assertIsViewResponse($response); $this->assertViewNameEquals('base.api.index', $response); $this->assertViewHasKey('keys', $response); @@ -88,10 +64,9 @@ class APIControllerTest extends TestCase */ public function testCreateController($admin) { - $model = factory(User::class)->make(['root_admin' => $admin]); - $this->request->shouldReceive('user')->withNoArgs()->once()->andReturn($model); + $this->setRequestUser(factory(User::class)->make(['root_admin' => $admin])); - $response = $this->controller->create($this->request); + $response = $this->getController()->create($this->request); $this->assertIsViewResponse($response); $this->assertViewNameEquals('base.api.new', $response); $this->assertViewHasKey('permissions.user', $response); @@ -111,8 +86,9 @@ class APIControllerTest extends TestCase */ public function testStoreController($admin) { - $this->request = m::mock(ApiKeyFormRequest::class); - $model = factory(User::class)->make(['root_admin' => $admin]); + $this->setRequestMockClass(ApiKeyFormRequest::class); + $model = $this->setRequestUser(factory(User::class)->make(['root_admin' => $admin])); + $keyModel = factory(APIKey::class)->make(); if ($admin) { $this->request->shouldReceive('input')->with('admin_permissions', [])->once()->andReturn(['admin.permission']); @@ -127,12 +103,12 @@ class APIControllerTest extends TestCase 'user_id' => $model->id, 'allowed_ips' => null, 'memo' => null, - ], ['test.permission'], ($admin) ? ['admin.permission'] : [])->once()->andReturn('testToken'); + ], ['test.permission'], ($admin) ? ['admin.permission'] : [])->once()->andReturn($keyModel); - $this->alert->shouldReceive('success')->with(trans('base.api.index.keypair_created', ['token' => 'testToken']))->once()->andReturnSelf() - ->shouldReceive('flash')->withNoArgs()->once()->andReturnNull(); + $this->alert->shouldReceive('success')->with(trans('base.api.index.keypair_created'))->once()->andReturnSelf(); + $this->alert->shouldReceive('flash')->withNoArgs()->once()->andReturnNull(); - $response = $this->controller->store($this->request); + $response = $this->getController()->store($this->request); $this->assertIsRedirectResponse($response); $this->assertRedirectRouteEquals('account.api', $response); } @@ -142,15 +118,14 @@ class APIControllerTest extends TestCase */ public function testRevokeController() { - $model = factory(User::class)->make(); - $this->request->shouldReceive('user')->withNoArgs()->once()->andReturn($model); + $model = $this->setRequestUser(); $this->repository->shouldReceive('deleteWhere')->with([ ['user_id', '=', $model->id], - ['public', '=', 'testKey123'], + ['token', '=', 'testKey123'], ])->once()->andReturnNull(); - $response = $this->controller->revoke($this->request, 'testKey123'); + $response = $this->getController()->revoke($this->request, 'testKey123'); $this->assertIsResponse($response); $this->assertEmpty($response->getContent()); $this->assertResponseCodeEquals(204, $response); @@ -165,4 +140,14 @@ class APIControllerTest extends TestCase { return [[0], [1]]; } + + /** + * Return an instance of the controller with mocked dependencies for testing. + * + * @return \Pterodactyl\Http\Controllers\Base\APIController + */ + private function getController(): APIController + { + return new APIController($this->alert, $this->repository, $this->keyService); + } } diff --git a/tests/Unit/Http/Middleware/API/AuthenticateIPAccessTest.php b/tests/Unit/Http/Middleware/API/AuthenticateIPAccessTest.php new file mode 100644 index 00000000..cd122f7c --- /dev/null +++ b/tests/Unit/Http/Middleware/API/AuthenticateIPAccessTest.php @@ -0,0 +1,82 @@ +make(['allowed_ips' => []]); + $this->setRequestAttribute('api_key', $model); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Test middleware works correctly when a valid IP accesses + * and there is an IP restriction. + */ + public function testWithValidIP() + { + $model = factory(APIKey::class)->make(['allowed_ips' => ['127.0.0.1']]); + $this->setRequestAttribute('api_key', $model); + + $this->request->shouldReceive('ip')->withNoArgs()->once()->andReturn('127.0.0.1'); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Test that a CIDR range can be used. + */ + public function testValidIPAganistCIDRRange() + { + $model = factory(APIKey::class)->make(['allowed_ips' => ['192.168.1.1/28']]); + $this->setRequestAttribute('api_key', $model); + + $this->request->shouldReceive('ip')->withNoArgs()->once()->andReturn('192.168.1.15'); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Test that an exception is thrown when an invalid IP address + * tries to connect and there is an IP restriction. + * + * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException + */ + public function testWithInvalidIP() + { + $model = factory(APIKey::class)->make(['allowed_ips' => ['127.0.0.1']]); + $this->setRequestAttribute('api_key', $model); + + $this->request->shouldReceive('ip')->withNoArgs()->once()->andReturn('127.0.0.2'); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Return an instance of the middleware to be used when testing. + * + * @return \Pterodactyl\Http\Middleware\API\AuthenticateIPAccess + */ + private function getMiddleware(): AuthenticateIPAccess + { + return new AuthenticateIPAccess(); + } +} diff --git a/tests/Unit/Http/Middleware/API/AuthenticateKeyTest.php b/tests/Unit/Http/Middleware/API/AuthenticateKeyTest.php new file mode 100644 index 00000000..e2032b58 --- /dev/null +++ b/tests/Unit/Http/Middleware/API/AuthenticateKeyTest.php @@ -0,0 +1,90 @@ +auth = m::mock(AuthManager::class); + $this->repository = m::mock(ApiKeyRepositoryInterface::class); + } + + /** + * Test that a missing bearer token will throw an exception. + */ + public function testMissingBearerTokenThrowsException() + { + $this->request->shouldReceive('bearerToken')->withNoArgs()->once()->andReturnNull(); + + try { + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } catch (HttpException $exception) { + $this->assertEquals(401, $exception->getStatusCode()); + $this->assertEquals(['WWW-Authenticate' => 'Bearer'], $exception->getHeaders()); + } + } + + /** + * Test that an invalid API token throws an exception. + * + * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException + */ + public function testInvalidTokenThrowsException() + { + $this->request->shouldReceive('bearerToken')->withNoArgs()->twice()->andReturn('abcd1234'); + $this->repository->shouldReceive('findFirstWhere')->andThrow(new RecordNotFoundException); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Test that a valid token can continue past the middleware. + */ + public function testValidToken() + { + $model = factory(APIKey::class)->make(); + + $this->request->shouldReceive('bearerToken')->withNoArgs()->twice()->andReturn($model->token); + $this->repository->shouldReceive('findFirstWhere')->with([['token', '=', $model->token]])->once()->andReturn($model); + + $this->auth->shouldReceive('guard->loginUsingId')->with($model->user_id)->once()->andReturnNull(); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + $this->assertEquals($model, $this->request->attributes->get('api_key')); + } + + /** + * Return an instance of the middleware with mocked dependencies for testing. + * + * @return \Pterodactyl\Http\Middleware\API\AuthenticateKey + */ + private function getMiddleware(): AuthenticateKey + { + return new AuthenticateKey($this->repository, $this->auth); + } +} diff --git a/tests/Unit/Http/Middleware/API/HasPermissionToResourceTest.php b/tests/Unit/Http/Middleware/API/HasPermissionToResourceTest.php new file mode 100644 index 00000000..7ef6c383 --- /dev/null +++ b/tests/Unit/Http/Middleware/API/HasPermissionToResourceTest.php @@ -0,0 +1,109 @@ +repository = m::mock(ApiKeyRepositoryInterface::class); + } + + /** + * Test that a non-admin user cannot access admin level routes. + * + * @expectedException \Symfony\Component\HttpKernel\Exception\NotFoundHttpException + */ + public function testNonAdminAccessingAdminLevel() + { + $model = factory(APIKey::class)->make(); + $this->setRequestAttribute('api_key', $model); + $this->setRequestUser(factory(User::class)->make(['root_admin' => false])); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Test non-admin accessing non-admin route. + */ + public function testAccessingAllowedRoute() + { + $model = factory(APIKey::class)->make(); + $model->setRelation('permissions', collect([ + factory(APIPermission::class)->make(['permission' => 'user.test-route']), + ])); + $this->setRequestAttribute('api_key', $model); + $this->setRequestUser(factory(User::class)->make(['root_admin' => false])); + + $this->request->shouldReceive('route->getName')->withNoArgs()->once()->andReturn('api.user.test.route'); + $this->repository->shouldReceive('loadPermissions')->with($model)->once()->andReturn($model); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), 'user'); + } + + /** + * Test admin accessing administrative route. + */ + public function testAccessingAllowedAdminRoute() + { + $model = factory(APIKey::class)->make(); + $model->setRelation('permissions', collect([ + factory(APIPermission::class)->make(['permission' => 'test-route']), + ])); + $this->setRequestAttribute('api_key', $model); + $this->setRequestUser(factory(User::class)->make(['root_admin' => true])); + + $this->request->shouldReceive('route->getName')->withNoArgs()->once()->andReturn('api.admin.test.route'); + $this->repository->shouldReceive('loadPermissions')->with($model)->once()->andReturn($model); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Test a user accessing a disallowed route. + * + * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException + */ + public function testAccessingDisallowedRoute() + { + $model = factory(APIKey::class)->make(); + $model->setRelation('permissions', collect([ + factory(APIPermission::class)->make(['permission' => 'user.other-route']), + ])); + $this->setRequestAttribute('api_key', $model); + $this->setRequestUser(factory(User::class)->make(['root_admin' => false])); + + $this->request->shouldReceive('route->getName')->withNoArgs()->once()->andReturn('api.user.test.route'); + $this->repository->shouldReceive('loadPermissions')->with($model)->once()->andReturn($model); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), 'user'); + } + + /** + * Return an instance of the middleware with mocked dependencies for testing. + * + * @return \Pterodactyl\Http\Middleware\API\HasPermissionToResource + */ + private function getMiddleware(): HasPermissionToResource + { + return new HasPermissionToResource($this->repository); + } +} diff --git a/tests/Unit/Http/Middleware/API/SetSessionDriverTest.php b/tests/Unit/Http/Middleware/API/SetSessionDriverTest.php new file mode 100644 index 00000000..dd9fa3e0 --- /dev/null +++ b/tests/Unit/Http/Middleware/API/SetSessionDriverTest.php @@ -0,0 +1,69 @@ +appMock = m::mock(Application::class); + $this->config = m::mock(Repository::class); + } + + /** + * Test that a production environment does not try to disable debug bar. + */ + public function testProductionEnvironment() + { + $this->appMock->shouldReceive('environment')->withNoArgs()->once()->andReturn('production'); + $this->config->shouldReceive('set')->with('session.driver', 'array')->once()->andReturnNull(); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Test that a local environment does disable debug bar. + */ + public function testLocalEnvironment() + { + $this->appMock->shouldReceive('environment')->withNoArgs()->once()->andReturn('local'); + $this->appMock->shouldReceive('make')->with(LaravelDebugbar::class)->once()->andReturnSelf(); + $this->appMock->shouldReceive('disable')->withNoArgs()->once()->andReturnNull(); + + $this->config->shouldReceive('set')->with('session.driver', 'array')->once()->andReturnNull(); + + $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); + } + + /** + * Return an instance of the middleware with mocked dependencies for testing. + * + * @return \Pterodactyl\Http\Middleware\API\SetSessionDriver + */ + private function getMiddleware(): SetSessionDriver + { + return new SetSessionDriver($this->appMock, $this->config); + } +} diff --git a/tests/Unit/Services/Api/KeyCreationServiceTest.php b/tests/Unit/Services/Api/KeyCreationServiceTest.php index 159d6425..49e0a2fd 100644 --- a/tests/Unit/Services/Api/KeyCreationServiceTest.php +++ b/tests/Unit/Services/Api/KeyCreationServiceTest.php @@ -12,8 +12,8 @@ namespace Tests\Unit\Services\Api; use Mockery as m; use Tests\TestCase; use phpmock\phpunit\PHPMock; +use Pterodactyl\Models\APIKey; use Illuminate\Database\ConnectionInterface; -use Illuminate\Contracts\Encryption\Encrypter; use Pterodactyl\Services\Api\PermissionService; use Pterodactyl\Services\Api\KeyCreationService; use Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface; @@ -23,45 +23,30 @@ class KeyCreationServiceTest extends TestCase use PHPMock; /** - * @var \Illuminate\Database\ConnectionInterface + * @var \Illuminate\Database\ConnectionInterface|\Mockery\Mock */ - protected $connection; + private $connection; /** - * @var \Illuminate\Contracts\Encryption\Encrypter + * @var \Pterodactyl\Services\Api\PermissionService|\Mockery\Mock */ - protected $encrypter; + private $permissionService; /** - * @var \Pterodactyl\Services\Api\PermissionService + * @var \Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface|\Mockery\Mock */ - protected $permissions; + private $repository; /** - * @var \Pterodactyl\Contracts\Repository\ApiKeyRepositoryInterface + * Setup tests. */ - protected $repository; - - /** - * @var \Pterodactyl\Services\Api\KeyCreationService - */ - protected $service; - public function setUp() { parent::setUp(); $this->connection = m::mock(ConnectionInterface::class); - $this->encrypter = m::mock(Encrypter::class); - $this->permissions = m::mock(PermissionService::class); + $this->permissionService = m::mock(PermissionService::class); $this->repository = m::mock(ApiKeyRepositoryInterface::class); - - $this->service = new KeyCreationService( - $this->repository, - $this->connection, - $this->encrypter, - $this->permissions - ); } /** @@ -69,37 +54,48 @@ class KeyCreationServiceTest extends TestCase */ public function testKeyIsCreated() { + $model = factory(APIKey::class)->make(); + $this->getFunctionMock('\\Pterodactyl\\Services\\Api', 'str_random') - ->expects($this->exactly(2))->willReturn('random_string'); + ->expects($this->exactly(1))->with(APIKey::KEY_LENGTH)->willReturn($model->token); $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->encrypter->shouldReceive('encrypt')->with('random_string')->once()->andReturn('encrypted-secret'); $this->repository->shouldReceive('create')->with([ 'test-data' => 'test', - 'public' => 'random_string', - 'secret' => 'encrypted-secret', - ], true, true)->once()->andReturn((object) ['id' => 1]); + 'token' => $model->token, + ], true, true)->once()->andReturn($model); - $this->permissions->shouldReceive('getPermissions')->withNoArgs()->once()->andReturn([ + $this->permissionService->shouldReceive('getPermissions')->withNoArgs()->once()->andReturn([ '_user' => ['server' => ['list', 'multiple-dash-test']], 'server' => ['create', 'admin-dash-test'], ]); - $this->permissions->shouldReceive('create')->with(1, 'user.server-list')->once()->andReturnNull(); - $this->permissions->shouldReceive('create')->with(1, 'user.server-multiple-dash-test')->once()->andReturnNull(); - $this->permissions->shouldReceive('create')->with(1, 'server-create')->once()->andReturnNull(); - $this->permissions->shouldReceive('create')->with(1, 'server-admin-dash-test')->once()->andReturnNull(); + $this->permissionService->shouldReceive('create')->with($model->id, 'user.server-list')->once()->andReturnNull(); + $this->permissionService->shouldReceive('create')->with($model->id, 'user.server-multiple-dash-test')->once()->andReturnNull(); + $this->permissionService->shouldReceive('create')->with($model->id, 'server-create')->once()->andReturnNull(); + $this->permissionService->shouldReceive('create')->with($model->id, 'server-admin-dash-test')->once()->andReturnNull(); $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); - $response = $this->service->handle( + $response = $this->getService()->handle( ['test-data' => 'test'], ['invalid-node', 'server-list', 'server-multiple-dash-test'], ['invalid-node', 'server-create', 'server-admin-dash-test'] ); $this->assertNotEmpty($response); - $this->assertEquals('random_string', $response); + $this->assertInstanceOf(APIKey::class, $response); + $this->assertSame($model, $response); + } + + /** + * Return an instance of the service with mocked dependencies for testing. + * + * @return \Pterodactyl\Services\Api\KeyCreationService + */ + private function getService(): KeyCreationService + { + return new KeyCreationService($this->repository, $this->connection, $this->permissionService); } }