Fido2 follow up continued #18663

Ollrogge · 2022-09-28T20:09:14Z

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

chrysn

I'm splitting review into multiple stages as that's a relatively large number of files for a subsystem with few major contributors. For this round, I'm only looking at changes visible outside the module, including the removed ztimer64 use.

Minor comments inline; one larger here:

There are several incompatible changes to the public fido2 API. Please prepare, ideally in the top post of the PR, a note that can then be placed in the release notes. It should describe to users of the API what they need to change in their applications, with emphasis on changes that might go undetected by the compiler and alter the program's meaning (such as when fido2_ctap_handle_request returns a status code rather than a length, which the compiler may cast implicitly).

sys/Makefile.dep

sys/include/fido2/ctap.h

sys/include/fido2/ctap/ctap_utils.h

tests/sys_fido2_ctap/Makefile

tests/sys_fido2_ctap/main.c

Ollrogge · 2023-03-27T10:41:39Z

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

Ollrogge · 2023-05-08T13:09:17Z

@chrysn Can we get this in ?

kfessel · 2023-05-25T10:22:41Z

link to #19559 native is using file backed mtd

kfessel · 2024-09-30T10:17:21Z

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

Ollrogge · 2024-09-30T10:48:36Z

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.

kfessel · 2024-10-07T10:39:05Z

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

kfessel · 2024-10-07T10:43:39Z

sys/fido2/ctap/ctap_mem.c

@@ -28,16 +28,22 @@
 #define ENABLE_DEBUG    (0)
 #include "debug.h"

+#ifdef BOARD_NATIVE
+#include "mtd_default.h"
+// native mtd is file backed => Start address of flash is 0.


style : single line comment is too modern for RIOT

content: is that always the case
(e.g. if something else is build into the same application and is using the mtd)
or is there always a dedicated file for fido?

having a specific mtd configuration seems application specific -> ?better in test app?

This line is only needed when running the fido2 layer natively (for testing). In this case I set the mtd_default because that is the only one that works correctly natively if I remember correctly.

Ollrogge · 2024-10-25T12:34:30Z

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

kfessel · 2024-11-01T20:26:12Z

sys/fido2/ctap/ctap_crypto.c

@@ -47,78 +46,80 @@ static int _sig_to_der_format(uint8_t *r, uint8_t *s, uint8_t *sig,
 */
 static int _RNG(uint8_t *dest, unsigned size);


that line is no longer needed ( since you moved the function that used that declaration)

kfessel · 2024-11-01T20:42:26Z

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

Ollrogge requested review from leandrolanzieri, jia200x, MrKevinWeiss, miri64, smlng, aabadie, MichelRottleuthner and fjmolinas as code owners September 28, 2022 20:09

github-actions bot added Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework labels Sep 28, 2022

benpicco requested review from chrysn and PeterKietzmann September 29, 2022 15:43

chrysn reviewed Oct 3, 2022

View reviewed changes

Ollrogge force-pushed the fido2_follow_up2 branch 4 times, most recently from 96cb0a4 to 9f341f9 Compare October 16, 2022 18:18

Ollrogge force-pushed the fido2_follow_up2 branch from 9f341f9 to a823a46 Compare March 27, 2023 10:31

Ollrogge force-pushed the fido2_follow_up2 branch from 73e1053 to 1b20cb7 Compare May 25, 2023 09:48

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: native Platform: This PR/issue effects the native platform and removed Area: doc Area: Documentation labels May 25, 2023

github-actions bot added the Area: doc Area: Documentation label May 25, 2023

Ollrogge force-pushed the fido2_follow_up2 branch 2 times, most recently from 68b8994 to 4e1b12d Compare May 26, 2023 07:57

Ollrogge added 3 commits September 27, 2024 07:31

fido2/ctap_hid: remove dependency on ztimer64

efcbc1e

fido2: Change public api && return ctap_status_code_t instead of int

9c6051b

fido2/ctap: Adjust ctap_mem code to work on native

18826d8

Ollrogge force-pushed the fido2_follow_up2 branch from 4e1b12d to c2e5d68 Compare September 28, 2024 14:30

github-actions bot removed the Area: Kconfig Area: Kconfig integration label Sep 28, 2024

kfessel added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Sep 30, 2024

Ollrogge added 2 commits October 1, 2024 22:00

fido2/ctap: add test for ctap without transport layer

504667c

sys/fido2: Small fix to dependencies

c11569c

Ollrogge force-pushed the fido2_follow_up2 branch from c2e5d68 to c11569c Compare October 1, 2024 20:17

kfessel reviewed Oct 7, 2024

View reviewed changes

sys/fido2: cleanup

7653a63

kfessel reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fido2 follow up continued #18663

Fido2 follow up continued #18663

Ollrogge commented Sep 28, 2022 •

edited

Loading

chrysn left a comment

Ollrogge commented Mar 27, 2023

Ollrogge commented May 8, 2023

kfessel commented May 25, 2023 •

edited

Loading

kfessel commented Sep 30, 2024

Ollrogge commented Sep 30, 2024

kfessel commented Oct 7, 2024

kfessel Oct 7, 2024 •

edited

Loading

kfessel Oct 7, 2024 •

edited

Loading

Ollrogge Oct 25, 2024

Ollrogge commented Oct 25, 2024

kfessel Nov 1, 2024

kfessel commented Nov 1, 2024

		@@ -47,78 +46,80 @@ static int _sig_to_der_format(uint8_t r, uint8_t s, uint8_t *sig,
		*/
		static int _RNG(uint8_t *dest, unsigned size);

Fido2 follow up continued #18663

Are you sure you want to change the base?

Fido2 follow up continued #18663

Conversation

Ollrogge commented Sep 28, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

chrysn left a comment

Choose a reason for hiding this comment

Ollrogge commented Mar 27, 2023

Ollrogge commented May 8, 2023

kfessel commented May 25, 2023 • edited Loading

kfessel commented Sep 30, 2024

Ollrogge commented Sep 30, 2024

kfessel commented Oct 7, 2024

kfessel Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

kfessel Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Ollrogge Oct 25, 2024

Choose a reason for hiding this comment

Ollrogge commented Oct 25, 2024

kfessel Nov 1, 2024

Choose a reason for hiding this comment

kfessel commented Nov 1, 2024

Ollrogge commented Sep 28, 2022 •

edited

Loading

kfessel commented May 25, 2023 •

edited

Loading

kfessel Oct 7, 2024 •

edited

Loading

kfessel Oct 7, 2024 •

edited

Loading