Switch generate_psa_test.py to automatic dependencies for negative test cases #104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gilles-peskine-arm
added
needs-work
needs-ci
5 tasks
Make the code that generates the test case be explicit about which usage(s) will be needed for key pairs (`PSA_WANT_KEY_TYPE_xxx_KEY_PAIR_uuu`). Allow more than one usage specifier. Do not systematically generalize BASIC to also include IMPORT and EXPORT: not all tests actually need this, and our test configurations don't try to have BASIC without IMPORT and EXPORT at the moment because we don't track those dependencies accurately in manually written tests anyway. Fix a bug whereby any usage other than BASIC or GENERATE led to the dependency being silently dropped. No change to the generated output. Signed-off-by: Gilles Peskine <[email protected]>
Don't always require all of BASIC, IMPORT and EXPORT. BASIC is always implied by any of the creation methods. * `KeyTypeNotSupported`: only does an IMPORT (or GENERATE) attempt. EXPORT is not needed. This reduces dependencies in `test_suite_psa_crypto_not_supported.generated.data`. * `OpFail`: only does an IMPORT, followed by a BASIC attempt. EXPORT is not needed. This reduces dependencies in `test_suite_psa_crypto_op_fail.generated.data`. * `StorageFormat`: only does an IMPORT for save (forward compatibility) tests, and only does an EXPORT for read (backward compatibility) tests. This reduces dependencies in `test_suite_psa_crypto_storage_format.current.data` and `test_suite_psa_crypto_storage_format.v0.data` respectively. Positive test cases that create and exercise a key are still potentially missing BASIC (which is implied) and EXPORT (which isn't) for exercising the key, but this is out of scope of this commit. The generated output has fewer test case dependencies as described above, with BASIC+IMPORT+EXPORT replaced by only one of IMPORT or EXPORT. Since we never test partial support for a key type with import or export disabled, this doesn't change which test cases are executed in each tested configuration. Signed-off-by: Gilles Peskine <[email protected]>
In `psa_test_case.TestCase`, add a method `assumes_not_supported` which allows using the automatic dependency calculation framework when the test case intends to run in configurations where one mechanism is not supported. Use `psa_test_case.TestCase` for not-supported test cases for key import and generation. No change to the generated output. Signed-off-by: Gilles Peskine <[email protected]>
In operation failure test cases, fix dependencies on DH or ECC groups, which were not spelled correctly and were missing the size suffix. This changes the dependencies of many test cases in `test_suite_psa_crypto_op_fail.generated.data` to no longer have a never-implemented symbol as a dependency. Thus more test cases will run. Signed-off-by: Gilles Peskine <[email protected]>
Use the automatic dependency generation mechanism from `psa_test_case.TestCase` for operation failure test cases. But tweak them explicitly to preserve the same set of (not-quite-right) dependencies, to facilitate understanding and reviewing how the current series of commits gradually changes the generated dependencies. No changes to the generated output. Signed-off-by: Gilles Peskine <[email protected]>
In automatically generated PSA test cases with automatically inferred dependencies, we were systematically skipping test cases when a dependency mentions a mechanism that is not supported, even when that dependency is negated. Fix this. This causes more not-supported test cases to run. Signed-off-by: Gilles Peskine <[email protected]>
…anisms In `OpFail` test cases, remove the temporary hack whereby test cases were not skipped when they should be due to a mechanism being never implemented. This changes many test cases in `test_suite_psa_crypto_op_fail.generated.data` to be commented out with a "skipped because" reason instead of having a dependency on an algorithm or an ECC/DH group that is not implemented. Signed-off-by: Gilles Peskine <[email protected]>
In `generate_psa_tests.py, `OpFail.make_test_case()` is only ever used with a single mechanism being not supported. Take advantage of that to simplify parts of the function. Call `psa_test_case.TestCase.assumes_not_supported()` instead of partly reinventing that wheel. No change to the generated output. Signed-off-by: Gilles Peskine <[email protected]>
ECDSA has two variants: deterministic (PSA_ALG_DETERMINISTIC_ECDSA) and randomized (PSA_ALG_ECDSA). The two variants are different for signature but identical for verification. Mbed TLS accepts either variant as the algorithm parameter for verification even when only the other variant is supported, so we need to handle this as a special case when generating not-supported test cases. In this commit, suppress generated test cases for operation failures due to unsupported ECDSA when exactly one of the two ECDSA variants is supported. This edge case will only be tested manually (done in mbedtls or TF-PSA-Crypto in the commit "Fix edge case with half-supported ECDSA (manual test cases)"). Changes to the generated output: in `test_suite_psa_crypto_op_fail.generated.data`, wherever one of `!PSA_WANT_ALG_DETERMINISTIC_ECDSA` or `!PSA_WANT_ALG_ECDSA` appears as a dependency, add the other one. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
psa-storage-test-cases-never-supported-negative-framework
branch
from
a8ec932 to
12f9495
Compare
gilles-peskine-arm
added
needs-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor and fix the the automatic generation of not-supported and operation-failure test cases.
There is one remaining bug with restartable signature which is fixed in the consuming branch.
This completes the forward port of Mbed-TLS/mbedtls#9025. The remaining commits to forward-port were 1ae57ec203a3abcecf6dd146743826d6ac26f25f, 764c2d301332dfd56980c9feeb60cf76328e2da6, 995d7d4c15406b0a115cadf3f5ec69becafdf20f and part of 9ffffab4d69f0be807a5b3b501b411222d221046. But the increasing divergence between 2.28 and 3.6+ required a different approach in several places.
Review recommendations
The commits are broken down in a way to make their impact on the generated files easy to understand.
To validate the claims in commit messages about the impact on the generated files, I use
mbedtls-trace-files.pyfrom Mbed-TLS/mbedtls-docs#23 combined with thediff-pairsscript attached here.diff-pairs.txt
From the
frameworksubdirectory with this pull request checked out, with Mbed-TLS/mbedtls#9857 (3.6) checked out in the parent directory:Then for each
NN-SHA.difffile, check that the content makes sense given the commit message for SHA.With https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/ checked out in the parent, the following invocation of
mbedtls-trace-files.pyworks:PR checklist