betrusted-io / xous-core

The Xous microkernel
Apache License 2.0
532 stars 85 forks source link

vault: support for using FIDO2 mode for SSH ecdsa-sk and/or ed25519-sk resident keys #209

Closed jokeyrhyme closed 1 year ago

jokeyrhyme commented 2 years ago

Howdie, thanks so much for sharing this project, and especially the work on the vault app in 0.9.9 <3

I'm probably getting ahead of myself, but I was wondering about the FIDO(2) mode in the Vault app and its support for use as a security key for authenticating SSH sessions?

I did try to generate a new key, but got a few errors...

❯ ssh-keygen -t ecdsa-sk
Generating public/private ecdsa-sk key pair.
You may need to touch your authenticator to authorize key generation.
Key enrollment failed: requested feature not supported

❯ ssh-keygen -t ed25519-sk
Generating public/private ed25519-sk key pair.
You may need to touch your authenticator to authorize key generation.
Key enrollment failed: requested feature not supported

❯ ssh-keygen -t ecdsa-sk -O resident
Generating public/private ecdsa-sk key pair.
You may need to touch your authenticator to authorize key generation.
Enter PIN for authenticator:
Key enrollment failed: requested feature not supported

❯ ssh-keygen -t ed25519-sk -O resident
Generating public/private ed25519-sk key pair.
You may need to touch your authenticator to authorize key generation.
Enter PIN for authenticator:
Key enrollment failed: requested feature not supported

Not urgent at all, I'm just excited about the possibilities :)

bunnie commented 2 years ago

I have heard about people using authenticator tokens to handle SSH keys, but, I couldn't find exactly what part of the FIDO2 spec supports this. According to the CTAP2 tests we support everything currently ratified in the "official" spec. So I'm wondering if this isn't strictly a part of the FIDO2 spec, and if not, where is the specification that covers this mode, so I can see what it takes to implement it.

Do you happen to know any more about how this actually works under the hood?

jokeyrhyme commented 2 years ago

Thanks for the quick response!

I don't know the specifics yet, but I'll have more of a look and see if I can find out

jokeyrhyme commented 2 years ago

I think Yubikey firmware 5.2.3 is when they introduced support for ecdsa-sk and ed25519-sk: https://www.yubico.com/blog/whats-new-in-yubikey-firmware-5-2-3/

The main thing that jumps out at me here is this:

Curve25519, the default curve used in ssh (EdDSA and Diffie-Hellman)

And I found the OpenSSH specification for their U2F work, but I haven't dug into where it differs from pure FIDO2: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.u2f

I'm wondering if there's a specific signature or certificate algorithm that is required for SSH that is not advertised by vault 0.9.9

bunnie commented 2 years ago

Hmmm interesting. These are good leads.

Let me hook up a USB protocol analyzer and capture a key generation event, that may give some more bread crumbs that identify where things go off the rails. What's the difference from a usage perspective of -O resident for you? Basically if I were to drill down into one of these key gen methods, why would a user prefer to use it with or without the resident key feature? I understand it from the spec side but I'm not fully clear on the trade-offs of the use cases and which one is more popular.

Would like to focus effort on the most common use cases first.

jokeyrhyme commented 2 years ago

Resident keys are described here: https://www.openssh.com/txt/release-8.2

FIDO2 resident keys

FIDO/U2F OpenSSH keys consist of two parts: a "key handle" part stored in the private key file on disk, and a per-device private key that is unique to each FIDO/U2F token and that cannot be exported from the token hardware. These are combined by the hardware at authentication time to derive the real key that is used to sign authentication challenges.

For tokens that are required to move between computers, it can be cumbersome to have to move the private key file first. To avoid this requirement, tokens implementing the newer FIDO2 standard support "resident keys", where it is possible to effectively retrieve the key handle part of the key from the hardware.

OpenSSH supports this feature, allowing resident keys to be generated using the ssh-keygen(1) "-O resident" flag. This will produce a public/private key pair as usual, but it will be possible to retrieve the private key part from the token later. This may be done using "ssh-keygen -K", which will download all available resident keys from the tokens attached to the host and write public/private key files for them. It is also possible to download and add resident keys directly to ssh-agent(1) without writing files to the file-system using "ssh-add -K".

