refactor(yubikey): dynamically load libusb at runtime#348
Conversation
Replace the yubico_manager crate (which hard-links libusb via rusb → libusb1-sys) with a minimal reimplementation that loads libusb-1.0 at runtime using libloading. This eliminates the link-time dependency on libusb, so the binary starts and runs normally on systems without libusb installed. Users only need libusb when they actually use the YubiKey provider — at which point a clear install hint is shown if the library is missing. Closes #346 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR replaces the compile-time Key changes:
Issues addressed from previous review rounds (all fixed): CRC coverage of the command byte, Remaining minor items:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as yubikey.rs
participant USB as yubikey_usb.rs
participant Lib as libusb (dlopen)
participant HW as YubiKey hardware
App->>USB: challenge_response_hmac(challenge, slot)
USB->>Lib: libloading::Library::new(libusb-1.0)
Lib-->>USB: function pointers (LibUsb)
USB->>Lib: libusb_init()
USB->>Lib: libusb_get_device_list()
Lib-->>USB: device list
USB->>USB: filter by VENDOR_ID + OTP_PRODUCT_IDS
USB->>Lib: libusb_open(device)
note over USB,Lib: Linux only
USB->>Lib: libusb_kernel_driver_active(iface 0)
USB->>Lib: libusb_detach_kernel_driver(iface 0)
USB->>Lib: libusb_claim_interface(iface 0)
USB->>Lib: libusb_free_device_list()
USB->>HW: wait_for SLOT_WRITE_FLAG==0 via control_transfer GET_REPORT
USB->>USB: build_frame(payload, command) - CRC16 over 65 bytes
loop 10 chunks, skip all-zero intermediate
USB->>HW: control_transfer SET_REPORT 7-byte chunk + seq
end
USB->>HW: wait_for RESP_PENDING_FLAG, poll up to 15s
loop until RESP_PENDING clear or seq wraps to 0
USB->>HW: control_transfer GET_REPORT
end
USB->>HW: write_reset packet 0x8F
USB->>USB: crc16(response 0..22) == CRC_RESIDUAL_OK
USB-->>App: 20-byte HMAC response
note over USB,Lib: DeviceHandle drop
USB->>Lib: libusb_release_interface(iface 0)
USB->>Lib: libusb_attach_kernel_driver(iface 0) if detached
USB->>Lib: libusb_close(handle)
USB->>Lib: libusb_exit(ctx)
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the YubiKey integration to remove a problematic hard dependency on Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the YubiKey provider to dynamically load libusb at runtime, which is a great improvement for portability by removing the hard link-time dependency. The visible changes in src/providers/yubikey.rs are positive, simplifying the code and improving error formatting. However, I've noted a subtle behavioral change in how YubiKey slots are handled; the previous implementation was more lenient with invalid slot numbers, whereas the new code passes them on directly. I've left comments suggesting that this should be handled gracefully to avoid confusing errors for the user. My primary concern is that the new yubikey_usb.rs file, which contains a new unsafe FFI implementation, was not available for review. This is the most critical and high-risk part of the change, and its correctness, security, and error handling (especially for missing libusb) are paramount. Assuming that part is thoroughly tested, this PR is a solid step forward.
| let hmac_result = yubikey_usb::challenge_response_hmac(&self.challenge, &device, self.slot) | ||
| .map_err(|e| FnoxError::Provider(format!("YubiKey HMAC-SHA1 challenge failed: {e}")))?; |
There was a problem hiding this comment.
The previous implementation coerced any slot value other than 1 to slot 2. By passing self.slot directly, this behavior is changed. If a user configures an invalid slot (e.g., 3), it will now be passed to the hardware, which might produce a cryptic error. Please ensure that yubikey_usb::challenge_response_hmac handles invalid slot numbers gracefully and provides a clear error message, or consider adding validation for self.slot at this call site to ensure it's either 1 or 2.
| yubikey_usb::challenge_response_hmac(&challenge, &device, slot_num).map_err(|e| { | ||
| FnoxError::Provider(format!( | ||
| "YubiKey HMAC-SHA1 challenge failed: {e}. Make sure HMAC-SHA1 is configured on slot {slot_num}.", | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
Similar to my other comment, removing the match statement for the slot number changes behavior. The old code would treat any input other than '1' as slot 2. Now, slot_num is passed directly. If a user enters an invalid number at the prompt, it could lead to a confusing error from the underlying USB communication. It would be more user-friendly to validate slot_num immediately after the user provides it to ensure it is either 1 or 2.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix CRC range to cover payload + command byte (65 bytes, not 64) - Add 15s timeout to wait_for() to prevent infinite hangs - Load and call libusb_kernel_driver_active/detach/claim_interface for Linux - Filter find_yubikey() by OTP-capable product IDs, not just vendor ID - Return error on short HID reads instead of silently discarding - Guard read_response against buffer overflow when r0 >= 36 - Validate slot is 1 or 2 in challenge_response_hmac - Document zero-chunk skip optimization in write_frame Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
After detaching the Linux kernel HID driver to perform USB control transfers, the DeviceHandle now properly releases the interface and re-attaches the kernel driver when dropped. Without this, other apps (browsers for WebAuthn, ykman, gpg-agent, etc.) cannot access the YubiKey until it is physically replugged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e pass Merge find_yubikey() and open_device() into a single find_and_open() that enumerates USB devices once, finds the first OTP-capable YubiKey, and opens it — all within one libusb context. This removes the redundant second enumeration and the time-of-check-time-of-use gap where the device could be unplugged between discovery and open. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove 0x0112 (NEO CCID-only, no OTP interface) from the allowlist - Add per-PID comments sourced from yubikey-manager (ykman) - Add common Windows install paths for libusb-1.0.dll (LibUSB-Win32, libusb.info installer, vcpkg) to match the macOS fallback pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Summary
yubico_managercrate with a minimal reimplementation inyubikey_usb.rsthat loadslibusb-1.0at runtime vialibloading(dlopen)Fixes #346
Details
The
yubico_managercrate depends onrusb→libusb1-sys, which links libusb at build time. On macOS, this hardcodes the Homebrew libusb path into the binary, causing a dyld crash at startup for users without Homebrew libusb — even if they never use the YubiKey provider.The new
yubikey_usbmodule:libloading::Library::new(), trying platform-appropriate pathsyubikey.rsprovider and setup code work identicallyThe FIDO2 provider (
ctap-hid-fido2→hidapi) was already fine —hidapiuses IOKit on macOS natively, no libusb needed.Test plan
cargo build— no warningsotool -L target/debug/fnox | grep libusb— no matches (no link-time dep)cargo test— all 118 unit tests passmise run test:bats— all 605 integration tests pass🤖 Generated with Claude Code
Note
High Risk
Introduces a new unsafe, FFI-based libusb implementation for YubiKey HMAC-SHA1 operations and removes the previous dependency stack; bugs here could break YubiKey encryption/decryption or cause platform-specific USB/runtime failures.
Overview
Reworks the YubiKey provider to avoid a link-time
libusbdependency. The PR removes theyubico_manager-based flow and replaces it with a newyubikey_usbmodule that dynamically loadslibusbvialibloadingand performs the HID feature-report challenge/response directly.This updates
yubikeyruntime and setup paths to callyubikey_usb::challenge_response_hmac, addslibloadingto dependencies, and updates the lockfile accordingly so the binary can start even whenlibusbis not installed (failing only when the YubiKey provider is used).Written by Cursor Bugbot for commit 714764d. This will update automatically on new commits. Configure here.