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

Legacy-to-PSA transition guide #7766

Merged
merged 35 commits into from
Dec 20, 2023

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Jun 13, 2023

A new document explains how to migrate application code from the legacy crypto API to the PSA API.

I initially thought this would go in the knowledge base, but we're going to have different versions in the 3.x series and in the 4.x series, so upon reflection it should be versioned. The document mostly focuses on what is in Mbed TLS today, but includes a few notes about upcoming changes.

As I was writing this document, some gaps between the two APIs became evident. I created a few issues for these gaps, with a link in the document.

Reviewers

The top-level sections are largely independent, so we are splitting the review load. For each section, we will have at least one person reviewing for clarity and one for correctness.

  • Clarity (including completeness of coverage): is the document comprehensible by people who aren't familiar with the PSA API, and maybe not with the legacy API either? The document isn't meant to be self-contained: readers are supposed to refer to the Mbed TLS reference documentation. The goal of this document is to allow readers to determine which functions to call and how to combine them. Note that this includes a completeness aspect: are all the elements of the legacy API discussed?
  • Correctness (including completeness of purpose): is this document making incorrect claims? Are there major use cases of the legacy API that are not covered but should be?

I recommend that all reviewers read “Introduction” and at least skim “General considerations” to get an idea of the goals of the document.

Sections Length Reviewers for correctness Reviewers for clarity
Introduction S Ronald ✅ @tom-daubney-arm
General considerations S Ronald ✅ @tom-daubney-arm
Summary of API modules S Ronald ✅ @tom-daubney-arm
Compile-time configuration M Ronald ✅ @tom-daubney-arm
Miscellaneous support modules S Ronald ✅ @tom-daubney-arm
Symmetric encryption L @davidhorstmann-arm @Ryan-Everett-arm
Hashes and MAC L @gowthamsk-arm @davidhorstmann-arm
Key derivation S @valeriosetti @tgonzalezorlandoarm
Random generation S @valeriosetti @tgonzalezorlandoarm
Asymmetric cryptography XL Manuel ✅ @tom-daubney-arm

PR checklist

  • changelog no (doc only)
  • backport no (no point in a 2.28 migration guide)
  • tests no (doc only)

Covers most modules, but missing most of ecp, ecdh and dhm.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-reviewer This PR needs someone to pick it up for review size-m Estimated task size: medium (~1w) priority-medium Medium priority - this can be reviewed as time permits labels Jun 13, 2023
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
docs/psa-transition.md Outdated Show resolved Hide resolved
docs/psa-transition.md Outdated Show resolved Hide resolved
docs/psa-transition.md Outdated Show resolved Hide resolved
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
per Mbed-TLS#7439 (comment)
and Mbed-TLS#7774 (comment)

State that EXPORT implies BASIC.

Also fix missing `WANT_` parts.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Also correct some statements about rsa/ecp/pk check functions.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Jun 15, 2023

I have now covered ecp.h and have no further changes planned for this first edition of the document, except for handling review comments as they come.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
It's not pretty.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
mbedtls_mpi_init(&d);
mbedtls_ecp_export(ec, &grp, &d, &Q);
size_t bits;
curve = mbedtls_ecc_group_to_psa(grp.id, &bits);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we did have the ECP→PSA bridge function here, since the beginning. On the other hand the way to extract metadata from the legacy object is ridiculous because we haven't finished 3.0 properly.

@gilles-peskine-arm gilles-peskine-arm added priority-very-high Highest priority - prioritise this over other review work and removed priority-medium Medium priority - this can be reviewed as time permits labels Nov 23, 2023
@gilles-peskine-arm
Copy link
Contributor Author

I'm raising the priority to very-high because the detailed investigation for the legacy-to-PSA API design depends on this and this investigation is overdue.

@mpg mpg self-requested a review November 23, 2023 09:13
@daverodgman daverodgman self-requested a review November 27, 2023 16:19
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

Regarding the "Compile-time configuration" section, only two propositions that make things more explicit to me.

docs/psa-transition.md Show resolved Hide resolved
docs/psa-transition.md Show resolved Hide resolved
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

One comment about the "Miscellaneous support modules" section.

docs/psa-transition.md Outdated Show resolved Hide resolved
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@mpg
Copy link
Contributor

mpg commented Dec 13, 2023

For the record, I'm happy with my section except for the part about static (EC)DH. Since it looks like this is unlikely to reveal any new gaps in existing APIs (in my experience, doing static (EC)DH is actually easier with PSA as there are fewer differences between static and ephemeral, it's really the legacy API that makes things awkward, as usual), I'd be happy to leave that to a follow-up as long as it's tracked.

@davidhorstmann-arm
Copy link
Contributor

I hereby note for the avoidance of doubt that I'm happy with the 'Hashes and MAC' section and therefore have ticked it.

Copy link
Contributor

@tom-daubney-arm tom-daubney-arm left a comment

Choose a reason for hiding this comment

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

My sections LGTM, thanks.

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM for the sections I've reviewed.

@davidhorstmann-arm davidhorstmann-arm dismissed their stale review December 18, 2023 16:38

Happy with this section, one more to review

Copy link
Contributor

@tgonzalezorlandoarm tgonzalezorlandoarm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gowthamsk-arm gowthamsk-arm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

I have finished going through symmetric encryption.

LGTM, thanks!

@gilles-peskine-arm
Copy link
Contributor Author

@mpg I've filed an issue for static ECDH. Is the document ok to merge now as far as you're concerned?

@mpg mpg self-requested a review December 20, 2023 10:16
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 for asymmetric crypto.

@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 Dec 20, 2023
@mpg mpg enabled auto-merge December 20, 2023 10:22
@mpg mpg added this pull request to the merge queue Dec 20, 2023
Merged via the queue into Mbed-TLS:development with commit 9934f83 Dec 20, 2023
6 checks passed
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 component-crypto Crypto primitives and low-level interfaces priority-very-high Highest priority - prioritise this over other review work size-m Estimated task size: medium (~1w)
Projects
Status: [3.6] Legacy-to-PSA migration helpers
Development

Successfully merging this pull request may close these issues.

10 participants