Resident keys are indexed on the token by the application string and user ID. By default, OpenSSH uses an application string of "ssh:" and an empty user ID. If multiple resident keys on a single token are desired then it may be necessary to override one or both of these defaults using the ssh-keygen(1) "-O application=" or "-O user=" options. Note: OpenSSH will only download and use resident keys whose application string begins with "ssh:"

Storing both parts of a key on a FIDO token increases the likelihood of an attacker being able to use a stolen token device. For this reason, tokens should enforce PIN authentication before allowing download of keys, and users should set a PIN on their tokens before creating any resident keys.

Mine is just one use case, where I prefer to use resident keys

Use of resident keys without PIN-protection leaves systems vulnerable if the security key is physically stolen (until the entry in ~/.ssh/authorized_keys is removed), but the Precursor is PIN-protected at the device level which is good enough for my use case and risk factors

This post discusses the security posture a little: https://www.deftly.net/posts/2020-06-04-openssh-fido2-resident-keys.html

jokeyrhyme commented 2 years ago

Huh, here's more verbose output from SSH:

❯ ssh-keygen -vvv -t ed25519-sk -O resident
Generating public/private ed25519-sk key pair.
You may need to touch your authenticator to authorize key generation.
Enter PIN for authenticator:
debug3: start_helper: started pid=51884
debug3: ssh_msg_send: type 5
debug3: ssh_msg_recv entering
debug1: start_helper: starting /usr/lib/ssh/ssh-sk-helper
debug1: sshsk_enroll: provider "internal", device "(null)", application "ssh:", userid "(null)", flags 0x21, challenge len 0
debug1: sshsk_enroll: using random challenge
debug1: sk_probe: 1 device(s) detected
debug1: sk_probe: selecting sk by touch
debug1: ssh_sk_enroll: using device /dev/hidraw7
debug1: ssh_sk_enroll: /dev/hidraw7 does not support credprot, refusing to create unprotected resident/verify-required key
debug1: sshsk_enroll: provider "internal" failure -2
debug1: ssh-sk-helper: Enrollment failed: requested feature not supported
debug1: main: reply len 8
debug3: ssh_msg_send: type 5
debug1: client_converse: helper returned error -59
debug3: reap_helper: pid=51884
Key enrollment failed: requested feature not supported

I wonder if debug1: ssh_sk_enroll: /dev/hidraw7 does not support credprot, refusing to create unprotected resident/verify-required key is related to this part from: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.u2f

During key generation, the hardware also returns attestation information that may be used to cryptographically prove that a given key is hardware-backed. Unfortunately, the protocol required for this proof is not privacy-preserving and may be used to identify U2F tokens with at least manufacturer and batch number granularity. For this reason, we choose not to include this information in the public key or save it by default.

jokeyrhyme commented 2 years ago

Well, I followed the error message through to this part of OpenSSH: https://github.com/openssh/openssh-portable/blob/c46f6fed419167c1671e4227459e108036c760f8/sk-usbhid.c#L915-L941

Then (I think) through to this part of (Yubico-provided) libfido2: https://github.com/Yubico/libfido2/blob/b0b849d8bf4802713a0a3170259d6608ba70480f/regress/cred.c#L2014-L2030

BryanJacobs commented 2 years ago

Hey, I've just been through the pain of implementing this, so I might be able to help a bit, and save you some sweat and tears.

According to the CTAP2 tests we support everything currently ratified in the "official" spec. So I'm wondering if this isn't strictly a part of the FIDO2 spec, and if not, where is the specification that covers this mode, so I can see what it takes to implement it.

You're right! OpenSSH is requiring features from FIDO_2_1_PRE, not even proper CTAP2.1. It's doing this even though your device doesn't say it supports those features. I can describe exactly what you need to make your implementation compatible...

Let me hook up a USB protocol analyzer and capture a key generation event,

Oh goodness don't do this! If you set the environment variable FIDO_DEBUG=1 when calling SSH, libfido2 will emit complete dumps of what it sends to the device and receives back!

Now, on to the implementation.

First, in order for SSH to use the authenticator at all, you need to implement the CTAP2.1 standard credProtect extension, with one caveat. Steps:

  1. Return key=credProtect with value true in the options object in response to the authenticatorGetInfo API call
  2. Accept a credProtect option to the makeCredential API. Read the spec description of the levels, and ensure the cred is protected at least at the requested level. Echo back the requested level in the makeCredential response.

