Update mbedtls importer to import mbed-crypto #8723

mohammad1603 · 2018-11-13T04:57:05Z

Description

Update the mbedtls importer to import the implementation of PSA Crypto APIs given by Mbed Crypto library.
In addition, it import the files to folders' structure as designed in #8680.
This PR depends on #8680 and the new version of mbedtls (Mbed TLS 2.14.0).

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Patater

What testing has been done so far? What's left to test?

Patater · 2018-11-13T08:15:51Z

features/mbedtls/importer/Makefile

 TARGET_SRC:=$(TARGET_PREFIX)src
 TARGET_INC:=$(TARGET_PREFIX)inc
 TARGET_TESTS:=$(TARGET_PREFIX)TESTS
+TARGET_SRV_IMPL:=$(TARGET_PREFIX_CRYPTO)/platform/COMPONENT_PSA_SRV_IMPL
+TARGET_SPE:=$(TARGET_PREFIX_CRYPTO)/platform/COMPONENT_SPE
+TARGETS_PSA_ACCELARATION:=$(TARGET_PREFIX_CRYPTO)/targets


Fix spelling of "ACCELARATION"

Patater · 2018-11-13T08:16:26Z

features/mbedtls/importer/Makefile

+TARGET_SRV_IMPL:=$(TARGET_PREFIX_CRYPTO)/platform/COMPONENT_PSA_SRV_IMPL
+TARGET_SPE:=$(TARGET_PREFIX_CRYPTO)/platform/COMPONENT_SPE
+TARGETS_PSA_ACCELARATION:=$(TARGET_PREFIX_CRYPTO)/targets
+COMPONENT_NSPE:=$(TARGET_SRV_IMPL)/COMPONENT_NSPE


Should this be "COMPONENT_" or "TARGET_". It isn't consistent with the previous things.

I am moving them back to component as a variable in the file, to be consistent with the other variables' names

Patater · 2018-11-13T08:17:38Z

features/mbedtls/importer/Makefile

@@ -62,6 +71,20 @@ rsync:
 	cp $(MBED_TLS_DIR)/LICENSE $(TARGET_PREFIX)
 	cp $(MBED_TLS_DIR)/apache-2.0.txt $(TARGET_PREFIX)
 	#
+	# Copying mbed Crypto into mbed library..


Use capital "Mbed". Probably also should say "Mbed OS" instead of "Mbed library".

Patater · 2018-11-13T08:18:04Z

features/mbedtls/importer/Makefile

 TARGET_SRC:=$(TARGET_PREFIX)src
 TARGET_INC:=$(TARGET_PREFIX)inc
 TARGET_TESTS:=$(TARGET_PREFIX)TESTS
+TARGET_SRV_IMPL:=$(TARGET_PREFIX_CRYPTO)/platform/COMPONENT_PSA_SRV_IMPL


Should this be prefixed with "COMPONENT_" or "TARGET_"?

same as here #8723 (comment)

Patater · 2018-11-13T08:20:08Z

features/mbedtls/importer/Makefile

 	# Updating checked out version tag
 	echo $(MBED_TLS_RELEASE) > $(TARGET_PREFIX)VERSION.txt
+	#
+	# Create mbed Crypto target folder
+	mkdir -p $(TARGET_PREFIX_CRYPTO)


Should this be done in "update" or perhaps "rsync"?

Patater · 2018-11-13T08:20:38Z

features/mbedtls/importer/Makefile

@@ -107,4 +136,5 @@ clean:
 	rm -rf $(TARGET_SRC)
 	rm -rf $(TARGET_INC)
 	rm -rf $(MBED_TLS_DIR)
-
+	rm -rf $(TARGET_SRV_IMPL)
+	rm -rf $(TARGET_SPE)


What about COMPONENT_NSPE or others? Should we remove those as well here?

