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
update the webauthn crate #4485
base: main
Are you sure you want to change the base?
Conversation
The whole migration part is why both me and @dani-garcia probably did not finished this yet too. I do think we need to create a migration, else we should release a 2.0 or something like that. Without the migration it will either lock out people, or make there account less secure if you would ignore the old version. Also, regarding testing, can't you use Bitwarden passkeys? |
Well, I don't think I've already chipped into the discussion about breaking changes and semantic versioning but to me doing a
You mean the U2F migration? Or is there a migration needed for compatibility between the way it was done and the "correct" way (according to RFC or at least the linked PR). Because if the latter I'd probably have to look a bit more into the details of the WebAuthn RFC some more before I can propose a solution myself. Maybe we should store the webauthn registrations in a separate table like it was done/suggested by @zacknewman?
I'm not sure that passkeys are handled the same as security keys (at least there are 4 separate functions in the
I probably could. Thanks for the suggestion. |
I'll provide some feedback that will hopefully be of help. First, Two, don't get hung up on the "safe"/"unsafe" APIs. Frankly I don't think there is anything unsafe about using Related to two, the ability to only use As far as attestation and assertion validation is concerned, I've personally pivoted how I perform that on my server implementation that only allows passkeys (i.e., not my fork of Vaultwarden). The state that needs to be "saved" is short-lived (e.g., Bitwarden server sets a 5 or 10 minute timeout). Additionally most of the state that is saved deals with registered credentials which Bitwarden caps to 5. Last well-intended clients will almost always complete the ceremony. This means using in-memory collections shouldn't use that much memory since you simply remove the entry once the ceremony is complete. Of course you would still want to deal with incomplete ceremonies (e.g., on a background thread iterate the map every <unit of time> and remove expired challenges), but that's also easy. Unfortunately You shouldn't need an actual security key to test your code. When talking about WebAuthn, one can reasonably partition authenticators on two criteria: user verification and server-side vs. client-side credential. This gives 2 x 2 = 4 possibilities. Despite how WebAuthn Level 3 defines "passkey", industry has settled on "passkey" to mean not only client-side credential but also one that enforces user verification. This usage is what Also beware that Last, related to the long-term goal of enabling passkey-based vault encryption, this is actually pretty trivial. You only need to add another column that stores another copy of the vault AES key which this time is encrypted with the output of the HMAC which itself is based on the secret associated with the passkey. Based on the OAuth |
I've reverted the removal of the U2F migration but I have not checked if I missed anything. |
0020d36
to
603e7d4
Compare
603e7d4
to
0045330
Compare
Just tested it with Chromium's built-in WebAuthn DevTools and it's not working. I'll try to find some time to fix this but it will probably take me a bit. |
I have been working on updating the
webauthn_rs
crate (as basis to implement passkey support).The feature
danger-allow-state-serialisation
is used to serialize the state into the database, which should be fine according to: https://docs.rs/webauthn-rs/0.4.8/webauthn_rs/index.html#allow-serialising-registration-and-authentication-stateI've also decided to remove the u2f migration because it would have required the use of the more low level, protocol interactions provided by the webauthn_core_rs crate, so I guess this could be considered a breaking change? (If this is deemed necessary I can revert the removal, I mainly did it because upgrading the crate was tedious enough.)
I could not actually test the changes because I don't have a security key myself. And therefore I also don't know if it addresses the issues raised in #4196 (but I think it should be easier with the use of the Safe API?). So someone else definitely needs to test it and/or take over this PR.