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

Driver-only ecdsa wrapup #7245

Merged
merged 3 commits into from
Mar 10, 2023
Merged

Conversation

mpg
Copy link
Contributor

@mpg mpg commented Mar 10, 2023

Description

This PR:

Gatekeeper checklist

  • changelog provided - for the entire driver-only ECDSA series
  • backport not required - new feature
  • tests not required for check_config.h and ChangeLog

mpg added 3 commits March 10, 2023 12:37
Having ECDSA in PSA doesn't help if we're not using PSA from TLS 1.2...

Also, move the definition of PSA_HAVE_FULL_ECDSA outside the
MBEDTLS_PSA_CRYPTO_CONFIG guards so that it is available in all cases.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Turns out TLS 1.3 is using the PK layer for signature generation &
verification, and the PK layer is influenced by USE_PSA_CRYPTO.

Also update docs/use-psa-crypto.md accordingly.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg mpg added needs-review Every commit must be reviewed by at least two team members, size-s Estimated task size: small (~2d) labels Mar 10, 2023
@mpg mpg self-assigned this Mar 10, 2023
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have one generic question but the PR looks OK to me

Comment on lines +846 to +850
#if defined(PSA_WANT_ALG_ECDSA) && defined(PSA_WANT_KEY_TYPE_ECC_KEY_PAIR) && \
defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY)
#define PSA_HAVE_FULL_ECDSA 1
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with moving this code forward in the file, but I was wondering: is it possible to have PSA_WANT symbols when MBEDTLS_PSA_CRYPTO_CONFIG is not defined?
I mean, without it we don't include crypto_config.h and also most of the config_psa.h is also skipped (at least the part which automatically sets PSA_WANT symbols).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have PSA_WANT whether MBEDTLS_PSA_CRYPTO_CONFIG is defined or not. The difference that MBEDTLS_PSA_CRYPTO_CONFIG makes is whether PSA_WANT symbols are calculated from MBEDTLS symbols or the other way round.

(I don't know how this applies specifically here.)

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 10, 2023
@mpg
Copy link
Contributor Author

mpg commented Mar 10, 2023

Open CI is fully green, so no need to wait for Internal to finish.

@mpg mpg merged commit 2301a80 into Mbed-TLS:development Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants