Don't test with MBEDTLS_USE_PSA_CRYPTO explicitly disabled #9610
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
gilles-peskine-arm
force-pushed
the
use_psa_crypto-no_tests_that_disable
branch
from
9ac5b2c to
59e783d
Compare
5 tasks
gilles-peskine-arm
force-pushed
the
use_psa_crypto-no_tests_that_disable
branch
from
59e783d to
8b7078b
Compare
gilles-peskine-arm
changed the title
Signed-off-by: Gilles Peskine <[email protected]>
Move the test case collection code out of check_test_cases.py and into its own module. This allows outcome analysis to depend only on the new module and not on check_test_cases.py. Signed-off-by: Gilles Peskine <[email protected]>
…mes.py Signed-off-by: Gilles Peskine <[email protected]>
The ignore list for coverage only has two test cases out of ~10000 that are currently reported as not executed. This is a drop in the sea and not useful. Remove them so that the class can be used generically. A follow-up will construct a comprehensive ignore list. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Use different names for task name, a task class and a task instance. The interpreter doesn't care, but it's less confusing for both humans and type checkers. Signed-off-by: Gilles Peskine <[email protected]>
Always have tasks_list be a list, not potentially some fancier iterable. Bypass mypy's somewhat legitimate complaint about REFERENCE and DRIVER in task_class: they could potentially be instance attributes, but we rely on them being class attributes. Python does normally guarantee their existence as class attributes (unless a derived class explicitly deletes them), but they could be overridden by an instance attribute; that's just something we don't do, so the class attribute's value is legitimate. We can't expect mypy to know that, so work around its complaint. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Place the code of outcome analysis (auxiliary functions, tasks, command line entry point) into a separate module, which will be moved to the version-independent framework repository so that it can be shared between maintained branches. Keep the branch-specific list of driver components and ignore lists in the per-repository script. We keep the executable script at `tests/scripts/analyze_outcomes.py`. It's simpler that way, because that path is hard-coded in CI scripts. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Upgrade mypy to 0.971, which is the last version that supports Python 3.6 (the oldest Python version that we currently run on the CI). This fixes the error ``` framework/scripts/mbedtls_framework/outcome_analysis.py:119: error: Incompatible return value type (got "IO[Any]", expected "TextIO") framework/scripts/mbedtls_framework/outcome_analysis.py:121: error: Incompatible return value type (got "IO[Any]", expected "TextIO") ``` As far as I can tell the fix is python/mypy#9275 which was released in mypy 0.940. Signed-off-by: Gilles Peskine <[email protected]>
mypy >=0.960 rejects macro_collector.py. Mbed-TLS/mbedtls-framework#50 We currently need mypy >=0.940, <0.960. Pick 0.942, which works, and is the system version on Ubuntu 22.04. Signed-off-by: Gilles Peskine <[email protected]>
Currently, many test cases are not executed. A follow-up pull request will take care of that. In the meantime, continue allowing partial test coverage. Signed-off-by: Gilles Peskine <[email protected]>
…_analysis.py Signed-off-by: Gilles Peskine <[email protected]>
This clears more than half of the test cases that are not executed. This also captures a few negative test cases that are executed. Subsequent commits will refine the filtering. Signed-off-by: Gilles Peskine <[email protected]>
… tests Some negative tests involving unsupported mechanisms are executed, because they're testing what happens if the mechanism is unsupported. Refine the ignore list for `test_suite_psa_crypto_generate_key.generated` and `test_suite_psa_crypto_op_fail.generated` accordingly. Signed-off-by: Gilles Peskine <[email protected]>
…aised Ignore certain test cases which either should not be generated or should be executed. For each ignore list entry, link to a GitHub issue whose definition of done includes removing the entry. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Our PSA crypto implementation does not and will not support SECP224K1. Signed-off-by: Gilles Peskine <[email protected]>
For each ignore list entry, link to a GitHub issue for its resolution, except for ssl-opt Valgrind tests which we never intend to run on the CI. Signed-off-by: Gilles Peskine <[email protected]>
For each ignore list entry, link to a GitHub issue for its resolution. Signed-off-by: Gilles Peskine <[email protected]>
For each ignore list entry, link to a GitHub issue for its resolution, except for a few configurations which there is a good reason to leave uncovered. Signed-off-by: Gilles Peskine <[email protected]>
TLS only supports actual restartable ECDH with the legacy code that's going away, not with the MBEDTLS_USE_PSA_CRYPTO code that's becoming the only variant. This leaves a few test cases that validate restartable ECDH in TLS as desirable, but not currently able to pass. Signed-off-by: Gilles Peskine <[email protected]>
Should we? To be decided later. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Remove all.sh components that explicitly disable MBEDTLS_USE_PSA_CRYPTO, and for which there is another component with MBEDTLS_USE_PSA_CRYPTO enabled that does the same or more testing. This commit does not consider configurations set up by another script, and skips one component for which there is no exact equivalent component with MBEDTLS_USE_PSA_CRYPTO enabled. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
With PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE disabled, test TLS 1.3 and USE_PSA TLS 1.2. With PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE disabled, just test crypto, because the TLS code needs that to generate ephemeral ECDH keys but this is not tracked properly (the ephemeral ECDH code is only gated on having ECDH). Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
use_psa_crypto-no_tests_that_disable
branch
from
8b7078b to
7c34de0
Compare
|
I'm closing this pull request because it was only intended as a step on the way to #9614, and both steps turned out to be shorter than I expected, so a single pull request is fine. |
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.
A step towards #9630. Continued in #9614.
Continues from #9593. Ready for review once that PR has been merged and this PR has been rebased on top of the merge.
PR checklist