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

[gilles-peskine-arm]

Revert the commits in mbed-crypto that remove X.509 and TLS files and associated code. Make some additional minor changes to prepare for merging mbed-crypto into mbedtls.

This pull request is not meant to be merged as such. It is a precursor for the repository merge. It will be merged only into mbedtls. See Mbed-TLS/mbedtls#3078

Reversals

Revert the following commits.

b8e4ae18cf24644fa8daea6add26ad33aa1e52a7 Remove certs.h
7242ea688a9c7b1702dd41a026e921a696a5e0e2 config: Remove explicit ciphersuite lists
dfcf84aea5413ef7c8bc1f30a972ba4ab04bc22b tests: Update generator with Mbed Crypto comments
32577734e2635da3684d03ad04ba07044775cef9 doxygen: Update for Mbed Crypto
ed05b29ea335dd12415b40570b31b08fa8c8bd09 scripts: Remove unneeded scripts
ef24980e667debd0cb8f1f26218c452bacbbe084 Remove unused test data files
356acc82ad413dfec8d49745793e94a2e2f4c69e scripts: Remove dependency on NET
43a450c858c4b4d681fc3cb695622fe8fd05c66a scripts: Remove dependency on X.509
b58ff9541ba6ce14d34215f8e40d3c0d90ade268 scripts: Remove dependency on TLS
a4308b29a42a00fcbffa7d6d041946feeddc0ce9 Remove unused TLS, NET, and X.509 files
bb1f70121218b461a4197224d547e6bcfae4f991 config: Remove X.509 options
1c66e48670b64b2ac598576cc08df3a715f3957b config: Remove TLS and NET options
7fcc7bc57699ce57fef8e590a0fb502ea6f65c0e check-names: Enable referencing Mbed TLS macros
1ad37309e4f17d73c2f22c3ff4bffe2523abe17c Remove irrelevant configs
8298d70beecb6c3c1a375954e03f4ed1a80efc0a Only build libmbedcrypto
986a15199d40f354d467144f0c55ced36d161c1a programs, tests: Depend only on libmbedcrypto
0688e4f2668dab8ad95b734c23b35977134a6d21 Remove programs that depend on TLS or X.509
d874a1fd14bdf3df8ee232f539ac613adaae648c Remove zlib
d832f187f756079552601867348d924582bf65de Remove pkcs11-helper option
b478bb6ddbb1f3b7969ad9d6ccfdb0fa6d4843bd tests: Add a crypto prefix to submodule tests
1264c2a86f0b578b6f82a4c1993a22cbbe956a27 tests: Exclude version suite when used as a submodule
120d571e8e835afde4a5c31fdc26c2452c0b54d7 tests: Use parent module includes when used as a submodule
9afb2e992136db3fae9a669c3faaf6d5d27602a8 Remove tests that depend on TLS or X.509
e23737c618e93c99143bbe8343f3df4c4888ddc8 recursion.pl: Don't depend on X.509
4c1fdb51292bbe0450dee6f7e3e794fd498635ec cpp_dummy_build: Remove X.509 dependency
d8087713aea2bf3d61bb2470a8d74409e74907fb asn1: Remove dependency on X.509
9b90f2e294970ade3e4aa94879a19470f2c052e0 all.sh: Remove dependency on TLS, NET, and X.509
ed16ca7b63a13358d62f1ad6882ec60fd92158e3 dhm: Remove dependency on TLS
de0a41b716ae4d9e938236771d49a880480eb66e ecp: Remove dependency on TLS and X.509
ebbc5f7940e5271d3cdd31818119d558ba040155 md: Remove dependency on X.509
bf564c77fa97e67ac577d28258918ba29cde6af3 pkey: Remove dependency on X.509
47a3635fc7107c7d838816475c6c816d9b47f047 selftest: Remove X.509 selftest
bea98b458136029c2585037c74c114ddc5af896e Remove Diffie-Hellman examples

The list is in reverse order of application (each commit is a descendent of the following one). To verify, copy the list and:

