Script to determine PSA crypto test dependencies automatically

[gilles-peskine-arm]

This pull request adds a new script tests/scripts/set_psa_test_dependencies.py which determines the PSA_WANT_xxx of PSA crypto test cases automatically.

• Script to determine PSA crypto dependencies automatically.
• Exceptions for cipher-related symbols because PSA_WANT_xxx is not implemented for them yet.
• Apply the script to test_suite_psa_crypto*.data (except for old-style SE tests).
• Ensure that the same test cases run as before in the default configuration (with excusable exceptions).
• Pass CI.
• Ensure that the same test cases run as before in the all configurations (with excusable exceptions).

Out of scope of this PR:

• Rewrite all dependencies in .function files (I only did enough to pass CI)
• Dynamic SE tests (MBEDTLS_PSA_CRYPTO_SE_C)
• MBEDTLS_USE_PSA_CRYPTO tests
• ssl-opt.sh
• Modify PSA driver testing in all.sh

---

Outcome files are useful to check whether the same test cases run as before. Here's how I compared before/after for the default configuration:

1. Download outcomes.csv.xz from a Jenkins CI run for a51e1db, which is the commit at which this PR's branch forks from development. Save the uncompressed file as a51e1dbe7611eb58593d0f63956f6eee2d489034.outcomes.csv
2. Apply the script (discarding any previous modifications to .data files):

   git checkout -- tests/suites/test_suite_psa_crypto*.data; tests/scripts/set_psa_test_dependencies.py tests/suites/test_suite_psa_crypto*.data~**/test_suite_psa_crypto_se_* && git diff tests/suites/test_suite_psa_crypto*.data
   

3. Rebuild and run the tests, with outcomes going to default.csv.

   export MBEDTLS_TEST_PLATFORM=Linux-x86_64
   rm -f default.csv; MBEDTLS_TEST_CONFIGURATION=component_test_default_cmake_gcc_asan MBEDTLS_TEST_OUTCOME_FILE=$PWD/default.csv make test
   

4. Compare the old outcomes with the new ones.

   grep '^Linux-x86_64;component_test_default_cmake_gcc_asan;test_suite_' a51e1dbe7611eb58593d0f63956f6eee2d489034.outcomes.csv | awk -F\; '{print $3 "/" $4 ";" $5 ";" $6}' | sort -t \; -k1,1 | join -t \; -j 1 -a 2 -o 1.1,1.2,2.2,1.3,2.3 - <(<default.csv awk -F\; '{print $3 "/" $4 ";" $5 ";" $6}' | sort -t \; -k1,1 | grep -v ';Unsupported suite') | awk -F\; '$2 != $3' | grep -P '(?<=/)[^;]*'
   

[gilles-peskine-arm]

Rebased 7191372 to ae48909 to change what I did in persistent_key_load_key_from_storage a bit. No point in making things complicated by testing something that's out of scope of this test function. Further changes are in new commits.

[ronald-cron-arm]

That's look great. I have a few comments.

[gilles-peskine-arm]

Her are snippets of code to compare outcomes.

1. Download the outcome files of two CI runs, such as a nightly for the commit I forked this PR from and a run of pr-head. I'll call the uncompressed outcome files a51e1dbe7611eb58593d0f63956f6eee2d489034.outcomes.csv and 3cceab1f3c4066d0a4e132e8934cab4a4c61b81f.outcomes.csv.
2. Group the first four fields which together make up the name of an individual test case run. I use tab to separate the resulting fields (and keep ; to separate subfields in the first column) so that sorting by whole lines is equivalent to sorting by columns (because the separator character comes before any field content character).

   <3cceab1f3c4066d0a4e132e8934cab4a4c61b81f.outcomes.csv awk -F ';' '{print $1 ";" $2 ";" $3 ";" $4 "\t" $5 "\t" $6}' | sort >3cceab1f3c4066d0a4e132e8934cab4a4c61b81f.sorted
   <a51e1dbe7611eb58593d0f63956f6eee2d489034.outcomes.csv awk -F ';' '{print $1 ";" $2 ";" $3 ";" $4 "\t" $5 "\t" $6}' | sort >a51e1dbe7611eb58593d0f63956f6eee2d489034.sorted
   

3. Eliminate common lines. Join the remaining records by test case names. Further filter out records where both sides skipped but with different reasons.

   join -t $'\t' -j 1 -o 1.1,1.2,2.2,2.3 <(comm -23 a51e1dbe7611eb58593d0f63956f6eee2d489034.sorted 3cceab1f3c4066d0a4e132e8934cab4a4c61b81f.sorted) <(comm -13 a51e1dbe7611eb58593d0f63956f6eee2d489034.sorted 3cceab1f3c4066d0a4e132e8934cab4a4c61b81f.sorted) | grep -v $'\tSKIP\tSKIP\t' >|diff.txt
   

4. You now have a list of actual differences between test outcomes (PASS/SKIP or SKIP/PASS). Sort it by test case first and configuration second instead of the other way round.

   <diff.txt tr '\t' ';' | sort -t ';' -k3,4 -k2,2 -k1,1 -k5 >|diff.sorted
   

5. Count how many test cases have a different outcome in at least one configuration.

   <diff.sorted awk -F ';' '{print $3 ";" $4}' |sort -u |wc -l
   

[gilles-peskine-arm]

Here's my analysis of the outcomes of 3cceab1 compared to a51e1db. See
#4012 (comment) for how to get exploitable information about the differences.

Easy cases

Formerly skipped tests now passing

These are either spurious dependencies that resulted from copypasta, or in a few cases negative tests with a dependency that doesn't really matter. All is fine.

test_suite_psa_crypto_metadata now skipped

It's unclear to me whether metadata tests should depend on support for the algorithm and key type or not. In theory the PSA_IS_xxx and PSA_GET_xxx macros might not work on unsupported algorithms. In practice, we currently don't have any optimizations on these macros that would make them fail in some configurations.

Before this PR, test_suite_psa_crypto_metadata.data usually depended on support for the mechanisms, but not always, and there were some outright mistakes (such as depending only on MBEDTLS_SHA512_C but not on !MBEDTLS_SHA512_NO_SHA384 for SHA-384). With 3cceab1, the test cases systematically depend on the mechanisms.

