tls-client for Pelion #216

k-stachowiak · 2018-11-19T11:06:32Z

This PR introduces a variant of the tls-client example application, that uses a Pelion-related configuration.

hanno-becker · 2018-11-19T17:19:11Z

tls-client-pelion/mbed_app.json

+    "macros": [
+        "MBED_CONF_APP_MAIN_STACK_SIZE=4096",
+        "MBEDTLS_USER_CONFIG_FILE=\"mbedtls_mbedos_pelion_config.h\"",
+        "MBEDTLS_USE_PSA_CRYPTO"


To my understanding, we'll also need MBEDTLS_PSA_CRYPTO_C here.

Since, as far as I know, this is only needed for testing, and not going to be required after PSA has been ultimately integrated, for the time being, we may limit ourselves to provide the missing define in the command line (as mbed-cli allows for that).

Do you think that will be sufficient or should it be included in the source and removed later?

@k-stachowiak IMHO it is not yet clear how the Mbed TLS - Mbed Crypto configuration will look like in the end, so I think for now we should adhere to the way it currently works, and there we need MBEDTLS_PSA_CRYPTO_C.

andresag01

I think that the PR looks good in general, but I have made a few comments. Also, I have a more general question: Why do we need a completely separate example for this? As far as I can see, you are duplicating close to 1000 lines of C++ code and I think the only differences are the configs and exactly 1 line of code in tls-client-pelion/main.cpp (which is actually a printf). Perhaps it is better to simply add more configs to tls-client and extend the README.md and CI test script to deal with it. If that is not ideal, perhaps it is possible to set up the mbed os config files to look up the sources in another directory or something like that?

andresag01 · 2018-11-19T20:24:36Z

tls-client-pelion/mbedtls_mbedos_pelion_config.h

+// and copies of this file may only be made by a person if such person is
+// permitted to do so under the terms of a subsisting license agreement
+// from ARM Limited or its affiliates.
+//----------------------------------------------------------------------------


Is this the correct copyright? If it is, could you please update the branding and dates?

I have copied this file from another ARM repository and I didn't feel authorized to change the copyright notice. If you can confirm, that we are allowed to perform such changes, I will gladly do so.

I cannot confirm this, but it would be good to check before merging.

andresag01 · 2018-11-19T20:26:37Z

tls-client-pelion/mbedtls_mbedos_pelion_config.h

+
+#ifndef MBEDTLS_HAVE_TIME_DATE
+    #define MBEDTLS_HAVE_TIME_DATE
+#endif //MBEDTLS_HAVE_TIME_DATE


Are we using secure time? Does this work with the library/software you are using? If I recall correctly, there were some issues with ARMCC because Mbed OS did not provide an implementation for gmtime() and ARMCC does not support it.

andresag01 · 2018-11-19T20:29:22Z

tls-client-pelion/mbedtls_entropy_config.h

+ *  limitations under the License.
+ *
+ *  This file is part of Mbed TLS (https://tls.mbed.org)
+ */


Is this file used anymore? It seems it has been replaced by tls-client-pelion/mbedtls_mbedos_pelion_config.h...

I think the removal of the file went missing in the latest rebase.

andresag01 · 2018-11-19T20:31:16Z

tls-client-pelion/mbedtls_mbedos_pelion_config.h

+#undef MBEDTLS_ECP_DP_BP256R1_ENABLED
+#undef MBEDTLS_ECP_DP_BP384R1_ENABLED
+#undef MBEDTLS_ECP_DP_BP512R1_ENABLED
+#undef MBEDTLS_ECP_DP_CURVE25519_ENABLED


What is the purpose of this config file? Is it to show pelion? are you trying to make a very minimal config (in this case I think this can be simplified further)? Also, I would suggest to include checks for stuff like entropy to avoid common misunderstanding and errors.

@andresag01 The configuration file is not @k-stachowiak's work - it is taken from https://github.com/ARMmbed/mbed-cloud-client/blob/master/mbed-client-pal/Configs/mbedTLS/mbedTLSConfig_mbedOS.h I think. To my understanding, reviewing the config is out of the scope of this PR, and should be tracked elsewhere. @sbutcher-arm Correct?

@hanno-arm @sbutcher-arm: I understand that this is not @k-stachowiak's work. All I am asking is: What are we trying to achieve? That would be useful to know to review the code before it is merged and try to anticipate what might be problematic. Are we even sure that the application passes in all the targets we want with this config?

andresag01 · 2018-11-19T20:32:28Z

tls-client-pelion/mbedtls_mbedos_pelion_config.h

+#undef MBEDTLS_ECP_DP_BP256R1_ENABLED
+#undef MBEDTLS_ECP_DP_BP384R1_ENABLED
+#undef MBEDTLS_ECP_DP_BP512R1_ENABLED
+#undef MBEDTLS_ECP_DP_CURVE25519_ENABLED


Last time I checked, SHA1 was needed because the website's certificate was using it. I am not sure this has changed, but I would suggest to check and enable it in necessary

That's currently being discussed in ARMmbed/mbed-os#8638.

It would be good to get that sorted out before merging. Unless we are happy to accept potential CI failures...

Also, I noticed that there were some changes to the certificates, perhaps this is no longer a problem?

andresag01 · 2018-11-19T20:33:59Z

tls-client-pelion/README.md

+The output in the terminal window should be similar to this:
+
+```
+Starting mbed-os-example-tls/tls-client


Does this need to be updated? For example, as far as I can see, the example is not called tls-client.

andresag01 · 2018-11-21T10:15:16Z

tls-client-pelion/README.md

+```
+Starting mbed-os-example-tls/tls-client-pelion
+Using Mbed OS 5.6.3
+[EasyConnect] IPv4 mode


Again, I think this is out of date. As far as I am aware, the example does not use easy-connect anymore

@andresag01 since it has not yet been decided, if this PR is going to be merged, and because it is outside of the PR's scope, I would rather raise an issue and fix it in a separate PR (this issue is also present in the regular tls-client). What do you think?

andresag01 · 2018-11-21T10:16:39Z

@k-stachowiak: Thanks for reworking the PR. I did another quick review and pointed out that the readme is still out-of-date. Also, I think a few of my previous comments/questions are still pending.

simonbutcher · 2018-11-21T14:13:44Z

Hi @k-stachowiak,

@andresag01 has already made the point - but I don't think we want to duplicate the example as tls-client-pelion and have it alongside the existing one. That will make maintenance harder, and I don't see any benefit.

That may meant there's no difference between the two examples - in which case we probably don't need this. (Easy!)

Did you have any reason for the second example? If not, can we make the fork as a branch, not in directories?

k-stachowiak · 2018-11-21T15:17:22Z

@sbutcher-arm I did it this way for pragmatic reasons. In the very beginning I was expecting even a potential rework of the client to the PSA API. Even though it didn't turn out necessary, I assumed an approach of not changing the design until we have ultimately confirmed the goal of this task.

I am still not sure if we want to just overwrite the config file. What if we decide to merge this branch? We will loose the current config, which is used across a wide range of different boards in the CI - it may break.

Maybe it would be better to implement simple a config selection mechanism (e.g. a compiletime constant that leads to including a different, non-default header file), so that we don't have to worry about the merge. I've got something like that in a local branch from the time, where we were considering testing against two different configs. What do you think?

k-stachowiak · 2019-01-28T08:17:50Z

@sbutcher-arm As #232 is nearing completion, and, as I believe, is a successor to this task, you may probably consider closing this.

k-stachowiak · 2019-02-01T11:42:44Z

#232 Has been merged obsoleting this PR, as I believe.

Krzysztof Stachowiak and others added 3 commits November 13, 2018 16:24

Add a copy of tls-client application

bb96b73

Rename the copied tls-client

c5f03f3

Add pelion configuration to the tls-client copy

e39d2f6

k-stachowiak added do not merge enhancement needs: review mbed TLS team labels Nov 19, 2018

k-stachowiak requested a review from hanno-becker November 19, 2018 15:54

k-stachowiak force-pushed the iotssl-2613-tls-client-psa branch from f13c725 to e39d2f6 Compare November 19, 2018 16:49

k-stachowiak added 2 commits November 19, 2018 17:49

Distinguish logs from the new client from the old one

7f9eb25

Enable PSA in Mbed TLS

687de63

hanno-becker reviewed Nov 19, 2018

View reviewed changes

andresag01 suggested changes Nov 19, 2018

View reviewed changes

k-stachowiak added 2 commits November 21, 2018 10:14

Remove an obsolete configuration file

be93dc9

Update README.md in the copy of tls-client

67e9c80

andresag01 reviewed Nov 21, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls-client for Pelion #216

tls-client for Pelion #216

k-stachowiak commented Nov 19, 2018 •

edited by hanno-becker

Loading

hanno-becker Nov 19, 2018

k-stachowiak Nov 20, 2018

hanno-becker Nov 20, 2018

andresag01 left a comment

andresag01 Nov 19, 2018

k-stachowiak Nov 20, 2018

andresag01 Nov 20, 2018

andresag01 Nov 19, 2018

andresag01 Nov 19, 2018

hanno-becker Nov 20, 2018

andresag01 Nov 19, 2018

hanno-becker Nov 20, 2018

andresag01 Nov 20, 2018

andresag01 Nov 19, 2018

hanno-becker Nov 20, 2018

andresag01 Nov 20, 2018

andresag01 Nov 19, 2018

andresag01 Nov 21, 2018

k-stachowiak Nov 21, 2018

andresag01 commented Nov 21, 2018

simonbutcher commented Nov 21, 2018

k-stachowiak commented Nov 21, 2018

k-stachowiak commented Jan 28, 2019

k-stachowiak commented Feb 1, 2019

tls-client for Pelion #216

Are you sure you want to change the base?

tls-client for Pelion #216

Conversation

k-stachowiak commented Nov 19, 2018 • edited by hanno-becker Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Nov 21, 2018

simonbutcher commented Nov 21, 2018

k-stachowiak commented Nov 21, 2018

k-stachowiak commented Jan 28, 2019

k-stachowiak commented Feb 1, 2019

k-stachowiak commented Nov 19, 2018 •

edited by hanno-becker

Loading