gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

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

Closed
gilles-peskine-arm wants to merge 36 commits from unremove-non-crypto into development
pull from: unremove-non-crypto
merge into: gh-ARMmbed:development
Conversation 20 Commits 36 Files changed 468 +86659 -222
gilles-peskine-arm commented 2020-03-04 09:15:26 -06:00 (Migrated from github.com)

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 https://github.com/ARMmbed/mbedtls/pull/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 https://github.com/ARMmbed/mbedtls/pull/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.

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 https://github.com/ARMmbed/mbedtls/pull/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](https://github.com/ARMmbed/mbed-crypto/pull/173): Prepare for removing crypto from mbedtls. Merged 2019-07-19. Only one commit is applicable: * "Remove certs.h" * [#71](https://github.com/ARMmbed/mbed-crypto/pull/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](https://github.com/ARMmbed/mbed-crypto/pull/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](https://github.com/ARMmbed/mbed-crypto/pull/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 https://github.com/ARMmbed/mbedtls/pull/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](https://github.com/ARMmbed/mbedtls/pull/3078#discussion_r387514034). ### History * [`unremove-non-crypto-1`](https://github.com/gilles-peskine-arm/mbed-crypto/tree/unremove-non-crypto-1) was a first set of reversals to see whether this strategy was sensible. * [`unremove-non-crypto-2`](https://github.com/gilles-peskine-arm/mbed-crypto/tree/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`](https://github.com/gilles-peskine-arm/mbed-crypto/tree/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`](https://github.com/gilles-peskine-arm/mbed-crypto/tree/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](https://github.com/ARMmbed/mbedtls/pull/3078#discussion_r386897329), 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 commented 2020-03-05 04:14:12 -06:00 (Migrated from github.com)

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?

> 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 commented 2020-03-05 04:17:35 -06:00 (Migrated from github.com)

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"
In case it might help others who want to reproduce the unremoval, here's the script I used to reproduce it myself. ```sh #!/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 commented 2020-03-05 04:19:48 -06:00 (Migrated from github.com)

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.

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 commented 2020-03-09 04:44:02 -05:00 (Migrated from github.com)

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

I fixed the duplication in `config.h`. I couldn't find the problem… I'd just forgotten to push my latest version :scream_cat:
mpg (Migrated from github.com) reviewed 2020-03-09 05:16:30 -05:00
mpg (Migrated from github.com) left a comment
No description provided.
Thanks for fixing the duplication. Looks good to me now, as a basis for merging back to mbedtls.
ronald-cron-arm (Migrated from github.com) reviewed 2020-03-11 09:35:54 -05:00
ronald-cron-arm (Migrated from github.com) left a comment

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.

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 commented 2020-03-11 09:43:04 -05:00 (Migrated from github.com)

@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?

@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 commented 2020-03-12 03:21:37 -05:00 (Migrated from github.com)

@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

@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 commented 2020-03-12 04:47:49 -05:00 (Migrated from github.com)

@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: https://github.com/ARMmbed/mbedtls/pull/3078#pullrequestreview-367824396

@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: https://github.com/ARMmbed/mbedtls/pull/3078#pullrequestreview-367824396
ronald-cron-arm (Migrated from github.com) reviewed 2020-03-12 09:27:30 -05:00
ronald-cron-arm (Migrated from github.com) left a comment

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.

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.
include/CMakeLists.txt
@ -15,7 +15,7 @@ if(INSTALL_MBEDTLS_HEADERS)
ronald-cron-arm (Migrated from github.com) commented 2020-03-12 06:55:17 -05:00

commit message: for include/CmakeLists.txt, immediately after, not immediately before

commit message: for include/CmakeLists.txt, immediately after, not immediately before
include/mbedtls/check_config.h
@ -57,8 +57,9 @@
#endif
ronald-cron-arm (Migrated from github.com) commented 2020-03-11 11:23:01 -05:00

No conflict.

#if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT) &&
( !defined(MBEDTLS_RSA_C) || !defined(MBEDTLS_PKCS1_V21) )
#error "MBEDTLS_X509_RSASSA_PSS_SUPPORT defined, but not all prerequisites"
#endif

and

