Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax requireUserVerification when the hardware key is not used for passwordless login | パスワードレスログインを使用されていない場合は、requireUserVerification: falseにしてほしい #15057

Open
1 task done
eternal-flame-AD opened this issue Nov 25, 2024 · 0 comments
Labels
✨Feature This adds/improves/enhances a feature 🔒Security Security related issue/PR

Comments

@eternal-flame-AD
Copy link
Contributor

Summary

タイトル通りです。Googleも同じロジックを採用していると思います。その理由は次のセクションで説明します。

Suggested change:

diff --git a/packages/backend/src/core/WebAuthnService.ts b/packages/backend/src/core/WebAuthnService.ts
index 75ab0a207c..734374aa55 100644
--- a/packages/backend/src/core/WebAuthnService.ts
+++ b/packages/backend/src/core/WebAuthnService.ts
@@ -83,7 +83,11 @@ export class WebAuthnService {
 	}
 
 	@bindThis
-	public async verifyRegistration(userId: MiUser['id'], response: RegistrationResponseJSON): Promise<{
+	public async verifyRegistration(
+		userId: MiUser['id'], 
+		response: RegistrationResponseJSON,
+		twoFactorOnly: boolean = false,
+	): Promise<{
 		credentialID: string;
 		credentialPublicKey: Uint8Array;
 		attestationObject: Uint8Array;
@@ -111,7 +115,7 @@ export class WebAuthnService {
 				expectedChallenge: challenge,
 				expectedOrigin: relyingParty.origin,
 				expectedRPID: relyingParty.rpId,
-				requireUserVerification: true,
+				requireUserVerification: !twoFactorOnly,
 			});
 		} catch (error) {
 			console.error(error);
@@ -245,7 +249,11 @@ export class WebAuthnService {
 	}
 
 	@bindThis
-	public async verifyAuthentication(userId: MiUser['id'], response: AuthenticationResponseJSON): Promise<boolean> {
+	public async verifyAuthentication(
+		userId: MiUser['id'], 
+		response: AuthenticationResponseJSON,
+		twoFactorOnly: boolean = false,
+	): Promise<boolean> {
 		const challenge = await this.redisClient.get(`webauthn:challenge:${userId}`);
 
 		if (!challenge) {
@@ -302,7 +310,7 @@ export class WebAuthnService {
 					counter: key.counter,
 					transports: key.transports ? key.transports as AuthenticatorTransportFuture[] : undefined,
 				},
-				requireUserVerification: true,
+				requireUserVerification: !twoFactorOnly,
 			});
 		} catch (error) {
 			console.error(error);
diff --git a/packages/backend/src/server/api/SigninApiService.ts b/packages/backend/src/server/api/SigninApiService.ts
index 0327f17ea5..56f41852fc 100644
--- a/packages/backend/src/server/api/SigninApiService.ts
+++ b/packages/backend/src/server/api/SigninApiService.ts
@@ -255,7 +255,7 @@ export class SigninApiService {
 				});
 			}
 
