Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export the mbedtls_md_psa_alg_from_type function. #8340

Closed
ManishVB-Arm opened this issue Oct 10, 2023 · 6 comments · Fixed by #8666
Closed

Export the mbedtls_md_psa_alg_from_type function. #8340

ManishVB-Arm opened this issue Oct 10, 2023 · 6 comments · Fixed by #8666
Assignees
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)

Comments

@ManishVB-Arm
Copy link

Suggested enhancement

I'm seeing the tagged release v3.5.0 where this function: mbedtls_md_psa_alg_from_type is introduced 1.
Probably TF-A code can make use of it, but it is in local header. Is it possible to export this?

Justification

It will be easier for the mbedTLS consumer to convert mbedtls_md_type to psa_algorithm.

@gilles-peskine-arm gilles-peskine-arm added enhancement component-crypto Crypto primitives and low-level interfaces component-psa PSA keystore/dispatch layer (storage, drivers, …) size-s Estimated task size: small (~2d) and removed component-psa PSA keystore/dispatch layer (storage, drivers, …) labels Oct 10, 2023
@mpg mpg added this to [3.6] Legacy-to-PSA migration helpers in EPICs for Mbed TLS Nov 24, 2023
@mpg
Copy link
Contributor

mpg commented Dec 8, 2023

Question: should this be part of mbedtls/md.h or psa/crypto_extra.h? Other similar functions are declared in crypto_extra.h. I think it would make sense for similar functions (that is, convert metadata between legacy and PSA) to be either all in crypto_extra.h or the legacy header for this area (md.h, ecp.h, etc) but not a mix of both.

Note there's a 3rd option: psa_util.h (though currently it has no such function, it used to have some in the past).

@frkv
Copy link

frkv commented Dec 8, 2023

Question: should this be part of mbedtls/md.h or psa/crypto_extra.h? Other similar functions are declared in crypto_extra.h. I think it would make sense for similar functions (that is, convert metadata between legacy and PSA) to be either all in crypto_extra.h or the legacy header for this area (md.h, ecp.h, etc) but not a mix of both.

Note there's a 3rd option: psa_util.h (though currently it has no such function, it used to have some in the past).

I would recommend psa_util.h as crypto_extra.h also exists in scope of TF-M. I'm assuming that TF-M will be less dependent on understanding the APIs you would want to put in psa_util.h in Mbed TLS 4.0 and later

@mpg
Copy link
Contributor

mpg commented Dec 26, 2023

Note: we probably want to make functions for both directions available: mbedtls_md_psa_alg_from_type() and mbedtls_md_type_from_psa_alg().

Question: currently the internal functions have quirks (see warnings in their documentation). Are those acceptable for public functions of should the public functions avoid those (at the cost of higher code size)?

@mpg
Copy link
Contributor

mpg commented Dec 26, 2023

Quoting from the document added by #8657:

Each action to implement a function entails:

  • Implement the library function.
  • Document it precisely, including error conditions.
  • Unit-test it.
  • Mention it where relevant in the PSA transition guide.

@valeriosetti valeriosetti self-assigned this Jan 2, 2024
@valeriosetti
Copy link
Contributor

valeriosetti commented Jan 2, 2024

Question: currently the internal functions have quirks (see warnings in their documentation). Are those acceptable for public functions of should the public functions avoid those (at the cost of higher code size)?

Here's my 2 cents: since we're speaking of conversion functions I would prefer them to just perform the translation and have some other function to check for availability. However this is not the approach used in elliptic curves (#8664), so I guess it's better to keep the same pattern for all the conversion functions and add the check also here.

@gilles-peskine-arm
Copy link
Contributor

conversion functions I would prefer them to just perform the translation and have some other function to check for availability

Conversion functions tend to be limited by availability because in a build with only a subset of algorithms, we don't want to have to include enough metadata to convert all algorithms, because that would cost code size. This limits the choice to: if algorithm X is unsupported, is convert(X) guaranteed to return NONE, or might it return something else due to an implementation quirk (e.g. if the conversion is a constant offset, like it is now for hashes)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)
Projects
Status: [3.6] Legacy-to-PSA migration helpers
Development

Successfully merging a pull request may close this issue.

5 participants