Break non-crypto dependencies #74

Patater · 2019-03-01T15:22:56Z

This is a portion of #71

The goal is only to break dependencies on TLS and X.509 and NET, without removing those modules yet.

gilles-peskine-arm

Two major issues (#74 (comment) and #74 (comment)) and a couple of suggestions.

library/oid.c

tests/scripts/all.sh

tests/scripts/check-generated-files.sh

programs/psa/crypto_examples.c

library/x509_crt.c

Patater · 2019-03-04T17:05:18Z

Rebased to address review feedback in original commits. More rebasing expected before this is ready to review again.

Previous branch at https://github.com/Patater/mbed-crypto/tree/break-non-crypto-dependencies-1

Patater · 2019-03-05T11:47:39Z

Rebased to ignore --memory type options. Still work outstanding on removing TLS dependency from X.509.

Patater · 2019-03-05T11:52:45Z

I'm removing the oid and X.509 detangling commits as well, since we can get those from Mbed TLS development once merged there.

gilles-peskine-arm

Looks ok except for inconsistencies about which calls to SSL test scripts are removed and which aren't.

tests/scripts/all.sh

include/mbedtls/asn1.h

programs/psa/crypto_examples.c

tests/scripts/check-generated-files.sh

k-stachowiak

I only have a single, minor remark, otherwise, provided that Gilles' remarks will be addressed, I approve the changes.

k-stachowiak · 2019-03-07T13:18:04Z

include/mbedtls/ecp.h

- *                  function equivalent to the function with the same name
+ *                  This only applies to functions whose documentation mentions
+ *                  they may return #MBEDTLS_ERR_ECP_IN_PROGRESS (or
+ *                  `MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS` for functions in the


Minor: MBEDLTS_ERR_SL_CRYPTO_IN_PROGRESS is back quoted here, but not elsewhere, e.g. in config.h

The commit is only to remove the dependency in the ecp module. A later commit that removes SSL options from config.h will take care to update other references.

Patater · 2019-03-07T14:42:09Z

Windows CI has failed in the "shipped build". I'm investigating and working on a fix.

Patater · 2019-03-08T11:36:04Z

Rebased to address feedback and hopefully fix the Windows build.

Patater · 2019-03-08T15:35:01Z

Force push to update visualc files...

gilles-peskine-arm

Approving 27e5907

Patater · 2019-03-11T11:29:27Z

This needs a rebase to resolve conflicts, plus CI and re-review.

This makes it easier to add or remove programs as well as see which programs were added or removed in diffs.

Our default configuration file, include/mbedtls/config.h, should always match configs/config-psa-crypto.h. It had gotten out of sync, so put it back into sync.

Doxygen will fail to build if we have references to files that don't exist. Since we are planning on removing X.509 soon, we even need to remove explicit Doxygen references to X.509 things as those will no longer resolve once the X.509 files are deleted. fixup! asn1: Remove dependency on X.509

As the SSL programs, like ssl_client2 and ssl_server2, are dependent on SSL and therefore about to be removed, the only consumer of query_config is the query_compile_time_config test. As such, it makes sense to move query_config to be next to what uses it.

platform.h should only be used internally by the library implementation itself, not the examples. Remove the dependency on platform.h from all PSA programs.

For Makefiles, enable overriding where includes can come from in order to enable the parent module to set the include path. This allows the parent module to specify that its config.h should be used, even when the submodule when built standalone would use a different config.h. For CMake, always look in the parent's include folder and our own. List the parent's include folder first, so that preference is given to parent include files.

The version test suite is duplicated between Mbed TLS and Mbed Crypto. Use TLS's copy and not Crypto's copy when Crypto is used as a submodule of TLS. The version test is the only test that is tested from both TLS and Crypto, despite being entirely in libmbedcrypto. This is because the test data is code-gen'd from the version updating script and the version between Mbed TLS and Mbed Crypto don't necessarily always agree. The test data must come from the top level module, as only the top level module will have test data that matches the expected version.

Prepend ".crypto" to tests that came from the crypto submodule. This allows, when this project is used as a submodule, for tests with names the same between the parent and this project when used as a submodule to both be built and run.

Patater · 2019-03-11T18:15:26Z

Rebased to latest crypto/development to resolve conflicts. Previous branch at https://github.com/Patater/mbed-crypto/tree/break-non-crypto-dependencies-12.5. No changes other than conflict resolution.

gilles-peskine-arm

I merged the version of this PR that I last approved (27e5907) with development (6d9cb25) and obtained identical results.

gilles-peskine-arm · 2019-03-12T10:41:41Z

The CI failures are due to a test script that calls compat.sh and ssl-opt.sh and needs to be updated (I believe that'll be fixed when Mbed-TLS/mbedtls-test#56 is merged) and one failure of the FreeBSD timing test which is a known issue. So this PR is good to merge once the offending test script is updated.

Patater · 2019-03-13T10:39:09Z

Retested after merging Mbed-TLS/mbedtls-test#56 and all is good.

Patater added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. labels Mar 1, 2019

Patater requested a review from gilles-peskine-arm March 1, 2019 15:22

gilles-peskine-arm requested changes Mar 1, 2019

View reviewed changes

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

Patater mentioned this pull request Mar 1, 2019

Remove non-crypto code #71

Merged

Patater force-pushed the break-non-crypto-dependencies branch from 503e1d6 to 7c36f6a Compare March 4, 2019 17:01

Patater force-pushed the break-non-crypto-dependencies branch from 7c36f6a to bf5082b Compare March 5, 2019 11:43

Patater force-pushed the break-non-crypto-dependencies branch from bf5082b to e92caf9 Compare March 7, 2019 11:37

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 Mar 7, 2019

gilles-peskine-arm requested changes Mar 7, 2019

View reviewed changes

k-stachowiak previously approved these changes Mar 7, 2019

View reviewed changes

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

Patater dismissed k-stachowiak’s stale review via bb60169 March 8, 2019 11:24

Patater force-pushed the break-non-crypto-dependencies branch from e92caf9 to bb60169 Compare March 8, 2019 11:24

Patater force-pushed the break-non-crypto-dependencies branch from bb60169 to 27e5907 Compare March 8, 2019 15:34

Patater mentioned this pull request Mar 11, 2019

tests: Test crypto via the crypto submodule Mbed-TLS/mbedtls#2509

Merged

2 tasks

gilles-peskine-arm previously approved these changes Mar 11, 2019

View reviewed changes

Patater added 2 commits March 11, 2019 16:46

programs/Makefile: List all programs one by one

30fae8e

This makes it easier to add or remove programs as well as see which programs were added or removed in diffs.

configs: Update example PSA config

bce557d

Our default configuration file, include/mbedtls/config.h, should always match configs/config-psa-crypto.h. It had gotten out of sync, so put it back into sync.

Patater added 15 commits March 11, 2019 16:46

pkey: Remove dependency on X.509

bf564c7

md: Remove dependency on X.509

ebbc5f7

ecp: Remove dependency on TLS and X.509

de0a41b

dhm: Remove dependency on TLS

ed16ca7

all.sh: Remove dependency on TLS, NET, and X.509

9b90f2e

cpp_dummy_build: Remove X.509 dependency

4c1fdb5

recursion.pl: Don't depend on X.509

e23737c

programs: psa: Remove dependency on platform.h

fa30c33

platform.h should only be used internally by the library implementation itself, not the examples. Remove the dependency on platform.h from all PSA programs.

cpp_dummy_build: Remove dependency on compat-1.3.h

2b725ef

Remove tests that depend on TLS or X.509

9afb2e9

tests: Add a crypto prefix to submodule tests

b478bb6

Prepend ".crypto" to tests that came from the crypto submodule. This allows, when this project is used as a submodule, for tests with names the same between the parent and this project when used as a submodule to both be built and run.

Patater dismissed gilles-peskine-arm’s stale review via b478bb6 March 11, 2019 18:13

Patater force-pushed the break-non-crypto-dependencies branch from 27e5907 to b478bb6 Compare March 11, 2019 18:13

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 Mar 11, 2019

k-stachowiak approved these changes Mar 12, 2019

View reviewed changes

gilles-peskine-arm approved these changes Mar 12, 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 Mar 13, 2019

Patater merged commit a52c059 into ARMmbed:development Mar 13, 2019

gilles-peskine-arm mentioned this pull request Feb 11, 2020

Side-port from crypto: programs/ Mbed-TLS/mbedtls#3038

Merged

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 Jan 18, 2022

PSA: example programs for HMAC and AEAD vs legacy Mbed-TLS/mbedtls#5436

Merged

Break non-crypto dependencies #74

Break non-crypto dependencies #74

Uh oh!

Conversation

Patater commented Mar 1, 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!

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

Uh oh!

Uh oh!

Patater commented Mar 5, 2019

Uh oh!

Patater commented Mar 5, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k-stachowiak 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!

k-stachowiak Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

Patater commented Mar 7, 2019

Uh oh!

Patater commented Mar 8, 2019

Uh oh!

Patater commented Mar 8, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Patater commented Mar 11, 2019

Uh oh!

Patater commented Mar 11, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Mar 12, 2019

Uh oh!

Patater commented Mar 13, 2019

Uh oh!

Uh oh!

gilles-peskine-arm left a comment •

edited

Loading

Patater commented Mar 4, 2019 •

edited

Loading

k-stachowiak left a comment •

edited

Loading