Add support for WebAuthn PRF extension #337
Conversation
Before this change, OpenSK (tag 2.1, commit 893faa5113f47457337ddb826b1a58870f00bc78) returns CTAP2_ERR_INVALID_PARAMETER in response to attempts to use the WebAuthn PRF extension. Original context: https://bugzilla.mozilla.org/show_bug.cgi?id=1863819
There was a problem hiding this comment.
This looks great, Emil! My only question is whether we need to do a better job of filtering the allow-list before calling AuthenticationExtensionsPRFInputs::calculate. The spec says
If evalByCredential is present and contains an entry whose key is the base64url encoding of the credential ID that will be returned, let ev be the value of that entry.
The use of the definite article in "the credential ID that will be returned" makes me think that we should filter down to an allow-list of length 1. Wdyt?
|
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. |
This is prescribed by the [CTAP spec][ctap]: >**Client extension processing** >1. [...] >2. If present in a get(): > 1. Verify that salt1 is a 32-byte ArrayBuffer. > 2. If salt2 is present, verify that it is a 32-byte ArrayBuffer. > [...] [ctap]: https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#sctn-hmac-secret-extension
…::process_hmac_secret_and_prf_extension
This is needed because the PRF extension should return an empty extension output
`prf: {}` when the extension is processed but no eligible authenticator is
found. Thus we need to differentiate these cases so that
`GetAssertion::finalize_result` can match on `PrfUnmatched` and generate the
empty output.
|
I force-pushed a bit because 8bdbc3d made the In addition to that I also added 978725b 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. |
|
Great! This looks like it's ready to go. Do you want me to merge the |
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) But maybe we'll take all separate from this PR? This one is probably big enough as it is already. 😆 |
Sounds good!
Yes, please submit a separate PR. Thanks again for your help with this! |
|
Alright, I've merged the 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? |
|
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. |
|
Thank you! I'm looking forward to passwordless login to the Bitwarden browser extension in Firefox! |
|
Hey, I know this is not the right place for this question... but when can we expect this feature to be available? |
Transplanted from https://bugzilla.mozilla.org/show_bug.cgi?id=1863819 and https://phabricator.services.mozilla.com/D211528, and refined from rough prototype state.