[riot-notifications] [RIOT-OS/RIOT] sys: add credman (D)TLS credential management module (#11564)

Martine Lenders notifications at github.com
Thu Jul 18 13:49:47 CEST 2019


miri64 requested changes on this pull request.

My doc and style review

> + * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup    net_credman (D)TLS Credential Manager
+ * @ingroup     net
+ * @brief       Credentials management module for (D)TLS
+ *
+ * @{
+ *
+ * @file
+ * @brief       (D)TLS credentials management module definitions
+ *
+ * @note        This module DOES NOT copies the credentials into the system. It

Grammar
```suggestion
 * @note        This module DOES NOT copy the credentials into the system. It
```

> +extern "C" {
+#endif
+
+/**
+ * @brief Maximum number of credentials in credential pool
+ */
+#ifndef CREDMAN_MAX_CREDENTIALS
+#define CREDMAN_MAX_CREDENTIALS  (2)
+#endif
+
+/**
+ * @brief Buffer of the credential
+ */
+typedef struct {
+    void *s;                /**< Pointer to the buffer */
+    size_t len;             /**< Length of s */

```suggestion
    size_t len;             /**< Length of credman_buffer_t::s */
```

This way it can be parsed properly by `doxygen`

> +#endif
+
+/**
+ * @brief Buffer of the credential
+ */
+typedef struct {
+    void *s;                /**< Pointer to the buffer */
+    size_t len;             /**< Length of s */
+} credman_buffer_t;
+
+/**
+ * @brief PSK parameters
+ */
+typedef struct {
+    credman_buffer_t key;   /**< Key buffer */
+    credman_buffer_t id;    /**< Id buffer */

```suggestion
    credman_buffer_t id;    /**< ID buffer */
```

> +    credman_buffer_t hint;  /**< Hint buffer */
+} psk_params_t;
+
+/**
+ * @brief ECDSA public keys
+ */
+typedef struct {
+    const void *x;          /**< X part of the public key */
+    const void *y;          /**< Y part of the public key */
+} ecdsa_public_key_t;
+
+/**
+ * @brief ECDSA parameters
+ */
+typedef struct {
+    const void * private_key;           /**< Pointer to the private key */

```suggestion
    const void *private_key;            /**< Pointer to the private key */
```

> +    pos = _find_credential_pos(CREDMAN_TAG_EMPTY, CREDMAN_TYPE_EMPTY);
+    if (pos < 0) {
+        DEBUG("credman: no space for new credential\n");
+        ret = CREDMAN_NO_SPACE;
+        goto end;
+    }
+
+    memcpy(&credentials[pos], credential, sizeof(*credential));
+    used++;
+    ret = CREDMAN_OK;
+end:
+    mutex_unlock(&_mutex);
+    return ret;
+}
+
+int credman_get(credman_credential_t *credential,credman_tag_t tag,

```suggestion
int credman_get(credman_credential_t *credential, credman_tag_t tag,
```

> +end:
+    mutex_unlock(&_mutex);
+    return ret;
+}
+
+int credman_get(credman_credential_t *credential,credman_tag_t tag,
+                credman_type_t type)
+{
+    assert(credential);
+    mutex_lock(&_mutex);
+    int ret = CREDMAN_ERROR;
+
+    int pos = _find_credential_pos(tag, type);
+    if (pos < 0) {
+        DEBUG("credman: credential with tag %d and type %d not found\n",
+               tag, type);

```suggestion
              tag, type);
```

> @@ -0,0 +1,254 @@
+/*
+* Copyright (C) 2019 HAW Hamburg
+*
+* This file is subject to the terms and conditions of the GNU Lesser
+* General Public License v2.1. See the file LICENSE in the top level
+* directory for more details.
+*/

Indentation of comment of (` ` missing at beginning of `*` lines).

> +    credman_credential_t in_credential = {
+        .tag = CREDMAN_TEST_TAG,
+        .type = CREDMAN_TYPE_ECDSA,
+        .params = {
+            .ecdsa = {
+                .private_key = ecdsa_priv_key,
+                .public_key = { .x = ecdsa_pub_key_x, .y = ecdsa_pub_key_y },
+                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+
+    /* get non-existing credential */
+    ret = credman_get(&out_credential, in_credential.tag,
+                                 in_credential.type);

```suggestion
                      in_credential.type);
```

> +                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+
+    /* get non-existing credential */
+    ret = credman_get(&out_credential, in_credential.tag,
+                                 in_credential.type);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_NOT_FOUND, ret);
+
+    ret = credman_add(&in_credential);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, ret);
+
+    ret = credman_get(&out_credential, in_credential.tag,
+                                 in_credential.type);

Dito

> + * @brief       Unittests for the ``credman`` module
+ *
+ * @author      Aiman Ismail <muhammadaimanbin.ismail at haw-hamburg.de>
+ */
+
+#ifndef TESTS_CREDMAN_H
+#define TESTS_CREDMAN_H
+#include "embUnit/embUnit.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+*  @brief   The entry point of this test suite.
+*/

` ` missing at beginning of `*` lines

> + * @brief ECDSA parameters
+ */
+typedef struct {
+    const void * private_key;           /**< Pointer to the private key */
+    ecdsa_public_key_t public_key;      /**< Public key */
+    ecdsa_public_key_t *client_keys;    /**< Array of clients public keys */
+    size_t client_keys_size;            /**< Size of clients_keys */
+} ecdsa_params_t;
+
+/**
+ * @brief Tag of the credential.
+ */
+typedef uint16_t credman_tag_t;
+
+/**
+ * @brief Used to identify credentials for application libraries.

I'm not really sure what this is supposed to mean. Maybe give an example in the details?

> +/**
+ * @brief Used to signal empty/no tag
+ */
+#define CREDMAN_TAG_EMPTY   (0)
+
+/**
+ * @brief Credential types
+ */
+typedef enum {
+    CREDMAN_TYPE_EMPTY  = 0,
+    CREDMAN_TYPE_PSK    = 1,
+    CREDMAN_TYPE_ECDSA  = 2,
+} credman_type_t;
+
+/**
+ * @brief Credential informations

```suggestion
 * @brief Credential information
```

(the plural of "information" is "information")

> +
+/**
+ * @brief Adds a credential to the credential pool
+ *
+ * @param[in] credential    Credential to add.
+ *
+ * @return CREDMAN_OK on success
+ * @return CREDMAN_EXIST if credential of @p tag and @p type already exist
+ * @return CREDMAN_NO_SPACE if credential pool is full
+ * @return CREDMAN_TYPE_UNKNOWN if @p credential has unknown
+ *         credman_credential_t::type
+ * @return CREDMAN_INVALID if @p credential has
+ *         credman_credential_t::tag with the value of CREDMAN_TAG_EMPTY OR
+ *         credman_credential_t::type with the value of CREDMAN_TYPE_EMPTY OR
+ *         credman_credential_t::params with invalid credential parameters i.e.
+ *         the key points to NULL or has a length of 0.

Instead of `OR` after every line I think it is more readable (especially in rendered doc) when every or-case is documented in its own `@return CREDMAN_INVALID`.

> +/**
+ * @brief Gets a credential from credential pool
+ *
+ * @param[out] credential   Found credential
+ * @param[in] tag           Tag of credential to get
+ * @param[in] type          Type of credential to get
+ *
+ * @return CREDMAN_OK on success
+ * @return CREDMAN_NOT_FOUND if no credential with @p tag and @p type found
+ * @return CREDMAN_ERROR on other errors
+ */
+int credman_get(credman_credential_t *credential, credman_tag_t tag,
+                credman_type_t type);
+
+/**
+ * @brief Delete a credential from the system. Does nothing if

```suggestion
 * @brief Delete a credential from credential pool. Does nothing if
```

At least if I interpret your comment in l.19 correctly ;-).

> @@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2018 Inria
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     tests
+ * @{
+ *
+ * @file
+ * @brief       credman test application (PSK and ECC keys)

This file does not contain an application, just the `credential` (just move the line below here).

> +    /* add one credential */
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_add(&credential));
+    TEST_ASSERT_EQUAL_INT(++exp_count, credman_get_used_count());
+
+    /* add duplicate credential */
+    ret = credman_add(&credential);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_EXIST, ret);
+    TEST_ASSERT_EQUAL_INT(exp_count, credman_get_used_count());
+
+    /* add invalid credential params */
+    memset(&credential.params.psk, 0, sizeof(psk_params_t));
+    ret = credman_add(&credential);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_INVALID, ret);
+    TEST_ASSERT_EQUAL_INT(exp_count, credman_get_used_count());
+
+    /* fill the system credential buffer */

~~system~~?

> +    /* add invalid credential params */
+    memset(&credential.params.psk, 0, sizeof(psk_params_t));
+    ret = credman_add(&credential);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_INVALID, ret);
+    TEST_ASSERT_EQUAL_INT(exp_count, credman_get_used_count());
+
+    /* fill the system credential buffer */
+    memcpy(&credential.params.psk, &exp_psk_params, sizeof(psk_params_t));
+    while (credman_get_used_count() < CREDMAN_MAX_CREDENTIALS) {
+        /* increase tag number so that it is not recognized as duplicate */
+        credential.tag++;
+        TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_add(&credential));
+        TEST_ASSERT_EQUAL_INT(++exp_count, credman_get_used_count());
+    }
+
+    /* add to full system credential buffer */

~~system~~?

> +                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+
+    /* delete non-existing credential */
+    credman_delete(in_credential.tag, in_credential.type);
+    TEST_ASSERT_EQUAL_INT(exp_count, credman_get_used_count());
+
+    /* add a credential */
+    ret = credman_add(&in_credential);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, ret);
+    TEST_ASSERT_EQUAL_INT(++exp_count, credman_get_used_count());
+
+    /* delete a credential from system buffer */

~~system~~?

> +    credman_credential_t out_credential;
+    credman_credential_t in_credential = {
+        .tag = tag1,
+        .type = CREDMAN_TYPE_ECDSA,
+        .params = {
+            .ecdsa = {
+                .private_key = ecdsa_priv_key,
+                .public_key = { .x = ecdsa_pub_key_x, .y = ecdsa_pub_key_y },
+                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+    TEST_ASSERT_EQUAL_INT(0, credman_get_used_count());
+
+    // fill the system pool, assume CREDMAN_MAX_CREDENTIALS is 2

Please use only C-style `/* ... */` comments.

> +                .private_key = ecdsa_priv_key,
+                .public_key = { .x = ecdsa_pub_key_x, .y = ecdsa_pub_key_y },
+                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+    TEST_ASSERT_EQUAL_INT(0, credman_get_used_count());
+
+    // fill the system pool, assume CREDMAN_MAX_CREDENTIALS is 2
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_add(&in_credential));
+    in_credential.tag = tag2;
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_add(&in_credential));
+    TEST_ASSERT_EQUAL_INT(2, credman_get_used_count());
+
+    // delete the first credential

Dito

> +            },
+        },
+    };
+    TEST_ASSERT_EQUAL_INT(0, credman_get_used_count());
+
+    // fill the system pool, assume CREDMAN_MAX_CREDENTIALS is 2
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_add(&in_credential));
+    in_credential.tag = tag2;
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_add(&in_credential));
+    TEST_ASSERT_EQUAL_INT(2, credman_get_used_count());
+
+    // delete the first credential
+    credman_delete(tag1, in_credential.type);
+    TEST_ASSERT_EQUAL_INT(1, credman_get_used_count());
+
+    // get the second credential

Dito

> +    credman_tag_t tag2 = CREDMAN_TEST_TAG + 1;
+
+    credman_credential_t in_credential = {
+        .tag = tag1,
+        .type = CREDMAN_TYPE_ECDSA,
+        .params = {
+            .ecdsa = {
+                .private_key = ecdsa_priv_key,
+                .public_key = { .x = ecdsa_pub_key_x, .y = ecdsa_pub_key_y },
+                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+
+    // add credentials

Here as well, not pointing further occurences

> +    credman_credential_t out_credential;
+    credman_credential_t in_credential = {
+        .tag = tag1,
+        .type = CREDMAN_TYPE_ECDSA,
+        .params = {
+            .ecdsa = {
+                .private_key = ecdsa_priv_key,
+                .public_key = { .x = ecdsa_pub_key_x, .y = ecdsa_pub_key_y },
+                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+    TEST_ASSERT_EQUAL_INT(0, credman_get_used_count());
+
+    // fill the system pool, assume CREDMAN_MAX_CREDENTIALS is 2

Also ~~system~~?

-- 
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/11564#pullrequestreview-263589304
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190718/667e30b2/attachment-0001.htm>


More information about the notifications mailing list