Conversation
|
Thanks for your work! I can see that it is a different approach than what folks developing yknotify chose, have you considered pros & cons of using those compared to your implementation? https://github.com/reo101/yknotify-rs?tab=readme-ov-file#detection-strategy Your FIDO approach is probably more resilient (though requires special permissions), and both PGP approaches are hacky and I'm not sure what's the lesser evil 😅 Are you able to check if PGP approach in this PR vs that code reacts better/consistently across PGP sign, PGP encrypt, as well as when ssh'ing to a remote server and using keypair from YubiKey? |
|
Thanks for the pointers to yknotify-rs and yknotify! I did look at both before implementing. Here's my thinking on the tradeoffs: FIDO/U2F detection yknotify uses macOS log stream to watch for IOHIDFamily kernel messages (startQueue/stopQueue). This is clever but fragile because the log messages are undocumented internals that could change across macOS versions. They also had to add workarounds to avoid false positives from non-YubiKey HID devices (filtering by AppleUserUSBHostHIDDevice and tracking client IDs). This PR instead opens the FIDO HID device directly via hidapi/IOKit and parses actual CTAPHID packets. The packet-parsing logic (runU2FPacketWatcher in u2f_common.go) is shared with the Linux path, so both platforms use the same proven detection. The tradeoff is requiring Input Monitoring permission on macOS 10.15+, which I've documented. PGP/GPG detection This is where both approaches are admittedly hacky, just in different ways:
I chose the socket-proxy approach because:
I've tested GPG sign (git commit), GPG decrypt, and SSH to remote servers using a YubiKey keypair, touch detection fires consistently across all three. SSH is handled by the existing WatchSSH detector which is already cross-platform. |
|
I'm sorry for the delay, I just wanted to let you know that I didn't forget about it and I will get back to you soon! |
max-baz
left a comment
There was a problem hiding this comment.
Thanks again for your work and sorry it took me long to go through the code changes, you caught me during a very busy period 😅 Let me know what you think about the comments below and how you prefer to proceed. This looks interesting overall, I only feel that this PR should have a much smaller diff before it's in a state that could be merged.
| if typ == HID_ITEM_TYPE_GLOBAL && tag == HID_GLOBAL_ITEM_TAG_USAGE_PAGE && val2b == FIDO_USAGE_PAGE { | ||
| isFido = true | ||
| } else if typ == HID_ITEM_TYPE_LOCAL && tag == HID_LOCAL_ITEM_TAG_USAGE && val1b == FIDO_USAGE_CTAPHID { | ||
| } else if typ == HID_ITEM_TYPE_LOCAL && tag == HID_LOCAL_ITEM_TAG_USAGE && val1b == FIDO_USAGE_U2F { |
There was a problem hiding this comment.
this looks suspicious on the first glance, can you give some context for this change?
| github.com/vtolstov/go-ioctl v0.0.0-20151206205506-6be9cced4810 | ||
| ) | ||
|
|
||
| require github.com/karalabe/hid v1.0.0 // indirect |
There was a problem hiding this comment.
how unfortunate, it looks like this repo is now archived, do you know if there are any available alternatives, or was it the only option?
| // macOS does not have XDG_RUNTIME_DIR; fall back to $TMPDIR (guaranteed on macOS). | ||
| if dir := os.Getenv("XDG_RUNTIME_DIR"); dir != "" { | ||
| return dir | ||
| } |
There was a problem hiding this comment.
I guess if the comment is true and macOS doesn't have XDG_RUNTIME_DIR, there's no need to try to check it here in this _darwin.go file? 😜
| func sendMacOSNotification(message, title string) { | ||
| imagePath := "/usr/local/share/yubikey-touch-detector/yubikey-touch-detector.png" | ||
| script := `display notification "` + message + `" with title "` + title + `" sound name "Ping" image from file "` + imagePath + `"` | ||
| if err := exec.Command("osascript", "-e", script).Run(); err != nil { |
There was a problem hiding this comment.
I'm not convinced this notifier should be part of the core app, as all it does is calls a shell script - it's trivial to build this kind of integration outside of the app, there are some examples on wiki. I was (and maybe still am 😂 ) skeptical about adding libnotify support in the first place, but at least it has plenty of logic about it, like auto-hiding notifications after the yubikey is pressed. I propose we focus on actual detection in this PR and if needed take the notifier part separately.
| known := map[string]bool{} | ||
|
|
||
| for { | ||
| devices := hid.Enumerate(0, 0) // enumerate all HID devices |
There was a problem hiding this comment.
just curious, this seems to be a cross-platform library, so we could even use this code on linux, and avoid splitting the code for linux vs darwin, could we not? I'm not worried about splitting like you did (especially in light of us maybe having to find another library, see another comment), but if the library we pick is cross-platform, it could be a way to save on some lines of code :)
|
Just a small update, I am taking a look at the review comments. I will try to resolve them soon since I am having a bit of a busy time! |
Remove the strategy pattern (DetectorStrategy struct + factory
functions) in favour of letting Go build tags dispatch between
platform-specific implementations directly, matching the
reviewer's suggestion.
- Delete detector/strategy{,_linux,_darwin}.go
- Rename Watch{GPG,U2F,HMAC}Linux/Darwin → Watch{GPG,U2F,HMAC};
main.go calls detector.Watch* directly
- Revert cosmetic variable renames (printVersion→version, sz→size)
and strip added comments to reduce diff noise
Replace the osascript notifier strategy shim with
build-tagged stub:
- Delete notifier/{macos,strategy_linux,strategy_d Jump to bottom
- Add notifier/libnotify_darwin.go: same SetupLibnotifyNotifier
name as the Linux implementation; sends a macOS
Center banner via osascript on *_ON events
- Revert main.go to call SetupLibnotifyNotifier di Jump to bottom
drop the --notify flag and YUBIKEY_TOUCH_DETECTOR_NOTIFY env var
Fix notifier/socket_dir_darwin.go: remove the pointless
XDG_RUNTIME_DIR check since macOS does not have it
|
Hi @max-baz, thanks for the detailed review, apologies for the delay. I've worked through most of the comments: Removed the strategy pattern. Deleted Reverted cosmetic changes. macOS notifier. Rather than dropping it, I kept it as a build-tagged
Two things still open:
|
|
Thanks! Yes I still prefer to drop the macOS notifier part and focus on detection for now. Could we try to see if github.com/sstallion/go-hid can equally be used for detection on macOS? I think let's not merge the code with Linux just yet, focus only on macOS, I just think it would be great if we don't launch the macOS support by using an already archived and deprecated github.com/karalabe/hid ... Regarding |
|
Well, I have a bad news. The U2F detection library actually blocks the FIDO device for detection and that results in the device not being available for regular use 🤦🏻♂️, so I think we will have to shift our logic to what yknotify does i.e. watching the macos log stream. Let me know if I should move ahead with that? |
|
Ooh really... So just to confirm, did it never work, or do you think some recent change broke it that we could revert? Have you seen the U2F events being detected by the code in this PR before? What about GPG and SSH events by the way? |
|
I hadn't tested the U2F flow previously because I generally didn't work with it. It was a genuine miss from my side. I tested it yesterday and it did not work 😞 The GPG and SSH flows are working as expected. |
|
I see, yeah I guess let's go with watching macos log stream in that case! |
|
Hi guys! Thank you both, looking forward for this PR. |
|
Testing is most definitely appreciated, as I don't have a macOS device to test this myself, I will release based on community feedback! @AnkitChahar's original idea for FIDO2/U2F detection was potentially a much less fragile solution, I'm sad it didn't work out, but if you want to try to tinker with it as well, and see if you might find a tweak that can help making it work, it's also appreciated! Otherwise as you've seen above we will move to another approach based on monitoring log streams. |
…ccess Replace the IOKit/hidapi approach with watching kernel IOHIDFamily log messages via log stream. The previous implementation held the FIDO HID device open exclusively, blocking concurrent browser WebAuthn sessions. The new approach tracks IOHIDLibUserClient handles associated with YubiKey devices and emits U2F_ON/U2F_OFF on startQueue/stopQueue events — without ever opening the device. - u2f_common.go: add //go:build linux (constants and runU2FPacketWatcher are now Linux-only) - go.mod: remove karalabe/hid dependency - libnotify_darwin.go: revert temporary osascript to a no-op stub
|
I have made the relevant changes. I have tested it on a macOS device and it seems to be working fine. I don't have a linux device so can't really test it on that. Would appreciate any help for linux. |
Summary
This PR ports
yubikey-touch-detectorto macOS by introducing a strategy pattern that selects platform-specific detector and notifier implementations at compile time using Go build tags. All existing Linux behaviour is unchanged.Motivation
macOS lacks the Linux-specific APIs this tool depends on (
inotify,hidraw+ioctl, D-Bus, libnotify). Rather than a one-off hack, this PR introduces a clean platform abstraction so future OS ports (e.g. Windows) follow the same pattern.Architecture
Strategy pattern
detector.DetectorStrategyis a struct of first-class functions populated by a platform-specific factory:Similarly,
notifier.SetupPlatformNotifier()resolves to the correct desktop notification backend per OS.New files
detector/strategy.goDetectorStrategystruct definitiondetector/strategy_linux.godetector/strategy_darwin.godetector/gpg_common.goCheckGPGOnRequestextracted — portable, no build tagdetector/gpg_darwin.godetector/u2f_common.gorunU2FPacketWatcher(io.ReadCloser)— portabledetector/u2f_darwin.gokaralabe/hid)detector/hmac_darwin.gonotifier/macos.goosascriptnotifier/strategy_linux.goSetupPlatformNotifier→ libnotifynotifier/strategy_darwin.goSetupPlatformNotifier→ osascriptnotifier/socket_dir_linux.gosocketRuntimeDir()→$XDG_RUNTIME_DIRnotifier/socket_dir_darwin.gosocketRuntimeDir()→$TMPDIRfallbackdbus_notifier_linux.gomain.gocan call D-Bus without build tagsdbus_notifier_darwin.go--dbusModified files
detector/gpg.go//go:build linux; renamedWatchGPG→WatchGPGLinuxdetector/u2f.go//go:build linux; renamedWatchU2F→WatchU2FLinux; device open extracteddetector/hmac.go//go:build linux; renamedWatchHMAC→WatchHMACLinuxdetector/util.go//go:build linuxnotifier/dbus.go//go:build linuxnotifier/libnotify.go//go:build linuxnotifier/unix_socket.go$XDG_RUNTIME_DIRwithsocketRuntimeDir()main.goNewDetectorStrategy(); unified--notifyflag (cross-platform alias for--libnotify)macOS implementation details
GPG detection
Proxies the
gpg-agentUnix socket (same technique the existingWatchSSHuses for the SSH agent socket). Every message through the proxy triggersCheckGPGOnRequest, which uses the existing GPGME AssuanLEARNcommand to confirm whether the YubiKey is actually waiting for a touch. Registers an exit handler to restore the original socket on graceful shutdown.U2F detection
Enumerates HID devices via
karalabe/hid(hidapi + IOKit). Identifies FIDO devices byUsagePage == 0xf1d0. The CTAPHID packet parsing logic (runU2FPacketWatcher) is shared with the Linux path viau2f_common.go.HMAC detection
Not yet implemented on macOS. The Linux implementation relies on
/sys/class/hidrawsysfs paths that don't exist on macOS. A stub is provided that logs a debug message. A full IOKit-based implementation is a follow-up TODO.Notifications
Uses
osascriptto send native macOS Notification Center banners. Requires the terminal app to have notification permission in System Settings.New flag
--notifyis introduced as a cross-platform alias for--libnotify. Both flags work on Linux; only--notifyhas effect on macOS. TheYUBIKEY_TOUCH_DETECTOR_NOTIFYenv var is the macOS equivalent ofYUBIKEY_TOUCH_DETECTOR_LIBNOTIFY.Dependencies
github.com/karalabe/hid v1.0.0for IOKit HID access on macOS.brew install gpgmerequired on macOS (same requirement as Linux forlibgpgme).Disclaimer: I have used AI to actually build out this support but I have vetted the code manually.