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

[martelletto]

Hi! I have left some comments. If you prefer to resume the conversation via e-mail, I am available at pedro@yubico.com. I would also be happy to arrange UV/non-UV keys for testing, if needed. Thank you.

[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 cd06a76

[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 f964809, 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 5a5c580.

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
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]

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

[github-cygwin]

Just for reference, this is what WinHello has to say about the key
with removed SSH_SK_USER_VERIFICATION_REQD flag:

Apologies for the path name claiming an OpenSSH version 8.9p0. That's
just my lazy ass who didn't rename the dir. I've built OpenSSH master
in that dir in fact 😅

[djmdjm]

I'm going to wait and not do anything until Pedro has had a chance to look at these changes - he's much better at this stuff then I am :)

[martelletto]

Thank you @github-cygwin and @djmdjm. I had a look at the patches and feedback.

A problem we will have in accommodating Windows Hello is that, contrary to FIDO's CTAP protocol, Microsoft's webauthn.h does not provide a way for the application to observe the actual capabilities of the token. All we can do is to perform enrollment/authentication operations and deduce the token's capabilities based on the result. That, however, does not allow us to distinguish between PIN and UV, since they look the same (by design) as far as results are concerned.

We have a good example of this difficulty here:

- 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.

This is happening because of our decision to consider Windows Hello a UV-capable token. Thinking the underlying token capable of built-in UV, we drop SSH_SK_USER_VERIFICATION_REQD when that does not reflect reality. I suggest not dropping SSH_SK_USER_VERIFICATION_REQD if fido_dev_is_winhello().

Overall, I suggest we take a step back and reevaluate the bare minimum needed to support Windows Hello. Once the bits for Windows Hello are all in, we could close this PR and look at improvements to existing functionality in a separate PR.

With that in mind, leaving the double "user presence" prompt aside for a second, is there anything else we need to do to support Windows Hello other than not dropping SSH_SK_USER_VERIFICATION_REQD if fido_dev_is_winhello()?

-p.

[github-cygwin]

Hi Pedro,

This is happening because of our decision to consider Windows Hello a UV-capable token. Thinking the underlying token capable of built-in UV, we drop SSH_SK_USER_VERIFICATION_REQD when that does not reflect reality. I suggest not dropping SSH_SK_USER_VERIFICATION_REQD if fido_dev_is_winhello().

But that's missing out on the other problem:

- 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.

By leaving that out, you're giving up on interoperability. A user enrolling
a key under any system won't accept that the key is unusable under another
system.

That's exactly the reason I reverted my patch so SSH_SK_USER_VERIFICATION_REQD
is always preserved.

The key itself is a "verify-user" key. The key should not enforce how
the user verification takes place.

Overall, I suggest we take a step back and reevaluate the bare minimum needed to support Windows Hello. Once the bits for Windows Hello are all in, we could close this PR and look at improvements to existing functionality in a separate PR.

So you would like to move the sk_test_option stuff entirely into a new PR?
That's ok, but it 's also a pity, becasue that was exactly the stuff I was
looking for input on 😳

With that in mind, leaving the double "user presence" prompt aside for a second, is there anything else we need to do to support Windows Hello other than not dropping SSH_SK_USER_VERIFICATION_REQD if fido_dev_is_winhello()?

Yes, see above. The "don't drop SSH_SK_USER_VERIFICATION_REQD only if
fido_dev_is_winhello()" was insufficient as the experiments with Yubikey Bio
clearly shows. I, and probably all other users using both systems, would really
prefer to be able to use a key independent of the system it has been enrolled on.

Thanks,
Corinna

[martelletto]

Hi Corinna,

But that's missing out on the other problem:

- 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.

Do we know why that is failing? I suspect it is because of this snippet in sk_sign():

	/*
	 * WinHello requests the PIN by default.  Make "uv" request explicit
	 * to allow keys with and without -O verify-required to make sense.
	 */
	if (pin == NULL && fido_dev_is_winhello (sk->dev) &&
	    (r = fido_assert_set_uv(assert, FIDO_OPT_FALSE)) != FIDO_OK) {
		skdebug(__func__, "fido_assert_set_uv: %s", fido_strerr(r));
	}

which explains why you want/need SSH_SK_USER_VERIFICATION_REQD. Without the flag, this snippet causes Windows Hello to hang with a "this security key does not look familiar" error message on UV-required keys.

