Fixup or re-enable tests with Use PSA

[superna9999]

Description

There are a number of tests that currently depend on !MBEDTLS_USE_PSA_CRYPTO. Since this USE_PSA is meant to become the default, and even the only option at some point, it is should enjoy at least as much test coverage as the non-PSA variant.

Resolves #5669

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

N/A

Todos

• Tests

Steps to test or reproduce

test_suite_ssl must run clean

[superna9999]

@mpg the only remaining test with !MBEDTLS_USE_PSA_CRYPTO is :

 Verify ext RSA #5 (PKCS1 v2.1, wrong salt_len)

in tests/suites/test_suite_pk.data

But there is an alternate one for PSA, because the wrong salt len doesn't return PSA_ERROR_INVALID_PADDING but PSA_ERROR_INVALID_SIGNATURE so pk return can't be MBEDTLS_ERR_RSA_INVALID_PADDING

Where this should be documented, can we add comments into tests/suites/test_suite_pk.data ?

[superna9999]

Found a very explicit comment in

mbedtls/library/psa_crypto_rsa.c

Lines 520 to 522 in 473d585

	/* Mbed TLS distinguishes "invalid padding" from "valid padding but
	* the rest of the signature is invalid". This has little use in
	* practice and PSA doesn't report this distinction. */

about the problem, so I copied the comment in tests/suites/test_suite_pk.data to document the PSA specific test need

[mpg]

I think a comment in the data file explaining why there are two versions of the test's data is good, but actually I'd prefer to handle that in the test function, for example:

#if defined(MBEDTLS_USE_PSA_CRYTPO)
if( result == MBEDTLS_ERR_RSA_INVALID_PADDING )
    result = MBEDTLS_ERR_RSA_VERIFY_FAILED;
#endif

(with a comment explaining why). Do you think that would work as well?

The reason I think this is preferable is that having no test case that depends on !MBEDTLS_USE_PSA_CRYPTO makes things easier to audit: we can just grep for that see that there are no occurrences, instead of finding a small number of occurrences and then checking each of them to see if there's a corresponding PSA test.

Actually, this makes me think: perhaps we should add a component in all.sh that checks that there is no occurrence of 'depends_on.*!MBEDTLS_USE_PSA_CRYPTO' in the code base. That way, we don't risk having to do that task again in a couple of months...

[superna9999]

@mpg Ok ! I'll move it to the function instead with the same comment.

I'll also try to detect the non-PSA test cases in all.sh

[mpg]

Cool, thanks!

Also, if you don't mind me expanding the scope of the task a bit: we should check in ssl-opt.sh too.

I just did, and found two tests that have requires_config_disabled MBEDTLS_USE_PSA_CRYPTO. As far as I can see, that's just a remnant from the time where TLS 1.3 wasn't compatible with that. I tried just removing that line, and the tests passed in a build with USE_PSA enabled, so I think those negative depends are no longer useful and we just forgot to remove them.

Can you remove those and also add that to the all.sh check?

[superna9999]

@mpg sure will do

[mpg]

I'm happy with the code, but I'm requesting further tuning of the comment.

[mpg]

Sorry for picking on grammar, but I don't think we should have a full stop here, the explanation introduced by the "because" is not complete yet - the reason is "and PSA doesn't report this distinction". While editing this comment, I think we might want to expand on why a non-PSA path might be taken while USE_PSA is active.

Here's a suggestion:

    /* Mbed TLS distinguishes "invalid padding" from "valid padding but
     * the rest of the signature is invalid". This has little use in
     * practice and PSA doesn't report this distinction.
     * In this case, PSA returns PSA_ERROR_INVALID_SIGNATURE translated
     * to MBEDTLS_ERR_RSA_VERIFY_FAILED.
     * However, currently `mbedtls_pk_verify_ext()` may use either the PSA or the
     * Mbed TLS API, depending on the PSS options used. So, it may return either
     * INVALID_PADDING or INVALID_SIGNATURE.
     */

[superna9999]

I'm ok with that, I'll update the comment