Remove non-crypto code by Patater · Pull Request #71 · ARMmbed/mbed-crypto

Patater · 2019-02-25T14:17:28Z

Remove non-crypto code, as that code is maintained in the Mbed TLS repo. Only crypto code should be present in our repo.

Our CI has been updated to cope with the lack of non-crypto tests, programs, and code; we expect this PR to pass the PR job and nightly job.

Internal ref: IOTCRYPT-678, IOTCRYPT-682

Notes:

error.h still reserves space for SSL and X509 errors, to ensure those aren't re-used on accident

Patater · 2019-03-01T16:42:22Z

Split out into #74 which focuses on breaking dependencies

Patater · 2019-04-04T15:41:31Z

Rebased on top of the latest development. Previous commits at https://github.com/Patater/mbed-crypto/tree/remove-non-crypto-1

Patater · 2019-04-04T15:42:40Z

Rebased to remove per-commit testing scripts that were used for local testing and aren't meant to go in yet. Previous branch at https://github.com/Patater/mbed-crypto/tree/remove-non-crypto-2

Patater · 2019-04-05T08:21:12Z

Rebased to organize the commits a bit better. Previous branch at https://github.com/Patater/mbed-crypto/tree/remove-non-crypto-6

Patater · 2019-04-05T08:23:21Z

Rebased to remove per-commit testing script, again. Previous branch at https://github.com/Patater/mbed-crypto/tree/remove-non-crypto-7

gilles-peskine-arm · 2019-04-05T09:18:15Z

Before merging this PR, we should make an empirical test: make a branch based on this PR that adds a bug that breaks X509 or TLS but not crypto, and check that it fails the crypto-in-TLS part of the CI (but passes the rest of the CI).

gilles-peskine-arm

This looks mostly ok, there are just a few mistakes here and there.

A few of the mistakes are in the commit structure or in commit messages. Since this is a huge PR, please synchronize with reviewers before rebasing. For my sake, it's ok to rebase from now on until the next review stage.

tests/data_files/Makefile

tests/Makefile

tests/scripts/list-macros.sh

include/mbedtls/config.h

scripts/list-unused-test-files.sh

tests/data_files/print_c.pl

scripts/rename.pl

scripts/data_files/rename-1.3-2.0.txt

tests/scripts/generate_test_code.py

andresag01

The PR looks good. Just a few more minor points in addition to what @gilles-peskine-arm already pointed out.

NOTE: I did not actually try integrating this with the rest of Mbed TLS. I think it would be ideal to check that this integration works correctly.

andresag01 · 2019-04-07T20:34:15Z

configs/config-suite-b.h


 /* For test certificates */
 #define MBEDTLS_BASE64_C
-#define MBEDTLS_CERTS_C


This was removed, but do we need the certs.h file at all? I think that the x509 file was deleted, so we cannot even parse it.

andresag01 · 2019-04-07T20:38:23Z

include/mbedtls/check_config.h

-#error "!MBEDTLS_SSL_KEEP_PEER_CERTIFICATE requires MBEDTLS_SHA512_C, MBEDTLS_SHA256_C or MBEDTLS_SHA1_C"
-#endif
-
 #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) &&                          \


I think that strictly speaking the memory buffer alloc thing is a platform/test thing. I would not be sure whether it is part of the crypto or tls. Why dont we just remove it?

We'll evaluate removing platform things later. Out of scope for this PR.

andresag01 · 2019-04-08T17:49:51Z

CMakeLists.txt

-option(USE_PKCS11_HELPER_LIBRARY "Build mbed TLS with the pkcs11-helper library." OFF)
-option(ENABLE_ZLIB_SUPPORT "Build mbed TLS with zlib library." OFF)
-
 option(ENABLE_PROGRAMS "Build mbed TLS programs." ON)


Are we keeping the name "mbed TLS"? There are a few other places where this happens

We'll change the name, just not in this PR.

andresag01 · 2019-04-08T19:11:31Z

include/mbedtls/config.h

@@ -2861,9 +1957,6 @@
 * Module:  library/sha256.c


I noticed that we are keeping all the docs for MBEDTLS_*_ALT macros that are part of the old Mbed TLS hardware accelerator interface. Do we really want to keep those considering that we are coming up with a new abstraction layer for drivers?

The ALT drivers should still work. We need to implement an alternative to move them to before we remove ALT. Mbed OS currently uses Mbed Crypto, so we'd have to at least update all ALT drivers in Mbed OS. Mbed TLS LTS releases (2.16, 2.7) will still support ALT for other OSes.

Remove TLS and NET options from config files and scripts. Note that this fails check-names.sh because options that TLS and NET files use are no longer present in config.h.

Note that this fails check-names.sh because options that TLS and X.509 files use are no longer present in config.h.

We've removed all software that depends on or uses the TLS, NET, and X.509 modules. This means TLS, NET, and X.509 are unused and can be removed. Remove TLS, NET, and X.509.

Remove references to the X.509, NET, and SSL modules. Update text from "Mbed TLS" to "Mbed Crypto". Update version number.

Update comments in the top of the test code generator script with the name of the parent project.

Periodic release notes and git history will work fine and be easier to maintain.

Make maintaining config files easier by removing any explicit ciphersuite lists. These explicit lists are prone to being incomplete as TLS defines more and more ciphersuites. Rather than try to play catch up, let's refer to sets of ciphersuites with declarative language.

GCM is not just for AES, but for at least Camellia as well.

Previously, GCM required enabling either AES or Camellia. However, we also support using GCM with ARIA and without other ciphers. Enable configurations with only ARIA enabled to use GCM.

Patater · 2019-04-25T10:49:54Z

