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.

Sign up for GitHub

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

Jump to bottom

Add ECP keypair export function #5642

Merged
merged 7 commits into from
Mar 29, 2022
Merged

Add ECP keypair export function #5642

merged 7 commits into from
Mar 29, 2022

Conversation

mprse
Copy link
Contributor

@mprse mprse commented Mar 18, 2022 •
edited by paul-elliott-arm
Loading

Description

Add ECP key pair export function.

Resolves #4838
Resolves #5005

Status

READY

Requires Backporting

NO

Migrations

NO

mprse added 2 commits March 18, 2022 13:52
@mprse
Add implemetation of ECP keypair export function
711d0f5 
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
@mprse
Add test for ECP export
4b30feb 
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
@mprse mprse added the needs-ci Needs to pass CI tests label Mar 18, 2022
@paul-elliott-arm paul-elliott-arm self-requested a review March 18, 2022 14:02
@paul-elliott-arm paul-elliott-arm added size-s Estimated task size: small (~2d) enhancement component-crypto Crypto primitives and low-level interfaces needs-work needs-review Every commit must be reviewed by at least two team members, needs: changelog needs-reviewer This PR needs someone to pick it up for review labels Mar 18, 2022
@mprse
Add ChangeLog file
815af94 
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
@paul-elliott-arm paul-elliott-arm mentioned this pull request Mar 18, 2022
Closed
@mprse
Fix minor issues
a677b5f 
- parameter name in function description
- test_suite_ecp.data: add new line at the end of file

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
mpg
mpg approved these changes Mar 25, 2022
View reviewed changes
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.

Looks pretty good to me. I left two suggestions for improvements, but you can treat them as optional.

library/ecp.c Outdated Show resolved Hide resolved
tests/suites/test_suite_ecp.function Outdated Show resolved Hide resolved
@mpg mpg removed needs-work needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Mar 25, 2022
mpg
mpg requested changes Mar 25, 2022
View reviewed changes
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.

Sorry, missed an issue with the ChangeLog entry.

ChangeLog.d/mbedtls_ecp_export.txt Outdated Show resolved Hide resolved
mprse added 3 commits March 28, 2022 07:22
@mprse
Add function name to changelog
2076cbe 
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
@mprse
Remove parameters validation using ECP_VALIDATE_RET
ab5274b 
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
@mprse
mbedtls_ecp_group_cmp: change names of parameters to more suitable
6a478ef 
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
@mprse mprse requested a review from mpg March 28, 2022 05:42
@mprse mprse removed the needs-work label Mar 28, 2022
mpg
mpg approved these changes Mar 28, 2022
View reviewed changes
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

paul-elliott-arm
paul-elliott-arm approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-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

@mprse mprse added the approved Design and code approved - may be waiting for CI or backports label Mar 29, 2022
@mprse mprse removed the needs-review Every commit must be reviewed by at least two team members, label Mar 29, 2022
@paul-elliott-arm
Copy link
Member

paul-elliott-arm commented Mar 29, 2022 •
edited
Loading

OpenCI declares erroneous ABI fail - seems to be comparing PR Head with the base branch rather than PR Merge with base branch.

pr-merge is also OpenCI fail. Confirmed internal CI merge ran ok.

@paul-elliott-arm paul-elliott-arm merged commit 571f118 into Mbed-TLS:development Mar 29, 2022
This was referenced Dec 22, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-elliott-arm paul-elliott-arm paul-elliott-arm approved these changes

@mpg mpg mpg approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing getters on some fields Members of mbedtls_ecp_keypair should be public or have accessors
3 participants
@mprse @paul-elliott-arm @mpg