Refactor authcrypt example #95

andresag01 · 2017-06-13T17:04:05Z

The example is refactored to use C++ classes and allocate the bulk of
the mbed TLS context structures in the heap instead of stack.

This change should fix #91.

RonEld

Since it seems that all you did is move the code to C++ style, and put the object in the heap, I have more of C++ comments, in addition to what I already wrote.
Since this is a C++ Class now, I would split the code to authcrypt.h , which defines the class, authcrypt.cpp which implements the class (except specific cases where you want iniline implementation) , and main.cpp which allocates and uses the object.

RonEld · 2017-06-14T09:56:23Z

authcrypt/main.cpp

+
+class Authcrypt {
+public:
+    Authcrypt() : ciphertext(), decrypted()


Since the default constructors are being called, no real need to implicitly call them.
I would add in the CTOR body also:
memset(ciphertext,0,sizeof(ciphertext))
memset(decrypted,0,sizeof(decrypted))

RonEld · 2017-06-14T09:58:31Z

authcrypt/main.cpp

-    if (ret != 0) {
-        mbedtls_printf("mbedtls_cipher_setup() returned -0x%04X\r\n", -ret);
-        return 1;
+        mbedtls_entropy_free(&entropy);


in general practice, it is better to deallocate\terminate resources in opposite sequence as theyu were initiated.
I would change the sequence to:
mbedtls_cipher_free
mbedtls_ctr_drbg_free
mbedtls_entropy_free

RonEld · 2017-06-14T10:00:48Z

authcrypt/main.cpp

-    int ret = example();
-    if (ret != 0) {
+    int ret;
+    Authcrypt *authcrypt = new Authcrypt();


check if the heap is contigous. If not, the ciphertext and decrypted will not be properly read by HW acceleration engines

andresag01 · 2017-06-14T10:38:02Z

@RonEld: I have fixed some of the issues you pointed out. However, I do not think that we should split the source file into .h and .c as this is a fairly simple class.

RonEld · 2017-06-14T11:00:43Z

@andresag01 thanks for addressing my comments
I think that although it is a simple class, it is not so readable, now that the implementation and definition are together.
In addition, without the split, there is no data abstraction and encapsulation here

mazimkhan · 2017-06-15T09:33:52Z

On splitting the code I agree with Ron. When we look at C++ code we want to see two things

How the class is defined i.e. its data, methods, permissions etc.
Method implementation
With everything together it is hard to view the class itself.

mazimkhan · 2017-06-15T09:14:44Z

authcrypt/main.cpp

+        if (ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED) {
+            mbedtls_printf("Something bad is happening! Data is not "
+                           "authentic!\r\n");
+            return 1;


Why are we changing return from ret to 0? At https://github.com/ARMmbed/mbed-os-example-tls/pull/95/files#diff-64da713f1246019fb29c1e604285e6e0R211 the return code is printed again and will always have 1.
Applies to all places returning 1.

Ahh thanks for spotting that. This is because I did not modify the original code, just changed the memory allocation. I will change this as well.

mazimkhan · 2017-06-15T09:18:19Z

authcrypt/main.cpp

+/* Message that should be protected */
+static const char message[] = "Some things are better left unread";
+/* Metadata transmitted in the clear but authenticated */
+static const char metadata[] = "eg sequence number, routing info";


Any particular reason for leaving message, metadata and secret_key outside the class. If this data belongs to this class then it should be encapsulated inside this class.

mazimkhan · 2017-06-15T09:25:48Z

authcrypt/main.cpp

-    int ret = example();
-    if (ret != 0) {
+    int ret;
+    Authcrypt *authcrypt = new Authcrypt();


new can fail to allocate the memory. We can assert or simply print error if we can run the example.

andresag01 · 2017-06-15T12:30:58Z

@mazimkhan, @RonEld: I have modified the example according to your comments. Could you please take a look again?

mazimkhan · 2017-06-15T13:21:03Z

Can you please also put a NULL check here b3cebca#diff-64da713f1246019fb29c1e604285e6e0R28

andresag01 · 2017-06-15T13:25:46Z

@mazimkhan: I looked into this at some point, but I do not think new returns null: http://www.cplusplus.com/reference/new/operator%20new/. I believe the normal behaviour is to throw an exception if allocation is not possible. However, exceptions are disabled in mbed OS. An alternative could be to use nothrow, but that is not enabled in mbed OS either...

RonEld

I added a comment, and still needs verifying whether the heap is contigous

RonEld · 2017-06-15T13:22:43Z

authcrypt/authcrypt.cpp

+
+const char Authcrypt::message[] = "Some things are better left unread";
+
+const char Authcrypt::metadata[] = "eg sequence number, routing info";


why not initialize message and metadata in the CTOR:

Authcrypt():message("Some things are better left unread"),metadata("eg sequence number, routing info")

This way, these members would remain private

These members are already private. They are under the private section of the class declaration.

message and metadata are static const members, in the public section

Thanks for pointing it out that they were public, I have now made them private members.

mazimkhan · 2017-06-15T13:26:54Z

Referring to my previous comment #95 (comment)

Just check whether mbed-os throws bad alloc if new fails or returns NULL. For former we don't need to do anything.

andresag01 · 2017-06-15T13:38:36Z

@mazimkhan: I do not think there is anything we can do. If new fails we just die, check here: https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_retarget.cpp#L941

mazimkhan · 2017-06-15T13:56:54Z

Thanks for looking into this.

mazimkhan · 2017-06-15T13:57:55Z

LGTM

The example is refactored to use C++ classes and allocate the bulk of the mbed TLS context structures in the heap instead of stack.

andresag01 · 2017-07-14T11:55:57Z

Rebase changes to run the CI with the latest mbed OS version 5.5.2

andresag01 · 2017-07-14T14:31:35Z

The change has been approved and the relevant tests pass in the CI, merging this PR.

andresag01 added enhancement mbed TLS team labels Jun 13, 2017

andresag01 requested review from mazimkhan and RonEld June 14, 2017 09:29

RonEld suggested changes Jun 14, 2017

View reviewed changes

andresag01 requested a review from gilles-peskine-arm June 14, 2017 10:43

andresag01 added do not merge needs: review labels Jun 14, 2017

mazimkhan reviewed Jun 15, 2017

View reviewed changes

RonEld reviewed Jun 15, 2017

View reviewed changes

RonEld approved these changes Jun 15, 2017

View reviewed changes

gilles-peskine-arm approved these changes Jun 30, 2017

View reviewed changes

Andres Amaya Garcia added 6 commits July 14, 2017 12:54

Refactor authcrypt

38259c3

The example is refactored to use C++ classes and allocate the bulk of the mbed TLS context structures in the heap instead of stack.

Change authcrypt constructor and destructor

3432b37

Split authcrypt code in .cpp and .h

c6b648c

Fix authcrypt main return code

bc494a9

Remove unused ret variable in authcrypt example

5167b66

Make message and key private members in authcrypt

a087163

andresag01 force-pushed the refactor-authcrypt branch from a4669be to a087163 Compare July 14, 2017 11:55

andresag01 removed the do not merge label Jul 14, 2017

andresag01 removed the needs: review label Jul 14, 2017

andresag01 merged commit 1fe10cb into ARMmbed:master Jul 14, 2017

sg- removed the enhancement label Jul 14, 2017


		const char Authcrypt::message[] = "Some things are better left unread";

		const char Authcrypt::metadata[] = "eg sequence number, routing info";

Refactor authcrypt example #95

Refactor authcrypt example #95

Uh oh!

Conversation

andresag01 commented Jun 13, 2017

Uh oh!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresag01 commented Jun 14, 2017

Uh oh!

RonEld commented Jun 14, 2017

Uh oh!

mazimkhan commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresag01 commented Jun 15, 2017

Uh oh!

mazimkhan commented Jun 15, 2017

Uh oh!

andresag01 commented Jun 15, 2017

Uh oh!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazimkhan commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andresag01 commented Jun 15, 2017

Uh oh!

mazimkhan commented Jun 15, 2017

Uh oh!

mazimkhan commented Jun 15, 2017

Uh oh!

andresag01 commented Jul 14, 2017

Uh oh!

andresag01 commented Jul 14, 2017

Uh oh!

Uh oh!

mazimkhan commented Jun 15, 2017 •

edited

Loading