paste <(xsel | cut -d ' ' -f 1) <(echo; xsel | cut -d ' ' -f 1) | tail -n +2 | head -n -1 | while read c1 c2; do git merge-base --is-ancestor $c1 $c2 || echo $c1 is not an ancestor of $c2; done

They are from the following pull requests:

• #173: Prepare for removing crypto from mbedtls. Merged 2019-07-19. Only one commit is applicable:
  • "Remove certs.h"
• #71: Remove non-crypto code. Merged 2019-04-30. Exclusions:
  • "config: Enable using ARIA-GCM without other ciphers": bug fix.
  • "config: Simplify incorrect GCM comment": bug fix.
  • "Remove ChangeLog": reverting it doesn't make the merge simpler.
• #74: Break non-crypto dependencies. Merged 2019-03-13. Exclusions:
  • "programs: psa: Remove dependency on platform.h": bug fix.
  • "query_config: Move to programs/test": improvement. Side-ported to tls.
  • "pkey/rsa_genkey: Remove commented out code": bug fix.
  • "configs: Update example PSA config": no need to change this.
  • "programs/Makefile: List all programs one by one": improvement. Side-ported to tls.
• #80: Remove Diffie-Hellman examples. Merged 2019-03-07. Complete (single commit).

Additional minor changes

• Link test programs that only use platform functions with mbedcrypto — Done to avoid reverting an improvement made in Side-port from crypto: programs/ Mbed-TLS/mbedtls#3038.
• Invoke config.py instead of config.pl in reverted content — This will reduce the noise when merging the mbed-crypto all.sh with the mbedtls one.
• DHM functions are not part of x509 — This was actually a fortuitous minor bug fix in the documentation that we got for free as part of removing x509 from crypto. Spotted by Manuel.

History

• unremove-non-crypto-1 was a first set of reversals to see whether this strategy was sensible.
• unremove-non-crypto-2 added a few more reversals and supplemental tweaks after gaining experience from merging the first unremove-non-crypto-1.
• unremove-non-crypto-3 reverted the same commits as unremove-non-crypto-2, but in reverse chronological order (the previous ones were close to reverse chronological order, but not exactly so). It was missing the supplemental tweaks from unremove-non-crypto-2.
• unremove-non-crypto-4 is unremove-non-crypto-3 rebased on top of a more recent mbed-crypto/development, which some slight changes, plus the supplemental tweaks from unremove-non-crypto-2 and a few more. The slight changes in the reversal are:
  • As noted by Manuel, the reversal of "config: Remove TLS and NET options" has an incorrect merege where I had added an older version of MBEDTLS_PARAM_FAILED after the newer one. This time I only keep the latest version.
  • Due to the change of base, the reversal of "all.sh: Remove dependency on TLS, NET, and X.509" had a slightly different conflict resolution.

Goals

This pull request is intended for review purposes only.

This pull request is not intended for merging. The result builds under Linux with make or cmake in the default configuration, and the crypto tests pass in the default configuration. Non-default configurations and other platforms may or may not build. X509 and TLS tests are not expected to pass.

[mpg]

As noted by Manuel, the reversal of "config: Remove TLS and NET options" has an incorrect merge where I had added an older version of MBEDTLS_PARAM_FAILED after the newer one. This time I only keep the latest version.

I'm afraid I'm still seeing the two versions: https://github.com/gilles-peskine-arm/mbed-crypto/blob/unremove-non-crypto-4/include/mbedtls/config.h#L3359 Is git rerere playing tricks on you?

[mpg]

In case it might help others who want to reproduce the unremoval, here's the script I used to reproduce it myself.

#!/bin/sh

# Usage:
# 1. git checkout -b my-unremove-branch-name base-branch-or-commit
# 2. Run this script with no arguments
# 3. Stops on conflicts?
#       yes: resolve & commit, then back to step 2
#       no: done

set -eu

COMMITS='
b8e4ae18cf24644fa8daea6add26ad33aa1e52a7
7242ea688a9c7b1702dd41a026e921a696a5e0e2
dfcf84aea5413ef7c8bc1f30a972ba4ab04bc22b
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
'

p=''
for c in $COMMITS; do
    if [ -n "$p" ]; then
        if ! git merge-base --is-ancestor $c $p; then
            echo "$c is not an ancestor of $p"
            return 1
        fi
    fi
    p=$c
