New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the config ajustment script of TLS for ENTROPY_NV_SEED #7877

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
7 participants
@TomoYamanaka
Contributor

TomoYamanaka commented Aug 24, 2018

Description

I think that ENTROPY_NV_SEED is one of "entropy", but it doesn't included to the "!defined" lineup in the following config file.
mbed-os\features\mbedtls\inc\mbedtls\config.h
Therefore, when MBEDTLS_ENTROPY_NV_SEED is defined, it is accidently invoked mbedtls/config-no-entropy.h at line 40.

I think that it should go through line 47 in the case of MBEDTLS_ENTROPY_NV_SEED.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change
@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Aug 24, 2018

@andcor02 - please review.

@0xc0170 0xc0170 requested review from andcor02 and ARMmbed/mbed-os-tls Aug 24, 2018

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Aug 29, 2018

How is it going?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 29, 2018

@k-stachowiak Can you review or someone from your team?

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Aug 29, 2018

Should this actually be done to the upstream MbedTLS repo, rather than Mbed OS? MbedTLS releases are done from the MbedTLS repo.

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Aug 29, 2018

This is changing code that's automatically generated by the Mbed TLS import script which you can see here.

You need to change the import script not the file itself.

cc: @RonEld, @dreemkiller

@sbutcher-arm

This changes the wrong code.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 29, 2018

@TomoYamanaka TomoYamanaka force-pushed the TomoYamanaka:improve_nv_seed_of_tls branch from 8f4f53b to 89fc0b1 Aug 31, 2018

Improve the config adjustment script for ENTROPY_NV_SEED
Although "nv_seed" is one of "entropy", it doesn't included to the "!defined" lineup in the following config file.
Therefore, when MBEDTLS_ENTROPY_NV_SEED is defined, it is accidently invoked "mbedtls/config-no-entropy.h".
mbed-os\features\mbedtls\inc\mbedtls\config.h
I think that correct processing should go to line 47, not line 40.

@TomoYamanaka TomoYamanaka force-pushed the TomoYamanaka:improve_nv_seed_of_tls branch from 89fc0b1 to 7c90a9e Aug 31, 2018

@TomoYamanaka TomoYamanaka changed the title from Improve MBEDTLS config for ENTROPY_NV_SEED to Improve the config ajustment script of TLS for ENTROPY_NV_SEED Aug 31, 2018

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Aug 31, 2018

@sbutcher-arm @JanneKiiskila
I changed my commit. Does this make sense?

"#include MBEDTLS_USER_CONFIG_FILE\n" \
"#endif\n" \
"\n" \
add_code \

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Sep 3, 2018

Contributor

Code which looks identical is marked as changed. Are there some whitespace errors in here?

This comment has been minimized.

@TomoYamanaka

TomoYamanaka Sep 3, 2018

Contributor

I do not think there is such an error. I just added spaces to align the end of line.

This comment has been minimized.

@TomoYamanaka

TomoYamanaka Sep 5, 2018

Contributor

According to the following viewer that hides whitespace changes, you can check that there is not some whitespace errors.
https://github.com/ARMmbed/mbed-os/pull/7877/files?utf8=%E2%9C%93&diff=unified&w=1
Thus, this is due to the Diff settings.

This comment has been minimized.

@RonEld

RonEld Sep 5, 2018

Contributor

I have checked on my machine, and the diff is because the alignment of the ending \ has changed:
image
This is because the addition of \\\\ at line 49 to add the " !defined(MBEDTLS_ENTROPY_NV_SEED)\n" part in line 50, i believe

This comment has been minimized.

@TomoYamanaka

TomoYamanaka Sep 5, 2018

Contributor

@RonEld Thank you for comments, it is correct.

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Sep 13, 2018

Contributor

Ok - the Mbed TLS coding standards specify 79 columns, but Mbed permit 120 and this is Mbed OS, so this is acceptable.

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Sep 3, 2018

@TomoYamanaka - The problem with this code and this area is that it needs to work for lots of different types of targets and platforms. Can you say what targets you've tested this with? A build test is all that's needed - not necessarily to test and run it.

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Sep 3, 2018

@sbutcher-arm

Can you say what targets you've tested this with?

My target is GR-PEACH that is Renesas Mbed board. Since it has not a hardware entropy(TRNG), we're considering that accessing to Mbed Cloud by utilizing NV_SEED entropy.

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Sep 10, 2018

Is there anything else required?

"#include MBEDTLS_USER_CONFIG_FILE\n" \
"#endif\n" \
"\n" \
add_code \

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Sep 13, 2018

Contributor

Ok - the Mbed TLS coding standards specify 79 columns, but Mbed permit 120 and this is Mbed OS, so this is acceptable.

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Sep 13, 2018

This is a trivial change, so I think I can accept the level of testing, assuming it passed the Mbed Cloud Client CI.

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Sep 13, 2018

I don't think the Cloud Client test would verify that configuration per se.

@0xc0170

0xc0170 approved these changes Oct 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 9, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2018

Build : SUCCESS

Build number : 3303
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7877/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Oct 10, 2018

@cmonr cmonr merged commit 0cf26eb into ARMmbed:master Oct 10, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 563 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10291 cycles (+79 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Oct 10, 2018

@TomoYamanaka TomoYamanaka deleted the TomoYamanaka:improve_nv_seed_of_tls branch Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment