Add NFC transport#105
Conversation
|
Thank you for your work @mikma, this is incredible. I'll make sure to review this over the next few days. As for testing, I have an ACR122U and a bunch of NFC keys to throw into the bunch - but I won't have access to them until Tue/Wed. |
There was a problem hiding this comment.
Great work so far @mikma!
Please consider adding an example script & updating the README. I'll be able to test with my ACR122U and a few more keys once we have an example script :)
| const APDU_FIDO: &[u8; 8] = b"\xa0\x00\x00\x06\x47\x2f\x00\x01"; | ||
| const SW1_MORE_DATA: u8 = 0x61; | ||
|
|
||
| #[derive(thiserror::Error)] |
| Apdu(#[from] apdu::Error), | ||
|
|
||
| /// Unexpected error occurred on the device. | ||
| Device(Box<dyn Display>), |
There was a problem hiding this comment.
I see this is only used with HandleError which implements Error. Can we use #[from] HandleError here? We could then derive Debug, Display,
| } | ||
| } | ||
|
|
||
| impl From<NfcError> for Error { |
| command: &[u8], | ||
| response: &mut [u8], | ||
| ) -> apdu_core::Result { | ||
| trace!("TX: {:?}", command); |
There was a problem hiding this comment.
Nit: Prefer using fields for structured logging, if possible.
trace!(?command, "TX")
* Reduces some boilerplate * Used in #105 - allow making dependency non-optional, rather than for `nfc` feature only * I plan to use this in new CBOR serialization and deserialization, as I would like to encapsulate underlying errors (more easily)
|
|
||
| const SELECT_P1: u8 = 0x04; | ||
| const SELECT_P2: u8 = 0x00; | ||
| const APDU_FIDO: &[u8; 8] = b"\xa0\x00\x00\x06\x47\x2f\x00\x01"; |
|
|
||
| #[instrument] | ||
| pub fn list_devices() -> Result<Vec<NfcDevice>, Error> { | ||
| let ctx = pcsc::Context::establish(pcsc::Scope::User).expect("PC/SC context"); |
There was a problem hiding this comment.
Have a look here about the context: https://pcsclite.apdu.fr/api/group__API.html#gaa1b8970169fd4883a6dc4a8f43f19b67
|
Hi @mikma, friendly ping on the above. If you're unable to address the comments at this point that's totally fine - another contributor has expressed interest in picking this up. It'd be a pity not to merge this! TIA :) |
u2f_hid webauthn_hid webauthn_preflight_hid:
|
Yes, let someone else pick up, that's fine. I pushed some changes in order for the branch to build. (I also added a branch with some experimental code which detects that the user places the NFC tag on the reader. Something like that is needed to test for user presence I think. https://github.com/mikma/libwebauthn/tree/nfc-wait-on-device) |
Very heavily based on #105, with the following additions: - Fleshed out the APDU-calls - Added `select_fido2()` to discover both U2F and FIDO2-only devices - Made `NfcDevice` derive `Clone` and other minor tweaks, so we can easily use it in `credentialsd` - Added the build with both NFC-features to the github CI (including the installation of libnfc et al.). I kept this separate from the original, un-featured build, so we can be sure the former works even without libnfc installed. Open questions: - Biggest question to me is, how to handle the multiple backends and multiple devices. Since we can't run the "blink and wait for user presence"-scheme with NFC as we do with USB, I'm leaning towards simply always returning the very first found NFC-device instead of a list of found devices and use that. With this, we could get rid of the duplication-problem. I'm not sure many setups would include more than one NFC-reader (with different devices on there). - The cancel-handle is currently unused, and I'm not sure if we can use it anywhere at all. - No testing yet. Not sure how easy that would be to add. - Given that `credentialsd` uses a poll-mechanism to find USB-devices (simply looping over `list_devices()`) as well, I did not investigate to get a blocking device discovery. - For testing, I hacked my local credentialsd-repo by simply replacing `hid` with `nfc` in the USB-handler. Thus, I needed a dummy `blink_and_wait_for_user_presence()`-implementation. This should probably be removed, esp. if we go for the "return first device"-scheme mentioned above. But doing that, I was able to successfully use NFC-devices with Firefox. --------- Co-authored-by: Mikael Magnusson <mikma@users.sourceforge.net>
|
Closing this in favour of #152, based on this PR. Thank you both @mikma and @msirringhaus! |
3 participants
This draft PR adds an NFC transport with two backends: PC/SC and libnfc. The backends are enabled using features: pcsc and libnfc.
The NFC transport is not complete yet, support for ctap1 is missing, and also support for NFCCTAP_GETRESPONSE, which seems to be optional.
When finished this PR should fix #5.
I have tested the transport with two NFC readers:
And I have used the USB-C Titan Security Key (K52T) with both readers.
What do you think?