COMPONENT_NSPE will be deleted with TARGET_SRV_IMPL since it is an inner folder.
As I understood from @alzix that the acceleration folder and TARGET_PREFIX_CRYPTO folder should be always there and not deleted

mohammad1603 · 2018-11-13T12:30:56Z

@Patater the tests importer was tested locally and it import the files as expected, in addition @orenc17 used the importer to run on-target tests over PSoC6 and they seems to be ok

Patater

Dependent on Mbed TLS tag that contains Mbed Crypto

Patater · 2018-11-15T16:58:48Z

features/mbedtls/importer/Makefile

@@ -27,20 +27,29 @@
 #

 # Set the mbed TLS release to import (this can/should be edited before import)
-MBED_TLS_RELEASE ?= mbedtls-2.13.1
+MBED_TLS_RELEASE ?= mbedtls-2.14.0


The tag we use for the Mbed OS 5.11 release will be later than mbedtls-2.14.0. Not known yet what we'll use.

simonbutcher

Some questions and some requested changes.

simonbutcher · 2018-11-15T16:59:07Z

features/mbedtls/importer/Makefile

@@ -27,20 +27,29 @@
 #

 # Set the mbed TLS release to import (this can/should be edited before import)
-MBED_TLS_RELEASE ?= mbedtls-2.13.1
+MBED_TLS_RELEASE ?= mbedtls-2.14.0


This shouldn't be changed yet. 2.14.0 hasn't been released, doesn't reflect the version in the library, and won't be the version shipping in Mbed OS 5.11. Please revert.

simonbutcher · 2018-11-15T17:09:08Z

features/mbedtls/importer/Makefile


 # Translate between mbed TLS namespace and mbed namespace
 TARGET_PREFIX:=../
+TARGET_PREFIX_CRYPTO:=../crypto/


The name we agreed for the directory was mbed-crypto, not crypto. Why the change?

it was based on: Mbed-TLS/mbedtls#2146
I followed the submodule folder name.
cc @Patater

Hi @sbutcher-arm ,
i hope it's not crucial for you. since the PR into mbedTLS also follows the 'crypto' naming

the target folder was renamed to mbed-crypto

simonbutcher · 2018-11-15T17:11:31Z

features/mbedtls/importer/Makefile

 TARGET_SRC:=$(TARGET_PREFIX)src
 TARGET_INC:=$(TARGET_PREFIX)inc
 TARGET_TESTS:=$(TARGET_PREFIX)TESTS
+TARGET_SRV_IMPL:=$(TARGET_PREFIX_CRYPTO)/platform/TARGET_PSA/COMPONENT_PSA_SRV_IMPL
+TARGET_SPE:=$(TARGET_PREFIX_CRYPTO)/platform/TARGET_PSA/COMPONENT_SPE
+TARGET_PSA_ACCELERATION:=$(TARGET_PREFIX_CRYPTO)/targets


I think calling it 'acceleration' is a mistake, and it should be 'drivers' or 'hardware'. Not all driver code/hardware support will speed up crypto operations, and may instead just offer additional security.

simonbutcher · 2018-11-15T17:13:09Z

features/mbedtls/importer/Makefile

@@ -92,6 +118,9 @@ update: $(MBED_TLS_GIT_CFG)  $(MBED_TLS_HA_GIT_CFG)
 	# Checking out the required release
 	git -C $(MBED_TLS_DIR) checkout $(MBED_TLS_RELEASE)
 	#
+	# Update and checkout git submodules
+	git -C $(MBED_TLS_DIR) submodule update --init --recursive
+	#
 	# Updating checked out version tag
 	echo $(MBED_TLS_RELEASE) > $(TARGET_PREFIX)VERSION.txt


Should we also capture the Crypto version? Or is the commit id of the submodule enough?

The submodule ties down the Mbed Crypto version, but I think it'd be nice to add to the VERSION.txt file for humans so they don't have to go dig it up themselves.

