From 81a6a8653f51913fa1f1f638e4c567f8f64d3b55 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 8 Aug 2021 11:24:11 -0700 Subject: [PATCH] Fix up creation of keys to fail when registering the same key again --- .../Api/Client/SecurityKeyController.php | 41 +++++---- app/Models/SecurityKey.php | 89 +++++++++++++++---- .../PublicKeyCredentialSourceRepository.php | 20 +++-- .../CreatePublicKeyCredentialsService.php | 9 +- ...8_07_170141_create_security_keys_table.php | 2 +- .../account/webauthn/registerWebauthnKey.ts | 6 +- 6 files changed, 110 insertions(+), 57 deletions(-) diff --git a/app/Http/Controllers/Api/Client/SecurityKeyController.php b/app/Http/Controllers/Api/Client/SecurityKeyController.php index 125b09c76..5f9ff713b 100644 --- a/app/Http/Controllers/Api/Client/SecurityKeyController.php +++ b/app/Http/Controllers/Api/Client/SecurityKeyController.php @@ -2,7 +2,6 @@ namespace Pterodactyl\Http\Controllers\Api\Client; -use Exception; use Carbon\CarbonImmutable; use Illuminate\Support\Str; use Illuminate\Http\Request; @@ -53,7 +52,7 @@ class SecurityKeyController extends ClientApiController public function create(Request $request): JsonResponse { $tokenId = Str::random(64); - $credentials = $this->createPublicKeyCredentials->handle($request->user(), $request->get('display_name')); + $credentials = $this->createPublicKeyCredentials->handle($request->user()); $this->cache->put( "register-security-key:$tokenId", @@ -72,30 +71,30 @@ class SecurityKeyController extends ClientApiController /** * Stores a new key for a user account. * - * @throws \Exception * @throws \Pterodactyl\Exceptions\DisplayException + * @throws \Throwable */ public function store(RegisterWebauthnTokenRequest $request): JsonResponse { - $stored = $this->cache->pull("register-security-key:{$request->input('token_id')}"); - - if (!$stored) { - throw new DisplayException('Could not register security key: no data present in session, please try your request again.'); - } - - $credentials = unserialize($stored); - if (!$credentials instanceof PublicKeyCredentialCreationOptions) { - throw new Exception(sprintf('Unexpected security key data pulled from cache: expected "%s" but got "%s".', PublicKeyCredentialCreationOptions::class, get_class($credentials))); - } - - $server = $this->webauthnServerRepository->getServer($request->user()); - - $source = $server->loadAndCheckAttestationResponse( - json_encode($request->input('registration')), - $credentials, - $this->getServerRequest($request), + $credentials = unserialize( + $this->cache->pull("register-security-key:{$request->input('token_id')}", serialize(null)) ); + if ( + !is_object($credentials) || + !$credentials instanceof PublicKeyCredentialCreationOptions || + $credentials->getUser()->getId() !== $request->user()->uuid + ) { + throw new DisplayException('Could not register security key: invalid data present in session, please try again.'); + } + + $source = $this->webauthnServerRepository->getServer($request->user()) + ->loadAndCheckAttestationResponse( + json_encode($request->input('registration')), + $credentials, + $this->getServerRequest($request), + ); + // Unfortunately this repository interface doesn't define a response — it is explicitly // void — so we need to just query the database immediately after this to pull the information // we just stored to return to the caller. @@ -108,7 +107,7 @@ class SecurityKeyController extends ClientApiController $created->update(['name' => $request->input('name')]); return new JsonResponse([ - 'data' => $created->toArray(), + 'data' => [], ]); } diff --git a/app/Models/SecurityKey.php b/app/Models/SecurityKey.php index 947491460..eee9d62d0 100644 --- a/app/Models/SecurityKey.php +++ b/app/Models/SecurityKey.php @@ -2,8 +2,14 @@ namespace Pterodactyl\Models; +use Stringable; +use Ramsey\Uuid\Uuid; +use Ramsey\Uuid\UuidInterface; +use Webauthn\TrustPath\TrustPath; use Webauthn\PublicKeyCredentialSource; +use Webauthn\TrustPath\TrustPathLoader; use Webauthn\PublicKeyCredentialDescriptor; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Factories\HasFactory; class SecurityKey extends Model @@ -15,7 +21,6 @@ class SecurityKey extends Model protected $casts = [ 'user_id' => 'int', 'transports' => 'array', - 'trust_path' => 'array', 'other_ui' => 'array', ]; @@ -24,12 +29,65 @@ class SecurityKey extends Model 'user_id', ]; - public function user() + public function getPublicKeyAttribute(string $value): string + { + return base64_decode($value); + } + + public function setPublicKeyAttribute(string $value): void + { + $this->attributes['public_key'] = base64_encode($value); + } + + public function getPublicKeyIdAttribute(string $value): string + { + return base64_decode($value); + } + + public function setPublicKeyIdAttribute(string $value): void + { + $this->attributes['public_key_id'] = base64_encode($value); + } + + public function getTrustPathAttribute(?string $value): ?TrustPath + { + if (is_null($value)) { + return null; + } + + return TrustPathLoader::loadTrustPath(json_decode($value, true)); + } + + public function setTrustPathAttribute(?TrustPath $value): void + { + $this->attributes['trust_path'] = json_encode($value); + } + + /** + * @param \Ramsey\Uuid\UuidInterface|string|null $value + */ + public function setAaguidAttribute($value): void + { + $value = $value instanceof UuidInterface ? $value->__toString() : $value; + + $this->attributes['aaguid'] = (is_null($value) || $value === Uuid::NIL) ? null : $value; + } + + public function getAaguidAttribute(?string $value): ?UuidInterface + { + if (!is_null($value) && Uuid::isValid($value)) { + return Uuid::fromString($value); + } + + return null; + } + + public function user(): BelongsTo { return $this->belongsTo(User::class); } - public function toCredentialsDescriptor() + public function getPublicKeyCredentialsDescriptorAttribute(): PublicKeyCredentialDescriptor { return new PublicKeyCredentialDescriptor( $this->type, @@ -38,19 +96,18 @@ class SecurityKey extends Model ); } - public function toCredentialSource(): PublicKeyCredentialSource + public function getPublicKeyCredentialSourceAttribute(): PublicKeyCredentialSource { - return PublicKeyCredentialSource::createFromArray([ - 'publicKeyCredentialId' => $this->public_key_id, - 'type' => $this->type, - 'transports' => $this->transports, - 'attestationType' => $this->attestation_type, - // 'trustPath' => $key->trustPath->jsonSerialize(), - 'aaguid' => $this->aaguid, - 'credentialPublicKey' => $this->public_key, - 'userHandle' => $this->user_handle, - 'counter' => $this->counter, - 'otherUI' => $this->other_ui, - ]); + return new PublicKeyCredentialSource( + $this->public_key_id, + $this->type, + $this->transports, + $this->attestation_type, + $this->trust_path, + $this->aaguid ?? Uuid::fromString(Uuid::NIL), + $this->public_key, + (string) $this->user_id, + $this->counter + ); } } diff --git a/app/Repositories/SecurityKeys/PublicKeyCredentialSourceRepository.php b/app/Repositories/SecurityKeys/PublicKeyCredentialSourceRepository.php index ed1971fce..8c5269ea4 100644 --- a/app/Repositories/SecurityKeys/PublicKeyCredentialSourceRepository.php +++ b/app/Repositories/SecurityKeys/PublicKeyCredentialSourceRepository.php @@ -29,7 +29,7 @@ class PublicKeyCredentialSourceRepository implements PublicKeyRepositoryInterfac ->where('public_key_id', $id) ->first(); - return $key ? $key->toCredentialSource() : null; + return $key ? $key->public_key_credential_source : null; } /** @@ -43,7 +43,7 @@ class PublicKeyCredentialSourceRepository implements PublicKeyRepositoryInterfac ->get(); return $results->map(function (SecurityKey $key) { - return $key->toCredentialSource(); + return $key->public_key_credential_source; })->values()->toArray(); } @@ -54,20 +54,24 @@ class PublicKeyCredentialSourceRepository implements PublicKeyRepositoryInterfac */ public function saveCredentialSource(PublicKeyCredentialSource $source): void { - $this->user->securityKeys()->forceCreate([ - 'uuid' => Uuid::uuid4()->toString(), + $key = $this->user->securityKeys()->make(); + + $key->forceFill([ + 'uuid' => Uuid::uuid4(), 'user_id' => $this->user->id, - 'public_key_id' => base64_encode($source->getPublicKeyCredentialId()), - 'public_key' => base64_encode($source->getCredentialPublicKey()), - 'aaguid' => $source->getAaguid()->toString(), + 'public_key_id' => $source->getPublicKeyCredentialId(), + 'public_key' => $source->getCredentialPublicKey(), + 'aaguid' => $source->getAaguid(), 'type' => $source->getType(), 'transports' => $source->getTransports(), 'attestation_type' => $source->getAttestationType(), - 'trust_path' => $source->getTrustPath()->jsonSerialize(), + 'trust_path' => $source->getTrustPath(), 'user_handle' => $source->getUserHandle(), 'counter' => $source->getCounter(), 'other_ui' => $source->getOtherUI(), ]); + + $key->saveOrFail(); } /** diff --git a/app/Services/Users/SecurityKeys/CreatePublicKeyCredentialsService.php b/app/Services/Users/SecurityKeys/CreatePublicKeyCredentialsService.php index 5c8a0daca..dadde95f7 100644 --- a/app/Services/Users/SecurityKeys/CreatePublicKeyCredentialsService.php +++ b/app/Services/Users/SecurityKeys/CreatePublicKeyCredentialsService.php @@ -2,7 +2,6 @@ namespace Pterodactyl\Services\Users\SecurityKeys; -use Ramsey\Uuid\Uuid; use Pterodactyl\Models\User; use Pterodactyl\Models\SecurityKey; use Webauthn\PublicKeyCredentialUserEntity; @@ -18,14 +17,12 @@ class CreatePublicKeyCredentialsService $this->webauthnServerRepository = $webauthnServerRepository; } - public function handle(User $user, ?string $displayName): PublicKeyCredentialCreationOptions + public function handle(User $user): PublicKeyCredentialCreationOptions { - $id = Uuid::uuid4()->toString(); - - $entity = new PublicKeyCredentialUserEntity($user->uuid, $id, $name ?? $user->email); + $entity = new PublicKeyCredentialUserEntity($user->username, $user->uuid, $user->email, null); $excluded = $user->securityKeys->map(function (SecurityKey $key) { - return $key->toCredentialsDescriptor(); + return $key->public_key_credentials_descriptor; })->values()->toArray(); $server = $this->webauthnServerRepository->getServer($user); diff --git a/database/migrations/2021_08_07_170141_create_security_keys_table.php b/database/migrations/2021_08_07_170141_create_security_keys_table.php index a2efaddb4..2636848e2 100644 --- a/database/migrations/2021_08_07_170141_create_security_keys_table.php +++ b/database/migrations/2021_08_07_170141_create_security_keys_table.php @@ -20,7 +20,7 @@ class CreateSecurityKeysTable extends Migration $table->string('name'); $table->text('public_key_id'); $table->text('public_key'); - $table->char('aaguid', 36); + $table->char('aaguid', 36)->nullable(); $table->string('type'); $table->json('transports'); $table->string('attestation_type'); diff --git a/resources/scripts/api/account/webauthn/registerWebauthnKey.ts b/resources/scripts/api/account/webauthn/registerWebauthnKey.ts index 4532a513c..4d62f7aa0 100644 --- a/resources/scripts/api/account/webauthn/registerWebauthnKey.ts +++ b/resources/scripts/api/account/webauthn/registerWebauthnKey.ts @@ -46,11 +46,7 @@ const registerCredentialForAccount = async (name: string, tokenId: string, crede }; export const register = async (name: string): Promise => { - const { data } = await http.get('/api/client/account/security-keys/register', { - params: { - display_name: name, - }, - }); + const { data } = await http.get('/api/client/account/security-keys/register'); const publicKey = data.data.credentials; publicKey.challenge = bufferDecode(base64Decode(publicKey.challenge));