WIP Add ctap hid api #539
WIP Add ctap hid api #539
Conversation
|
This is a draft PR for the CTAP HID api. I still have to implement the actual read/write using the usbd-ctaphid crate. Just wanted to get initial feedback on the approach. I'm copying the read/write signatures we have in the usb-serial API currently. Do we also want to have the asynchronous reads/writes like we have in the usb serial api? |
crates/board/src/usb/ctap.rs
Outdated
|
|
||
| impl<T: HasCtapHid> Api for WithCtapHid<T> { | ||
| fn read(output: &mut [u8]) -> Result<usize, Error> { | ||
| match T::with_ctaphid(|ctap_hid| ctap_hid.pipe.read_and_handle_packet()) { |
There was a problem hiding this comment.
I have a hard time understanding how to use the usbd_ctaphid crate or if it is at all usable yet. Curious to see how this will look like once the PR is ready.
There was a problem hiding this comment.
I wasn't able to figure out how to use this crate. Changed to use ctap-hid-fido2 instead
There was a problem hiding this comment.
The PR is out-of-date, there was some merged changes from main. You'll need to merge into dev/opensk. Also, it would probably be good to start using the real CI. You can try to git checkout main -- .github/workflows/ci.yml scripts/ci.sh and fix any issues when running ./scripts/ci.sh.
| /// Reads from USB serial into a buffer. | ||
| /// | ||
| /// Returns the number of bytes read. This function does not block and may return zero. |
There was a problem hiding this comment.
| /// Reads from USB serial into a buffer. | |
| /// | |
| /// Returns the number of bytes read. This function does not block and may return zero. | |
| /// Reads a CTAP packet (64 bytes) from USB. | |
| /// | |
| /// Returns whether a packet was read. This function does not block. |
| /// Length of the buffer in bytes. | ||
| len: usize, |
There was a problem hiding this comment.
we don't need this now that we fix the packet size
| /// | ||
| /// Returns the number of bytes read. This function does not block and may return zero. | ||
| fn read "usr" { | ||
| /// Address of the buffer. |
There was a problem hiding this comment.
| /// Address of the buffer. | |
| /// Address of the buffer (must be at least 64 bytes). |
| } -> usize | ||
| }, | ||
| item! { | ||
| /// Writes to USB serial from a buffer. |
| } -> usize | ||
| }, | ||
| item! { | ||
| /// USB serial events. |
There was a problem hiding this comment.
| /// USB serial events. | |
| /// USB CTAP events. |
| pub enum Event { | ||
| /// There might be data to read. | ||
| /// | ||
| /// This is only guaranteed to be triggered after a short read. |
There was a problem hiding this comment.
| /// This is only guaranteed to be triggered after a short read. | |
| /// This is only guaranteed to be triggered after an empty read. |
(same below)
| } | ||
|
|
||
| pub trait Api: Send { | ||
| /// Reads from the USB serial into a buffer. |
There was a problem hiding this comment.
| /// Reads from the USB serial into a buffer. | |
| /// Reads a packet from USB CTAP HID. |
| /// Returns the number of bytes read. It could be zero if there's nothing to read. | ||
| fn read(output: &mut [u8; 64]) -> Result<usize, Error>; |
There was a problem hiding this comment.
| /// Returns the number of bytes read. It could be zero if there's nothing to read. | |
| fn read(output: &mut [u8; 64]) -> Result<usize, Error>; | |
| /// Returns whether a packet was read. | |
| fn read(output: &mut [u8; 64]) -> Result<bool, Error>; |
| /// Writes from a buffer to the USB serial. | ||
| /// | ||
| /// Returns the number of bytes written. It could be zero if the other side is not ready. | ||
| fn write(input: &[u8; 64]) -> Result<usize, Error>; |
There was a problem hiding this comment.
| /// Writes from a buffer to the USB serial. | |
| /// | |
| /// Returns the number of bytes written. It could be zero if the other side is not ready. | |
| fn write(input: &[u8; 64]) -> Result<usize, Error>; | |
| /// Writes a packet to USB CTAP HID. | |
| /// | |
| /// Returns whether the packet was written. | |
| fn write(input: &[u8; 64]) -> Result<bool, Error>; |
| @@ -21,6 +21,7 @@ aes-gcm = { version = "0.10.3", default-features = false, optional = true } | |||
| bytemuck = { version = "1.16.0", default-features = false, optional = true } | |||
| ccm = { version = "0.5.0", default-features = false, optional = true } | |||
| crypto-common = { version = "0.1.6", default-features = false, optional = true } | |||
| ctap-hid-fido2 = { version = "3.5.1", default-features = false, optional = true } | |||
There was a problem hiding this comment.
This crate does not do what you think it does. It implements the client part of CTAP2, not the authenticator part. I don't think there are any crates. You might have to implement something yourself. You can implement a UsbClass directly like https://github.com/google/wasefire/blob/main/crates/protocol-usb/src/device.rs or use https://crates.io/crates/usbd-human-interface-device which might work.
There was a problem hiding this comment.
I didn't realize that, thank you! I'll re-implement this.
There was a problem hiding this comment.
No worries, I agree this is quite confusing. Ideally, this should be caught by CI, but due to the conflicts it didn't run. (But maybe because it's WASM, the fact that that crate was not no-std didn't fail.)
|
ccing @kaczmarczyck who might also have ideas on how to implement the authenticator side of CTAP2 |
| } | ||
|
|
||
| fn read(&mut self) -> bool { | ||
| let mut data = [0; 32]; |
There was a problem hiding this comment.
Why use a buffer of size 32 here?
| for i in pos .. pos + len { | ||
| if self.buffer[i] == self.delimiter { | ||
| self.frame = Some(i); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be implemented with position?
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.position
| frame: Option<usize>, // index of first delimiter in buffer, if any | ||
| delimiter: u8, |
There was a problem hiding this comment.
What is this delimiter, and what kind of traffic uses it?
I assume you mean CTAP HID instead of CTAP2? And you need descriptors etc? OpenSK has this for Tock: https://github.com/google/OpenSK/blob/2.1/patches/tock/03-add-ctap-modules.patch |
Yes
Not necessarily. It depends on the crate. I guess the HID crate would provide some support for the report. And the USB crate definitely provided support for descriptors. (By the way, the code you reviewed was copy-pasted and will probably not make it to the end.) |
No description provided.