-			const authorized = await this.webAuthnService.verifyAuthentication(user.id, body.credential);
+			const authorized = await this.webAuthnService.verifyAuthentication(user.id, body.credential, !profile.usePasswordLessLogin);
 
 			if (authorized) {
 				return this.signinService.signin(request, reply, user);
diff --git a/packages/backend/src/server/api/endpoints/i/2fa/key-done.ts b/packages/backend/src/server/api/endpoints/i/2fa/key-done.ts
index 65eece5b97..d46032c7bf 100644
--- a/packages/backend/src/server/api/endpoints/i/2fa/key-done.ts
+++ b/packages/backend/src/server/api/endpoints/i/2fa/key-done.ts
@@ -95,7 +95,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> {
 				throw new ApiError(meta.errors.twoFactorNotEnabled);
 			}
 
-			const keyInfo = await this.webAuthnService.verifyRegistration(me.id, ps.credential);
+			const keyInfo = await this.webAuthnService.verifyRegistration(me.id, ps.credential, !profile.usePasswordLessLogin);
 			const keyId = keyInfo.credentialID;
 
 			await this.userSecurityKeysRepository.insert({
diff --git a/packages/frontend/src/components/MkSignin.passkey.vue b/packages/frontend/src/components/MkSignin.passkey.vue
index e5a56ab66d..4e9ab61b4d 100644
--- a/packages/frontend/src/components/MkSignin.passkey.vue
+++ b/packages/frontend/src/components/MkSignin.passkey.vue
@@ -45,7 +45,8 @@ const queryingKey = ref(true);
 async function queryKey() {
 	queryingKey.value = true;
 	await webAuthnRequest(props.credentialRequest)
-		.catch(() => {
+		.catch((e) => {
+			console.error(e);
 			return Promise.reject(null);
 		})
 		.then((credential) => {
@@ -53,7 +54,7 @@ async function queryKey() {
 		})
 		.finally(() => {
 			queryingKey.value = false;
-		});
+	});
 }
 
 onMounted(() => {

Purpose

Yubikeyを2FAとして登録してみました。同じキーで OATH アプリケーションを使用しといるが、これによりコピペの手間が省けることを期待しています。

しかしこれでキーの登録拒否されました。普段使用している同じモデルのパスワード付きキーを登録できます。

expectedOrigin: relyingParty.origin,
expectedRPID: relyingParty.rpId,
requireUserVerification: true,
});
} catch (error) {
console.error(error);
throw new IdentifiableError('5c1446f8-8ca7-4d31-9f39-656afe9c5d87', 'verification failed');
}

YubikeyなどのキーはPIN を 8 回間違えると、秘密が破棄されます。したがって、自分の間違いや悪意のある破壊を防ぐために、パスワードで保護されていないキーをバックアップとして自宅に保管しています。

If the FIDO2 PIN is entered incorrectly 3 times in a row, the key will need to be reinserted before it will accept additional PIN attempts (reinserting "reboots" the device). If the PIN is entered incorrectly a total of 8 times in a row, the FIDO2 function will become blocked, requiring that it be reset. The number of remaining retries can be viewed at any time in YubiKey Manager by navigating to Applications > FIDO2.

これにより、正当な理由があるにもかかわらず、ユーザーは PIN を設定することを余儀なくされ、実際に2FAエンゲージメントが低下する恐れがあります。OATH アプリにパスワードを設定している人もあまりいないと思います。2FA 専用のキーについては、ユーザーの要求に応じて、保護されていないキーの設定を許可する必要があると思います。その際には、その意味をユーザーが理解できるように警告を出すとよいでしょう。

2FA 専用のキーのは userVerification: 'preferred' は十分だと思います。

excludeCredentials: keys.map(key => (<{ id: string; transports?: AuthenticatorTransportFuture[]; }>{
id: key.id,
transports: key.transports ?? undefined,
})),
authenticatorSelection: {
residentKey: 'required',
userVerification: 'preferred',
},
});

Key spec:

Device type: YubiKey 5C NFC
Serial number: XXXXXX
Firmware version: 5.4.3
Form factor: Keychain (USB-C)
Enabled USB interfaces: OTP, FIDO, CCID
NFC transport is enabled

Applications	USB    	NFC
Yubico OTP  	Enabled	Enabled
FIDO U2F    	Enabled	Enabled
FIDO2       	Enabled	Enabled
OATH        	Enabled	Enabled
PIV         	Enabled	Enabled
OpenPGP     	Enabled	Enabled
YubiHSM Auth	Enabled	Enabled

PIN:                8 attempt(s) remaining
Minimum PIN length: 4

Do you want to implement this feature yourself?

  • Yes, I will implement this by myself and send a pull request
@eternal-flame-AD eternal-flame-AD added the ✨Feature This adds/improves/enhances a feature label Nov 25, 2024
@samunohito samunohito added the 🔒Security Security related issue/PR label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨Feature This adds/improves/enhances a feature 🔒Security Security related issue/PR
Projects
Development

No branches or pull requests

2 participants