Replace MBEDTLS_PK_HAVE_ECDSA* with PSA_WANT counterparts

[eleuzi01]

Description

Resolves #9337

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

• changelog not required, internal changes
• 3.6 backport partial backport -> Backport 3.6: Fix incorrect dependencies #9568
• 2.28 backport not required, needed for 4.0
• tests not required

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.

[gilles-peskine-arm]

This needs to be defined(PSA_WANT_ALG_ECDSA) || defined(PSA_WANT_ALG_DETERMINISTIC_ECDSA). IIRC we've already decided to add a definition for that in include/psa/crypto_adjust_*.h. I don't know if it's already being added by a PR in flight.

[gilles-peskine-arm]

PSA_HAVE_ALG_SOME_ECDSA from #9384

[gilles-peskine-arm]

This is one of many places where we were using MBEDTLS_PK_CAN_ECDSA_SOME but we should actually have used MBEDTLS_PK_CAN_ECDSA_SIGN. (Or MBEDTLS_PK_CAN_ECDSA_VERIFY in some places, but there's no way to configure the library with MBEDTLS_PK_CAN_ECDSA_SOME && !MBEDTLS_PK_CAN_ECDSA_VERIFY, since there's no way to have private-key ECC operations without the corresponding public-key ECC operation.) See #9473.

Technically, we're losing information here: now, when we see PSA_HAVE_ALG_SOME_ECDSA, we won't know whether that came from the incorrect usage of MBEDTLS_PK_CAN_ECDSA_SOME. It doesn't matter much because we don't currently test any configuration where MBEDTLS_PK_CAN_ECDSA_VERIFY is enabled but not MBEDTLS_PK_CAN_ECDSA_SIGN, so I expect there are more places where we don't have the correct dependencies. Still, I think it would be better to change to PSA_HAVE_ALG_SOME_ECDSA && PSA_WANT_KEY_TYPE_ECC_KEY_PAIR in places where ECDSA-sign is required.

(Incidentally, PSA_HAVE_ALG_SOME_ECDSA && PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY is effectively equivalent to PSA_HAVE_ALG_SOME_ECDSA, because ECDSA can only be done with an ECC key.)

[gilles-peskine-arm]

I've reviewed this pull request, paying particular attention to where MBEDTLS_PK_CAN_ECDSA_SOME should have been MBEDTLS_PK_CAN_ECDSA_SIGN (see #9473). I think we should fix those while we're at it.

I also noticed a couple of other preexisting problems, which we can either fix now or declare out of scope.

