Pre-flight and AppID improvements #297
Conversation
| if !allow_list.is_empty() { | ||
| // Try to silently discover U2F credentials that require the FIDO App ID extension. If | ||
| // any are found, we should use the alternate RP ID instead of the provided RP ID. | ||
| let silent_creds = silently_discover_credentials( |
There was a problem hiding this comment.
Why do we do this here again? Didn't we already do this in StateMachine::sign()?
Couldn't we reuse the silent_creds-list we acquired in StateMachine, and simply overwrite the args.allow_list and args.relying_party_id there, before handing it in?
There was a problem hiding this comment.
StateMachine is only used by USB HID devices, and I intend to write virtual device tests for this.
| if dev.get_protocol() != FidoProtocol::CTAP2 { | ||
| info!("Device does not support CTAP2"); | ||
| let _ = selector.send(DeviceSelectorEvent::NotAToken(dev.id())); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Not really important, but why remove that from init_device() and duplicate it here multiple times?
Then all NotAToken()-messages would be in the same place.
There was a problem hiding this comment.
It just seemed logically separate from device initialization. I could go either way.
There was a problem hiding this comment.
Yes, from a 'functionality separation'-POV, this makes sense. I just slightly dislike the code duplication. But given that both solutions have their drawback, I'm fine with either, too. :)
src/statemachine.rs
Outdated
| // Attempt to silently discover credentials. If we find useful credentials, then we | ||
| // can potentially skip a device selection tap. We may find acceptable credentials | ||
| // on several devices. In that case, the first device to ask for automatic | ||
| // selection will be selected. |
There was a problem hiding this comment.
I don't fully understand the auto_select feature, yet.
Why do want potentially racy device-selection?
I figured, we filter out devices that provide no suitable credentials here, by telling the DeviceSelector, they are NotAToken(). Then the DeviceSelector can decide if only a single device was discovered, and thus the selection tap can be skipped, or if multiple are discovered and a selection tap is necessary.
There was a problem hiding this comment.
We can't send NotAToken, because we need to blink at least one of the connected tokens. So we would need a new DeviceSelector message like NotAUsefulToken.
As for why we want this, suppose a user has multiple tokens connected and consider these cases:
- Only one token stores a valid credential, but they forget which.
- Several tokens contain valid credentials, and the relying party will accept any of them.
- Several tokens contain valid credentials, and the relying party will only accept some of them.
- Several tokens contain valid credentials, and the user really wants to use Token 1.
I believe 1 and 2 are common, and auto_select improves user experience in both.
As for 3, the current patch is not perfect, but with a few changes auto_select will improve UX in this case too. We just need to check that the device is suitable for the request, e.g. by checking the user verification requirement, after init and before wait_for_device_selector.
I don't think there's anything we can do about case 4. IMO it's not worth sacrificing the UX improvements for cases 1, 2, and 3 for the whim of a user who could just unplug their other tokens.
There was a problem hiding this comment.
We can't send NotAToken, because we need to blink at least one of the connected tokens.
Right, I hadn't considered this.
My question was indeed mostly from the standpoint of 4.
I don't have a good overview right now, how this change works with resident keys / discoverable credentials. There, a user may have account A on token 1 and account B on token 2, and thus it's important which token is used.
I do like the idea of having NotAUsefulToken, where the DeviceSelector could basically keep track of them, instead of throwing them away immediately. If one or more useful tokens is found, it can cancel the not useful ones as usual. And if all devices report back as being not useful, it could just send a dummy blink or simply Continue to all of them.
Maybe in a follow-up PR?
There was a problem hiding this comment.
I'm going to disable auto_select in sign for now. I want to get the AppID changes into Firefox 118, and I think you're right that this feature isn't ready to go.
| @@ -1155,7 +1146,7 @@ pub mod test { | |||
|
|
|||
| // Sending first GetAssertion with first allow_list-entry, that will return an error | |||
| let mut msg = cid.to_vec(); | |||
| msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x94]); | |||
| msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x90]); | |||
There was a problem hiding this comment.
Not a huge fan of "this is the magic length byte (I assume?) that you have to know to modify if you change anything below", but I think that's out of scope for this change.
There was a problem hiding this comment.
Agreed, I filed #298.
|
I opened #299 to revisit |
|
@jschanck, Can this fix be backported to earlier versions of FF? It's impacting FF versions 115-118. |
The first patch splits
StateMachine::init_and_selectinto distinctinitandselectstages. It adds anauto_selecthint to theselectstage that can be used to skip device selection when the caller can determine that a particular device is suitable for the request.The second patch uses the
auto_selecthint when we find acceptable credentials on a device through silent discovery. This resolves Bug 1451111The third patch uses silent discovery to determine if there are acceptable (U2F) credentials on the device that require the use of the AppID extension. This resolves #269 and Bug 1846836.
I also added a
-aflag to the ctap2 example which causes it to use an alternate RP ID. (This is added in the second patch but is used to test the third patch's behavior).