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 ECDH: enable ssl-opt.sh with parity #7148

Closed
mpg opened this issue Feb 22, 2023 · 1 comment · Fixed by #7243
Closed

driver-only ECDH: enable ssl-opt.sh with parity #7148

mpg opened this issue Feb 22, 2023 · 1 comment · Fixed by #7243
Assignees
Labels
enhancement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Feb 22, 2023

Context: #6839.

This task is to progress TL-ecdh.TLS by running ssl-opt.sh in component_test_psa_crypto_config_accel_ecdh_use_psa() (and the companion reference component) and getting testing parity.

This will likely require updating existing dependency declarations in ssl-opt.sh and adding missing declarations.

Implementation note: Please do this in two separate PRs in order to facilitate review, see comment below.

This is the ECDH equivalent of #6861, but hopefully it will be simpler as ECDH is used if and only if:

  • A TLS 1.2 key exchange with ECDH(E) in its name is use -> then the correct dependency is either requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_ECDH (variable to be create) or requires_config_enabled MBEDTLS_KEY_EXCHANGE_xxx depending on whether we expect any ECDH-based key exchange or a specific one. (The later will often be auto-detected based on force_cipersuite in the command line, in which case no explicit requirement is needed.)
  • A TLS 1.3 key exchange mode other than pure-PSK is used (that is, ephemeral or psk-ephemeral) -> then the correct dependency is requires_pk_alg "ECDH" (to be created to mean that either MBEDTLS_ECDH_C or PSA_WANT_ALG_ECDH is defined).

(That is, we don't have the special case "TLS 1.2 client authentication depending on what certificate is loaded" that we had with ECDSA.)

If dependencies have to be adapted in the code, since ECDH is only used by TLS, we should create macros like (after checking if they don't exist yet):

  • MBEDTLS_KEY_EXCHANGE_WITH_ECDH_ENABLED if any TLS 1.2 ECDH-based key exchange is enabled
  • MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_EPHEMERAL_ENABLED for TLS 1.3
  • MBEDTLS_SSL_SOME_ECDH_ENABLED if something's needed for both?

Note: since this modifies dependency declarations, we'll want to use docs/architecture/psa-migration/outcome-analysis.sh on the PR that implements this.

Note: enabling more key exchanges in config_psa_crypto_config_ecdh_use_psa is out of scope here, it will be a separate task.

Depends on: #7142

@mpg mpg added enhancement size-s Estimated task size: small (~2d) size-m Estimated task size: medium (~1w) and removed size-s Estimated task size: small (~2d) labels Feb 22, 2023
@mpg
Copy link
Contributor Author

mpg commented Feb 22, 2023

Note: this can be executed in two steps (either one PR par step, or one PR for both steps if each step is small enough):

  1. get ssl-opt.sh to just pass in the driver component (this mostly requires adding missing declarations) - without parity (add exceptions in analyze_outcomes.py if this is a separate PR);
  2. get it to parity with the reference component (this mostly requires updating existing declarations to make them more fine-grained)

Edit: after doing a trial run, I think step 1 is significant enough that the two steps should be done in separate PRs. I think this should ease review, as step 1 will mostly be about adding missing dependency declarations, and after that step 2 should be about refining them. I find things easier to review when the two are not intermixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size-m Estimated task size: medium (~1w)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants