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

Conversion function from ecp group to PSA curve #7764

Closed
gilles-peskine-arm opened this issue Jun 13, 2023 · 9 comments · Fixed by #8664
Closed

Conversion function from ecp group to PSA curve #7764

gilles-peskine-arm opened this issue Jun 13, 2023 · 9 comments · Fixed by #8664
Assignees
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

There should be a simple way to convert from a mbedtls_ecp_group or mbedtls_ecp_group_id to the corresponding PSA encoding (curve and bit-size). This is needed when migrating code that needs low-level ecp.h code to create a key, which is then used through the PSA API. For example, to import a compressed point using mbedtls_ecp_point_read_binary followed by mbedtls_ecp_point_write_binary and psa_import_key.

Goals of this issue:

  • Design the new API.
  • Implement and test it.
  • List it in the upcoming PSA transition guide.
@gilles-peskine-arm gilles-peskine-arm added enhancement component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d) labels Jun 13, 2023
@gilles-peskine-arm
Copy link
Contributor Author

Independent proposal

psa_ecc_curve_family_t mbedtls_psa_from_grp_id(mbedtls_ecp_group_id grp_id, size_t *bits);

Info proposal

Add psa_family and psa_bits fields to mbedtls_ecp_curve_info.

Annoyingly, psa_bits would be equal to bit_size except for Curve25519 which has the size 256 in the legacy API but 255 in the PSA API.

Common questions

If a curve is supported in the legacy API but not in the PSA API, should the conversion return the correct result or a not-supported indicator?

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Jun 20, 2023
Resolves Mbed-TLS#7764.

The PSA and ECP sizes are the same, except for Curve25519 where the ECP size
is 256 rather than 255 even though private values are 255-bit numbers.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Copy link
Contributor Author

We actually already have these conversions functions. I somehow missed them when I was writing the document. They're mbedtls_ecc_group_to_psa and mbedtls_ecc_group_of_psa and they've been in crypto_extra.h, documented as experimental, since the PSA API was in beta (and so in particular they're in 2.28 LTS).

I'm not sure they are the API I want for the 3.x/4.x transition period. I was looking at how to integrate this more into the ECP module, and the curve_info structure seems like a good fit to me — it's about curve metadata relating the internal encoding with some mathematical properties (the size) and with TLS encodings, so adding the PSA encoding would be a natural fit. Also, the bits_is_sloppy=true case for PSA→ECC is a quirk only for the sake of psa_import_key, and that quirk should be implemented independently of the use of the ECP module since it also applies when ECC is entirely implemented with drivers.

So I think I want to design new, clean functions, well in time for the 3.x LTS so that we can remove the existing functions.

@gilles-peskine-arm
Copy link
Contributor Author

the curve_info structure seems like a good fit to me

@mpg notes that this is a good fit for users who want this functionality because they're doing fancy things with the ECP module. But it's not a good fit for users who are merely using mbedtls_ecp_group_id to identify curves, and are using PSA drivers and don't want to build ecp.c at all. So I think we'll want a different design in 3.x and in 4.x. Perhaps introduce the curve_info approach now, but keep mbedtls_ecc_group_{from,to}_psa in the 3.x LTS?

Though I still think the sloppy case of mbedtls_ecc_group_of_psa doesn't belong in this API, and we should remove that parameter from the function that's made official in 3.x.

@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

Now that we have support for driver-only ECC, I'm strongly inclined against the curve_info approach, and would really prefer instead making mbedtls_ecc_group_{from,to}_psa officially public with a decent API.

I agree about removing the sloppy case of mbedtls_ecc_group_of_psa and while at it I think from is better than of in the name.

@mpg
Copy link
Contributor

mpg commented Dec 11, 2023

Btw, should these functions really be declared in psa/crypto_extra.h? Arguably, something under mbedtls would be better suited. IMO psa/crypto_extra.h is better suited for PSA extensions that would still make sense in a pure-PSA world, but things that are related to the legacy API (including transition helpers) should be kept under mbedtls.

I think mbedtls/psa_utils.h would be a good place for these conversion functions.

(See Frank's comment on a related issue: #8340 (comment))

@frkv frkv mentioned this issue Dec 12, 2023
3 tasks
@valeriosetti valeriosetti self-assigned this Dec 21, 2023
@valeriosetti
Copy link
Contributor

valeriosetti commented Dec 21, 2023

Moving of mbedtls_ecc_group_{of,to}_psa has already been done as part of #8630. So it seems to me that according to the last design guides proposed here and here the remaining things are:

  • rename mbedtls_ecc_group_of_psa to mbedtls_ecc_group_from_psa
  • remove bits_is_sloppy from mbedtls_ecc_group_of_psa
  • [Edit] perhaps update the documentation of those functions saying that they are now "officially supported conversion function" instead of "mostly for internal usage and can change at any time"
  • add some testing?

Am I missing something?

@mpg
Copy link
Contributor

mpg commented Dec 21, 2023

Sounds right to me!

@mpg
Copy link
Contributor

mpg commented Dec 26, 2023

Note: when renaming _of_ -> _from_, if #8657 has been merged in the meantime, update the function name in the document it introduces, or else leave a comment in #8657 about it.

@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.

@mpg mpg mentioned this issue Dec 26, 2023
3 tasks
@minosgalanakis minosgalanakis removed this from [3.6] Legacy-to-PSA migration helpers in EPICs for Mbed TLS Apr 29, 2024
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.

3 participants