From 4f1cca9f3ef58f3ca7cc59106f4fe156cdf1a2ef Mon Sep 17 00:00:00 2001 From: Richard Steinmetz Date: Thu, 18 Apr 2024 19:11:19 +0200 Subject: [PATCH] chore: refactor webauthn code after bumping web-auth/webauthn-lib Signed-off-by: Richard Steinmetz --- lib/Db/PublicKeyCredentialEntity.php | 4 +-- lib/Service/WebAuthnManager.php | 15 ++++----- src/components/AddDeviceDialog.vue | 32 ++++--------------- src/components/Challenge.vue | 12 +++---- src/tests/unit/utils/base64.spec.js | 12 +++---- src/utils/base64.js | 8 ++--- .../Controller/SettingsControllerTest.php | 8 ++++- tests/Unit/Provider/WebAuthnProviderTest.php | 2 +- 8 files changed, 38 insertions(+), 55 deletions(-) diff --git a/lib/Db/PublicKeyCredentialEntity.php b/lib/Db/PublicKeyCredentialEntity.php index 3be944af..9398a07d 100644 --- a/lib/Db/PublicKeyCredentialEntity.php +++ b/lib/Db/PublicKeyCredentialEntity.php @@ -28,7 +28,7 @@ namespace OCA\TwoFactorWebauthn\Db; use OCP\AppFramework\Db\Entity; -use Ramsey\Uuid\Uuid; +use Symfony\Component\Uid\Uuid; use Webauthn\PublicKeyCredentialSource; use Webauthn\TrustPath\TrustPathLoader; @@ -134,7 +134,7 @@ public static function fromPublicKeyCrendentialSource(string $name, $publicKeyCredentialEntity->setTransports(json_encode($publicKeyCredentialSource->getTransports())); $publicKeyCredentialEntity->setAttestationType($publicKeyCredentialSource->getAttestationType()); $publicKeyCredentialEntity->setTrustPath(json_encode($publicKeyCredentialSource->getTrustPath()->jsonSerialize())); - $publicKeyCredentialEntity->setAaguid($publicKeyCredentialSource->getAaguid()->toString()); + $publicKeyCredentialEntity->setAaguid($publicKeyCredentialSource->getAaguid()->toRfc4122()); $publicKeyCredentialEntity->setCredentialPublicKey(base64_encode($publicKeyCredentialSource->getCredentialPublicKey())); $publicKeyCredentialEntity->setUserHandle($publicKeyCredentialSource->getUserHandle()); $publicKeyCredentialEntity->setCounter($publicKeyCredentialSource->getCounter()); diff --git a/lib/Service/WebAuthnManager.php b/lib/Service/WebAuthnManager.php index 47446027..8756b6f1 100644 --- a/lib/Service/WebAuthnManager.php +++ b/lib/Service/WebAuthnManager.php @@ -26,7 +26,6 @@ namespace OCA\TwoFactorWebauthn\Service; -use Assert\Assertion; use Cose\Algorithm\Manager; use Cose\Algorithm\Signature\ECDSA; use Cose\Algorithm\Signature\EdDSA; @@ -132,8 +131,9 @@ public function startRegistration(IUser $user, string $serverHost): PublicKeyCre $authenticatorSelectionCriteria = new AuthenticatorSelectionCriteria( AuthenticatorSelectionCriteria::AUTHENTICATOR_ATTACHMENT_NO_PREFERENCE, + AuthenticatorSelectionCriteria::USER_VERIFICATION_REQUIREMENT_DISCOURAGED, + AuthenticatorSelectionCriteria::RESIDENT_KEY_REQUIREMENT_NO_PREFERENCE, false, - AuthenticatorSelectionCriteria::USER_VERIFICATION_REQUIREMENT_DISCOURAGED ); $publicKeyCredentialCreationOptions = new PublicKeyCredentialCreationOptions( @@ -141,11 +141,10 @@ public function startRegistration(IUser $user, string $serverHost): PublicKeyCre $userEntity, $challenge, $publicKeyCredentialParametersList, - $timeout, - $excludedPublicKeyDescriptors, $authenticatorSelectionCriteria, PublicKeyCredentialCreationOptions::ATTESTATION_CONVEYANCE_PREFERENCE_NONE, - null + $excludedPublicKeyDescriptors, + $timeout, ); $this->session->set(self::TWOFACTORAUTH_WEBAUTHN_REGISTRATION, $publicKeyCredentialCreationOptions->jsonSerialize()); @@ -280,15 +279,15 @@ function ($device) { $publicKeyCredentialRequestOptions = new PublicKeyCredentialRequestOptions( random_bytes(32), // Challenge - 60000, // Timeout null, // Relying Party ID [], // Registered PublicKeyCredentialDescriptor classes null, // User verification requirement + 60000, // Timeout $extensions ); $publicKeyCredentialRequestOptions ->setRpId($this->stripPort($serverHost)) - ->allowCredentials($registeredPublicKeyCredentialDescriptors) + ->allowCredentials(...$registeredPublicKeyCredentialDescriptors) ->setUserVerification(PublicKeyCredentialRequestOptions::USER_VERIFICATION_REQUIREMENT_DISCOURAGED); $this->session->set(self::TWOFACTORAUTH_WEBAUTHN_REQUEST, $publicKeyCredentialRequestOptions->jsonSerialize()); @@ -364,7 +363,6 @@ public function finishAuthenticate(IUser $user, string $data) { public function removeDevice(IUser $user, int $id) { $credential = $this->mapper->findById($id); - Assertion::eq($credential->getUserHandle(), $user->getUID()); $this->mapper->delete($credential); @@ -382,7 +380,6 @@ public function deactivateAllDevices(IUser $user) { public function changeActivationState(IUser $user, int $id, bool $active) { $credential = $this->mapper->findById($id); - Assertion::eq($credential->getUserHandle(), $user->getUID()); $credential->setActive($active); diff --git a/src/components/AddDeviceDialog.vue b/src/components/AddDeviceDialog.vue index c7bb140c..66974e7b 100644 --- a/src/components/AddDeviceDialog.vue +++ b/src/components/AddDeviceDialog.vue @@ -76,6 +76,7 @@ import { } from '../services/RegistrationService.js' import logger from '../logger.js' +import { arrayToBase64UnpaddedString, base64url2base64 } from '../utils/base64.js' const RegistrationSteps = Object.freeze({ READY: 1, @@ -106,27 +107,6 @@ export default { }, methods: { - arrayToBase64String(a) { - return btoa(String.fromCharCode(...a)) - }, - - base64url2base64(input) { - input = input - .replace(/=/g, '') - .replace(/-/g, '+') - .replace(/_/g, '/') - - const pad = input.length % 4 - if (pad) { - if (pad === 1) { - throw new Error('InvalidLengthError: Input base64url string is the wrong length to determine padding') - } - input += new Array(5 - pad).join('=') - } - - return input - }, - async start() { this.errorMessage = null logger.info('Starting to add a new twofactor webauthn device') @@ -156,11 +136,11 @@ export default { async getRegistrationData() { try { const publicKey = await startRegistration() - publicKey.challenge = Uint8Array.from(window.atob(this.base64url2base64(publicKey.challenge)), c => c.charCodeAt(0)) + publicKey.challenge = Uint8Array.from(window.atob(base64url2base64(publicKey.challenge)), c => c.charCodeAt(0)) publicKey.user.id = Uint8Array.from(window.atob(publicKey.user.id), c => c.charCodeAt(0)) if (publicKey.excludeCredentials) { publicKey.excludeCredentials = publicKey.excludeCredentials.map(data => { - data.id = Uint8Array.from(window.atob(this.base64url2base64(data.id)), c => c.charCodeAt(0)) + data.id = Uint8Array.from(window.atob(base64url2base64(data.id)), c => c.charCodeAt(0)) return data }) } @@ -180,10 +160,10 @@ export default { this.credential = { id: data.id, type: data.type, - rawId: this.arrayToBase64String(new Uint8Array(data.rawId)), + rawId: arrayToBase64UnpaddedString(new Uint8Array(data.rawId)), response: { - clientDataJSON: this.arrayToBase64String(new Uint8Array(data.response.clientDataJSON)), - attestationObject: this.arrayToBase64String(new Uint8Array(data.response.attestationObject)), + clientDataJSON: arrayToBase64UnpaddedString(new Uint8Array(data.response.clientDataJSON)), + attestationObject: arrayToBase64UnpaddedString(new Uint8Array(data.response.attestationObject)), }, } logger.debug('mapped credentials data') diff --git a/src/components/Challenge.vue b/src/components/Challenge.vue index 32c99d46..fe1cbbf8 100644 --- a/src/components/Challenge.vue +++ b/src/components/Challenge.vue @@ -67,7 +67,7 @@ import { mapGetters } from 'vuex' import { NcButton } from '@nextcloud/vue' import logger from '../logger.js' -import { arrayToBase64String, base64StringToArray, base64url2base64 } from '../utils/base64.js' +import { arrayToBase64UnpaddedString, base64StringToArray, base64url2base64 } from '../utils/base64.js' export default { name: 'Challenge', @@ -138,12 +138,12 @@ export default { const challenge = { id: data.id, type: data.type, - rawId: arrayToBase64String(new Uint8Array(data.rawId)), + rawId: arrayToBase64UnpaddedString(new Uint8Array(data.rawId)), response: { - authenticatorData: arrayToBase64String(new Uint8Array(data.response.authenticatorData)), - clientDataJSON: arrayToBase64String(new Uint8Array(data.response.clientDataJSON)), - signature: arrayToBase64String(new Uint8Array(data.response.signature)), - userHandle: data.response.userHandle ? arrayToBase64String(new Uint8Array(data.response.userHandle)) : null, + authenticatorData: arrayToBase64UnpaddedString(new Uint8Array(data.response.authenticatorData)), + clientDataJSON: arrayToBase64UnpaddedString(new Uint8Array(data.response.clientDataJSON)), + signature: arrayToBase64UnpaddedString(new Uint8Array(data.response.signature)), + userHandle: data.response.userHandle ? arrayToBase64UnpaddedString(new Uint8Array(data.response.userHandle)) : null, }, } logger.debug('mapped credentials', { challenge }) diff --git a/src/tests/unit/utils/base64.spec.js b/src/tests/unit/utils/base64.spec.js index e7115d5c..fb77c35e 100644 --- a/src/tests/unit/utils/base64.spec.js +++ b/src/tests/unit/utils/base64.spec.js @@ -20,14 +20,14 @@ * */ -import { arrayToBase64String, base64StringToArray, base64url2base64 } from '../../../utils/base64.js' +import { arrayToBase64UnpaddedString, base64StringToArray, base64url2base64 } from '../../../utils/base64.js' describe('utils/base64', () => { - it('should convert byte arrays to base64 strings', () => { - expect(arrayToBase64String(new Uint8Array([4, 2]))).to.equal('BAI=') - expect(arrayToBase64String(new Uint8Array([4]))).to.equal('BA==') - expect(arrayToBase64String(new Uint8Array([1, 2, 3, 4, 5, 6]))).to.equal('AQIDBAUG') - expect(arrayToBase64String(new Uint8Array([]))).to.equal('') + it('should convert byte arrays to unpadded base64 strings', () => { + expect(arrayToBase64UnpaddedString(new Uint8Array([4, 2]))).to.equal('BAI') + expect(arrayToBase64UnpaddedString(new Uint8Array([4]))).to.equal('BA') + expect(arrayToBase64UnpaddedString(new Uint8Array([1, 2, 3, 4, 5, 6]))).to.equal('AQIDBAUG') + expect(arrayToBase64UnpaddedString(new Uint8Array([]))).to.equal('') }) it('should convert base64 strings to byte arrays', () => { diff --git a/src/utils/base64.js b/src/utils/base64.js index 596872aa..b3812384 100644 --- a/src/utils/base64.js +++ b/src/utils/base64.js @@ -21,13 +21,13 @@ */ /** - * Encode an array of bytes as a base64 string. + * Encode an array of bytes as an unpadded base64 string. * * @param {Uint8Array} a An array of bytes - * @return {string} A base64 string + * @return {string} An unpadded base64 string */ -export function arrayToBase64String(a) { - return btoa(String.fromCharCode(...a)) +export function arrayToBase64UnpaddedString(a) { + return btoa(String.fromCharCode(...a)).replace(/=+$/g, '') } /** diff --git a/tests/Unit/Controller/SettingsControllerTest.php b/tests/Unit/Controller/SettingsControllerTest.php index d44aedd7..f8f854af 100644 --- a/tests/Unit/Controller/SettingsControllerTest.php +++ b/tests/Unit/Controller/SettingsControllerTest.php @@ -34,6 +34,8 @@ use OCP\IUserSession; use PHPUnit\Framework\MockObject\MockObject; use Webauthn\PublicKeyCredentialCreationOptions; +use Webauthn\PublicKeyCredentialRpEntity; +use Webauthn\PublicKeyCredentialUserEntity; class SettingsControllerTest extends TestCase { @@ -64,7 +66,11 @@ public function testStartRegister(): void { $this->userSession->expects(self::once()) ->method('getUser') ->willReturn($user); - $publicKeyCredentialCreationOptions = $this->createMock(PublicKeyCredentialCreationOptions::class); + $publicKeyCredentialCreationOptions = new PublicKeyCredentialCreationOptions( + new PublicKeyCredentialRpEntity('relying_party'), + new PublicKeyCredentialUserEntity('user', 'user_id', 'User'), + 'challenge', + ); $this->webauthnManager->expects(self::once()) ->method('startRegistration') ->with(self::equalTo($user)) diff --git a/tests/Unit/Provider/WebAuthnProviderTest.php b/tests/Unit/Provider/WebAuthnProviderTest.php index a0612cfe..ba9f4969 100644 --- a/tests/Unit/Provider/WebAuthnProviderTest.php +++ b/tests/Unit/Provider/WebAuthnProviderTest.php @@ -111,7 +111,7 @@ public function testGetDescription(): void { public function testGetTemplate(): void { $user = $this->createMock(IUser::class); - $key = $this->createMock(PublicKeyCredentialRequestOptions::class); + $key = new PublicKeyCredentialRequestOptions('challenge'); $serverHost = 'my.next.cloud'; $this->request->expects(self::once()) ->method('getServerHost')