My preference would be to drop this snippet, even if it causes unexpected PIN prompts from Windows Hello, and eventually retire SSH_SK_USER_VERIFICATION_REQD. It would be unfortunate to need the flag (and the complexity it entails) just because of Windows Hello.

Perhaps there is something we can do. If possible, can you try to patch libfido2's set_assert_uv() in winhello.c so that the function sets *out to WEBAUTHN_USER_VERIFICATION_REQUIREMENT_ANY instead of WEBAUTHN_USER_VERIFICATION_REQUIREMENT_PREFERRED if uv == FIDO_OPT_OMIT?

So you would like to move the sk_test_option stuff entirely into a new PR? That's ok, but it 's also a pity, becasue that was exactly the stuff I was looking for input on 😳

Absolutely! My bad. I just wanted to get Windows Hello out of the picture and look at the functionality as a whole. In a nutshell, I agree with the intention, but believe the execution could be simpler. For example, it would be best if we could avoid having code outside sk-usbhid.c care about FIDO concepts such as PIN and UV and FIDO options in general. Something along the lines of:

 retry_sk:                                                                      
        sk_flags = SSH_SK_CHECK_TOKEN;                                          
        if ((r = sshkey_sign(sign_key, NULL, 0, NULL, 0, alg,                   
            options.sk_provider, pin, compat, &sk_flags)) != 0) {               
                /* error */                                                     
        }                                                                       
        if (sk_flags & SSH_SK_NO_TOKEN_FOUND) {                                 
                /* prompt user to insert token */                               
                goto retry_sk;                                                  
        }                                                                       
        if (pin == NULL && (sk_flags & SSH_SK_PIN_NEEDED)) {                    
                /* read pin */                                                  
        }                                                                       
        if (sk_flags & SSH_SK_USER_PRESENCE_NEEDED) {                           
                /* prompt for user presence */                                  
        }
        sk_flags &= ~SSH_SK_CHECK_TOKEN;                  
        if ((r = sshkey_sign(sign_key, sigp, lenp, data, datalen,               
            alg, options.sk_provider, pin, compat)) != 0) {                     
                /* error */                                                     
        }        

Does that make sense?

-p.

[github-cygwin]

Hi Pedro,

[Editorial Note: See my followup to my own reply first, before answering here. I just kept this in for the sake of discussion]

just the first point for now, I have to check into the other stuff later.

Hi Corinna,

But that's missing out on the other problem:

- 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.

Do we know why that is failing? I suspect it is because of this snippet in sk_sign():

	/*
	 * WinHello requests the PIN by default.  Make "uv" request explicit
	 * to allow keys with and without -O verify-required to make sense.
	 */
	if (pin == NULL && fido_dev_is_winhello (sk->dev) &&
	    (r = fido_assert_set_uv(assert, FIDO_OPT_FALSE)) != FIDO_OK) {
		skdebug(__func__, "fido_assert_set_uv: %s", fido_strerr(r));
	}

which explains why you want/need SSH_SK_USER_VERIFICATION_REQD. Without the flag, this snippet causes Windows Hello to hang with a "this security key does not look familiar" error message on UV-required keys.

That's very certainly not the reason for this problem. I didn't test this yet, but it would be very strange.

WinHello defaults to user verification. The result is that for OpenSSH keys created without "verify-required", WinHello still requests user verification. On other systems I can create OpenSSH keys which only ask for user presence, on WinHello this is impossible, because of this default.

So this snippet just sets the default for user verification to FALSE. This allows keys created without "verify-required" to be checked for user presence only on WinHello just as well as on any other system.

However, we're talking about "verify-required" keys here only. For these keys, the next code snippet will do the right thing, independent of the above code snippet:

        if (pin == NULL && (flags & SSH_SK_USER_VERIFICATION_REQD)) {

Just what we get, no PIN, but the SSH_SK_USER_VERIFICATION_REQD flag is set

                if (check_sk_options(sk->dev, "uv", &internal_uv) < 0 ||
                    internal_uv != 1) {

Nope, we actually have a device supporting "uv", so we don't return with SSH_SK_ERR_PIN_REQUIRED, but instead...

                        skdebug(__func__, "check_sk_options uv");
                        ret = SSH_SK_ERR_PIN_REQUIRED;
                        goto out;
                }
                if ((r = fido_assert_set_uv(assert,
                    FIDO_OPT_TRUE)) != FIDO_OK) {

...we set the "uv" flag explicitely to TRUE.

                        skdebug(__func__, "fido_assert_set_uv: %s",
                            fido_strerr(r));
                        ret = fidoerr_to_skerr(r);
                        goto out;
                }
        }

I don't see how setting the value to FALSE by default changes anything here, because the subsequent code will set the value for "verify-required" keys to TRUE anyway, isn't it?

Thx,
Corinna

[github-cygwin]

Oh, wait. I confused cause and effect!

If we actually drop the SSH_SK_ERR_PIN_REQUIRED flag, the condition

  if (pin == NULL && (flags & SSH_SK_USER_VERIFICATION_REQD)) {

fails, and thus

  fido_assert_set_uv(assert, FIDO_OPT_TRUE)

is never called.

So if I change

        if (pin == NULL && fido_dev_is_winhello (sk->dev) &&
            (r = fido_assert_set_uv(assert, FIDO_OPT_FALSE)) != FIDO_OK) {
                skdebug(__func__, "fido_assert_set_uv: %s", fido_strerr(r));
        }

to

        if (pin == NULL && fido_dev_is_winhello (sk->dev) &&
            !(flags & SSH_SK_USER_VERIFICATION_REQD) &&
            (r = fido_assert_set_uv(assert, FIDO_OPT_FALSE)) != FIDO_OK) {
                skdebug(__func__, "fido_assert_set_uv: %s", fido_strerr(r));
        }

it should work as desired. This would allow user presence keys to be used as actual user presence keys on WinHello, too, and user verification keys should work, even if we drop the SSH_SK_USER_VERIFICATION_REQD flag for keys enrolled on "uv" devices.

But still, IMHO, the SSH_SK_USER_VERIFICATION_REQD flag is used wrongly. Its name is confusing, because it *seems to include keys enrolled on "uv"-capable devices, but in fact it appears to include only keys enrolled on "clientPin" devices.

While the above might fix the problem, I really wonder if it's the right thing to do.

IMHO, a "verify-required" key should not have different flags, just because the device it has been enrolled on has different capabilities. It's the wrong place to make such a decision.

So, yes, it would be great to get rid of the SSH_SK_USER_VERIFICATION_REQD flag in the long run, but wouldn't it be just as well to clean up how it's used as long as we still have it?

Thanks,
Corinna

[martelletto]

Hi Corinna,

Your analysis is correct, but do note that it is predicated on SSH_SK_USER_VERIFICATION_REQD being set for tokens advertising built-in UV. That is what I meant when I said this is the reason you want/need the flag for Windows Hello.

On a not completely unrelated note, have you had a chance to try -O no-touch-required with Windows Hello? Last I checked, disabling user presence was not supported by webauthn.dll. This is bound to create annoying interoperability inconsistencies as well.

-p.

[martelletto]

oops, we replied at the same time. Glad we are on the same page. Let me mull it a bit and get back to you; an adequate reply would take more time than I have at the moment. In the meantime, it would probably make sense to look into -O no-touch-required as per preceding comment.

[github-cygwin]

Hi Corinna,

Your analysis is correct, but do note that it is predicated on SSH_SK_USER_VERIFICATION_REQD being set for tokens advertising built-in UV. That is what I meant when I said this is the reason you want/need the flag for Windows Hello.

Yes, we need the flag for WinHello, but the problem is, we need it for keys enrolled on any system, not only for keys on WinHello. The current code breaks interoperability,
because the enrollment decides over the fate of the key, not the signing. Some decisions can not be performed at signing time for that reason.

On a not completely unrelated note, have you had a chance to try -O no-touch-required with Windows Hello? Last I checked, disabling user presence was not supported by webauthn.dll. This is bound to create annoying interoperability inconsistencies as well.

Yes, I did this right at the start. no-touch-required keys don't work on WinHello.
I ignored that so far, but we need a solution for this, too, at one point, even if it's just a message telling the user about this fact.

Corinna

[github-cygwin]

So if I change
[...]
to
[...]
it should work as desired. This would allow user presence keys to be used as actual user presence keys on WinHello, too, and user verification keys should work, even if we drop the SSH_SK_USER_VERIFICATION_REQD flag for keys enrolled on "uv" devices.

On second thought, no, it wouldn't.

If I enroll a "verify-required" key on a "uv" device, the SSH_SK_USER_VERIFICATION_REQD flag will be removed. Thus, there's no way anymore to distinguish plain keys which only require user presence from keys requiring user verification. Thus the idea to allow plain user presence keys will be broken for "uv" devices, if the "uv" device defaults to user verification.

So, if I'm not again missing a crucial detail here, that would be another reason never to drop the SSH_SK_USER_VERIFICATION_REQD flag, as outlined in my previous monologue...

Corinna

[github-cygwin]

For example, it would be best if we could avoid having code outside sk-usbhid.c care about FIDO concepts such as PIN and UV and FIDO options in general. Something along the lines of:

 retry_sk:                                                                      
        sk_flags = SSH_SK_CHECK_TOKEN;                                          
        if ((r = sshkey_sign(sign_key, NULL, 0, NULL, 0, alg,                   
            options.sk_provider, pin, compat, &sk_flags)) != 0) {               
                /* error */                                                     
        }                                                                       
        if (sk_flags & SSH_SK_NO_TOKEN_FOUND) {                                 
                /* prompt user to insert token */                               
                goto retry_sk;                                                  
        }                                                                       
        if (pin == NULL && (sk_flags & SSH_SK_PIN_NEEDED)) {                    
                /* read pin */                                                  
        }                                                                       
        if (sk_flags & SSH_SK_USER_PRESENCE_NEEDED) {                           
                /* prompt for user presence */                                  
        }
        sk_flags &= ~SSH_SK_CHECK_TOKEN;                  
        if ((r = sshkey_sign(sign_key, sigp, lenp, data, datalen,               
            alg, options.sk_provider, pin, compat)) != 0) {                     
                /* error */                                                     
        }        

Does that make sense?

It makes sense, and it would really be nice if we could encapsulate the FIDO concepts inside sk-usbhid.c.

I just see a problem there as soon as user prompting is required. Right now, the UI is solely in the application layer. The above code is moving prompting into the middleware. That means, ssh-sk-helper would have to be able to call all the prompting calls like read_passphrase(), notify_start(), etc. I think that's not feasible, the helper is not UI-aware.

My stance is this: The application layer is responsible for the UI. If the UI gets changed or improved, the middleware should be unaffected.

To do the UI right in terms of hardware tokens, there's no easy way out. The application layer needs some information about the current scenario, and that includes to learn what way of user verification should be performed, because that's UI.

Having said that, the way I solved it is not necessarily the best way. It requires actual knowledge of the FIDO2 protocol and the meaning of strings used therein ("uv", "clientPin").

Either way, the UI basically needs the information

• is some kind of user verification required?
• does the current situation require entering a PIN?

Maybe for a start we should just dumb down the sk_test_option call I added in 99d7032, along the lines of

  int sk_test_if_we_need_pin_prompting();  /* true == device doesn't support "uv" */
  int sk_test_if_we_allow_pin_prompting(); /* true == device supports "clientPin" */

The function names need some polishing, but you get my drift, I think.

Thanks,
Corinna

[martelletto]

Hi Corinna,

Quickly chiming in here to keep the conversation going. Please excuse me for my brevity.

If I enroll a "verify-required" key on a "uv" device, the SSH_SK_USER_VERIFICATION_REQD flag will be removed. Thus, there's no way anymore to distinguish plain keys which only require user presence from keys requiring user verification. Thus the idea to allow plain user presence keys will be broken for "uv" devices, if the "uv" device defaults to user verification.

There is a way to differentiate between keys requiring only user presence from keys requiring user verification without SSH_SK_USER_VERIFICATION_REQD, which is to attempt to use the key and check for FIDO_ERR_PIN_REQUIRED. This is how the code currently works.

However, as observed in this PR, that is not possible when using Windows Hello, because Windows Hello is not a FIDO2 token, and therefore does not conform to the FIDO2 client-to-authenticator (CTAP) spec. Instead of returning FIDO_ERR_PIN_REQUIRED, Windows Hello hangs with a "this security does not look familiar" message.

I am not fundamentally opposed to the reintroduction of SSH_SK_USER_VERIFICATION_REQD, but it is important that we are on the same page about what it does and why it is needed. The only reason to reintroduce SSH_SK_USER_VERIFICATION_REQD (even if we end up using it in other code paths) is Windows Hello.

Regarding changes to the notification/prompting process:

I just see a problem there as soon as user prompting is required. Right now, the UI is solely in the application layer. The above code is moving prompting into the middleware. That means, ssh-sk-helper would have to be able to call all the prompting calls like read_passphrase(), notify_start(), etc. I think that's not feasible, the helper is not UI-aware.

If that is how you understood it, then I wasn't clear. My apologies. I did not mean to suggest moving the prompting to the middleware. What I had in mind was having the middleware try to use the key and respond with a set of flags telling ssh/ssh-agent what to prompt for. The prompting would take place where it currently does, it would just be "instructed" by the middleware.

-p.

[github-cygwin]

There is a way to differentiate between keys requiring only user presence from keys requiring user verification without SSH_SK_USER_VERIFICATION_REQD, which is to attempt to use the key and check for FIDO_ERR_PIN_REQUIRED. This is how the code currently works.

However, as observed in this PR, that is not possible when using Windows Hello, because Windows Hello is not a FIDO2 token, and therefore does not conform to the FIDO2 client-to-authenticator (CTAP) spec. Instead of returning FIDO_ERR_PIN_REQUIRED, Windows Hello hangs with a "this security does not look familiar" message.

I am not fundamentally opposed to the reintroduction of SSH_SK_USER_VERIFICATION_REQD, but it is important that we are on the same page about what it does and why it is needed. The only reason to reintroduce SSH_SK_USER_VERIFICATION_REQD (even if we end up using it in other code paths) is Windows Hello.

Ok, I think I got that now. Bummer that WinHello is not working conformant in this single area, it could have been so easy, oh well. Unless... there isn't a way in WinHello to enforce that the call returns immediately, by any chance?

So, yeah, I accept that we need the SSH_SK_USER_VERIFICATION_REQD flag for WinHello only, but apparently we need it yet.

How do we proceed? I guess in the light of the above, we can push the first two patches, baa2928 and 6b3c62d now?

Also, 2f2b0b1 should be obvious, right? @djmdjm?

Regarding changes to the notification/prompting process:

I just see a problem there as soon as user prompting is required. Right now, the UI is solely in the application layer. The above code is moving prompting into the middleware. That means, ssh-sk-helper would have to be able to call all the prompting calls like read_passphrase(), notify_start(), etc. I think that's not feasible, the helper is not UI-aware.

If that is how you understood it, then I wasn't clear. My apologies. I did not mean to suggest moving the prompting to the middleware. What I had in mind was having the middleware try to use the key and respond with a set of flags telling ssh/ssh-agent what to prompt for. The prompting would take place where it currently does, it would just be "instructed" by the middleware.

I'm pretty sure I don't understand exactly how to do that. Look at 7262b10, please. I rearranged prompting again, to be able to generate the correct user information in the UI before calling sshkey_sign.

For instance if the device is a "uv" device, how would you be able to do this inside the sshkey_sign call itself? The first call might already succeed and you're too late to do any prompting.

How you would do that without giving the UI at least some information prior to the sshkey_sign call, some kind of call allowing the UI to ask the middleware what is supposed to happen next. That's where my sshsk_test_option() call comes into play. Apart from stylistic changes, I'm really unsure how to do this differently, let alone, encapsulated inside sshkey_sign.

BTW., I'm more or less offline the next 4 weeks, starting this weekend.

Corinna

[github-cygwin]

just rebased...

[djmdjm]

I don't think this will do the right thing when multiple devices are present, which is a case we've worked fairly hard to support. Given multiple devices IMO "device supports feature X" is only useful the presence of a key handle that refers to that device.

Maybe a better way of supporting this would be to add another per-key flag, say SSH_SK_UV_PROVIDED that is set when the token provides user verification internally. We could set this for biometric keys, etc and then use it to skip or proactively prompt for PINs.

Would this help?

I.e. putting something like this in sk_enroll()

if (check_sk_options(sk->dev, "uv", &internal_uv) == 0 && internal_uv != -1) {
        /* user verification handled by token */
        response->flags &= ~SSH_SK_UV_PROVIDED;
}

(yes, this looks a lot like the code that I just removed via Corinna's change)

[github-cygwin]

just rebased onto current master

[github-cygwin]

The Fuzzing check failed with a weird DWARF error, followed by

ssh-sk.o: in function `sshsk_open':
ssh-sk.c:(.text.sshsk_open[sshsk_open]+0x354): undefined reference to `ssh_sk_test_option'

I built and ran the testsuite successfully, so I'm puzzeled why this fails in the fuzzing test. What's missing?