Implemented a test driver for opaque signatures without storage as defined in issue #3343. #3526
Conversation
|
Requesting review from @gilles-peskine-arm @ronald-cron-arm |
tests/src/opaque_test_driver.c
Outdated
| if( status != PSA_SUCCESS ) | ||
| return( status ); | ||
|
|
||
| status = psa_import_key( attributes, |
There was a problem hiding this comment.
Did you test this when this driver is spliced to the PSA APIs? I haven't looked at it thoroughly but I would expect this call to fail as it imports a key that has already been imported. What am I missing?
There was a problem hiding this comment.
At one point I thought that the objective of this pull request was to write a driver that performs negative testing as well as positive testing on the driver dispatch code. Then I realized that it was too early for that and negative testing is out of scope for now. I indicated in those comments that I'm leaving them up only for future reference, not as requested changes here.
include/psa/crypto_extra.h
Outdated
| /** Convert an mbed TLS error code to a PSA error code | ||
| * | ||
| * \note This function is provided solely for the convenience of | ||
| * Mbed TLS and may be removed at any time without notice. |
There was a problem hiding this comment.
I'd prefer to avoid cluttering a public header with functions that are only used in tests. Given that this is a preexisting issue, it's ok to add this function here. But we should make a follow-up that creates a separate header, possibly in library.
tests/include/opaque_test_driver.h
Outdated
| */ | ||
|
|
||
| /* | ||
| * Copyright (C) 2020, ARM Limited, All Rights Reserved |
There was a problem hiding this comment.
The copyright notice format has changed. Please copy from a file in the current development branch.
tests/scripts/all.sh
Outdated
| @@ -722,6 +722,11 @@ component_check_doxygen_warnings () { | |||
| #### Build and test many configurations and targets | |||
| ################################################################ | |||
|
|
|||
| component_test_opaque_test_driver () { | |||
| msg "build+test: Opaque test driver test" # ~ 40s | |||
| make CC=gcc CFLAGS='-Werror -Wall -Wextra -DMBEDTLS_OPAQUE_TEST_DRIVER_C' test | |||
There was a problem hiding this comment.
We should build the sample programs as well, and enable ASan(+UBSan).
| make CC=gcc CFLAGS='-Werror -Wall -Wextra -DMBEDTLS_OPAQUE_TEST_DRIVER_C' test | |
| make CC=gcc CFLAGS="-Werror -Wall -Wextra -DMBEDTLS_OPAQUE_TEST_DRIVER_C $ASAN_CFLAGS "LDFLAGS="$ASAN_CFLAGS" | |
| make test |
tests/src/opaque_test_driver.c
Outdated
| #include "test/random.h" | ||
| #include "mbedtls/error.h" | ||
| #include <string.h> | ||
| #include <ctype.h> |
There was a problem hiding this comment.
We currently don't use ctype.h and I'd prefer not to add a dependency on it. For this test, you could do something like ^ 42 on every byte.
tests/src/opaque_test_driver.c
Outdated
| char c; | ||
| while( len-- ) | ||
| { | ||
| c = (char) *in; |
There was a problem hiding this comment.
This is wrong on platforms where char is signed and uses sign-magnitude representation. Not that such platforms are likely, but in principle we do support them, and this might trip static analyzers.
| * \retval #PSA_ERROR_BUFFER_TOO_SMALL | ||
| * The size of the \p out data buffer is too small. |
There was a problem hiding this comment.
A driver import entry point shouldn't return BUFFER_TOO_SMALL. If the context is smaller than expected, that indicates a bug in the core, and the driver should convey that.
| /* Check validity of parameter set. */ | ||
| TEST_ASSERT( PSA_SUCCESS == opaque_test_driver_export_public_key( | ||
| &attr, | ||
| (uint8_t *) "OPQTDKHEADERHello world 1234", |
There was a problem hiding this comment.
Why suddenly start to hard-code strings here? Please define and use a const uint8_t[], and use sizeof rather than hard-code its size.
| 27, | ||
| &out_key_len ) ); | ||
|
|
||
| /* Check validity of parameter set. */ |
There was a problem hiding this comment.
I don't understand what this comment means.
| } | ||
| /* END_CASE */ | ||
|
|
||
| /* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ |
There was a problem hiding this comment.
depends_on:MBEDTLS_CHECK_PARAMS has no effect on PSA APIs, only on traditional Mbed TLS APIs. (It was added because traditional APIs didn't perform certain checks, but the PSA code either performs those checks or has an interface that makes them moot.) This test function is not useful.
| TEST_ASSERT( sign_size != 0 ); | ||
| TEST_ASSERT( sign_size <= PSA_SIGNATURE_MAX_SIZE ); | ||
| TEST_ASSERT( sign_size <= OPQ_BUFSIZE ); |
There was a problem hiding this comment.
This is testing the PSA_SIGN_OUTPUT_SIZE macro, which is not an objective of this test suite.
|
For reference, @kjeir is currently on vacation and moving to a different position, meaning I will take over the work. Thanks for the early feedback. Do you want to continue driving this PR to completion (meaning I need to wait for @kjeir to give me permission on his fork), or should this one be closed and followed up with a new PR from my fork? |
|
@stevew817 It's usually more convenient to keep the same PR in order to keep track of what's been reviewed and what comments have been resolved, but that's not a blocker. And if you rebase the code, keeping the same PR loses most of its advantage. So feel free to close this PR and open a new one. |
carlaageamundsen
force-pushed
the
feature/opaque_driver_pr
branch
from
31671c7 to
31962b1
Compare
|
With #3501, #3526 and #3605 PRs we have started developing two transparent test drivers and two opaque test drivers in parallel. As we go along in adding support for other driver APIs in psa_crypto.c and psa_crypto_driver_wrappers.c, it doesn't seem possible giving the time constraints to keep developing both. I see momentum in #3501 way of doing things with #3644 adding some support for ciphers. Furthermore, the testing in #3501 is the testing we definitely we need right now (exercising the test drivers and their interactions with psa_crypto.c through calls to the PSA APIs) whereas the testing in #3526 and #3605 (direct calls to the test driver APIs) is not something we need right now. For all those reasons, it would make sense to me to move the opaque test driver entry points provided by this PR to the way things are done in #3501 regarding code organization and testing. @stevew817 and @kjeir what do you think about that? |
|
@gilles-peskine-arm @ronald-cron-arm I updated this PR given your comments and now we see "Pre Test Checks failed" and "This commit cannot be built" but I am not able to see the Details on jenkins-internal.mbed.com. How should I go about to handle these issues? |
The test driver supports key import, key generation, public key export and sign/verify hash. A test suite validates the functionality. Signed-off-by: Kjell Eirik Andersen <kjell.andersen@silabs.com>
Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
…order to avoid cluttering a public header. Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
…ell, and enable ASan(+UBSan). Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
…elated macros. Replace TEST_ASSERT with PSA_ASSERT and TEST_EQUAL. Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
…e of PSA Crypto API in opaque_test_driver_sign_hash() and opaque_test_driver_verify_hash() which can not be used as is when lifetime support is added. Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
…tests. Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
…lace 'OPQ_BUFSIZE + ...' with sizeof(). Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
carlaageamundsen
force-pushed
the
feature/opaque_driver_pr
branch
from
f97109e to
d2c210c
Compare
|
@carlaageamundsen The previous run of the pr-merge job failed due to the merge conflict. If “PR-NNNN-merge Pre Test Checks” fails, that's the most likely reason. Now that you've rebased, Jenkins is running. Note that Travis has reported a build failure, you need to tweak something because of the change of where |
…and use the one in psa_crypto.c which has become global. Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
|
@gilles-peskine-arm I removed the local copy of mbedtls_to_psa_error() and use the one in psa_crypto.c which is now global. However there is still some error (see below) indicating issue in all.sh which I have not touched for a while. Do you have any ideas? https://travis-ci.org/github/ARMmbed/mbedtls/jobs/725825258 |
|
Look through the logs for where that component failed: And look backwards to see why it failed. Here it tells you: That's because you added a file in |
Description
Implemented a test driver for opaque signatures without storage.
The test driver supports key import, key generation, public key export and sign/verify hash. A test suite validates the functionality.
Status
READY
Requires Backporting
NO
Migrations
NO
Todos
Steps to test or reproduce
Added test suite test_suite_opaque_test_driver, all.sh updated with new component test_opaque_test_driver.