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

Add unit tests for ecp_mod_p192k1() #7424

Merged

Conversation

gabor-mezei-arm
Copy link
Contributor

Description

The task is to add unit tests for ecp_mod_p192k1(). This function reduces the input modulo secp192k1_p.

Resolve #7256

Gatekeeper checklist

  • changelog not required
  • backport not required
  • tests provided

@gabor-mezei-arm gabor-mezei-arm added enhancement component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) labels Apr 11, 2023
@gabor-mezei-arm gabor-mezei-arm self-assigned this Apr 11, 2023
@minosgalanakis minosgalanakis self-requested a review April 11, 2023 15:25
@gabor-mezei-arm gabor-mezei-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-ci Needs to pass CI tests labels Apr 12, 2023
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 and functionality works as intended. Minor remarks, which should not warrant a rework, considering we will be refactoring those methods in the coming weeks.

I will approve is second reviewer is happy with the state as is.

size_t limbs = limbs_N;
size_t bytes = limbs * sizeof(mbedtls_mpi_uint);

TEST_EQUAL(limbs_X, 2 * limbs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should that be TEST_LE_U given that the issue requirements are:

Only cases where the input is non-negative and at most twice as long as the modulus.

TEST_EQUAL(mbedtls_test_read_mpi(&N, input_N), 0);
TEST_EQUAL(mbedtls_test_read_mpi(&res, result), 0);

limbs_X = X.n;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are using old mpi's I see no reason for intermediate assignments to limbs_X, limbs_N, as they can be directly be acccessed by X.n, N.n

@gabor-mezei-arm gabor-mezei-arm force-pushed the 7256_unit_tests_for_p192k1 branch from ea8a2ed to 0a11ee6 Compare April 13, 2023 11:08
@gabor-mezei-arm
Copy link
Contributor Author

Rebased

@gabor-mezei-arm gabor-mezei-arm added needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 13, 2023
minosgalanakis
minosgalanakis previously approved these changes Apr 13, 2023
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 ( provided it passes internal CI )

The failure in the OpenCI appears to be caused by other issues

[2023-04-13T12:21:44.102Z] stdout: 

[2023-04-13T12:21:44.102Z] stderr: fatal: unable to access 'https://github.com/Mbed-TLS/mbedtls.git/': Failed to connect to github.com port 443: Connection timed out

[2023-04-13T12:21:44.102Z] 

[2023-04-13T12:21:44.102Z] 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:2734)

@gabor-mezei-arm gabor-mezei-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Apr 14, 2023
Signed-off-by: Gabor Mezei <[email protected]>
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 the priority-high High priority - will be reviewed soon label Apr 18, 2023
@yanesca
Copy link
Contributor

yanesca commented Apr 18, 2023

Just a reminder: all PRs related to this quarter's bignum work should have the priority-high label and added to the unified board.

Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

LGTM

@yanesca yanesca merged commit 3c3b94a into Mbed-TLS:development Apr 18, 2023
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 enhancement 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_p192k1()
3 participants