#if defined(MBEDTLS_SHA512_NO_SHA384) && !defined(MBEDTLS_SHA512_C)
#error "MBEDTLS_SHA512_NO_SHA384 defined without MBEDTLS_SHA512_C"
#endif

swapped

No conflict. #if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT) && \ ( !defined(MBEDTLS_RSA_C) || !defined(MBEDTLS_PKCS1_V21) ) #error "MBEDTLS_X509_RSASSA_PSS_SUPPORT defined, but not all prerequisites" #endif and #if defined(MBEDTLS_SHA512_NO_SHA384) && !defined(MBEDTLS_SHA512_C) #error "MBEDTLS_SHA512_NO_SHA384 defined without MBEDTLS_SHA512_C" #endif swapped
tests/CMakeLists.txt
@ -37,3 +45,3 @@
OUTPUT test_suite_${data_name}.c
COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_test_code.py -f ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${suite_name}.function -d ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${data_name}.data -t ${CMAKE_CURRENT_SOURCE_DIR}/suites/main_test.function -p ${CMAKE_CURRENT_SOURCE_DIR}/suites/host_test.function -s ${CMAKE_CURRENT_SOURCE_DIR}/suites --helpers-file ${CMAKE_CURRENT_SOURCE_DIR}/suites/helpers.function -o .
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_test_code.py mbedcrypto ${CMAKE_CURRENT_SOURCE_DIR}/suites/helpers.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/main_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/host_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${suite_name}.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${data_name}.data
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_test_code.py mbedtls ${CMAKE_CURRENT_SOURCE_DIR}/suites/helpers.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/main_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/host_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${suite_name}.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${data_name}.data
ronald-cron-arm (Migrated from github.com) commented 2020-03-12 06:26:57 -05:00

Probably need to remove the final ")" in this line

Probably need to remove the final ")" in this line
tests/scripts/test-ref-configs.pl
@ -21,1 +23,4 @@
},
'config-mini-tls1_1.h' => {
'compat' => '-m tls1_1 -f \'^DES-CBC3-SHA$\|^TLS-RSA-WITH-3DES-EDE-CBC-SHA$\'',
},
ronald-cron-arm (Migrated from github.com) commented 2020-03-12 04:36:47 -05:00

Commit message
Rather:
"Keep in the list of configurations 'config-symmetrice-only.h' that was added to the list after the removal of the configurations that are added back by this revert."?

Commit message Rather: "Keep in the list of configurations 'config-symmetrice-only.h' that was added to the list after the removal of the configurations that are added back by this revert."?
ronald-cron-arm commented 2020-03-19 07:40:17 -05:00 (Migrated from github.com)

@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".

> > > @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".
👍 1
ronald-cron-arm (Migrated from github.com) reviewed 2020-03-19 07:54:40 -05:00
include/mbedtls/check_config.h
@ -57,8 +57,9 @@
#endif
ronald-cron-arm (Migrated from github.com) commented 2020-03-19 07:54:39 -05:00

Moved the comment about MBEDTLS_X509_RSASSA_PSS_SUPPORT out of the "Conflicts" section.

Moved the comment about MBEDTLS_X509_RSASSA_PSS_SUPPORT out of the "Conflicts" section.
ronald-cron-arm (Migrated from github.com) reviewed 2020-03-19 08:10:34 -05:00
tests/scripts/test-ref-configs.pl
@ -21,1 +23,4 @@
},
'config-mini-tls1_1.h' => {
'compat' => '-m tls1_1 -f \'^DES-CBC3-SHA$\|^TLS-RSA-WITH-3DES-EDE-CBC-SHA$\'',
},
ronald-cron-arm (Migrated from github.com) commented 2020-03-19 08:10:33 -05:00

Changed:

  • config-ccm-psk-tls1_2.h: there has been an addition (of
    config-symmetric-only.h) at the place where it was removed.
    Re-add it in arbitrary order.
    by:
  • config-ccm-psk-tls1_2.h: there has been an addition of
    "config-symmetric-only.h" at the place where the configurations
    that are added back were removed. Keep this configuration.