The spec says it's okay for an authenticator to protect all credentials to level 3, and that it can respond with the effective level if greater than the requested one. This will not work with SSH. SSH requires that the returned level equal the requested one.

After making these two changes, SSH will work with non-resident and resident keys, but ssh-keygen -K will still fail - so while technically resident keys are working, you won't be able to discover them like, you know, the reason to use them... To make that work, you need to also implement the CTAP2.1 credManagement option, also with a caveat...

  1. Implement the CTAP2.1 credManagement option, specifically the getCredsMetadata, enumerateRPsBegin, enumerateRPsNext, enumerateCredsBegin, and enumerateCredsNext function. Doing this will require storing non-hashed user IDs (with some max length), and non-hashed RP IDs (at least 32 bytes), and credential PUBLIC (not private!) keys, if you're not already doing that and/or can't derive the public key from the private one. So it can be a bit tricky and increase the on-authenticator state...
  2. Wire the CTAP2.1 credManagement API up to the FIDO_2_1_PRE specific command byte - 0x41, in the CTAP vendor area. SSH will not call it with the CTAP2.1-assigned command ID!

Notice that I don't say you have to advertise support for credManagement like the spec says you should, or implement the setUser or deleteCred calls which are part of CTAP2.1. That's because SSH doesn't care :-). But you should still do those things. I'd advise not advertising support for the "preview" credentials management, and just hooking it up to the stand impl, but it's your call.

Edit: adding specification link for credManagement: https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#authenticatorCredentialManagement . There's a bit at the bottom about the old prerelease command IDs.

jokeyrhyme commented 2 years ago

OMG thanks @BryanJacobs !!!

bunnie commented 2 years ago

Thanks @BryanJacobs ! This is a big help, because google searches were sending me off into a world of some PGP smartcard emulation over USB which didn't seem right.

Based on your comments, I think we're somewhat close to getting this working with the existing implementation. We're based off of OpenSK stable, which has credProtect extensions, but I need to understand a bit more about the caveat. We probably don't do that which is why it's not working.

We don't have credManagement implemented, but I looked and the upstream development branch of OpenSK has it, so one possible option is to cherry pick those extensions out and merge them in, or I can try to just whole-hog absorb the latest.

My gut is to try and cherry-pick the extension if this is really all we need, because our code base has diverged sufficiently from OpenSK that a big merge is likely to not go well at all, and our general philosophy is to implement less code so we can maintain at a higher quality.

Also @jokeyrhyme the OpenSK reference implementation only has support for ECDSA right now. I think I see where the hooks for ED25519 might go. Natively we prefer ED25519 because we have hardware acceleration for that (and the Curve25519 implementation are way more mature than ECDSA in RustCrypto), but the middleware to plug the protocol into the crypto looks non-trivial; I'd have to bury my nose in some CBOR stuff that I have managed to largely avoid so far. How big of a problem is it if initially all that's initially supported is ECDSA for SSH keys?

bunnie commented 2 years ago

Making a to-do list for my own tracking:

BryanJacobs commented 2 years ago

I don't think the response is what's going wrong with your implementation, because you can see this log line:

debug1: ssh_sk_enroll: /dev/hidraw7 does not support credprot, refusing to create unprotected resident/verify-required key

SSH thinks your token does not support the credProtect extension. Check the output of fido2-token -I <yourdevice> and see what's there. That comes from the getAuthenticatorInfo response.

BryanJacobs commented 2 years ago

As a kind of protip, the extension names are case sensitive. You need to return the byte sequence 0x63, 0x72, 0x65, 0x64, 0x50, 0x72, 0x6F, 0x74, 0x65, 0x63, 0x74.

jokeyrhyme commented 2 years ago
❯ ssh-keygen -vvv -t ed25519-sk -O resident # just doing this again to find the device path
Generating public/private ed25519-sk key pair.
...
debug1: ssh_sk_enroll: using device /dev/hidraw14
debug1: ssh_sk_enroll: /dev/hidraw14 does not support credprot, refusing to create unprotected resident/verify-required key
...
Key enrollment failed: requested feature not supported

