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

Juan I Carrano notifications at github.com
Tue May 28 18:29:08 CEST 2019


jcarrano commented on this pull request.

If I get this right, this is a sort of in-memory index to user supplied buffers. That is, it creates a mapping of (tag, type) -> (buffer with key) and except the data definitions, nothing seems to be specific to credentials.

I think this may be over-specialized. If the user must remember a (32 bit) numerical tag _and_ this does not copy the buffers, how is this different from just remembering a pointer to the buffer.

Also, why mix the storage and validation logic. If what is needed is an int->pointer mapping (thread safe and all) why not make that a module and implement the validation and retrieval on top of it.

The design of this does not allow for persisting the credentials in any way.

If credentials are known at compile time, why not use constfs. And if they are dynamically added in run-time, where will they be allocated?

> +} 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;

Beware, that will only save memory if `-fshort-enum` is on (because of credman_type_t).

> +    }
+    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);
+        ret = CREDMAN_EXIST;
+        goto end;
+    }
+    pos = _find_next_free_pos();
+    if (pos < 0) {
+        DEBUG("credman: no space for new credential\n");
+        ret = CREDMAN_NO_SPACE;
+        goto end;
+    }
+
+    credentials[pos] = *credential;

In the description you say 

> It just holds the pointers to the credentials given by the user.

but here the value is being copied. So that the user must keep is the "*_params_t" structure _and_ the buffers pointed to by that structure.

-- 
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-242772964
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190528/6d7329cc/attachment.html>


More information about the notifications mailing list