Backport 2.28: Fix the detection of not-supported mechanisms in systematically generated PSA tests #9025
Conversation
Indicate which dependencies are not implemented. This makes it feasible to audit the not-implemented detection. The generated test cases that are detected as never-executed now have one or more `DEPENDENCY_NOT_IMPLEMENTED_YET_PSA_WANT_xxx` instead of a single `DEPENDENCY_NOT_IMPLEMENTED`. This does not affect when the test cases run. Signed-off-by: Gilles Peskine <[email protected]>
The output is less nice, since it no longer mostly matches the order in which symbols are used in the test case arguments. But this makes the output more canonical, so it will be easier to notice if semantic changes occur in subsequent commits. Signed-off-by: Gilles Peskine <[email protected]>
Create a class for test cases with automatically inferred dependencies, suitable for PSA crypto API test cases. As of this commit, only basic cases that use a key are supported. Subsequent commits will address negative tests and key generation tests that require fancier handling of dependencies. No change to the generated output. Signed-off-by: Gilles Peskine <[email protected]>
To determine PSA mechanisms that are not implemented, also read PSA_WANT symbols that cannot (or are not intended to) be configured independently, and thus are not listed in psa/crypto_config.h. Find those symbols in the config adjustment header mbedtls/config_psa.h. No impact on generated files yet, because hack_dependencies_not_implemented is currently only used on key types that have explicit dependencies. This will allow using hack_dependencies_not_implemented in other places, for example to handle algorithm variants like PSA_WANT_ALG_ECDSA_ANY which is inferred from PSA_WANT_ALG_ECDSA. Signed-off-by: Gilles Peskine <[email protected]>
In automatically generated PSA test cases, annotate the test cases that are expected to be never executed due to a dependency that is not implemented. This was already done for not-supported test cases and for key generation, but not for positive test cases of key usage. You can audit which mechanisms are detected as not-implemented with ``` grep -hEo 'DEPENDENCY_NOT_IMPLEMENTED_YET_\w+' tests/suites/*.data | sort -u ``` Signed-off-by: Gilles Peskine <[email protected]>
Use psa_information.TestCase for positive test cases for key generation. The caller remains responsible for tweaking dependencies for some key types (public keys for which the test is a negative case, RSA which requires an additional dependency). No change to the generated output. Signed-off-by: Gilles Peskine <[email protected]>
Use psa_information.TestCase for not-supported test cases for key import and generation. No change to the generated output. Signed-off-by: Gilles Peskine <[email protected]>
Use psa_information.TestCase for operation failure test cases. This changes the generated output in two ways: * Not-implemented mechanisms now have a `DEPENDENCY_NOT_IMPLEMENTED_YET_xxx` dependency in addition to the never-fulfilled `PSA_WANT_xxx` dependency. This does not affect when test cases run. * ECC test cases now have correct dependency symbols, e.g. `PSA_WANT_ECC_SECP_R1_192` instead of `PSA_WANT_ECC_FAMILY_SECP_R1`. This is a bug fix: ECC test cases were formerly never executed because of incorrect dependency symbols. Signed-off-by: Gilles Peskine <[email protected]>
Uniformly return PSA_ERROR_NOT_SUPPORTED if given an algorithm that includes a hash, but that hash algorithm is not supported. This will make it easier to have a uniform treatment of unsupported hashes in automatically generated tests. Signed-off-by: Gilles Peskine <[email protected]>
In automatically generated PSA test cases, we detect cryptographic mechanisms that are not implemented, and skip the corresponding test cases. Originally this detection was intended for mechanisms for which the PSA_WANT symbols were not implemented, but then it morphed into skipping mechanisms that are declared in crypto_values.h but not actually implemented. So it no longer makes sense to skip the test cases for which a negative dependency (!PSA_WANT_xxx) is not implemented. This causes more not-supported test cases to run. Signed-off-by: Gilles Peskine <[email protected]>
Move hack_dependencies_not_implemented into a class to make the file structure easier to understand and reduce the visibility of the _implemented_dependencies cache. Rename it because it's no longer a temporary hack (originally intended to work around the fact that not all PSA_WANT symbols were implemented), it's now a way to detect test cases for cryptographic mechanisms that are declared but not implemented. Internal refactoring only. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>
Allow "skipping" a test case, meaning that the test case is generated commented out. This is useful when systematically generating test cases according to certain rules, where some generated tests cannot be executed but we still want them to be visible when auditing the generation output. Signed-off-by: Gilles Peskine <[email protected]>
When we generate a test case for a mechanism that is not implemented, comment out the test case rather than giving it a never-fulfilled dependency. That way we don't create test cases that cannot be executed. Signed-off-by: Gilles Peskine <[email protected]>
If we don't exclude them from test case enumeration, then detect_not_implemented_dependencies would cause the generated test cases to be commented out, but the test case generation would fail before that because asymmetric_key_data.py doesn't include DH and DSA keys. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
bug
needs-ci
gilles-peskine-arm
changed the title
gilles-peskine-arm
changed the title
| for filename in ['include/psa/crypto_config.h', | ||
| 'include/mbedtls/config_psa.h']: |
There was a problem hiding this comment.
This isn't quite right. For example, mechanisms that are commented out in crypto_config.h will be considered as implemented, and conversely if you run this script with an edited crypto_config.h then some supported mechanisms might not be considered to be implemented. I'm considering some solutions, but unsure yet whether to apply them here or in a follow-up, because this pull request is already pretty long and I think I've reached a decent stepping point.
There was a problem hiding this comment.
I would definitely agree that this PR is long enough already - I would have said a follow up would be better at this point.
As it stands this is exactly the same as it has been for some time, although the better detection implemented in this PR will possibly cause more issues. Is it worth documenting the issue, if a fix is forthcoming soon?
There was a problem hiding this comment.
The overall issue remains #5390.
Following "PSA sign/verify: more uniform error on an unsupported hash", some error cases are detected earlier, so there is some sloppiness in test case dependencies that is not longer acceptable. * In test_suite_psa_crypto, one test case for a hash+sign algorithm now returns NOT_SUPPORTED rather than INVALID_ARGUMENT when the hash is not supported and the key is invalid. * In test_suite_psa_crypto_se_driver_hal_mocks, some test cases now error out before reaching the mocks rather than after when they attempt to use an unsupported hash. 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: * Automatically generated not-supported test cases for ECDSA now require both variants to be disabled. * Add manually written not-supported test cases for the signature operation when exactly one variant is supported. * Add manually written positive test cases for the verification operation when exactly one variant is supported. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
psa-storage-test-cases-never-supported-2.28
branch
from
7636518
to
9ffffab
Compare
gilles-peskine-arm
added
needs-review
gilles-peskine-arm
added
the
needs-reviewer
| #if defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) | ||
| if (actual_status == PSA_ERROR_INVALID_ARGUMENT) { | ||
| /* Edge case: when importing an ECC public key with an unspecified | ||
| * bit-size (as we do here), the infers the bit-size from the input. |
There was a problem hiding this comment.
Nit:
Either the code infers or we infer
| for filename in ['include/psa/crypto_config.h', | ||
| 'include/mbedtls/config_psa.h']: |
There was a problem hiding this comment.
I would definitely agree that this PR is long enough already - I would have said a follow up would be better at this point.
As it stands this is exactly the same as it has been for some time, although the better detection implemented in this PR will possibly cause more issues. Is it worth documenting the issue, if a fix is forthcoming soon?
Signed-off-by: Gilles Peskine <[email protected]>
What do you mean? I could easily refrain from generating those test cases at all (and that's how it used to work, except it was partly not implemented and partly not working as intended). I thought it was better to generate them so that if we want to look at precisely, we have it available. But I don't see why we'd want to “track” the not-executed cases — track how anyway? In particular, we have a check in the CI if we remove (or modify) generated test cases, which won't apply to generated test cases, and I don't think it should: we don't need an alert from the CI if the never-executed-but-would-be-if-we-added-an-algorithm test cases change. |
My thinking was that if test cases are commented out due to not implemented dependancies, there is surely a danger that the implementation of those dependencies is somehow missed, and the tests remain commented out (potentially forever) This, however, is based on the presumption that we intend to implement every single one of those dependancies, which is clearly not the case, so thanks for that. I was only thinking of some kind of infrequently calculated stat, similar to how we track test coverage, that sort of thing. |
My intention to avoid that is to ensure that we generate a not-supported test case for anything that's skipped because we think it's never-supported. (It was in the initial implementation of not-supported, but it had bitrotted; I've been refactoring the code a bit to reduce the risk that the two paths will diverge.) |
gilles-peskine-arm
added
the
needs-backports
gilles-peskine-arm
added
approved
Fix some false positives and some true positives in the list of test cases that are reported as not executed by
outcome_analysison the CI, among automatically generated PSA tests (test_suite_psa_crypto_generate_key.generated,test_suite_psa_crypto_not_supported.generated,test_suite_psa_crypto_op_fail.generated,test_suite_psa_crypto_storage_format.current,test_suite_psa_crypto_storage_format.v0). Part of #5390.Resolves #9167. There is no completeness objective here. I want to shrink the number of test cases from these test suites that are reported as not executed to something manageable, not necessarily resolve all of them.
Does this fix #7915? To be checked on the 3.6/development PR.
Note about 3.6/development: it's harder to check the end result there due to #8300, but you can download the outcome file and run
make generated_files && tests/scripts/analyze_outcomes.py outcomes.csv analyze_coveragelocally to take the generated files into account.Note for reviewers: the difference in generated output for the PR as a whole is pretty much incomprehensible, but if you go commit by commit, it should be ok. For "PSA test case generation: comment out always-skipped test cases", the default
git diffhas some noise, butgit diff --histogramis reasonably clean if you want to check that only expected things have changed.Status: 9ffffab is an improvement, but still leaves some test cases that are not executed. This PR fixes the code that eliminates never-supported mechanisms, but the detection of never-supported mechanisms still isn't right. At this stage, given how long it took, I propose to stop here and continue in a follow-up PR.
PR checklist