-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add support for the "Login with passkey" option #5929
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
base: main
Are you sure you want to change the base?
Conversation
* without encryption not implemented * deletion not implemented * does not handle errors well
|
First, thanks for contributing to Vaultwarden! Second, if adding this, it might also be the best to update webauthn to the latest version. The only issue here is, that this needs some form of migration for the currently configured 2FA objects, which would become invalid. Luckily the webauthn crate has the old struct still available for this reason in their latest version, but it might be complex. We already have a migration which is done from an even older format we used before. Though, I'm not sure if we would still need to support that as it has been a long long time ago already, and admins should have installed a newer version already for a long time now. (What do you think @dani-garcia ? ) The main reason for this is that there are several fixes and updates to better support passkeys via webauthn in the latest version. |
I don't think it is necessary, unless the migration is not a one-go approach, but only happens when an item with one or more 2FA objects is accessed (which would be a serious problem anyway). Admins can migrate to the latest version of vw with the "old" migration path and then to the most recent vw version. Such migration paths are vey common: wikimedia, database engines, operating systems, ....
Hasn't Stefan started a branch for upgrading webauthn at one point? |
The current migration check/step is done during startup. That would make it much easier to migrate, and maybe even act upon invalid or unsupported webauthn objects. Try to convert from old, if that also doesn't work, then disable. |
In that case, it is very easy. Rmove the old migration path from the code. Upon startup start transaction. Migrate to new webauthn objects. If it encounters an "old" object that cannot be migrated, roll back transaction and show the following message:
P.S.: In the release notes and maybe even in the |
|
Just to be sure I understood you both correctly, there would be 3 versions for 2FA Webauthn Registration Structs:
The current migration at the start is from If that is the case, I think this should be done in other PR first, should it? |
Kinda, the only thing I'm not sure about is what to do with the old So... Do we want to support migration from |
|
Another thing I just noticed is that the webauthn-rs 0.5 crate doesn't seem support resident keys without also forcing attestation. In the 0.3 version there was the option to implement get_require_resident_key() to ensures that a resident key is created on the security key of the user. This wasn't needed for 2FA but since the Bitwarden frontend decides to use Discoverable Credentials (That's why you don't need to supply a username when logging in with a Passkey), "Login with Passkey" can only be implemented with that feature to stay compatible with the Frontend. The maintainers of webauthn-rs seem to have a strong opinion on residential keys (see here and here). Since there was a way to do this without attestation in the past, maybe this could be copied and done outside of the webauthn crate. Alternatively I could try if the Frontend cares if registrations request includes attestation, if it doesn't, then the passkey support could still be implemented with the 0.5 release and the |
|
Ok, after looking through the webauthn crate, I found webauthn_rs_core, this should solve this problem as it exposes the internally used webauthn builders, so that could be used for the resident key case. My previous concern shouldn't matter therefore. |
|
When will it be available in testing? |
Hello,
This PR implements the endpoints needed for passkey login into the Bitwarden webclient.
I marked this as a draft for now, since I'd like to first get some feedback on this PR before I start pursuing this further or if this is even a wanted feature for Vaultwarden.
Working Features
Testing this Branch
To test this branch, the fronted needs to be build with this line appended, to allow for passkey registration.
Open TODOs
tokenfield (I think this is what bitwarden does with this C# Api)