We should backport the fixes to 3.6 (e.g. if we're adding && PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_xxx here then we should replace _SOME by _SIGN in 3.6).

[gilles-peskine-arm]

Suggested change

	#if defined(PSA_HAVE_ALG_SOME_ECDSA)
	#if defined(PSA_HAVE_ALG_SOME_ECDSA) && defined(PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT)

[gilles-peskine-arm]

Suggested change

	/* BEGIN_CASE depends_on:MBEDTLS_PSA_CRYPTO_C:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_USE_PSA_CRYPTO:MBEDTLS_PKCS1_V15:MBEDTLS_SSL_PROTO_TLS1_2:PSA_WANT_ECC_SECP_R1_256:MBEDTLS_RSA_C:MBEDTLS_ECP_HAVE_SECP384R1:PSA_HAVE_ALG_SOME_ECDSA */
	/* BEGIN_CASE depends_on:MBEDTLS_PSA_CRYPTO_C:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_USE_PSA_CRYPTO:MBEDTLS_PKCS1_V15:MBEDTLS_SSL_PROTO_TLS1_2:PSA_WANT_ECC_SECP_R1_256:MBEDTLS_RSA_C:MBEDTLS_ECP_HAVE_SECP384R1:PSA_HAVE_ALG_SOME_ECDSA:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT */

Preexisting and out of scope, but related to your recent work: why is there both PSA_WANT_ECC_SECP_R1_256 and MBEDTLS_ECP_HAVE_SECP384R1 here?

Also those dependencies look wrong in other preexisting ways (missing ECDH, and I don't see why RSA is required).

[gilles-peskine-arm]

All the test cases that have MBEDTLS_PK_ECxxx as the first parameter and FROM_PAIR as the second parameter should depend on PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT. (The ones with FROM_PUBLIC should depend on PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY, but that's not important since you can't usefully enable ECDSA without that.) But that may be covered later when replacing MBEDTLS_PK_HAVE_ECC_KEYS.

[gilles-peskine-arm]

Technically all handshake_cipher and handshake_ciphersuite_select test cases involving a cipher suite with ECDH, ECDHE or ECDSA in its name should depend on PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT (for ECDSA on the server) and PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE (for ECDHE on both sides). But the dependency on MBEDTLS_KEY_EXCHANGE_xxx_ENABLED is enough and we could remove the dependencies on PSA_WANT_ALG_ECxxx and PSA_WANT_KEY_TYPE_ECC_xxx.

[gilles-peskine-arm]

Preexisting, but I think ECC-related dependencies on ssl_tls13_record_protection are wrong. As far as I can tell from the name and code, this test does not involve ECC in any way. I'm sure that MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED is wrong since that's a TLS 1.2 feature but the test function is about TLS 1.3 only.

Suggested change

	depends_on:PSA_WANT_KEY_TYPE_AES:PSA_WANT_ALG_GCM:PSA_WANT_ALG_SHA_256:PSA_HAVE_ALG_SOME_ECDSA:MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED
	depends_on:PSA_WANT_KEY_TYPE_AES:PSA_WANT_ALG_GCM:PSA_WANT_ALG_SHA_256

and same for the other ssl_tls13_record_protection test cases.

[davidhorstmann-arm]

Code changes LGTM (but CI is unhappy)

[eleuzi01]

Forgot to ask: are we happy with the names PSA_HAVE_ALG_ECDSA_SIGN and PSA_HAVE_ALG_ECDSA_VERIFY or should these be called something else?

[davidhorstmann-arm]

LGTM, thanks!

[gilles-peskine-arm]

LGTM

[gilles-peskine-arm]

A note on commit messages — never mind for this time, but please keep this in mind in the future.

Commit messages are useful for future readers. They should summarize what is being done, and explain why it's done. The relative importance can vary: sometimes one or the other is obvious, or part of a larger series.

For example, the first commit in this PR does a mass renaming. Its commit message “Replace MBEDTLS_PK_CAN_ECDSA_SOME with PSA_HAVE_ALG_SOME_ECDSA” is a good because the diff is very long, but the content is conceptually simple and nicely summarized by the title line. The reason why we're doing this renaming is that it's part of a larger task, and there's no point in explaining why we do this task in every commit. So that commit message is fine.

On the other hand, “Address review comments: remove dependencies” is not a very good commit message. “Remove dependencies” is ok as a summary, but not very informative: which dependencies? As a rule of thumb, apart from really boring things like “fix typo in documentation”, if you can potentially have multiple commits in the same pull request with the same commit title line, the title line isn't informative enough. On the other hand, “address review comments” is not relevant in a commit message. Technically that's why you're doing it, sure, but that reason won't matter after the pull request is merged. Why was there a review comment that suggested to remove these dependencies? For a future reader, there are several plausible reasons and it's hard to tell. Maybe (1) the tests were skipped when they shouldn't be? Or maybe (2) we've made the test code or the library more capable and so we're running more tests? Or (3) the dependencies are actually redundant, with no change in behavior. As it turns out, it's a mixture of (1) and (3), and it would be useful to convey this to a future reader. So a better commit message would be something like

Remove redundant dependencies on ECC/ECDSA

• Requiring ECC/ECDSA from cryptography is redundant to requiring ECDH/ECDSA in the key exchange.
• Don't require ECDSA in test cases that only do record protection and not a handshake.

Note how writing this down has brought me to mention both ECC and ECDSA, and the distinction between the two is part of why “Bring back some dependencies” was needed. (Though I'm not sure why MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED doesn't imply PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY — I guess for some legacy builds because we're only part way through the configuration changes?)