-
Notifications
You must be signed in to change notification settings - Fork 2k
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
FIDO2 support in RIOT #16489
FIDO2 support in RIOT #16489
Conversation
thanks @Ollrogge for this nice contribution! |
My implementation doesn't support all the features yet that the solokeys testcases test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First set of questions and comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
second intermediate blob of comments and questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
third blob of comments and questions
Initial testing:
this output
|
I only ever got this error when I didn't wait long enough before pressing enter after a device reboot. The connection between authenticator and host has not been reestablished yet, therefore no device can be found. Could you run the tests again and wait a little longer after a reboot ? |
Okay, much better! I waited ~10 seconds now after each reboot. One failure remaining, can you check: this output
|
I also get this error. This one originates in the USB layer I think. I have not yet been able to fix this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ollrogge thanks for addressing my comments! Here are some responses to the open discussions. Next, I will run the tests again, and start a fresh review round
/** | ||
* @brief MakeCredential method | ||
* | ||
* CTAP specification (version 20190130) section 5.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are (still) multiple occurrences of "CTAP2" in your code (for example in line 35 of this file). Please correct them. I wanted to say that you should also check new files&folders for usage of "CTAP2" instead of "CTAP"
Short notice: I've tested again with the nrf52840dk.
this output
|
Will do today :) In my opinion the second solution you proposed is the best ? |
I would like to merge this PR soon, knowing that there is follow-up work. What is the state of this discussion? @benpicco, do you see your other comments addressed? @Ollrogge I think you can squash already your latest commits. |
Tested kconfig with nrf52840dk with this change applied to the test file: diff --git a/tests/sys_fido2_ctap/main.c b/tests/sys_fido2_ctap/main.c
index 31ffbc44ef..ac5f5b24da 100644
--- a/tests/sys_fido2_ctap/main.c
+++ b/tests/sys_fido2_ctap/main.c
@@ -25,12 +25,35 @@
#include "xtimer.h"
-#include "fido2/ctap.h"
+#include "fido2/ctap/ctap.h"
+#include "fido2/ctap/ctap_mem.h"
+#include "fido2/ctap/transport/hid/ctap_hid.h"
#include "fido2/ctap/transport/ctap_transport.h"
int main(void)
{
/* sleep in order to see early DEBUG outputs */
xtimer_sleep(3);
+ printf("CTAP_STACKSIZE: %i\n", CTAP_STACKSIZE);
+ printf("CTAP_AAGUID: %s\n", CTAP_AAGUID);
+#if IS_ACTIVE(CONFIG_FIDO2_CTAP_DISABLE_UP)
+ printf("CONFIG_FIDO2_CTAP_DISABLE_UP: %i\n", CONFIG_FIDO2_CTAP_DISABLE_UP);
+#endif
+#if IS_ACTIVE(CONFIG_FIDO2_CTAP_DISABLE_LED)
+ printf("CONFIG_FIDO2_CTAP_DISABLE_LED: %i\n", CONFIG_FIDO2_CTAP_DISABLE_LED);
+#endif
+ printf("CTAP_UP_TIMEOUT: %lu\n", CTAP_UP_TIMEOUT);
+#if IS_ACTIVE(CONFIG_FIDO2_CTAP_UP_BUTTON_PORT)
+ printf("CONFIG_FIDO2_CTAP_UP_BUTTON_PORT: %i\n", CONFIG_FIDO2_CTAP_UP_BUTTON_PORT);
+#endif
+#if IS_ACTIVE(CONFIG_FIDO2_CTAP_UP_BUTTON_PIN)
+ printf("CONFIG_FIDO2_CTAP_UP_BUTTON_PIN: %i\n", CONFIG_FIDO2_CTAP_UP_BUTTON_PIN);
+#endif
+ printf("CTAP_UP_BUTTON: %i\n", CTAP_UP_BUTTON);
+ printf("CTAP_UP_BUTTON_MODE: %i\n", CTAP_UP_BUTTON_MODE);
+ printf("CTAP_UP_BUTTON_FLANK: %i\n", CTAP_UP_BUTTON_FLANK);
+ printf("CTAP_FLASH_START_PAGE: %i\n", CTAP_FLASH_START_PAGE);
+ printf("CTAP_HID_TRANSACTION_TIMEOUT: %lu\n", CTAP_HID_TRANSACTION_TIMEOUT);
+
fido2_ctap_transport_init();
}
Before config change
(had to fix this to make compile)
|
For the HID part
|
nrf52840dk unit test with disabled user presence ( output
nrf52840dk unit test with enabled user presence ( output
--> pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a note somewhere that this can lead to surprise firmware corruption if FLASHPAGE_SIZE is not right.
Where did you address this?
ACK from my side. Please address the latest documentation fixes (in a single commit). You can squash the rest already |
OK, please squash |
I executed again some functional tests on webauthn.io and the solokeys tests (without user presence) after the squash -> everything still works well. My ACK holds if Murdock agrees. |
So many green lights, finally! Lets do this, ACK and go |
Contribution description
This PR adds support for the Fast Identity Online 2 (FIDO2) specification in RIOT. FIDO2 is an authentication standard that seeks to solve the password problem by enabling passwordless authentication. FIDO2 consists of the W3C Web Authentication specification (WebAuthn) and the Client to Authenticator Protocol (CTAP).
This PR adds a basic implementation of the CTAP protocol. CTAP is an application layer protocol for the communication between an authenticator and host. Most of the time an authenticator is either a mobile device or security key like YubiKey.
As of now not many websites support the passwordless login flow added by the second version of the FIDO standard. Therefore future PR's will add the backward compatibility to FIDO1 in order to enable the usage of this implementation as part of 2FA authentication flows.
Testing procedure
tests/sys_fido2_ctap
Issues/PRs references