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

go-libusb is deprecated and archived #3

Closed
Jakuje opened this issue Feb 7, 2019 · 10 comments · Fixed by #9
Closed

go-libusb is deprecated and archived #3

Jakuje opened this issue Feb 7, 2019 · 10 comments · Fixed by #9

Comments

@Jakuje
Copy link

Jakuje commented Feb 7, 2019

The go-libusb library [1] used in yubihsm-connector is deprecated (and archived fork of [2]).

From packaging and security point of view, it is not a good idea to package archived fork of deprecated library.

Would it be possible to use the original version or "migrate" to the new gousb [3] that is alive and kicking?

I am not a go developer so I do not know what would it take to do so.

[1] https://github.com/thorduri/go-libusb
[2] https://github.com/kylelemons/gousb
[3] https://github.com/google/gousb

@QuLogic
Copy link
Contributor

QuLogic commented Feb 8, 2019

The conversion looks pretty straightforward to me. There are a few renames and there's no simple function like OpenEndpoint (you need to open a control, then an interface, then an endpoint). I also couldn't find an obvious way to do ReadTimeout/WriteTimeout.

Unfortunately, I don't any of this hardware, so I don't want to open a PR for something I can't test.

@QuLogic
Copy link
Contributor

QuLogic commented Feb 8, 2019

Please also note that the repository owner does not think anyone should use it (though now I'm thinking one of the contributors here probably opened that PR.)

@nevun
Copy link
Contributor

nevun commented Feb 12, 2019

We are aware of this and we are happy for any patches if you cannot wait for us to migrate.

@trebortech
Copy link

trebortech commented Jun 21, 2019

he conversion looks pretty straightforward to me. There are a few renames and there's no simple function like OpenEndpoint (you need to open a control, then an interface, then an endpoint). I also couldn't find an obvious way to do ReadTimeout/WriteTimeout.

Unfortunately, I don't any of this hardware, so I don't want to open a PR for something I can't test.

@QuLogic
If you can provide the code updates in a gist I'll test on my hardware.

@QuLogic
Copy link
Contributor

QuLogic commented Jun 21, 2019

@trebortech feel free to try this branch: https://github.com/QuLogic/yubihsm-connector/tree/gousb

@syntaxcase
Copy link
Member

@QuLogic I had some time to test your fork and I found two cases where it doesn't work correctly:

  1. the ZLP is not sent anymore, you can test that with yubihsm-shell by executing plain echo 3 61. I did some digging and it's gousb that doesn't allow sending an empty buffer.
  2. Errors out when resetting a device. If you run the tests in pytnon-yubihsm it will fail when the tests reset the device (don't run these tests on a production device!).

Regarding point 1, gousb doesn't even set LIBUSB_TRANSFER_ADD_ZERO_PACKET on xfer->flags (which only works on Linux btw) when calling libusb, so the ZLP can't be sent unless gousb is fixed. At least that's my impression from a quick look at gousb, are you aware of this limitation?

If these two points are resolved I would be happy to merge this.

@QuLogic
Copy link
Contributor

QuLogic commented Jul 24, 2019

OK, I guess that's what's done here, but I don't see LIBUSB_TRANSFER_ADD_ZERO_PACKET mentioned in thorduri/go-libusb either, so maybe it's something else.

@syntaxcase
Copy link
Member

Yes, that's where the ZLP is sent.

Let me elaborate on LIBUSB_TRANSFER_ADD_ZERO_PACKET. Libusb can automatically send the ZLP if that flag is set in the xfer data structure. Since gousb does not allow sending an empty buffer, I went looking if it at least used that flag, but it doesn't. That's why I think it's an actual limitation in gousb. The thorduri/go-libusb library allows sending an empty buffer, so it has no need to use that flag.
And by the way, setting LIBUSB_TRANSFER_ADD_ZERO_PACKET would not be enough in our case anyway, because it only works on Linux as far as I can see from the libusb code.

@QuLogic
Copy link
Contributor

QuLogic commented Jul 30, 2019

Maybe try removing this condition and see if it works?

@syntaxcase
Copy link
Member

Yes, that's the condition that doesn't let the ZLP through; without it the plain echo works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants