two-factor
two-factor copied to clipboard
WordPress
Support WebAuthn
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 thanks, MUCH appreciated!
@mcguffin any thoughts on the feedback above / ability to work on updates accordingly?
I tested this (as of 4b5513f) w/ Firefox Dev Edition 106 and Chrome 105, with a Yubikey 5C NFC and a Yubikey 5C Nano. It worked in all cases 👍🏻
@kasparsd @georgestephanis any further changes / testing you'd like here before merging in for the 0.8.0 release?
I apologize for raising this late in the process, but I've been looking at this closer and have some concerns:
- There aren't any tests (here or upstream) for the 2tvenom/cborencode and davidearl/webauthn libraries that were forked into this PR. They appear to be mostly inactively, too. The madwizard-org/webauthn-server library that's used in sjinks/wp-two-factor-provider-webauthn seems like a safer choice.
- There aren't any tests for first-party code.
- Forking the libraries could mean we miss out on Composer/GitHub notifications of any future security issues.
Those issues make me a bit nervous to put this into production. I can see a couple potential solutions, but first I'm curious to hear what everyone else thinks. I‘m also happy to help with any work that may be needed.
Better to raise late than not and run into issues later, so thanks for calling this out @iandunn. I share the concerns you raise in reading through, would be curious what alternatives you have in mind to weigh against the current PR implementation to try and keep the plugin on a path towards adding WebAuthn support.
I agree that getting 0.8 out the door soon is important 👍🏻
If we want to migrate U2F keys, then the bare minimum we'll need to do is switch to madwizard-org/webauthn-server. That alone would resolve most of my concerns. Adding first-party tests could be a stretch goal, or be done after the release (maybe as part of #468).
We could probably borrow a lot of code from sjinks/wp-two-factor-provider-webauthn to make the library switch go quicker, but I'd want to keep the leaner nature of this PR.
That's still a decent chunk of work, but it seems doable. I'm happy to take that on if folks like that approach (but I'm also happy to just help w/ testing/review if someone else wants to do it; I'm not trying to take over).
@kasparsd @georgestephanis any opinions (strong or weak) on Ian's proposal above for a shift in approach on this PR?
@iandunn I made some minor improvements and added a basic test class.
Concerning U2F-Key I hope I can relieve some of from your concerns.
I played around with key migration today and finaly got it to work:
https://github.com/mcguffin/two-factor/blob/migrate-u2f/includes/WebAuthn/class-webauthn-key-migrator.php
It's basically a rephrased version sjinks/wp-two-factor-provider-webauthn + madwizard-org/webauthn-server.
Don't know whether to move this to another PR or merge into this one...?
Migrating the keys is awesome 🎉 A separate PR seems better to me, to help keep things manageable 👍🏻
What are your thoughts on removing the 2tvenom/cborencode and davidearl/webauthn libraries, and using madwizard-org/webauthn-server for everything instead? And about including the library via Composer instead of forking it?
@iandunn I've chosen 2venom/cborencode and davidearl/webauthn mainly because they were easy to rephrase. I didn't want to poke into the two-factor codebase and infrastructure too deeply and keep it the way I found it: lean and self-contained.
I'm having similar concerns about webauthand cborencode like you and replacing them with a sustainable, well aintained and lean 3rd party solution would be the right thing to do. I just have my doubts if (1) now is the right time, (2) madwizard-org/webauthn-server is the right library and (3) the change in architecture and the follow-up-work is worth it.
The README of madwizard-org/webauthn-server declares the current plugin state to be:
Pretty stable but the API may still change slightly until the 1.0 release.
It is bringing 11 direct dependencies resulting in 24 packages being installed. The vendor/ dir alone weights 3.4M. https://github.com/lbuchs/WebAuthn has zero dependecies and a codebase of 132k. (Although not a recommendation — no unit tests either, < 1 commit/month and an open php file included in the distribution.)
madwizard-org looks like a one-person-show. There's a certain risk of the package being abandoned. (Not pointing at anyone but myself—there are tons of abandoned repos in my own github.)
WordPress has no central vendor/ dir; every plugin brings its own instead. To avoid conflicting package versions, we would need to prefix the namespace of all depending classes.
- sjinks/wp-two-factor-provider-webauthn is using typisttech/imposter-plugin (probably abandoned).
- WordPress SEO has added PHP Scoper to their build process.
From a system resources point of view (CPU, RAM, disk-operations, traffic, ...) it's a not a good idea to load the exact same code multiple times into memory. On high-traffic sites system resources are already a big problem. With climate desaster and energy crisis in mind, the whole resource issue may gain even more significance in the very near future.
[Edit] ~~With the upcoming Plugin Dependencies feature (kudos @afragen!), there could be a WordPress-specific way to handle 3rd party libs—saving all of us from clunky composer constructs.~~
@mcguffin while I appreciate the kudos, Plugin Dependencies only handles plugins as dependencies.
It cannot handle libraries or frameworks as dependencies unless these are packaged as plugins. Then it might be possible to add them similarly to adding a non-dot org plugin as a dependency.
@afragen
[...] unless these are packaged as plugins.
This is exactly what's fueling my optimism!
Maybe I'm wrong, but I would expect some kind of dependency-only plugins to appear wp.org, providing certain commonly used PHP libraries like guzzle, phpseclib, crypto, …
In an ideal world as a plugin author, I would simply add all the PHP-Packages I need to the "Requires Plugins" header, instead of composer requireing them and thus bloating my plugin with a vendor/ directory.
I don't believe the plugins team any longer allows for frameworks, etc.
You could, however, host them on GitHub and hook into the Plugin Dependencies just like I do with the example plugin for non dot org dependencies.
Happy to help get you started with this.
@afragen thanks for the clearification.
I guess you're refering to afragen/wp-dependency-installer, right?
Actually referring to https://github.com/afragen/add-plugin-dependency-api
It a composer requirement in the Plugin Dependencies feature plugin as an example.
@mcguffin you've raised some good points 🤔
I also wish that there was a more mature & popular library, but unfortunately it doesn't look like that exists yet. It seems like madwizard/webauthn-server is the most mature, though, despite the imperfections.
I also wish it was leaner, but I don't think it'll be a big issue in practice, since the code is autoloaded and only used in a few flows. That's also why I think it's ok to to just include it without any prefixing. Not ideal, but it shouldn't cause any tangible problems in practice.
For me, it ultimately comes down not having enough confidence that the other libraries are secure enough to run in production on my sites, which is a dealbreaker. I understand that's subjective, though, and I can definitely understand your perspective.
@iandunn What do you think of web-auth/webauthn-framework? Or at least parts of it like cose-lib and webauthn-lib? There is also a class for U2F key conversion.
The downside: We would likely have to use v3.3.x instead of 4.x because of the PHP requirement.
That one does look mature and well tested too 🤔 IIRC @sjinks tried it out for his plugin and found that the easy way didn't support U2F migration, and the hard way was pretty difficult to work with.
It looks like the 4.0 docs don't have that distinction, though, so maybe they made it easier to use? If that's the case, then I think it'd definitely be worth considering. IMO it'd be fine to require PHP 7.x for this particular provider (while still supporting 5.6 in the other providers), per the discussion in #461.
@sjinks, do you have any thoughts on web-auth/webauthn-framework?
I didn't play with it much, tbh. As far as I remember, it had a lot of dependencies (circa 20 MB, if I am not mistaken). IIRC, that was more than wordpress.org allowed (10 MB).
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.
Would funding help with getting this and #491 finished, tested, and merged soon, and 0.8.0 released?
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.
@kasparsd could you review it or assign someone else for review to unblock this?
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.