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: Add Unit Tests for secp224k1 #7425

Conversation

minosgalanakis
Copy link
Contributor

@minosgalanakis minosgalanakis commented Apr 11, 2023

Description

Resolves #7257

Gatekeeper checklist

  • changelognot required. Extending testing coverage, does not affect library's behaviour
  • backport not required. This is worked required by a newly developped interface
  • tests provided

@minosgalanakis minosgalanakis force-pushed the ecp/7257_ecp_mod_p224k1_add_test_cases branch from 397b9ce to 761104d Compare April 11, 2023 16:30
@minosgalanakis minosgalanakis added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) component-test Test framework and CI scripts labels Apr 11, 2023
@minosgalanakis minosgalanakis force-pushed the ecp/7257_ecp_mod_p224k1_add_test_cases branch 2 times, most recently from d779def to 1229efb Compare April 12, 2023 13:13
@minosgalanakis minosgalanakis removed the needs-preceding-pr Requires another PR to be merged first label Apr 12, 2023
@minosgalanakis minosgalanakis force-pushed the ecp/7257_ecp_mod_p224k1_add_test_cases branch 3 times, most recently from a18fe9b to 3346e0f Compare April 13, 2023 16:12
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-arm left a comment

Choose a reason for hiding this comment

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

The changes for the P192K1 testing is not part of this PR and must be removed.


TEST_EQUAL(mbedtls_ecp_mod_p224k1(&X), 0);
TEST_LE_U(mbedtls_mpi_core_bitlen(X.p, X.n), 224);
mbedtls_mpi_mod_raw_fix_quasi_reduction(X.p, &m);
Copy link
Contributor

Choose a reason for hiding this comment

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

As the description of this issue recommend the mbedtls_mpi_mod_mpi() should be used here.

@yanesca
Copy link
Contributor

yanesca commented Apr 17, 2023

@minosgalanakis this is a deliverable for this quarter, it should have a priority-high label, could you please add it?

@minosgalanakis minosgalanakis added the priority-high High priority - will be reviewed soon label Apr 17, 2023
@minosgalanakis minosgalanakis force-pushed the ecp/7257_ecp_mod_p224k1_add_test_cases branch from 3346e0f to 56ca0e3 Compare April 18, 2023 10:43
@minosgalanakis minosgalanakis added the needs-preceding-pr Requires another PR to be merged first label Apr 18, 2023
@minosgalanakis
Copy link
Contributor Author

I have rebased and addressed the comment. The code is still based on #7424 since it makes resolving conflicts easier and we can test the code as we expect it to come in.

Please only review the last three commits

When the pr for p192k1 is merged, I will rebase it

This patch introduces basic unit-testing for the `ecp_mod_p224k1()`.

The method is exposed through the ecp_invasive interface, and
the standard testing data is being provided by the python framework.

Signed-off-by: Minos Galanakis <[email protected]>
This patch introduces a `MBEDTLS_STATIC_TESTABLE` helper
method which exposes `ecp_mod_p256k1()` to the test-framework

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis minosgalanakis force-pushed the ecp/7257_ecp_mod_p224k1_add_test_cases branch from 56ca0e3 to 357b9e1 Compare April 18, 2023 13:13
@minosgalanakis minosgalanakis removed the needs-preceding-pr Requires another PR to be merged first label Apr 18, 2023
@minosgalanakis minosgalanakis removed the needs-ci Needs to pass CI tests label Apr 19, 2023
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-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
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

@paul-elliott-arm paul-elliott-arm merged commit 4bf08f8 into Mbed-TLS:development Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-test Test framework and CI scripts 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 priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit tests for ecp_mod_p224k1()
4 participants