Repository navigation
Only ask to select among tokens holding an allowed credential - #387
ThibaudDauce wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Thanks! Unfortunately, the bug you've reported doesn't describe the trigger for this issue at all, so I can't evaluate how the current logic triggers this issue, nor how this patch fixes it. Lets follow up in that bug first.
Per the contributing guide (was in #385, now merged), this should be made as a patch to Firefox's source tree directly, as this significantly impacts the authenticator selection logic in the browser, so is very high risk.
Edit: this PR touches code that's been changed in Firefox that hasn't been synced back yet (https://phabricator.services.mozilla.com/D333509). I'll sync that change back to here (#388), but this PR should be made to Firefox first.
| ) -> bool { | ||
| // Holding the credential doesn't help a token that `ctap2::sign` rejects anyway. | ||
| if dev.get_protocol() == FidoProtocol::CTAP1 | ||
| && args.user_verification_req == UserVerificationRequirement::Required |
There was a problem hiding this comment.
Why does a CTAP2-only request get here?
There was a problem hiding this comment.
Oh, answering my own question, because https://phabricator.services.mozilla.com/D333509 isn't published here! I've created #388 for this.
| }; | ||
|
|
||
| // Before the selection, so that the silent discovery speaks the protocol of the | ||
| // actual request. |
There was a problem hiding this comment.
The CTAP2.1 Selection command may be used with a token that supports CTAP 2.1 and CTAP 1, even when use_ctap1_fallback = true.
With two security keys plugged in, every sign request starts by asking to touch one of them (both blink), then the PIN, then a second touch. When the RP sends an allow list, a silent assertion (up=false) already tells which key holds the credential, so the selection can be skipped.
Each token now runs
silently_discover_credentialsbefore the selection, and only the ones holding a credential stay candidates. If none says yes (registration, empty allow list, credProtect 3 credentials not discoverable without UV…) all tokens stay candidates, like before.A
NotATokenreceived while blinking doesn't run the selection logic anymore: blinking tokens only listen forCancel, so aContinuesent then was lost.I kept the App ID discovery inside
ctap2::signeven if it now runs twice on the selected token: the Firefox test token callsctap2::signdirectly (dom/webauthn/authrs_bridge/src/test_token.rs), I didn't want to change its behavior. The first commit only extracts it in a shared function.Tested with two YubiKeys and
examples/ctap2: the register still asks to select a key, the sign doesn't anymore.Ticket on Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=2087064