PK: refactor wrappers in the USE_PSA case

[valeriosetti]

This PR tries:

• to reshape the code in pk_wrap.c in order to make it clearer
• add verify support for opaque ECDSA

Resolves #7746

PR checklist

• changelog deferred - see ChangeLog for driver-only work #7413 (for extensions to opaque keys)
• backport not required
• tests provided for the new functionalities

[mpg]

[ ] changelog question to reviewers: is it required for the new verify support for opaque ECDSA?

Normally yes, but in this case we'll defer it so we can add a single entry later for all the changes to opaque keys.

[valeriosetti]

@mpg I opened the PR yesterday mostly to:

• check what the CI thinks about the code changes
• give you the chance to have some initial look at what I did in order to see if there are further area of improvements in your opinion

That's why it's not labeled "need-review" yet ;)

[mpg]

Just a first general look. Looking pretty good. Noticed a minor behaviour change we could do (as we never documented otherwise) that would make things more uniform and allow further code sharing.

Also, we should have a consistent naming scheme. I'm note sure yet what exactly it should be, but let's brainstorm it. Initial proposal to get the ball rolling:

• type_operation_legacy (for example ecdsa_sign_legacy) -> the full wrapper for the non-USE_PSA case
• type_operation_use_psa -> the core function for the USE_PSA case
• type_operation_use_psa_from_psa -> the wrapper for opaque keys and the USE_PSA_EC_DATA cases
• type_operation_use_psa_from_legacy -> the wrapper for the !USE_PSA_EC_DATA case (and for RSA in all cases)

Then perhaps something like #define type_operation_wrapper type_operation_xxx, probably close to the function's definition, for names we can use in the info structures? (Just a suggestion.)

[mpg]

Thanks for taking care not to change the defaults. However in this case I think the difference was not intentional, so we can actually merge the two functions and prefer deterministic when available. (For now: we'll change again in #7715 but it will be simpler if there's a single place to change - I think this could go in ecdsa_sign_psa() above once all three cases are aligned, by making it take an mbedtls_md_type_t param instead of psa_algorigthm_t - which I agree made perfect sense considering the constraints.)

I don't think the guard is an issue: we can define ecdsa_sign_wrap_psa_held unconditionally, then ecdsa_sign_wrap_ecp_keypair only when !PK_USE_PSA_EC_DATA. Then in the non-opaque info structure we use one or the other depending on PK_USE_PSA_EC_DATA. Or just define a function pointer externally to avoid cluttering the info structure, whichever is more readable.

[valeriosetti]

we can actually merge the two functions and prefer deterministic when available

I have a question here: when wrapping into opaque key, the alg is given as parameter to mbedtls_pk_wrap_as_opaque. There are test functions, like pk_psa_sign(), that only set alg_psa = PSA_ALG_ECDSA(PSA_ALG_SHA_256) and not the deterministic one. What should I do in these cases?

• add the option to set deterministic ECDSA? or
• add the guard to skip this test?

I'm asking because at the beginning I didn't notice the fact that opaque keys didn't support deterministic ECDSA and there were tests failing. That's why I come up with introducing separate function and comments.

[valeriosetti]

Following our discussion on Slack, it turned out that actually opaque keys cannot be easily changed to use deterministic ECDSA, because not only the key is owned externally, but also its properties. So if the application requires ALG_ECDSA and we set ALG_ECDSA_DETERMINISTIC the operations will be rejected.
This means that a proper wrapper for opaque keys will be added to the list proposed here

[mpg]

Actually, the right thing to do in the opaque wrapper would be to look which of ALG_ECDSA or ALG_ECDSA_DETERMINISTIC is supported by the key and pick that one. (Currently we just assume that the key allows ALG_ECDSA which is an arbitrary and undocumented restriction.)

It's out of the original scope of this PR, but it seems like small addition, so perhaps that's something we can do as part of this clean-up. Otherwise I'll create a separate issue to track it.

[valeriosetti]

Oh right, I didn't thought about it, thanks! No need for another PR, let's do it in this one

[mpg]

Pre-existing: this seems redundant: surely psa_sign_hash() will check this for us and return an error, and then hopefully PSA_PK_ECDSA_TO_MBEDTLS_ERR() will translate that to something sensible.

[mpg]

Pre-existing: this should be PSA_WANT_ALG_DETERMINISTIC_ECDSA really.

[mpg]

Oh yes, thank you! :)

However, shouldn't we still have

#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE)
    .verify_rs_func = NULL,
    .sign_rs_func = NULL,
#endif

[valeriosetti]

maybe I'm missing something here, but it seems to me that the original code only had

#if defined()
#endif

So those functions should always be NULL and we don't necessarily need to fill all the fields. Am I wrong?

[mpg]

According to the standard you're correct, but I was worried that perhaps some compilers would complain if we don't explicitly initialize all the fields.

[valeriosetti]

Well in this case can we wait for the CI feedback before going back to fill all the fields? I like the new "shape" since it looks much clearer to me and also avoid declaring things we're not going to use.

[gilles-peskine-arm]

Clang does, in fact, complain about a_struct_with_two_fields x = {0}. That's why we have all these PSA_xxx_INIT macros instead of just telling people to write {0}.

[mpg]

Clang does, in fact, complain about a_struct_with_two_fields x = {0}. That's why we have all these PSA_xxx_INIT macros instead of just telling people to write {0}.

And I think that's also why C23 introduced {}.

That said, apparently the warnings are not always the same in the presence of designated initializers.

Well in this case can we wait for the CI feedback before going back to fill all the fields? I like the new "shape" since it looks much clearer to me and also avoid declaring things we're not going to use.

I'm divided. On one hand, I was going to suggest the same myself: as long as the CI is happy we're happy. OTOH, we only have a relatively small selection of compilers on our CI (GCC, Clang, a few versions of MSVC, not even IAR), so who knows that other compilers are doing? (On the other other hand, we could wait and see if people report issues with the compiler they're using. But then again, isn't it better to try and prevent that from happening?)

[valeriosetti]

Then perhaps something like #define type_operation_wrapper type_operation_xxx, probably close to the function's definition, for names we can use in the info structures? (Just a suggestion.)

I would prefer to keep the same wrappers' name functions for all the cases instead of defining several functions and then having a macro to redefine symbols. There are a couple of macros in my current implementation, but only to save some code size (i.e not having a function to call another one without doing anything useful).

For the record I also tried to redefine some pk_info functions when USE_PSA is defined since most of them get rid of the f_rng and p_rng parameters. This was helpful to save even more functions on the EC side, but unfortunately RSA_ALT uses those parameters (ex: rsa_alt_sign_wrap) no matter what is the status of USE_PSA so in the end I gave up with that change.

[mpg]

Looking pretty good to me, just one question left, and then I want to make a final pass to be sure.

[valeriosetti]

I have some questions for reviewers:

1. Right now the code in pk_wrap is aggregated for functionality, i.e. sign functions all close together, the same for verify, etc. Do you think/feel it would be clearer if instead I aggregate functions by guards? I mean, all cases with USE_PSA && PK_USE_PSA_EC_DATA, then USE_PSA && !PK_USE_PSA_EC_DATA, etc. The main advantage is that there would be much less #if defined in that code and this might improve clearness even more

2. Do you think it makes sense to have opaque ECDSA key support (i.e. mbedtls_ecdsa_opaque_info) when PK module has no way to manage EC keys (i.e. !MBEDTLS_PK_HAVE_ECC_KEYS)? I know that some functions for opaque keys are generic and they do not depend on EC functionality, like opaque_get_bitlen(); however for what it is of practical interest then some EC support is required. Wdyt?

[mpg]

1. Right now the code in `pk_wrap` is aggregated for functionality, i.e. sign functions all close together, the same for verify, etc.  Do you think/feel it would be clearer if instead I aggregate functions by guards? I mean, all cases with `USE_PSA && PK_USE_PSA_EC_DATA`, then `USE_PSA && !PK_USE_PSA_EC_DATA`, etc. The main advantage is that there would be much less `#if defined` in that code and this might improve clearness even more

Ah, good question. On one hand, I agree that grouping by guards would reduce the total number of #ifs, which is generally good. On the other hand, the way things are done now, some functions delegate to others, for example ecdsa_opaque_verify_wrap and the two USE_PSA versions of ecdsa_verify_wrap ultimately call ecdsa_verify_psa so I think it's nice for them to be grouped together.

I had a look at the file, and I think I like the way it's organized now. For each operation, we have the same logical order:

• USE_PSA:
  • core function
  • opaque wrapper
  • USE_PSA_EC_DATA wrapper
  • !USE_PSA_EC_DATA wrapper
• !USE_PSA wrapper

I think that still keep things logically grouped by guard within each operation.

Let's see what the review think, it's probably partially a matter of taste, but I like it the way it is now.

2. Do you think it makes sense to have opaque ECDSA key support (i.e. `mbedtls_ecdsa_opaque_info`) when PK module has no way to manage EC keys (i.e. `!MBEDTLS_PK_HAVE_ECC_KEYS`)? I know that some functions for opaque keys are generic and they do not depend on EC functionality, like `opaque_get_bitlen()`; however for what it is of practical interest then some EC support is required. Wdyt?

No, I don't think that makes sense, or would be particularly useful to anyone, so let's save our time and not do it. (Note: if we did it, we'd have to document it (explaining all the limitations) and test it too.)

[AndrzejKurek]

I know that it's just a public key, but don't we want to clear the key buffer here?

[valeriosetti]

I think that not too long ago we had a similar situation in an old PR and together with @mpg we agreed that it was not an issue if the buffer holding the public key was not cleared on exit. Basically with only this information you cannot derive any private secret, right? And at the same time the public key is something publicly available by definition so no new information is leaked here in IMO.
@mpg wdyt?

[mpg]

That's my recollection as well, we don't need to wipe buffers that only contained public keys.

[AndrzejKurek]

Same question about clearing a key buffer here.

[valeriosetti]

Ok, I'll keep note of this as well, but let's wait for the feedback on the previous comment before taking any action here ;)

[AndrzejKurek]

prv_key_buf, which is now internal for this function, is not cleared on failure.

[valeriosetti]

Oh, this is really bad indeed. Thanks for catching it!

[AndrzejKurek]

I left some questions. The grouping of functions is fine the way it is now :)

[mpg]

LGTM, thanks!