❯ fido2-token -I /dev/hidraw14
proto: 0x02
major: 0x01
minor: 0x00
build: 0x00
caps: 0x05 (wink, cbor, msg)
version strings: U2F_V2, FIDO_2_0, FIDO_2_1_PRE
extension strings: hmac-secret
transport strings: usb
algorithms: es256 (public-key)
aaguid: 48e0371c325257f7d2f0000fbd7a8919
options: rk, up, noclientPin
maxmsgsiz: 1024
maxcredcntlst: 0
maxcredlen: 112
maxlargeblob: 0
fwversion: 0x0
pin protocols: 1
pin retries: 8
uv retries: undefined
BryanJacobs commented 2 years ago

As you can see, extension strings does not contain credProtect. It needs to.

jokeyrhyme commented 2 years ago

I believe that's this line: https://github.com/betrusted-io/xous-core/blob/7ef4e4dc419234f1ca02b0bb9db8e4a3766eeb31/apps/vault/src/ctap/mod.rs#L905

bunnie commented 2 years ago

Thanks -- that pointed me in the right direction. I just pushed this patch:

https://github.com/betrusted-io/xous-core/commit/3ab403170640547b398442f65e28b569b786a128

On my test system, I'm able to run the following and it works with my Precursor:

ssh-keygen -t ecdsa-sk
Generating public/private ecdsa-sk key pair.
You may need to touch your authenticator to authorize key generation.
Enter file in which to save the key (/home/bunnie/.ssh/id_ecdsa_sk):
Enter passphrase (empty for no passphrase):
Enter same passphrase again:
Your identification has been saved in /home/bunnie/.ssh/id_ecdsa_sk
Your public key has been saved in /home/bunnie/.ssh/id_ecdsa_sk.pub
The key fingerprint is:
SHA256:XDrUcqi8VipxK1k27rBokezLZVO915JctJ5BTrJNTlc

The patch should be live in the "bleeding-edge" update branch if you want to try it.

bunnie commented 2 years ago

(Well, live in about 5 minutes, however long it takes for the build system to complete. Whenever this guy finishes running: https://ci.betrusted.io/view/Enabled/job/xous-kernel/854/)

bunnie commented 2 years ago

Assuming that works for you, I presume there is still an interest in the credManagement extension for residential keys? And howabout ed25519? Just trying to gauge priorities, currently embroiled in trying to debug our network stack.

BryanJacobs commented 2 years ago

It's not incredibly useful to support SSH resident keys without credential management, because you can't take the authenticator to a new computer and use it. ED25519 is nice and generally better than ecdsa/p256/secp256r1 (whatever you want to call it), but IMO less important than letting users carry their SSH keys on the token via credManagement...

bunnie commented 2 years ago

Got it. Thank you for the ELI5 on how the feature works. That bit of context was always missing for me.

jokeyrhyme commented 2 years ago

I can reproduce the expected workflow with the bleeding-edge build, yay <3

  1. ssh-keygen -t ecdsa-sk
  2. add contents of ~/.ssh/id_ecdsa_sk.pub to ~/.ssh/authorized_keys on remote host
  3. ssh to attempt connection to remote host
  4. tap-to-approve prompt appears on the Precursor (Relying Party is ssh:), tap
  5. connection to remote host established

Separately:

Yay!

bunnie commented 2 years ago

Alright, after poking around a bit at the OpenSK code base and what we have, here's the current verdict on supporting residential credentials:

I think it doesn't make sense to cherry-pick the 2.1 features into the 2.0 code base -- there's enough divergence between the two that it's not a small change, and if I'm going to put that kind of effort in I think it's better saved for bringing the overall system to 2.1, instead of wedging a 2.1 feature into a device that reports 2.0 compatibility.

The "problem" is that the development branch on OpenSK is still rapidly evolving, and indeed it seems the Fido 2.1 spec is still in "pre-release" form, with a date stamp as recent as June 21, 2022.

This might be the wrong call, but one thing I don't want to be doing is chasing down the latest features before they have been well-vetted and finalized. It's partially because we don't have the people-power to do it, but also a philosophical stance on security -- ideally if you're trusting high value keys to a device it's done right.

There's a point at which we upgrade our implementation to 2.1, but, looking at the most recent commits to the OpenSK code base, I think that day is not today, but it is probably "soon" -- it'll depend on when OpenSK stabilizes 2.1, and/or the 2.1 development status reflects that it's matured enough that we could do the merge and then chase the final tail of patches without too much pain. I guess you could say we have a Debian-ish approach to things, but it does feel kind of weird to be pushing a constant drip of feature updates and security patches to something that's supposed to be ostensibly secure.

This leaves open the question of should we expose the OpenSSH "feature" if we don't support residential credentials, given that it might not be so useful. One thing I'll note is that the slot in the 2.0 spec that reports we support credProtect is a bit of a lie. It's not that it "works", it's more that it just so happens if we return true there OpenSSH doesn't break entirely because it ignores the fact that we're 2.0 and just assumes we're 2.1.

Thus returning true for that field breaks compatibility with the strict 2.0 compliance test (this now makes sense why the Yubico tokens pass so few 2.0 tests -- they are 2.1 tokens, and the two specs are only partially interoperable, especially when it comes to PINs and user credentials).

@jokeyrhyme if being able to use Precursor without residential credentials is meaningful to you, I'll leave the patch in that allows it to happen so you can use it with openSSH for at least one use case for now, even though it creates regressions in the formal 2.0 test bench. The regressions don't make me lose sleep, if I am reading them correctly.

However, if it's the sort of thing where it's just not useful without the residential credentials and you'd rather wait until we're at FIDO2.1, then, I'll turn it off and leave openSSH support as a "roadmap" thing for when we transition to the OpenSK FIDO2.1 code base, which is presumably predicated on FIDO2.1 being fully released among other things. Hopefully "soon", but I am not privy to the machinations of the Fido Alliance.

jokeyrhyme commented 2 years ago

@bunnie none of this is an urgent deal-breaker for me

Non-resident keys are useful, too, but I'm happy for you to back out the patch and keep it as a roadmap item

Probaby best to not leave it in a weird state in a project with a privacy/security focus

Thanks heaps for putting in the time here, I hope it was a healthy/fun distraction from other priorities and not a stressful one :)

