Change attstmt to handle x5c array/list

Yubico / libfido2

Provides library functionality for FIDO2, including communication with a device over USB or NFC.

Other

581 stars 153 forks source link

Change attstmt to handle x5c array/list #781

Closed yesvivek closed 7 months ago

yesvivek commented 7 months ago

According to webauthn spec the x5c object under attStmt is of array type. But current code stores only the leaf certificate and discards the other certificates - reported as #749. This patch attempts to fix that.

Summary of changes,

x5c object under fido_attstmt is fido_blob_array_t type.
decode_x5c and fido_cred_set_x509 is updated to store as array.
fido_cred_x5c_ptr and fido_cred_x5c_len are updated to retain backward compatibility; they now work with first certificate in the list.
New API fido_cred_x5c_list_count added to return number of certificates in the x5c.
New API fido_cred_x5c_list_ptr added to return requested certificate data in the x5c.
New API fido_cred_x5c_list_len added to return byte length of requested certificate in the x5c.

yesvivek commented 7 months ago

@LDVG do note this is my first PR, ever. Also, am usually not good at naming functions/variables (still learning). So, feel free to correct them :) Also, I have only linux (ubuntu 22) with me. So, no clue how to update for other platforms.

LDVG commented 7 months ago

@LDVG do note this is my first PR, ever. Also, am usually not good at naming functions/variables (still learning). So, feel free to correct them :)

We'll ponder on what to call them as the dust settles. FWIW, I think the current naming is fine. :-)

Also, I have only linux (ubuntu 22) with me. So, no clue how to update for other platforms.

The code that this PR touches is common for all platforms that we support, so that should be fine.

P.S.: I think src/export.msvc was left out of the most recent commit?

yesvivek commented 7 months ago

Ah, my bad, got missed when adding. Pushed now..

LDVG commented 7 months ago

I added a couple of minor tweaks on top of your commits. For some reason, this caused GitHub's UI to not recognize your account authoring them anymore, despite the commits having the same SHA as previously (maybe it doesn't recognize your author email address?). If this is important to you and you want to correct this, you can feel free to squash your commits and my tweaks down to one commit. :-)

yesvivek commented 7 months ago

@LDVG I dont see any option other than using git rebase or reset and doing a commit again, but I dont know what side effects it might have. So, is it possible for you to squash them?

LDVG commented 7 months ago

@LDVG I dont see any option other than using git rebase or reset and doing a commit again, but I dont know what side effects it might have. So, is it possible for you to squash them?

I went ahead and squashed them, by rebasing. :-)

This looks good to me, thanks for your contribution and congratulations on your first GitHub PR!

yesvivek commented 7 months ago

Thank you @LDVG . Appreciate your guidance, certainly learned new things from you.