As a consequence, the FFDH tests do not run in any configuration (since Mbed TLS doesn't implement FFDH in the PSA API yet). This is unfortunate, but I think this is a reasonable consequence of making the dependencies consistent.

In summary, I don't intend to change anything for the sake of test_suite_psa_crypto_metadata.

test_suite_psa_crypto_persistent_key now skipped

• Storage format tests that only look at how the file is structured and don't care about the format of the key material don't depend on any cryptographic mechanisms. Added a rule in the script.

test_suite_psa_crypto formerly passing, now skipped

<diff.sorted grep -v -e ';SKIP;PASS;' | awk -F \; '$3 == "test_suite_psa_crypto" {print $4 ";" $2 ";" $7}'
<diff.sorted grep -v -e ';SKIP;PASS;' | awk -F \; '$3 == "test_suite_psa_crypto" {print $4}' | sort -u

324 record encompassing 67 test cases

• “Copy fail: RSA…” (copy_fail): the negative test cases don't actually care that the target policy is supported. Similar to negative _key_policy test cases. I added a rule about copy_fail to the script.
• “PSA decrypt: invalid algorithm”, “PSA encrypt: invalid algorithm”: the negative test cases don't actually care that the invalid algorithm is supported. Similar to negative _key_policy test cases, but here there's no particular value in running these test cases with an unsupported algorithm, so I'm excusing this difference.
• “PSA generate key: RSA, 0 bits: invalid”: this test case now has an added dependency on PKCS#1v1.5 (the algorithm in the key policy). Since we already know that this key creation fails with a valid policy, there's no particular value in running the test with an invalid policy. This is an excusable difference.
• “PSA hash compare: bad algorithm (not a hash)”, “PSA hash compute: bad algorithm (not a hash)”: same reasoning as “PSA decrypt: invalid algorithm” above. Excused.
• “PSA import/export …” and “PSA import/export-public …”: these test cases only depended on the key type and not on the algorithm in the key policy. They now also depend on the algorithm in the key policy, so the ECC test cases are now skipped when ECDSA is disabled and the RSA test cases are now skipped when PKCS#1v1.5 is disabled. This makes sense: the test cases were only passing because import_export and import_export_public didn't call exercise_key (which arguably they should have). It may be a good idea to test the import of a key with an unsupported policy (AFAIR there's no explicit mention in the PSA specification whether this should be accepted), but until then, it's ok to not run the test cases. Excused.
• “PSA key agreement setup: …”: these are negative tests for operation setup with an invalid algorithm. Similar to “PSA decrypt: invalid algorithm” above. Excused.
• “PSA key policy: ECC SECP256R1, …”: these test cases create a key with a certain policy, but don't attempt to use the key (their goal is only to check that the key has the declared policy). When the policy specifies an unsupported algorithm, the tests are no longer executed. This is similar to “PSA import/export …” above, so excused.
• “PSA key policy: asymmetric signature, …”: this key policy test case combines a positive test and negative test. It is now no longer run when attempting the negative part with an unsupported algorithm. Running the test case when the wrong algorithm is supported is enough to validate that the library code does check the algorithm, so there's little added value in also checking when the wrong algorithm is unsupported. Excused.
• “PSA key policy: raw agreement, key specifies KDF”: this test case didn't have a dependency on the algorithm in the key policy. Arguably this was ok since the test case was passing. However this was a deviation from other similar tests, so arguably it was a bug that the test wasn't skipped. Therefore this is an excusable difference.
• “PSA sign/verify: randomized ECDSA SECP256R1 SHA-256”: with randomized ECDSA, when signing or verifying a hash, the declared hash algorithm doesn't actually need to be supported. (It would need to be supported for deterministic ECDSA, or when signing/verifying a message.) This test case didn't have a dependency on the hash, but other similar test cases did, so this was arguably a bug. This is a good case and its primary aim is to validate the calculation, so it doesn't matter that it doesn't run in a configuration where it isn't unambiguously guaranteed to work. Therefore this is an excusable difference.
• “PSA sign: deterministic ECDSA SECP256R1 SHA-256, empty output buffer”: this test case only depended on ECDSA and not on deterministic ECDSA. It was passing without deterministic ECDSA because it's a negative test case and our implementation happens to error out before it would get to the point where it needs code that's specific to deterministic ECDSA. The missing dependency was a bug, and this difference is a bug fix and thus excused.
• “PSA sign: deterministic ECDSA SECP256R1, invalid hash algorithm …”: these test cases lacked a dependency on ECDSA. This was clearly not intended: there was a dependency on MBEDTLS_ECDSA_DETERMINISTIC, but not on MBEDTLS_ECDSA_C which is also necessary to get deterministic ECDSA. The missing dependency was harmless because the code errored out before reaching the point where ECDSA support was necessary. Arguably, as a negative test, it's still useful when the signature part of the algorithm is unsupported, but that's not the main goal of the test, so I'm excusing this difference.
• “PSA sign: invalid key type, signing with a public key”: this is a negative test case that tries to perform a valid algorithm with a key of the wrong type. It didn't have a dependency on the algorithm before, but now it does. The lack of a dependency was harmless because the library code checked the key type first; if the code had checked the algorithm first, it might have returned a different error code (NOT_SUPPORTED instead of INVALID_ARGUMENT) in configurations where the algorithm was not supported. The test cases normally avoid these ambiguous situations, so this was a bug. There is little value in testing an invalid type with an unsupported algorithm in addition to testing that invalid type with a valid algorithm. Excused.
• “PSA signature size: …” (signature_size): this test function only performs metadata tests: it's testing the signature size macros, it isn't actually performing a signature. It passes in every configuration because we don't make the definition of PSA_SIGN_OUTPUT_SIZE depend on which algorithms are supported; however this could be a meaningful (if minor) code size optimization, so the test code was fragile. Excused.

[stevew817]

Is there an intention to have this script be part of all.sh's check_* suite? I.e. to avoid re-introducing MBEDTLS_xxx dependencies in PSA test suites?

[ronald-cron-arm]

We are even thinking to improve it to stop having to set up dependencies manually anymore: see #4018.

[stevew817]

I'm not sure whether the one comment I have needs to be addressed in this PR or taken in a follow-up, therefore not blocking:

Now that SHA384 and SHA512 are disentangled, would it make sense to change the PSA Crypto config to set MBEDTLS_SHA512_NO_SHA384 in case there's a want for 512 but not 384?

[stevew817]

LGTM, see my previous comment for the one minor thing I had.

I haven't run the new script and compared the results myself, since it looks like @ronald-cron-arm did that already. I have reviewed the generated test suite changes and they look consistent to me.

[ronald-cron-arm]

I'm not sure whether the one comment I have needs to be addressed in this PR or taken in a follow-up, therefore not blocking:

Now that SHA384 and SHA512 are disentangled, would it make sense to change the PSA Crypto config to set MBEDTLS_SHA512_NO_SHA384 in case there's a want for 512 but not 384?

You mean something like the following in config_psa.h:

#if defined(PSA_WANT_ALG_SHA_512) && !defined(MBEDTLS_PSA_ACCEL_ALG_SHA_512)
#define MBEDTLS_PSA_BUILTIN_ALG_SHA_512 1
#define MBEDTLS_SHA512_C
#define MBEDTLS_SHA512_NO_SHA384
#endif

#if defined(PSA_WANT_ALG_SHA_384) && !defined(MBEDTLS_PSA_ACCEL_ALG_SHA_384)
#define MBEDTLS_PSA_BUILTIN_ALG_SHA_384 1
#define MBEDTLS_SHA512_C
#undef MBEDTLS_SHA512_NO_SHA384
#endif

[stevew817]

You mean something like the following in config_psa.h:
...

Or, with less changes:

#if defined(PSA_WANT_ALG_SHA_512) && !defined(MBEDTLS_PSA_ACCEL_ALG_SHA_512)
#define MBEDTLS_PSA_BUILTIN_ALG_SHA_512 1
#define MBEDTLS_SHA512_C
#if !defined(PSA_WANT_ALG_SHA_384) || defined(MBEDTLS_PSA_ACCEL_ALG_SHA_384)
#define MBEDTLS_SHA512_NO_SHA384
#endif
#endif

[gilles-peskine-arm]

Regarding SHA-384: the PSA config is supposed to be additive to the classic config. If it sets MBEDTLS_SHA512_NO_SHA384, that prevents classic code from using SHA-384. For example, if there's PSA code using SHA-512, and TLS code not using MBEDTLS_USE_PSA_CRYPTO but using SHA-384, the TLS code wouldn't work.

I hope we can resolve that in Mbed TLS 3.0 by changing MBEDTLS_SHA512_NO_SHA384 to a positive symbol. In Mbed TLS 2.x, I don't see a feasible solution.

[carlaageamundsen]

[EDITED]

I invoked the set_psa_test_dependencies.py script on test_suite_psa_crypto.data in commit 9a68810 ( our patch release branch ) and I applied some different test configs and I found a some issues. Some of them are in Silicon Labs device drivers, and the following in standard mbedTLS code:

When I applied a "minimal" test config with no key derivation algos supported there was a test case that failed because psa_key_derivation_setup() returned PSA_ERROR_INVALID_ARGUMENT when PSA_ERROR_NOT_SUPPORTED is expected. I will apply the following fix in the integration branch of SIlicon Labs (which should eventually be requested merged upstream), however you may want to apply the patch for this PR :

diff --git a/library/psa_crypto.c b/library/psa_crypto.c
index 6c26e821c..14f683848 100644
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -5895,6 +5895,12 @@ psa_status_t psa_key_derivation_setup( psa_key_derivation_operation_t *operation
     {
         status = psa_key_derivation_setup_kdf( operation, alg );
     }
+#else
+    else if( PSA_ALG_IS_KEY_AGREEMENT( alg ) ||
+             PSA_ALG_IS_KEY_DERIVATION( alg ) )
+    {
+        return( PSA_ERROR_NOT_SUPPORTED );
+    }
 #endif
     else
         return( PSA_ERROR_INVALID_ARGUMENT );

Additionally, I needed to apply the following patches in test_suite_psa_crypto.function in order to pass a few tests:

git diff tests/suites/test_suite_psa_crypto.function
diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function
index c34c015f7..a5cbdba69 100644
--- a/tests/suites/test_suite_psa_crypto.function
+++ b/tests/suites/test_suite_psa_crypto.function
@@ -62,7 +62,7 @@ static const size_t INVALID_EXPORT_LENGTH = ~0U;
  *
  * This is used in some smoke tests.
  */
-#if defined(KNOWN_SUPPORTED_HASH_ALG)
+#if defined(KNOWN_SUPPORTED_HASH_ALG) && defined(PSA_WANT_ALG_HMAC)
 #define KNOWN_SUPPORTED_MAC_ALG ( PSA_ALG_HMAC( KNOWN_SUPPORTED_HASH_ALG ) )
 #define KNOWN_SUPPORTED_MAC_KEY_TYPE PSA_KEY_TYPE_HMAC
 #elif defined(KNOWN_SUPPORTED_BLOCK_CIPHER) && defined(MBEDTLS_CMAC_C)
@@ -2899,7 +2899,7 @@ exit:
 }
 /* END_CASE */

-/* BEGIN_CASE */
+/* BEGIN_CASE depends_on:PSA_WANT_ALG_HMAC:PSA_WANT_ALG_SHA_256:PSA_WANT_KEY_TYPE_HMAC*/
 void mac_bad_order( )
 {
     psa_key_handle_t handle = 0;
@@ -3226,7 +3226,7 @@ void cipher_setup( int key_type_arg,
     psa_status_t expected_status = expected_status_arg;
     psa_cipher_operation_t operation = PSA_CIPHER_OPERATION_INIT;
     psa_status_t status;
-#if defined(KNOWN_SUPPORTED_MAC_ALG)
+#if defined(KNOWN_SUPPORTED_CIPHER_ALG)
     const uint8_t smoke_test_key_data[16] = "kkkkkkkkkkkkkkkk";
 #endif

[gilles-peskine-arm]

There were merge conflicts, so I rebased on top of the target branch (at 53943ca). I also rewrote the history to squash the one fixup commit. I also tried merging and the result https://github.com/gilles-peskine-arm/mbedtls/tree/psa-test-new-dependencies-3-merge is identical to what I obtained by rebasing. The previous version is at https://github.com/gilles-peskine-arm/mbedtls/tree/psa-test-new-dependencies-3.

[stevew817]

e4f539c has too long of a commit message. Otherwise still looking good.

[gilles-peskine-arm]

e4f539c has too long of a commit message.

At this point, given that we haven't historically been consistently enforcing any particular maximum length, I'd prefer to let it go.

[ronald-cron-arm]

I've been through the rebase and ended up with the same result but for the new tests introduced in test_suite_psa_crypto_entropy.data by #3912. After running tests/scripts/set_psa_test_dependencies.py as in "Change PSA crypto test dependencies to PSA_WANT_xxx" commit I ended up with the same tree. Thus this looks good to me.
tests/suites/test_suite_psa_crypto*.data