On 2019-11-14, Damien Miller <djm at mindrot.org> wrote:> Please give this a try - security key support is a substantial change and > it really needs testing ahead of the next release.Hi Damien, Thanks for working on security key support, this is a really nice feature to have in openssh. My non-FIDO2 security key (YubiKey NEO) doesn't work with the latest changes to openssh and libfido2, failing with `try_device: fido_dev_get_assert: FIDO_ERR_USER_PRESENCE_REQUIRED`. I'm not sure if this is a problem in libfido2 or sk-usbhid.c (I also reported this issue at https://github.com/Yubico/libfido2/issues/73). Is try_device incompatible with U2F keys? It seems to me to be trying to detect the presence of a key handle using an assert with up=0, but that causes the U2F codepath in libfido2 to return an error FIDO_ERR_USER_PRESENCE_REQUIRED. I believe that since try_device is only trying to find the device with the key, FIDO_ERR_USER_PRESENCE_REQUIRED should be ignored here, since that seems to indicate that the key lookup succeeded, but authentication was not attempted. I attached a diff that makes this change and it seems to fix my issue. -------------- next part -------------- diff --git a/sk-usbhid.c b/sk-usbhid.c index c0a6bd0d..00c07685 100644 --- a/sk-usbhid.c +++ b/sk-usbhid.c @@ -204,7 +204,7 @@ try_device(fido_dev_t *dev, const uint8_t *message, size_t message_len, out: fido_assert_free(&assert); - return r != FIDO_OK ? -1 : 0; + return r != FIDO_OK && r != FIDO_ERR_USER_PRESENCE_REQUIRED ? -1 : 0; } /* Iterate over configured devices looking for a specific key handle */
On Thu, 14 Nov 2019, Michael Forney wrote:> On 2019-11-14, Damien Miller <djm at mindrot.org> wrote: > > Please give this a try - security key support is a substantial change and > > it really needs testing ahead of the next release. > > Hi Damien, > > Thanks for working on security key support, this is a really nice > feature to have in openssh. > > My non-FIDO2 security key (YubiKey NEO) doesn't work with the latest > changes to openssh and libfido2, failing with `try_device: > fido_dev_get_assert: FIDO_ERR_USER_PRESENCE_REQUIRED`. I'm not sure if > this is a problem in libfido2 or sk-usbhid.c (I also reported this > issue at https://github.com/Yubico/libfido2/issues/73). > > Is try_device incompatible with U2F keys? It seems to me to be trying > to detect the presence of a key handle using an assert with up=0, but > that causes the U2F codepath in libfido2 to return an error > FIDO_ERR_USER_PRESENCE_REQUIRED. > > I believe that since try_device is only trying to find the device with > the key, FIDO_ERR_USER_PRESENCE_REQUIRED should be ignored here, since > that seems to indicate that the key lookup succeeded, but > authentication was not attempted. I attached a diff that makes this > change and it seems to fix my issue.Thanks for testing this! Does this patch help? If you're able to test multiple U2F-only keys in a host then that would be ideal - you'll be able to see whether ssh is trying each device if you run it in verbose mode (i.e. ssh -vvv ...) Basically, I want to make sure that FIDO_ERR_USER_PRESENCE_REQUIRED is returned only when a token actually claims a key handle, and not all the time... diff --git a/sk-usbhid.c b/sk-usbhid.c index 63c7cb2..8758e2d 100644 --- a/sk-usbhid.c +++ b/sk-usbhid.c @@ -197,6 +197,10 @@ try_device(fido_dev_t *dev, const uint8_t *message, size_t message_len, } r = fido_dev_get_assert(dev, assert, NULL); skdebug(__func__, "fido_dev_get_assert: %s", fido_strerr(r)); + if (r == FIDO_ERR_USER_PRESENCE_REQUIRED) { + /* U2F tokens may return this */ + r = FIDO_OK; + } out: fido_assert_free(&assert);
On 2019-11-14, Damien Miller <djm at mindrot.org> wrote:> Thanks for testing this! > > Does this patch help? If you're able to test multiple U2F-only keys in > a host then that would be ideal - you'll be able to see whether ssh is > trying each device if you run it in verbose mode (i.e. ssh -vvv ...)Yep, this patch works too: debug1: skdebug: found 1 device(s) debug1: skdebug: trying device 0: /dev/hidraw0 debug1: skdebug: fido_dev_get_assert: FIDO_ERR_USER_PRESENCE_REQUIRED debug1: skdebug: found key debug1: Authentication succeeded (publickey). Authenticated to localhost ([::1]:22). and without the key plugged in: debug1: skdebug: found 0 device(s) debug1: skdebug: couldn't find device for key handle debug1: sshsk_sign: sk_sign failed with code -1 debug1: identity_sign: sshkey_sign: unexpected internal error sign_and_send_pubkey: signing failed: unexpected internal error Unfortunately I only have the one key to test with.> Basically, I want to make sure that FIDO_ERR_USER_PRESENCE_REQUIRED is > returned only when a token actually claims a key handle, and not all the > time...Yeah, this crossed my mind after I sent the diff. Your patch looks good :)