Support WebAuthn #427

mcguffin · 2022-01-21T20:37:28Z

As announced in #423 here's my PR.

Tested with:

Windows 10 (VirtualBox)
Firefox macOS 10.15 using SoftU2F
Chrome macOS using Chrome's native OS support
Firefox Android 10
Safari on iOS 14.4

Todo (any help on these is very welcome):

Test more browsers and more devices
Accessibility
Make UI less mcguffin and more WordPress

sjinks · 2022-01-24T02:26:43Z

includes/WebAuthn/class-webauthn-keystore.php

+		return $wpdb->query( $wpdb->prepare(
+			"DELETE FROM $wpdb->usermeta WHERE user_id=%d AND meta_key=%s AND meta_value LIKE %s",
+			$user_id,
+			self::PUBKEY_USERMETA_KEY,
+			'%' . $keyLike . '%'
+		) ) !== 0;


Ideally, you need to fetch the record first and then delete it by ID. You need the "fetch first" thing to update the meta cache, the same way that delete_metadata() does. Otherwise, get_keys() may yield unexpected result if the site uses an object cache.

sjinks · 2022-01-24T02:31:56Z

providers/class-two-factor-webauthn.php

+	 */
+	public function get_app_id() {
+
+		$fqdn = wp_parse_url( network_site_url(), PHP_URL_HOST );


It is probably not critical, but WebAuthn requires an origin here, not just the domain. That is if the webserver for some reason uses a non-standard port for HTTPS (say, 8443), it needs to be included in the AppID.

The best way is probably to copy the logic from get_u2f_app_id() (just omit the protocol).

sjinks · 2022-01-24T02:41:58Z

providers/class-two-factor-webauthn.php

+			$key->last_used = false;
+			$key->tested    = false;
+
+			if ( false !== $this->key_store->find_key( $user_id, $key->md5id ) ) {


This is probably not critical, but…

According to the Registration Ceremony, Item 22, the credential must be unique site-wide:

Check that the credentialId is not yet registered to any other user. If registration is requested for a credential that is already registered to a different user, the Relying Party SHOULD fail this registration ceremony, or it MAY decide to accept the registration, e.g. while deleting the older registration.

mcguffin · 2022-01-24T11:09:26Z

@sjinks Thanks for review!

jeffpaul · 2022-03-09T16:08:30Z

@sjinks mind re-reviewing after the latest changes from @mcguffin?

jeffpaul · 2022-09-09T15:54:52Z

@sjinks mind re-reviewing after the latest changes from @mcguffin?

sjinks

LGTM, but I haven't tried to run this locally.

includes/WebAuthn/class-cbor-decoder.php

includes/WebAuthn/class-webauthn-keystore.php

providers/js/webauthn-admin.js

providers/js/webauthn-login.js

jeffpaul · 2022-09-20T02:51:58Z

@sjinks thanks, MUCH appreciated!

@mcguffin any thoughts on the feedback above / ability to work on updates accordingly?

iandunn · 2022-11-14T22:54:15Z

Ah, good point, it looks like even just the .php files is still 17MB 🙈 ( composer install --no-dev && find . -name "*.php" | xargs du -sch). madwizard/webauthn-server is 4.4MB in comparison.

I was also assuming that the 4.x.x branches required PHP 7.x, but it looks like it's actually 8.1, which would probably be too strict even if it was only for this provider. So we'd probably would need to use the 3.3.x branch like Jörn said. But then, we probably can't assume that they'll do security patches for that branch, so that doesn't seem viable to me.

paulschreiber · 2023-02-20T03:57:55Z

What's the ETA on getting this merged?

riastradh · 2023-03-04T02:53:29Z

Would funding help with getting this and #491 finished, tested, and merged soon, and 0.8.0 released?

iandunn · 2023-03-10T23:36:58Z

IIRC, that was more than wordpress.org allowed (10 MB).

I looked for the details on that, and didn't see anything in the handbook or on make/Plugins. Jetpack is 222MB unzipped and WordFence is 63MB, but Two Factor is only 1.7MB, so I think we're probably safe to add whatever library we need. We should still prune any files that won't be used, though.

I'm still leaning towards madwizard-org/webauthn-server being the best one that supports PHP 7.x.

StevenDufresne · 2023-04-21T02:53:03Z

providers/css/webauthn-admin.css

+	flex-wrap: wrap;
+	-webkit-box-align: last baseline;
+	-ms-flex-align: last baseline;
+	align-items: last baseline;


We can probably center align this row.

Suggested change

align-items: last baseline;

align-items: center;

State Before After

With Notice

StevenDufresne · 2023-04-21T02:56:15Z

providers/css/webauthn-admin.css

+	content: "";
+}
+.webauthn-key [data-tested="untested"] {
+	color: #ccc;


Suggested change

color: #ccc;

color: #ccc;

vertical-align: bottom;

Align the icon better with text.

Before After

Needing vertical-align is usually a signal to refactor torwards flexbox. We've come a long way, but browsers still render vertical-align differently in various cases. Life is too short for that.

StevenDufresne · 2023-04-21T03:04:41Z

providers/js/webauthn-login.js

+
+	const login = ( opts, callback ) => {
+
+		const { action, payload, _wpnonce } = opts;


action & _wpnonce are deconstructured but I don't any references.

StevenDufresne · 2023-04-21T03:11:25Z

providers/js/webauthn-admin.js

+			url: wp.ajax.settings.url,
+			method: 'post',
+			data: opts,
+			success:callback


Suggested change

success:callback

success: callback

ravinderk

@mcguffin I left a few suggestions on code. Feel free to ask questions.

I noticed that new files need to be formatted correctly. You cannot catch formatting issues because includes directory is excluded. I think you can remove [this line]( /includes/) to get the correct formatting results locally.

ravinderk · 2023-05-03T06:14:32Z

includes/WebAuthn/class-webauthn-keystore.php

+			self::PUBKEY_USERMETA_KEY,
+			'%' . $wpdb->esc_like( $keyLike ) . '%'
+		) );
+		foreach ( $found as $key ) {


get_results function returns array|object|null. Can you validate the result before processing it?

Is there any reason we are querying all possible results? But returning the result value from the first item in the result.
Do you think we can limit the result to 1?

ravinderk · 2023-05-03T06:20:48Z

includes/WebAuthn/class-webauthn-keystore.php

+	 * @return bool
+	 */
+	public function delete_key( $user_id, $keyLike ) {
+		global $wpdb;


Can we remove unused global param?

ravinderk · 2023-05-03T06:23:35Z

providers/class-two-factor-webauthn.php

+	/**
+	 * Enqueue assets for login form.
+	 */
+	public function login_enqueue_assets() {


Are we using this function anywhere?

ravinderk · 2023-05-03T06:26:42Z

providers/class-two-factor-webauthn.php

+					'error'   => $err->getMessage(),
+				)
+			);
+			return;


This is not required because execution will die in wp_send_json . Do you think we can remove this?

ravinderk · 2023-05-03T06:30:23Z

providers/class-two-factor-webauthn.php

+			$this->key_store->create_key( $user_id, $key );
+
+		} catch ( Exception $err ) {
+			wp_send_json(


Canw e use wp_send_json_success and wp_send_json_error function?

ravinderk · 2023-05-03T06:47:55Z

providers/class-two-factor-webauthn.php

+			$this->key_store->save_key( $user_id, $key, $key->md5id );
+		}
+
+		wp_send_json(


Can we replace it with wp_send_json_error and pass WP_Error as in other places?

xknown · 2023-05-24T11:00:37Z

includes/WebAuthn/class-webauthn-handler.php

+        if ( $info->response->clientData->challenge
+					!==
+			rtrim( strtr( base64_encode( self::arrayToString( $info->originalChallenge ) ), '+/', '-_'), '=')
+		) {
+			$this->last_error[ $this->last_call ] = 'info-challenge-mismatch';
+			return false;
+        }


We need to make sure the challenge sent in prepareAuthenticate needs to be same here. We are checking $info->originalChallenge here, but the problem is that $info is a user supplied value -- see https://www.w3.org/TR/webauthn-2/#sctn-cryptographic-challenges

xknown · 2023-05-24T11:24:41Z

includes/WebAuthn/class-webauthn-handler.php

+		if ( ! is_array( $info->response->attestationObject ) ) {
+			$this->last_error[ $this->last_call ] = 'info-response-malformed-property';
+			return false;
+		}


The registration challenge needs to be checked against the challenge sent in prepareRegister -- see https://www.w3.org/TR/webauthn-2/#sctn-cryptographic-challenges

xknown · 2023-05-24T11:52:08Z

includes/WebAuthn/class-webauthn-handler.php

+    	/* check response from key and store as new identity. This is a hex string representing the raw CBOR
+    	attestation object received from the key */
+
+		$attData = $this->parseAttestationObject( $info->response->attestationObject );


It seems like there are quite a bit of steps that are not currently implemented here -- https://www.w3.org/TR/webauthn-2/#sctn-registering-a-new-credential. Is that intentional? If so, why?

xknown · 2023-05-24T12:03:22Z

includes/WebAuthn/class-webauthn-keystore.php

+			"SELECT COUNT(*) FROM $wpdb->usermeta WHERE meta_key=%s AND meta_value LIKE %s",
+			self::PUBKEY_USERMETA_KEY,
+			'%' . $wpdb->esc_like( serialize( $keyLike ) ) . '%'
+		) );


Would it make sense to only use core functions like get_user_meta( $user_id, self::PUBKEY_USERMETA_KEY ) and then doing the lookup on PHP? This would avoid uncached queries for sites that make use of object caching plugins.

jornfranke · 2023-09-30T15:15:51Z

@kasparsd could you review it or assign someone else for review to unblock this?

georgestephanis · 2023-10-02T18:23:34Z

I think the main blocker on the current PR is getting context or resolution on the questions raised in a few of the above reviews.

joshbetz · 2024-07-06T16:28:50Z

providers/class-two-factor-webauthn.php

+	 */
+	public function get_app_id() {
+
+		$url_parts = wp_parse_url( network_site_url() );


What is the ideal workflow on a network that has sites with different domains?

joshbetz · 2024-07-06T16:31:03Z

It's not working in Safari for me 🤔 I'm able to register my browser with Touch ID, but logging in with Touch ID is not working for me.

mcguffin added 3 commits January 21, 2022 20:02

Chew and swallow mcguffin/two-factor-webauthn

f357743

unminify css

be4602e

Test and debug

bad7b0d

sjinks reviewed Jan 24, 2022

View reviewed changes

mcguffin added 4 commits January 24, 2022 11:35

change WebAuthnKeyStore::find_key() function args

735bb93

WebAuthnKeyStore: check if key exists before delete

edbd8ff

allow non-standard ports in Two_Factor_WebAuthn::get_app_id()

a65f706

Two_Factor_WebAuthn: make sure key is not registered for another user

9974a5d

mcguffin mentioned this pull request Jan 24, 2022

U2F support in the future versions of Chrome #423

Open

jeffpaul linked an issue Jan 24, 2022 that may be closed by this pull request

U2F support in the future versions of Chrome #423

Open

jeffpaul requested a review from kasparsd January 24, 2022 18:01

jeffpaul assigned mcguffin Jan 24, 2022

jeffpaul added this to the 0.8.0 milestone Jan 24, 2022

dziudek mentioned this pull request May 20, 2022

Remove U2F #439

Open

jeffpaul modified the milestones: 0.7.2, 0.8.0 Sep 12, 2022

sjinks approved these changes Sep 13, 2022

View reviewed changes

sjinks reviewed Sep 13, 2022

View reviewed changes

includes/WebAuthn/class-cbor-decoder.php Outdated Show resolved Hide resolved

sjinks reviewed Sep 13, 2022

View reviewed changes

includes/WebAuthn/class-webauthn-keystore.php Outdated Show resolved Hide resolved

sjinks reviewed Sep 13, 2022

View reviewed changes

providers/js/webauthn-admin.js Outdated Show resolved Hide resolved

sjinks reviewed Sep 13, 2022

View reviewed changes

providers/js/webauthn-admin.js Outdated Show resolved Hide resolved

sjinks reviewed Sep 13, 2022

View reviewed changes

providers/js/webauthn-admin.js Outdated Show resolved Hide resolved

sjinks reviewed Sep 13, 2022

View reviewed changes

providers/js/webauthn-admin.js Outdated Show resolved Hide resolved

sjinks reviewed Sep 13, 2022

View reviewed changes

providers/js/webauthn-login.js Outdated Show resolved Hide resolved

dd32 mentioned this pull request Dec 14, 2022

Use the Rest API for setting up individual providers #504

Merged

paulschreiber mentioned this pull request Feb 20, 2023

Use simpler/less-technical wording and UI. #521

Merged

carstingaxion mentioned this pull request Feb 21, 2023

Suport 2FA-Plugin stuttter/wp-user-profiles#76

Closed

riastradh mentioned this pull request Mar 4, 2023

Upgrade path question for future two-factor 0.8.0 sjinks/wp-two-factor-provider-webauthn#400

Closed

iandunn modified the milestones: 0.8.0, 0.9.0 Mar 6, 2023

This was referenced Apr 17, 2023

U2F Keys broken with WordPress 6.2 #553

Closed

Turn on WebAuthn in wp-admin WordPress/wporg-two-factor#114

Closed

StevenDufresne reviewed Apr 21, 2023

View reviewed changes

iandunn mentioned this pull request Apr 25, 2023

Fork upstream WebAuthn provider WordPress/wporg-two-factor#134

Closed

dd32 mentioned this pull request Apr 28, 2023

Make text spacing and buttons consistent between TOTP and Backup Code screens #515

Open

ravinderk suggested changes May 3, 2023

View reviewed changes

xknown reviewed May 24, 2023

View reviewed changes

kasparsd mentioned this pull request Apr 25, 2024

Plugin hangs when the security plugin option is clicked #596

Closed

dd32 mentioned this pull request May 7, 2024

Use TOTP token with display #609

Closed

jeffpaul modified the milestones: 0.9.0, 0.10.0 May 8, 2024

joshbetz reviewed Jul 6, 2024

View reviewed changes

jeffpaul modified the milestones: 0.10.0, 0.11.0 Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support WebAuthn #427

Support WebAuthn #427

mcguffin commented Jan 21, 2022

sjinks Jan 24, 2022

sjinks Jan 24, 2022

sjinks Jan 24, 2022

mcguffin commented Jan 24, 2022

jeffpaul commented Mar 9, 2022

jeffpaul commented Sep 9, 2022

sjinks left a comment

jeffpaul commented Sep 20, 2022

iandunn commented Nov 14, 2022

paulschreiber commented Feb 20, 2023

riastradh commented Mar 4, 2023 •

edited

Loading

iandunn commented Mar 10, 2023

StevenDufresne Apr 21, 2023

StevenDufresne Apr 21, 2023

lkraav Apr 22, 2023

StevenDufresne Apr 21, 2023

StevenDufresne Apr 21, 2023

ravinderk left a comment

ravinderk May 3, 2023

ravinderk May 3, 2023

ravinderk May 3, 2023

ravinderk May 3, 2023

ravinderk May 3, 2023

ravinderk May 3, 2023

ravinderk May 3, 2023

xknown May 24, 2023

xknown May 24, 2023

xknown May 24, 2023

xknown May 24, 2023

jornfranke commented Sep 30, 2023

georgestephanis commented Oct 2, 2023

joshbetz Jul 6, 2024

joshbetz commented Jul 6, 2024


		const login = ( opts, callback ) => {

		const { action, payload, _wpnonce } = opts;

Support WebAuthn #427

Are you sure you want to change the base?

Support WebAuthn #427

Conversation

mcguffin commented Jan 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcguffin commented Jan 24, 2022

jeffpaul commented Mar 9, 2022

jeffpaul commented Sep 9, 2022

sjinks left a comment

Choose a reason for hiding this comment

jeffpaul commented Sep 20, 2022

iandunn commented Nov 14, 2022

paulschreiber commented Feb 20, 2023

riastradh commented Mar 4, 2023 • edited Loading

iandunn commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravinderk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jornfranke commented Sep 30, 2023

georgestephanis commented Oct 2, 2023

Choose a reason for hiding this comment

joshbetz commented Jul 6, 2024

riastradh commented Mar 4, 2023 •

edited

Loading