Experimental merge of Mbed Crypto #3078
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test suite generator has been a Python script for a long time, but tests/CMakeLists.txt still looked for Perl. The reference to PYTHON_INTERP only worked due to a call to find_package(PythonInterp) in the toplevel CMakeLists.txt, and cmake would not have printed the expected error message if python was not available.
Normally a valueless symbol remains valueless and a symbol with a value keeps having one. But just in case a symbol does get changed from valueless to having a value, make sure there's a space between the symbol and the value. And if a symbol gets changed from having a value to valueless, strip trailing whitespace. Add corresponding tests. Also fix the case of a valueless symbol added with the set method, which would have resulted in attempting to use None as a string. This only happened with the Python API, not with the command line API.
We currently test setting a symbol with a value even if it didn't originally had one and vice versa. So there's no need to have separate lists of symbols to test with. Just test everything we want to test with each symbol.
The tests were not covering get for a symbol with a value. No symbol has an uncommented value in the default config.h. (Actually there's _CRT_SECURE_NO_DEPRECATE, but that's a bit of a hack that this script is not expected to handle, so don't use it). Add tests of "get FOO" after "set FOO" and "set FOO value", so that we have coverage for "get FOO" when "FOO" has a value.
Replace config.pl by config.py
…rypto-development-20191115
First deal with deleted files.
* Files deleted by us: keep them deleted.
* Files deleted by them, whether modified by us or not: keep our version.
```
git rm $(git status -s | sed -n 's/^DU //p')
git reset -- $(git status -s | sed -n 's/^D //p')
git checkout -- $(git status -s | sed -n 's/^ D //p')
git add -- $(git status -s | sed -n 's/^UD //p')
```
Individual files with conflicts:
* `3rdparty/everest/library/Hacl_Curve25519_joined.c`: spurious conflict because git mistakenly identified this file as a rename. Keep our version.
* `README.md`: conflict due to their change in a paragraph that doesn't exist in our version. Keep our version of this paragraph.
* `docs/architecture/Makefile`: near-identical additions. Adapt the definition of `all_markdown` and include the clean target.
* `doxygen/input/docs_mainpage.h`: conflict in the version number. Keep our version number.
* `include/mbedtls/config.h`: two delete/modify conflicts. Keep the removed chunks out.
* `library/CMakeLists.txt`: discard all their changes as they are not relevant.
* `library/Makefile`:
* Discard the added chunk about the crypto submodule starting with `INCLUDING_FROM_MBEDTLS:=1`.
* delete/modify: keep the removed chunk out.
* library build: This is almost delete/modify. Their changes are mostly not applicable. Do keep the `libmbedcrypto.$(DLEXT): | libmbedcrypto.a` order dependency.
* `.c.o`: `-o` was added on both sides but in a different place. Change to their place.
* `library/error.c`: to be regenerated.
* `library/version_features.c`: to be regenerated.
* `programs/Makefile`: Most of the changes are not relevant. The one relevant change is in the `clean` target for Windows; adapt it by removing `/S` from our version.
* `programs/test/query_config.c`: to be regenerated.
* `scripts/config.py`: added in parallel on both sides. Keep our version.
* `scripts/footprint.sh`: parallel changes. Keep our version.
* `scripts/generate_visualc_files.pl`: one delete/modify conflict. Keep the removed chunks out.
* `tests/Makefile`: discard all of their changes.
* `tests/scripts/all.sh`:
* `pre_initialize_variables` add `append_outcome`: add it.
* `pre_initialize_variables` add `ASAN_CFLAGS`: already there, keep our version.
* `pre_parse_command_line` add `--no-append-outcome`: add it.
* `pre_parse_command_line` add `--outcome-file`: add it.
* `pre_print_configuration`: add `MBEDTLS_TEST_OUTCOME_FILE`.
* Several changes in SSL-specific components: keep our version without them.
* Several changes where `config.pl` was changed to `config.py` and there was an adjacent difference: keep our version.
* Changes regarding the inclusion of `MBEDTLS_MEMORY_xxx`: ignore them here, they will be normalized in a subsequent commit.
* `component_test_full_cmake_gcc_asan`: add it without the TLS tests.
* `component_test_no_use_psa_crypto_full_cmake_asan`: keep the fixed `msg`, discard other changes.
* `component_test_memory_buffer_allocator_backtrace`, `component_test_memory_buffer_allocator`: add them without the TLS tests.
* `component_test_m32_everest`: added in parallel on both sides. Keep our version.
* `tests/scripts/check-names.sh`, `tests/scripts/list-enum-consts.pl`, `tests/scripts/list-identifiers.sh`, ``tests/scripts/list-macros.sh`: discard all of their changes.
* `tests/scripts/test-ref-configs.pl`: the change in the conflict is not relevant, so keep our version there.
* `visualc/VS2010/*.vcxproj`: to be regenerated.
Regenerate files:
```
scripts/generate_visualc_files.pl
git add visualc/VS2010/*.vcxproj
scripts/generate_errors.pl
git add library/error.c
scripts/generate_features.pl
git add library/version_features.c
scripts/generate_query_config.pl
git add programs/test/query_config.c
```
Rejected changes in non-conflicting files:
* `CMakeLists.txt`: discard their addition which has already been side-ported.
* `doxygen/mbedtls.doxyfile`: keep the version number change. Discard the changes related to `../crypto` paths.
Keep the following changes after examination:
* `.travis.yml`: all of their changes are relevant.
* `include/mbedtls/error.h`: do keep their changes. Even though Crypto doesn't use TLS errors, it must not encroach on TLS's allocated numbers.
* `tests/scripts/check-test-cases.py`: keep the code dealing with `ssl-opt.sh`. It works correctly when the file is not present.
Enabling MBEDTLS_MEMORY_BUFFER_ALLOC_C module together with MBEDTLS_PLATFORM_MEMORY causes the library to use its own malloc replacement. This makes memory management analyzers such as ASan largely ineffective. We now test MBEDTLS_MEMORY_BUFFER_ALLOC_C separately. Disable it in the "full" config. This mirrors a change that was made in Mbed TLS on config.pl and had not been ported to Mbed Crypto yet. With this commit, config.py is aligned in Mbed Crypto and Mbed TLS.
…development-20191115 Update Mbed Crypto with latest Mbed TLS changes as of 2019-11-15
Small performance improvement of mbedtls_mpi_div_mpi()
…evelopment-restricted-merge-development-20191120 Update development-restricted with the latest development
In the CTR_DRBG module, add selftest data for when MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled. I generated the test data by running our own code. This is ok because we have other tests that ensure that the algorithm is implemented correctly. This makes programs/self/selftest pass when MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled.
The test suites should always run self-tests for all enabled features. Otherwise we miss failing self-tests in CI runs, because we don't always run the selftest program independently. There was one spurious dependency to remove: MBEDTLS_CTR_DRBG_USE_128_BIT_KEY for ctr_drbg, which was broken but has now been fixed.
This is a variant toggle, not an extra feature, so it should be tested separately. We test most of the effect of MBEDTLS_ENTROPY_FORCE_SHA256 (namely, using SHA-256 in the entropy module) when we test the library with the SHA512 module disabled (which we do at least via depends-hashes.pl). This commit removes testing of the MBEDTLS_ENTROPY_FORCE_SHA256 option itself, which should be added separately.
The size of the seedfile used by the entropy module when MBEDTLS_ENTROPY_NV_SEED is enabled is 32 byte when MBEDTLS_ENTROPY_FORCE_SHA256 is enabled or MBEDTLS_SHA512_C is disabled, and 64 bytes otherwise. A larger seedfile is ok on entry (the code just grabs the first N bytes), but a smaller seedfile is not ok. Therefore, if you run a component with a 32-byte seedfile and then a component with a 64-byte seedfile, the second component fails in the unit tests (up to test_suite_entropy which erases the seedfile and creates a fresh one). This is ok up to now because we only enable MBEDTLS_ENTROPY_NV_SEED together with MBEDTLS_ENTROPY_FORCE_SHA256. But it prevents enabling MBEDTLS_ENTROPY_NV_SEED without MBEDTLS_ENTROPY_FORCE_SHA256. To fix this, unconditionally create a seedfile before each component.
Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY and MBEDTLS_ENTROPY_FORCE_SHA256 together and separately.
FOO(BAR) is an expression, not a name. Pack expression generation into a method. No behavior change.
No need to split lines, or remove whitespace after removing whitespace. No behavior change.
No behavior change.
No behavior change.
No behavior change.
No behavior change.
No behavior change.
No behavior change.
This makes the structure of the code more apparent. No behavior change.
No behavior change.
Merge many hunks that only add lines without removing anythings. Those hunks are generally obviously ok. Of the remaining hunks, this commit excludes many hunks in `tests/scripts/all.sh` which are actually moved (with or without modification) rather than added.
* `include/mbedtls/config.h`:
* `MBEDTLS_USE_PSA_CRYPTO`: keep the documentation from tls.
* `MBEDTLS_CMAC_C`: don't enable it by default, for backward API
compatibility in mbedtls.
* `MBEDTLS_TLS_DEFAULT_ALLOW_SHA1_IN_CERTIFICATES`: keep the
more usual lack of whitespace in `//#define` from tls.
* `programs/test/cmake_subproject/CMakeLists.txt`:
* `Link against`: all the libraries.
* `tests/scripts/all.sh`:
* `cleanup`: `cmake_subproject`: remove duplicated `rm` calls
* `component_test_valgrind`: keep the better comment from tls.
* `tests/scripts/test-ref-configs.pl`:
* `config-mini-tls1_1.h`: keep `#'`
* `tests/scripts/all.sh`: discard 4 more hunks that remove `ssl-opt.sh` or `compat.sh` (in functions that have moved).
gilles-peskine-arm
added
enhancement
mbed TLS team
needs-design-approval
DO-NOT-MERGE
component-crypto
Add a changelog entry for an already-released version to indicate when the crypto submodule became mandatory. Add a changelog entry for the removal of the crypto submodule.
mpg
reviewed
Mar 4, 2020
Contributor
There was a problem hiding this comment.
So far I have reviewed up to commit 51b23f5 (tip of the unremove-noncrypto-3 branch).
Review methodology
-
I checked that 046fb66 (the base of that branch) is indeed the conflictless merge of the current tip of crypto/development and the last-but-one commit of ARMmbed/mbed-crypto#376
-
Using that commit as a base I reconstructed the branch using the following script and resolving conflicts on my own without looking at your resolution, then compared the result with your branch.
#!/bin/sh
# Usage:
# 1. git checkout -b my-unremove-branch-name base-branch-or-commit
# 2. Run this script
# 3. Stops on conflicts?
# yes: resolve & commit, then back to step 2
# no: done
set -eu
COMMITS='
b8e4ae18cf24644fa8daea6add26ad33aa1e52a7
dfcf84aea5413ef7c8bc1f30a972ba4ab04bc22b
7242ea688a9c7b1702dd41a026e921a696a5e0e2
32577734e2635da3684d03ad04ba07044775cef9
ed05b29ea335dd12415b40570b31b08fa8c8bd09
ef24980e667debd0cb8f1f26218c452bacbbe084
356acc82ad413dfec8d49745793e94a2e2f4c69e
43a450c858c4b4d681fc3cb695622fe8fd05c66a
b58ff9541ba6ce14d34215f8e40d3c0d90ade268
a4308b29a42a00fcbffa7d6d041946feeddc0ce9
bb1f70121218b461a4197224d547e6bcfae4f991
1c66e48670b64b2ac598576cc08df3a715f3957b
7fcc7bc57699ce57fef8e590a0fb502ea6f65c0e
1ad37309e4f17d73c2f22c3ff4bffe2523abe17c
8298d70beecb6c3c1a375954e03f4ed1a80efc0a
986a15199d40f354d467144f0c55ced36d161c1a
0688e4f2668dab8ad95b734c23b35977134a6d21
d874a1fd14bdf3df8ee232f539ac613adaae648c
d832f187f756079552601867348d924582bf65de
b478bb6ddbb1f3b7969ad9d6ccfdb0fa6d4843bd
1264c2a86f0b578b6f82a4c1993a22cbbe956a27
120d571e8e835afde4a5c31fdc26c2452c0b54d7
9afb2e992136db3fae9a669c3faaf6d5d27602a8
e23737c618e93c99143bbe8343f3df4c4888ddc8
4c1fdb51292bbe0450dee6f7e3e794fd498635ec
d8087713aea2bf3d61bb2470a8d74409e74907fb
9b90f2e294970ade3e4aa94879a19470f2c052e0
ed16ca7b63a13358d62f1ad6882ec60fd92158e3
de0a41b716ae4d9e938236771d49a880480eb66e
ebbc5f7940e5271d3cdd31818119d558ba040155
bf564c77fa97e67ac577d28258918ba29cde6af3
47a3635fc7107c7d838816475c6c816d9b47f047
bea98b458136029c2585037c74c114ddc5af896e
'
N_COMMITS=$(( $(printf "$COMMITS" | wc -l) - 1 ))
N=0
for c in $COMMITS; do
if git log | grep -F "This reverts commit ${c}." >/dev/null
then
N=$(( N + 1 ))
echo "Skip already reverted commit ${c} (${N}/${N_COMMITS})"
else
if git revert --no-edit $c >/dev/null; then
N=$(( N + 1 ))
echo "Successfully reverted commit ${c} (${N}/${N_COMMITS})"
else
git status
exit 1
fi
fi
done
echo "DONE"-
Tried building and running test suites with make and cmake (out-of-tree).
-
Checked the difference between
unremmove-noncrypto-2(rebased on 046fb66) andunremove-noncrypto-3 -
Reviewed commits one by one to make sure the reversals make sense. (Here I mostly reviewed the commit message of the reverted commit, had a quick glance at modified files and completely skipped whole-file additions as they're covered by reproducing the reversal myself.)
Findings
-
That part of the PR can be reviewed efficiently by a mix of reproducing the reversals and manual review of what's being reversed.
-
Everything builds and all crypto test suites pass (the only failure is in
test_suite_x509parse), as is expected at this point. -
Two new commits (not reversals) at the tip of
unremove-noncrypto-2(gilles-peskine-arm/mbed-crypto@3afbb2e and gilles-peskine-arm/mbed-crypto@d86b687) were lost when preparingunremove-noncrypto-3which I think were useful. (Note: I'll also prepare a PR against mbedtls/development to fix the remaining occurrences ofconfig.plthere.) -
Other than that, I only found one mistake in a reversal (near-duplicating a paragraph in
config.h). My other comments below are only notes to self for things I want to check later, but are not issues in theunremove-noncrypto-3branch.
|
|
||
| ${$code_check} .= "${white_space}if( use_ret == -($error_name) )\n". | ||
| "${white_space} mbedtls_snprintf( buf, buflen, \"$module_name - $description\" );\n" | ||
| if ($error_name eq "MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE") |
Contributor
There was a problem hiding this comment.
Pre-existing and out of scope: this could really use a comment, I don't have the slightest idea why this particular code needs to be special-cased. (This is just a note to myself so that I can remember to investigate & open a bug or PR later.)
| ssl/ssl_pthread_server | ||
| ssl/ssl_server | ||
| ssl/ssl_server2 | ||
| ssl/mini_client |
Contributor
There was a problem hiding this comment.
Note: the list is not sorted. In the future we might want to keep these lists sorted. (Out of scope here, again just a note to self.)
| ssl/ssl_server$(EXEXT) \ | ||
| ssl/ssl_server2$(EXEXT) \ | ||
| ssl/ssl_fork_server$(EXEXT) \ | ||
| ssl/mini_client$(EXEXT) \ |
Contributor
There was a problem hiding this comment.
Note: same as above regarding sorting.
Contributor
Author
|
Contributor
Author
Thanks. I'll pick them in |
Contributor
Author
|
I'd forgotten to mention the unremoval in my original description of the PR. I've added it, together with my notes on which commits are included and which PR they're from. |
This was referenced Mar 4, 2020
Closed
Merged
Contributor
Author
|
Superseded by #3085 |
Contributor
Contributor
Author
|
@mpg Yes, thanks. Edited. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an experimental merge of Mbed Crypto into Mbed TLS. At the time of posting, I'm not aware of anything wrong with it, other than being based on slight out-of-date versions of mbed-crypto and mbedtls. However the CI and reviews are likely to reveal mistakes.
Prerequisite: ARMmbed/mbed-crypto#376 up to ARMmbed/mbed-crypto@ae463e6, i.e. excluding the reordering of functions in
tests/scripts/all.shwhich I only realized would be useful while doing the merge.The basic strategy:
unremove-non-crypto-3starting at Modify path handling in some build scripts to be closer to mbedtls ARMmbed/mbed-crypto#376. In the final merge, the starting point will bembed-crypto/development.mbedtls/development(as of a few days ago).git mergeof the crypto branch into mbedtls.This piecewise approach to merging is very useful to understand what's happening during the merge. However I think it has a downside that it doesn't let git know that all the “merge continuation” commits are really part of the merge: git believes that the initial commit is the merge and that's all there is to it. I don't know how to get the best of both worlds: maybe make one tree with the piecewise changes, another tree with the same changes in a single merge commit, and do a 3-way merge (or 2 successive merges?) of these two?
Unremoval of tls files from crypto
Revert the following commits.
The list is in reverse order of application (each commit is a descendent of the following one). To verify, copy the list and:
They are from the following pull requests: