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

Declare p256-m as ready for production #8203

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

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

I have reviewed the p256-m implementation and integration. The conclusion of my review is:

PR checklist

Note: Will need rebasing if #8041 gets merged first, see #8203 (comment)

Add some guidance as to whether and how to enable it.

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 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 priority-very-high Highest priority - prioritise this over other review work size-xs Estimated task size: extra small (a few hours at most) labels Sep 13, 2023
@gilles-peskine-arm
Copy link
Contributor Author

@d3zd3z Please review to indicate your approval of p256-m as a whole.

(You need extra steps if you want to disable the built-in implementation of ECC algorithms, which includes more features than p256-m. Refer to the documentation of `MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED` for more information.)

The driver prefix for p256-m is `P256`/`p256`.
The p256-m driver implements four entry points: `generate_key`, `key_agreement`, `sign_hash`, `verify_hash`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: #8041 adds the key management entry points. Depending on which one (of this PR and 8041) gets merged first, the other one will need updating.

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 mpg removed the needs-reviewer This PR needs someone to pick it up for review label Sep 14, 2023
@mpg mpg mentioned this pull request Sep 18, 2023
3 tasks
@gilles-peskine-arm gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Sep 20, 2023
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Sep 20, 2023
@gilles-peskine-arm
Copy link
Contributor Author

@d3zd3z indicated that he's happy with the p256-m code in Slack. In the interest of progressing with the productization (including the follow-up #8202), I'm merging this pull request without waiting for David to confirm on GitHub.

Merged via the queue into Mbed-TLS:development with commit 452beb9 Sep 20, 2023
mpg added a commit to mpg/mbedtls that referenced this pull request Sep 22, 2023
There was a bit of a race condition between Mbed-TLS#8041 which introduced the
new entry points, and Mbed-TLS#8203 which documented the list of entry points.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to mpg/mbedtls that referenced this pull request Sep 24, 2023
There was a bit of a race condition between Mbed-TLS#8041 which introduced the
new entry points, and Mbed-TLS#8203 which documented the list of entry points.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to mpg/mbedtls that referenced this pull request Sep 24, 2023
There was a bit of a race condition between Mbed-TLS#8041 which introduced the
new entry points, and Mbed-TLS#8203 which documented the list of entry points.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@d3zd3z
Copy link
Contributor

d3zd3z commented Sep 25, 2023

I approve of the p256-m code. This is probably the cleanest and most readable crypto code I've encountered.

mpg added a commit to mpg/mbedtls that referenced this pull request Sep 25, 2023
There was a bit of a race condition between Mbed-TLS#8041 which introduced the
new entry points, and Mbed-TLS#8203 which documented the list of entry points.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to mpg/mbedtls that referenced this pull request Sep 26, 2023
There was a bit of a race condition between Mbed-TLS#8041 which introduced the
new entry points, and Mbed-TLS#8203 which documented the list of entry points.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to mpg/mbedtls that referenced this pull request Sep 27, 2023
There was a bit of a race condition between Mbed-TLS#8041 which introduced the
new entry points, and Mbed-TLS#8203 which documented the list of entry points.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to mpg/mbedtls that referenced this pull request Sep 28, 2023
There was a bit of a race condition between Mbed-TLS#8041 which introduced the
new entry points, and Mbed-TLS#8203 which documented the list of entry points.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces priority-very-high Highest priority - prioritise this over other review work size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants