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

Add TRNG API test #7057

Merged
merged 16 commits into from Sep 26, 2018

Conversation

Projects
None yet
@theamirocohen
Contributor

theamirocohen commented May 30, 2018

Use TRNG api to construct different buffers and try to compress them, If the random data is not really random, the test will be able to compress it.
We also check that after device reset when we use TRNG the API will create different data.

We do use lzf lib to compress the random buffer, the library should be checked for copyrights violation by @ChiefBureaucraticOfficer , all files are documented and under the lzflib library.

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change
else
{
printf("compression for concatenated buffer after reset was unsuccessful");
TEST_ASSERT(false);

This comment has been minimized.

@0xc0170

0xc0170 May 30, 2018

Member

This can be ASSERT with msg (then printf not needed). I think this entire if can be turned into one assert.

Similar to: TEST_ASSERT_TRUE_MSG(compr_res < buffer_len , "failed......msg....here")

This comment has been minimized.

@theamirocohen

theamirocohen May 30, 2018

Contributor

Because the test contains several parts I wanted the user to have a clear view of the progress, the assert message is only shown when the test fails.

(unsigned char **)htab);

if (comp_res >= BUFFER_LEN)
{

This comment has been minimized.

@0xc0170

0xc0170 May 30, 2018

Member

Can you fix formatting (follow mbed OS code style) , this one ({) should be attached

This comment has been minimized.

@theamirocohen

theamirocohen May 30, 2018

Contributor

Sorry for that, all formatting will be fixed

@@ -0,0 +1,16 @@
#include <stdint.h>

This comment has been minimized.

@0xc0170

0xc0170 May 30, 2018

Member

license header missing in some files (please review), not only this one

This comment has been minimized.

@theamirocohen

theamirocohen May 30, 2018

Contributor

Sorry for that, all license header will be added.

@0xc0170 0xc0170 added the needs: work label May 30, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 30, 2018

From the changes, looks like mbed tls and strorage teams should review this PR?

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented May 30, 2018

This appears to have a GPL on it. @0xc0170

@theamirocohen

This comment has been minimized.

Contributor

theamirocohen commented May 30, 2018

@ARMmbed/mbed-os-storage
@ARMmbed/mbed-os-tls
Hope you can help review PR

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented May 30, 2018

GPL is still present in this PR. We cannot integrate GPL code with Mbed OS.

@theamirocohen

This comment has been minimized.

Contributor

theamirocohen commented Jun 4, 2018

Compression lib was changed to pithy (with BSD license)

@theamirocohen

This comment has been minimized.

Contributor

theamirocohen commented Jun 4, 2018

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Jun 4, 2018

You need @ARMmbed/mbed-os-crypto to review this, not @ARMmbed/mbed-os-tls!

As seems to be common these days, we may be duplicating work, and I believe the Crypto team were planning on providing their own statistical measurement of the TRNG.

cc: @Patater, @yanesca

@adbridge adbridge requested a review from ARMmbed/mbed-os-crypto Jun 4, 2018

License headers added.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 18, 2018

@ARMmbed/mbed-os-crypto Mind reviewing?

@cmonr cmonr removed the request for review from ChiefBureaucraticOfficer Jun 18, 2018

@yanesca

This comment has been minimized.

Contributor

yanesca commented Jun 19, 2018

@cmonr We'll review it shortly! Could you please add the "TLS" label to the PR?

@cmonr cmonr requested a review from ARMmbed/mbed-os-tls Jun 19, 2018

@yanesca

Looks good to me!

@mohammad1603 will be giving a review too and we need his approval before merging.

@0xc0170 0xc0170 requested a review from mohammad1603 Jun 22, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 22, 2018

Thansk for the heads up @yanesca

@mohammad1603

looks good to me, some minor issues

"""
This script is the host script for trng test sequence, it send the
step signaling sequence and receive and transmit data to the device after
reset if necesarry (default lading and storing while reseting the device

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

typo: lading -> loading

This script is the host script for trng test sequence, it send the
step signaling sequence and receive and transmit data to the device after
reset if necesarry (default lading and storing while reseting the device
is NVstore, in case NVstore isn't enabled we'll use current infrastructure,

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

what do you mean "device is NVstore" ?

This comment has been minimized.

@theamirocohen

theamirocohen Jul 8, 2018

Contributor

"default lading and storing while reseting the device is NVstore", the device is the device used for testing, the NVstore is one way of storing and loading data


wait_for_communication = yield

if self.reset == False:

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

do you expect the device to reset after step1 (dummy message) ?

This comment has been minimized.

@theamirocohen

theamirocohen Jul 8, 2018

Contributor

Yes, If not an error will occur.

return BASE64_INVALID_PARAMETER;
}

if ((base64 >= 'A') && (base64 <= 'Z'))

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

Style (minor): mixed style of using {.

if (a){
xx;
}

AND

if (a)
{ xx;  }
}

*writePtr = 0;
while (( string[currPos] != 0 ) &&

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

i think you should switch between the first two conditions - the current code can cause an out of bounds access.
in case the string doesn't end with 0 you can see this behavior.
should be:

while (( currPos < stringMaxSize ) &&
( string[currPos] != 0 ) &&
( writePtr  <  bufferEnd ) &&
( !isEndOfString )) {
( !isEndOfString )) {
uint8_t val;

if (string[currPos] == 0 || currPos >= stringMaxSize)

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

in the while you validate this, you can avoid this checks (they will always be true)

// Check if additional bytes should have been processed but buffer isn't sufficient.
if (( result == BASE64_SUCCESS ) &&
( !isEndOfString ) &&
( string[currPos] != '=' ) &&

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

if currPos == stringMaxSize this can be a problem as above

/*
* The test is based on the assumption that trng will generate random data, random so
* there will not be any similar patterns in it, that kind of data will be impossible to
* compress, if compression will acuur the test will result in failure.

This comment has been minimized.

@mohammad1603

mohammad1603 Jun 23, 2018

Contributor

typo: acuur -> occur

@dannybenor

Before approving this PR, TPIP must be approved by Aaron

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 25, 2018

@dannybenor TPIP?

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented Jun 25, 2018

@cmonr - TPIP == Third Party IP. This looks to be BSD, which is compatible. I'll run it past legal for a TPIP review just to be sure.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

@ChiefBureaucraticOfficer Is this good to progress?

@yossi2le yossi2le force-pushed the theamirocohen:trng_test branch from 000bdab to 79e5111 Sep 6, 2018

@theamirocohen

This comment has been minimized.

Contributor

theamirocohen commented Sep 12, 2018

Hi @0xc0170 ,
In internal checks we see that K22 is running properly, the fail might have been a statistical issue, where the string did have a similarity and compressed.
Any how @yossi2le added tests prints so we could track any error if it will fail again.
Can we run morph build again please.
Thanks!

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 12, 2018

/morph test

@cmonr cmonr added needs: CI and removed needs: work labels Sep 12, 2018

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 12, 2018

Oh, I feel silly. This needed to have been a build, not a re-test.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 12, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Sep 16, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 16, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Sep 16, 2018

TRNG test seem to fail on K22F. @theamirocohen please take a look.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 25, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: work labels Sep 25, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 25, 2018

Build : SUCCESS

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

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

This comment has been minimized.

Contributor

cmonr commented Sep 26, 2018

sigh

It looks like failure was an intermittent network issue.

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit a0a9b54 into ARMmbed:master Sep 26, 2018

15 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 , RTOS ROM(+0.0%) RAM(+0.0%)
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 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 616 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9925 cycles (+849 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment