Enable check names component

[Harry-Ramsey]

Enable check names component for TF-PSA-Crypto. Closes #52.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

• changelog provided | not required because: testing enhancement.
• framework PR provided Mbed-TLS/mbedtls-framework: #117
• mbedtls PR provided Mbed-TLS/mbedtls: #9915
• tests provided | not required because:

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

• Multiple simple commits
  • please structure your PR into a series of small commits, each of which does one thing
• Avoid force-push
  • please do not force-push to update your PR - just add new commit(s)
• See our Guidelines for Contributors for more details about the review process.

[Harry-Ramsey]

The failures reported here are to do with the test successfully running but have been propogated to TF-PSA-Crypto.

[ronald-cron-arm]

The failures reported here are to do with the test successfully running but have been propogated to TF-PSA-Crypto.

I am not sure to understand. What is needed for the new component tf_psa_crypto_check_names to run successfully? We cannot merge this PR in that state.

[Harry-Ramsey]

The failures reported here are to do with the test successfully running but have been propogated to TF-PSA-Crypto.

I am not sure to understand. What is needed for the new component tf_psa_crypto_check_names to run successfully? We cannot merge this PR in that state.

Sorry, there was an extreme number of errors much higher than expected. I forgot to remove code from check_names.py which overwrote selected paths resulting in these errors.

[ronald-cron-arm]

Regarding

#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && defined(MBEDTLS_USE_PSA_CRYPTO) && \   
    !(defined(PSA_WANT_ALG_SHA_1) || defined(PSA_WANT_ALG_SHA_256) || defined(PSA_WANT_ALG_SHA_512))
#error "MBEDTLS_SSL_PROTO_TLS1_2 defined, but not all prerequisites"            
#endif 

in ./drivers/builtin/src/check_crypto_config.h, this should rather be in mbedtls:include/mbedtls/check_config.h. No need for defined(MBEDTLS_USE_PSA_CRYPTO) as MBEDTLS_USE_PSA_CRYPTO is always on now. Thus just:

#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \   
    !(defined(PSA_WANT_ALG_SHA_1) || defined(PSA_WANT_ALG_SHA_256) || defined(PSA_WANT_ALG_SHA_512))
#error "MBEDTLS_SSL_PROTO_TLS1_2 defined, but not all prerequisites"            
#endif

[ronald-cron-arm]

Regarding MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS in crypto_config.h and ecp.h I think we can just remove the references to MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS. Looking in ssl.h at the documentation of the TLS functions that return MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS I do not feel the need to change their documentation because of the removals of the comments in TF-PSA-Crypto.

[ronald-cron-arm]

Regarding psa_util_internal.h:

#if defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3)
extern const mbedtls_error_pair_t psa_to_ssl_errors[7];
#endif

can be moved to library/ssl_misc.h. It is defined in library/ssl_tls.c.

[ronald-cron-arm]

In entropy_poll.c

#if defined(MBEDTLS_TIMING_C)
#include "mbedtls/timing.h"
#endif

can just be removed it seems.

[ronald-cron-arm]

In crypto_config.h the note:

 * \note If MBEDTLS_TIMING_C is set - to enable the semi-portable timing
 *       interface - timing.c will include time.h on suitable platforms
 *       regardless of the setting of MBEDTLS_HAVE_TIME, unless
 *       MBEDTLS_TIMING_ALT is used. See timing.c for more information.

can be just removed. We already have the same note in MBEDTLS_TIMING_C documentation in mbedtls_config.h.

[Harry-Ramsey]

Just need to create a Mbed TLS pull request where I add the necessary checks and this can be reviewed.

[ronald-cron-arm]

Regarding the MBEDTLS_SSL_MAX_... symbols in cipher.h, after some discussions with @gilles-peskine-arm, I propose to remove the comments in cipher.h and add some unit tests in test_suite_ssl that assert that MBEDTLS_SSL_MAX... >= MBEDTLS_MAX_.... A static_checks() test function similar to the one in test_suite_psa_crypto.function should do the job.