bunnie commented 2 years ago

Part of the struggle for me is that I'm implementing a feature that I don't clearly understand myself. I probably should use tokens more to manage things like SSH keys but it hasn't been a part of my work flow, so I have to rely on user feedback to guide this process.

I think the balance I was trying to strike in my question was more along the line of is the SSH key thing entirely useless without residential keys -- sounds like it's not.

I have looked at the regressions in the CTAP2 bench and I think it's worth letting them stand in order to see if more people discover/use the openSSH functionality and ask for residential keys. I need to get an accurate gauge of utility of various features so I can make the right call on how aggressively to pursue things -- when the FIDO feature was pitched in the chat channel to me, I recall a user simply said they wanted "FIDO2", which we implemented, but maybe they actually meant FIDO2.1, because this is what they actually wanted? Or maybe they actually meant FIDO2 -- I never really got any follow-up either way.

Part of the problem is also it seems that everyone uses U2F and FIDO2 and FIDO2.1 interchangeably but it turns out these are all materially different protocols with extremely different methods of handling secrets. I wish as an industry we could be a bit more strict about specifying these things because certainly it makes a world of difference whether you are doing, for example, SSL vs TLSv1 vs TLSv2. (The unfortunate reality is probably that when most people say they want a FIDO2 token what they probably actually mean is they want the superset of features (and bugs) present in a late-gen Yubico key, which is a bit like saying you want whatever version of the Internet Chrome can deliver, regardless of the W3C's spec)

Anyways, that is just a very long way of saying - since you didn't say the feature was entirely worthless, I think it's worth the risk at this stage to keep it around to see what other people discover and if there are more votes for residential keys versus other features on the docket like TLS, secure messaging, crypto wallet and/or ergonomic and performance improvements in the UX. I'll roll it back the first hint I hear of people having trouble with e.g. PIN authentication which is where it's most likely to conflict.

Thanks! I appreciate all the help in this issue, and the patience people had in explaining the basics of how this works to me. I have a much better understanding of this feature and overall my personal take is I want it; I can see a clear use case for keeping SSH keys on my Precursor, especially when I travel. Just gotta wait for FIDO2.1 to firm up a bit.

bunnie commented 2 years ago

Heh - entertaining thread related to this issue on the OpenSK repo:

https://github.com/google/OpenSK/issues/526#issuecomment-1213301164

Looks like they are hammering things out. Can't wait for them to push the latest to stable!

0xRake commented 2 years ago

@bunnie

hope these help (pretty sure they're old news though).


bunnie commented 1 year ago

Just ported the latest development branch from OpenSK to Xous. The results are now pushed to main, so bleeding-edge builds should have ed25519-sk resident keys.

Would be curious if this fixes the issue for you.

jokeyrhyme commented 1 year ago

Okay, running bleeding-edge ver xous -> v0.9.11-199-g25cc5712

One thing I noticed is that these SSH keys don't seem to show up anywhere in the Vault app, or maybe they do but the second page of my FIDO section is empty/blank? I think there should be a single "ssh:" (that is overwritten with each new keygen) :shrug:

For the above keys, I could not use ssh-add -K or ssh-add -L to retrieve the public key (which works for Yubikeys), which isn't a problem on the computer you first set them up on because the .pub file exists, but is a problem if you need to know the public key and you're on a different computer

When I tried, I got:

Enter PIN for authenticator:
You may need to touch your authenticator to authorize key download.
Provider "internal" returned failure -3
Unable to load resident keys: incorrect passphrase supplied to decrypt private key

I tried 0000, 000000, 1234, 123456, and the unlock code for the Precursor PDDB, and none of these worked :shrug:

Aside from that, it looks like we have end-to-end SSH authentication for both resident and non-resident keys, and for both ECDSA and ED25519 !!! <3 :tada:

bunnie commented 1 year ago

Strange. I see this on my vault app:

image

I do have to refresh the screen by toggling to one of the other modes and going back again after the key is added, however.

For the PIN issue, you can work around it by setting a PIN. The PIN is independent of the SSH protocol. For example, I went to https://demo.yubico.com/ and enrolled Precursor as a key with passwordless login; the site prompts me to create a PIN, which I create, and puts a key on the device.

Later on, the PIN is the same PIN that openSSH will ask for. You can delete the demo credential, as it expires in 24hrs anyways.

BryanJacobs commented 1 year ago

A way to set a PIN that doesn't involve visiting a random FIDO2 web site would be to use fido2-token -S <device>. You can change the PIN with fido2-token -C <device>, which I don't think you can do at all through the web browser.

I don't believe listing credentials that are CredProtect Level 2 without a PIN being provided or the credential ID itself being passed in complies with the spec. See https://fidoalliance.org/specs/fido-v2.1-rd-20210309/fido-client-to-authenticator-protocol-v2.1-rd-20210309.html#sctn-credProtect-extension . It's likely that your implementation doesn't match the standard (by returning CredProtect Level 2 creds in response to an unauthenticated ListCredentials call), and that's leading to the poor user experience above.

If memory serves, OpenSSH always creates Level 2 credentials and can't be configured not to do that.

bunnie commented 1 year ago

The listing of credentials on a device in the screen isn't even part of the spec since devices aren't supposed to have a screen...either way, the intent of listing things on the screen is to allow users to manage what credentials are on the device without having to use a command line.

I think the question that was being referred to was about credentials not showing on the screen, and not via commandline @jokeyrhyme ?

jokeyrhyme commented 1 year ago

Ah, thanks @BryanJacobs , I was trying to use ykman before to set the PIN, but that only seems to work on Yubikeys (which makes sense)

Once the PIN is set via fido2-token, it is possible to extract the public key using any machine:

nu ❯ fido2-token -L
/dev/hidraw0: vendor=0x1209, product=0x3613 (Kosagi Precursor)

nu ❯ fido2-token -S /dev/hidraw0
Enter new PIN for /dev/hidraw0:
Enter the same PIN again:

nu ❯ ssh-add -K
Enter PIN for authenticator:
Resident identity added: ED25519-SK SHA256:3E/...

nu ❯ ssh-add -L
sk-ssh-ed25519@openssh.com AAAA...

And yep, @bunnie , I was referring to the credentials list on the Precursor screen, and did try switching between different sections but could not see the "ssh: / --- (FIDO2)" item (even though I could see other credentials, and even though the SSH credentials were functional)

However, when I came back to the device later, it did show up in the list, so seems like a teeny glitch (or user error), not a blocker or a deal-breaker

I think we can close this issue, assuming you're happy with the implementation

BryanJacobs commented 1 year ago

The listing of credentials on a device in the screen isn't even part of the spec since devices aren't supposed to have a screen...either way, the intent of listing things on the screen is to allow users to manage what credentials are on the device without having to use a command line.

I think the question that was being referred to was about credentials not showing on the screen, and not via commandline @jokeyrhyme ?

@bunnie when running ssh-add -K, first OpenSSH will do a PIN Auth, and then it will do a FIDO2 ListCredentials operation. If the PIN Auth has failed or was skipped, the ListCredentials operation should omit returning any CredProtect Level 2 or 3 credentials.

I think what's happened here is that the PIN Auth failed (because the PIN was not set). After that, the FIDO2 ListCredentials still showed the credential - in violation of the spec. Then the ssh-add tried to get the credential and failed.

The user experience problem I'm talking about here is this:

Provider "internal" returned failure -3
Unable to load resident keys: incorrect passphrase supplied to decrypt private key

Please double-check that your implementation of CredProtect conforms to the spec for level 2 and 3 credentials.

jokeyrhyme commented 1 year ago

Are you referring to ssh-add -K here? https://github.com/betrusted-io/xous-core/issues/209#issuecomment-1407296321

I did enter the PIN correctly for that, so I think this complies

Back over here was when I didn't have a PIN set at all: https://github.com/betrusted-io/xous-core/issues/209#issuecomment-1406135026

And the only reason I could get connections to work without the PIN was because ssh-keygen spits out the public key during generation, just like any non-FIDO2 SSH key

bunnie commented 1 year ago

The listing of credentials on a device in the screen isn't even part of the spec since devices aren't supposed to have a screen...either way, the intent of listing things on the screen is to allow users to manage what credentials are on the device without having to use a command line. I think the question that was being referred to was about credentials not showing on the screen, and not via commandline @jokeyrhyme ?

@bunnie when running ssh-add -K, first OpenSSH will do a PIN Auth, and then it will do a FIDO2 ListCredentials operation. If the PIN Auth has failed or was skipped, the ListCredentials operation should omit returning any CredProtect Level 2 or 3 credentials.

I think what's happened here is that the PIN Auth failed (because the PIN was not set). After that, the FIDO2 ListCredentials still showed the credential - in violation of the spec. Then the ssh-add tried to get the credential and failed.

The user experience problem I'm talking about here is this:

Provider "internal" returned failure -3
Unable to load resident keys: incorrect passphrase supplied to decrypt private key

Please double-check that your implementation of CredProtect conforms to the spec for level 2 and 3 credentials.

When you run ssh-add -L, it will list all keys previously added, even if the authenticator is unplugged. I think this is actually the expected behavior though, because it's just listing you the locally cached public keys for that account.

When you run ssh-add -K, it will ask for a PIN, and if you don't provide it, it will print this:

Provider "internal" returned failure -3
Unable to load resident keys: incorrect passphrase supplied to decrypt private key

That looks to me like it's not returning any credentials, because it'll return that whether you have credentials, or you don't have credentials -- it's the same error message either way. You don't get anything else unless you enter a correct PIN.

If the behavior is supposed to be something different, please describe what that experience should be. I also suspect that openssh isn't even trying to return level 1 credentials (why would it, since it wouldn't do anything with them?), which is why there isn't a path that works without a PIN.

BryanJacobs commented 1 year ago

You can check whether the authenticator is working properly by doing a fido2-token -L -k <RPID> <device>. You can get the RPID via fido2-token -L -r <device>.

If you see the protected (CredProtect >=2) credential on a device with no PIN set... it's broken. I don't have this device to test, but I would have expected the ssh-add command to print something about not finding any credentials, rather than an error message saying that the credential's private key was invalid (in other words, a decryption failure).

bunnie commented 1 year ago

If you see the protected (CredProtect >=2) credential on a device with no PIN set... it's broken.

Your premise is you can get anywhere without a PIN set. I just wiped a device and tried talking to it with no PIN, this is how it goes:

% fido2-token -L -r /dev/hidraw0
fido2-token: fido_credman_get_dev_rp: FIDO_ERR_PIN_REQUIRED

% ssh-add -L
Could not open a connection to your authentication agent.
% ssh-add -K
Could not open a connection to your authentication agent.

So that's with no PIN set at all.

I set a PIN:

% fido2-token -S /dev/hidraw0
Enter new PIN for /dev/hidraw0:
Enter the same PIN again:

% fido2-token -L -r /dev/hidraw0
Enter PIN for /dev/hidraw0:
fido2-token: fido_credman_get_dev_rp: FIDO_ERR_NO_CREDENTIALS

In this case, I enter the PIN, and it reports no credentials, which is correct, because there are none.

I make a credential:

% ssh-keygen -t ed25519-sk -O resident
Generating public/private ed25519-sk key pair.
You may need to touch your authenticator to authorize key generation.
Enter PIN for authenticator:
...
The key's randomart image is:
+[ED25519-SK 256]-+
|       ++E.o=..=B|
|        B .+ o=B*|
|         .. o OB=|
|         . . =o*+|
|        S . . +  |
|         . . .   |
|          . = .  |
|         . = =   |
|         .=.. .  |
+----[SHA256]-----+

I now look up the RPID for the credential:

% fido2-token -L -r /dev/hidraw0
Enter PIN for /dev/hidraw0:
00: 4wYQ6KFiEVlg/h7CI+ZSnJ9LboAgDcteXDIcivHisb8= ssh:

I can access the credential when I give my PIN:

% fido2-token -L -k ssh: /dev/hidraw0
Enter PIN for /dev/hidraw0:
00: BfF67Mt3Yuu/rYo8jdav8muL6xNsY9fZ/lXItvmKu/o= openssh AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= eddsa uvopt+id

Now I try accessing an RPID that doesn't exist, but with a correct PIN:

% fido2-token -L -k foo /dev/hidraw0
Enter PIN for /dev/hidraw0:
fido2-token: fido_credman_get_dev_rk: FIDO_ERR_NO_CREDENTIALS

Now I try accessing the correct RPID, but "without a PIN", e.g., hitting "enter" when prompted:

% fido2-token -L -k ssh: /dev/hidraw0
Enter PIN for /dev/hidraw0:
fido2-token: fido_credman_get_dev_rk: FIDO_ERR_INVALID_ARGUMENT

Basically, the UX does not allow me to get to the state of even getting a list of credentials with no PIN set, or an incorrect PIN. So I am not even sure how I can get to this case of "list a level 2 credential without a PIN" because there is no ability to do that from the UI.

Now, I can do something totally off-spec and manually delete the PIN on the authenticator. When I now try to go and access the list of credentials, this is what I get:

% fido2-token -L -k ssh: /dev/hidraw0
fido2-token: fido_credman_get_dev_rk: FIDO_ERR_PIN_REQUIRED

Again, the UX isn't allowing me to proceed without first setting a PIN.

Thus I'm not sure how you're able to conclude that we're sending level 2 or 3 credentials without a PIN because there isn't a way to bypass entering a PIN on the UI to access SSH credentials. Of course it doesn't rule out someone writing a malicious client that can try to make a request without a PIN, but I would need that client or an equivalent command line to effectively test that corner case.

bunnie commented 1 year ago

Ah, thanks @BryanJacobs , I was trying to use ykman before to set the PIN, but that only seems to work on Yubikeys (which makes sense)

Once the PIN is set via fido2-token, it is possible to extract the public key using any machine:

nu ❯ fido2-token -L
/dev/hidraw0: vendor=0x1209, product=0x3613 (Kosagi Precursor)

nu ❯ fido2-token -S /dev/hidraw0
Enter new PIN for /dev/hidraw0:
Enter the same PIN again:

nu ❯ ssh-add -K
Enter PIN for authenticator:
Resident identity added: ED25519-SK SHA256:3E/...

nu ❯ ssh-add -L
sk-ssh-ed25519@openssh.com AAAA...

And yep, @bunnie , I was referring to the credentials list on the Precursor screen, and did try switching between different sections but could not see the "ssh: / --- (FIDO2)" item (even though I could see other credentials, and even though the SSH credentials were functional)

However, when I came back to the device later, it did show up in the list, so seems like a teeny glitch (or user error), not a blocker or a deal-breaker

I think we can close this issue, assuming you're happy with the implementation

I think I agree we can close this issue, but maybe we can open a new one to poke at the missing key in the UI issue after creation later on. It's orthogonal to the feature missing, and a problem in a different subsystem.

BryanJacobs commented 1 year ago

Looks like the authenticator here is indeed working properly, so I'll look into the OpenSSH-side behaviour to make it produce a better error message (and not prompt for a PIN when the device has none set, which should cause the noClientPin option to get returned?).

bunnie commented 1 year ago

Alright, thanks everyone for the feedback. I've opened a new issue https://github.com/betrusted-io/xous-core/issues/304 to track progress on the UX update problem. The root cause is that manipulations via USB don't trigger redraws the same way that interaction via the console does.