update the webauthn crate

[stefan0xC]

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-state

I'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.

[BlackDex]

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?
Else you could try (if using Linux at least) https://github.com/danstiner/rust-u2f

[stefan0xC]

I do think we need to create a migration, else we should release a 2.0 or something like that.

Well, I don't think I've already chipped into the discussion about breaking changes and semantic versioning but to me doing a 2.0 release (and communicating that you'd have to re-register any FIDO2 webauthn compatible security key) would probably be my preferred option (at the moment) if it means that we don't have to do this work at all (especially if the intention is to make our implementation more secure and RFC compliant).

Without the migration it will either lock out people, or make there account less secure if you would ignore the old version.

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?

Also, regarding testing, can't you use Bitwarden passkeys?

I'm not sure that passkeys are handled the same as security keys (at least there are 4 separate functions in the webauthn_rs crate for handling passkey registration and authentication). So I don't think so.

Else you could try (if using Linux at least) https://github.com/danstiner/rust-u2f

I probably could. Thanks for the suggestion.

[zacknewman]

I'll provide some feedback that will hopefully be of help.

First, webauthn-rs-core does provide a struct that can be used to migrate to the newer version. Not to beat a dead horse, but this is one reason SemVer adherence would be nice. Sounds like the project will at least adopt some form of semantic versioning—increment the first number when changes are "big enough"—but using a well-defined and established standard would be beneficial.

Two, don't get hung up on the "safe"/"unsafe" APIs. Frankly I don't think there is anything unsafe about using webauthn-rs-core or webauthn-rs-proto. A lot of the RP operations are done on the client and almost all of the RP operations done by webauthn-rs-core and webauthn-rs-proto (e.g., CBOR decoding) are done internally. The only reason to not want to use those two crates should be about maintainability.

Related to two, the ability to only use webauthn-rs requires making an assumption about how data is sent from the client. Despite what I said in the related issue, Bitwarden is not actually violating the spec by using AttestationObject and clientDataJson. It's clear they intended to conform to the same key names, but technically the client-portion of the RP can send whatever it wants however it wants (e.g., a compressed stream of bytes based on a proprietary binary protocol). It's fortunate that Bitwarden sends the data in a way that almost aligns with how webauthn-rs expects, but there is no guarantee that will always be the case.

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 webauthn-rs doesn't implement useful traits to get this done (e.g., Eq and Hash) which is one reason I created my own webauthn_rp library. If you were to use in-memory maps, then you would have to continue to use webauthn-rs-core so that the wrapper types you create do implement such traits. If you decide to go the persistent storage route, then I do recommend having three separate tables: webauthn, webauthn-attestation, and webauthn-assertion. With that you probably don't even have to worry about cleaning out incomplete ceremonies.

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 webauthn-rs relies on for functions like Webauthn::start_passkey_registration. Another example of the four possibilities is a server-side credential that does not require user verification. webauthn-rs uses that definition for the "securitykey" functions like Webauthn::start_securitykey_registration (i.e., they don't mean it is an actual security key). Note that you will want to enable the danger-user-presence-only-security-keys feature so that authenticators are discouraged from using user verification. The reason for that is that Vaultwarden will only rely on that for 2FA; at which point, the user already has to rely on a second factor (i.e., their master password). If you don't enable that, then webauthn-rs will actual rely on the default value as defined by WebAuthn: prefer. Note that many authenticators always require user verification during registration which means a user will potentially be confused why they had to verify themselves when registering but don't when authenticating.

Also beware that webauthn-rs requires OpenSSL 3.x.y. This means that Vaultwarden via the transitive property will also require OpenSSL 3.x.y. Fortunately almost all of the functionality of webauthn-rs still works using OpenSSL 1.a.b; however for reasons unbeknownst to me, webauthn-rs-core relies on a portion of the OpenSSL API that other "compatible" libraries (e.g., LibreSSL) don't have. What's especially unfortunate is that such reliance is only relevant for tests in webauthn-rs. You might be able to convince them to more sanely hide that behind a cfg attribute, but they do explicitly state that only OpenSSL 3.x.y is supported so they very well could decline. I had to patch those libraries when I was using my fork of Vaultwarden so that it was compatible with LibreSSL—I also patched in support for Ed25519, but that's not relevant to this point.

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 grant_type (and HTTPS endpoint), you pass the correct encrypted copy of the vault key when authentication succeeds. Furthermore, you can completely ignore the newly added section about the PRF extension since Bitwarden implements that entirely on the client. Technically RPs may reject ceremonies if unsolicited extensions are passed back, but webauthn-rs-core does not; thus upon receiving { "hmac-secret": true } (encoded in JSON for visualization purposes) in the CBOR-encoded attestationObject, webauthn-rs will not complain despite such an extension not being indirectly requested via the PRF extension (from webauthn-rs's perspective).

[stefan0xC]

I've reverted the removal of the U2F migration but I have not checked if I missed anything.
I also did not have time to test it either so that's why I've changed the status of the PR to WIP for now.

[stefan0xC]

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.

[BlackDex]

@stefan0xC, could you update this to have it rebased? That way i would also be able to test it using the Bitwarden Native Android app without any issues.

[stefan0xC]

@BlackDex I've squashed and rebased it. But it's still a work in progress and I don't think it works yet, so if you want to take over this PR feel free to do so.

[BlackDex]

Well, it is at least easier to check and validate now.

[BlackDex]

Did a quick check, but it breaks. The CredentialsV3 and CredentialsV5 are not compatible.
There already is a migration functions which runs to migrate very old keys to the CredentialsV3 format.

Maybe we can use that same function or an other function to do the same for CredentialsV5?
It currently at least reports that user_verified is missing during the json from_str().

I think you can test this by using The Bitwarden Browser Extension to create a Passkey which can be used as a WebAuthn login using a the current main branch, and then switch to this PR and test it out.

[stefan0xC]

I don't really have the time to work on this, so going to close it for now.

[BlackDex]

I might be able to pick this up just don't delete the branch hehe.