Fix and extend FreeBSD support

[grembo]

This unbreaks FreeBSD support that was damaged when removing
the dependency on uhid-freebsd in bd36cbc.

It also adds support for FreeBSD 13's optional hidraw(4) driver
that will become the default eventually.

[dainnilsson]

Thanks! I've done some very quick tests in a FreeBSD 13 VM and things seem to work. Are there instructions somewhere on enabling the hidraw driver so that I know how to test with that as well?

[grembo]

Thanks! I've done some very quick tests in a FreeBSD 13 VM and things seem to work. Are there instructions somewhere on enabling the hidraw driver so that I know how to test with that as well?

@dainnilsson Thank you for looking into this.

You can test the hidraw(4) driver on 13.0 with:

echo 'hw.usb.usbhid.enable="1"' >>/boot/loader.conf
sysrc kld_list+="hidraw"
reboot

This should be sufficient for testing FIDO.

---

In case you want to run ykman commands for other purposes, you need to make sure the kbd driver doesn't attach.
In this case I did (with the yubikey plugged in):

DEV=$(usbconfig | grep Yubico | cut -d: -f1)
usbconfig $DEV power_off
usbconfig $DEV add_quirk UQ_KBD_IGNORE
usbconfig $DEV power_on

You can check if drivers attached as expected by using:

usbconfig show_ifdrv

Example output:

ugen0.3: <Yubico YubiKey OTP+FIDO+CCID> at usbus0, cfg=0 md=HOST spd=FULL (12Mbps) pwr=ON (30mA)
ugen0.3.0: usbhid1: <Yubico YubiKey OTP+FIDO+CCID, class 0/0, rev 2.00/5.24, addr 2>
ugen0.3.1: usbhid2: <Yubico YubiKey OTP+FIDO+CCID, class 0/0, rev 2.00/5.24, addr 2>

[dainnilsson]

Thanks! I followed the instructions and it looks like the hidraw support is mostly working as well. One issue I ran into is that the Product Name wasn't being displayed, whereas that did work with the uhid backend. I noticed that the call to length = fcntl.ioctl(f, HIDIOCGRAWNAME_128, buf, True) does seem to set the buffer contents correctly, but the return value length is set to 0. Maybe the ioctl call on FreeBSD doesn't return the length on success?

[grembo]

@dainnilsson Good catch, thanks again for testing.

I changed the code to work around the problem and also submitted a patch to FreeBSD's hidraw(4) driver .

[dainnilsson]

Nice, thanks! I think you have an off-by-one error in the length of both values though, since length - 1 is used due to the length returned on Linux including the null.

[grembo]

Nice, thanks! I think you have an off-by-one error in the length of both values though, since length - 1 is used due to the length returned on Linux including the null.

Thanks again. To be honest, the Linux behavior seemed a bit unnatural/unexpected to me at first. But since the driver aims to be compatible, I'll change it of course (rusty C skills don't help either :) ).

As for this pull request, I really should've noticed the off-by-one no matter what - sigh ;)

[dainnilsson]

Thanks for your work on this! I've now merged it into master.

[grembo]

@dainnilsson Thank you for the thorough review, much appreciated.

[grembo]

@dainnilsson As I just realized, that all the separate commits making up this PR were merged individually (they are all in the master branch's history): Should I squash my PRs next time?

[dainnilsson]

It's always a balancing act between having small enough changes in separate commits and adding too much noise (and you could certainly argue that a PR should be small/focused enough that a single commit should be enough). Sure, in this case it probably could have been squashed, but I'm not too concerned with a clean history. More important is being able to figure out when and why a change was made in the past, and then more information is typically better than less, even if it means sifting a bit more.

One other aspect (only tangentially related to this) is that we enable Github to enforce that all commits are signed. According to Github that requires each commit from an external contributor to be signed as well. I would prefer it if it just ensured that the merge commit was signed instead, but as it stands now I have to go in and add signatures to contributors commits, which unfortunately has the side effect of causing Github to not realize that they are the same commits anymore, which is why the PR shows as "Closed" and not "Merged", even though the commits are all merged. If we start always squashing PRs when merging, this might make a difference here. If so, that might be another argument for doing so, in which case it doesn't matter what the contributor does, as it will all be squashed when merging anyway.

[grembo]

Thanks for the detailed explanation. I found the commit history to be a bit confusing, hence my question.

In this specific case, squashing by me would have helped, as it was really just one feature and the changes I did during your review really didn't change anything substantial from the perspective of someone looking at it half a year later. Squashing (or force pushing) during the review would still have mangled it and its natural progression.

So, long story short, I'll just handle it exactly the same next time, ideally requiring fewer iterations ;)