search

mozilla / authenticator-rs

Rust library to interact with Security Keys, used by Firefox
https://crates.io/crates/authenticator
Mozilla Public License 2.0
275 stars 72 forks source link

Add support for WebAuthn PRF extension #337

Closed emlun closed 3 months ago

emlun commented 4 months ago

Transplanted from https://bugzilla.mozilla.org/show_bug.cgi?id=1863819 and https://phabricator.services.mozilla.com/D211528, and refined from rough prototype state.

emlun commented 3 months ago

Thanks @jschanck and @msirringhaus for the reviews! I believe I've addressed them all now, and I also noticed a few more things while self-reviewing the new code. While awaiting your re-reviews I'll proceed with testing re-integrating these changes into the Firefox branch.

emlun commented 3 months ago

I force-pushed a bit because 8bdbc3d16d444ebe250c398d91fa7d1617945844 made the crypto_dummy tests fail, and I also took the opportunity to split up 1aa54f08ee738fb7aa99fb11722bf2ab9605eecc into a series of smaller change units: fd8e062a73acdd44098831350b732121c49a1c59, 5cebc5ba24b23886ca3c7aee3bdf77dddf889abb and 9036264bd1b9dd52347197b67fecce2a5b65858a.

In addition to that I also added 978725b534161a9cba107dc0c1bbcadbfc971370 in order to correctly return an empty PRF extension output when the authenticator is not eligible, and added a little more debug output to failure cases.

jschanck commented 3 months ago

Great! This looks like it's ready to go. Do you want me to merge the serialize_map! patch first? I haven't checked whether there are conflicts.

emlun commented 3 months ago

Great! This looks like it's ready to go. Do you want me to merge the serialize_map! patch first? I haven't checked whether there are conflicts.

Please do! I'll update this PR to make use of it here too.

My local branch also has a few more refactorizations and some additions to the examples. While fiddling with the examples I also found that the (pre-existing) can_skip_user_verification logic seems to be a bit strange, but I might be missing something there.

But maybe we'll take all separate from this PR? This one is probably big enough as it is already. :laughing:

jschanck commented 3 months ago

Please do! I'll update this PR to make use of it here too.

Sounds good!

My local branch also has a few more refactorizations and some additions to the examples. While fiddling with the examples I also found that the (pre-existing) can_skip_user_verification logic seems to be a bit strange, but I might be missing something there.

But maybe we'll take all separate from this PR? This one is probably big enough as it is already. 😆

Yes, please submit a separate PR.

Thanks again for your help with this!

emlun commented 3 months ago

Alright, I've merged the serialize_map! changes and added a couple of tests that were now missing. Anything else that needs resolution?

I saw you rebase-merged #338 so at first I tried to rebase this on top of that, but the conflicts were frankly too much work to deal with, and it would have made the history not make sense anymore either, so I decided against it and just merged instead. Is that okay?

jschanck commented 3 months ago

We don't have merge commits enabled in this repo. I'm not too worried about the history---it's always available here in this PR if anyone needs it.

Be-ing commented 3 months ago

Thank you! I'm looking forward to passwordless login to the Bitwarden browser extension in Firefox!

ThomasSteinbinder commented 3 weeks ago

Hey, I know this is not the right place for this question... but when can we expect this feature to be available?