Incorrect dependencies for key pairs in test_suite_psa_crypto_not_supported.generated.data

[gilles-peskine-arm]

The dependencies in test_suite_psa_crypto_not_supported.generated.data (generated by generate_psa_tests.py are wrong in development. This leads to never executing the corresponding test cases.

For example, compare

scripts/config.py -f configs/config-suite-b.h
scripts/config.py set MBEDTLS_PSA_CRYPTO_C
scripts/config.py set MBEDTLS_USE_PSA_CRYPTO
make lib tests
(cd tests && ./test_suite_psa_crypto_not_supported.generated.run) | grep RSA

in 3.4.0 vs development. In development at 6aca2c96137518dbcdb1820112cf176a1b667208:

PSA import RSA_KEY_PAIR 1024-bit not supported .................... ----
PSA generate RSA_KEY_PAIR 1024-bit not supported .................. ----
PSA import RSA_KEY_PAIR 1536-bit not supported .................... ----
PSA generate RSA_KEY_PAIR 1536-bit not supported .................. ----
PSA import RSA_PUBLIC_KEY 1024-bit not supported .................. PASS
PSA import RSA_PUBLIC_KEY 1536-bit not supported .................. PASS

But all these test cases should run and pass.

[gilles-peskine-arm]

@valeriosetti @mpg This is very likely something we missed in one of the changes related to https://github.com/Mbed-TLS/mbedtls/issues/7439. The CI generates a report that lists test cases that are never executed (it's the output of the outcome_analysis job), but we never look at it because many cases are not resolved. So we need to be careful when changing test dependencies.

[mpg]

Thanks for catching this! We have a specialised script docs/architecture/psa-migration/outcome-analysis.sh that we normally run on PRs that change dependencies, in order to catch this kind of thing. I guess we forgot to run in on #7810 and/or on the final version of #7641 - we did run it on an intermediate version, though.

Perhaps in addition to this custom script we should also grab the log of the outcome_analysis step in the CI in the PR and diff it with the log from a recent nightly?

[gilles-peskine-arm]

Perhaps in addition to this custom script we should also grab the log of the outcome_analysis step in the CI in the PR and diff it with the log from a recent nightly?

In general it would be a good idea to check the effect of a PR on the output of outcome_analysis, but I confess that I tend to forget.

Getting outcome_analysis to pass with no warnings, so we can make it fail if a test case is not executed, is high on my to-do list for tech debt. Maybe 2023Q4?

[mpg]

Getting outcome_analysis to pass with no warnings, so we can make it fail if a test case is not executed, is high on my to-do list for tech debt. Maybe 2023Q4?

Fingers crossed! I think that would be a very nice improvement to our testing.

[mpg]

I believe I fixed this in the version of #7909 I just pushed.

[mpg]

I find hack_dependencies_not_implemented() a bit dangerous: I wonder if we could replace it with an explicit list of know-unimplemented dependencies, so it doesn't again kick in when we don't expect it to?

[gilles-peskine-arm]

@mpg I think that function is in fact no longer necessary, although I'm not sure if we can remove it outright. Filed as https://github.com/Mbed-TLS/mbedtls/issues/7952.

[mpg]

In current development:

cp configs/config-suite-b.h include/mbedtls/mbedtls_config.h
scripts/config.py set MBEDTLS_PSA_CRYPTO_C
scripts/config.py set MBEDTLS_USE_PSA_CRYPTO
make clean
(cd tests && make test_suite_psa_crypto_not_supported.generated && ./test_suite_psa_crypto_not_supported.generated) | grep RSA

outputs:

PSA import RSA_KEY_PAIR 1024-bit not supported .................... PASS
PSA generate RSA_KEY_PAIR 1024-bit not supported .................. PASS
PSA import RSA_KEY_PAIR 1536-bit not supported .................... PASS
PSA generate RSA_KEY_PAIR 1536-bit not supported .................. PASS
PSA import RSA_PUBLIC_KEY 1024-bit not supported .................. PASS
PSA import RSA_PUBLIC_KEY 1536-bit not supported .................. PASS

The problem was with the temporary legacy dependencies: the generation script adding a dependency on PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_LEGACY (without the MBEDTLS_ prefix that the real macro has), so hack_dependencies_not_implemented() was kicking in, resulting in all tests being skipped (even those that had a negative dependency on the non-existent symbol).

This was fixed in this commit - adding the missing MBEDLTS_ prefix, and making sure hack_dependencies_not_implemented() only kicks in on macros starting with PSA_WANT.

Additionally, the problem specifically for RSA was also fixed by #7902 as we are no longer using the _LEGACY macros anyway.

@gilles-peskine-arm I believe this issue is fully resolved and can be closed.

[gilles-peskine-arm]

RSA is ok now, but that's not from #7909 since it was actually fixed before it, looks like that was indeed from #7902.

DH is not ok since the DH tests are still skipped as of 51ed3139d1f833c0698c834e1869747f97bcd432 (merge of 7909).

The never-supported elliptic curve types are also not running. That's probably from a different hack around never-supported mechanisms, which we normally want to completely omit from generated test cases, but should be included and running in the not-supported test suite.

[mpg]

DH is not ok since the DH tests are still skipped as of 51ed313 (merge of 7909).

Ah, good point. For DH, that might be a different problem than the one we had for RSA: I think what's causing hack_dependencies_not_implemented() to kick in and add DEPENDENCY_NOT_IMPLEMENTED_YET is not !MBEDTLS_PSA_WANT_KEY_TYPE_DH_KEY_PAIR_LEGACY but rather PSA_WANT_DH_RFC7919_2048 which is indeed not defined in any header file.

It looks like we forgot to add macros for DH parameters when we added support for DH in PSA. Wdyt?

[gilles-peskine-arm]

It looks like we forgot to add macros for DH parameters when we added support for DH in PSA.

Yes, that seems to be the problem.

[mpg]

Note: I'm moving this out of the driver-only ECC EPIC, as it seems quite unrelated to it at this point.