Add platform setup and teardown support #165

AndrzejKurek · 2018-04-12T13:13:16Z

Add platform setup and teardown for authcrypt, benchmark, hashing and tls-client examples.
Tested on K64F with GCC_ARM.

RonEld · 2018-04-12T13:17:25Z

Although logically it is correct, I think it is better to have the calls where all the cryptography functions are, so platform_ctx can be accessible ( after Mbed-TLS/mbedtls#1200 is resolved )

AndrzejKurek · 2018-04-13T06:49:46Z

@RonEld Currently the platform context is not needed or used by the tests. Even if it were, it would be an external dependency for a test case (not a part of ones setup), so I'd opt for a dependency injection using a function parameter.

AndrzejKurek · 2018-04-13T07:24:57Z

The CI fails on two known issues - memory allocation failure on one board, and lack of serial on another.

RonEld · 2018-04-15T07:59:45Z

@AndrzejKurek I agree, but if you don't have access to platform_ctx in your test code(without having it external, which loses the idea of having it as a parameter ), how can you inject it as a function parameter?

AndrzejKurek · 2018-04-16T07:31:17Z

@RonEld I don't really understand. If the platform context is somehow needed in the, for example, benchmark test, it can be passed to the function handling the test (and therefore where the cryptographic calls are made, as you suggested), instead of making it a global structure.

RonEld · 2018-04-16T07:49:49Z

@AndrzejKurek I believe we are talking about the same thing.
I am asking the context to be defined in benchmark() function (in the benchmark example), and not in main(), as all the cryptographic functions are called within benchmark, and not within main.
I am not asking it to be global.

AndrzejKurek · 2018-04-16T07:58:41Z

@RonEld And I'm saying that platform initialization is not part of the benchmark test, it is an "external" (from the benchmark test point of view) dependency, which could be satisfied using dependency injection - which would be creating it within main. This way, we can for example swap the platform context for a debug platform context, or extend it in any way we want to. This is a common pattern in, for example, unit testing, where objects or dependencies can be swapped with their respective mocks to perform the testing efficiently and at the relevant layer of abstraction.

RonEld · 2018-04-16T08:05:03Z

OK, so let's compromise on having the test constructors getting the context as parameter? (or adding a "set platform context" function?). Without the tests themselves getting access to the context, there is no much use in having this context

RonEld

minor comments, in addition to the context setup location discussion
There is style inconsistency in the if statements. Please remove white-space after parentheses for consistency

RonEld · 2018-04-16T08:05:54Z

authcrypt/main.cpp

@@ -24,6 +24,11 @@
 #include "mbedtls/platform.h"

 int main() {
+    mbedtls_platform_context platform_ctx;
+    if(mbedtls_platform_setup(&platform_ctx)) {


I prefer explicitly checking result for !=0

RonEld · 2018-04-16T08:06:03Z

benchmark/main.cpp

@@ -961,8 +961,14 @@ static int benchmark( int argc, char *argv[] )
 }

 int main(void) {
+    mbedtls_platform_context platform_ctx;
+    if(mbedtls_platform_setup(&platform_ctx)) {


I prefer explicitly checking result for !=0

RonEld · 2018-04-16T08:06:17Z

tls-client/main.cpp

@@ -412,6 +412,10 @@ class HelloHTTPS {
 * The main loop of the HTTPS Hello World test
 */
 int main() {
+    mbedtls_platform_context platform_ctx;
+    if( mbedtls_platform_setup(&platform_ctx)) {


I prefer explicitly checking result for !=0

RonEld · 2018-04-16T08:06:51Z

hashing/main.cpp

@@ -152,8 +152,14 @@ static int example(void)
 }

 int main() {
+    mbedtls_platform_context platform_ctx;
+    if( mbedtls_platform_setup(&platform_ctx)) {


I prefer explicitly checking result for !=0

AndrzejKurek · 2018-04-16T08:13:36Z

@RonEld The tests getting the context is the next step, but I'm hesitant to make this change without the tests doing anything with the context at all. Do You think that adding it, adding a (void) call in the beginning and commenting that it can be used if necessary will be OK?

RonEld · 2018-04-16T08:25:41Z

I believe you will probably get compilation warnings for unused parameter, but we can handle this.
I prefer adding this call, so it would show intent, with adding a comment as you mentioned, referring Mbed-TLS/mbedtls#1200
I believe there will be more questions raised, if the the functions will not get the context.

AndrzejKurek · 2018-04-16T12:44:34Z

@RonEld I've addressed your comments, please re-review.

RonEld

One minor comment added.
At the moment I am not approving nor rejecting.
I have two general comments:

As mentioned before, please add comment about the parameter.
Since this is .cpp code, the pointer is sent by value. I think it would be better to change the parameter type to const by reference (const mbedtls_platform_context& platform_ctx)

RonEld · 2018-04-16T12:50:00Z

authcrypt/authcrypt.h

@@ -23,6 +23,7 @@
 #include "mbedtls/cipher.h"
 #include "mbedtls/entropy.h"
 #include "mbedtls/ctr_drbg.h"
+#include "mbedtls/platform.h"


In this case it's better to have mbedtls_platform_context* as forward declaration , instead of adding include for the header. I prefer minimal includes in header files, unless really needed, and keep the include in the .cpp file

AndrzejKurek · 2018-04-17T07:32:43Z

Hi @RonEld,
I've added the comment pointing to issue 1200. As for the two other requests:

As much as I'd like to, I can't really forward declare an anonymous, typedef-ed struct. If you know a way to do it, please let me know.
I'm against having a class member reference pointing to an externally managed object for the following reasons:

If the context gets deinitialized, there's nothing you can do with such dangling reference;
The reference can't be set to null if there's no context to be used;
They can't be easily replaced with smart pointers, if we ever decide to use any;
We're forced to initialize the reference in the constructor, and we can't change it later.

RonEld

minor comments about the comments position.

I'll approve after this will be changerd,

RonEld · 2018-04-17T07:51:11Z

authcrypt/main.cpp

@@ -30,6 +30,8 @@ int main() {
    }

    int exit_code = MBEDTLS_EXIT_SUCCESS;
+    // The platform context is passed just in case any crypto calls need it.
+    // Please refer to https://github.com/ARMmbed/mbedtls/issues/1200 for more information.


Please move this to the Authcrypt ctor, and have same comment like in the hashing and benchmark examples.

RonEld · 2018-04-17T07:52:24Z

tls-client/main.cpp

@@ -443,6 +443,8 @@ int main() {
        return 1;
    }

+    // The platform context is passed just in case any crypto calls need it.
+    // Please refer to https://github.com/ARMmbed/mbedtls/issues/1200 for more information.


Please move this to the HelloHTTPS ctor, and have same comment like in the hashing and benchmark

RonEld

Much better, but please change the comment to be same as in hash and benchmarking ( with the comment on the uninitialized parameter)

AndrzejKurek · 2018-04-17T08:57:36Z

@RonEld - but the variable is being used, so the comment is irrelevant in the two classes that save it as a member.

RonEld · 2018-04-17T08:59:41Z

You are right, but it is being used by setting a member of the class. We should be consistent, and have this member for benchmarking and hash as well.

AndrzejKurek · 2018-04-17T09:10:23Z

I don't understand your request. Benchmark and hash tests don't use classes, they are just functions.

RonEld

Sorry, my bad. I haven't noticed that benchmark and hash weren't changed to classes, like the others

andresag01 · 2018-04-25T13:14:13Z

authcrypt/main.cpp

@@ -25,16 +25,18 @@

 int main() {
    mbedtls_platform_context platform_ctx;
-    if(mbedtls_platform_setup(&platform_ctx) != 0) {
-        return -1;
+    int exit_code = MBEDTLS_EXIT_FAILURE;


As far as I can tell, regardless of whther mbedtls_platform_setup and run return, MBEDTLS_EXIT_FAILURE will always be returned. Is this correct?

No, the value will be overriden by platform_setup and authcrypt->run.

andresag01 · 2018-04-25T13:22:23Z

benchmark/main.cpp

@@ -965,13 +965,19 @@ static int benchmark( int argc, char *argv[], mbedtls_platform_context* ctx )

 int main(void) {
    mbedtls_platform_context platform_ctx;
-    if(mbedtls_platform_setup(&platform_ctx) != 0) {
-        return -1;
+    int exit_code = MBEDTLS_EXIT_FAILURE;


Again, I think this function will always return MBEDTLS_EXIT_FAILURE, a failure code...

No, the value will be overriden by platform_setup and benchmark(...).

andresag01 · 2018-04-25T13:23:20Z

hashing/main.cpp

@@ -158,13 +158,19 @@ static int example(mbedtls_platform_context* ctx)

 int main() {
    mbedtls_platform_context platform_ctx;
-    if(mbedtls_platform_setup(&platform_ctx) != 0) {
-        return -1;
+    int exit_code = MBEDTLS_EXIT_FAILURE;


I think this function will always return MBEDTLS_EXIT_FAILURE, a failure code...

No, the value will be overridden by platform_setup and example(...)

andresag01 · 2018-04-25T13:24:46Z

tls-client/main.cpp

@@ -417,8 +417,11 @@ class HelloHTTPS {
 */
 int main() {
    mbedtls_platform_context platform_ctx;
-    if(mbedtls_platform_setup(&platform_ctx) != 0) {
-        return -1;
+    int ret = MBEDTLS_EXIT_FAILURE;


I think this function will always return MBEDTLS_EXIT_FAILURE, a failure code...

Ret is only used here to store the value of platform setup and print the error. When errors occur, sure, it returns MBEDTLS_EXIT_FAILURE, but when everything goes right, MBEDTLS_EXIT_SUCCESS is returned (at the end).

Add platform setup and teardown for authcrypt, benchmark, hashing and tls-client examples.

Patater · 2018-05-04T12:29:11Z

We've just merged a large refactoring to the examples. In the interest of getting this platform PR in sooner, I've rebased and resolved conflicts. The old branch is available at https://github.com/ARMmbed/mbed-os-example-tls/tree/feature-platform-init-old for reference, if desired.

Add platform context as a parameter for tests to enable the usage of it in cryptographic calls

Unify error handling in tests

Patater · 2018-05-04T14:38:20Z

CI passes on most targets, but got an out of memory error on KW24D with IAR.

14:13:08 [1525443188.04][CONN][RXD]   RSA-4096                 :  FAILED: RSA - The private key operation failed : BIGNUM - Memory allocation failed

AndrzejKurek · 2018-05-07T09:41:41Z

@Patater - this error also happens on master.

mazimkhan · 2018-05-08T11:52:46Z

authcrypt/authcrypt.cpp

@@ -41,8 +39,11 @@ const char Authcrypt::message[] = "Some things are better left unread";

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

-Authcrypt::Authcrypt()
+Authcrypt::Authcrypt(mbedtls_platform_context* platform_ctx)


Why is it required to pass platform_ctx to the application(test)?

It is not required, but we do so as a way of showing the user how to access it. It's an approach that we have chosen with Ron after a discussion that can be followed above, on this PR.

This can be limited to the tests that actually test things from the platform context.

mazimkhan · 2018-05-08T11:53:38Z

benchmark/main.cpp

@@ -300,7 +300,7 @@ typedef struct {
         rsa, dhm, ecdsa, ecdh;
 } todo_list;

-static int benchmark( int argc, char *argv[] )
+static int benchmark( int argc, char *argv[], mbedtls_platform_context* ctx )


Same question as https://github.com/ARMmbed/mbed-os-example-tls/pull/165/files#r186700998

mazimkhan · 2018-05-08T12:04:53Z

benchmark/main.cpp

@@ -309,7 +309,10 @@ static int benchmark( int argc, char *argv[] )
 #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C)
    unsigned char malloc_buf[HEAP_SIZE] = { 0 };
 #endif
-
+    // The call below is used to avoid the "unused parameter" warning.
+    // The context itself can be used by cryptographic calls which require it.


An application like this test will not directly call a cryptographic function. The cryptographic functions should have a way of accessing the context like a getter as suggested in Mbed-TLS/mbedtls#1200

mazimkhan · 2018-05-08T12:05:33Z

hashing/main.cpp

+    int exit_code = MBEDTLS_EXIT_FAILURE;
+
+    if((exit_code = mbedtls_platform_setup(&platform_ctx)) != 0) {
+        printf("Platform initialization failed with error %d\r\n", exit_code);


printf -> mbedtls_printf

mbedtls_printf may be a substituted for a custom printer function that may not work if the platform setup fails.

mazimkhan

Overall looks good. Only thing I don't like is passing platform_ctx to test functions that don't need it. But if that is approved by design I approve this PR.

AndrzejKurek requested review from andresag01 and RonEld April 12, 2018 13:13

AndrzejKurek added enhancement needs: review mbed TLS team runCI labels Apr 12, 2018

AndrzejKurek added runCI and removed runCI labels Apr 12, 2018

RonEld suggested changes Apr 16, 2018

View reviewed changes

AndrzejKurek added runCI and removed runCI labels Apr 16, 2018

RonEld reviewed Apr 16, 2018

View reviewed changes

RonEld suggested changes Apr 17, 2018

View reviewed changes

RonEld reviewed Apr 17, 2018

View reviewed changes

RonEld approved these changes Apr 17, 2018

View reviewed changes

AndrzejKurek added runCI and removed runCI labels Apr 25, 2018

andresag01 suggested changes Apr 25, 2018

View reviewed changes

andresag01 approved these changes Apr 25, 2018

View reviewed changes

AndrzejKurek added ready for merge and removed needs: review labels Apr 25, 2018

Andrzej Kurek added 2 commits May 4, 2018 12:33

Add platform setup and teardown support

f9d17b3

Add platform setup and teardown for authcrypt, benchmark, hashing and tls-client examples.

Add explicit checks for non-zero result of platform setup

7c78056

Patater force-pushed the feature-platform-init branch from 006a991 to c46f9b6 Compare May 4, 2018 12:27

Andrzej Kurek added 4 commits May 4, 2018 13:29

Pass platform context to tests

9a36b87

Add platform context as a parameter for tests to enable the usage of it in cryptographic calls

Add a link to the platform context passing issue

93b8992

Move platform context comments

3781f08

Add return value printing for failed platform initialization

4bd4bca

Unify error handling in tests

Patater force-pushed the feature-platform-init branch from c46f9b6 to 4bd4bca Compare May 4, 2018 12:30

Patater added needs: review runCI and removed ready for merge runCI labels May 4, 2018

RonEld approved these changes May 8, 2018

View reviewed changes

mazimkhan reviewed May 8, 2018

View reviewed changes

mazimkhan approved these changes May 8, 2018

View reviewed changes

AndrzejKurek added ready for merge and removed needs: review labels May 9, 2018

Patater merged commit 76832ce into master May 9, 2018

Add platform setup and teardown support #165

Add platform setup and teardown support #165

Uh oh!

Conversation

AndrzejKurek commented Apr 12, 2018

Uh oh!

RonEld commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrzejKurek commented Apr 13, 2018

Uh oh!

AndrzejKurek commented Apr 13, 2018

Uh oh!

RonEld commented Apr 15, 2018

Uh oh!

AndrzejKurek commented Apr 16, 2018

Uh oh!

RonEld commented Apr 16, 2018

Uh oh!

AndrzejKurek commented Apr 16, 2018

Uh oh!

RonEld commented Apr 16, 2018

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!

AndrzejKurek commented Apr 16, 2018

Uh oh!

RonEld commented Apr 16, 2018

Uh oh!

AndrzejKurek commented Apr 16, 2018

Uh oh!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek commented Apr 17, 2018

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!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek commented Apr 17, 2018

Uh oh!

RonEld commented Apr 17, 2018

Uh oh!

AndrzejKurek commented Apr 17, 2018

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!

Choose a reason for hiding this comment

Uh oh!

RonEld commented Apr 12, 2018 •

edited

Loading