[WIP] Add basic NFC support #114
Conversation
|
Thank you for this, @Crote! I'm happy to start reviewing this for inclusion into Firefox when you think it ready. I'm expecting to start work in earnest on |
|
I'm doing a re-layout right now, and probably going to end up splitting out platform code into code for USB HID vs other transports. I can certainly take on helping to rebase this patch if you need the assistance, but apologies in advance! |
|
Actually, that's the very reason why it stalled: I made an attempt to support multiple transports at the same time, but it quickly turned into a complete rewrite of basically the whole architecture. As a novice Rust programmer, that turned out to be way above my pay grade. I'll look into it some more in the coming week! |
|
Probably won't be ready this week. Hopefully next week though! :) |
|
I've merged the auth_service branch, so if you'd like to look at what this might look like as its own subfolder, feel free! I still want to do the mass rename of the u2f code, which will necessarily move the HID stuff away from just |
|
There are still a few issues left to be worked out, but most of the big stuff is done. Could you give your thoughts on the overall structure so far? I've tried to separate the refactoring into logical commits for easy reviewing. |
|
I will review today! Thanks! |
|
(oops, hit enter too soon) I haven't done the nitty-gritty review here, but I'm pleased with the refactoring around the device info trait, and the rework of how the state machines start and the APDU movement. I just want to run through those with the proverbial fine comb. Also I am not sure what the best testing strategy for this will be. Do you know of any virtual NFC devices we could fake pcsc to use? Or are we similarly stuck there like we are with USB? |
|
Yeah, it's turning into a bit of a mess. The term "U2F" is starting to lose all meaning to me. There's still an issue in the way the NFCManager works though, as it seems that the old transaction is never actually cancelled. There seems to be some sort of race condition there or something. I'm not that familiar with testing this. We wouldn't even need a virtual NFC device, any virtual smart card reader would be fine. |
Crote
force-pushed
the
nfc
branch
2 times, most recently
from
84d9628
to
66c54bf
Compare
|
@jcjones I've been working on it again after some time off. Basically, the first five commits are done and should be merge-able. I've got NFC just about working, but I'm running into some bigger architectural issues. Mainly, the way the callbacks are structured. Right now, there's a status callback, and the StateCallback. The latter is quite finicky to work with due to the whole observer stuff. The observer only fires on the original, so any clone won't result in a transport cancel. Besides not being documented at all, it doesn't make sense to me behaviour-wise. Ideally, I'd say there are three kinds of callbacks:
So basically, the current division between status- and state-callback make it impossible to properly handle the type of events which actually happen, and they make the proper cancelling of transports extremely difficult. What are your thoughts on this? Should I rework it to use a single callback and overhaul all the error types? Is there something I'm missing? |
|
Thank you for continuing to work on this! I'm taking holiday this week, but I didn't want to leave this with no reply, so here's a shorter-ish reply (I've been thinking this through all weekend). I think you're right that we need multiple callbacks. I'm not totally clear on your suggestion of what the third one to add would be, though. We currently have a "completion" callback that indicates success, timeout, or critical error. We also have a status callback that indicates devices coming and going. I think I understand you're suggesting adding a third callback for transient errors that don't halt. If I'm correct in that, it seems like that could be just another condition type added to the status callback, at least from the Rust side, since the callback enum objects can be made to contain all the necesssary data. I agree, however, that the C API would require a separate callback. Assuming the above to all be correct, I'm fine with either plumbing a third channel as a non-critical error message channel, or adding to the existing status channel type. I was pondering this exact situation with the CTAP2 PIN protocol, actually, and imagining adding to the status channel, something like: pub enum StatusUpdate {
DeviceAvailable { dev_info: u2ftypes::U2FDeviceInfo },
DeviceUnavailable { dev_info: u2ftypes::U2FDeviceInfo },
Success { dev_info: u2ftypes::U2FDeviceInfo },
+ Condition { dev_info: u2ftypes::U2FDeviceInfo, details: TemporaryCondition },
}
+ pub trait TemporaryCondition {
+ // ... stuff
+ }I think that would end up being easier than overhauling the error types, but I haven't figured out what traits such an object would have. Then again, maybe it's better to just have more Gotta run to do holiday things... thank you again @Crote, I am really looking forward to merging this into Firefox! |
|
That's not quite what I meant. The main issue I ran into, is that there are multiple callbacks to begin with. Something like this makes the most sense to me: enum StatusEvent {
DeviceAvailable { ... }, // New device found
DeviceUnavailable { ... }, // Connection to old device lost
}
enum ResultEvent {
Success { ... }, // Everything went well!
Timeout, // Too much time elapsed, so we aborted
Unable, // Turns out we have no viable transports,
// so a successful result is impossible
}
enum ErrorEvent {
// All but the last case could be transient
// The user should probably be prompted to
// retry the device which caused the error
U2FError { U2FTokenError }, // We successfully talked to a device using U2F,
// but we used the wrong command in this state or something
ProtocolError { ... }, // We tried talking to a device, but we got garbage we can't decode
CommunicationError { ... }, // The OS gave us a device, but we got an error trying to use it.
// Maybe a write to a file descriptor failed or something?
TransportError { ... }, // We tried using a transport, but it gave an error.
// Maybe we got a permissions error trying to enumerate devices or something?
// In any case, this transport is unusable now.
}
enum AuthEvent {
Update { StatusEvent }, // Something changed, but we can safely ignore this
Warning { ErrorEvent }, // Something went wrong, we should probably tell the user
Finished { ResultEvent }, // We're done, this is the last callback which will happen
}And probably a kind of internal error somewhere, but that's not that important right now. The callback would simply accept an As to the interface on the C side, I don't know enough about it to form a meaningful opinion. But combining two C-side callbacks into a single Rust-side callback should be trivial, right? My main intention is to have a single, clear, and unambiguous way to tell the caller what is happening. I'm not sure if this approach is the right one, but something in this direction is probably needed. Enjoy your holiday! |
|
Oh, I like that very much. I'll leave more for later, but I would happily handle that patch. Now to your earlier point - you said the first 5 commits of 7 here are ready for review? I can at least start taking a look in evenings this week... |
|
Yeah, the first five are basically just refactoring to make it possible to sanely implement an NFC transport. This branch should pass all tests and linters after every single commit and I've tried to make them all self-containing, so the first five should be completely safe to merge already. Some parts (especially the fifth commit) do make clear that quite a lot of the existing code should probably be cleaned up afterwards, but I wanted to avoid doing basically a complete rewrite without knowing for sure that this is the right approach to use. The names used could probably really use a second pair of eyes, as I seem to be horrible at naming things. But there's no need to hurry. Why not just read a book in your evenings or something? It's your holiday, after all! |
| } | ||
|
|
||
| // Size of header + data + 2 zero bytes for maximum return size. | ||
| let mut bytes = vec![0u8; U2FAPDUHEADER_SIZE + data.len() + 2]; |
There was a problem hiding this comment.
Maybe construct this the same as you do size down in serialize_short?
|
|
||
| let mut size = 5; // class, ins, p1, p2, response size field | ||
| if !data.is_empty() { | ||
| size += 1 + data.len(); // data size field and data itself |
There was a problem hiding this comment.
This needs a bit more description, because it appears that the data-size field is bytes[4], accounted for in the initial value of 5. This is actually because of the overflow zeroes?
There was a problem hiding this comment.
This is already correct: when there is no command data payload (Ne=0), you omit the command length bytes (Lc).
| @@ -66,7 +66,84 @@ impl StateMachine { | |||
| Default::default() | |||
| } | |||
|
|
|||
| pub fn register( | |||
| pub fn register<T>( | |||
There was a problem hiding this comment.
You're right, I'm not a big fan of the _register / register<T> naming convention, but it's a fast rename I think.
My preference would be that the generics-using register/sign, being static methods that take the exact device to work upon, be named register_dev and sign_dev, so that usage is like
StateMachine::register_dev(dev, flags..)
Whereas the instance methods remain as just register/sign so they go back to usage of sm.register(flags..)
What do you think?
| return Err(io_err("payload length > 2^8")); | ||
| } | ||
|
|
||
| let mut size = 5; // class, ins, p1, p2, response size field |
There was a problem hiding this comment.
This should be 4: the response length bytes (Le) are omitted when the desired response length (Ne) is zero.
| bytes[1] = ins; | ||
| bytes[2] = p1; | ||
| // p2 is always 0, at least, for our requirements. | ||
| bytes[4] = data.len() as u8; |
There was a problem hiding this comment.
This should be skipped when data.len() == 0
| bytes[2] = p1; | ||
| // p2 is always 0, at least, for our requirements. | ||
| // lc[0] should always be 0. | ||
| bytes[5] = (data.len() >> 8) as u8; |
There was a problem hiding this comment.
This should be skipped when data.len() = 0.
Also, Lc should be entirely skipped when there is no data, not even a prefixed zero byte.
| // When sending zero data, the two data length bytes should be omitted. | ||
| // Luckily, all later bytes are zero, so we can just truncate. | ||
| if data.is_empty() { | ||
| bytes.truncate(bytes.len() - 2); |
There was a problem hiding this comment.
This is wrong - if Nc is zero (no command data length), then it should be omitted.
In extended mode:
-
If
Neis non-zero, then the last three (ifNc=0) or two (ifNc>0) bytes are theNe(which haven't been included here). -
If
Neis zero, then there should be no trailing bytes.
At the moment I think you're trying to go for maximum length (65536 bytes), which should have a message ending in 00 00 00.
(Edit: this previously incorrectly stated that Ne is always 3 bytes in extended mode, when it can sometimes be 2... also the U2F spec confusingly is "based" on ISO7816-4 but then is actually incompatible... except some revisions of the spec (not the newest) actually correct this 😢 )
|
For context on my drive-by commentary above, I've been having a look at smart card support in another FIDO2 library, which depends on this library. That other library has errors in it's ISO 7816 implementation as well, which I'm in the process of fixing up. Given the complexity of the standards, I thought I should have a peek at what's happening here as well. FYI: some tokens don't actually implement ISO 7816 correctly, which can cause your local tests to "unexpectedly pass" but fail with other tokens. The FIDO2 NFC specs aren't very easy to read either, and they gloss over some important details that you'll only find in the ISO 7816-3/-4 specs. |
|
Is anyone actively working on this or planning to work on it in the near, foreseeable future? The branch currently doesn't merge cleanly because the following changes conflict:
If I merge this branch instead with 660a701, the parent of the commit 47d6920 which introduced the conflicts, the merge is clean and the code compiles, but the example program fails here: This happens because, as implemented, a single NFCManager can only do one request in its lifetime -- there's no mechanism to queue multiple requests the way U2FManager does -- and the example program tries to reuse it both to make a credential and to authenticate with it. If I insert manager.cancel() between the manager.register(...) logic and the manager.sign(...) logic, I can verify that the code works to make and authenticate credentials with a real fido card, both via contact smartcard and via NFC. (I can also verify that all automatic tests pass.) But I think that's the wrong approach -- should do it like U2FManager in manager.rs to allow multiple queued operations, and perhaps even factor the runloop and queue management logic out of U2FManager and NFCManager. Happy to draft a patch to do this, and happy to do a bit more work to resolve the merge conflict, but if the work is already in progress, don't want to step on anyone's toes. |
|
I'm not working on this. @msirringhaus are you? Active development is happening on the ctap2-2021 branch. We're about a month away from being able to cut that over to main. We'll be removing a lot of duplicate / unnecessary code at the same time. So if you're willing to wait, it's going to be a lot easier to work on then. Feel free to start now if you'd prefer. I can take a closer look at the patch attached to this PR on Monday and let you know what I think needs to be changed. |
|
I took a closer look at this branch than just by blindly merging and testing, and I took a look at the ctap2-2021 branch too in order to estimate what work would be involved in bringing NFC support into it. This nfc branch retrofits the NFC/smartcard support into two different sets of StateMachine methods:
In other words, the NFCManager inverts the discovery vs operation logic from U2FManager: NFCManager discovers cards first, then uses StateMachine just for operation; U2FManager uses StateMachine to do discovery and operation, as in main and ctap2-2021. But the two kinds of manager do the same operation under the hood -- the same CTAP1 command/response protocol -- just on a different underlying protocol transport (PCSC library calls vs OS HID I/O). This is a little awkward, and the ctap2-2021 branch looks like it will make this approach a little more difficult because there are two different state machines, one for CTAP1 and one for CTAP2, which would both need to be adapted for the retrofit. It seems to me it would make more sense if either:
It looks like some renovation might still be under way in the organization of src/transport, judging by all the TODO comments, so maybe I should hold off on trying to take any of this up in the ctap2-2021 branch until that renovation has settled further. |
|
I am not (yet) actively working on this, but it is on my (ever-growing) TODO-list.
That is one of the things that will go away, once we merge over to main. We have currently a lot of code-duplication, because we needed a way to switch back and forth between old and new code in case a critical bug is encountered. Once we are satisfied that the new code works, we'll remove the old completely. In general, I'd say it will be easiest to I would need to experiment a bit with this myself, in order to figure out, how exactly this should be integrated into the existing code. |
|
Is anyone working/planning to work on this or contributions are welcome? |
Recently I bought a fancy ACR122U NFC reader because I got tired of plugging in my Yubikey all the time.
To my great surprise, Firefox didn't support Webauthn via NFC yet.
So I took it upon myself to try implementing this myself as #51 isn't really showing a lot of progress 😉 .
About the implementation:
The old library was tightly coupled to wrapping all APDU command in a HID layer. This has been reworked in a backend-agnostic variant, making it possible to add new communication methods.
The NFC implementation is done using
pcsc-rust, which should provide bindings on Windows, Linux, and MacOS.To use NFC, use
--features nfc. This will also disable USB support for now.This is my first time writing Rust, so there's probably a lot of improvement to be made style-wise.
Work remaining:
StateMachineonly allows for a singleTransactionbackend at the moment. This means that enabling NFC currently disables USB, soStateMachineshould be reworked.Open issues: