Add WinHello support

[github-cygwin]

Adding WinHello support to OpenSSH-portable

Version2:

• Make check_sk_options("uv") return true for WinHello devices. This is only required up to libfido2 1.10.0, a matching patch has been added to 1.11.0.
• sk_enroll: Never drop SSH_SK_USER_VERIFICATION_REQD flag from response. Explain why.
• Change how the token PIN is request in ssh-keygen and ssh. First ask the device. Only if it returns with error SSH_ERR_KEY_WRONG_PASSPHRASE, ask for the PIN.

[github-cygwin]

Just rebased to latest master

[github-cygwin]

Just rebased to latest master

[github-cygwin]

Rebased to latest master

[github-cygwin]

Is there any more input on this? I think I explained every design decision, so it would be nice to know if this stuff has a chance to be merged in at all, and if there's still something to discuss or to change in the code or whatever...

[github-cygwin]

@djmdjm ping?

[djmdjm]

I have merged most of this, but added a couple of comments to the remaining part

[github-cygwin]

I have merged most of this, but added a couple of comments to the remaining part

Hi Damien @djmdjm,

I changed the pending two patches per your and @martelletto's input, built and tested it under Cygwin. Pleae have a look.

Thanks, Corinna

[djmdjm]

I just merged something equivalent to the windows://hello change in cd06a76b7

[djmdjm]

I think that just leaves the Defer FIDO token PIN prompt when signing the credentials change to go; please see my last comments on that.

[github-cygwin]

I think that just leaves the Defer FIDO token PIN prompt when signing the credentials change to go; please see my last comments on that.

Hi Damien,

thanks for merging this stuff.

In terms of deferring the PIN prompt, after looking at my patch for quite a while today, I realized it was buggy from the start: I dropped the notify_complete() call from the retry loop for no apparent reason! By reverting this, the pieces fall into place by themselves.

Tested on Linux and Cygwin with "up"-only keys as well as "uv/up"-keys.

Please note that the "You may need to touch your authenticator." message is not targeting WinHello. It's targeting all other platforms. Without it, if the key is a "up"-only key, the first call to sshkey_sign(pin == NULL) would require to touch the authenticator without prompting for it.

Please see the latest patch I just pushed into this MR. I'm pretty sure it now works as desired.

As an extension, we could replace the "You may need to touch your authenticator." message with a "You may need to confirm user presence for key %s %s" message instead, but that might look a bit over the top with "uv/up"-keys:

$ ./ssh server
You may need to confirm user presence for key ED25519-SK SHA256:DHNZMpmDM7HQLUgdn6JUgUf6wwuC4DHsnrmXubxfs98
Enter PIN for ED25519-SK key /home/corinna/.ssh/id_ed25519_sk:
Confirm user presence for key ED25519-SK SHA256:DHNZMpmDM7HQLUgdn6JUgUf6wwuC4DHsnrmXubxfs98
User presence confirmed

so I think that's not feasible. What do you think?

Thanks, Corinna

[github-cygwin]

Hmm, what are those failures on Ubuntu 22.04? I can't see the error log, unfortunately. I built and tested this on Fedora 36, so I'm pretty sure it works on latest Linux...

[djmdjm]

I've pushed a slightly-tweaked version of that last commit as f9648090, so I think that is everything. Please take a look and close the PR if you're happy

[daztucker]

Hmm, what are those failures on Ubuntu 22.04?

I replace the deprecated 18.04 builders with 22.04, but 22.04 made a change to the home directory permissions which tripped up the agent-getpeereid test when it tried to run ssh-add as "nobody". It was fixed by 5a5c580b48fc6006bdfa731fc2f6d4945c2c0e4e.

I can't see the error log, unfortunately.

There's nothing sensitive in the logs, but I don't know if it's possible to make the logs world readable. I had a quick look at the settings and didn't see anything obvious.

Possibly relevant to your interests: I got a Cygwin build working on the Github hosted runners: https://github.com/openssh/openssh-portable/runs/7913583568?check_suite_focus=true

I'll be adding the "cygwin-release" config shortly, if there's anything else that should be added please let me know.

[daztucker]

I can't see the error log, unfortunately.

If I'm reading https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/using-workflow-run-logs correctly you should be able to see the logs for the public repo as long as you're logged into github itself. "You must be logged in to a GitHub account to view workflow run information, including for public repositories."

If that's not the case I'll see what we can do.

[github-cygwin]

I've pushed a slightly-tweaked version of that last commit as f964809, so I think that is everything. Please take a look and close the PR if you're happy

Hi Damien, I added a comment to https://github.com/openssh/openssh-portable/commit/f964809

I'm honestly sure that's not exactly how it should work, but please have a look.

Thanks, Corinna

[github-cygwin]

I can't see the error log, unfortunately.

There's nothing sensitive in the logs, but I don't know if it's possible to make the logs world readable. I had a quick look at the settings and didn't see anything obvious.

Actually, I can see the logs. I'm just not used to working on github, so I expected an error output in the "Details" view of a CI check. However, this only shows a red line with the text "Error:" and nothing else. Your reply here got me thinking and I searched for other ways to see the logs and, lo and behold, I finally found the "View raw logs" menu entry in the cogwheel menu on the upper right. D'oh. Thanks for nudging me.

Possibly relevant to your interests: I got a Cygwin build working on the Github hosted runners: https://github.com/openssh/openssh-portable/runs/7913583568?check_suite_focus=true

I'll be adding the "cygwin-release" config shortly, if there's anything else that should be added please let me know.

This is great news, thanks! I just noticed that the required devel packages are missing to build a full distro OpenSSH package. It needs all the Kerberos stuff, libedit-devel, libfido2-devel and all their dependencies. The config options used when building the distro package with cygport are

CYGCONF_ARGS="--libexecdir=/usr/sbin \
              --with-kerberos5=/usr \
              --with-libedit \
              --with-xauth=/usr/bin/xauth \
              --disable-strip \
              --with-security-key-builtin"

Thanks, Corinna

[github-cygwin]

One of my patches from the start of this endeavor got lost on the way, apparently. I pushed it again and the commit message tries to explain the problem it solves as detailed as possible.

I tested this scenario like this:

With master branch:

1. On Linux: ssh-keygen -t ed25519-sk
2. On Linux: ssh-keygen -t ed25519-sk -O verify-required
3. On Cygwin: ssh-keygen -t ed25519-sk
4. On Cygwin: ssh-keygen -t ed25519-sk -O verify-required

1: works on Cygwin, works on Linux 2: works on Cygwin, works on Linux 3: works on Cygwin, works on Linux 4: fails on Cygwin, fails on Linux

==> There's no way to use a "verify-required" key enrolled on Cygwin at present

With my patch c842d3568c81:

1. On Linux: ssh-keygen -t ed25519-sk
2. On Linux: ssh-keygen -t ed25519-sk -O verify-required
3. On Cygwin: ssh-keygen -t ed25519-sk
4. On Cygwin: ssh-keygen -t ed25519-sk -O verify-required

1: works on Cygwin, works on Linux 2: works on Cygwin, works on Linux 3: works on Cygwin, works on Linux 4: works on Cygwin, works on Linux

==> The user can now use the same "verify-required" key on Cygwin as well as on Linux, independent of the system is has been enrolled on!

Also, verify-required does not define if the verification is using "uv" or "clientPin". There's no real reason to generate different keys based on the input method used to enroll the key.

[martelletto]

Won't c842d3568c811bd5cb29404606c2456f1a680542 cause ssh(1) to prompt for the PIN on tokens capable of built-in UV, like the Yubikey Bio? That was the reason the flag was dropped.

[github-cygwin]

Won't c842d35 cause ssh(1) to prompt for the PIN on tokens capable of built-in UV, like the Yubikey Bio? That was the reason the flag was dropped.

I don't have such a device for testing. I only have a device called "Yubico.com Yubikey 4/5 OTP+U2F+CCID" in lsusb output.

I only see that "verify-required" keys for this token enrolled under WinHello works neither under WinHello, nor under Linux. That's very certainly not as desired. I'm not even clear on why the WinHello-enrolled key doesn't work under WinHello if the SSH_SK_USER_VERIFICATION_REQD flag gets removed from the key response.

A user enrolling a key on this token would very much like to use the same key on different systems, and enrolling a verify-required key should create identical keys independent of the OS the device has been enrolled under, or the method used for entering the PIN.

[github-cygwin]

Won't c842d35 cause ssh(1) to prompt for the PIN on tokens capable of built-in UV, like the Yubikey Bio? That was the reason the flag was dropped.

Actually, would it? I mean, if the token is "uv" capable, then the first call to sshkey_sign (with NULL pin) would request the PIN entry on the device. As such, the loop is as much short-circuited as when using WinHello, isn't it?

[martelletto]

I don't have such a device for testing. I only have a device called "Yubico.com Yubikey 4/5 OTP+U2F+CCID" in lsusb output.

If you would like one, please ping me at pedro at yubico.com.

Actually, would it? I mean, if the token is "uv" capable, then the first call to sshkey_sign (with NULL pin) would request the PIN entry on the device. As such, the loop is as much short-circuited as when using WinHello, isn't it?

I believe ssh-agent(1) would, since the check for SSH_SK_USER_VERIFICATION_REQD in process_sign_request2() happens before the first call to sshkey_sign(): https://github.com/openssh/openssh-portable/blob/master/ssh-agent.c#L810-L834.

[github-cygwin]

I don't have such a device for testing. I only have a device called "Yubico.com Yubikey 4/5 OTP+U2F+CCID" in lsusb output.

If you would like one, please ping me at pedro at yubico.com.

Oh, that might come in handy for testing "uv" under WinHello w/ Cygwin. Thanks for the offer!

Actually, would it? I mean, if the token is "uv" capable, then the first call to sshkey_sign (with NULL pin) would request the PIN entry on the device. As such, the loop is as much short-circuited as when using WinHello, isn't it?

I believe ssh-agent(1) would, since the check for SSH_SK_USER_VERIFICATION_REQD in process_sign_request2() happens before the first call to sshkey_sign(): https://github.com/openssh/openssh-portable/blob/master/ssh-agent.c#L810-L834.

Oh, darn, I completely lost track of ssh-agent. I always tested by just calling ssh on the command line without running ssh-agent.

Does anything speak against tweaking ssh-agent accordingly?

[github-cygwin]

I force-pushed a less intrusive version of the previous patch. This patch now only drops the SSH_SK_USER_VERIFICATION_REQD flag if the device used for enrolling the key is WinHello.

Edit: Sorry! Make that

... if the device used for enrolling the key is NOT WinHello.

The downside is that it might not work entirely correctly with keys on real "uv" devices, only for "clientPin" scenarios.

@martelletto, your input on this would be highly appreciated, especially whether or not the commit message makes sense.

I'm already looking into ssh-agent, but this might take a few days.

[github-cygwin]

Actually, the patch to ssh-agent was surprisingly simple. I tested it on Linux and Cygwin under WinHello, and it looks good on both systems.

[github-cygwin]

I force-pushed a less intrusive version of the previous patch. This patch now only drops the SSH_SK_USER_VERIFICATION_REQD flag if the device used for enrolling the key is [NOT] WinHello. [...] The downside is that it might not work entirely correctly with keys on real "uv" devices, only for "clientPin" scenarios.

And so it is. I got a Yubikey Bio from Yubikey for testing and it turns out that we now have the following situation (using Linux as a placeholder for "any non-WinHello scenario"):

- If we enroll a verify-required key under WinHello on a standard
  "pinEntry" YubiKey, and if we drop the SSH_SK_USER_VERIFICATION_REQD flag,
  the key is not recognized as a valid key, neither under WinHello, nor
  under Linux.

  Thus we must not drop the SSH_SK_USER_VERIFICATION_REQD flag.
  The resulting key works under WinHello and Linux alike.

- If we enroll a verify-required key under Linux on a "uv" YubiKey Bio
  with fingerprint recognition active, and if we drop the
  SSH_SK_USER_VERIFICATION_REQD, the key is not recognized as a valid key
  under WinHello, it only works under Linux.

  Again, not dropping the SSH_SK_USER_VERIFICATION_REQD flag makes the key
  work under both systems.

