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

Peter Kietzmann notifications at github.com
Fri Aug 27 09:48:35 CEST 2021


@PeterKietzmann commented on this pull request.

Please address comments grouped into separate commits 

> +    uint8_t pin_auth[CTAP_PIN_AUTH_SZ];                         /**< first 16 bytes of HMAC-SHA-256 of encrypted contents  */
+    bool pin_auth_present;                                      /**< indicate if pin_auth present */
+    uint8_t new_pin_enc[CTAP_PIN_ENC_MAX_SIZE];                 /**< Encrypted new PIN using sharedSecret. */
+    uint16_t new_pin_enc_size;                                  /**< size of encrypted new pin */
+    uint8_t pin_hash_enc[SHA256_DIGEST_LENGTH / 2];             /**< Encrypted first 16 bytes of SHA-256 of PIN using sharedSecret. */
+    bool pin_hash_enc_present;                                  /**< indicate pin_hash_enc is present */
+} ctap_client_pin_req_t;
+
+/**
+ * @brief CTAP get_assertion state
+ */
+typedef struct {
+    ctap_resident_key_t rks[CTAP_MAX_EXCLUDE_LIST_SIZE];    /**< eligible resident keys found */
+    uint8_t count;                                          /**< number of rks found  */
+    uint8_t cred_counter;                                   /**< amount of creds sent to host */
+    uint32_t timer;                                         /**< time gap between get_next_assertion calls in microseconds  */

The logic is now in milli seconds after the change to ztimer.

> +/**
+ * @brief CTAP resident key struct
+ *
+ * A resident key is a fido2 credential that is being stored on the
+ * authenticator.
+ */
+struct __attribute__((packed)) ctap_resident_key {
+    uint8_t rp_id_hash[SHA256_DIGEST_LENGTH];   /**< hash of rp domain string */
+    uint8_t user_id[CTAP_USER_ID_MAX_SIZE];     /**< id of user */
+    uint8_t user_id_len;                        /**< length of the user id */
+    uint8_t priv_key[CTAP_CRYPTO_KEY_SIZE];     /**< private key */
+    uint32_t sign_count;                        /**< signature counter.
+                                                   See webauthn specification
+                                                   (version 20190304) section 6.1.1
+                                                   for details. */
+    uint64_t creation_time;                     /**< timestamp for when credential

Why is this `uint64_t`?

> +    CTAP2_ERR_PIN_REQUIRED              = 0x36,
+    CTAP2_ERR_PIN_POLICY_VIOLATION      = 0x37,
+    CTAP2_ERR_PIN_TOKEN_EXPIRED         = 0x38,
+    CTAP2_ERR_REQUEST_TOO_LARGE         = 0x39,
+    CTAP2_ERR_ACTION_TIMEOUT            = 0x3A,
+    CTAP2_ERR_UP_REQUIRED               = 0x3B,
+    CTAP1_ERR_OTHER                     = 0x7F,
+    CTAP2_ERR_SPEC_LAST                 = 0xDF,
+    CTAP2_ERR_EXTENSION_FIRST           = 0xE0,
+    CTAP2_ERR_EXTENSION_LAST            = 0xEF,
+    CTAP2_ERR_VENDOR_FIRST              = 0xF0,
+    CTAP2_ERR_VENDOR_LAST               = 0xFF
+} ctap_status_codes_t;
+/** @} */
+
+

Newline introduced by last commit.

> + *
+ * Helper containing functionality to parse and encode CBOR messages. Uses the [tinyCBOR](https://doc.riot-os.org/group__pkg__tinycbor.html) pkg.
+ *
+ * **ctap_crypto**
+ *
+ * Helper containing functionality for cryptographic operations.
+ *
+ * Abstraction for cryptographic operations (SHA256, HMAC-SHA-256, AES CCM).
+ *
+ * @note This abstraction exposes error return values which are currently not implemented in all cases by the RIOT crypto API.
+ *
+ * Abstraction for Elliptic curve cryptography (ECC) operations. Uses the [micro-ecc](https://api.riot-os.org/group__pkg__micro__ecc.html) pkg.
+ *
+ * Parsing of cryptographic signatures into ASN.1 DER format. Uses the [tiny-asn1](http://doc.riot-os.org/group__pkg__tiny-asn1.html) pkg.
+ *
+ * **ctap_mem**

This section needs update after you added mtd.

> +
+config FIDO2_CTAP_UP_TIMEOUT
+    int "Seconds until user presence test times out"
+    default 15
+
+config FIDO2_CTAP_UP_BUTTON_PORT
+    int "Port of user presence button"
+    default 0
+    help
+        Default config is for BTN0_PIN on nrf52840dk
+
+config FIDO2_CTAP_UP_BUTTON_PIN
+    int "Pin of user presence button"
+    default 11
+    help
+        Default config is for BTN0_PIN on nrf52840dk

This is the wrong place for board specific information.

> +        a new credential pair or authenticating.
+
+config FIDO2_CTAP_DISABLE_LED
+    bool "Disable LED animations"
+    help
+        When set, the authenticator will not use LED's.
+
+config FIDO2_CTAP_UP_TIMEOUT
+    int "Seconds until user presence test times out"
+    default 15
+
+config FIDO2_CTAP_UP_BUTTON_PORT
+    int "Port of user presence button"
+    default 0
+    help
+        Default config is for BTN0_PIN on nrf52840dk

This is the wrong place for board specific information.

> +config FIDO2_CTAP_UP_BUTTON_MODE_IN
+    bool "GPIO_IN"
+
+endchoice
+
+choice
+    bool "User presence button pin flank"
+    default FIDO2_CTAP_UP_BUTTON_FLANK_FALLING
+
+config FIDO2_CTAP_UP_BUTTON_FLANK_FALLING
+    bool "GPIO_FALLING"
+
+config FIDO2_CTAP_UP_BUTTON_FLANK_RISING
+    bool "GPIO_RISING"
+
+config FIDO2_CTAP_UP_BUTTON_FLANK_BOTH

Does this option make sense at all?

> +
+/**
+ * @brief CTAP user presence button
+ */
+#if defined(CONFIG_FIDO2_CTAP_UP_BUTTON_PORT) && defined(CONFIG_FIDO2_CTAP_UP_BUTTON_PIN)
+#define CTAP_UP_BUTTON GPIO_PIN(CONFIG_FIDO2_CTAP_UP_BUTTON_PORT, CONFIG_FIDO2_CTAP_UP_BUTTON_PIN)
+#else
+#ifdef BTN0_PIN
+#define CTAP_UP_BUTTON BTN0_PIN
+#endif
+#endif
+
+/**
+ * @brief CTAP user presence button mode
+ */
+#if IS_ACTIVE(FIDO2_CTAP_UP_BUTTON_MODE_IN_PU)

Missing `CONFIG_` prefix?

-- 
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#pullrequestreview-740192251
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210827/7b4dc863/attachment-0001.htm>


More information about the notifications mailing list