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

Remove U2F #439

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Remove U2F #439

wants to merge 5 commits into from

Conversation

georgestephanis
Copy link
Collaborator

As discussed with @jeffpaul with its deprecation we should remove the provider.

@jeffpaul jeffpaul requested a review from kasparsd April 20, 2022 19:51
@jeffpaul jeffpaul added this to the 0.8.0 milestone Apr 20, 2022
@jeffpaul
Copy link
Member

@kasparsd note that this relates to #423 (comment) wherein the next step after removing U2F is the determining the best path forward from here (happy to hear your input there).

@georgestephanis
Copy link
Collaborator Author

So this handles the code change, my only uncertainty at this point is whether we should do something to handle the data / ux.

That being, if someone had U2F enabled, but no other providers, would its sudden absence disable 2fa on their account entirely, and is that a case in which we should force enable an alternative, such as emailed codes so there is still some second factor?

package.json Outdated Show resolved Hide resolved
@kasparsd
Copy link
Collaborator

That being, if someone had U2F enabled, but no other providers, would its sudden absence disable 2fa on their account entirely, and is that a case in which we should force enable an alternative, such as emailed codes so there is still some second factor?

Yeah, this is a really important point!

Otherwise this looks good!

@georgestephanis
Copy link
Collaborator Author

@jeffpaul Do you have any thoughts on what the workflow should be if a user only has u2f enabled but no others?

@dziudek
Copy link

dziudek commented May 20, 2022

Hi,

U2F will be removed in v.0.8 but it will be still possible to use physical keys with webauthn? #427

Also - when we can expect v.0.8 release?

@jeffpaul
Copy link
Member

jeffpaul commented Sep 9, 2022

@georgestephanis if a user only has U2F enabled and the plugin is updated to whatever version this removal will be part of (e.g. 0.8.0), then we could possibly go with one of the following:

  • Enable and set Primary on their Email method (this relies on them still having access to the email attached to their profile but at least keeps 2FA active for them)
  • Add a non-dismissable (until resolved) admin notice for affected users directing them to the 2FA portion of their profile to enable a new method (not a huge fan of adding another admin notice to what could be a lengthy list already, but this would be a more graceful yet less 2FA-secure approach)
  • something else I've yet to consider?

What are your thoughts on this?

@jeffpaul jeffpaul modified the milestones: 0.7.2, 0.8.0 Sep 12, 2022
@iandunn
Copy link
Member

iandunn commented Oct 14, 2022

If we end up switching libraries in #427, then I think we could seamlessly migrate the existing U2F keys to the WebAuthn provider.

Update: @mcguffin started a PR for this in #491 🎉

Copy link

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@georgestephanis Can you update this pull request with master because PHPUnit tests are falling in this pull request?

image

@TimothyBJacobs
Copy link
Member

Discussing with @georgestephanis at WC US, we think it would be a good idea to "fail closed" here. That is, if someone has only the U2F 2FA method available, when we remove support for it, we don't want the user to have 2FA bypassed. Instead, we think we can enable the Two_Factor_Email method for the user if it is still available.

Otherwise, we should return a WP_Error from Two_Factor_Core::get_available_providers_for_user indicating that 2FA is not available, and they should contact their site administrator. This would be adding a new return type to the method, which may fatal a site. But that would be an edge case of an edge case, and still fail closed which is preferable to failing open.

@jeffpaul jeffpaul modified the milestones: 0.9.0, 0.10.0 May 8, 2024
@jeffpaul
Copy link
Member

@georgestephanis @TimothyBJacobs as discussed at WCUS Contributor Day, and besides the merge conflicts here, are there any additional updates or tweak to approach to getting this on towards merge?

@TimothyBJacobs
Copy link
Member

Yeah so I think this still needs adjusting for the changes to "fail close" instead of fail open. Since this will probably require a bit more work. If we do want to get a U2F fix in sooner rather than later, it probably makes sense to go with #571 now.

@jeffpaul jeffpaul modified the milestones: 0.10.0, 0.11.0 Sep 17, 2024
@dd32
Copy link
Member

dd32 commented Sep 18, 2024

Discussing with @georgestephanis at WC US, we think it would be a good idea to "fail closed" here. That is, if someone has only the U2F 2FA method available, when we remove support for it, we don't want the user to have 2FA bypassed. Instead, we think we can enable the Two_Factor_Email method for the user if it is still available.

I agree that fail-close to Email makes the most sense here, and that's only an edge-case for when Backup Codes are not enabled.

Due to how long U2F has been broken, I don't think we actually need to matter too much about this though, the affected users have been locked out of their sites for a while now unless they're running severely outdated browsers..

@jeffpaul
Copy link
Member

jeffpaul commented Dec 3, 2024

Due to how long U2F has been broken, I don't think we actually need to matter too much about this though, the affected users have been locked out of their sites for a while now unless they're running severely outdated browsers..

So @dd32 assuming we get #571 merged in, would resolving conflicts here be sufficient to getting this merged in or do you feel the fail-close to email is needed or skip and chalk up to "water under the bridge"?

@dd32
Copy link
Member

dd32 commented Dec 5, 2024

do you feel the fail-close to email is needed or skip and chalk up to "water under the bridge"?

For safety, fail-close to email would be the most secure option (If no other providers are enabled). I don't feel strongly about this though.

That could be implemented through a filter such as...

if ( $user_2fa_methods AND $user_2fa_methods only includes unavailable / unknown 2fa methods ) {
   $2fa_methods[] = email;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants