-
Notifications
You must be signed in to change notification settings - Fork 8
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
Enable WebAuthn plugin in wp-admin #153
Conversation
0e003ea
to
2a6334d
Compare
2a6334d
to
a90b3a7
Compare
I ran into sjinks/wp-two-factor-provider-webauthn#456 when testing on my sandbox. We may be able to work around that by having folks register their keys on @dd32 do you have any thoughts on whether or not we should grant folks access until we build the custom UI? I'm guessing we'd do it on-the-fly rather than actually adding roles in the db for everyone. |
This works for me using a Yubikey, but not using Mac TouchID. I guess that's the difference between a PassKey and a Security Key? I might see if I can make TouchID work today based on your notes.
I would Errr on the side of not providing any access for any user to wp-admin, unless they have access that allows that already (Such as via a Make site). |
Looks like you're right, the Index: two-factor-provider-webauthn/inc/class-utils.php
===================================================================
--- two-factor-provider-webauthn/inc/class-utils.php (revision 2909844)
+++ two-factor-provider-webauthn/inc/class-utils.php (working copy)
@@ -31,7 +31,9 @@
public static function create_webauthn_server(): ServerInterface {
$builder = new ServerBuilder();
- $builder->setRelyingParty( new RelyingParty( get_bloginfo( 'name' ), self::get_u2f_app_id() ) );
+ $relay = new RelyingParty( get_bloginfo( 'name' ), self::get_u2f_app_id() );
+ $relay->setId( 'wordpress.org' );
+ $builder->setRelyingParty( $relay );
$builder->setCredentialStore( new WebAuthn_Credential_Store() );
$builder->enableExtensions( 'appid' );
return $builder->build(); The proper way to do that in the plugin is probably a new option for the PassKey domain, rather than assuming it's the top-level domain. Or I guess the other way that might not require a setting at all is this: $relay->setId( ltrim( COOKIE_DOMAIN, '.' ) ); // Convert 'example.org' OR '.example.org' to 'example.org'. Followed up in sjinks/wp-two-factor-provider-webauthn#457 with a similar approach. |
Huh, it didn't work for me w/ a Yubikey 🤔 . It does now with sjinks/wp-two-factor-provider-webauthn#457, though 🤷🏻♂️ I don't have a fingerprint scanner, but will see if I can setup a different soft key to test with. Great job on the PR 👍🏻
Ah, ok, and then we would just tell folks who don't have it to wait until the custom UI is built 👍🏻 |
That's my best understanding. There's some things that end-users could probably safely access, such as the user-dashboard ( |
That sounds good to me 👍🏻 |
9ced987
to
bd0fe5b
Compare
4562744
to
1736cab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work well for me, with the before mentioned change: https://github.com/WordPress/wporg-two-factor/pull/153/files/1736cab7c6c180291baf6deffb1fa54224d4df6a#r1195957886
I've created #165 for the WebAuthN + android issues.
I would assume this applies whereby a memcache server is shared across multiple unrelated sites. If that's not the case for WP.org it might be unnecessary, but it might be helpful to avoid having an additional layer where credentials are stored to reduce complexity etc. Worth double checking with secops to get their thoughts before deciding though. |
I'm seeing duplicate queries on load, probably due to the way we're running
Annoying that the plugin believes object caches are insecure.. edit: This is unrelated to our changes, this is caused by |
Seems to work for both yubikey (5nfc) and touch id (14" 2021 M1) on my sandbox - both got registered, and both were invalid when trying to add a second time. The only strange thing is the revalidation prompt - since it's time based, you don't get asked again after doing it once. So in my testing, I removed all webauthn keys, and then re-added, but wasn't prompted to reauth, which seems like we should? Also, when I was asked to re-auth, it didn't choose the webauthn key which was already registered (prior to switching to this branch from some earlier testing), but the TOTP key. Not sure if it's because that was already setup prior to adding a touch id, but probably worth investigating further. |
Adding a key is an act of authenticating in a manner, so revalidating immediately after adding it seems.. duplicative? But yeah, it's time-based, I'm not sure this is something that needs fixing?
That's because for re-auth it prefers the method used to login with. A change upstream to Two-Factor (or on wporg) to prefer the most-recently-configured method probably makes sense though. A good example of the preference thing would be if maybe |
If that's the case and method we follow then we should avoid triggering the re-auth once done - my example was after adding one key I needed to reauth (since it hadn't been done), but not after adding-removing-adding others. Maybe the timer needs resetting upon log-in? |
Ah! Gotcha, Two-Factor bumps the time when a provider is enabled for a user (via the two-factor API, when they previously didn't have 2FA enabled), but it has no knowledge of the plugin adding a new key. So I don't think this is at all related to this PR itself. There's a few options here I think:
|
Agreed - i'll make a new issue for it rather than it getting stuck in this PR 😄. Part of me wonders whether it just needs re-testing once live, as it could have been my profile and having partially set it up for an earlier test of this. |
There was one active key and I added another. |
Co-authored-by: Dion Hulse <[email protected]>
Fixes #114
Closes #134
Closes #146
Related #142 -- This uses the same technique for bootstrapping the test suite.
This is an alternate approach to #134 / #146.
To Test
wp plugin install two-factor-provider-webauthn
. Make sure it's at least version2.1.0
.Things to note:
wp_2fa_webauthn_credentials
andwp_2fa_webauthn_users
tables to the database. AFAICT the reason for that is to avoid shared memcache servers leaking credentials. If that's rare in practice, then it doesn't seem necessary to me. It could make it harder to migrate keys when we eventually use whatevertwo-factor
implements.webauthn_preregister
andwebauthn_register
AJAX endpoints. Or we may want to write an additional REST API endpoint to grab some raw data likeCredentialRow->counter
, rather than parsing it out of the AJAX response.user_verification_requirement
setting todiscouraged
, so that users won't have to enter a PIN before authenticating, but was still prompted. I think the flow would be much smoother without it, and the added security doesn't outweigh that in this scenario IMO. We're using WebAuthn as a 2nd factor, and Google, GitHub, and most others I've seen discourage the PIN, so it seems safe to me.User Verification
andDiscoverable Credential
todiscouraged
before I can authenticate without a PIN. I don't see a way to discourage discoverable credentials here without forking.wp-admin
UI, since we're building a custom one anyway, but there are two strings that show up during login. We may want to highlight this in the beta announcement to get more translations.There's a small issue PHP 8 issue, but it's shouldn't affect us yet.This has been resolved.