psa_generate_key_custom #9446

gilles-peskine-arm · 2024-08-02T17:14:25Z

Migrate from psa_generate_key_ext() and psa_key_derivation_output_key_ext() to psa_generate_key_custom() and psa_key_derivation_output_key_custom():

Define the new functions.
Change references in the documentation to point to the new functions.
Remove the old functions. This resolves Flexible array members are not standard C++ #9020.

The API now matches the current state of ARM-software/psa-api#194.

Note to reviewers: I made this pull request by rebasing #9235 then adding commits to remove the old functions and fix problems that didn't arise in 3.6. I experimented a bit with writing a neater history where the existing functions and tests are modified rather than having commits to add the new stuff then commits to remove the old stuff. But this ended up with a giant commit that changes a lot of things, which wouldn't be very helpful in the history, and would be more of a pain to review.

If you'd like to rely on git range-diff, it's a bit overwhelmed by the amount of patches that have moved (due to files being moved under tf-psa-crypto, the order in which the files are listed has changed). But you do get something useful with

git range-diff --color-moved --creation-factor=90 mbedtls-3.6..pull/9235 development..pull/9446

PR checklist

valeriosetti

This looks like a faithful forward-port of #9235 + the removal of "old" _key_ext stuff.
I would just ask @tom-cosgrove-arm to check the 2 last commits as they modify the Perl scripts and corresponding generated code for psasim. They look good to me, but having a second check might help :)

There seems to be a conflict with all.sh (due to #8226 I guess), but it should be easy to fix.

Implement `psa_generate_key_custom()` and `psa_key_derivation_output_key_custom()`. These functions replace `psa_generate_key_ext()` and `psa_key_derivation_output_key_ext()`. They have the same functionality, but a slightly different interface: the `ext` functions use a structure with a flexible array member to pass variable-length data, while the `custom` functions use a separate parameter. Keep the `ext` functions for backward compatibility with Mbed TLS 3.6.0. But make them a thin wrapper around the new `custom` functions. Duplicate the test code and data. The test cases have to be duplicated anyway, and the test functions are individually more readable this way. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Replace references to the deprecated functions `psa_generate_key_ext()` and `psa_key_derivation_output_key_ext()` by their replacements Implement `psa_generate_key_custom()` and `psa_key_derivation_output_key_custom()`. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Don't formally deprecate them because we don't do that in a long-time support branch. But do point readers away from them. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

We know it's a thin wrapper around psa_generate_key_custom, so we just need to check that it's passing the information through, we don't need coverage of the parameter interpretation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Document psa_generate_key_ext() and psa_key_derivation_output_key_ext() as deprecated in favor of psa_generate_key_custom() and psa_key_derivation_output_key_custom(), and no longer declared in C++ builds. Resolves Mbed-TLS#9020. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

In public headers, we want to avoid things that are not standard C++, including features that GCC and Clang support as extensions, such as flexible array members. So compile with `-pedantic`. Non-regression for Mbed-TLS#9020. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Remove the experimental functions psa_generate_key_ext() and psa_key_derivation_output_key_ext(), which require a flexible array member and therefore break C++ code that includes Mbed TLS headers. They have been replaced by psa_generate_key_custom() and psa_key_derivation_output_key_custom(). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2024-08-05T13:54:32Z

I've rebased on top of the latest development. The previous version is in gilles-peskine-arm:psa_generate_key_custom-development-forward_then_remove-3. The changes to all.sh components are now in the corresponding components file.

git range-diff origin/development e92e117af8add2ca8d651802d04c442930bdb59d origin/pull/9446

valeriosetti

Thanks for the update. LGTM now :)

eleuzi01

LGTM

gilles-peskine-arm · 2024-08-05T16:40:56Z

Gatekeeper note — the 3.6 backport is waiting on #9444.

tom-cosgrove-arm

I would just ask @tom-cosgrove-arm to check the 2 last commits as they modify the Perl scripts and corresponding generated code for psasim. They look good to me, but having a second check might help :)

They look good to me too - thanks

tom-cosgrove-arm · 2024-09-11T15:17:17Z

tests/scripts/components-build-system.sh

@@ -65,7 +65,7 @@ component_test_cmake_out_of_source () {
    mkdir "$OUT_OF_SOURCE_DIR"
    cd "$OUT_OF_SOURCE_DIR"
    # Note: Explicitly generate files as these are turned off in releases
-    cmake -D CMAKE_BUILD_TYPE:String=Check -D GEN_FILES=ON "$MBEDTLS_ROOT_DIR"
+    cmake -D CMAKE_BUILD_TYPE:String=Check -D GEN_FILES=ON _D TEST_CPP=1 "$MBEDTLS_ROOT_DIR"


@gilles-peskine-arm Is this meant to be _D before TEST_CPP=1

Oops, clearly a typo for -D (I do have _ on Shift+- on my keyboard). I wonder what cmake is doing there.

#9561

gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, component-psa PSA keystore/dispatch layer (storage, drivers, …) size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Aug 2, 2024

gilles-peskine-arm mentioned this pull request Aug 2, 2024

Backport 3.6: psa_generate_key_custom #9235

Merged

6 tasks

gilles-peskine-arm requested review from valeriosetti and eleuzi01 August 2, 2024 17:14

valeriosetti previously approved these changes Aug 5, 2024

View reviewed changes

gilles-peskine-arm added 16 commits August 5, 2024 15:49

Update PSA wrappers

b45af2f

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Document the key_ext functions as deprecated

8ae5028

Don't formally deprecate them because we don't do that in a long-time support branch. But do point readers away from them. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Remove some tests of psa_generate_key_ext

ae1da98

We know it's a thin wrapper around psa_generate_key_custom, so we just need to check that it's passing the information through, we don't need coverage of the parameter interpretation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

key_custom: update analyze_outcomes.py

1084e8e

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Rename internal function psa_key_production_parameters_are_default

ae8fa71

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Documentation improvements

6b2709b

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Rename one more deprecated identifier

63d06ef

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Match spacing in pointer types in documentation with the code style

d5d69bb

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Follow include path updates for C++ builds

6b3bca5

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Migrate psasim wrappers to psa_custom_key_parameters_t

fd46f7f

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Update generated PSA wrappers

7db1bcd

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm dismissed valeriosetti’s stale review via 7db1bcd August 5, 2024 13:52

gilles-peskine-arm force-pushed the psa_generate_key_custom-development-forward_then_remove branch from e92e117 to 7db1bcd Compare August 5, 2024 13:52

valeriosetti approved these changes Aug 5, 2024

View reviewed changes

eleuzi01 approved these changes Aug 5, 2024

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 5, 2024

tom-cosgrove-arm approved these changes Aug 6, 2024

View reviewed changes

gilles-peskine-arm added this pull request to the merge queue Aug 6, 2024

Merged via the queue into Mbed-TLS:development with commit bac7c96 Aug 6, 2024
4 of 6 checks passed

tom-cosgrove-arm reviewed Sep 11, 2024

View reviewed changes

gilles-peskine-arm mentioned this pull request Sep 11, 2024

Fix typo that caused cpp_dummy_build not to be built with CMake #9561

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

psa_generate_key_custom #9446

psa_generate_key_custom #9446

gilles-peskine-arm commented Aug 2, 2024

valeriosetti left a comment

gilles-peskine-arm commented Aug 5, 2024

valeriosetti left a comment

eleuzi01 left a comment

gilles-peskine-arm commented Aug 5, 2024

tom-cosgrove-arm left a comment

tom-cosgrove-arm Sep 11, 2024

gilles-peskine-arm Sep 11, 2024

psa_generate_key_custom #9446

psa_generate_key_custom #9446

Conversation

gilles-peskine-arm commented Aug 2, 2024

PR checklist

valeriosetti left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 5, 2024

valeriosetti left a comment

Choose a reason for hiding this comment

eleuzi01 left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 5, 2024

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm Sep 11, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Sep 11, 2024

Choose a reason for hiding this comment