[gilles-peskine-arm]

some unit tests in test_suite_ssl that assert that MBEDTLS_SSL_MAX... >= MBEDTLS_MAX_....

I'm actually not sure if those tests are still relevant. The comments date back from before MBEDTLS_USE_PSA_CRYPTO, and it's technically a bug that we didn't update it to take MBEDTLS_USE_PSA_CRYPTO into account (but I don't propose to fix a comment in an LTS branch that's about values that will never change in that branch). Since that was introduced, in theory, PSA could have defined larger values. When MBEDTLS_USE_PSA_CRYPTO is active, the cipher.h constants are not relevant. So if we wanted to do things perfectly right:

• Pre-PSA versions of Mbed TLS should assert MBEDTLS_SSL_MAX_xxx against the constants from cipher.h.
• Mbed TLS 2.18 (?) through 3.6 should assert MBEDTLS_SSL_MAX_xxx against the constants from cipher.h when MBEDTLS_USE_PSA_CRYPTO is disabled, and against constants from psa/crypto_sizes.h when MBEDTLS_USE_PSA_CRYPTO is enabled.
• Mbed TLS 4.0 should assert MBEDTLS_SSL_MAX_xxx against constants from psa/crypto_sizes.h.

In practice the values defined in ssl_misc.h are very unlikely to need changing. And if they do, it'll be part of a large project where we'll know that a lot of buffer sizes need to be updated.

So I'd be ok with not adding any assertions. And I'm a bit doubtful about adding assertions to 4.0 that mention the deprecated constants from cipher.h. We shouldn't start using deprecated symbols in new code.

The PSA values to compare against would be:

MBEDTLS_SSL_MAX_BLOCK_LENGTH >= PSA_BLOCK_CIPHER_BLOCK_MAX_SIZE
MBEDTLS_SSL_MAX_IV_LENGTH >= PSA_CIPHER_IV_MAX_SIZE
MBEDTLS_SSL_MAX_KEY_LENGTH >= no suitable macro — mind you cipher.h didn't have one either

[ronald-cron-arm]

Do you mean: MBEDTLS_SSL_MAX_BLOCK_LENGTH >= PSA_BLOCK_CIPHER_BLOCK_MAX_SIZE (greater or equal than)?

[gilles-peskine-arm]

Oops, yes, I had the comparisons the wrong way round. I edited my comment.

[mpg]

I don't think those tests would be logically correct. It may very well be that in the future we'll add a cipher with variable IV length and TLS only uses a certain IV size that's smaller than the maximum, and then SSL_MAX_IV_LENGTH will be smaller than PSA_CIPHER_IV_MAX_SIZE and that would be perfectly OK. Similar thought about key length, and block size (we could have a cipher supported in PSA that's not used by any TLS ciphersuite).

So I'm actually opposed to adding those assertions.

[ronald-cron-arm]

I don't think those tests would be logically correct. It may very well be that in the future we'll add a cipher with variable IV length and TLS only uses a certain IV size that's smaller than the maximum, and then SSL_MAX_IV_LENGTH will be smaller than PSA_CIPHER_IV_MAX_SIZE and that would be perfectly OK. Similar thought about key length, and block size (we could have a cipher supported in PSA that's not used by any TLS ciphersuite).

So I'm actually opposed to adding those assertions.

Thanks, I think I understand better the ins and outs of the MBEDTLS_SSL_MAX_ macros now. If I understand correctly, the comments in cipher.h stating that MBEDTLS_MAX_ macros should be kept in sync with the MBEDTLS_SSL_MAX_ ones in ssl_misc.h are actually at least misleading thus we are just going to remove them.

[valeriosetti]

Suggested change

	// #define MBEDTLS_ECP_RESTARTABLE
	//#define MBEDTLS_ECP_RESTARTABLE

[valeriosetti]

Note: as we agreed, I cherry-picked e7caf8c in #165 ;)