Skip to content

Conversation

@gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Sep 21, 2024

A step towards #9630: we no longer test with MBEDTLS_USE_PSA_CRYPTO disabled. It's still possible to disable it explicitly; that will be handled in a follow-up.

Note: this may conflict with the work to migrate depends.py to the PSA configuration (#9292, #9612, #9633, #9634, #9654).

PR checklist

  • changelog provided
  • development PR here
  • framework PR not required
  • 3.6 PR not required because: new stuff
  • 2.28 PR not required because: new stuff
  • tests provided

@gilles-peskine-arm gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Sep 21, 2024
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Oct 4, 2024
5 tasks
@gilles-peskine-arm gilles-peskine-arm changed the title Enable MBEDTLS_USE_PSA_CRYPTO by default Always enable MBEDTLS_USE_PSA_CRYPTO Oct 5, 2024
@gilles-peskine-arm gilles-peskine-arm added size-xs Estimated task size: extra small (a few hours at most) and removed needs-work needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) labels Oct 9, 2024
@gilles-peskine-arm gilles-peskine-arm added needs-ci Needs to pass CI tests and removed needs-preceding-pr Requires another PR to be merged first labels Oct 22, 2024
With PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE disabled, test TLS 1.3 and
USE_PSA TLS 1.2.

With PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE disabled, just test crypto,
because the TLS code needs that to generate ephemeral ECDH keys but this is
not tracked properly (the ephemeral ECDH code is only gated on having ECDH).

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Oct 23, 2024
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks good to me except it doesn't do exactly what it says on the tin: now USE_PSA is enabled by default, but could still be disabled - either explicitly, or just by failing to define it when using a custom config.

Reading the PR's title, I would have expected something like removing USE_PSA from mbedtls_config.h and then defining it unconditionally in config_adjust_legacy_crypto.h for example. Reading the issue's description only re-inforced that impression.

@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Oct 24, 2024

Looks good to me except it doesn't do exactly what it says on the tin: now USE_PSA is enabled by default, but could still be disabled - either explicitly, or just by failing to define it when using a custom config.

That's a good point. This accomplishes the title of #9630: we're no longer building code that's guarded by !defined(MBEDTLS_USE_PSA_CRYPTO). But it doesn't reach the definition of done of #9630, which says “MBEDTLS_USE_PSA_CRYPTO is no longer a selectable option in mbedtls_config.h`”.

I think this is a useful milestone, which has passed the CI and which you've reviewed. After this, we no longer have to worry about the non-use-PSA code paths in test code and test scripts. So I propose to declare this as a step towards #9630, and make a follow-up that fully resolves #9630. After that follow-up, we'll no longer have to worry about accidentally not enabling MBEDTLS_USE_PSA_CRYPTO.

The follow-up (#9727) will need to take care of test scripts that query MBEDTLS_USE_PSA_CRYPTO, since that will no longer be a public option covered by query_compile_time_config. So there'll at least be some code to remove in ssl-opt.sh.

@gilles-peskine-arm gilles-peskine-arm changed the title Always enable MBEDTLS_USE_PSA_CRYPTO Always enable MBEDTLS_USE_PSA_CRYPTO in all.sh Oct 24, 2024
mpg
mpg previously approved these changes Oct 24, 2024
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Approving now, considering #9614 (comment)

@gilles-peskine-arm
Copy link
Contributor Author

Uh, I clearly made some mistake in a rebase. Originally this branch had no unset MBEDTLS_USE_PSA_CRYPTO in components-*.sh, but now there are some. I don't think that the current state is a useful milestone: my intent here was that we would never do a CI run with MBEDTLS_USE_PSA_CRYPTO disabled (except possibly in configurations with only low-level crypto). I'm going to fix that.

@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Oct 24, 2024

So I just accidentally dropped one commit in a rebase. It still applies cleanly so here goes. (But with an amended commit message, because the message didn't work with the changed commit order.)

Remove all.sh components that explicitly disable MBEDTLS_USE_PSA_CRYPTO, and
for which there is another component with MBEDTLS_USE_PSA_CRYPTO enabled
that does the same or more testing.

Signed-off-by: Gilles Peskine <[email protected]>
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

@mpg
Copy link
Contributor

mpg commented Oct 25, 2024

Looking at the outcomes file:

% grep 'Config: !MBEDTLS_USE_PSA_CRYPTO;PASS' outcomes.csv 
Linux-x86_64;component_test_ccm_aes_sha256;test_suite_config.mbedtls_boolean;Config: !MBEDTLS_USE_PSA_CRYPTO;PASS;
Linux-x86_64;component_test_tfm_config_as_is;test_suite_config.mbedtls_boolean;Config: !MBEDTLS_USE_PSA_CRYPTO;PASS;

The first one has nothing that's affected by USE_PSA so it's OK. The 2nd one is out of our control, but FWIW it doesn't seem to have anything that would be affected by USE_PSA either.

Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

LGTM.

(API break check expected to fail as the PR turns USE_PSA_CRYPTO on in the default configuration.)

@gilles-peskine-arm gilles-peskine-arm 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, needs-reviewer This PR needs someone to pick it up for review labels Oct 28, 2024
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Oct 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2024
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Oct 29, 2024
Merged via the queue into Mbed-TLS:development with commit 8ed4d94 Oct 29, 2024
@gilles-peskine-arm
Copy link
Contributor Author

The successor #9727 is ready for review.

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 priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)

Development

Successfully merging this pull request may close these issues.

3 participants