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

Martine Lenders notifications at github.com
Wed Jul 17 11:59:26 CEST 2019


miri64 requested changes on this pull request.



> +        }
+    }
+    return -1;
+}
+
+static int _find_next_free_pos(void)
+{
+    for (int i = 0; i < CREDMAN_MAX_CREDENTIALS; i++) {
+        credman_credential_t *c = &credentials[i];
+        if ((c->type == CREDMAN_TYPE_EMPTY) &&
+            (c->tag == CREDMAN_TAG_EMPTY)) {
+            return i;
+        }
+    }
+    return (used == 0) ? 0 : -1;
+}

Can be simplified to

```C
static int _find_next_free_pos(void) {
    int i = _find_credential_pos(CREDMAN_TYPE_EMPTY, CREDMAN_TAG_EMPTY);
    if (i < 0) {
        return (used == 0) ? 0 : -1;
    }
    else {
        return i;
    }
}
```

It also reduces code duplication.

> +        }
+    }
+    return -1;
+}
+
+static int _find_next_free_pos(void)
+{
+    for (int i = 0; i < CREDMAN_MAX_CREDENTIALS; i++) {
+        credman_credential_t *c = &credentials[i];
+        if ((c->type == CREDMAN_TYPE_EMPTY) &&
+            (c->tag == CREDMAN_TAG_EMPTY)) {
+            return i;
+        }
+    }
+    return (used == 0) ? 0 : -1;
+}

Also, the `(used == 0) ? 0 : -1` check is redundant, since `(used == 0)` can only be true if all entries are empty and thus also `credentials[0]`.

> +static int _find_next_free_pos(void)
+{
+    for (int i = 0; i < CREDMAN_MAX_CREDENTIALS; i++) {
+        credman_credential_t *c = &credentials[i];
+        if ((c->type == CREDMAN_TYPE_EMPTY) &&
+            (c->tag == CREDMAN_TAG_EMPTY)) {
+            return i;
+        }
+    }
+    return (used == 0) ? 0 : -1;
+}
+
+#ifdef TEST_SUITES
+void credman_reset(void)
+{
+    memset(credentials, 0,

This function should also be mutex locked.

> +    memcpy(credential, &credentials[pos], sizeof(credman_credential_t));
+    ret = CREDMAN_OK;
+end:
+    mutex_unlock(&_mutex);
+    return ret;
+}
+
+int credman_delete(credman_tag_t tag, credman_type_t type)
+{
+    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);
+        ret = CREDMAN_NOT_FOUND;

IMHO it is okay to return `CREDMAN_OK` in this case

> +    memcpy(credential, &credentials[pos], sizeof(credman_credential_t));
+    ret = CREDMAN_OK;
+end:
+    mutex_unlock(&_mutex);
+    return ret;
+}
+
+int credman_delete(credman_tag_t tag, credman_type_t type)
+{
+    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);
+        ret = CREDMAN_NOT_FOUND;

So the whole function can be simplified to

```C
int credman_delete(credman_tag_t tag, credman_type_t type)
{
    mutex_lock(&_mutex);
    int ret = CREDMAN_ERROR;
    int pos = _find_credential_pos(tag, type);
    if (pos >= 0) {
        memset(&credentials[pos], 0, sizeof(credman_credential_t));
        used--;
    }
    mutex_unlock(&_mutex);
    return CREDMAN_OK;
}

> +    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);
+        ret = CREDMAN_NOT_FOUND;
+        goto end;

Goto can be prevented with else

```C
    /* ... */
    if (pos < 0) {
        DEBUG("credman: credential with tag %d and type %d not found\n",
               tag, type);
        ret = CREDMAN_NOT_FOUND;
    }
    else {
        memcpy(credential, &credentials[pos], sizeof(credman_credential_t));
        ret = CREDMAN_OK;
    }
    mutex_unlock(&_mutex);
    return ret;
```

> +    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 */
+    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.

This is just the type definition, so adding value restrictions to it seems off (also its "[…] greater than 0").

> +};
+
+/**
+ * @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

> This is just the type definition, so adding value restrictions to it seems off (also its "[…] greater than 0").

Rather put the restriction in the (missing) doc for `CREDMAN_INVALID` (e.g. `@return CREDMAN_INVALID when […] or credman_credential_t::tag of @p credential is @ref CREDMAN_TAG_EMPTY`).

> +* 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.
+*/
+
+#include <string.h>
+#include "embUnit.h"
+#include "tests-credman.h"
+#include "credentials.h"
+
+#include "net/credman.h"
+
+#define CREDMAN_TEST_TAG (1)
+
+static int _compare_credentials(const credman_credential_t *a,
+                               const credman_credential_t *b)

Indentation

> +                .client_keys = NULL,
+                .client_keys_size = 0,
+            },
+        },
+    };
+
+    // add credentials
+    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 starting from first added credential
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_delete(tag1, in_credential.type));
+    TEST_ASSERT_EQUAL_INT(CREDMAN_OK, credman_delete(tag2, in_credential.type));
+    TEST_ASSERT_EQUAL_INT(0, credman_get_used_count());

Please also add a test that tries adding stuff after space what made through deletion

> +*/
+void tests_credman(void);
+
+/**
+ * @brief   Generates tests for credman
+ *
+ * @return  embUnit tests if successful, NULL if not.
+ */
+Test *tests_credman_tests(void);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* TESTS_CREDMAN_H */
+/** @} */

Github notes some whitespace error "No newline at end of file" here (also in some of the other files). Please check.

-- 
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-262900764
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190717/3b31e6f8/attachment-0001.html>


More information about the notifications mailing list