search

google / OpenSK

OpenSK is an open-source implementation for security keys written in Rust that supports both FIDO U2F and FIDO2 standards.
Apache License 2.0
3k stars 289 forks source link

Failed to load "resident" ssh key generated by OpenSK device #526

Closed Jun-Amane closed 2 years ago

Jun-Amane commented 2 years ago

Expected Behavior

　　After successfully generating the residential ssh keys by ssh-keygen -t ecdsa-sk -O resident, the private key should can be load to another computer via the OpenSK devices by ssh-add -K, but it failed to load.

Actual Behavior

　　Generation step on residental key seems to be successful, but when I tried to load the key to anthoer PC, by pluging in the OpenSK device and ssh-add -K, it shows Enter PIN for authenticator: Provider "internal" returned failure -1 Unable to load resident keys: invalid format 　　(the PIN was set correctly.) 　　After enable verbose mode by ssh-add -K -v, it shows Enter PIN for authenticator: debug1: start_helper: starting /usr/lib/ssh/ssh-sk-helper debug1: sshsk_load_resident: provider "internal", have-pin debug1: sk_probe: 1 device(s) detected debug1: sk_probe: selecting sk by touch debug1: ssh_sk_load_resident_keys: trying /dev/hidraw5 debug1: check_sk_options: option uv is unknown debug1: read_rks: get metadata for /dev/hidraw5 failed: FIDO_ERR_MISSING_PARAMETER debug1: ssh_sk_load_resident_keys: read_rks failed for /dev/hidraw5 Provider "internal" returned failure -1 debug1: ssh-sk-helper: sshsk_load_resident failed: invalid format debug1: main: reply len 8 debug1: client_converse: helper returned error -4 Unable to load resident keys: invalid format 　　which makes me confused.

Steps to Reproduce the Problem

  1. Generate a pair of residental ssh key by ssh-keygen -t ecdsa-sk -O resident.
  2. Plug the OpenSK device to another PC.
  3. Try to load the key generated to that PC ssh-add -K -v

Specifications

　　I would appreciate it if you could do me a favour to solve that problem. Looking forward to your reply on your earliest convenience.

ia0 commented 2 years ago

Thanks for the issue!

My understanding is that ssh-add -K is implemented by https://github.com/openssh/openssh-portable which uses https://github.com/Yubico/libfido2 for security key features. And libfido2 uses a vendor command (0x41 which conflicts with our upgrade command) instead of the credential management command (0x0a) for credential management.

I think we should create an issue on libfido2 to change this constant from 0x41 to 0x0a (I didn't find any open PR or issue regarding that). It should probably be enough because it looks like the rest of the code matches the specification (the specification probably copied credential management from Yubico).

@kaczmarczyck what do you think?

Jun-Amane commented 2 years ago

Thanks for the issue!

My understanding is that ssh-add -K is implemented by https://github.com/openssh/openssh-portable which uses https://github.com/Yubico/libfido2 for security key features. And libfido2 uses a vendor command (0x41 which conflicts with our upgrade command) instead of the credential management command (0x0a) for credential management.

I think we should create an issue on libfido2 to change this constant from 0x41 to 0x0a (I didn't find any open PR or issue regarding that). It should probably be enough because it looks like the rest of the code matches the specification (the specification probably copied credential management from Yubico).

@kaczmarczyck what do you think?

　　Thanks for your reply. 　　Please forgive my ignorance, but is it enough to change only "CTAP_CBOR_CRED_MGMT_PRE" from 0x41 to 0x0a, or there are something more should be modify? I've found a document regarding CTAP, which told me that the 0x0a is surely the "authenticatorCredentialManagement" command. So why libfido2 uses 0x41 instead of 0x0a. Will it make other fido keys go wrong after doing such changing? 　　I can issue a pull request for libfido2 to do it if there is nothing would go wrong. Looking forward to your reply.

Jun.

ia0 commented 2 years ago

is it enough to change only "CTAP_CBOR_CRED_MGMT_PRE" from 0x41 to 0x0a

AFAICT yes, the rest of the code looks pretty close to the specification. But you would need to ask libfido2 to be sure.

why libfido2 uses 0x41 instead of 0x0a

AFAICT the authenticatorCredentialManagement is part of FIDO 2.1 and not FIDO 2.0, so for security keys that only implement FIDO 2.0 or earlier, using 0x0a would not work. However, if such security keys are Yubico, then they probably support 0x41. And I guess Yubico security keys implementing FIDO 2.1 still support the legacy 0x41 command, so for Yubico, 0x41 is always working.

I can issue a pull request for libfido2 to do it

I think opening an issue asking whether libfido2 could use 0x0a instead of 0x41 when the device supports FIDO 2.1 would probably be better. Because I suspect that libfido2 would still prefer to use 0x41 for "old" devices (FIDO 2.0 or earlier).

jmichelp commented 2 years ago

They could do both:

Jun-Amane commented 2 years ago

Sounds Good! I’ll open an issue for libfido2 later to ask about the command. And do some research on “GetInfo”.

jmichelp commented 2 years ago

Here is a link: GetInfo command specification. It's something the libfido2 should already send very early in the protocol to see what it supports.

Jun-Amane commented 2 years ago

I have opened a short issue for libfido2 to ask whether that command could be changed to 0x0a. Furthermore, I will do some research on CTAP just like @jmichelp said, since I am not familiar with it CTAP. Thanks again for supporting me.

ia0 commented 2 years ago

libfido2 had an interesting idea in https://github.com/Yubico/libfido2/issues/628: supporting 0x41 as a duplicate command for 0x0a. The reasoning being that there might be other tools that assume credential management to be on 0x41. The change should be pretty easy on OpenSK side since there is not yet any external tooling calling into our vendor upgrade command.

ia0 commented 2 years ago

@JunASAKA You can try #527 . I'm not sure what the correct output of ssh-add -K -v should be, but it doesn't fail and ends with Resident identity added: ECDSA-SK SHA256: [...]. So this PR might fix your issue.

kaczmarczyck commented 2 years ago

@ia0 I think your assumption is correct, and we should support libfido2 and other legacy clients by blocking 0x41 the way to did in #527. Also thanks for the bug report @JunASAKA . If you create an issue in libfido, feel free to CC me there.

Jun-Amane commented 2 years ago

Thanks for the reply. I've tested the PR, and it works as excepted. Waiting for the PR to be merged.

kaczmarczyck commented 2 years ago

Merged! Your comment on the PR sounds like this issue can be closed.

Jun-Amane commented 2 years ago

Good! Thanks a lot!