Skip to content
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

PSA compliance tests suite #9312

Merged
merged 13 commits into from Mar 7, 2019

Conversation

@orenc17
Copy link
Contributor

commented Jan 9, 2019

Description

Add PSA compliance tests suite.
Note: this PR has been expanded to contain the entire suite.
This PR include tests for:

  1. PSA internal-trusted-storage.
  2. PSA Crypto
  3. PSA Attestation

Relies on PRs
#9708 (merged)
#9795 (merged)
#9668 (merged)
#9822 (merged)
upcoming mbedTLS release
Note: this PR will not build without these PRs

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[X] Test update
[ ] Breaking change

Reviewers

@alzix @jaypit02 @dreemkiller

@jaypit02
Copy link

left a comment

Added review comments

TESTS/psa-compliance/test_s001/main.c Outdated

const psa_api_t psa_api = {
.framework_version = pal_ipc_framework_version,
.version = pal_ipc_version,

This comment has been minimized.

Copy link
@jaypit02

jaypit02 Jan 9, 2019

These structures are common to all tests. Therefore, these can be moved to a common file. Moving to common file is more scalable to consume any addition/deletion of an element.

Also you may want add copyright header to such files.

This comment has been minimized.

Copy link
@orenc17

orenc17 Jan 9, 2019

Author Contributor

I prefer to generate the structure for each test
That way it won't mistakenly compiled to an mbed-app and waste flash

This comment has been minimized.

Copy link
@orenc17

orenc17 Jan 10, 2019

Author Contributor

after a check I've moved the struct to the framework directory

components/TARGET_PSA/spm/spm_client.h Outdated
size_t in_len,
const psa_outvec_t *out_vec,
const psa_outvec *out_vec,

This comment has been minimized.

Copy link
@jaypit02

jaypit02 Jan 9, 2019

As per the latest PSA FF spec, out_vec parameter is no more "const". It is now:

Suggested change
const psa_outvec *out_vec,
psa_outvec *out_vec,
features/frameworks/TARGET_PSA/mbed_lib.json Outdated
@@ -0,0 +1,26 @@
{
"name": "psa-compliance",
"config": {

This comment has been minimized.

Copy link
@jaypit02

jaypit02 Jan 9, 2019

FYI- These macros definition will be available in pal_config.h in PSA compliance test suite release. Once you have pal_config.h, this json may not be required.

This comment has been minimized.

Copy link
@orenc17

orenc17 Jan 9, 2019

Author Contributor

This is the "mbed way" for configuration
We could add platform specific configuration in the future through this file

TESTS/psa-compliance/test_s001/test_s001.c Outdated
* See the License for the specific language governing permissions and
* limitations under the License.
**/
#define ITS_TEST

This comment has been minimized.

Copy link
@jaypit02

jaypit02 Jan 9, 2019

Is is possible to maintain test specific mbed_lib.json to pass ITS_TEST macro?
Idea is to avoid test editing.

This comment has been minimized.

Copy link
@orenc17

orenc17 Jan 9, 2019

Author Contributor

No

@alzix
Copy link
Contributor

left a comment

test_s001 name does not describe it actually tests PSA ITS implementation.
please rename TESTS/psa-compliance/test_s001 to TESTS/psa-compliance/psa-ist-s001 and fix the importer

@alzix

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

@orenc17 - ping

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Making a note here. I think this now relies on #9192 instead.

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

@cmonr actually like many others, it depends on #9529

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@orenc17 What is the status for this PR? The dependencies were integrated, weren't they?

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

the porting is being continued by another team.. i believe they use this PR as a base

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

@orenc17 They'll still hit the same problem once they introduce a PR.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

the porting is being continued by another team.. i believe they use this PR as a base

Shall this be closed?

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

@orenc17 This became unblocked once #9529 came in.

Any updates?

@amiraloosh

This comment has been minimized.

Copy link

commented Feb 19, 2019

Yes, we will continue with this PR. The PSA team will update tomorrow.

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Hi @cmonr , @jaypit02 @alzix,
This PR has been forced pushed with multiple changes (multiple additional tests added for attestation and crypto modules as well as its), please re-review the new version.

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Checking in, this is still waiting on two other PRs before it can progress, correct?

@cmonr cmonr added the risk: R label Feb 25, 2019

@0xc0170 0xc0170 removed the needs: review label Mar 6, 2019

@alekla01

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

exporter likely needs to be restarted, as it probably has incorrectly status as pending after the license issues.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

CI job restarted: jenkins-ci/mbed-os-ci_exporter

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Restarted CI.

Was getting odd null pointer exception issue when restarting single job.

@cmonr cmonr referenced this pull request Mar 6, 2019

Merged

Fix for secure partition #9939

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 6, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 8
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Mar 6, 2019

{
#ifndef PSA_ATTESTATION_DISABLED
const uint8_t private_key_data[] = {
0x49, 0xc9, 0xa8, 0xc1, 0x8c, 0x4b, 0x88, 0x56,

This comment was marked as resolved.

Copy link
@cmonr

cmonr Mar 7, 2019

Contributor

Where does this key come from, and how was it generated?

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

This is a hard-coded attestation key used for testing the attestation feature. it was randomly generated.
The specific key chosen shouldn't matter to the test it just needs a key to be injected before it is run (in practice each decide is expected to have it's own randomly generated key).

if (IS_TEST_FAIL(status) || IS_TEST_SKIP(status))
{
GREENTEA_TESTSUITE_RESULT(false);
return;

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 7, 2019

Contributor

Seems odd that this was needed.

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

if you mean the explicit return, you are right, it should not be needed.
It is likely an artifact from the previous implementation of the function. - Fixed

bool continue_test = true;

test_info.test_num = test_num;
if (boot.state == BOOT_NOT_EXPECTED || boot.state == BOOT_EXPECTED_CRYPTO)

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 7, 2019

Contributor

I'm probably missing something, but how would this ever not be true?

boot_t boot isn't static.

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

good point this check should be removed - fixed

mbed_val_print(PRINT_ERROR, "Call to dispatch SF failed. Status=%x\n", status_of_call);
}

pal_ipc_close(*handle);

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 7, 2019

Contributor

Why should the handle be closed here instead of outside of the function?

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

this function is called from a pointer to function in the original attestation test framework (prior to our adaptation to greentea) in the struct val_api_t. The original implementation had the calling semantic that this function frees the handle inside and we preserved this for future compatibility (had we implemented the test framework we would have done many things very differently).

}


}

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 7, 2019

Contributor
} // extern "C"

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

good point, fixed

#endif

#if defined(MBEDTLS_ENTROPY_NV_SEED) || defined(COMPONENT_PSA_SRV_IPC)
inject_entropy();

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 7, 2019

Contributor

Seems weird that this and the following line of code are indented.

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

you are right, however this file has since been replaced (git move) with the file pal_mbed_os_intf.cpp which has been refactored and no longer contains this anomaly

@cmonr cmonr added needs: review and removed ready for merge labels Mar 7, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@orenc17 A couple of questions/nits, but just looking for answers before merging.

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@cmonr i've handed over the PR to @NirSonnenschein
i'm no longer working on this PR, we kept it open for convenience

@NirSonnenschein
Copy link
Contributor

left a comment

Thanks @cmonr

#endif

#if defined(MBEDTLS_ENTROPY_NV_SEED) || defined(COMPONENT_PSA_SRV_IPC)
inject_entropy();

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

you are right, however this file has since been replaced (git move) with the file pal_mbed_os_intf.cpp which has been refactored and no longer contains this anomaly

if (IS_TEST_FAIL(status) || IS_TEST_SKIP(status))
{
GREENTEA_TESTSUITE_RESULT(false);
return;

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

if you mean the explicit return, you are right, it should not be needed.
It is likely an artifact from the previous implementation of the function. - Fixed

bool continue_test = true;

test_info.test_num = test_num;
if (boot.state == BOOT_NOT_EXPECTED || boot.state == BOOT_EXPECTED_CRYPTO)

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

good point this check should be removed - fixed

{
#ifndef PSA_ATTESTATION_DISABLED
const uint8_t private_key_data[] = {
0x49, 0xc9, 0xa8, 0xc1, 0x8c, 0x4b, 0x88, 0x56,

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

This is a hard-coded attestation key used for testing the attestation feature. it was randomly generated.
The specific key chosen shouldn't matter to the test it just needs a key to be injected before it is run (in practice each decide is expected to have it's own randomly generated key).

mbed_val_print(PRINT_ERROR, "Call to dispatch SF failed. Status=%x\n", status_of_call);
}

pal_ipc_close(*handle);

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

this function is called from a pointer to function in the original attestation test framework (prior to our adaptation to greentea) in the struct val_api_t. The original implementation had the calling semantic that this function frees the handle inside and we preserved this for future compatibility (had we implemented the test framework we would have done many things very differently).

}


}

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 7, 2019

Contributor

good point, fixed

Nir Sonnenschein
@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

restarted CI on review fixes

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Mar 7, 2019

@alekla01

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Restarted jenkins-ci/exporter

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 7, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 10
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

CI has passed again on the CR changes, @cmonr please take a look, if all is ok we can proceed.

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Mar 7, 2019

@cmonr

cmonr approved these changes Mar 7, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

#9312 (comment)

@NirSonnenschein It would be good to capture this as a comment in the file, but that can be added in a seperate PR.

Not going to block the PR on that.

@cmonr cmonr merged commit a87c7c8 into ARMmbed:master Mar 7, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9292 cycles (-885 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.