pushd $(MBED_TLS_DIR)/crypto git describe --tags --abbrev=12 --dirty --always >> $(TARGET_PREFIX)VERSION.txt popd

simonbutcher · 2018-11-15T17:19:46Z

features/mbedtls/importer/Makefile

+	# Create Mbed Crypto target folder
+	mkdir -p $(TARGET_PREFIX_CRYPTO)
+	#
+	# Copying Mbed Crypto into Mbed OS..


I think what's going on here is a bit opaque to outsiders and other teams. There needs to be explanation here, (or pointers to documentation elsewhere) what these directories are and what they mean.

We need to explain what SPE, NSPE are, and what the other directories are and what they mean.

And most of all, we need to explain why we have so many directories. It will seem bizarre and unnecessarily complicated to outsiders.

Since this directory hierarchy is exist in mbed-os I don't know how adding explanation here will help outsiders.
#8680 PR description explains that but not the merged code.
This PR: ARMmbed/mbed-os-5-docs#815 explains more about that and i think it will be helpful for outsiders

Ok - Mbed TLS is my project that I work on daily, and these directories mean nothing to me. What do they do? What are they for? There's no explanation.

I think we need to either:

add comments to explain these directories, either in this file or in another file in this directory

tell me where there's other existing documentation that explains the structure elsewhere in the repository

add a comment that points the reader to documentation somewhere else

I added the required documentation in the importer.

simonbutcher · 2018-11-15T17:21:38Z

features/mbedtls/importer/Makefile

+	rm -rf $(TARGET_SPE)
+	rm -rf $(TARGET_PSA_ACCELERATION)
+
+	mkdir -p $(TARGET_SRV_IMPL)


Why are we removing and making these directories? Unlike the other directories being made, won't there be other files in these directories which aren't being imported?

I think only $(TARGET_PSA_ACCELERATION) shouldn't be removed, the others are created and only the importer copies files into them.

simonbutcher · 2018-11-15T17:24:19Z

@andresag01 - Can you please review this?

Note to maintainers - please don't merge this until the TLS team have signed off on it.

…ypto

mohammad1603 · 2018-11-19T07:08:27Z

features/mbedtls/importer/Makefile

 TARGET_SRC:=$(TARGET_PREFIX)src
 TARGET_INC:=$(TARGET_PREFIX)inc
 TARGET_TESTS:=$(TARGET_PREFIX)TESTS
+TARGET_SRV_IMPL:=$(TARGET_PREFIX_CRYPTO)/platform/TARGET_PSA/COMPONENT_PSA_SRV_IMPL


@orenc17 does the build system recognize the PSA_TARGET folder?

alzix · 2018-11-19T07:25:11Z

It will after #8744 will be merged. Till then mbed-cli will ignore this folder.

set crypto storage related defines.

(cherry picked from commit 296001a)

alekshex · 2018-11-21T10:20:15Z

@sbutcher-arm All the comments were treated can you please re review. Thank you!

dgreen-arm

I'm happy with the changes I requested.

simonbutcher · 2018-11-21T15:15:24Z

features/mbedtls/importer/adjust-config.sh

@@ -140,3 +140,6 @@ conf unset MBEDTLS_SSL_TRUNCATED_HMAC

 conf unset MBEDTLS_PLATFORM_TIME_TYPE_MACRO

+conf set MBEDTLS_PSA_CRYPTO_STORAGE_C


These will be configurations of the crypto config.h. At this point we share a config.h, but that won't be true in the next release.

Therefore, I suggest putting a comment in to identify these as Crypto configs, as they have no meaning outside the Crypto submodule, and possibly structuring this to make the transition easier later.

Added documentation for the configurations

simonbutcher · 2018-11-21T16:48:41Z

features/mbedtls/importer/Makefile

+	# Create Mbed Crypto target folder
+	mkdir -p $(TARGET_PREFIX_CRYPTO)
+	#
+	# Copying Mbed Crypto into Mbed OS..


