RIOT icon indicating copy to clipboard operation
RIOT copied to clipboard

Fido2 follow up continued

Open Ollrogge opened this issue 3 years ago • 6 comments

Contribution description

This PR adds changes to make the FIDO2 API usable without a transport layer as well as improve the overall usability. With this come changes specific to the native target, to account for the fact that mtd flash handling is file backed in this case.

This also includes breaking changes to the public API:

  • All public methods defined in "sys/include/fido2/ctap.h" now return a ctap_status_code_t instead of the size of the response.
  • The size of the FIDO2 response is now contained within the response structure ctap_resp_t.

Users of the FIDO2 API need to adjust their applications to now expect a status code as the return value of functions such as fido2_ctap_handle_request and expect the length of the response in resp->len.

To test the usability of FIDO2 without a transport layer, this PR also adds another test module which tests the CTAP implementation without transport layer.

With the new test module being added, the old tests are renamed to sys_fido2_ctap_hid to highlight the fact that they test the CTAP2 implementation using CTAPHID as transport binding.

Furthermore, this PR removes the dependency of ctap_hid for ztimer64 as having 64 bit timestamps is not required.

Testing procedure

  • tests/sys_fido2_ctap
  • tests/sys_fido2_ctap_hid

Issues/PRs references

Depends on PR #18637 Issue regarding file backed flash memory on native: #19559

Ollrogge avatar Sep 28 '22 20:09 Ollrogge

With #18637 having been merged a while a go, I would like to get this in as well. Rebased on latest master.

Ollrogge avatar Mar 27 '23 10:03 Ollrogge

@chrysn Can we get this in ?

Ollrogge avatar May 08 '23 13:05 Ollrogge

link to #19559 native is using file backed mtd

kfessel avatar May 25 '23 10:05 kfessel

i think one of the test application is "copied" ( old copy should be deleted) to the new path structure one is not moved yet.

I am not sure how many users of this module are out there, this changes a lot of api from return the size of data to return an error code (misinterpretation might not be nice).

kfessel avatar Sep 30 '24 10:09 kfessel

i think one of the test application is "copied" ( old copy should be deleted) to the new path structure one is not moved yet.

Ah yes, I missed the directory structure changes. Will fix

I am not sure how many users of this module are out there, this changes a lot of api from return the size of data to return an error code (misinterpretation might not be nice).

Currently I don't think very many. I personally don't know of anybody. One reason for this was (at least the last time I checked) that many applications lacked FIDO2 support and only support FIDO U2F, which is not implemented in RIOT currently. I am planning to put some more work into the FIDO2 implementation in order to get a fully working RIOT security key at some point :)

Either way, I don't think the API change will affect many people.

Ollrogge avatar Sep 30 '24 10:09 Ollrogge

please add a "cleanup" commit that fixes longlines ( >100 chars) and consecutive empty lines if possible

kfessel avatar Oct 07 '24 10:10 kfessel

please add a "cleanup" commit that fixes longlines ( >100 chars) and consecutive empty lines if possible

Fixed for every location that is not a structure documentation

Ollrogge avatar Oct 25 '24 12:10 Ollrogge

@Ollrogge @chrysn

I just looked at the code the changes seem sane and build tests are happy. As @Ollrogge is the main consumer of this code and he is the original author I would just accept it if nothing comes up within 2 weeks, please remind me.

kfessel avatar Nov 01 '24 20:11 kfessel

@kfessel its been 2 weeks :)

Ollrogge avatar Nov 15 '24 15:11 Ollrogge

:heavy_check_mark: all files are reviewed :heavy_check_mark: no opposition in 2 weeks

please squash your commits ( to a level of your liking)

kfessel avatar Nov 18 '24 09:11 kfessel

✔️ all files are reviewed ✔️ no opposition in 2 weeks

please squash your commits ( to a level of your liking)

Done

Ollrogge avatar Nov 19 '24 06:11 Ollrogge

Murdock results

:heavy_check_mark: PASSED

db95ef6f104dcc6a87ba0c7d040e8cd6391708fd sys/fido2: Small fix to dependencies && cleanup

Success Failures Total Runtime
10249 0 10249 19m:27s

Artifacts

riot-ci avatar Nov 20 '24 20:11 riot-ci

if fixup chages are related to a previous commit -- please prepare them to be integrated with these commits (aka fixup)

kfessel avatar Nov 21 '24 11:11 kfessel

tests/sys/fido2_ctap/ > BOARD=native64 make failed

https://ci.riot-os.org/results/da5dcdb3166b445dbec7480ea226323c/output/builds/tests/sys/fido2_ctap/native64:gnu.txt

kfessel avatar Nov 21 '24 14:11 kfessel

This is better than before and successfully went through CI

I assume the author tested it.

Done. Ran all tests and everything still works as expected :)

Ollrogge avatar Nov 27 '24 21:11 Ollrogge

@kfessel thanks for reviewing !

Ollrogge avatar Nov 28 '24 15:11 Ollrogge