Define PSA_WANT_xxx_KEY_PAIR_yyy step 2/DH

[valeriosetti]

This PR replaces temporary occurrences of DH_KEY_TYPE_LEGACY with proper new symbols.

Depends on #7902 and #7909
Resolves #7773.

PR checklist

• changelog let's extend the existing entry, see Define PSA_WANT_xxx_KEY_PAIR_yyy step 2/DH #7903 (comment)
• backport not required
• tests provided

[mpg]

changelog not required: already introduced in #7614 - can be extended once #7772 and #7773 are done.

Well, this PR is when 7773 is done :) I think we could indeed extend the existing "features" entry a bit, to mention that (1) DERIVE is only available for ECC so far, and (2) the implementation is free to enable more than was requested (for example it currently enables import and export as soon as basic is present, though that might change in the future).

[valeriosetti]

Just rebased after #7902 was merged yesterday. CI was green before this rebase so hopefully it would be again soon.
I think that this PR is ready for review ;)

[valeriosetti]

Rebased again on development because #7909 was merged yesterday

[mpg]

I'm happy with the changes that are there (except two hickups in the Changelog), but I think a few more changes are needed:

• fulfill the prophecy in tests/scripts/generate_psa_tests.py
• adjust the opening comment in psa/crypto_legacy.h: I think we just want to remove _LEGACY here, which probably was added by mistake (mass replace) in the first place.

Once these two changes are done, git grep KEY_PAIR_LEGACY will no longer find any match, as should be.

[mpg]

LGTM, thanks!

[tom-daubney-arm]

I noticed that we still have the line //#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR 1 in the file configs/crypto_config_profile_medium.h which we import from TF-M. I think we should remove it now in this PR however others might not agree so we will check with the team. Other than that this PR is looking good so I will approve once we get an answer on this.

[valeriosetti]

I noticed that we still have the line //#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR 1 in the file configs/crypto_config_profile_medium.h which we import from TF-M. I think we should remove it now in this PR however others might not agree so we will check with the team. Other than that this PR is looking good so I will approve once we get an answer on this.

Correct. I would say we have 2 options here: either use new symbols for the RSA key pair or remove it. Then what about DH? Should I add it here as well (still commented out of course)?

Edit: thinking a bit more about this, usually having something commented out in a config file it means that it is supposed to be enabled in some test (even though I didn't find any of such case in all.sh), so I think answers to my questions are:

• replace PSA_WANT_KEY_TYPE_RSA_KEY_PAIR with new symbols for RSA, still keeping them commented out
• do not add DH keys support

Am I right?

[mpg]

I think there are several possible reasons from having things commented out like this:

• so that it's easy to re-enable it for testing, as you mentionned,
• or just because they want to keep their file close to the upstream psa/crypto_config.h file,
• or to be explicit about what could be enable and what isn't (which can be seen as a mix of the above two).

There seems to be consensus on Slack that we want to keep the top of this file unchanged from their version and add our changes in an explicitly-delimited section at the end, with #undefs and/or new #defines. Since this is about a commented-out line, I guess there's no #undef-ing to do, we can just add a comment. For example

/***********************************************************************
 * Local edits below this delimiter
 **********************************************************************/

/* Between Mbed TLS 3.4 and 3.5, the following macro, commented-out above,
//#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR 1
 * has been replaced with the following new macros:
//#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_BASIC 1
//#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_IMPORT 1
//#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_EXPORT 1
//#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_GENERATE 1
//#define PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_DERIVE 1
 */

/* Between Mbed TLS 3.4 and 3.5, the following macros have been added:
// [...]
 */

Wdyt?

[mpg]

LGTM

[tom-daubney-arm]

LGTM, thank you. I am marking "needs-ci" just while CI completes.

[gilles-peskine-arm]

@tom-daubney-arm FYI now that we're using merge queues, the merge action is “put on a queue to merge when the CI has finished” and no longer “merge now”. So we no longer need “needs-ci” when a pull request has been approved.

[tom-daubney-arm]

@tom-daubney-arm FYI now that we're using merge queues, the merge action is “put on a queue to merge when the CI has finished” and no longer “merge now”. So we no longer need “needs-ci” when a pull request has been approved.

That's great! Ok I will remove the label then. Thanks for letting me know.

[mpg]

Ah, I hadn't realised either the move to merge queues has that effect, so I was also waiting for the CI to complete before pressing the "merge when ready" button, which sounds a bit silly now that I'm writing it - thanks for connecting the dots :)