So I rolled back my sk_enroll patch to the previous version, which never drops the SSH_SK_USER_VERIFICATION_REQD. I tested this again on Linux and Cygwin, and the keys work fine on both systems with active "uv" on the Yubikey Bio, too.

Actually, the patch to ssh-agent was surprisingly simple. I tested it on Linux and Cygwin under WinHello, and it looks good on both systems.

It still looked ok on Cygwin under WinHello, but it has an unfortunate behaviour under Linux: No prompt while waiting for the "uv" input. So, not good.

The first two patches in the new patchset constitute the bare minimum to make it work with and without ssh-agent, on WInHello and other systems.

I tested them with keys created on Windows and Linux, both in the "clientPin" entry as well as in the biometric "uv" variation:

                Cygwin  w/ Agent  Linux   w/ Agent

Linux Bio       OK      OK        OK      OK

Cygwin Bio      OK      OK        OK      OK

Linux PIN       OK(1)   OK        OK(1)   OK(2)

Cygwin PIN      OK(1)   OK        OK(1)   OK(2)

(1) Preceeding "Confirm user presence ..." message. not acted upon (2) Preceeding "Confirm user presence ..." dialog, shown for just a fraction of a second, followed by the real "Enter PIN" dialog.

To get rid of the (1) and (2) downsides, we actually need more patches with a function call asking the middleware if we're talking to a "clientPin" or "uv" device. I added matching patches to allow just that.

@martelletto, any chance you can check if the new sk_test_option functionality makes sense as is, or if it requires addition stuff?

With this new feature, I also added a fallback mechanism from "uv" to "clientPin" if "uv" doesn't work for some reason (e. g. biometric reader fails to recognize user). This is default in WinHello and I think it's a good idea to support this in OpenSSH in general.

Thanks, Corinna

[github-cygwin]

I forgot to add a sk_test_option function to the regress sk-dummy, sorry about that. Now added.

[martelletto]

Thank you, @github-cygwin. I hope to give the changes a look on Friday or over the weekend. Meanwhile:

- If we enroll a verify-required key under WinHello on a standard
  "pinEntry" YubiKey, and if we drop the SSH_SK_USER_VERIFICATION_REQD flag,
  the key is not recognized as a valid key, neither under WinHello, nor
  under Linux.

That sounds a bit strange. Could you try to capture debug output from ssh on Linux?

[github-cygwin]

• If we enroll a verify-required key under WinHello on a standard "pinEntry" YubiKey, and if we drop the SSH_SK_USER_VERIFICATION_REQD flag, the key is not recognized as a valid key, neither under WinHello, nor under Linux.

That sounds a bit strange. Could you try to capture debug output from ssh on Linux?

What kind of debug output? strace or running ssh with SK_DEBUG/DEBUG_SK flags?

[martelletto]

Apologies for being unclear. The output of FIDO_DEBUG=1 ssh -vvv <host> should suffice.

[github-cygwin]

Apologies for being unclear. The output of FIDO_DEBUG=1 ssh -vvv <host> should suffice.

I collected the debug logs. Two files attached, one of which is the log for a key enrolled under WinHello with the SSH_SK_USER_VERIFICATION_REQD removed, as is the current default in the master branch: ssh-with-key-enrolled-under-winhello-with-SSH_SK_USER_VERIFICATION_REQD-flag-removed.log This key fails to work on WinHello as well as on Linux.

For comparison I attached the debug log for a key enrolled under WinHello, but with the SSH_SK_USER_VERIFICATION_REQD preserved, as in my patchset: ssh-with-key-enrolled-under-winhello-with-SSH_SK_USER_VERIFICATION_REQD-flag-preserved.log This key works as desired.

Edit: Please don't be confused by the usage of paths starting with /cygwin. That's my build dir for Cygwin stuff on my Linux server. The logs have been collected on my Linux desktop with OpenSSH built from current master, nevertheless.

HTH, Corinna