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

Generalize mbedtls_pk_setup_opaque to MBEDTLS_PSA_CRYPTO_CLIENT #8710

Closed
gilles-peskine-arm opened this issue Jan 17, 2024 · 6 comments
Closed
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jan 17, 2024

Extend mbedtls_pk_setup_opaque to provide support for MBEDTLS_PK_OPAQUE whenever PSA crypto API functions are available, i.e. whenever MBEDTLS_PSA_CRYPTO_CLIENT is enabled, regardless of the status of MBEDTLS_USE_PSA_CRYPTO.

For testing, we assume MBEDTLS_PSA_CRYPTO_C, since the case of MBEDTLS_PSA_CRYPTO_CLIENT without MBEDTLS_PSA_CRYPTO_C is currently untested and not officially supported.

See docs/architecture/psa-migration/psa-legacy-bridges.md (from #7760) for some background.

Definition of done:

  • The relevant library code and the declaration in pk.h are gated by MBEDTLS_PSA_CRYPTO_CLIENT instead of MBEDTLS_USE_PSA_CRYPTO.
  • The unit tests for the function are enabled whenever MBEDTLS_PSA_CRYPTO_C is enabled.
  • This may involve writing additional unit tests if the coverage is deemed insufficient according to the implementer and reviewers.

Related, but should in principle be independent: #8712

@gilles-peskine-arm
Copy link
Contributor Author

@mpg notes that if we do that, all.sh test_psa_crypto_client won't work anymore, since we'll be missing some functions at link time. The only thing we'd be able to do with CLIENT && !C would be make lib.

@mpg
Copy link
Contributor

mpg commented Feb 8, 2024

I'm also suggesting we grep the generated libmbedtls.a in order to ensure mbedtls_pk_setup_opaque() is indeed present.

@gilles-peskine-arm
Copy link
Contributor Author

Idea to be explored: we prepare and test two client-only scenarios:

  • MBEDTLS_PSA_CRYPTO_CLIENT && !MBEDTLS_PSA_CRYPTO_C && !MBEDTLS_USE_PSA_CRYPTO → can link everything, but some legacy-psa bridge functions are not available. Note that “everything” is default config minus LMS only, i.e. what we're currently testing in component_test_psa_crypto_client. Other stuff (especially LMS and TLS 1.3) need PSA functions.
  • MBEDTLS_PSA_CRYPTO_CLIENT && !MBEDTLS_PSA_CRYPTO_C && MBEDTLS_USE_PSA_CRYPTO → all legacy-psa bridge functions are available, but can't link due to missing implementation of some PSA functions. We try compiling with everything (full config, minus ad hoc exceptions if needed), and check that certain functions are present in the build.

@valeriosetti
Copy link
Contributor

valeriosetti commented Feb 12, 2024

Idea to be explored: we prepare and test two client-only scenarios:

Since I need the same test case also for #8709 (PR #8774) I'm planning to add this testing support there since it's already ongoing. This issue will be started on top of that for simplicity.

Very late edit: as suggested by @mpg I'll implement test components here instead of #8774 in order to keep that PR simpler (it already includes a good amount of changes...)

@valeriosetti
Copy link
Contributor

#8797 seems to implement some changes to check_config.h that are interesting for the test components proposed above. I think that PR will be a prerequisite for the one aimed at solving this issue.

@gilles-peskine-arm
Copy link
Contributor Author

@gilles-peskine-arm gilles-peskine-arm closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2024
@gilles-peskine-arm gilles-peskine-arm removed this from [3.6] Legacy-to-PSA migration helpers in EPICs for Mbed TLS Mar 12, 2024
@gilles-peskine-arm gilles-peskine-arm removed the priority-very-high Highest priority - prioritise this over other review work label Mar 12, 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
None yet
Development

Successfully merging a pull request may close this issue.

4 participants