Ok - Mbed TLS is my project that I work on daily, and these directories mean nothing to me. What do they do? What are they for? There's no explanation.

I think we need to either:

add comments to explain these directories, either in this file or in another file in this directory

tell me where there's other existing documentation that explains the structure elsewhere in the repository

add a comment that points the reader to documentation somewhere else

* Introduce the new folders' structure for the mbed crypto target folder. * Explain about the need of the new configuration added to adjust-config.sh script

bulislaw · 2018-11-22T16:46:31Z

@Patater @sbutcher-arm please see the fixes.

simonbutcher · 2018-11-22T19:28:51Z

features/mbedtls/importer/adjust-config.sh

@@ -140,6 +140,8 @@ conf unset MBEDTLS_SSL_TRUNCATED_HMAC

 conf unset MBEDTLS_PLATFORM_TIME_TYPE_MACRO

+# The following configurations are a needed for Mbed Crypto submodule.


Not a blocker - but it should be:

The following configurations are needed

simonbutcher · 2018-11-22T19:32:10Z

features/mbedtls/importer/Makefile

+# New folder structure is introduced here for targets with Secured-Partition-Environment
+# and Non-Secured-Partition-Environment, below documentation for each folder:
+# COMPONENT_PSA_SRV_IMPL - include secure service business logic implementation
+# code. For example mbedCrytpo or secure time core logic


Not a blocker - there are a few typos here, and the phrasing can be better. You don't need to say "New folder". In a year's time, the comment will still be there, and it won't be new! Revising the language I'd suggest:

# The folder structure is for targets that compile to the Secured-Partition-Environment # and Non-Secured-Partition-Environments. Documentation for each folder: # COMPONENT_PSA_SRV_IMPL - includes secure service business logic implementation # code. For example Mbed Crypto or the secure time core logic

There's also a few typos in the following lines. eg. "includes" not "include"

simonbutcher

The new documentation changes are good enough. Thanks @mohammad1603!

simonbutcher · 2018-11-23T11:10:12Z

@adbridge / @cmonr / @0xc0170 - Can we merge this ASAP? The Mbed TLS release PR is dependent on it.

If it isn't merged soon, we'll have to absorb the changes into the release PR, and cancel this one.

0xc0170 · 2018-11-23T11:16:39Z

If it isn't merged soon, we'll have to absorb the changes into the release PR, and cancel this one.

Let's do it in one (there are still some CI issues being fixed)

adbridge · 2018-11-23T11:17:48Z

Agree with @0xc0170

simonbutcher · 2018-11-23T11:24:58Z

Ok - we can do that if it's easier for you. If you wish, you can label this as 'DO NOT MERGE' and I'll include it in my TLS release PR later in the day.

Patater · 2018-11-23T16:34:32Z

Note that this additionally depends on #8730 for psa_prot_internal_storage.h. This is not a hard dependency, but it means some parts of this PR are not testable without #8730 merged before (or after).

Patater · 2018-11-23T19:28:55Z

This should be closed in favor of #8859

NirSonnenschein · 2018-11-23T19:57:05Z

closed in favor of #8859 which contains all changes from this PR.

NirSonnenschein requested a review from Patater November 13, 2018 07:59

NirSonnenschein added needs: review do not merge needs: preceding PR labels Nov 13, 2018

Patater suggested changes Nov 13, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Nov 13, 2018

NirSonnenschein added needs: review and removed needs: work labels Nov 14, 2018

NirSonnenschein approved these changes Nov 14, 2018

View reviewed changes

adbridge added the release-version: 5.11.0-rc1 label Nov 14, 2018

Patater mentioned this pull request Nov 15, 2018

Add mbed_lib.json for Mbed Crypto #8755

Closed

Patater reviewed Nov 15, 2018

View reviewed changes

simonbutcher suggested changes Nov 15, 2018

