Skip to content

Update mbedtls importer to import mbed-crypto #8723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 15 commits into from
Closed

Update mbedtls importer to import mbed-crypto #8723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cefb436
Update mbedtls importer to import mbed-crypto
mohammad1603 Nov 13, 2018
af37f41
update importer to copy any psa_*.c file from crypto folder
mohammad1603 Nov 13, 2018
d15fba7
Change variables' names to keep consistent with the file convention
mohammad1603 Nov 13, 2018
32585ed
create the creation of the crypto target folder to the rsync rule
mohammad1603 Nov 13, 2018
1f30e34
Change "mbed crypto" to "Mbed Crypto"
mohammad1603 Nov 13, 2018
6438b30
Add extra folder "TARGET_PSA" for the destination path of the Mbed Cr…
mohammad1603 Nov 15, 2018
43c399a
update mbed crypto branch version
mohammad1603 Nov 18, 2018
8b8990d
Change target "acceleration" to "drivers"
mohammad1603 Nov 18, 2018
387665b
Adding psa_*.h copy to the importer script
Nov 20, 2018
f10e107
Update adjust-config.sh
Nov 20, 2018
8514fba
Update adjust-config.sh
Nov 20, 2018
6cb9678
Add mbed_lib.json for Mbed Crypto
mohammad1603 Nov 15, 2018
fa2f108
Change the crypto target prefix to "mbed-crypto"
mohammad1603 Nov 21, 2018
e8e61b1
revert file permissions to original one
mohammad1603 Nov 21, 2018
f2b9f2f
Documentation improvements for Importer and configuration script.
mohammad1603 Nov 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions features/mbedtls/crypto/targets/TARGET_PSA/mbed_lib.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "mbed-crypto",
"macros": ["MBEDTLS_PSA_HAS_ITS_IO"]
}
46 changes: 45 additions & 1 deletion features/mbedtls/importer/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,35 @@ MBED_TLS_RELEASE ?= mbedtls-2.13.1

# Translate between mbed TLS namespace and mbed namespace
TARGET_PREFIX:=../
TARGET_PREFIX_CRYPTO:=../mbed-crypto/
TARGET_SRC:=$(TARGET_PREFIX)src
TARGET_INC:=$(TARGET_PREFIX)inc
TARGET_TESTS:=$(TARGET_PREFIX)TESTS

# 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
Copy link
Contributor

@simonbutcher simonbutcher Nov 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

TARGET_SRV_IMPL:=$(TARGET_PREFIX_CRYPTO)/platform/TARGET_PSA/COMPONENT_PSA_SRV_IMPL
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orenc17 does the build system recognize the PSA_TARGET folder?

# COMPONENT_SPE - include code that compiles ONLY to secure image and never
# compiles to non-secure image
TARGET_SPE:=$(TARGET_PREFIX_CRYPTO)/platform/TARGET_PSA/COMPONENT_SPE
# The folder contain specific target implementation using hardware.
TARGET_PSA_DRIVERS:=$(TARGET_PREFIX_CRYPTO)/targets
# COMPONENT_NSPE - include code that compiles ONLY to snon-secure image and
# never compiles to secure image
TARGET_NSPE:=$(TARGET_SRV_IMPL)/COMPONENT_NSPE

# mbed TLS source directory - hidden from mbed via TARGET_IGNORE
MBED_TLS_URL:[email protected]:ARMmbed/mbedtls-restricted.git
MBED_TLS_DIR:=TARGET_IGNORE/mbedtls
MBED_TLS_API:=$(MBED_TLS_DIR)/include/mbedtls
MBED_TLS_GIT_CFG=$(MBED_TLS_DIR)/.git/config

# Mbed Crypto directory - hidden from mbed via TARGET_IGNORE
MBED_CRYPTO_DIR:=$(MBED_TLS_DIR)/crypto
MBED_CRYPTO_API:=$(MBED_CRYPTO_DIR)/include/psa

.PHONY: all deploy deploy-tests rsync mbedtls clean update

all: mbedtls
Expand All @@ -62,6 +81,23 @@ rsync:
cp $(MBED_TLS_DIR)/LICENSE $(TARGET_PREFIX)
cp $(MBED_TLS_DIR)/apache-2.0.txt $(TARGET_PREFIX)
#
# Create Mbed Crypto target folder
mkdir -p $(TARGET_PREFIX_CRYPTO)
#
# Copying Mbed Crypto into Mbed OS..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the required documentation in the importer.

rm -rf $(TARGET_SRV_IMPL)
rm -rf $(TARGET_SPE)

mkdir -p $(TARGET_SRV_IMPL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mkdir -p $(TARGET_SPE)
mkdir -p $(TARGET_NSPE)
mkdir -p $(TARGET_PSA_DRIVERS)

rsync -a --delete --exclude='crypto_struct.h' $(MBED_CRYPTO_API) $(TARGET_INC)
rsync -a --delete $(MBED_CRYPTO_API)/crypto_struct.h $(TARGET_NSPE)
rsync -a --delete $(MBED_CRYPTO_API)/crypto_struct.h $(TARGET_SPE)/crypto_struct_spe.h
rsync -a --delete $(MBED_CRYPTO_DIR)/library/psa_*.c $(TARGET_SRV_IMPL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header files for these .c files are in the library folder as well, they'll need to be handled as well. They follow a psa_*.h format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgreen-arm changes added. can you please re review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks fine. The other change that's needed is adding the following lines to the end of `adjust-config.sh:

conf unset MBEDTLS_PSA_CRYPTO_STORAGE_C
conf unset MBEDTLS_PSA_CRYPTO_STORAGE_FILE_C

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we have ITS storage, we'll have to conf set MBEDTLS_PSA_CRYPTO_STORAGE_C, but I agree we should unset MBEDTLS_PSA_CRYPTO_STORAGE_FILE_C

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgreen-arm I've updated adjust-config.sh, please take a look.

rsync -a --delete $(MBED_CRYPTO_DIR)/library/psa_*.h $(TARGET_SRV_IMPL)

deploy: rsync
#
Expand Down Expand Up @@ -92,8 +128,15 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

#
# Updating Mbed Crypto checked out version tag
git -C $(MBED_TLS_DIR) describe --tags --abbrev=12 --dirty --always >> $(TARGET_PREFIX)VERSION.txt


$(MBED_TLS_GIT_CFG):
rm -rf $(MBED_TLS_DIR)
Expand All @@ -107,4 +150,5 @@ clean:
rm -rf $(TARGET_SRC)
rm -rf $(TARGET_INC)
rm -rf $(MBED_TLS_DIR)

rm -rf $(TARGET_SRV_IMPL)
rm -rf $(TARGET_SPE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mohammad1603 mohammad1603 Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

5 changes: 5 additions & 0 deletions features/mbedtls/importer/adjust-config.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker - but it should be:

The following configurations are needed

# They are related to the persistent key storage feature.
conf set MBEDTLS_PSA_CRYPTO_STORAGE_C
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added documentation for the configurations

conf set MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C
conf unset MBEDTLS_PSA_CRYPTO_STORAGE_FILE_C
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itayzafrir I think we need to do this:
conf unset MBEDTLS_PSA_CRYPTO_STORAGE_FILE_C
conf set MBEDTLS_PSA_CRYPTO_STORAGE_C
conf set MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the file permissions were changed, I'm not sure that was intended.
If ITS storage is guaranteed to be on Mbed OS targets, then this is correct. If it isn't guaranteed, then all three macros should be unset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgreen-arm I fixed the file permission.
Thanks!