strongbox-password-safe / Strongbox

A KeePass/Password Safe Client for iOS and OS X
https://strongboxsafe.com
GNU Affero General Public License v3.0
1.32k stars 101 forks source link

HMACSHA1 support on OnlyKey not detected #550

Open elonen opened 2 years ago

elonen commented 2 years ago

The OnlyKey hardware authenticator (with on-device PIN keyboard!) is compatible with Yubikey and can do HMACSHA1 challenges on KeepassXC.

However, when trying it with Strongbox on MacOS, the Stronbox GUI says "No YubiKeys found", even if I try clicking the checkbox. Is the key device detection perhaps hardcoded in some way, that could be generalized?

onlykey commented 2 years ago

Hi one of our users requested we provide some detail here. OnlyKey is supported by other password managers that utilize the HMACSHA1 challenge-response feature such as KeePassXC, to add support for OnlyKey the only change required is to select OnlyKey's USB VID/PID. You do this by using function yk_open_key_vid_pid which is available in ykpers . It looks like you have this function available here - https://github.com/strongbox-password-safe/Strongbox/blob/b0fde082f88ccbd78080fc8edc245ed6ede7db30/macbox/include/yubikey/ykpers-1/ykcore.h#L84

Here is an implementation example:

https://github.com/keepassxreboot/keepassxc/blob/6d1fc31e961d46bd459ba1e89529da3a6943c575/src/keys/drivers/YubiKeyInterfaceUSB.cpp#L48

If you need more information please let us know. Thanks!

strongbox-mark commented 2 years ago

Hi there, this sounds great and seems straightforward technically.

I'd be happy to add support for OnlyKey... Could you send me a device for testing purposes please? Drop me a line on support@strongboxsafe.com to arrange. Thanks.

strongbox-mark commented 2 years ago

Hi @onlykey - thank you for the key.

I have received it and have made a first attempt at integrating. Unfortunately I am have some very intermittent results trying to communicate with the key itself. Some times it doesn't respond to a Challenge Response, I see a Timeout (error code 4) returned, other times the call to yk_get_serial() fails. Are there known issues with HMACSHA1 CR or getting the serial number?

Of course it could be something my side, but YubiKeys don't have this issue. I'm using the same code for both, ykcore mainly with the addition of the OnlyKey PID/VID.

Unfortunately I am pretty busy with other tasks and I don't think I can release support for this in this current condition, it would lead to a support headache on our side I imagine.

onlykey commented 2 years ago

@strongbox-mark Currently OnlyKey supports challenge response with KeepassXC and Authlite and is stable. I am not sure what would be different here as those also use ykcore. There are some features to consider here as by default OnlyKey requires button press for CR, this can be disabled by changing this setting in the app:

image

If the app is expecting an immediate response without button press that could be the reason for timeout. There is also no need to call yk_get_serial for OnlyKey as it does not use Yubikey serial numbers.

strongbox-mark commented 2 years ago

Hi, thanks for the extra detail. We'll continue with a few ideas and try gather more data. Definitely suboptimal at the moment.

I doubt the issue is the "Require Touch" feature, because the code accounts for this and uses it to request in the UI that the user touches the key. The key returns properly E_WOULDBLOCK sometimes, indicating this slot requires touch. Other times it times out.

As to the serial number. Strongbox uses this to check the correct key is in place, as an identifier. Does Onlykey have an equivalent of a YubiKey serial number or identifier we can use?

onlykey commented 2 years ago

@strongbox-mark

Does Onlykey have an equivalent of a YubiKey serial number or identifier we can use?

OnlyKey does not provide a unique identifier to prevent user tracking. Implementations assume that the first OnlyKey seen is the correct key. In the event that it is not the correct key this will result in mismatch error. Here the check for serial number can be done but the serial is always going to be a static value (1097617), this could also be skipped for OnlyKey (when the VID/PID matches an OnlyKey).

The key returns properly E_WOULDBLOCK sometimes, indicating this slot requires touch. Other times it times out.

For the E_WOULDBLOCK we recommend calling the CR function with mayblock set to 0 for OnlyKey:

yk_challenge_response(YK_KEY yk, uint8_t yk_cmd, int may_block, unsigned int challenge_len, const unsigned char challenge, unsigned int response_len, unsigned char *response);

https://github.com/Yubico/yubikey-personalization/blob/97448fbdec9fc0dd1bebb044917b79a10eb21389/ykcore/ykcore.c#L362

Making those two changes should allow you to see consistent stable CRs to be generated.

strongbox-mark commented 2 years ago

Thanks @onlykey

Yeah, the serial number is used to drive a good UX experience (i.e. Connected Key is the right one, or wrong one, or right is disconnected but another different key is plugged in etc). The static 1097617 might be fine though for OnlyKey users.

For the E_WOULDBLOCK we recommend calling the CR function with mayblock set to 0 for OnlyKey:

Interesting. We'll experiment with that in the coming weeks... Thanks.

getlarge commented 2 years ago

@strongbox-mark As a user of both Strongbox (on several devices) and Onlykeys, i would really appreciate to have this possibility, as that would avoid me to always have both my Yubikey and/or Onlykey plugged in, also that would reduce the need to switch between KeepassXC and Strongbox. So i am wondering if you had any chance to make some progress since the last comment here ?

strongbox-mark commented 2 years ago

Unfortunately I did try to fix the issues I was seeing but was unsuccessful and the severity of the issue was such that I couldn't possibly release it in that condition. I can try to take another look in the coming months but honestly I'm not hopeful, I'm sorry to say.

getlarge commented 2 years ago

That would be a very appreciated feature, but i guess it would only worth the effort if enough people asks for it. Thanks for giving an update though.