Update with Mbed TLS development branch as of 2019-06-27 #158

2019-06-28T07:03:34-05:00

Patater commented

2019-06-28 07:03:34 -05:00

(Migrated from github.com)

Merge from Mbed TLS development as of 2019-06-27, plus two commits on top to get tests working.

dgreen-arm (Migrated from github.com) reviewed

2019-06-28 07:03:34 -05:00

Patater commented

2019-07-04 13:28:49 -05:00

(Migrated from github.com)

Tests should pass if the PR job is updated to work with MBEDTLS_USE_PSA_CRYPTO in the full configuration, which requires seedfiles. Raised PR https://github.com/ARMmbed/mbedtls-test/pull/90 for updating the PR job.

Tests should pass if the PR job is updated to work with `MBEDTLS_USE_PSA_CRYPTO` in the full configuration, which requires seedfiles. Raised PR https://github.com/ARMmbed/mbedtls-test/pull/90 for updating the PR job.

Patater commented

2019-07-04 13:30:52 -05:00

(Migrated from github.com)

Force pushed to remove commits added to help get better CI output.

gilles-peskine-arm (Migrated from github.com) requested changes

2019-07-05 08:11:02 -05:00

gilles-peskine-arm (Migrated from github.com) left a comment

I did the merge and ended up with the following differences:

