Yubico / yubihsm-connector

https://developers.yubico.com/yubihsm-connector/
Apache License 2.0
30 stars 14 forks source link

go-libusb is deprecated and archived #3

Closed Jakuje closed 4 years ago

Jakuje commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

trebortech commented 5 years ago

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 commented 5 years ago

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

syntaxcase commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Maybe try removing this condition and see if it works?

syntaxcase commented 5 years ago

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