View reviewed changes

NirSonnenschein added the risk: A label Nov 18, 2018

mohammad1603 force-pushed the mbed_crypto_importer branch from 783f884 to 506e5c7 Compare November 18, 2018 16:10

mohammad1603 added 8 commits November 18, 2018 18:37

Update mbedtls importer to import mbed-crypto

cefb436

update importer to copy any psa_*.c file from crypto folder

af37f41

Change variables' names to keep consistent with the file convention

d15fba7

create the creation of the crypto target folder to the rsync rule

32585ed

Change "mbed crypto" to "Mbed Crypto"

1f30e34

Add extra folder "TARGET_PSA" for the destination path of the Mbed Cr…

6438b30

…ypto

update mbed crypto branch version

43c399a

Change target "acceleration" to "drivers"

8b8990d

mohammad1603 force-pushed the mbed_crypto_importer branch from 506e5c7 to 8b8990d Compare November 18, 2018 16:42

mohammad1603 commented Nov 19, 2018

View reviewed changes

0xc0170 removed the needs: preceding PR label Nov 19, 2018

Moran Peker and others added 2 commits November 20, 2018 15:14

Update adjust-config.sh

8514fba

set crypto storage related defines.

Add mbed_lib.json for Mbed Crypto

6cb9678

(cherry picked from commit 296001a)

mohammad1603 added 2 commits November 21, 2018 15:26

Change the crypto target prefix to "mbed-crypto"

fa2f108

revert file permissions to original one

e8e61b1

dgreen-arm approved these changes Nov 21, 2018

View reviewed changes

simonbutcher suggested changes Nov 21, 2018

View reviewed changes

mohammad1603 mentioned this pull request Nov 21, 2018

PSA Crypto SPM #8804

Merged

Documentation improvements for Importer and configuration script.

f2b9f2f

* Introduce the new folders' structure for the mbed crypto target folder. * Explain about the need of the new configuration added to adjust-config.sh script

simonbutcher reviewed Nov 22, 2018

View reviewed changes

simonbutcher approved these changes Nov 22, 2018

View reviewed changes

Patater approved these changes Nov 22, 2018

View reviewed changes

adbridge added needs: CI and removed needs: review labels Nov 23, 2018

Patater mentioned this pull request Nov 23, 2018

Import Mbed TLS v2.15.0 with Mbed Crypto #8859

Merged

NirSonnenschein closed this Nov 23, 2018

NirSonnenschein removed do not merge needs: CI labels Nov 23, 2018

cmonr removed risk: A needs: preceding PR release-version: 5.11.0-rc1 labels Nov 26, 2018

		@@ -140,3 +140,6 @@ conf unset MBEDTLS_SSL_TRUNCATED_HMAC

		conf unset MBEDTLS_PLATFORM_TIME_TYPE_MACRO

		conf set MBEDTLS_PSA_CRYPTO_STORAGE_C

		@@ -140,6 +140,8 @@ conf unset MBEDTLS_SSL_TRUNCATED_HMAC

		conf unset MBEDTLS_PLATFORM_TIME_TYPE_MACRO

		# The following configurations are a needed for Mbed Crypto submodule.

Update mbedtls importer to import mbed-crypto #8723

Update mbedtls importer to import mbed-crypto #8723

Uh oh!

Conversation

mohammad1603 commented Nov 13, 2018

Description

Pull request type

Uh oh!

Patater 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!

mohammad1603 Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

mohammad1603 Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohammad1603 commented Nov 13, 2018

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbutcher 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!

mohammad1603 Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbutcher commented Nov 15, 2018

mohammad1603 Nov 13, 2018 •

edited

Loading

mohammad1603 Nov 13, 2018 •

edited

Loading

mohammad1603 Nov 18, 2018 •

edited

Loading

alekshex commented Nov 21, 2018 •

edited

Loading

simonbutcher Nov 22, 2018 •

edited

Loading