See individual comments regarding config.pl.
I omitted the changes in generate_visualc_files.pl and the associated templates altogether. I see advantages both ways: keep the code simple here (we don't need the extra variable) vs keep the code as close as possible in the two projects. Since the code isn't completely identical, I'd rather do it my way, i.e. don't add INCLUDE_DIRECTORIES.
None of the changes in tests/data_files are relevant.

I did the merge and ended up with the following differences: * See individual comments regarding `config.pl`. * I omitted the changes in `generate_visualc_files.pl` and the associated templates altogether. I see advantages both ways: keep the code simple here (we don't need the extra variable) vs keep the code as close as possible in the two projects. Since the code isn't completely identical, I'd rather do it my way, i.e. don't add `INCLUDE_DIRECTORIES`. * None of the changes in `tests/data_files` are relevant.

scripts/config.pl

We can add MBEDTLS_USE_PSA_CRYPTO to full, but this should be done in a separate PR that updates all.sh accordingly, not as part of a merge.

We can add `MBEDTLS_USE_PSA_CRYPTO` to `full`, but this should be done in a separate PR that updates `all.sh` accordingly, not as part of a merge.

scripts/config.pl

We should not MBEDTLS_ECP_RESTARTABLE from full. Especially not since there's no specific test for it in all.sh, but I don't see why we'd start excluding it from full.

We should not `MBEDTLS_ECP_RESTARTABLE` from `full`. Especially not since there's no specific test for it in `all.sh`, but I don't see why we'd start excluding it from `full`.

tests/data_files/Makefile

					
				@ -51,2 +51,3 @@

					$(MBEDTLS_CERT_WRITE) is_ca=1 serial=3 request_file=test-ca.req.sha256 selfsign=1 issuer_name="C=NL,O=PolarSSL,CN=PolarSSL Test CA" issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20110212144400 not_after=20210212144400 md=SHA1 version=3 output_file=$@

				test-ca.der: test-ca.crt

				all_final += test-ca.crt

None of the changes in the directory tests/data_files are relevant. They're all changes to certificates or to keys that are only used to run tests with certificates.

None of the changes in the directory `tests/data_files` are relevant. They're all changes to certificates or to keys that are only used to run tests with certificates.

Patater (Migrated from github.com) reviewed

2019-07-05 08:30:41 -05:00

scripts/config.pl

This wasn't introduced in a merge commit, but came from upstream mbedtls where it was done in a separate PR. https://github.com/ARMmbed/mbedtls/pull/2403/commits/971dea37453c9c45a64f32d91bf4e6da73d0c047

Patater (Migrated from github.com) reviewed

2019-07-05 08:31:26 -05:00

scripts/config.pl

See upstream source commit for why: https://github.com/ARMmbed/mbedtls/pull/2403/commits/03c35a5744ffddea35ce050439ab5580ea760dbf

Patater (Migrated from github.com) reviewed

2019-07-05 08:34:23 -05:00

tests/data_files/Makefile

					
				@ -51,2 +51,3 @@

					$(MBEDTLS_CERT_WRITE) is_ca=1 serial=3 request_file=test-ca.req.sha256 selfsign=1 issuer_name="C=NL,O=PolarSSL,CN=PolarSSL Test CA" issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20110212144400 not_after=20210212144400 md=SHA1 version=3 output_file=$@

				test-ca.der: test-ca.crt

				all_final += test-ca.crt

This was a particularly hairy bit of the merge. For the sake of future merges, it's good to have this match more closely with what's in Mbed TLS.

The changes came along with upstream changes that allow testing even with PEM is disabled, which is still relevant to Mbed Crypto. See upstream PR https://github.com/ARMmbed/mbedtls/pull/2260

Note that library/certs.c is part of libmbedcrypto.[so|a] until we do an Mbed TLS 3.0.

This was a particularly hairy bit of the merge. For the sake of future merges, it's good to have this match more closely with what's in Mbed TLS. The changes came along with upstream changes that allow testing even with PEM is disabled, which is still relevant to Mbed Crypto. See upstream PR https://github.com/ARMmbed/mbedtls/pull/2260 Note that `library/certs.c` is part of libmbedcrypto.[so|a] until we do an Mbed TLS 3.0.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-07-05 08:38:05 -05:00

scripts/config.pl

Yes, but mbedtls and mbed-crypto have diverged here. We must not import the config.pl change without also adapting all.sh accordingly. As it stands, this PR completely removes testing of ECP_RESTARTABLE.

Yes, but mbedtls and mbed-crypto have diverged here. We must not import the `config.pl` change without also adapting `all.sh` accordingly. As it stands, this PR completely removes testing of ECP_RESTARTABLE.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-07-05 08:39:00 -05:00

scripts/config.pl

This is being introduced as part of the merge of mbedtls, and we should not do it as part of a merge, especially since it would cause us to stop testing ECP_RESTARTABLE.

Patater commented

2019-07-05 08:41:07 -05:00

(Migrated from github.com)

* I omitted the changes in `generate_visualc_files.pl` and the associated templates altogether. I see advantages both ways: keep the code simple here (we don't need the extra variable) vs keep the code as close as possible in the two projects. Since the code isn't completely identical, I'd rather do it my way, i.e. don't add `INCLUDE_DIRECTORIES`.

I had initially gone down the route of no changes to generate_visualc_files, but decided it'd be nicer to include some changes given that I'd like to eventually use the same generation script in both repos. I chose to keep the templates aligned, even if the code isn't 100% aligned (it is closer to aligned now, though). We may be able to share generators and templates in the future if they are the same, and it's also easier for future merges to have 1 file need resolution than all the visualc files, plus the resolution in that one file isn't as tricky any more now that the code is more similar.

> * I omitted the changes in `generate_visualc_files.pl` and the associated templates altogether. I see advantages both ways: keep the code simple here (we don't need the extra variable) vs keep the code as close as possible in the two projects. Since the code isn't completely identical, I'd rather do it my way, i.e. don't add `INCLUDE_DIRECTORIES`. I had initially gone down the route of no changes to `generate_visualc_files`, but decided it'd be nicer to include some changes given that I'd like to eventually use the same generation script in both repos. I chose to keep the templates aligned, even if the code isn't 100% aligned (it is closer to aligned now, though). We may be able to share generators and templates in the future if they are the same, and it's also easier for future merges to have 1 file need resolution than all the visualc files, plus the resolution in that one file isn't as tricky any more now that the code is more similar.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-07-05 08:44:33 -05:00

tests/data_files/Makefile

					
				@ -51,2 +51,3 @@

					$(MBEDTLS_CERT_WRITE) is_ca=1 serial=3 request_file=test-ca.req.sha256 selfsign=1 issuer_name="C=NL,O=PolarSSL,CN=PolarSSL Test CA" issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20110212144400 not_after=20210212144400 md=SHA1 version=3 output_file=$@

				test-ca.der: test-ca.crt

				all_final += test-ca.crt

Ugh, right. Even before Mbed TLS 3.0, we could move certs.c to mbedtls, and link it into libmbedcrypto when building that in mbedtls, but I guess we won't get around to doing it that way. So ok, let's keep these changes.

Ugh, right. Even before Mbed TLS 3.0, we could move `certs.c` to mbedtls, and link it into `libmbedcrypto` when building that in mbedtls, but I guess we won't get around to doing it that way. So ok, let's keep these changes.

Patater (Migrated from github.com) reviewed

2019-07-05 09:16:08 -05:00

scripts/config.pl

Upstream doesn't test with MBEDTLS_ECP_RESTARTABLE right now either. The merge would bring us parity there. I've raised https://github.com/ARMmbed/mbedtls/pull/2738 to fix this.

Upstream doesn't test with `MBEDTLS_ECP_RESTARTABLE` right now either. The merge would bring us parity there. I've raised https://github.com/ARMmbed/mbedtls/pull/2738 to fix this.

Patater (Migrated from github.com) reviewed

2019-07-05 09:17:59 -05:00

scripts/config.pl

The goal of a merge is to bring two branches together, aligning them, with the destination branch gaining all the applicable advantages and disadvantages of the source branch.

We should align on all.sh where applicable (and a test with USE_PSA_CRYPTO off would make sense for Mbed Crypto at this point in time), but it's too bad Mbed TLS currently has this disadvantage.

The goal of a merge is to bring two branches together, aligning them, with the destination branch gaining all the applicable advantages and disadvantages of the source branch. We should align on all.sh where applicable (and a test with USE_PSA_CRYPTO off would make sense for Mbed Crypto at this point in time), but it's too bad Mbed TLS currently has this disadvantage.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-07-05 09:22:08 -05:00

scripts/config.pl

mbed-crypto and mbedtls are close, but they are not identical in all points. In particular, we do have a different default configuration. I would have opposed dropping the testing of MBEDTLS_ECP_RESTARTABLE in mbedtls, and I oppose dropping it here. Let's keep this difference until the testing is in place.

mbed-crypto and mbedtls are close, but they are not identical in all points. In particular, we do have a different default configuration. I would have opposed dropping the testing of `MBEDTLS_ECP_RESTARTABLE` in mbedtls, and I oppose dropping it here. Let's keep this difference until the testing is in place.