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

ECP keypair utility functions #7815

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

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

Add utility functions to the ECP module to query and construct key pairs and points. This replaces the direct access to mbedtls_ecp_keypair and mbedtls_ecp_point fields that was possible in Mbed TLS 2.x. Fixes #5441, fixes #8367, fixes #8652.

Update the sample programs. They now no longer do any direct access to private fields of mbedtls_ecp_keypair or mbedtls_ecp_point. Fixes #5017. Note that any unrelated improvement of sample programs or their testing is out of scope here. I used the following to test them:

  • run programs/pkey/ecdsa
  • run smoke.sh secp256r1 && smoke.sh x25519 using smoke.sh.txt

Follow-up: use the new functions to simplify the PSA transition guide.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog provided
  • backport no (new feature)
  • tests provided

@gilles-peskine-arm gilles-peskine-arm added needs-work needs-design-approval component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Jun 21, 2023
@mpg mpg self-requested a review June 22, 2023 07:27
@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-work needs-ci Needs to pass CI tests labels Jun 26, 2023
@tom-cosgrove-arm
Copy link
Contributor

This increases code size by 200 bytes when compiled for TF-M (their config, P-256 only, no programs/ used) with ArmClang 6.19 with -Oz

orig:5647d06be pr:7815:1c8f9d61d Delta Filename
6464, 0 6664, 0 +200+0 ecp.o

@minosgalanakis minosgalanakis added priority-high High priority - will be reviewed soon and removed priority-medium Medium priority - this can be reviewed as time permits labels Nov 13, 2023
@daverodgman daverodgman self-requested a review November 27, 2023 15:15
@daverodgman daverodgman mentioned this pull request Nov 27, 2023
9 tasks
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.

Only a design review for now. I'm mostly happy except for export_coordinates() where I disagree with the current API and am not convinced we really need it at all (but not opposed to adding it either).

include/mbedtls/ecp.h Outdated Show resolved Hide resolved
include/mbedtls/ecp.h Outdated Show resolved Hide resolved
include/mbedtls/ecp.h Outdated Show resolved Hide resolved
ChangeLog.d/ecp-keypair-utilities.txt Show resolved Hide resolved
include/mbedtls/ecp.h Show resolved Hide resolved
tests/suites/test_suite_ecp.function Show resolved Hide resolved
Add a simple function to get the group id from a key object.

This information is available via mbedtls_ecp_export, but that function
consumes a lot of memory, which is a waste if all you need is to identify
the curve.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Sometimes you don't need to have all the parts of a key pair object. Relax
the behavior of mbedtls_ecp_keypair so that you can extract just the parts
that you need.

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

I've pushed a new history that never creates mbedtls_ecp_point_export_coordinates. The actual development history, with that function added and removed and with some commits to fix minor issues based on CI or review feedback, but rebased midway on top of a recent development to avoid merge conflicts, is in https://github.com/gilles-peskine-arm/mbedtls/tree/ecp-export-partial-4.

@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 Dec 24, 2023
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.

Looking pretty good, just a few minor things and a couple of questions.

include/mbedtls/ecp.h Outdated Show resolved Hide resolved
programs/pkey/gen_key.c Show resolved Hide resolved
include/mbedtls/ecp.h Show resolved Hide resolved
programs/pkey/gen_key.c Show resolved Hide resolved
tests/suites/test_suite_ecp.data Show resolved Hide resolved
tests/suites/test_suite_ecp.function Outdated Show resolved Hide resolved
tests/suites/test_suite_ecp.function Outdated Show resolved Hide resolved
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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.

Looking good, apart from the superfluous BIGNUM_C guards that I'd like removed.

@mpg
Copy link
Contributor

mpg commented Jan 3, 2024

I'm removing "needs-design-approval" as I think we know have an agreement on the design.

All of ECP requires the bignum module and there is no plan to change that,
so guarding a few bits of code is just noise.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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

@minosgalanakis minosgalanakis requested review from minosgalanakis and removed request for daverodgman January 16, 2024 10:46
Copy link
Contributor

@minosgalanakis minosgalanakis 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 overall.

Two notes:

  • I assume that the setter for public key mbedtls_ecp_read_public_key is intentionally missing?

    If a user needs to importa public key they should import the private one using mbedtls_ecp_read_key () and then calculate the public using mbedtls_ecp_keypair_calc_public() ?

  • The show_ecp_key(const mbedtls_ecp_keypair *ecp, int has_private) method could be either a helper function for programs or even an API call? Given the complexity to produce a readable printout, we should at least point the users to how to do it.

The latter is not a strong ask, and should not block this PR.

@gilles-peskine-arm
Copy link
Contributor Author

I assume that the setter for public key mbedtls_ecp_read_public_key is intentionally missing?

I'm not sure what that would be. How would it differ from mbedtls_ecp_set_public_key?

The show_ecp_key(const mbedtls_ecp_keypair *ecp, int has_private) method could be either a helper function for programs

It could be, but those programs are currently self-contained. They're sample programs, so there's a strong argument that people should be able to just take the one .c file and modify it. That's a high barrier for introducing a new dependency. Conversely, this is code that we rarely modify, so the cost of duplication is low. So this is a rare case where I think code duplication is ok, even if we didn't have time constraints.

or even an API call? Given the complexity to produce a readable printout, we should at least point the users to how to do it.

Printing out keys in a readable format is rarely done apart from demos. So it doesn't belong in the API, and I don't think we need to call it out in documentation.

Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@minosgalanakis minosgalanakis 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 Jan 17, 2024
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Jan 18, 2024
Merged via the queue into Mbed-TLS:development with commit b1f96c0 Jan 18, 2024
4 of 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-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Status: [3.6] Mbed TLS PRIVATE
5 participants