Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

USB HID: add product name and serial to descriptor. #113

Conversation

enrikb
Copy link
Contributor

@enrikb enrikb commented Mar 14, 2021

I tried to add product name and serial number support to the internal descriptor for USB HID devices. If there is something similar for NFC devices it might be possible to add it, as well.

Tested in following environments:

  • Linux (Ubuntu 20.10, python3): OK (product name contains manufacturer)
  • Windows (Windows 10, python3): OK
  • MacOSX (Catalina, python3): OK
  • OpenBSD (6.8, python3): OK (Note:there have been changes in struct usb_device_info, see OpenBSD bug mailing list. I don't know how this can be addressed to support older versions, too.)
  • FreeBSD (12.2, python3): OK (serial number support needs updated uhid-freebsd, see Provide device serial number. grembo/uhid-freebsd#1; fragmented CTAP messages did not work in my VM setup, but seems unrelated)

Copy link
Member

@dainnilsson dainnilsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks very promising!

In general the approach looks fine. One change I'd request is to use None when no value is present instead of an empty string.

For NFC, I suppose we could use the PCSC reader name instead of product name. I'm not sure how useful it is, but then again, maybe it's better than nothing?

I ran some quick tests of this on Ubuntu, seems to work fine there.
I also tried running this on Windows and it immediately broke things. It looks like it just needs to handle the case where there is no value present by returning None instead of raising a WinError.

I'll try to do some testing on the other OSes as well!

fido2/hid/macos.py Outdated Show resolved Hide resolved
fido2/hid/macos.py Outdated Show resolved Hide resolved
fido2/hid/windows.py Show resolved Hide resolved
examples/get_info.py Outdated Show resolved Hide resolved
@enrikb
Copy link
Contributor Author

enrikb commented Mar 15, 2021

I changed the code to return None whenever there is no serial number / product name or the empty string is returned by the respective native interface. Many interfaces do not seem to distinguish here. As an empty serial number or product name is quite useless anyway, None should be OK in either case as well as in most error cases.

Thank you very much for your feedback and the valuable hints for improvement!

@dainnilsson
Copy link
Member

Great, functionally looks good. One small improvement is to replace the pattern:

foobar = <expr>
foobar = foobar if foobar else None

With:

foobar = <expr> or None

I'm not done testing this on the different platforms yet, but so far I've run into some Python2 specific problems on Linux. I've also found a few other Python2 compatibility issues that are unrelated to this PR (like the use of enum.auto, I've pushed a fix for that on master). The plan is to drop support for Python 2 for the 1.0 release, but for now we do have a requirement to be Python 2 compatible.

array.tobytes doesn't exist. I think the least ugly workaround is to replace:

buf.tobytes().decode() with bytearray(buf).decode("utf-8").

I'll get back to you when I've done more testing with anything I find!

@enrikb enrikb force-pushed the feature/provide_device_serial_numbers_where_available branch from 66caee5 to 14ff435 Compare March 16, 2021 19:44
@enrikb
Copy link
Contributor Author

enrikb commented Mar 16, 2021

I rebased to include your Python 2 fixes and incorporated your latest feedback.
Unfortunately, it's getting harder to test Python 2 with current operating systems. But at least on MacOS (Catalina), get_info.py works fine using the system's default Python 2.

@dainnilsson
Copy link
Member

Everything looks good to me! I've run some very basic tests of the latest code on Ubuntu (py2 and 3), MacOS (x86, py2 and 3), FreeBSD (py 2 and 3) and OpenBSD (py3 only, couldn't get python 2 to run properly) and Windows 10 (py2 and 3). No issues found.

I'm not super happy about the FreeBSD code relying on an unmerged PR to uhid-freebsd (which looks like it hasn't been updated in some time), but I'm OK with making an exception here since it's so minor and shouldn't break anything.

Again, thanks for your work on this!

@dainnilsson dainnilsson merged commit 23a2a50 into Yubico:master Mar 17, 2021
@enrikb enrikb deleted the feature/provide_device_serial_numbers_where_available branch March 17, 2021 10:59
@enrikb
Copy link
Contributor Author

enrikb commented Mar 17, 2021

Thanks!

enrikb added a commit to enrikb/python-fido2 that referenced this pull request Apr 26, 2021
The changes of Yubico#113 left trailing zeroes in the serial number and
product name on Linux.

The ioctl() for retrieving the attributes returns the string length on
success. Use the returned value to retrieve strings of the correct
length.
enrikb added a commit to enrikb/python-fido2 that referenced this pull request Apr 26, 2021
The changes of Yubico#113 left trailing zeroes in the serial number and
product name on Linux.

The ioctl() for retrieving the attributes returns the string length on
success. Use the returned value to retrieve strings of the correct
length.
enrikb added a commit to enrikb/python-fido2 that referenced this pull request Apr 27, 2021
The changes of Yubico#113 left trailing zeroes in the serial number and
product name on Linux.

The ioctl() for retrieving the attributes returns the string length on
success. Use the returned value to retrieve strings of the correct
length.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants