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

Martine Lenders notifications at github.com
Fri May 24 17:50:37 CEST 2019


miri64 requested changes on this pull request.

Some initial review.

Regarding naming, what do you think about removing the `credential` in most function names. It seems redundant to me and makes the names somewhat clunky.

> +} ecdsa_public_key_t;
+
+/**
+ * @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. Must be bigger than 0.
+ */
+typedef unsigned credman_tag_t;

Do we really need up to 32-bits to identify a credential? I think `uint16_t` suffices here (and since you nicely abstracted the type we still go up to `uint32_t` in case I was wrong ;-)).

> +              credential->tag, credential->type);
+        return CREDMAN_EXIST;
+    }
+    pos = _find_next_free_pos();
+    if (pos < 0) {
+        DEBUG("credman: no space for new credential\n");
+        return CREDMAN_NO_SPACE;
+    }
+
+    credman_credential_t *c = &credentials[pos];
+    c->tag = credential->tag;
+    c->type = credential->type;
+    /* unions only point to one address at a time,
+     * so it doesn't matter which entry in the union
+     * we copied here, as all of them are the same */
+    c->params.psk = credential->params.psk;

```C
    credentials[pos] = *credential
```

should also work here and might save some ROM (not sure though). It definitely is more future stable, in case `credman_credential_t` is extended.

> +    credman_tag_t tag;          /**< Tag of the credential */
+    union {
+        psk_params_t *psk;      /**< PSK credential parameters */
+        ecdsa_params_t *ecdsa;  /**< ECDSA credential parameters */
+    } params;                   /**< Credential parameters */
+} credman_credential_t;
+
+/**
+ * @brief Return values
+ */
+enum {
+    CREDMAN_OK          = 0,
+    CREDMAN_EXIST       = -1,
+    CREDMAN_NO_SPACE    = -2,
+    CREDMAN_NOT_FOUND   = -3,
+    CREDMAN_ERROR       = -4,

Doc missing.

> +};
+
+/**
+ * @brief Initialize system credentials pool. Called from autoinit.
+ */
+void credman_init(void);
+
+/**
+ * @brief Add a credential to system
+ *
+ * @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 system buffer full
+ * @return CREDMAN_ERROR on other errors

IMHO what you use for `CREDMAN_ERROR` in the implementation are all input-parameter errors, so why not have a `CREDMAN_INVALID` as well to cover this case more clearly?

> +        }
+        break;
+    case CREDMAN_TYPE_ECDSA:
+        if (credential->params.ecdsa == NULL) {
+            DEBUG("credman: no ECDSA credential found\n");
+            return CREDMAN_ERROR;
+        }
+        if ((credential->params.ecdsa->private_key == NULL) ||
+            (credential->params.ecdsa->public_key.x == NULL) ||
+            (credential->params.ecdsa->public_key.y == NULL)) {
+            DEBUG("credman: invalid ECDSA parameters\n");
+            return CREDMAN_ERROR;
+        }
+        break;
+    default:
+        return CREDMAN_ERROR;

Here you could return e.g. `CREDMAN_TYPE_UNKNOWN`.

> +               tag, type);
+        return CREDMAN_NOT_FOUND;
+    }
+    memset(&credentials[pos], 0, sizeof(credman_credential_t));
+    used--;
+    return CREDMAN_OK;
+}
+
+int credman_get_used_count(void)
+{
+    return used;
+}
+
+void credman_init(void)
+{
+    memset(credentials, 0, sizeof(credman_credential_t) * CREDMAN_MAX_CREDENTIALS);

This only should be required for testing. How about calling the function `credman_reset()` instead and only making it available `#ifdef TEST_SUITES`?

> + *
+ * @file
+ * @brief       (D)TLS Credentials management module implementation
+ *
+ * @author  Aiman Ismail <muhammadaimanbin.ismail at haw-hamburg.de>
+ */
+
+#include "net/credman.h"
+
+#include <string.h>
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+static credman_credential_t credentials[CREDMAN_MAX_CREDENTIALS];
+static unsigned used = 0;

This variable and how you access `credentials` makes the library not thread-safe. Either document that or deal with it.

> +}
+
+int credman_get_used_count(void)
+{
+    return used;
+}
+
+void credman_init(void)
+{
+    memset(credentials, 0, sizeof(credman_credential_t) * CREDMAN_MAX_CREDENTIALS);
+    used = 0;
+}
+
+static int _find_credential_pos(credman_tag_t tag, credman_type_t type)
+{
+    for (unsigned i = 0; i < used; i++) {

This does not work.

If one calls e.g.

```C
credman_add_credential(&credential1);
credman_add_credential(&credential2);
credman_add_credential(&credential3);
credman_delete_credential(&credential2);
```

`used` will be 2, so `credential3` will not be reached as its index is 2.


> +        }
+        if ((credential->params.ecdsa->private_key == NULL) ||
+            (credential->params.ecdsa->public_key.x == NULL) ||
+            (credential->params.ecdsa->public_key.y == NULL)) {
+            DEBUG("credman: invalid ECDSA parameters\n");
+            return CREDMAN_ERROR;
+        }
+        break;
+    default:
+        return CREDMAN_ERROR;
+    }
+    pos = _find_credential_pos(credential->tag, credential->type);
+    if (pos >= 0) {
+        DEBUG("credman: credential with tag %d and type %d already exist\n",
+              credential->tag, credential->type);
+        return CREDMAN_EXIST;

What if `credential->params` is identical to `credentials[pos]`

> +    int ret;
+    credman_credential_t out_credential;
+    credman_credential_t exp_credential = {
+        .tag = CREDMAN_TEST_TAG,
+        .type = CREDMAN_TYPE_ECDSA,
+        .params = { .ecdsa = &exp_ecdsa_params }
+    };
+
+    /* get non-existing credential */
+    ret = credman_get_credential(&out_credential, exp_credential.tag,
+                                 exp_credential.type);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_NOT_FOUND, ret);
+
+    ret = credman_add_credential(&exp_credential);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, ret);
+

A test case for 

```C
credman_add_credential(&credential1);
credman_add_credential(&credential2);
credman_add_credential(&credential3);
credman_delete_credential(&credential2);
credman_get_credential(&out_credential, credential3.tag, credential3.type);
```

should exist ;-)

> +    TEST_ASSERT_EQUAL_INT(exp_count, credman_get_used_count());
+
+    /* add a credential */
+    ret = credman_add_credential(&exp_credential);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, ret);
+    TEST_ASSERT_EQUAL_INT(++exp_count, credman_get_used_count());
+
+    /* delete a credential from system buffer */
+    ret = credman_delete_credential(exp_credential.tag, exp_credential.type);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, ret);
+    TEST_ASSERT_EQUAL_INT(--exp_count, credman_get_used_count());
+
+    /* delete a deleted credential */
+    ret = credman_delete_credential(exp_credential.tag, exp_credential.type);
+    TEST_ASSERT_EQUAL_INT(CREDMAN_NOT_FOUND, ret);
+    TEST_ASSERT_EQUAL_INT(exp_count, credman_get_used_count());

A test case for 

```C
credman_add_credential(&credential1);
credman_add_credential(&credential2);
credman_add_credential(&credential3);
credman_delete_credential(&credential1);
credman_delete_credential(&credential2);
credman_delete_credential(&credential3);
```

should exist ;-)

-- 
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-241792706
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190524/29120e59/attachment.html>


More information about the notifications mailing list