done

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"

[mpg]

I reproduced the unremoval based on the current development branch in mbed-crypto, and compared the result to your branch. All differences were insignificant (ordering of concurrent additions in ordering-insensitive contexts) except for the duplicated paragraph in config.h mentioned above.

I also reviewed the three final commits and I'm happy with them.

[gilles-peskine-arm]

I fixed the duplication in config.h. I couldn't find the problem… I'd just forgotten to push my latest version 🙀

[mpg]

Thanks for fixing the duplication. Looks good to me now, as a basis for merging back to mbedtls.

[ronald-cron-arm]

In:

Revert "scripts: Remove dependency on NET"

This reverts commit 356acc8.

Conflicts:

• scripts/generate_errors.pl: a line consecutive to a changed line has
  independently changed in the meantime. Just revert the change done
  in the commit that's being reverted.

This is not a line consecutive to a changed line but a line preceding a changed line it seems.

[gilles-peskine-arm]

@ronald-cron-arm I don't understand: a line immediately preceding another line is consecutive to it, so in what way is the comment incorrect?

[mpg]

@gilles-peskine-arm Perhaps "adjacent" would be less ambiguous, as "consecutive" seems to imply an idea of one thing following the other (hence coming after it, not before it): https://dictionary.cambridge.org/dictionary/english/consecutive

[mpg]

@ronald-cron-arm Btw I gave more details on my review strategy for an earlier version of this branch here, in case you're interested: Mbed-TLS/mbedtls#3078 (review)

[ronald-cron-arm]

I've been commit by commit through the revert process and conflict resolution. This PR does what it claims to do. I've only minor comments about some commit messages and found a minor error in a cmake file change in a commit but the code containing the error is removed by the next commit.

[ronald-cron-arm]

@gilles-peskine-arm Perhaps "adjacent" would be less ambiguous, as "consecutive" seems to imply an idea of one thing following the other (hence coming after it, not before it): https://dictionary.cambridge.org/dictionary/english/consecutive

I changed "consecutive" by "adjacent".

[mpg]

I checked how the history changed compared to the version I previously approved, using:

git log --reverse --patch 3d5f05b9fb47..bd623148e358 | egrep -v '^commit|^index' > before
git log --reverse --patch 3d5f05b9fb47..43aa905d1e0c | egrep -v '^commit|^index' > after
vimdiff before after

I found that all the differences are expected and addressing one of the comments above (most about improving the commit messages, one about make one commit more correct (previously introduced an issue fixed in a later commit), except one: the addition of a blank line in a CMake file, which is harmless.

Therefore I'm happy to approve this (again, not for merging here, just as a basis for the repo merge).

[ronald-cron-arm]

The previous version of the PR ending @ bd62314 can be found here https://github.com/ronald-cron-arm/mbed-crypto/tree/unremove-non-crypto.

[ronald-cron-arm]

LGTM

[gilles-peskine-arm]

I reviewed the changes made by @ronald-cron-arm: from https://github.com/gilles-peskine-arm/mbed-crypto/tree/unremove-non-crypto-4 (bd62314) to https://github.com/gilles-peskine-arm/mbed-crypto/tree/unremove-non-crypto-5 (43aa905). The commit histories are very similar; the differences are:

• ~16 (Revert "tests: Add a crypto prefix to submodule tests") fixes a mistake that I'd made in the conflict resolution in tests/CMakeLists.txt. This code is removed in ~14 so there is no impact on the final content.
• ~14 (Revert "tests: Use parent module includes when used as a submodule") has an extra blank line in tests/CMakeLists.txt which is unimportant.
• There are a few minor changes to commit messages which are fine.

for n in {30..0}; do diff -u -I '^commit ' --label unremove-non-crypto-4~$n --label unremove-non-crypto-5~$n <(git show unremove-non-crypto-4~$n) <(git show unremove-non-crypto-5~$n); done

I hereby approve the changes made by Ronald.

[gilles-peskine-arm]

This has been merged into Mbed TLS via Mbed-TLS/mbedtls#3085