Changed: * config-ccm-psk-tls1_2.h: there has been an addition (of config-symmetric-only.h) at the place where it was removed. Re-add it in arbitrary order. by: * config-ccm-psk-tls1_2.h: there has been an addition of "config-symmetric-only.h" at the place where the configurations that are added back were removed. Keep this configuration.
ronald-cron-arm (Migrated from github.com) reviewed 2020-03-19 08:14:28 -05:00
tests/CMakeLists.txt
@ -37,3 +45,3 @@
OUTPUT test_suite_${data_name}.c
COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_test_code.py -f ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${suite_name}.function -d ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${data_name}.data -t ${CMAKE_CURRENT_SOURCE_DIR}/suites/main_test.function -p ${CMAKE_CURRENT_SOURCE_DIR}/suites/host_test.function -s ${CMAKE_CURRENT_SOURCE_DIR}/suites --helpers-file ${CMAKE_CURRENT_SOURCE_DIR}/suites/helpers.function -o .
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_test_code.py mbedcrypto ${CMAKE_CURRENT_SOURCE_DIR}/suites/helpers.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/main_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/host_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${suite_name}.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${data_name}.data
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_test_code.py mbedtls ${CMAKE_CURRENT_SOURCE_DIR}/suites/helpers.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/main_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/host_test.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${suite_name}.function ${CMAKE_CURRENT_SOURCE_DIR}/suites/test_suite_${data_name}.data
ronald-cron-arm (Migrated from github.com) commented 2020-03-19 08:14:27 -05:00

fixed

fixed
ronald-cron-arm (Migrated from github.com) reviewed 2020-03-19 08:22:01 -05:00
include/CMakeLists.txt
@ -15,7 +15,7 @@ if(INSTALL_MBEDTLS_HEADERS)
ronald-cron-arm (Migrated from github.com) commented 2020-03-19 08:22:01 -05:00

Replaced:

  • "Make config.h available" commit: there has been a change
    immediately before where it was removed. Just re-add what was removed.
    by:
  • "Make config.h available" comment: there has been a change
    adjacent to where it was modified. Just re-add what was removed.
Replaced: * "Make config.h available" commit: there has been a change immediately before where it was removed. Just re-add what was removed. by: * "Make config.h available" comment: there has been a change adjacent to where it was modified. Just re-add what was removed.
mpg (Migrated from github.com) approved these changes 2020-03-20 03:30:22 -05:00
mpg (Migrated from github.com) left a comment

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).

I checked how the history changed compared to the version I previously approved, using: ```sh 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 commented 2020-03-20 04:21:25 -05:00 (Migrated from github.com)

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

The previous version of the PR ending @ bd623148e358 can be found here https://github.com/ronald-cron-arm/mbed-crypto/tree/unremove-non-crypto.
ronald-cron-arm (Migrated from github.com) approved these changes 2020-03-20 04:26:41 -05:00
ronald-cron-arm (Migrated from github.com) left a comment

LGTM

LGTM
gilles-peskine-arm (Migrated from github.com) reviewed 2020-03-23 10:26:21 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

I reviewed the changes made by @ronald-cron-arm: from https://github.com/gilles-peskine-arm/mbed-crypto/tree/unremove-non-crypto-4 (bd623148e3) to https://github.com/gilles-peskine-arm/mbed-crypto/tree/unremove-non-crypto-5 (43aa905d1e). 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.

I reviewed the changes made by @ronald-cron-arm: from https://github.com/gilles-peskine-arm/mbed-crypto/tree/unremove-non-crypto-4 (bd623148e3581a80647b10d349da5acc42d95b81) to https://github.com/gilles-peskine-arm/mbed-crypto/tree/unremove-non-crypto-5 (43aa905d1e0c5c3e8fbec98adc496bcd7d46b51f). 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 commented 2020-03-24 07:56:34 -05:00 (Migrated from github.com)

This has been merged into Mbed TLS via https://github.com/ARMmbed/mbedtls/pull/3085

This has been merged into Mbed TLS via https://github.com/ARMmbed/mbedtls/pull/3085

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
mpg
ronald-cron-arm
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
needs: preceding PR
Requires another PR to be merged first

  
needs: review
The pull request is ready for review. This generally means that it has no known issues.

  
needs: work
The pull request needs rework before it can be merged.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!384
No description provided.