Rebased to fix the patch that removes udp_proxy to also remove udp_proxy from .gitignore. Previous patch set at https://github.com/Patater/mbed-crypto/tree/remove-non-crypto-11

Patater · 2019-04-29T16:07:36Z

CI is expected to pass with the exception of the TLS-testing-with-new-crypto-submodule job on check-generated-files. This is because of changes to how error descriptions will be placed into errors.c, when TLS uses a Crypto submodule that doesn't contain SSL or X.509 headers. See Patater/mbedtls@7df4245 for what the differences are and how to rectify the issue to make the test pass. This issue should not hold up this PR, as its easily fixed when TLS updates to the new subodule concurrently.

Patater · 2019-04-30T10:09:36Z

https://jenkins-internal.mbed.com/job/mbed-crypto-pr-ci-testing/job/PR-71-merge/6/display/redirect passes all tests, except the expected errors.c issue as previously mentioned.

Update the Mbed Crypto submodule to revision 461fd58 ("Merge pull request ARMmbed#71 from Patater/remove-non-crypto"). This includes removing SSL, NET, and X.509 modules from Mbed Crypto.

Patater added enhancement New feature or request needs: work The pull request needs rework before it can be merged. needs: ci Needs a passing full CI run labels Feb 25, 2019

Patater force-pushed the remove-non-crypto branch 11 times, most recently from 0da6d7d to ab52966 Compare March 1, 2019 14:20

Patater mentioned this pull request Mar 1, 2019

Break non-crypto dependencies #74

Merged

Patater force-pushed the remove-non-crypto branch from ab52966 to f071035 Compare April 4, 2019 15:40

Patater force-pushed the remove-non-crypto branch from f071035 to 7621d96 Compare April 4, 2019 15:42

Patater force-pushed the remove-non-crypto branch from 7621d96 to fb98280 Compare April 5, 2019 08:20

Patater force-pushed the remove-non-crypto branch from fb98280 to d7ce1f8 Compare April 5, 2019 08:22

Patater changed the title ~~WIP: Remove non-crypto code~~ Remove non-crypto code Apr 5, 2019

Patater added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Apr 5, 2019

gilles-peskine-arm requested changes Apr 5, 2019

View reviewed changes

andresag01 suggested changes Apr 8, 2019

View reviewed changes

Patater added 14 commits April 25, 2019 11:46

config: Remove TLS and NET options

1c66e48

Remove TLS and NET options from config files and scripts. Note that this fails check-names.sh because options that TLS and NET files use are no longer present in config.h.

config: Remove X.509 options

bb1f701

Note that this fails check-names.sh because options that TLS and X.509 files use are no longer present in config.h.

Remove unused TLS, NET, and X.509 files

a4308b2

We've removed all software that depends on or uses the TLS, NET, and X.509 modules. This means TLS, NET, and X.509 are unused and can be removed. Remove TLS, NET, and X.509.

scripts: Remove dependency on TLS

b58ff95

scripts: Remove dependency on X.509

43a450c

scripts: Remove dependency on NET

356acc8

Remove unused test data files

ef24980

scripts: Remove unneeded scripts

ed05b29

doxygen: Update for Mbed Crypto

3257773

Remove references to the X.509, NET, and SSL modules. Update text from "Mbed TLS" to "Mbed Crypto". Update version number.

tests: Update generator with Mbed Crypto comments

dfcf84a

Update comments in the top of the test code generator script with the name of the parent project.

Remove ChangeLog

8cedaec

Periodic release notes and git history will work fine and be easier to maintain.

config: Simplify incorrect GCM comment

7accf44

GCM is not just for AES, but for at least Camellia as well.

config: Enable using ARIA-GCM without other ciphers

651ae68

Previously, GCM required enabling either AES or Camellia. However, we also support using GCM with ARIA and without other ciphers. Enable configurations with only ARIA enabled to use GCM.

Patater dismissed gilles-peskine-arm’s stale review via 651ae68 April 25, 2019 10:48

Patater force-pushed the remove-non-crypto branch from 9b05f35 to 651ae68 Compare April 25, 2019 10:48

Patater requested review from dgreen-arm and gilles-peskine-arm April 25, 2019 10:50

dgreen-arm approved these changes Apr 25, 2019

View reviewed changes

gilles-peskine-arm approved these changes Apr 25, 2019

View reviewed changes

Patater removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Apr 25, 2019

Patater merged commit 461fd58 into ARMmbed:development Apr 30, 2019

Patater removed the needs: ci Needs a passing full CI run label Apr 30, 2019

This was referenced Mar 4, 2020

Experimental merge of Mbed Crypto Mbed-TLS/mbedtls#3078

Closed

[DO NOT MERGE] Unremove tls files in preparation for the repository merge #384

Closed

gilles-peskine-arm mentioned this pull request May 5, 2021

Backport 2.x: Remove config-psa-crypto.h Mbed-TLS/mbedtls#4456

Merged

Conversation

Patater commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patater commented Mar 1, 2019

Uh oh!

Patater commented Apr 4, 2019

Uh oh!

Patater commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patater commented Apr 5, 2019

Uh oh!

Patater commented Apr 5, 2019

Uh oh!

gilles-peskine-arm commented Apr 5, 2019

Uh oh!

gilles-peskine-arm left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andresag01 left a comment

Choose a reason for hiding this comment

Uh oh!

andresag01 Apr 7, 2019

Choose a reason for hiding this comment

Uh oh!

andresag01 Apr 7, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

andresag01 Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

andresag01 Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

Patater commented Apr 25, 2019

Uh oh!

Patater commented Apr 29, 2019

Uh oh!

Patater commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Patater commented Feb 25, 2019 •

edited

Loading

Patater commented Apr 4, 2019 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading