[riot-notifications] [RIOT-OS/RIOT] FIDO2 support in RIOT (#16489)

Peter Kietzmann notifications at github.com
Thu Jul 22 09:37:47 CEST 2021


@PeterKietzmann commented on this pull request.



> +#include "uECC.h"
+#include "tiny-asn1.h"
+
+#include "fido2/ctap/ctap_crypto.h"
+#include "fido2/ctap/ctap_status.h"
+#include "fido2/ctap/ctap_utils.h"
+
+static ctap_crypto_key_agreement_key_t g_ag_key;
+static int sig_to_der_format(uint8_t *r, uint8_t *s, uint8_t *sig,
+                             size_t *sig_len);
+static int RNG(uint8_t *dest, size_t size);
+
+int fido2_ctap_crypto_init(void)
+{
+
+    uECC_set_rng(&RNG);

Then why `static int RNG(uint8_t *dest, size_t size)` in line 39?
(using `size_t` instead of `unsigned int` as in the copied function signature?)

However, I am fine with the additional abstraction and was just wondering about the amount of indirection without any changing prototypes.

> +#include "debug.h"
+
+static bool g_user_present = false;
+
+#if !IS_ACTIVE(CONFIG_FIDO2_CTAP_DISABLE_UP)
+static void gpio_cb(void *arg);
+
+int fido2_ctap_utils_user_presence_test(void)
+{
+#ifdef BTN0_PIN
+    int ret;
+#ifdef MODULE_FIDO2_CTAP_HID
+    fido2_ctap_transport_hid_send_keepalive(CTAP_HID_STATUS_UPNEEDED);
+#endif
+
+    if (gpio_init_int(BTN0_PIN, BTN0_MODE, GPIO_FALLING, gpio_cb, NULL) < 0) {

Then remove it. I'll mark comment as resolved.

> +#ifdef BTN0_PIN
+    int ret;
+#ifdef MODULE_FIDO2_CTAP_HID
+    fido2_ctap_transport_hid_send_keepalive(CTAP_HID_STATUS_UPNEEDED);
+#endif
+
+    if (gpio_init_int(BTN0_PIN, BTN0_MODE, GPIO_FALLING, gpio_cb, NULL) < 0) {
+        return CTAP1_ERR_OTHER;
+    }
+
+#if !IS_ACTIVE(CONFIG_FIDO2_CTAP_DISABLE_LED)
+    fido2_ctap_utils_led_animation();
+#endif
+
+    ret = g_user_present ? CTAP2_OK : CTAP2_ERR_ACTION_TIMEOUT;
+    gpio_irq_disable(BTN0_PIN);

Ok, then I would prefer to call `gpio_init_int` in some initialization function, and use `gpio_irq_disable` and `gpio_irq_enable` to de-/activate the pin.

> +        delay /= 2;
+    }
+#endif /* CONFIG_FIDO2_CTAP_DISABLE_LED  */
+
+    ctap_hid_write(cmd, cid, NULL, 0);
+}
+
+void fido2_ctap_transport_hid_send_keepalive(uint8_t status)
+{
+    ctap_hid_write(CTAP_HID_COMMAND_KEEPALIVE, g_ctap_buffer.cid, &status,
+                   sizeof(status));
+}
+
+static void send_error_response(uint32_t cid, uint8_t err)
+{
+    DEBUG("ctap_trans_hid err resp: %02x \n", err);

I usually expect `DEBUG` to print the function name first. Here, the **file** name is printed. What I've missed yesterday is the fact this function is called throughout the whole file. Fine with me, leave it this way.

> + */
+
+/**
+ * @defgroup    fido2_ctap_transport_hid CTAP USB_HID transport binding - CTAPHID
+ * @ingroup     fido2_ctap_transport
+ * @brief       CTAP_HID defines and functions
+ *
+ * @{
+ *
+ * @file
+ * @brief       Definition for CTAPHID helper functions
+ *
+ * @author      Nils Ollrogge <nils-ollrogge at outlook.de>
+ */
+
+#ifndef FIDO2_CTAP_TRANSPORT_HID_CTAP_TRANSPORT_HID_H

Sure. How about renaming ctap_transport_hid.h -> ctap_hid.h? At least a little something.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16489#discussion_r674563291
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210722/c6e679e2/attachment.htm>


More information about the notifications mailing list