Skip to content

Backport 3.6: psa_generate_key_custom #9235

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

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Jun 6, 2024

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.
  • Keep the old functions for backward compatibility with Mbed TLS 3.6.0. Document them as deprecated, but don't formally deprecate them because we don't add formal deprecations in an LTS branch.
  • Stop declaring the old functions when building as C++. This is necessary to resolve Flexible array members are not standard C++ #9020.

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

Resolves #9020.

I started with the 3.6 PR because that's the hardest part where we care about backward compatibility. In development, since we're working on a new major version, we can just remove the old function.

PR checklist

@gilles-peskine-arm gilles-peskine-arm added bug needs-ci Needs to pass CI tests component-psa PSA keystore/dispatch layer (storage, drivers, …) size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jun 6, 2024
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa_generate_key_custom-3.6 branch from 452ded4 to 4afef10 Compare June 7, 2024 06:41
@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 labels Jun 7, 2024
@paul-elliott-arm paul-elliott-arm self-requested a review June 7, 2024 08:37
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

Initial pass

@paul-elliott-arm paul-elliott-arm removed their request for review June 20, 2024 16:54
@valeriosetti valeriosetti self-requested a review July 18, 2024 08:43
@gilles-peskine-arm gilles-peskine-arm removed needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Jul 18, 2024
valeriosetti
valeriosetti previously approved these changes Jul 19, 2024
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.

Thanks for the replies and the fix.
LGTM :)

eleuzi01
eleuzi01 previously approved these changes Jul 30, 2024
Copy link
Contributor

@eleuzi01 eleuzi01 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

valeriosetti
valeriosetti previously approved these changes Jul 30, 2024
@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Jul 30, 2024

ABI-API check failure expected.

The Windows-2013-Release-x64-cmake-retarget failure is unrelated:

[2024-07-29T20:07:22.580Z] 2024-07-29 20:07:18,033 - VS2013 Release x64 Retargeted cmake - ERROR - Cloning into 'C:/builds/workspace/mbed-tls-pr-head_PR-9235-head/worktrees/tmps982v0p8/framework'...
[2024-07-29T20:07:22.580Z] fatal: early EOF
[2024-07-29T20:07:22.580Z] fatal: The remote end hung up unexpectedly
[2024-07-29T20:07:22.580Z] fatal: index-pack failed
[2024-07-29T20:07:22.580Z] error: RPC failed; curl 18 transfer closed with outstanding read data remaining
[2024-07-29T20:07:22.580Z] fatal: clone of 'https://github.com/Mbed-TLS/mbedtls-framework' into submodule path 'C:/builds/workspace/mbed-tls-pr-head_PR-9235-head/worktrees/tmps982v0p8/framework' failed
[2024-07-29T20:07:22.580Z] Failed to clone 'framework'. Retry scheduled
[2024-07-29T20:07:22.580Z] Cloning into 'C:/builds/workspace/mbed-tls-pr-head_PR-9235-head/worktrees/tmps982v0p8/framework'...
[2024-07-29T20:07:22.580Z] fatal: early EOF
[2024-07-29T20:07:22.580Z] fatal: The remote end hung up unexpectedly
[2024-07-29T20:07:22.580Z] fatal: index-pack failed
[2024-07-29T20:07:22.580Z] error: RPC failed; curl 18 transfer closed with outstanding read data remaining
[2024-07-29T20:07:22.580Z] fatal: clone of 'https://github.com/Mbed-TLS/mbedtls-framework' into submodule path 'C:/builds/workspace/mbed-tls-pr-head_PR-9235-head/worktrees/tmps982v0p8/framework' failed
[2024-07-29T20:07:22.580Z] Failed to clone 'framework' a second time, aborting
[2024-07-29T20:07:22.580Z] 
[2024-07-29T20:07:22.580Z] 2024-07-29 20:07:18,035 - VS2013 Release x64 Retargeted cmake - ERROR - Checking out worktree failed, aborting
[2024-07-29T20:07:22.580Z] Traceback (most recent call last):
[2024-07-29T20:07:22.580Z]   File "windows_testing.py", line 222, in get_clean_worktree_for_git_reference
[2024-07-29T20:07:22.580Z]     check=True
[2024-07-29T20:07:22.580Z]   File "C:\Python36\lib\subprocess.py", line 418, in run
[2024-07-29T20:07:22.580Z]     output=stdout, stderr=stderr)
[2024-07-29T20:07:22.580Z] subprocess.CalledProcessError: Command '['git', 'submodule', 'update', '--init']' returned non-zero exit status 1.

@tom-cosgrove-arm tom-cosgrove-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, labels Jul 30, 2024
@tom-cosgrove-arm
Copy link
Contributor

(minor, related to PR title) it's not really a backport, it's normal development, just against the 3.6 branch, not the development branch. There will be a (quite different!) forward port - effectively a different fix - for development

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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa_generate_key_custom-3.6 branch from 4acd8c9 to 14b87f6 Compare August 6, 2024 11:13
@gilles-peskine-arm gilles-peskine-arm added needs-ci Needs to pass CI tests and removed approved Design and code approved - may be waiting for CI or backports labels Aug 6, 2024
@gilles-peskine-arm
Copy link
Contributor Author

Rebased for the all.sh breakdown. The previous version is in https://github.com/gilles-peskine-arm/mbedtls/tree/psa_generate_key_custom-3.6-2

git range-diff origin/mbedtls-3.6 4acd8c91c04e914ddc6d3efd105d873500464bdc 14b87f6318f3aa0e9b02866618cf471bf7934825

@gilles-peskine-arm
Copy link
Contributor Author

@tom-cosgrove-arm Technically, yes, this is the original PR and the development version is a forward port. But once both PR are up, it doesn't make a difference any longer. And for gatekeeping, it's easier if the title convention is uniform.

@gilles-peskine-arm
Copy link
Contributor Author

By the way, framework/scripts/generate_psa_wrappers.py --log stdout was already broken in 3.6.0, so fixing that is out of scope. I'll probably repair it as par of the fix to #9447.

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Aug 6, 2024
@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, labels Aug 6, 2024
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Aug 6, 2024
Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 7ee1a4e Aug 6, 2024
4 of 6 checks passed
@Tachi107
Copy link
Contributor

Tachi107 commented Oct 23, 2024

Hi all, commit 52504f8, which got merged with this pull request, removed (renamed) psa_key_production_parameters_are_default from the ABI. I seem to understand that this was intended, but why? The symbol was exposed in a header file, so was it really "internal" as mentioned in the commit message?

This is currently holding back upgrading to the latest MbedTLS LTS release in Debian.

@gilles-peskine-arm
Copy link
Contributor Author

@Tachi107 psa_key_production_parameters_are_default was not declared in a public header, so it was ambiguously not part of the ABI of 3.6.0.

Our build scripts are not set to hide symbols that are not exposed in public headers because the way to do that depends on the toolchain, and we don't like to maintain toolchain-dependent code. That being said, we have considered adding MBEDTLS_EXPORTED annotations to all declarations of public functions, which could help if you want to declare which symbols are to be exported.

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 bug component-psa PSA keystore/dispatch layer (storage, drivers, …) priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Development

Successfully merging this pull request may close these issues.

5 participants