Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple cred pub key types during credential creation #722

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

bobomb
Copy link
Contributor

@bobomb bobomb commented Aug 31, 2023

Add a member of type fido_blob_t named type_winhello to fido_cred_t.

Add fido_cred_set_type_winhello which takes a pointer to an array of 32 bit signed integers and length. The old type member of fido_cred_t is set to the first element of this array of public key types. It is stored as a fido_blob_t.

Create a copy of decode_attcred as decode_attcred_multiple_cose to handle using the list of key types.

Create a copy of cbor_decode_cred_authdata as cbor_decode_cred_authdata_multiple_cose to handle using the array of key types.

When translating back from windows hello to libfido2 cred, set the existing cred type field to the type that was returned.

This is work that was done to support using Windows Hello face/pin and common USB authenticators in a single call to fido_dev_make_cred instead of having to call it once for each type.

What I found in practice is that RP's that request both RS256 and ES256 required me to pick which one to try. Windows Hello face/pin only supports RS256, but most of the USB authenticators I tried only supported ES256. When setting the cred type to only ES256, Windows Hello gave me the option for face/pin but failed. When setting to only RS256, face/pin works but USB keys failed. By setting both and passing that along to Windows Webauthn API's, either one works and the credential is returned with the actual type that was used. This also helps avoid a situation where if you specify only RS256 you get a Windows Hello prompt, but if you cancel then it still seems to ask if you want to use USB authenticators, and it gets stuck into an infinite cycle where you can input pin/touch the key and then get prompted to input pin/touch key repeatedly unless you cancel, most likely due to the USB authenticator not supporting RS256.

… creation

Add a member of type `fido_blob_t` named `type_winhello` to `fido_cred_t`.

Add `fido_cred_set_type_winhello` which takes a pointer to an array of
32 bit signed integers and length. The old `type` member of
`fido_cred_t` is set to the first element of this array of public key
types. It is stored as a `fido_blob_t`.

Create a copy of `decode_attcred` as `decode_attcred_multiple_cose` to
handle using the list of key types.

Create a copy of `cbor_decode_cred_authdata` as `cbor_decode_cred_authdata_multiple_cose` to
handle using the array of key types.
@bobomb
Copy link
Contributor Author

bobomb commented Sep 1, 2023

Seems like my warnings generated more warnings. Will fix and get it building properly first.

@bobomb
Copy link
Contributor Author

bobomb commented Sep 1, 2023

I hadn't paid attention to the open issues, but this should partially resolve the issue #140 for the windows hello case

@LDVG
Copy link
Contributor

LDVG commented Sep 4, 2023

Hi,

Thank you for your contribution. This does seem like a use-case we want to support.

As you have found out already, there is an open issue on the same topic and I'd probably want to see an API which would suit all of our transport layers rather than something windows://hello specific (fido_cred_set_type_array() or something similar) while also keeping backwards compatibility with fido_cred_{set_,}type().

Having briefly thought about this, maybe it'd be sufficient to apart from the above:

  • replace the existing fido_cred_t::type member with a new fido_int_array_t struct (to avoid having to cast from fido_blob_t's unsigned char *);
  • let fido_cred_set_type() set an array with a single item; and
  • let fido_cred_type() return the type used by the authenticator (is a getter for the array necessary?)

You may see that this bears a lot of similarities with the approach you've already implemented but would allow for us to drop the (duplicated) cbor_decode_cred_authdata_multiple_cose() and decode_attcred_multiple_cose() functions and multiple if/else conditions depending on whether there is an array or not -- with the usual caveat that I may be missing something.

What do you think about such an approach?

@bobomb
Copy link
Contributor Author

bobomb commented Sep 5, 2023

Hi @LDVG,

Thank you for the feedback on my implementation. In my haste to implement this functionality I tried to ensure the existing mechanism for setting the credential type was not touched at all, which is why I went with adding a second field and API set. For a proper implementation though I agree with your approach, this would allow the existing uses of the public API fido_cred_set_type() to continue working as is, while allowing the addition of fido_cred_set_type_array() to handle the multiple credential type case.

I haven't looked too much at the usages of the fido_cred_t::type field but I would guess there are a few places it is used that would need refactoring as well in order to work with the new type of fido_int_array_t

@bobomb bobomb changed the title Allow multiple cred pub key types for windows hello during credential creation Allow multiple cred pub key types during credential creation Jan 3, 2024
@bobomb
Copy link
Contributor Author

bobomb commented Jan 3, 2024

Hi @LDVG ,
This is my first pass at making the changes you have suggested earlier. I've incorporated your suggestions and kept the API backwards compatible. A an array of credential algorithms can be taken as input, and during the response decode stage the algorithm is matched against the array, and the credential type is set to an array that contains only the matched element, otherwise an error is returned and the operation fails. I'll be refining it as I test it, but it would be great to have some feedback in the meantime.

@bobomb
Copy link
Contributor Author

bobomb commented Jan 3, 2024

Actually, looks like I forgot the new setter on fido_cred_t to set an array of algorithms, and there is still some cleanup from my prior commit that was windows specific.

Copy link
Contributor

@LDVG LDVG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! This looks similar to what I had in mind. I've left a couple of high-level comments that may warrant some discussion:

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/cbor.c Outdated Show resolved Hide resolved
src/cbor.c Outdated Show resolved Hide resolved
src/cred.c Show resolved Hide resolved
src/cred.c Show resolved Hide resolved
src/cred.c Outdated Show resolved Hide resolved
src/u2f.c Outdated Show resolved Hide resolved
src/int_array.h Outdated Show resolved Hide resolved
src/int_array.h Outdated Show resolved Hide resolved
src/touch.c Outdated Show resolved Hide resolved
(body = cbor_new_definite_map(2)) == NULL ||
cose_alg > -1 || cose_alg < INT16_MIN)
goto fail;
if ((item = cbor_new_definite_array(cose_alg_array->count)) == NULL)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to check the length of cose_alg_array and error out if it's zero before creating these structs in libcbor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants