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

Bring in changes from Mbed TLS as of 2019-07-22 #187

Merged
Patater merged 380 commits from tls-development-20190722 into development 2019-07-31 05:50:16 -05:00
Conversation 21 Commits 380 Files changed 95 +802 -608
Patater commented 2019-07-23 02:45:50 -05:00 (Migrated from github.com)

Suggested ways of reviewing:

  • Don't use GitHub
  • Compare Mbed TLS development files to the files changed in this PR
  • Compare previous version of Mbed Crypto files with the files changed in this PR
Suggested ways of reviewing: - Don't use GitHub - Compare Mbed TLS `development` files to the files changed in this PR - Compare previous version of Mbed Crypto files with the files changed in this PR - Review [this diff](https://github.com/ARMmbed/mbed-crypto/files/3430701/20190722-changes.txt) to easily see this
Patater commented 2019-07-25 08:39:21 -05:00 (Migrated from github.com)

CI failure is known issue with ABI checker script. Fix for ABI checker is here.

Ran locally and got the following output, which is a pass:

$ scripts/abi_check.py -o crypto/development -n HEAD -s identifiers --brief

Checking evolution from crypto/development (456674d585b76b2cccce8ace9f392ce985d3c8b0) to HEAD (ff4f094ee766ad465a143365c6ac1bdf1903170a)
No compatibility issues for libmbedcrypto
CI failure is known issue with ABI checker script. [Fix for ABI checker is here](https://github.com/ARMmbed/mbedtls/pull/2760/). Ran locally and got the following output, which is a pass: ``` $ scripts/abi_check.py -o crypto/development -n HEAD -s identifiers --brief Checking evolution from crypto/development (456674d585b76b2cccce8ace9f392ce985d3c8b0) to HEAD (ff4f094ee766ad465a143365c6ac1bdf1903170a) No compatibility issues for libmbedcrypto ```
gilles-peskine-arm commented 2019-07-30 07:40:59 -05:00 (Migrated from github.com)

Previous iteration: #158. Diff: github.com/ARMmbed/mbed-crypto@ac90bb77f2..ff4f094ee7

Previous iteration: #158. Diff: https://github.com/ARMmbed/mbed-crypto/compare/ac90bb77f2d5b7aae5248158ffea5ce6165b1bfd..ff4f094ee766ad465a143365c6ac1bdf1903170a
gilles-peskine-arm (Migrated from github.com) requested changes 2019-07-30 08:47:42 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

Looks almost good to me.

I did the locally merge using some cached resolutions from my previous merge at https://github.com/ARMmbed/mbed-crypto/pull/158#pullrequestreview-258388604 and ended up with some differences:

  • Some errors on my part.
  • I did not review the changes in tests/data_files extensively. If you just run make -C tests/data_files, you get a complaint about mbedtls_cert_req: not found, but it was broken before (https://github.com/ARMmbed/mbed-crypto/issues/194). When we have a baseline that this works, we can review exactly what we have in the crypto repository.
  • This time I made the choice to take on the include directory parametrization in the visualc files this time but you didn't. As we discussed last time, both approaches have their benefits and drawbacks.
  • See individual comments and questions.
Looks almost good to me. I did the locally merge using some cached resolutions from my previous merge at https://github.com/ARMmbed/mbed-crypto/pull/158#pullrequestreview-258388604 and ended up with some differences: * Some errors on my part. * I did not review the changes in `tests/data_files` extensively. If you just run `make -C tests/data_files`, you get a complaint about `mbedtls_cert_req: not found`, but it was broken before (https://github.com/ARMmbed/mbed-crypto/issues/194). When we have a baseline that this works, we can review exactly what we have in the crypto repository. * This time I made the choice to take on the include directory parametrization in the visualc files this time but you didn't. [As we discussed last time](https://github.com/ARMmbed/mbed-crypto/pull/158#issuecomment-508762573), both approaches have their benefits and drawbacks. * See individual comments and questions.
include/mbedtls/config.h
@ -300,2 +310,4 @@
//#define MBEDTLS_CHECK_PARAMS
/**
* \def MBEDTLS_CHECK_PARAMS_ASSERT
gilles-peskine-arm (Migrated from github.com) commented 2019-07-30 08:21:13 -05:00

You changed #MBEDTLS_PARAM_FAILED to MBEDTLS_PARAM_FAILED to get doxygen.sh to pass. This isn't mentioned in the commit message. Why wasn't it necessary in mbedtls?

You changed `#MBEDTLS_PARAM_FAILED` to `MBEDTLS_PARAM_FAILED` to get `doxygen.sh` to pass. This isn't mentioned in the commit message. Why wasn't it necessary in mbedtls?
include/mbedtls/config.h
@ -1007,18 +1031,27 @@
/**
gilles-peskine-arm (Migrated from github.com) commented 2019-07-30 08:21:59 -05:00

Please move back to the previous wording of the warning about PSA. In Mbed Crypto, we don't advertise PSA Crypto itself as being experimental, only mbedtls_xxx-over-PSA. And this is Mbed Crypto, not Mbed TLS.

Please move back to the previous wording of the warning about PSA. In Mbed Crypto, we don't advertise PSA Crypto itself as being experimental, only mbedtls_xxx-over-PSA. And this is Mbed Crypto, not Mbed TLS.
tests/scripts/all.sh
gilles-peskine-arm (Migrated from github.com) commented 2019-07-30 08:36:35 -05:00

The merge is correct, but why is MBEDTLS_PSA_CRYPTO_C set here in mbedtls?

The merge is correct, but why is `MBEDTLS_PSA_CRYPTO_C` set here in mbedtls?
tests/scripts/basic-in-docker.sh
@ -0,0 +1,47 @@
#!/bin/bash -eu
gilles-peskine-arm (Migrated from github.com) commented 2019-07-30 08:37:57 -05:00

Remove the calls to compat.sh, ssl-opt.sh and key-exchanges.pl. You mentioned removing compat and ssl-opt in the commit message, but didn't actually do it.

Remove the calls to `compat.sh`, `ssl-opt.sh` and `key-exchanges.pl`. You mentioned removing compat and ssl-opt in the commit message, but didn't actually do it.
Patater (Migrated from github.com) reviewed 2019-07-31 02:47:29 -05:00
include/mbedtls/config.h
@ -1007,18 +1031,27 @@
/**
Patater (Migrated from github.com) commented 2019-07-31 02:47:29 -05:00

This warning is about the mbedtls_xxx-over-PSA option, so it should be advertised as being beta/experimental. I've reworded "as the rest of Mbed TLS yet" to "as Mbed TLS yet".

This warning is about the mbedtls_xxx-over-PSA option, so it should be advertised as being beta/experimental. I've reworded "as the rest of Mbed TLS yet" to "as Mbed TLS yet".
Patater (Migrated from github.com) reviewed 2019-07-31 02:49:52 -05:00
tests/scripts/all.sh
Patater (Migrated from github.com) commented 2019-07-31 02:49:52 -05:00

The test used to be a "cmake, full config + MBEDTLS_USE_PSA_CRYPTO, ASan" as the message says, but is actually a "full minus MBEDTLS_USE_PSA_CRYPTO: run the same set of tests as basic-build-test.sh" as the comment says. I believe the scripts/config.pl set MBEDTLS_PSA_CRYPTO_C line is an artifact leftover from the test's previous purpose, same with how the message is incorrect now. This should be fixed separate from this merge.

The test used to be a "cmake, full config + MBEDTLS_USE_PSA_CRYPTO, ASan" as the message says, but is actually a "full minus MBEDTLS_USE_PSA_CRYPTO: run the same set of tests as basic-build-test.sh" as the comment says. I believe the `scripts/config.pl set MBEDTLS_PSA_CRYPTO_C` line is an artifact leftover from the test's previous purpose, same with how the message is incorrect now. This should be fixed separate from this merge.
Patater (Migrated from github.com) reviewed 2019-07-31 02:50:54 -05:00
tests/scripts/basic-in-docker.sh
@ -0,0 +1,47 @@
#!/bin/bash -eu
Patater (Migrated from github.com) commented 2019-07-31 02:50:54 -05:00

Fixed

Fixed
Patater (Migrated from github.com) reviewed 2019-07-31 03:01:21 -05:00
include/mbedtls/config.h
@ -300,2 +310,4 @@
//#define MBEDTLS_CHECK_PARAMS
/**
* \def MBEDTLS_CHECK_PARAMS_ASSERT
Patater (Migrated from github.com) commented 2019-07-31 03:01:21 -05:00

I've reviewed the doxygen/mbedtls.doxyfile and include/mbedtls/config.h in both repos and don't see why this link should work in one but not the other.

I've reviewed the doxygen/mbedtls.doxyfile and include/mbedtls/config.h in both repos and don't see why this link should work in one but not the other.
Patater commented 2019-07-31 03:08:35 -05:00 (Migrated from github.com)

This time I made not to take on the include directory parametrization in the visualc files this time but you didn't. As we discussed last time, both approaches have their benefits and drawbacks.

I am a bit confused here about what is being claimed both on your end and my end.

Last time, I made it so we could use the same visualc template file style as Mbed TLS, even if the visualc script in Mbed Crypto is not identical.

This time, I did not change the visualc generation system within Mbed Crypto.

Even when generating visualc from the top level Mbed TLS project, it isn't necessary for the template styles to match (e.g. use of INCLUDE_DIRECTORIES), as Mbed TLS will use its versions of the templates, not Crypto's. Crypto's are used only when Crypto generates its visualc files.

Which approach did you go with in your trial merge?

> This time I made not to take on the include directory parametrization in the visualc files this time but you didn't. As we discussed last time, both approaches have their benefits and drawbacks. I am a bit confused here about what is being claimed both on your end and my end. Last time, I made it so we could use the same visualc template file style as Mbed TLS, even if the visualc script in Mbed Crypto is not identical. This time, I did not change the visualc generation system within Mbed Crypto. Even when generating visualc from the top level Mbed TLS project, it isn't necessary for the template styles to match (e.g. use of INCLUDE_DIRECTORIES), as Mbed TLS will use its versions of the templates, not Crypto's. Crypto's are used only when Crypto generates its visualc files. Which approach did you go with in your trial merge?
Patater (Migrated from github.com) reviewed 2019-07-31 03:17:56 -05:00
include/mbedtls/config.h
@ -300,2 +310,4 @@
//#define MBEDTLS_CHECK_PARAMS
/**
* \def MBEDTLS_CHECK_PARAMS_ASSERT
Patater (Migrated from github.com) commented 2019-07-31 03:17:56 -05:00

Filed https://github.com/ARMmbed/mbed-crypto/issues/195 to track fixing this

Filed https://github.com/ARMmbed/mbed-crypto/issues/195 to track fixing this
Patater commented 2019-07-31 03:18:44 -05:00 (Migrated from github.com)

Force pushed to fix review feedback:

  • Update commit message to describe Doxygen bug workaround
  • Remove attempts to use ssl-opt.sh, compat.sh, key-exchanges.pl
  • Update mbedtls_xxx-over-PSA option documentation
Force pushed to fix review feedback: - Update commit message to describe Doxygen bug workaround - Remove attempts to use ssl-opt.sh, compat.sh, key-exchanges.pl - Update mbedtls_xxx-over-PSA option documentation
gilles-peskine-arm (Migrated from github.com) reviewed 2019-07-31 03:50:45 -05:00
tests/scripts/basic-in-docker.sh
@ -0,0 +1,47 @@
#!/bin/bash -eu
gilles-peskine-arm (Migrated from github.com) commented 2019-07-31 03:50:44 -05:00

In the commit message, you mention rejecting compat and ssl-opt. Please either add key-exchanges or, preferably, use a higher-level formulation like “Remove calls to SSL-related scripts from basic-in-docker.sh”.

In the commit message, you mention rejecting compat and ssl-opt. Please either add key-exchanges or, preferably, use a higher-level formulation like “Remove calls to SSL-related scripts from basic-in-docker.sh”.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-07-31 03:51:54 -05:00
include/mbedtls/config.h
@ -1007,18 +1031,27 @@
/**
gilles-peskine-arm (Migrated from github.com) commented 2019-07-31 03:51:54 -05:00

This warning is not about “the PSA Crypto API”, it's about using that API underneath mbedtls_xxx APIs. Please revert the whole paragraph.

This warning is not about “the PSA Crypto API”, it's about using that API underneath mbedtls_xxx APIs. Please revert the whole paragraph.
gilles-peskine-arm commented 2019-07-31 03:53:43 -05:00 (Migrated from github.com)

About visualc include parametrization: there was a spurious “not” in my sentence, sorry. We both exchanged approaches. But as I said, it doesn't matter: both approaches are acceptable.

About visualc include parametrization: there was a spurious “not” in my sentence, sorry. We both exchanged approaches. But as I said, it doesn't matter: both approaches are acceptable.
yanesca (Migrated from github.com) reviewed 2019-07-31 04:27:00 -05:00
yanesca (Migrated from github.com) left a comment
No description provided.
I have reviewed the diff. I couldn't find anything worth mentioning. Looks good to me (given that @gilles-peskine-arm 's feedback has been addressed).
Patater (Migrated from github.com) reviewed 2019-07-31 04:40:19 -05:00
include/mbedtls/config.h
@ -1007,18 +1031,27 @@
/**
Patater (Migrated from github.com) commented 2019-07-31 04:40:19 -05:00

I made a change to avoid documenting the PSA Crypto API as is in beta stage in documentation for MBEDTLS_PSA_CRYPTO_C (we still have this documented in our README and possibly other places)

I made a change to avoid documenting the PSA Crypto API as is in beta stage in documentation for MBEDTLS_PSA_CRYPTO_C (we still have this documented in our README and possibly other places)
Patater (Migrated from github.com) reviewed 2019-07-31 04:40:36 -05:00
tests/scripts/basic-in-docker.sh
@ -0,0 +1,47 @@
#!/bin/bash -eu
Patater (Migrated from github.com) commented 2019-07-31 04:40:35 -05:00

Fixed to be more precise

Fixed to be more precise
Patater commented 2019-07-31 04:42:00 -05:00 (Migrated from github.com)

Force pushed to address review feedback:

  • Avoid documenting the PSA Crypto API as is in beta stage in documentation for MBEDTLS_PSA_CRYPTO_C
  • Make language in commit message more precise in regards to conflict resolution in basic-in-docker.sh
Force pushed to address review feedback: - Avoid documenting the PSA Crypto API as is in beta stage in documentation for MBEDTLS_PSA_CRYPTO_C - Make language in commit message more precise in regards to conflict resolution in basic-in-docker.sh
gilles-peskine-arm (Migrated from github.com) approved these changes 2019-07-31 04:47:15 -05:00
yanesca (Migrated from github.com) approved these changes 2019-07-31 04:56:06 -05:00
yanesca (Migrated from github.com) left a comment

LGTM

LGTM
Patater commented 2019-07-31 05:49:31 -05:00 (Migrated from github.com)

CI failure is known issue with ABI checker script. Fix for ABI checker is here.

Ran locally and got the following output, which is a pass:

$ scripts/abi_check.py -o origin/development -n HEAD -s identifiers --brief
Checking evolution from origin/development (aad3dabd2be9f3c1a6cf7586b3103cacd6241242) to HEAD (8dd16909939587fff0ca635808ada94dee7fdab1)
No compatibility issues for libmbedcrypto
CI failure is known issue with ABI checker script. [Fix for ABI checker is here](https://github.com/ARMmbed/mbedtls/pull/2760/). Ran locally and got the following output, which is a pass: ``` $ scripts/abi_check.py -o origin/development -n HEAD -s identifiers --brief Checking evolution from origin/development (aad3dabd2be9f3c1a6cf7586b3103cacd6241242) to HEAD (8dd16909939587fff0ca635808ada94dee7fdab1) No compatibility issues for libmbedcrypto ```
gilles-peskine-arm (Migrated from github.com) reviewed 2019-07-31 11:41:50 -05:00
tests/scripts/all.sh
gilles-peskine-arm (Migrated from github.com) commented 2019-07-31 11:41:50 -05:00

Fixing via https://github.com/ARMmbed/mbedtls/pull/2771

Fixing via https://github.com/ARMmbed/mbedtls/pull/2771
Sign in to join this conversation.
Reviewers
No reviewers
gilles-peskine-arm
yanesca
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!187
No description provided.